From f746e8634c9278960116cdec1d60bdc88952d8e4 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Tue, 28 Nov 2023 16:56:20 +0100 Subject: [PATCH] requested changes WIP --- .../common/service/common-tool.service.ts | 11 ++++- .../service/context-external-tool.service.ts | 19 +++----- ...tricted-context-mismatch-loggabble.spec.ts | 47 +++++++++++++++++++ .../restricted-context-mismatch-loggabble.ts | 24 ++++++++++ .../uc/context-external-tool.uc.ts | 2 +- ...xternal-tool-configuration.service.spec.ts | 10 ++++ .../external-tool-configuration.service.ts | 15 +++--- .../tool-launch.controller.api.spec.ts | 22 +++------ 8 files changed, 112 insertions(+), 38 deletions(-) create mode 100644 apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.spec.ts create mode 100644 apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.ts diff --git a/apps/server/src/modules/tool/common/service/common-tool.service.ts b/apps/server/src/modules/tool/common/service/common-tool.service.ts index 1dccc42ab1f..eea1706aaf1 100644 --- a/apps/server/src/modules/tool/common/service/common-tool.service.ts +++ b/apps/server/src/modules/tool/common/service/common-tool.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common'; import { ExternalTool } from '../../external-tool/domain'; import { SchoolExternalTool } from '../../school-external-tool/domain'; import { ContextExternalTool } from '../../context-external-tool/domain'; -import { ToolConfigurationStatus } from '../enum'; +import { ToolConfigurationStatus, ToolContextType } from '../enum'; import { ToolVersion } from '../interface'; // TODO N21-1337 remove class when tool versioning is removed @@ -11,7 +11,7 @@ export class CommonToolService { /** * @deprecated use ToolVersionService */ - determineToolConfigurationStatus( + public determineToolConfigurationStatus( externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool, contextExternalTool: ContextExternalTool @@ -30,4 +30,11 @@ export class CommonToolService { private isLatest(tool1: ToolVersion, tool2: ToolVersion): boolean { return tool1.getVersion() >= tool2.getVersion(); } + + public isContextRestricted(externalTool: ExternalTool, context: ToolContextType): boolean { + if (externalTool.restrictToContexts?.length && !externalTool.restrictToContexts.includes(context)) { + return true; + } + return false; + } } diff --git a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts index f212019185e..c75dbc2a5d5 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts @@ -1,21 +1,22 @@ import { Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain'; import { ContextExternalToolRepo } from '@shared/repo'; -import { AuthorizationContext } from '@modules/authorization'; -import { ForbiddenLoggableException } from '@modules/authorization/domain/error'; import { ContextExternalTool, ContextRef } from '../domain'; import { ContextExternalToolQuery } from '../uc/dto/context-external-tool.types'; import { SchoolExternalTool } from '../../school-external-tool/domain'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalToolService } from '../../school-external-tool/service'; +import { RestrictedContextMismatchLoggable } from './restricted-context-mismatch-loggabble'; +import { CommonToolService } from '../../common/service'; @Injectable() export class ContextExternalToolService { constructor( private readonly contextExternalToolRepo: ContextExternalToolRepo, private readonly externalToolService: ExternalToolService, - private readonly schoolExternalToolService: SchoolExternalToolService + private readonly schoolExternalToolService: SchoolExternalToolService, + private readonly commonToolService: CommonToolService ) {} public async findContextExternalTools(query: ContextExternalToolQuery): Promise { @@ -58,21 +59,15 @@ export class ContextExternalToolService { return contextExternalTools; } - public async checkContextRestrictions( - contextExternalTool: ContextExternalTool, - userId: EntityId, - context: AuthorizationContext - ): Promise { + public async checkContextRestrictions(contextExternalTool: ContextExternalTool): Promise { const schoolExternalTool: SchoolExternalTool = await this.schoolExternalToolService.findById( contextExternalTool.schoolToolRef.schoolToolId ); const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - if (externalTool.restrictToContexts && externalTool.restrictToContexts[0]) { - if (!externalTool.restrictToContexts.includes(contextExternalTool.contextRef.type)) { - throw new ForbiddenLoggableException(userId, 'ContextExternalTool', context); - } + if (this.commonToolService.isContextRestricted(externalTool, contextExternalTool.contextRef.type)) { + throw new RestrictedContextMismatchLoggable(externalTool.name, contextExternalTool.contextRef.type); } } } diff --git a/apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.spec.ts b/apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.spec.ts new file mode 100644 index 00000000000..c7de14f196e --- /dev/null +++ b/apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.spec.ts @@ -0,0 +1,47 @@ +import { ToolContextType } from '../../common/enum'; +import { RestrictedContextMismatchLoggable } from './restricted-context-mismatch-loggabble'; + +describe('RestrictedContextMismatchLoggable', () => { + describe('constructor', () => { + const setup = () => { + const externalToolName = 'name'; + const context: ToolContextType = ToolContextType.COURSE; + + return { externalToolName, context }; + }; + + it('should create an instance of RestrictedContextMismatchLoggable', () => { + const { externalToolName, context } = setup(); + + const loggable = new RestrictedContextMismatchLoggable(externalToolName, context); + + expect(loggable).toBeInstanceOf(RestrictedContextMismatchLoggable); + }); + }); + + describe('getLogMessage', () => { + const setup = () => { + const externalToolName = 'name'; + const context: ToolContextType = ToolContextType.COURSE; + const loggable = new RestrictedContextMismatchLoggable(externalToolName, context); + + return { loggable, externalToolName, context }; + }; + + it('should return a loggable message', () => { + const { loggable, externalToolName, context } = setup(); + + const message = loggable.getLogMessage(); + + expect(message).toEqual({ + type: 'UNPROCESSABLE_ENTITY_EXCEPTION', + message: `Could not create an instance of ${externalToolName} in context: ${context} because of the context restrictions of the tool.`, + stack: loggable.stack, + data: { + externalToolName, + context, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.ts b/apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.ts new file mode 100644 index 00000000000..8e6fc4e643a --- /dev/null +++ b/apps/server/src/modules/tool/context-external-tool/service/restricted-context-mismatch-loggabble.ts @@ -0,0 +1,24 @@ +import { UnprocessableEntityException } from '@nestjs/common'; +import { Loggable } from '@src/core/logger/interfaces'; +import { ErrorLogMessage, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; +import { ToolContextType } from '../../common/enum'; + +export class RestrictedContextMismatchLoggable extends UnprocessableEntityException implements Loggable { + constructor(private readonly externalToolName: string, private readonly context: ToolContextType) { + super(); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + const message: LogMessage | ErrorLogMessage | ValidationErrorLogMessage = { + type: 'UNPROCESSABLE_ENTITY_EXCEPTION', + message: `Could not create an instance of ${this.externalToolName} in context: ${this.context} because of the context restrictions of the tool.`, + stack: this.stack, + data: { + externalToolName: this.externalToolName, + context: this.context, + }, + }; + + return message; + } +} diff --git a/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts b/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts index e1875ba1300..c0e218f3beb 100644 --- a/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts +++ b/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts @@ -45,7 +45,7 @@ export class ContextExternalToolUc { await this.toolPermissionHelper.ensureContextPermissions(userId, contextExternalTool, context); - await this.contextExternalToolService.checkContextRestrictions(contextExternalTool, userId, context); + await this.contextExternalToolService.checkContextRestrictions(contextExternalTool); await this.contextExternalToolValidationService.validate(contextExternalTool); diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts index 475b823d7b8..aec1dd8353c 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts @@ -7,6 +7,7 @@ import { schoolExternalToolFactory, setupEntities, } from '@shared/testing'; +import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { CustomParameter } from '../../common/domain'; import { CustomParameterScope, ToolContextType } from '../../common/enum'; import { ContextExternalTool } from '../../context-external-tool/domain'; @@ -16,10 +17,12 @@ import { ExternalTool } from '../domain'; import { ContextExternalToolTemplateInfo } from '../uc'; import { ExternalToolConfigurationService } from './external-tool-configuration.service'; import { ToolContextTypesList } from '../controller/dto/response/tool-context-types-list'; +import { CommonToolService } from '../../common/service'; describe('ExternalToolConfigurationService', () => { let module: TestingModule; let service: ExternalToolConfigurationService; + let commonToolservice: DeepMocked; let toolFeatures: IToolFeatures; @@ -35,11 +38,16 @@ describe('ExternalToolConfigurationService', () => { contextConfigurationEnabled: false, }, }, + { + provide: CommonToolService, + useValue: createMock(), + }, ], }).compile(); service = module.get(ExternalToolConfigurationService); toolFeatures = module.get(ToolFeatures); + commonToolservice = module.get(CommonToolService); }); afterEach(() => { @@ -197,6 +205,8 @@ describe('ExternalToolConfigurationService', () => { const availableTools: ContextExternalToolTemplateInfo[] = [{ externalTool, schoolExternalTool }]; + commonToolservice.isContextRestricted.mockReturnValueOnce(false); + return { contextType, availableTools, diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts index 8a76f3c45a4..b0738aa691c 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts @@ -8,10 +8,14 @@ import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { ExternalTool } from '../domain'; import { ContextExternalToolTemplateInfo } from '../uc/dto'; import { ToolContextTypesList } from '../controller/dto/response/tool-context-types-list'; +import { CommonToolService } from '../../common/service'; @Injectable() export class ExternalToolConfigurationService { - constructor(@Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures) {} + constructor( + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, + private readonly commonTooLService: CommonToolService + ) {} public filterForAvailableTools(externalTools: Page, toolIdsInUse: EntityId[]): ExternalTool[] { const visibleTools: ExternalTool[] = externalTools.data.filter((tool: ExternalTool): boolean => !tool.isHidden); @@ -79,12 +83,9 @@ export class ExternalToolConfigurationService { availableTools: ContextExternalToolTemplateInfo[], contextType: ToolContextType ): ContextExternalToolTemplateInfo[] { - const availableToolsForContext: ContextExternalToolTemplateInfo[] = availableTools.filter((availableTool) => { - if (availableTool.externalTool.restrictToContexts && availableTool.externalTool.restrictToContexts[0]) { - return availableTool.externalTool.restrictToContexts.includes(contextType); - } - return true; - }); + const availableToolsForContext: ContextExternalToolTemplateInfo[] = availableTools.filter( + (availableTool) => !this.commonTooLService.isContextRestricted(availableTool.externalTool, contextType) + ); return availableToolsForContext; } diff --git a/apps/server/src/modules/tool/tool-launch/controller/api-test/tool-launch.controller.api.spec.ts b/apps/server/src/modules/tool/tool-launch/controller/api-test/tool-launch.controller.api.spec.ts index 23133a1654e..0cd0e59cb26 100644 --- a/apps/server/src/modules/tool/tool-launch/controller/api-test/tool-launch.controller.api.spec.ts +++ b/apps/server/src/modules/tool/tool-launch/controller/api-test/tool-launch.controller.api.spec.ts @@ -12,6 +12,7 @@ import { schoolFactory, TestApiClient, UserAndAccountTestFactory, + customParameterFactory, } from '@shared/testing'; import { ServerTestModule } from '@modules/server'; import { Response } from 'supertest'; @@ -19,12 +20,7 @@ import { SchoolExternalToolEntity } from '../../../school-external-tool/entity'; import { LaunchRequestMethod } from '../../types'; import { ToolLaunchRequestResponse, ToolLaunchParams } from '../dto'; import { ContextExternalToolEntity, ContextExternalToolType } from '../../../context-external-tool/entity'; -import { - CustomParameterLocation, - CustomParameterScope, - CustomParameterType, - ExternalToolEntity, -} from '../../../external-tool/entity'; +import { CustomParameterLocation, CustomParameterScope, ExternalToolEntity } from '../../../external-tool/entity'; import { ToolConfigType } from '../../../common/enum'; describe('ToolLaunchController (API)', () => { @@ -70,22 +66,16 @@ describe('ToolLaunchController (API)', () => { config: basicToolConfigFactory.build({ baseUrl: 'https://mockurl.de', type: ToolConfigType.BASIC }), version: 0, parameters: [ - { + customParameterFactory.build({ name: 'schoolMockParameter', - displayName: 'MockParameter', scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, location: CustomParameterLocation.PATH, - isOptional: false, - }, - { + }), + customParameterFactory.build({ name: 'contextMockParameter', - displayName: 'MockParameter', scope: CustomParameterScope.CONTEXT, - type: CustomParameterType.STRING, location: CustomParameterLocation.PATH, - isOptional: false, - }, + }), ], }); const schoolExternalToolEntity: SchoolExternalToolEntity = schoolExternalToolEntityFactory.buildWithId({