Improve MCP server

apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts

@@ -2,6 +2,8 @@ import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-
 import { ImportService } from '@ghostfolio/api/app/import/import.service';
 import { UserService } from '@ghostfolio/api/app/user/user.service';
 import { REQUIRES_SCOPE_KEY } from '@ghostfolio/api/decorators/requires-scope.decorator';
+import { McpToolExceptionFilter } from '@ghostfolio/api/filters/mcp-tool-exception.filter';
+import { AccessGuard } from '@ghostfolio/api/guards/access.guard';
 import { ConfigurationService } from '@ghostfolio/api/services/configuration/configuration.service';
 import { MCP_MAX_ACTIVITIES } from '@ghostfolio/common/config';
 import { Activity } from '@ghostfolio/common/interfaces';
@@ -13,6 +15,10 @@ import type {
 } from '@ghostfolio/common/types';
 
 import { HttpException } from '@nestjs/common';
+import {
+  EXCEPTION_FILTERS_METADATA,
+  GUARDS_METADATA
+} from '@nestjs/common/constants';
 import { DataSource, Type as ActivityType } from '@prisma/client';
 import { MCP_TOOL_METADATA_KEY, ToolMetadata } from '@rekog/mcp-nest';
 
@@ -133,6 +139,34 @@ describe('GhostfolioMcpController', () => {
 
       expect(toolMethodNamesWithoutScope).toEqual([]);
     });
+
+    // The decorator RequiresScope sets the same metadata as the decorator
+    // RequiresScopeOfAccess, but applies AuthGuard('jwt'), which a request of
+    // an access cannot pass, hence the guards tell the two decorators apart
+    it('Applies the guard of the access to each tool', () => {
+      const toolMethodNames = getToolMethodNames();
+
+      expect(toolMethodNames.length).toBeGreaterThan(0);
+
+      const toolMethodNamesWithoutGuardOfAccess = toolMethodNames.filter(
+        (methodName) => {
+          return !getMetadataOfMethod<unknown[]>(
+            GUARDS_METADATA,
+            methodName
+          )?.includes(AccessGuard);
+        }
+      );
+
+      expect(toolMethodNamesWithoutGuardOfAccess).toEqual([]);
+    });
+
+    // The tools have no try and catch, hence the filter is the only guarantee
+    // that an unexpected exception does not expose internals
+    it('Applies the filter of the exceptions of the tools', () => {
+      expect(
+        Reflect.getMetadata(EXCEPTION_FILTERS_METADATA, GhostfolioMcpController)
+      ).toEqual([McpToolExceptionFilter]);
+    });
   });
 
   describe('Import activities', () => {

apps/api/src/app/endpoints/mcp/mcp.controller.ts

@@ -202,7 +202,7 @@ export class GhostfolioMcpController {
       };
     });
 
-    // The filter passes on the message of an ImportValidationError, which is
+    // The filter passes on the message of a CallerFacingError, which is
     // written for the caller, and hides the message of every other error
     const importedActivities = await this.importService.import({
       activitiesDto,

apps/api/src/app/endpoints/mcp/mcp.module.ts

@@ -21,6 +21,7 @@ import { McpService } from './mcp.service';
   controllers: [GhostfolioMcpController],
   imports: [AiModule, ApiModule, ConfigurationModule, ImportModule, UserModule],
   providers: [
+    McpService,
     {
       inject: [ConfigurationService],
       provide: MCP_STRATEGY,
@@ -45,8 +46,7 @@ import { McpService } from './mcp.service';
           websiteUrl: 'https://ghostfol.io'
         });
       }
-    },
-    McpService
+    }
   ]
 })
 export class McpModule {}

apps/api/src/filters/mcp-tool-exception.filter.spec.ts

@@ -2,7 +2,6 @@ import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-
 import { PortfolioSnapshotComputationError } from '@ghostfolio/api/app/portfolio/errors/portfolio-snapshot-computation.error';
 
 import { ForbiddenException, Logger } from '@nestjs/common';
-import { RpcException } from '@nestjs/microservices';
 import { getReasonPhrase, StatusCodes } from 'http-status-codes';
 import { firstValueFrom } from 'rxjs';
 
@@ -18,6 +17,8 @@ describe('McpToolExceptionFilter', () => {
     } catch (error) {
       return error;
     }
+
+    throw new Error('The filter gave no error');
   }
 
   beforeEach(() => {
@@ -43,16 +44,6 @@ describe('McpToolExceptionFilter', () => {
     expect(logError).not.toHaveBeenCalled();
   });
 
-  it('Passes on the error of an RpcException', async () => {
-    const exception = new RpcException('The access cannot be resolved');
-
-    expect(await getErrorOfException(exception)).toBe(
-      'The access cannot be resolved'
-    );
-
-    expect(logError).not.toHaveBeenCalled();
-  });
-
   it('Hides the message of an unexpected error and writes it to the log', async () => {
     const exception = new Error(
       'Unique constraint failed on the fields: (dataSource)'
@@ -66,11 +57,15 @@ describe('McpToolExceptionFilter', () => {
     expect(logError).toHaveBeenCalledWith(exception);
   });
 
-  it('Gives the reason phrase of the status of an HttpException', async () => {
+  // An access without the scope of a tool causes a refused call at each
+  // attempt, which would fill the log
+  it('Gives the reason phrase of the status of an HttpException and writes no log', async () => {
     expect(await getErrorOfException(new ForbiddenException())).toEqual({
       message: getReasonPhrase(StatusCodes.FORBIDDEN),
       status: 'error'
     });
+
+    expect(logError).not.toHaveBeenCalled();
   });
 
   it('Gives the reason phrase of a service which is not available if a snapshot cannot be computed', async () => {
@@ -82,5 +77,7 @@ describe('McpToolExceptionFilter', () => {
       message: getReasonPhrase(StatusCodes.SERVICE_UNAVAILABLE),
       status: 'error'
     });
+
+    expect(logError).toHaveBeenCalledWith(exception);
   });
 });

apps/api/src/filters/mcp-tool-exception.filter.ts

@@ -7,7 +7,6 @@ import {
   Logger,
   RpcExceptionFilter
 } from '@nestjs/common';
-import { RpcException } from '@nestjs/microservices';
 import { getReasonPhrase, StatusCodes } from 'http-status-codes';
 import { Observable, throwError } from 'rxjs';
 
@@ -21,32 +20,32 @@ export class McpToolExceptionFilter implements RpcExceptionFilter {
   private readonly logger = new Logger(McpToolExceptionFilter.name);
 
   public catch(exception: unknown): Observable<never> {
-    // The message of these exceptions is written for the caller, hence it is
+    // The message of this exception is written for the caller, hence it is
     // passed on and is not written to the log
-    if (exception instanceof RpcException) {
-      return throwError(() => {
-        return exception.getError();
-      });
-    }
-
     if (exception instanceof CallerFacingError) {
       return throwError(() => {
         return { message: exception.message, status: 'error' };
       });
     }
 
-    this.logger.error(exception);
+    const statusCode = this.getStatus(exception);
 
-    return throwError(() => {
-      return { message: this.getMessage(exception), status: 'error' };
-    });
-  }
+    // An exception which the caller causes, for example a refused call, is
+    // expected, hence only an exception of the application is written to the
+    // log
+    if (statusCode >= StatusCodes.INTERNAL_SERVER_ERROR) {
+      this.logger.error(exception);
+    }
 
-  private getMessage(exception: unknown) {
     // The message of an exception can carry internals, for example the
     // property names of a data transfer object of a failed validation, hence
     // the reason phrase of the status is passed on instead
-    return this.getReasonPhraseOfStatus(this.getStatus(exception));
+    return throwError(() => {
+      return {
+        message: this.getReasonPhraseOfStatus(statusCode),
+        status: 'error'
+      };
+    });
   }
 
   private getReasonPhraseOfStatus(statusCode: number) {