diff --git a/README.md b/README.md index 7c6f5ee7ad..3397f8c3f2 100644 --- a/README.md +++ b/README.md @@ -366,7 +366,7 @@ The _Model Context Protocol_ (MCP) server lets an AI client read your portfolio - Set `ROOT_URL` to the public URL of your instance if a client calls the endpoint from a browser page. The host name of `ROOT_URL` is the only accepted origin. - Grant an access of the type _MCP_ in _My Ghostfolio_ under _Access_ and copy its identifier -An _MCP_ access has (restricted) read scopes and never reads the monetary values. Grant the _Restricted view and manage_ permission to let the client also import activities. +An _MCP_ access has (restricted) read scopes and never reads the monetary values. Grant the _Restricted view and manage_ permission to let the client also import activities. The client sees the tools of the scopes of the access only. ### Connect a client diff --git a/apps/api/src/app/endpoints/ai/ai.service.spec.ts b/apps/api/src/app/endpoints/ai/ai.service.spec.ts index 6328df1952..8aa2c5ec71 100644 --- a/apps/api/src/app/endpoints/ai/ai.service.spec.ts +++ b/apps/api/src/app/endpoints/ai/ai.service.spec.ts @@ -22,9 +22,12 @@ jest.mock('ai', () => { * can read the columns and the rows which the service gives to it. */ interface AiServiceWithMarkdownTable { - getMarkdownTable(parameters: { - columnDefinitions: readonly { name: string }[]; - rows: Record[]; + getMarkdownTable(parameters: { + columnDefinitions: readonly { + getValue: (item: T) => string; + name: string; + }[]; + items: T[]; }): Promise; } @@ -62,17 +65,17 @@ function createAiService(accounts: AccountWithValue[]) { aiService as unknown as AiServiceWithMarkdownTable, 'getMarkdownTable' ) - .mockImplementation(async ({ columnDefinitions, rows }) => { - const columnNames = columnDefinitions.map(({ name }) => { - return name; - }); - + .mockImplementation(async ({ columnDefinitions, items }) => { return [ - columnNames.join(' | '), - ...rows.map((row) => { - return columnNames - .map((columnName) => { - return row[columnName]; + columnDefinitions + .map(({ name }) => { + return name; + }) + .join(' | '), + ...items.map((item) => { + return columnDefinitions + .map(({ getValue }) => { + return getValue(item); }) .join(' | '); }) @@ -114,6 +117,21 @@ describe('AiService', () => { }); }); + describe('getHoldingsTableColumnNames', () => { + it('gives no column with a monetary value', () => { + expect(AiService.getHoldingsTableColumnNames()).toEqual([ + 'Name', + 'Symbol', + 'Currency', + 'Asset Class', + 'Asset Sub Class', + 'Date of First Activity', + 'Activities Count', + 'Allocation in Percentage' + ]); + }); + }); + describe('getAccountsTable', () => { it('gives no cash balance and no value of an account', async () => { const aiService = createAiService([createAccount()]); diff --git a/apps/api/src/app/endpoints/ai/ai.service.ts b/apps/api/src/app/endpoints/ai/ai.service.ts index 758499c6e6..b93e97d624 100644 --- a/apps/api/src/app/endpoints/ai/ai.service.ts +++ b/apps/api/src/app/endpoints/ai/ai.service.ts @@ -8,8 +8,12 @@ import { PROPERTY_OPENROUTER_MODEL } from '@ghostfolio/common/config'; import { DATE_FORMAT, isAccountExcluded } from '@ghostfolio/common/helper'; -import { Filter } from '@ghostfolio/common/interfaces'; -import type { AiPromptMode } from '@ghostfolio/common/types'; +import { + Activity, + Filter, + PortfolioPosition +} from '@ghostfolio/common/interfaces'; +import type { AccountWithValue, AiPromptMode } from '@ghostfolio/common/types'; import { Injectable } from '@nestjs/common'; import { createOpenRouter } from '@openrouter/ai-sdk-provider'; @@ -22,75 +26,18 @@ import { generateText } from 'ai'; import { format } from 'date-fns'; import type { ColumnDescriptor } from 'tablemark'; +/** + * Describes one column of a table which a tool or a prompt presents. The value + * of a cell is derived from the item of the row, hence a table needs no + * mapping of its own. + */ +type TableColumnDefinition = ColumnDescriptor & { + getValue: (item: T) => string; + name: string; +}; + @Injectable() export class AiService { - private static readonly ACCOUNTS_TABLE_COLUMN_DEFINITIONS: ({ - key: - | 'ACTIVITIES_COUNT' - | 'ALLOCATION_PERCENTAGE' - | 'CURRENCY' - | 'EXCLUDED_FROM_ANALYSIS' - | 'ID' - | 'NAME' - | 'PLATFORM'; - } & ColumnDescriptor)[] = [ - { key: 'ID', name: 'Id' }, - { key: 'NAME', name: 'Name' }, - { key: 'CURRENCY', name: 'Currency' }, - { key: 'PLATFORM', name: 'Platform' }, - { align: 'right', key: 'ACTIVITIES_COUNT', name: 'Activities Count' }, - { - align: 'right', - key: 'ALLOCATION_PERCENTAGE', - name: 'Allocation in Percentage' - }, - { key: 'EXCLUDED_FROM_ANALYSIS', name: 'Excluded from Analysis' } - ]; - - private static readonly ACTIVITIES_TABLE_COLUMN_DEFINITIONS: ({ - key: - | 'ACCOUNT' - | 'CURRENCY' - | 'DATE' - | 'NAME' - | 'SYMBOL' - | 'TYPE' - | 'UNIT_PRICE'; - } & ColumnDescriptor)[] = [ - { key: 'DATE', name: 'Date' }, - { key: 'TYPE', name: 'Type' }, - { key: 'NAME', name: 'Name' }, - { key: 'SYMBOL', name: 'Symbol' }, - { key: 'CURRENCY', name: 'Currency' }, - { align: 'right', key: 'UNIT_PRICE', name: 'Unit Price' }, - { key: 'ACCOUNT', name: 'Account' } - ]; - - private static readonly HOLDINGS_TABLE_COLUMN_DEFINITIONS: ({ - key: - | 'ACTIVITIES_COUNT' - | 'ALLOCATION_PERCENTAGE' - | 'ASSET_CLASS' - | 'ASSET_SUB_CLASS' - | 'CURRENCY' - | 'DATE_OF_FIRST_ACTIVITY' - | 'NAME' - | 'SYMBOL'; - } & ColumnDescriptor)[] = [ - { key: 'NAME', name: 'Name' }, - { key: 'SYMBOL', name: 'Symbol' }, - { key: 'CURRENCY', name: 'Currency' }, - { key: 'ASSET_CLASS', name: 'Asset Class' }, - { key: 'ASSET_SUB_CLASS', name: 'Asset Sub Class' }, - { key: 'DATE_OF_FIRST_ACTIVITY', name: 'Date of First Activity' }, - { align: 'right', key: 'ACTIVITIES_COUNT', name: 'Activities Count' }, - { - align: 'right', - key: 'ALLOCATION_PERCENTAGE', - name: 'Allocation in Percentage' - } - ]; - public constructor( private readonly activitiesService: ActivitiesService, private readonly configurationService: ConfigurationService, @@ -100,23 +47,189 @@ export class AiService { ) {} public static getAccountsTableColumnNames() { - return AiService.ACCOUNTS_TABLE_COLUMN_DEFINITIONS.map(({ name }) => { + return AiService.getAccountsTableColumnDefinitions().map(({ name }) => { return name; }); } public static getActivitiesTableColumnNames() { - return AiService.ACTIVITIES_TABLE_COLUMN_DEFINITIONS.map(({ name }) => { + return AiService.getActivitiesTableColumnDefinitions().map(({ name }) => { return name; }); } public static getHoldingsTableColumnNames() { - return AiService.HOLDINGS_TABLE_COLUMN_DEFINITIONS.map(({ name }) => { + return AiService.getHoldingsTableColumnDefinitions().map(({ name }) => { return name; }); } + private static getAccountsTableColumnDefinitions(): TableColumnDefinition[] { + return [ + { + getValue: ({ id }) => { + return id; + }, + name: 'Id' + }, + { + getValue: ({ name }) => { + return name ?? ''; + }, + name: 'Name' + }, + { + getValue: ({ currency }) => { + return currency ?? ''; + }, + name: 'Currency' + }, + { + getValue: ({ platform }) => { + return platform?.name ?? ''; + }, + name: 'Platform' + }, + { + align: 'right', + getValue: ({ activitiesCount }) => { + return activitiesCount.toString(); + }, + name: 'Activities Count' + }, + { + align: 'right', + getValue: ({ allocationInPercentage }) => { + return `${(allocationInPercentage * 100).toFixed(3)}%`; + }, + name: 'Allocation in Percentage' + }, + { + getValue: ({ tags }) => { + return isAccountExcluded({ tags }).toString(); + }, + name: 'Excluded from Analysis' + } + ]; + } + + private static getActivitiesTableColumnDefinitions(): TableColumnDefinition[] { + return [ + { + getValue: ({ date }) => { + return format(date, DATE_FORMAT); + }, + name: 'Date' + }, + { + getValue: ({ type }) => { + return type; + }, + name: 'Type' + }, + { + getValue: ({ assetProfile }) => { + return assetProfile.name ?? ''; + }, + name: 'Name' + }, + { + getValue: ({ assetProfile }) => { + return assetProfile.symbol; + }, + name: 'Symbol' + }, + { + getValue: ({ assetProfile, currency }) => { + return currency ?? assetProfile.currency; + }, + name: 'Currency' + }, + { + align: 'right', + getValue: ({ unitPrice }) => { + return unitPrice.toString(); + }, + name: 'Unit Price' + }, + { + getValue: ({ account }) => { + return account?.name ?? ''; + }, + name: 'Account' + } + ]; + } + + /** + * The translations of the enumerations depend on the language of the caller, + * hence the definitions are built per request. The names of the columns are + * independent of them, hence a caller which needs the names only passes no + * translation. + */ + private static getHoldingsTableColumnDefinitions({ + assetClassTranslations = {}, + assetSubClassTranslations = {} + }: { + assetClassTranslations?: Record; + assetSubClassTranslations?: Record; + } = {}): TableColumnDefinition[] { + return [ + { + getValue: ({ assetProfile }) => { + return assetProfile.name ?? ''; + }, + name: 'Name' + }, + { + getValue: ({ assetProfile }) => { + return assetProfile.symbol; + }, + name: 'Symbol' + }, + { + getValue: ({ assetProfile }) => { + return assetProfile.currency; + }, + name: 'Currency' + }, + { + getValue: ({ assetProfile }) => { + return assetClassTranslations[assetProfile.assetClass] ?? ''; + }, + name: 'Asset Class' + }, + { + getValue: ({ assetProfile }) => { + return assetSubClassTranslations[assetProfile.assetSubClass] ?? ''; + }, + name: 'Asset Sub Class' + }, + { + getValue: ({ dateOfFirstActivity }) => { + return dateOfFirstActivity + ? format(dateOfFirstActivity, DATE_FORMAT) + : ''; + }, + name: 'Date of First Activity' + }, + { + align: 'right', + getValue: ({ activitiesCount }) => { + return activitiesCount.toString(); + }, + name: 'Activities Count' + }, + { + align: 'right', + getValue: ({ allocationInPercentage }) => { + return `${(allocationInPercentage * 100).toFixed(3)}%`; + }, + name: 'Allocation in Percentage' + } + ]; + } + public async generateText({ prompt, requestTimeout = this.configurationService.get('REQUEST_TIMEOUT') @@ -157,73 +270,12 @@ export class AiService { withExcludedAccounts: true }); - const accountsTableRows = accounts.map( - ({ - activitiesCount, - allocationInPercentage, - currency, - id, - name: label, - platform, - tags - }) => { - return AiService.ACCOUNTS_TABLE_COLUMN_DEFINITIONS.reduce( - (row, { key, name }) => { - switch (key) { - case 'ACTIVITIES_COUNT': - row[name] = activitiesCount.toString(); - break; - - case 'ALLOCATION_PERCENTAGE': - row[name] = `${(allocationInPercentage * 100).toFixed(3)}%`; - break; - - case 'CURRENCY': - row[name] = currency ?? ''; - break; - - case 'EXCLUDED_FROM_ANALYSIS': - row[name] = isAccountExcluded({ tags }).toString(); - break; - - case 'ID': - row[name] = id; - break; - - case 'NAME': - row[name] = label ?? ''; - break; - - case 'PLATFORM': - row[name] = platform?.name ?? ''; - break; - - default: - row[name] = ''; - break; - } - - return row; - }, - {} as Record - ); - } - ); - - const accountsSection = ['## Accounts', '']; - - if (accountsTableRows.length > 0) { - accountsSection.push( - await this.getMarkdownTable({ - columnDefinitions: AiService.ACCOUNTS_TABLE_COLUMN_DEFINITIONS, - rows: accountsTableRows - }) - ); - } else { - accountsSection.push('No accounts found.'); - } - - return accountsSection.join('\n'); + return this.getSection({ + columnDefinitions: AiService.getAccountsTableColumnDefinitions(), + items: accounts, + messageIfEmpty: 'No accounts found.', + title: '## Accounts' + }); } public async getActivitiesTable({ @@ -260,51 +312,6 @@ export class AiService { withExcludedAccountsAndActivities: true }); - const activitiesTableRows = activities.map( - ({ account, assetProfile, currency, date, type, unitPrice }) => { - return AiService.ACTIVITIES_TABLE_COLUMN_DEFINITIONS.reduce( - (row, { key, name }) => { - switch (key) { - case 'ACCOUNT': - row[name] = account?.name ?? ''; - break; - - case 'CURRENCY': - row[name] = currency ?? assetProfile.currency; - break; - - case 'DATE': - row[name] = format(date, DATE_FORMAT); - break; - - case 'NAME': - row[name] = assetProfile.name ?? ''; - break; - - case 'SYMBOL': - row[name] = assetProfile.symbol; - break; - - case 'TYPE': - row[name] = type; - break; - - case 'UNIT_PRICE': - row[name] = unitPrice.toString(); - break; - - default: - row[name] = ''; - break; - } - - return row; - }, - {} as Record - ); - } - ); - const activitiesSection = [ '## Activities', '', @@ -315,12 +322,12 @@ export class AiService { }) ]; - if (activitiesTableRows.length > 0) { + if (activities.length > 0) { activitiesSection.push( '', await this.getMarkdownTable({ - columnDefinitions: AiService.ACTIVITIES_TABLE_COLUMN_DEFINITIONS, - rows: activitiesTableRows + columnDefinitions: AiService.getActivitiesTableColumnDefinitions(), + items: activities }) ); } @@ -328,17 +335,13 @@ export class AiService { return activitiesSection.join('\n'); } - public async getPrompt({ + public async getHoldingsTable({ filters, languageCode, - mode, - userCurrency, userId }: { filters?: Filter[]; languageCode: string; - mode: AiPromptMode; - userCurrency: string; userId: string; }) { const { holdings } = await this.portfolioService.getDetails({ @@ -346,92 +349,47 @@ export class AiService { userId }); - const assetClassTranslations = this.getEnumTranslations({ - languageCode, - id: 'assetClass', - values: Object.values(AssetClass) - }); - - const assetSubClassTranslations = this.getEnumTranslations({ - languageCode, - id: 'assetSubClass', - values: Object.values(AssetSubClass) + const columnDefinitions = AiService.getHoldingsTableColumnDefinitions({ + assetClassTranslations: this.getEnumTranslations({ + languageCode, + id: 'assetClass', + values: Object.values(AssetClass) + }), + assetSubClassTranslations: this.getEnumTranslations({ + languageCode, + id: 'assetSubClass', + values: Object.values(AssetSubClass) + }) }); - const holdingsTableRows = [...holdings] - .sort((a, b) => { + return this.getSection({ + columnDefinitions, + items: [...holdings].sort((a, b) => { return b.allocationInPercentage - a.allocationInPercentage; - }) - .map( - ({ - activitiesCount, - allocationInPercentage, - assetProfile: { - assetClass, - assetSubClass, - currency, - name: label, - symbol - }, - dateOfFirstActivity - }) => { - return AiService.HOLDINGS_TABLE_COLUMN_DEFINITIONS.reduce( - (row, { key, name }) => { - switch (key) { - case 'ACTIVITIES_COUNT': - row[name] = activitiesCount.toString(); - break; - - case 'ALLOCATION_PERCENTAGE': - row[name] = `${(allocationInPercentage * 100).toFixed(3)}%`; - break; - - case 'ASSET_CLASS': - row[name] = assetClassTranslations[assetClass] ?? ''; - break; - - case 'ASSET_SUB_CLASS': - row[name] = assetSubClassTranslations[assetSubClass] ?? ''; - break; - - case 'CURRENCY': - row[name] = currency; - break; - - case 'DATE_OF_FIRST_ACTIVITY': - row[name] = dateOfFirstActivity - ? format(dateOfFirstActivity, DATE_FORMAT) - : ''; - break; - - case 'NAME': - row[name] = label; - break; - - case 'SYMBOL': - row[name] = symbol; - break; - - default: - row[name] = ''; - break; - } - - return row; - }, - {} as Record - ); - } - ); + }), + messageIfEmpty: 'No holdings found.', + title: '## Holdings' + }); + } - const holdingsSection = [ - '## Holdings', - '', - await this.getMarkdownTable({ - columnDefinitions: AiService.HOLDINGS_TABLE_COLUMN_DEFINITIONS, - rows: holdingsTableRows - }) - ].join('\n'); + public async getPrompt({ + filters, + languageCode, + mode, + userCurrency, + userId + }: { + filters?: Filter[]; + languageCode: string; + mode: AiPromptMode; + userCurrency: string; + userId: string; + }) { + const holdingsSection = await this.getHoldingsTable({ + filters, + languageCode, + userId + }); if (mode === 'portfolio') { return holdingsSection; @@ -509,12 +467,12 @@ export class AiService { ); } - private async getMarkdownTable({ + private async getMarkdownTable({ columnDefinitions, - rows + items }: { - columnDefinitions: readonly ColumnDescriptor[]; - rows: Record[]; + columnDefinitions: readonly TableColumnDefinition[]; + items: T[]; }) { // Dynamic import to load ESM module from CommonJS context // eslint-disable-next-line @typescript-eslint/no-implied-eval @@ -523,10 +481,43 @@ export class AiService { ) => Promise; const { tablemark } = await dynamicImport('tablemark'); + const rows = items.map((item) => { + return columnDefinitions.reduce( + (row, { getValue, name }) => { + row[name] = getValue(item); + + return row; + }, + {} as Record + ); + }); + return tablemark(rows, { columns: columnDefinitions.map(({ align, name }) => { return { name, align: align ?? 'left' }; }) }); } + + private async getSection({ + columnDefinitions, + items, + messageIfEmpty, + title + }: { + columnDefinitions: readonly TableColumnDefinition[]; + items: T[]; + messageIfEmpty: string; + title: string; + }) { + const section = [title, '']; + + if (items.length > 0) { + section.push(await this.getMarkdownTable({ columnDefinitions, items })); + } else { + section.push(messageIfEmpty); + } + + return section.join('\n'); + } } diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts index b07441e73e..eab95bf579 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts @@ -1,6 +1,5 @@ -import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-validation.error'; import { ImportService } from '@ghostfolio/api/app/import/import.service'; -import { UserService } from '@ghostfolio/api/app/user/user.service'; +import { HAS_PERMISSION_KEY } from '@ghostfolio/api/decorators/has-permission.decorator'; import { REQUIRES_SCOPE_KEY } from '@ghostfolio/api/decorators/requires-scope.decorator'; import { ConfigurationService } from '@ghostfolio/api/services/configuration/configuration.service'; import { MCP_MAX_ACTIVITIES } from '@ghostfolio/common/config'; @@ -12,10 +11,7 @@ import type { UserWithSettings } from '@ghostfolio/common/types'; -import { HttpException, Logger } from '@nestjs/common'; -import { RpcException } from '@nestjs/microservices'; import { DataSource, Type as ActivityType } from '@prisma/client'; -import { getReasonPhrase, StatusCodes } from 'http-status-codes'; import { GhostfolioMcpController, @@ -48,19 +44,16 @@ function createActivity(overrides: Record = {}) { } describe('GhostfolioMcpController', () => { - const impersonation = { userId: 'user-id' } as ImpersonationContext; + // The AccessPermissionGuard puts the user into the impersonation context + const impersonation = { + user: { id: 'user-id' } as UserWithSettings, + userId: 'user-id' + } as ImpersonationContext; let configuration: Record; let configurationService: ConfigurationService; let controller: GhostfolioMcpController; let importService: ImportService; - let userService: UserService; - - function setupUser(userPermissions: string[]) { - jest.spyOn(userService, 'user').mockResolvedValue({ - permissions: userPermissions - } as UserWithSettings); - } beforeEach(() => { configuration = { @@ -78,14 +71,11 @@ describe('GhostfolioMcpController', () => { import: jest.fn().mockResolvedValue([]) } as unknown as ImportService; - userService = { user: jest.fn() } as unknown as UserService; - controller = new GhostfolioMcpController( undefined, undefined, configurationService, - importService, - userService + importService ); }); @@ -103,21 +93,26 @@ describe('GhostfolioMcpController', () => { ).toEqual([scopes.activityCreate]); }); - it('Refuses a user without the permission to create an activity', async () => { - setupUser([]); + it('Requires the permission to create an activity', () => { + expect( + Reflect.getMetadata( + HAS_PERMISSION_KEY, + GhostfolioMcpController.prototype.importActivities + ) + ).toEqual(permissions.createActivity); + }); - await expect( - controller.importActivities(impersonation, { - activities: [createActivity()] - }) - ).rejects.toThrow(HttpException); + it('Imports the activities for the user of the access', async () => { + await controller.importActivities(impersonation, { + activities: [createActivity()] + }); - expect(importService.import).not.toHaveBeenCalled(); + expect(importService.import).toHaveBeenCalledWith( + expect.objectContaining({ user: impersonation.user }) + ); }); it('Gives the number of the imported and of the skipped activities', async () => { - setupUser([permissions.createActivity]); - jest .spyOn(importService, 'import') .mockResolvedValue([{ id: 'activity-id' } as Activity]); @@ -137,8 +132,6 @@ describe('GhostfolioMcpController', () => { }); it('Resolves the mask of the data source of the Ghostfolio data provider', async () => { - setupUser([permissions.createActivity]); - configuration.DATA_SOURCES_GHOSTFOLIO_DATA_PROVIDER = [DataSource.YAHOO]; configuration.ENABLE_FEATURE_SUBSCRIPTION = true; @@ -156,8 +149,6 @@ describe('GhostfolioMcpController', () => { }); it('Keeps the data source if the subscription is not enabled', async () => { - setupUser([permissions.createActivity]); - configuration.DATA_SOURCES_GHOSTFOLIO_DATA_PROVIDER = [DataSource.YAHOO]; configuration.ENABLE_FEATURE_SUBSCRIPTION = false; @@ -174,68 +165,43 @@ describe('GhostfolioMcpController', () => { ); }); - it('Passes on the message of a validation only', async () => { - setupUser([permissions.createActivity]); - - jest - .spyOn(importService, 'import') - .mockRejectedValue( - new ImportValidationError('activities.0.symbol ("X") is not valid') - ); - - await expect( - controller.importActivities(impersonation, { - activities: [createActivity()] - }) - ).rejects.toThrow( - new RpcException('activities.0.symbol ("X") is not valid') - ); - }); - - it('Hides the message of an unexpected error and writes it to the log', async () => { - setupUser([permissions.createActivity]); - + // The McpToolExceptionFilter turns an error into the answer of the caller + it('Passes on an error of the import', async () => { const error = new Error( 'Unique constraint failed on the fields: (dataSource)' ); - const logError = jest - .spyOn(Logger.prototype, 'error') - .mockImplementation(); - jest.spyOn(importService, 'import').mockRejectedValue(error); await expect( controller.importActivities(impersonation, { activities: [createActivity()] }) - ).rejects.toThrow( - new RpcException(getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR)) - ); - - expect(logError).toHaveBeenCalledWith(error); + ).rejects.toThrow(error); }); + }); - it('Does not write the message of a validation to the log', async () => { - setupUser([permissions.createActivity]); - - const logError = jest - .spyOn(Logger.prototype, 'error') - .mockImplementation(); - - jest - .spyOn(importService, 'import') - .mockRejectedValue( - new ImportValidationError('activities.0.accountId ("X") is not valid') - ); - - await expect( - controller.importActivities(impersonation, { - activities: [createActivity({ accountId: 'X' })] - }) - ).rejects.toThrow(RpcException); + describe('Scopes of the read tools', () => { + it.each([ + ['getAccounts', scopes.accountRead], + ['getActivities', scopes.activityRead], + ['getHoldings', scopes.portfolioRead] + ])('%s requires the scope %s', (methodName, scope) => { + expect( + Reflect.getMetadata( + REQUIRES_SCOPE_KEY, + GhostfolioMcpController.prototype[methodName] + ) + ).toEqual([scope]); + }); - expect(logError).not.toHaveBeenCalled(); + it('Requires no permission, because a read tool reads granted data only', () => { + expect( + Reflect.getMetadata( + HAS_PERMISSION_KEY, + GhostfolioMcpController.prototype.getHoldings + ) + ).toBeUndefined(); }); }); diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.ts index baf68659b0..bc53608fc2 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.ts @@ -1,7 +1,6 @@ import { AiService } from '@ghostfolio/api/app/endpoints/ai/ai.service'; -import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-validation.error'; import { ImportService } from '@ghostfolio/api/app/import/import.service'; -import { UserService } from '@ghostfolio/api/app/user/user.service'; +import { HasPermission } from '@ghostfolio/api/decorators/has-permission.decorator'; import { Impersonation } from '@ghostfolio/api/decorators/impersonation.decorator'; import { RequiresScopeOfAccess } from '@ghostfolio/api/decorators/requires-scope-of-access.decorator'; import { DATE_RANGE_PATTERN } from '@ghostfolio/api/dtos/date-range-filter.dto'; @@ -20,63 +19,60 @@ import { isValidCurrencyCode, isValidDateAfter1970 } from '@ghostfolio/common/helper'; -import { Activity } from '@ghostfolio/common/interfaces'; -import { hasPermission, permissions } from '@ghostfolio/common/permissions'; +import { permissions } from '@ghostfolio/common/permissions'; import { scopes } from '@ghostfolio/common/scopes'; import type { ImpersonationContext } from '@ghostfolio/common/types'; -import { HttpException, Logger, UseFilters } from '@nestjs/common'; -import { Payload, RpcException } from '@nestjs/microservices'; +import { UseFilters } from '@nestjs/common'; +import { Payload } from '@nestjs/microservices'; import { AssetClass, DataSource, Type as ActivityType } from '@prisma/client'; import { McpController, Tool } from '@rekog/mcp-nest'; -import { getReasonPhrase, StatusCodes } from 'http-status-codes'; import { z } from 'zod'; +const ACCOUNT_IDS_PARAMETER = z + .array(z.string().min(1)) + .min(1) + .max(MCP_MAX_ACCOUNTS) + .optional(); + +const ASSET_CLASSES_PARAMETER = z.array(z.enum(AssetClass)).min(1).optional(); + +const HOLDING_PARAMETER = z + .object({ + dataSource: z + .enum(DataSource) + .describe('The data source of the asset profile'), + symbol: z.string().describe('The symbol of the asset profile') + }) + .optional(); + const GET_ACCOUNTS_PARAMETERS = z.object({ - accountIds: z - .array(z.string().min(1)) - .min(1) - .max(MCP_MAX_ACCOUNTS) - .optional() - .describe( - `The identifiers of the accounts to get, at most ${MCP_MAX_ACCOUNTS}` - ), - assetClasses: z - .array(z.enum(AssetClass)) - .min(1) - .optional() - .describe('The asset classes of the accounts to get'), - holding: z - .object({ - dataSource: z - .enum(DataSource) - .describe('The data source of the asset profile'), - symbol: z.string().describe('The symbol of the asset profile') - }) - .optional() - .describe('The asset profile of the accounts to get') + accountIds: ACCOUNT_IDS_PARAMETER.describe( + `The identifiers of the accounts to get, at most ${MCP_MAX_ACCOUNTS}` + ), + assetClasses: ASSET_CLASSES_PARAMETER.describe( + 'The asset classes of the accounts to get' + ), + holding: HOLDING_PARAMETER.describe( + 'The asset profile of the accounts to get' + ) }); const GET_ACTIVITIES_PARAMETERS = z.object({ + accountIds: ACCOUNT_IDS_PARAMETER.describe( + `The identifiers of the accounts of the activities to get, at most ${MCP_MAX_ACCOUNTS}` + ), activityTypes: z .array(z.enum(ActivityType)) .min(1) .optional() .describe('The types of the activities to get'), - assetClasses: z - .array(z.enum(AssetClass)) - .min(1) - .optional() - .describe('The asset classes of the activities to get'), - holding: z - .object({ - dataSource: z - .enum(DataSource) - .describe('The data source of the asset profile'), - symbol: z.string().describe('The symbol of the asset profile') - }) - .optional() - .describe('The asset profile of the activities to get'), + assetClasses: ASSET_CLASSES_PARAMETER.describe( + 'The asset classes of the activities to get' + ), + holding: HOLDING_PARAMETER.describe( + 'The asset profile of the activities to get' + ), range: z .string() .regex(DATE_RANGE_PATTERN) @@ -101,6 +97,18 @@ const GET_ACTIVITIES_PARAMETERS = z.object({ .describe(`The number of activities to get, at most ${MCP_MAX_ACTIVITIES}`) }); +const GET_HOLDINGS_PARAMETERS = z.object({ + accountIds: ACCOUNT_IDS_PARAMETER.describe( + `The identifiers of the accounts of the holdings to get, at most ${MCP_MAX_ACCOUNTS}` + ), + assetClasses: ASSET_CLASSES_PARAMETER.describe( + 'The asset classes of the holdings to get' + ), + holding: HOLDING_PARAMETER.describe( + 'The asset profile of the holdings to get' + ) +}); + export const IMPORT_ACTIVITIES_PARAMETERS = z.object({ activities: z .array( @@ -139,17 +147,22 @@ export const IMPORT_ACTIVITIES_PARAMETERS = z.object({ .describe(`The activities to import, at most ${MCP_MAX_ACTIVITIES}`) }); +/** + * Gives the result of a tool, which is a text in every case, because the tools + * present tables of the markdown format + */ +function getTextContent(text: string) { + return { content: [{ text, type: 'text' as const }] }; +} + @McpController() @UseFilters(McpToolExceptionFilter) export class GhostfolioMcpController { - private readonly logger = new Logger(GhostfolioMcpController.name); - public constructor( private readonly aiService: AiService, private readonly apiService: ApiService, private readonly configurationService: ConfigurationService, - private readonly importService: ImportService, - private readonly userService: UserService + private readonly importService: ImportService ) {} @RequiresScopeOfAccess(scopes.accountRead) @@ -174,16 +187,16 @@ export class GhostfolioMcpController { holding }: z.infer ) { - const filters = this.apiService.buildFiltersFromQueryParams({ - filterByAccounts: accountIds?.join(','), - filterByAssetClasses: assetClasses?.join(','), - filterByDataSource: holding?.dataSource, - filterBySymbol: holding?.symbol + const filters = this.apiService.buildFilters({ + accountIds, + assetClasses, + dataSource: holding?.dataSource, + symbol: holding?.symbol }); const table = await this.aiService.getAccountsTable({ filters, userId }); - return { content: [{ text: table, type: 'text' as const }] }; + return getTextContent(table); } @RequiresScopeOfAccess(scopes.activityRead) @@ -204,6 +217,7 @@ export class GhostfolioMcpController { { userId, userSettings }: ImpersonationContext, @Payload() { + accountIds, activityTypes, assetClasses, holding, @@ -221,10 +235,11 @@ export class GhostfolioMcpController { })); } - const filters = this.apiService.buildFiltersFromQueryParams({ - filterByAssetClasses: assetClasses?.join(','), - filterByDataSource: holding?.dataSource, - filterBySymbol: holding?.symbol + const filters = this.apiService.buildFilters({ + accountIds, + assetClasses, + dataSource: holding?.dataSource, + symbol: holding?.symbol }); const table = await this.aiService.getActivitiesTable({ @@ -238,7 +253,7 @@ export class GhostfolioMcpController { userCurrency: userSettings.baseCurrency }); - return { content: [{ text: table, type: 'text' as const }] }; + return getTextContent(table); } @RequiresScopeOfAccess(scopes.portfolioRead) @@ -246,32 +261,40 @@ export class GhostfolioMcpController { annotations: { openWorldHint: false, readOnlyHint: true, - title: 'Get portfolio' + title: 'Get holdings' }, - description: `Gives the holdings of the portfolio with these columns: ${AiService.getHoldingsTableColumnNames().join( + description: `Gives the holdings of the portfolio, the largest first, with these columns: ${AiService.getHoldingsTableColumnNames().join( ', ' - )}.`, - name: 'get-portfolio' + )}. The allocation in percentage is relative to the holdings of the result, hence the parameters change it.`, + name: 'get-holdings', + parameters: GET_HOLDINGS_PARAMETERS }) - public async getPortfolio( - @Impersonation() { userId, userSettings }: ImpersonationContext + public async getHoldings( + @Impersonation() { userId }: ImpersonationContext, + @Payload() + { + accountIds, + assetClasses, + holding + }: z.infer ) { - const prompt = await this.aiService.getPrompt({ + const filters = this.apiService.buildFilters({ + accountIds, + assetClasses, + dataSource: holding?.dataSource, + symbol: holding?.symbol + }); + + const table = await this.aiService.getHoldingsTable({ + filters, userId, - languageCode: DEFAULT_LANGUAGE_CODE, - mode: 'portfolio', - userCurrency: userSettings.baseCurrency + languageCode: DEFAULT_LANGUAGE_CODE }); - return { content: [{ text: prompt, type: 'text' as const }] }; + return getTextContent(table); } - /** - * The transport gives the tool to every client, because it filters the list - * of the tools by the scopes of request.user, which a request of an access - * never has. The guard refuses the call itself, hence the description names - * the permission which the access needs. - */ + @HasPermission(permissions.createActivity) @RequiresScopeOfAccess(scopes.activityCreate) @Tool({ annotations: { @@ -285,18 +308,9 @@ export class GhostfolioMcpController { parameters: IMPORT_ACTIVITIES_PARAMETERS }) public async importActivities( - @Impersonation() { userId }: ImpersonationContext, + @Impersonation() { user }: ImpersonationContext, @Payload() { activities }: z.infer ) { - const user = await this.userService.user({ id: userId }); - - if (!hasPermission(user?.permissions, permissions.createActivity)) { - throw new HttpException( - getReasonPhrase(StatusCodes.FORBIDDEN), - StatusCodes.FORBIDDEN - ); - } - const ghostfolioDataSources = this.configurationService.get( 'ENABLE_FEATURE_SUBSCRIPTION' ) @@ -313,40 +327,24 @@ export class GhostfolioMcpController { }; }); - let importedActivities: Activity[]; - - try { - importedActivities = await this.importService.import({ - activitiesDto, - user, - accountsWithBalancesDto: [], - assetProfilesWithMarketDataDto: [], - platformsDto: [], - tagsDto: [] - }); - } catch (error) { - // The message of a validation names the activity which is not valid and - // is written for the caller, hence it is passed on - if (error instanceof ImportValidationError) { - throw new RpcException(error.message); - } - - // Every other message can carry internals of the application, hence it - // is written to the log and the reason phrase is passed on instead - this.logger.error(error); - - throw new RpcException( - getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR) - ); - } - - const text = [ - `Imported activities: ${importedActivities.length}`, - `Skipped duplicate activities: ${ - activities.length - importedActivities.length - }` - ].join('\n'); + // The McpToolExceptionFilter decides which message reaches the caller, + // hence an error is not handled here + const importedActivities = await this.importService.import({ + activitiesDto, + user, + accountsWithBalancesDto: [], + assetProfilesWithMarketDataDto: [], + platformsDto: [], + tagsDto: [] + }); - return { content: [{ text, type: 'text' as const }] }; + return getTextContent( + [ + `Imported activities: ${importedActivities.length}`, + `Skipped duplicate activities: ${ + activities.length - importedActivities.length + }` + ].join('\n') + ); } } diff --git a/apps/api/src/app/endpoints/mcp/mcp.module.ts b/apps/api/src/app/endpoints/mcp/mcp.module.ts index e2fbce79f3..fe487fa227 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.module.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.module.ts @@ -18,6 +18,8 @@ import { GhostfolioMcpController } from './mcp.controller'; @Module({ controllers: [GhostfolioMcpController], + // UserModule provides the UserService of the AccessPermissionGuard, which + // the RequiresScopeOfAccess decorator of a tool applies imports: [AiModule, ApiModule, ConfigurationModule, ImportModule, UserModule], providers: [ { diff --git a/apps/api/src/app/import/import.service.ts b/apps/api/src/app/import/import.service.ts index 5f202f4ac4..8b49712782 100644 --- a/apps/api/src/app/import/import.service.ts +++ b/apps/api/src/app/import/import.service.ts @@ -741,19 +741,6 @@ export class ImportService { } } - const assetProfiles = await this.dataProviderService.validateActivities({ - activitiesDto, - assetProfilesWithMarketDataDto, - maxActivitiesToImport, - subscription: user.subscription - }); - - const activitiesExtendedWithErrors = await this.extendActivitiesWithErrors({ - activitiesDto, - userCurrency, - userId: user.id - }); - const accounts = (await this.accountService.getAccounts(user.id)).map( ({ id, name }) => { return { id, name }; @@ -777,8 +764,9 @@ export class ImportService { }); } - // Validate the accounts before any activity is created, since an account - // which does not belong to the user is dropped without a notice otherwise + // Validate the accounts before the asset profiles are resolved, since an + // account which does not belong to the user is dropped without a notice + // otherwise and the resolution spends requests of the data provider for (const [index, { accountId }] of activitiesDto.entries()) { if ( accountId && @@ -792,6 +780,19 @@ export class ImportService { } } + const assetProfiles = await this.dataProviderService.validateActivities({ + activitiesDto, + assetProfilesWithMarketDataDto, + maxActivitiesToImport, + subscription: user.subscription + }); + + const activitiesExtendedWithErrors = await this.extendActivitiesWithErrors({ + activitiesDto, + userCurrency, + userId: user.id + }); + const tags = (await this.tagService.getTagsForUser(user.id)).map( ({ id, name }) => { return { id, name }; diff --git a/apps/api/src/decorators/requires-scope-of-access.decorator.spec.ts b/apps/api/src/decorators/requires-scope-of-access.decorator.spec.ts new file mode 100644 index 0000000000..89ca27dda7 --- /dev/null +++ b/apps/api/src/decorators/requires-scope-of-access.decorator.spec.ts @@ -0,0 +1,45 @@ +import { AccessPermissionGuard } from '@ghostfolio/api/guards/access-permission.guard'; +import { AccessGuard } from '@ghostfolio/api/guards/access.guard'; +import { ScopeGuard } from '@ghostfolio/api/guards/scope.guard'; +import { scopes } from '@ghostfolio/common/scopes'; + +import { GUARDS_METADATA } from '@nestjs/common/constants'; +import { MCP_SCOPES_METADATA_KEY } from '@rekog/mcp-nest'; + +import { RequiresScopeOfAccess } from './requires-scope-of-access.decorator'; +import { REQUIRES_SCOPE_KEY } from './requires-scope.decorator'; + +class TestController { + @RequiresScopeOfAccess(scopes.portfolioRead) + public getHoldings() { + return null; + } +} + +describe('Requires scope of access', () => { + it('Sets the required scopes', () => { + expect( + Reflect.getMetadata( + REQUIRES_SCOPE_KEY, + TestController.prototype.getHoldings + ) + ).toEqual([scopes.portfolioRead]); + }); + + // The transport lists the tools of the scopes of the access only, hence it + // reads the same scopes as the guards + it('Gives the required scopes to the transport of the model context protocol', () => { + expect( + Reflect.getMetadata( + MCP_SCOPES_METADATA_KEY, + TestController.prototype.getHoldings + ) + ).toEqual([scopes.portfolioRead]); + }); + + it('Applies the guards in the required order', () => { + expect( + Reflect.getMetadata(GUARDS_METADATA, TestController.prototype.getHoldings) + ).toEqual([AccessGuard, ScopeGuard, AccessPermissionGuard]); + }); +}); diff --git a/apps/api/src/decorators/requires-scope-of-access.decorator.ts b/apps/api/src/decorators/requires-scope-of-access.decorator.ts index b76f1441c1..026bc541c7 100644 --- a/apps/api/src/decorators/requires-scope-of-access.decorator.ts +++ b/apps/api/src/decorators/requires-scope-of-access.decorator.ts @@ -1,18 +1,29 @@ import { REQUIRES_SCOPE_KEY } from '@ghostfolio/api/decorators/requires-scope.decorator'; +import { AccessPermissionGuard } from '@ghostfolio/api/guards/access-permission.guard'; import { AccessGuard } from '@ghostfolio/api/guards/access.guard'; import { ScopeGuard } from '@ghostfolio/api/guards/scope.guard'; import { Scope } from '@ghostfolio/common/scopes'; import { applyDecorators, SetMetadata, UseGuards } from '@nestjs/common'; +import { ToolScopes } from '@rekog/mcp-nest'; /** * Marks a handler which requires the given scopes of an access, but no * authenticated user. The access itself is the credential, hence a client of * the model context protocol can use it. + * + * The scopes are also given to the transport of the model context protocol, + * which lists the tools of the access only, hence the client never sees a tool + * which the guards refuse. Both consumers read the same argument, hence the + * scopes of a handler are declared one time. + * + * The AccessPermissionGuard evaluates the permission of the HasPermission + * decorator, if the handler has one. */ -export function RequiresScopeOfAccess(...requiredScopes: Scope[]) { +export function RequiresScopeOfAccess(...requiredScopes: [Scope, ...Scope[]]) { return applyDecorators( SetMetadata(REQUIRES_SCOPE_KEY, requiredScopes), - UseGuards(AccessGuard, ScopeGuard) + ToolScopes(requiredScopes), + UseGuards(AccessGuard, ScopeGuard, AccessPermissionGuard) ); } diff --git a/apps/api/src/filters/mcp-tool-exception.filter.spec.ts b/apps/api/src/filters/mcp-tool-exception.filter.spec.ts new file mode 100644 index 0000000000..d8d5d95297 --- /dev/null +++ b/apps/api/src/filters/mcp-tool-exception.filter.spec.ts @@ -0,0 +1,87 @@ +import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-validation.error'; +import { PortfolioSnapshotComputationError } from '@ghostfolio/api/app/portfolio/errors/portfolio-snapshot-computation.error'; + +import { HttpException, Logger } from '@nestjs/common'; +import { RpcException } from '@nestjs/microservices'; +import { getReasonPhrase, StatusCodes } from 'http-status-codes'; +import { firstValueFrom } from 'rxjs'; + +import { McpToolExceptionFilter } from './mcp-tool-exception.filter'; + +describe('McpToolExceptionFilter', () => { + let filter: McpToolExceptionFilter; + let logError: jest.SpyInstance; + + function catchError(exception: unknown) { + return firstValueFrom(filter.catch(exception)); + } + + beforeEach(() => { + filter = new McpToolExceptionFilter(); + logError = jest.spyOn(Logger.prototype, 'error').mockImplementation(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('Passes on the error of an RpcException', async () => { + await expect( + catchError(new RpcException('The activity is not valid')) + ).rejects.toEqual('The activity is not valid'); + }); + + it('Passes on the message of a validation', async () => { + await expect( + catchError( + new ImportValidationError('activities.0.symbol ("X") is not valid') + ) + ).rejects.toEqual({ + message: 'activities.0.symbol ("X") is not valid', + status: 'error' + }); + }); + + it('Does not write the message of a validation to the log', async () => { + await expect( + catchError(new ImportValidationError('activities.0.symbol is not valid')) + ).rejects.toBeDefined(); + + 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)' + ); + + await expect(catchError(exception)).rejects.toEqual({ + message: getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR), + status: 'error' + }); + + expect(logError).toHaveBeenCalledWith(exception); + }); + + it('Gives the reason phrase of the status of an HttpException', async () => { + await expect( + catchError( + new HttpException('The user has no permission', StatusCodes.FORBIDDEN) + ) + ).rejects.toEqual({ + message: getReasonPhrase(StatusCodes.FORBIDDEN), + status: 'error' + }); + }); + + it('Tells that the service is unavailable if the snapshot fails', async () => { + await expect( + catchError( + new PortfolioSnapshotComputationError('The snapshot is not computed') + ) + ).rejects.toEqual({ + message: getReasonPhrase(StatusCodes.SERVICE_UNAVAILABLE), + status: 'error' + }); + }); +}); diff --git a/apps/api/src/filters/mcp-tool-exception.filter.ts b/apps/api/src/filters/mcp-tool-exception.filter.ts index 7525c65598..c6fc520271 100644 --- a/apps/api/src/filters/mcp-tool-exception.filter.ts +++ b/apps/api/src/filters/mcp-tool-exception.filter.ts @@ -1,3 +1,4 @@ +import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-validation.error'; import { PortfolioSnapshotComputationError } from '@ghostfolio/api/app/portfolio/errors/portfolio-snapshot-computation.error'; import { @@ -14,32 +15,42 @@ import { Observable, throwError } from 'rxjs'; * Turns an exception of a tool of the model context protocol into an error of * the caller. Only a message which is written for the caller is passed on, so * that an unexpected exception cannot expose internals of the application. + * This is the only location which makes that decision, hence a tool needs no + * error handling of its own. */ @Catch() export class McpToolExceptionFilter implements RpcExceptionFilter { private readonly logger = new Logger(McpToolExceptionFilter.name); public catch(exception: unknown): Observable { - this.logger.error(exception); - + // The error of an RpcException is written for the caller already if (exception instanceof RpcException) { return throwError(() => { return exception.getError(); }); } + // The message of a validation names the value which is not valid and is + // written for the caller, hence it is passed on and stays out of the log + if (exception instanceof ImportValidationError) { + return throwError(() => { + return { message: exception.message, status: 'error' }; + }); + } + + // Every other message can carry internals, for example the property names + // of a data transfer object of a failed validation, hence it is written to + // the log and the reason phrase of the status is passed on instead + this.logger.error(exception); + return throwError(() => { - return { message: this.getMessage(exception), status: 'error' }; + return { + message: this.getReasonPhraseOfStatus(this.getStatus(exception)), + status: 'error' + }; }); } - 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)); - } - private getReasonPhraseOfStatus(statusCode: number) { try { return getReasonPhrase(statusCode); diff --git a/apps/api/src/guards/access-permission.guard.spec.ts b/apps/api/src/guards/access-permission.guard.spec.ts new file mode 100644 index 0000000000..3b174ff036 --- /dev/null +++ b/apps/api/src/guards/access-permission.guard.spec.ts @@ -0,0 +1,128 @@ +import { UserService } from '@ghostfolio/api/app/user/user.service'; +import { permissions } from '@ghostfolio/common/permissions'; +import type { + ImpersonationContext, + UserWithSettings +} from '@ghostfolio/common/types'; + +import { ExecutionContext, HttpException } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; + +import { AccessPermissionGuard } from './access-permission.guard'; + +// The reflector reads the metadata of the handler and of the class, which the +// tests set directly, hence the two only have to be identifiable +class HandlerClass { + public handler() { + return undefined; + } +} + +describe('AccessPermissionGuard', () => { + let guard: AccessPermissionGuard; + let reflector: Reflector; + let request: { impersonation?: ImpersonationContext }; + let userService: UserService; + + function createExecutionContext() { + return { + getClass: () => { + return HandlerClass; + }, + getHandler: () => { + return HandlerClass.prototype.handler; + }, + getType: () => { + return 'http'; + }, + switchToHttp: () => { + return { + getRequest: () => { + return request; + } + }; + } + } as unknown as ExecutionContext; + } + + function setupRequiredPermission(requiredPermission?: string) { + jest + .spyOn(reflector, 'getAllAndOverride') + .mockReturnValue(requiredPermission); + } + + function setupUser(userPermissions: string[]) { + jest.spyOn(userService, 'user').mockResolvedValue({ + id: 'user-id', + permissions: userPermissions + } as UserWithSettings); + } + + beforeEach(() => { + reflector = new Reflector(); + request = { impersonation: { userId: 'user-id' } as ImpersonationContext }; + userService = { user: jest.fn() } as unknown as UserService; + + guard = new AccessPermissionGuard(reflector, userService); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('Admits a route which requires no permission', async () => { + setupRequiredPermission(undefined); + + expect(await guard.canActivate(createExecutionContext())).toBe(true); + + expect(userService.user).not.toHaveBeenCalled(); + }); + + it('Admits a user who has the permission', async () => { + setupRequiredPermission(permissions.createActivity); + setupUser([permissions.createActivity]); + + expect(await guard.canActivate(createExecutionContext())).toBe(true); + }); + + it('Refuses a user who does not have the permission', async () => { + setupRequiredPermission(permissions.createActivity); + setupUser([]); + + await expect(guard.canActivate(createExecutionContext())).rejects.toThrow( + HttpException + ); + }); + + it('Refuses a request which has no impersonation context', async () => { + setupRequiredPermission(permissions.createActivity); + request = {}; + + await expect(guard.canActivate(createExecutionContext())).rejects.toThrow( + HttpException + ); + + expect(userService.user).not.toHaveBeenCalled(); + }); + + // The handler reads the user from the context, hence it does no second query + it('Puts the user into the impersonation context', async () => { + setupRequiredPermission(permissions.createActivity); + setupUser([permissions.createActivity]); + + await guard.canActivate(createExecutionContext()); + + expect(request.impersonation.user).toEqual( + expect.objectContaining({ id: 'user-id' }) + ); + }); + + it('Reads the permission of the user of the access', async () => { + setupRequiredPermission(permissions.createActivity); + setupUser([permissions.createActivity]); + + await guard.canActivate(createExecutionContext()); + + expect(userService.user).toHaveBeenCalledWith({ id: 'user-id' }); + }); +}); diff --git a/apps/api/src/guards/access-permission.guard.ts b/apps/api/src/guards/access-permission.guard.ts new file mode 100644 index 0000000000..bd55d4862e --- /dev/null +++ b/apps/api/src/guards/access-permission.guard.ts @@ -0,0 +1,62 @@ +import { UserService } from '@ghostfolio/api/app/user/user.service'; +import { HAS_PERMISSION_KEY } from '@ghostfolio/api/decorators/has-permission.decorator'; +import { getRequest } from '@ghostfolio/api/helper/execution-context.helper'; +import { hasPermission } from '@ghostfolio/common/permissions'; +import type { RequestWithUser } from '@ghostfolio/common/types'; + +import { + CanActivate, + ExecutionContext, + HttpException, + Injectable +} from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { StatusCodes, getReasonPhrase } from 'http-status-codes'; + +/** + * Denies a request whose access does not carry the permission required by the + * route. The permission belongs to the user who granted the access and not to + * the grantee, hence the user is read from the impersonation context and not + * from the authenticated user, which such a request never has. + * + * The user is kept in the impersonation context, hence a handler which needs + * the user reads it there and does no second query. + * + * It has to be applied after the AccessGuard, which resolves the context, + * hence the RequiresScopeOfAccess decorator applies both. + */ +@Injectable() +export class AccessPermissionGuard implements CanActivate { + public constructor( + private readonly reflector: Reflector, + private readonly userService: UserService + ) {} + + public async canActivate(context: ExecutionContext) { + const requiredPermission = this.reflector.getAllAndOverride( + HAS_PERMISSION_KEY, + [context.getHandler(), context.getClass()] + ); + + if (!requiredPermission) { + return true; + } + + const { impersonation } = getRequest(context) ?? {}; + + const user = impersonation?.userId + ? await this.userService.user({ id: impersonation.userId }) + : undefined; + + if (!hasPermission(user?.permissions, requiredPermission)) { + throw new HttpException( + getReasonPhrase(StatusCodes.FORBIDDEN), + StatusCodes.FORBIDDEN + ); + } + + impersonation.user = user; + + return true; + } +} diff --git a/apps/api/src/middlewares/mcp-authorization.middleware.ts b/apps/api/src/middlewares/mcp-authorization.middleware.ts index ba3a960535..a90275dfa6 100644 --- a/apps/api/src/middlewares/mcp-authorization.middleware.ts +++ b/apps/api/src/middlewares/mcp-authorization.middleware.ts @@ -21,7 +21,10 @@ export function createMcpAuthorizationMiddleware( impersonationService: ImpersonationService ) { return async ( - request: Request & { impersonationOfBearerToken?: ImpersonationContext }, + request: Request & { + impersonationOfBearerToken?: ImpersonationContext; + user?: { scopes: string[] }; + }, response: Response, next: NextFunction ) => { @@ -56,6 +59,11 @@ export function createMcpAuthorizationMiddleware( // hence the access is looked up once request.impersonationOfBearerToken = impersonation; + // The transport lists the tools which the scopes of request.user permit, + // hence the client sees the tools of its access only. A tool declares its + // scopes with the RequiresScopeOfAccess decorator. + request.user = { scopes: impersonation.scopes }; + return next(); }; } diff --git a/apps/api/src/services/api/api.service.ts b/apps/api/src/services/api/api.service.ts index 11074870ef..93d6f645a2 100644 --- a/apps/api/src/services/api/api.service.ts +++ b/apps/api/src/services/api/api.service.ts @@ -4,34 +4,25 @@ import { Injectable } from '@nestjs/common'; @Injectable() export class ApiService { - public buildFiltersFromQueryParams({ - filterByAccounts, - filterByAssetClasses, - filterByAssetSubClasses, - filterByDataSource, - filterByHoldingType, - filterBySearchQuery, - filterBySymbol, - filterByTags + public buildFilters({ + accountIds = [], + assetClasses = [], + assetSubClasses = [], + dataSource, + holdingType, + searchQuery, + symbol, + tagIds = [] }: { - filterByAccounts?: string; - filterByAssetClasses?: string; - filterByAssetSubClasses?: string; - filterByDataSource?: string; - filterByHoldingType?: string; - filterBySearchQuery?: string; - filterBySymbol?: string; - filterByTags?: string; + accountIds?: string[]; + assetClasses?: string[]; + assetSubClasses?: string[]; + dataSource?: string; + holdingType?: string; + searchQuery?: string; + symbol?: string; + tagIds?: string[]; }): Filter[] { - const accountIds = filterByAccounts?.split(',') ?? []; - const assetClasses = filterByAssetClasses?.split(',') ?? []; - const assetSubClasses = filterByAssetSubClasses?.split(',') ?? []; - const dataSource = filterByDataSource; - const holdingType = filterByHoldingType; - const searchQuery = filterBySearchQuery?.toLowerCase(); - const symbol = filterBySymbol; - const tagIds = filterByTags?.split(',') ?? []; - const filters = [ ...accountIds.map((accountId) => { return { @@ -45,9 +36,9 @@ export class ApiService { type: 'ASSET_CLASS' } as Filter; }), - ...assetSubClasses.map((assetClass) => { + ...assetSubClasses.map((assetSubClass) => { return { - id: assetClass, + id: assetSubClass, type: 'ASSET_SUB_CLASS' } as Filter; }), @@ -75,7 +66,7 @@ export class ApiService { if (searchQuery) { filters.push({ - id: searchQuery, + id: searchQuery.toLowerCase(), type: 'SEARCH_QUERY' }); } @@ -90,6 +81,41 @@ export class ApiService { return filters; } + /** + * Builds the filters of the query parameters of a route, which carry the + * identifiers as a list separated by commas + */ + public buildFiltersFromQueryParams({ + filterByAccounts, + filterByAssetClasses, + filterByAssetSubClasses, + filterByDataSource, + filterByHoldingType, + filterBySearchQuery, + filterBySymbol, + filterByTags + }: { + filterByAccounts?: string; + filterByAssetClasses?: string; + filterByAssetSubClasses?: string; + filterByDataSource?: string; + filterByHoldingType?: string; + filterBySearchQuery?: string; + filterBySymbol?: string; + filterByTags?: string; + }): Filter[] { + return this.buildFilters({ + accountIds: filterByAccounts?.split(','), + assetClasses: filterByAssetClasses?.split(','), + assetSubClasses: filterByAssetSubClasses?.split(','), + dataSource: filterByDataSource, + holdingType: filterByHoldingType, + searchQuery: filterBySearchQuery, + symbol: filterBySymbol, + tagIds: filterByTags?.split(',') + }); + } + public buildFiltersFromUserSettings({ userSettings }: { diff --git a/libs/common/src/lib/types/impersonation-context.type.ts b/libs/common/src/lib/types/impersonation-context.type.ts index 29b7970b1f..778c4d4d4c 100644 --- a/libs/common/src/lib/types/impersonation-context.type.ts +++ b/libs/common/src/lib/types/impersonation-context.type.ts @@ -12,6 +12,8 @@ export interface ImpersonationContext { authenticatedUserSubscription?: UserWithSettings['subscription']; isActive: boolean; scopes: string[]; + /** Only set if a guard needs the permissions of the impersonated user */ + user?: UserWithSettings; userId: string; userSettings: UserSettings; userSubscription?: UserWithSettings['subscription'];