From e74db835364f016fb4d3f47f3979b9c550d7b0b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marvin=20=C3=96hlerking?= <103562092+MarvinOehlerkingCap@users.noreply.github.com> Date: Thu, 22 Feb 2024 13:51:03 +0100 Subject: [PATCH] N21-1768 fix CTL LTI resource_link_id (#4784) --- .../mikro-orm/Migration20240221131029.ts | 19 ++++ .../config/lti11-tool-config-create.params.ts | 9 +- .../config/lti11-tool-config-update.params.ts | 5 - .../config/lti11-tool-config.response.ts | 6 +- .../domain/config/lti11-tool-config.do.ts | 3 - .../entity/config/lti11-tool-config.entity.ts | 4 - .../entity/external-tool.entity.spec.ts | 1 - .../external-tool-request.mapper.spec.ts | 4 - .../external-tool-response.mapper.spec.ts | 1 - .../lti11-tool-launch.strategy.spec.ts | 99 +++++++------------ .../lti11-tool-launch.strategy.ts | 7 +- .../external-tool.repo.integration.spec.ts | 1 - .../externaltool/external-tool.repo.mapper.ts | 6 +- .../tool/external-tool.factory.ts | 1 - .../factory/external-tool-entity.factory.ts | 1 - backup/setup/migrations.json | 17 +++- scripts/copy-legacy-tool-to-ctl.js | 2 - 17 files changed, 79 insertions(+), 107 deletions(-) create mode 100644 apps/server/src/migrations/mikro-orm/Migration20240221131029.ts diff --git a/apps/server/src/migrations/mikro-orm/Migration20240221131029.ts b/apps/server/src/migrations/mikro-orm/Migration20240221131029.ts new file mode 100644 index 00000000000..5406d1c478b --- /dev/null +++ b/apps/server/src/migrations/mikro-orm/Migration20240221131029.ts @@ -0,0 +1,19 @@ +import { Migration } from '@mikro-orm/migrations-mongodb'; + +export class Migration20240221131029 extends Migration { + async up(): Promise { + const contextExternalToolResponse = await this.driver.nativeUpdate( + 'external-tools', + { config_type: 'lti11' }, + { $unset: { config_resource_link_id: '' } } + ); + + console.info(`Removed ${contextExternalToolResponse.affectedRows} resource_link_id(s) in context-external-tools`); + } + + // eslint-disable-next-line @typescript-eslint/require-await + async down(): Promise { + // do nothing + console.error(`Migration down not implemented. You might need to restore database from backup!`); + } +} diff --git a/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-create.params.ts b/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-create.params.ts index 5e3d41f20f9..86887a9ab42 100644 --- a/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-create.params.ts +++ b/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-create.params.ts @@ -1,5 +1,5 @@ -import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; -import { IsEnum, IsLocale, IsOptional, IsString } from 'class-validator'; +import { ApiProperty } from '@nestjs/swagger'; +import { IsEnum, IsLocale, IsString } from 'class-validator'; import { LtiMessageType, LtiPrivacyPermission, ToolConfigType } from '../../../../../common/enum'; import { ExternalToolConfigCreateParams } from './external-tool-config.params'; @@ -20,11 +20,6 @@ export class Lti11ToolConfigCreateParams extends ExternalToolConfigCreateParams @ApiProperty() secret!: string; - @IsString() - @IsOptional() - @ApiPropertyOptional() - resource_link_id?: string; - @IsEnum(LtiMessageType) @ApiProperty() lti_message_type!: LtiMessageType; diff --git a/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-update.params.ts b/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-update.params.ts index 87de384fb2d..7b238690f58 100644 --- a/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-update.params.ts +++ b/apps/server/src/modules/tool/external-tool/controller/dto/request/config/lti11-tool-config-update.params.ts @@ -21,11 +21,6 @@ export class Lti11ToolConfigUpdateParams extends ExternalToolConfigCreateParams @ApiPropertyOptional() secret?: string; - @IsString() - @IsOptional() - @ApiPropertyOptional() - resource_link_id?: string; - @IsEnum(LtiMessageType) @ApiProperty() lti_message_type!: LtiMessageType; diff --git a/apps/server/src/modules/tool/external-tool/controller/dto/response/config/lti11-tool-config.response.ts b/apps/server/src/modules/tool/external-tool/controller/dto/response/config/lti11-tool-config.response.ts index a38a62542fe..a3c68ba5f43 100644 --- a/apps/server/src/modules/tool/external-tool/controller/dto/response/config/lti11-tool-config.response.ts +++ b/apps/server/src/modules/tool/external-tool/controller/dto/response/config/lti11-tool-config.response.ts @@ -1,4 +1,4 @@ -import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; +import { ApiProperty } from '@nestjs/swagger'; import { LtiMessageType, LtiPrivacyPermission, ToolConfigType } from '../../../../../common/enum'; import { ExternalToolConfigResponse } from './external-tool-config.response'; @@ -12,9 +12,6 @@ export class Lti11ToolConfigResponse extends ExternalToolConfigResponse { @ApiProperty() key: string; - @ApiPropertyOptional() - resource_link_id?: string; - @ApiProperty() lti_message_type: LtiMessageType; @@ -29,7 +26,6 @@ export class Lti11ToolConfigResponse extends ExternalToolConfigResponse { this.type = ToolConfigType.LTI11; this.baseUrl = props.baseUrl; this.key = props.key; - this.resource_link_id = props.resource_link_id; this.lti_message_type = props.lti_message_type; this.privacy_permission = props.privacy_permission; this.launch_presentation_locale = props.launch_presentation_locale; diff --git a/apps/server/src/modules/tool/external-tool/domain/config/lti11-tool-config.do.ts b/apps/server/src/modules/tool/external-tool/domain/config/lti11-tool-config.do.ts index faf9ae41f66..b17e6fd5979 100644 --- a/apps/server/src/modules/tool/external-tool/domain/config/lti11-tool-config.do.ts +++ b/apps/server/src/modules/tool/external-tool/domain/config/lti11-tool-config.do.ts @@ -6,8 +6,6 @@ export class Lti11ToolConfig extends ExternalToolConfig { secret: string; - resource_link_id?: string; - lti_message_type: LtiMessageType; privacy_permission: LtiPrivacyPermission; @@ -21,7 +19,6 @@ export class Lti11ToolConfig extends ExternalToolConfig { }); this.key = props.key; this.secret = props.secret; - this.resource_link_id = props.resource_link_id; this.lti_message_type = props.lti_message_type; this.privacy_permission = props.privacy_permission; this.launch_presentation_locale = props.launch_presentation_locale; diff --git a/apps/server/src/modules/tool/external-tool/entity/config/lti11-tool-config.entity.ts b/apps/server/src/modules/tool/external-tool/entity/config/lti11-tool-config.entity.ts index c5edfd09900..7ebd6b2ca67 100644 --- a/apps/server/src/modules/tool/external-tool/entity/config/lti11-tool-config.entity.ts +++ b/apps/server/src/modules/tool/external-tool/entity/config/lti11-tool-config.entity.ts @@ -11,9 +11,6 @@ export class Lti11ToolConfigEntity extends ExternalToolConfigEntity { @Property() secret: string; - @Property({ nullable: true }) - resource_link_id?: string; - @Enum() lti_message_type: LtiMessageType; @@ -28,7 +25,6 @@ export class Lti11ToolConfigEntity extends ExternalToolConfigEntity { this.type = ToolConfigType.LTI11; this.key = props.key; this.secret = props.secret; - this.resource_link_id = props.resource_link_id; this.lti_message_type = props.lti_message_type; this.privacy_permission = props.privacy_permission; this.launch_presentation_locale = props.launch_presentation_locale; diff --git a/apps/server/src/modules/tool/external-tool/entity/external-tool.entity.spec.ts b/apps/server/src/modules/tool/external-tool/entity/external-tool.entity.spec.ts index 9b144b2c725..0ab767552c8 100644 --- a/apps/server/src/modules/tool/external-tool/entity/external-tool.entity.spec.ts +++ b/apps/server/src/modules/tool/external-tool/entity/external-tool.entity.spec.ts @@ -33,7 +33,6 @@ describe('ExternalToolEntity', () => { baseUrl: 'mockBaseUrl', key: 'mockKey', secret: 'mockSecret', - resource_link_id: 'mockLink', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, privacy_permission: LtiPrivacyPermission.ANONYMOUS, launch_presentation_locale: 'de-DE', diff --git a/apps/server/src/modules/tool/external-tool/mapper/external-tool-request.mapper.spec.ts b/apps/server/src/modules/tool/external-tool/mapper/external-tool-request.mapper.spec.ts index 8089b5b4f87..fb372b73ae4 100644 --- a/apps/server/src/modules/tool/external-tool/mapper/external-tool-request.mapper.spec.ts +++ b/apps/server/src/modules/tool/external-tool/mapper/external-tool-request.mapper.spec.ts @@ -135,7 +135,6 @@ describe('ExternalToolRequestMapper', () => { lti11ConfigParams.baseUrl = 'mockUrl'; lti11ConfigParams.key = 'mockKey'; lti11ConfigParams.secret = 'mockSecret'; - lti11ConfigParams.resource_link_id = 'mockLink'; lti11ConfigParams.lti_message_type = LtiMessageType.BASIC_LTI_LAUNCH_REQUEST; lti11ConfigParams.privacy_permission = LtiPrivacyPermission.NAME; lti11ConfigParams.launch_presentation_locale = 'de-DE'; @@ -144,7 +143,6 @@ describe('ExternalToolRequestMapper', () => { privacy_permission: LtiPrivacyPermission.NAME, secret: 'mockSecret', key: 'mockKey', - resource_link_id: 'mockLink', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, type: ToolConfigType.LTI11, baseUrl: 'mockUrl', @@ -389,7 +387,6 @@ describe('ExternalToolRequestMapper', () => { lti11ConfigParams.baseUrl = 'mockUrl'; lti11ConfigParams.key = 'mockKey'; lti11ConfigParams.secret = 'mockSecret'; - lti11ConfigParams.resource_link_id = 'mockLink'; lti11ConfigParams.lti_message_type = LtiMessageType.BASIC_LTI_LAUNCH_REQUEST; lti11ConfigParams.privacy_permission = LtiPrivacyPermission.NAME; lti11ConfigParams.launch_presentation_locale = 'de-DE'; @@ -398,7 +395,6 @@ describe('ExternalToolRequestMapper', () => { privacy_permission: LtiPrivacyPermission.NAME, secret: 'mockSecret', key: 'mockKey', - resource_link_id: 'mockLink', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, type: ToolConfigType.LTI11, baseUrl: 'mockUrl', diff --git a/apps/server/src/modules/tool/external-tool/mapper/external-tool-response.mapper.spec.ts b/apps/server/src/modules/tool/external-tool/mapper/external-tool-response.mapper.spec.ts index e87502a5d21..f388ef810ca 100644 --- a/apps/server/src/modules/tool/external-tool/mapper/external-tool-response.mapper.spec.ts +++ b/apps/server/src/modules/tool/external-tool/mapper/external-tool-response.mapper.spec.ts @@ -222,7 +222,6 @@ describe('ExternalToolResponseMapper', () => { type: ToolConfigType.LTI11, baseUrl: 'mockUrl', launch_presentation_locale: 'de-DE', - resource_link_id: 'linkId', }); const customParameterResponse: CustomParameterResponse = new CustomParameterResponse({ diff --git a/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.spec.ts b/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.spec.ts index dfc26bb2bea..760a50cdcb7 100644 --- a/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.spec.ts +++ b/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.spec.ts @@ -14,7 +14,7 @@ import { import { pseudonymFactory } from '@shared/testing/factory/domainobject/pseudonym.factory'; import { ObjectId } from 'bson'; import { Authorization } from 'oauth-1.0a'; -import { LtiMessageType, LtiPrivacyPermission, LtiRole, ToolContextType } from '../../../common/enum'; +import { LtiMessageType, LtiPrivacyPermission, LtiRole } from '../../../common/enum'; import { ContextExternalTool } from '../../../context-external-tool/domain'; import { ExternalTool } from '../../../external-tool/domain'; import { SchoolExternalTool } from '../../../school-external-tool/domain'; @@ -93,7 +93,6 @@ describe('Lti11ToolLaunchStrategy', () => { const mockKey = 'mockKey'; const mockSecret = 'mockSecret'; const ltiMessageType = LtiMessageType.BASIC_LTI_LAUNCH_REQUEST; - const resourceLinkId = 'resourceLinkId'; const launchPresentationLocale = 'de-DE'; const externalTool: ExternalTool = externalToolFactory @@ -102,7 +101,6 @@ describe('Lti11ToolLaunchStrategy', () => { secret: mockSecret, lti_message_type: ltiMessageType, privacy_permission: LtiPrivacyPermission.PUBLIC, - resource_link_id: resourceLinkId, launch_presentation_locale: launchPresentationLocale, }) .buildWithId(); @@ -136,7 +134,7 @@ describe('Lti11ToolLaunchStrategy', () => { mockKey, mockSecret, ltiMessageType, - resourceLinkId, + contextExternalTool, launchPresentationLocale, }; }; @@ -155,7 +153,7 @@ describe('Lti11ToolLaunchStrategy', () => { }); it('should contain mandatory lti attributes', async () => { - const { data, ltiMessageType, resourceLinkId, launchPresentationLocale } = setup(); + const { data, ltiMessageType, contextExternalTool, launchPresentationLocale } = setup(); const result: PropertyData[] = await strategy.buildToolLaunchDataFromConcreteConfig('userId', data); @@ -165,7 +163,7 @@ describe('Lti11ToolLaunchStrategy', () => { new PropertyData({ name: 'lti_version', value: 'LTI-1p0', location: PropertyLocation.BODY }), new PropertyData({ name: 'resource_link_id', - value: resourceLinkId, + value: contextExternalTool.id as string, location: PropertyLocation.BODY, }), new PropertyData({ @@ -183,57 +181,6 @@ describe('Lti11ToolLaunchStrategy', () => { }); }); - describe('when no resource link id is available', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory - .withLti11Config({ - key: 'mockKey', - secret: 'mockSecret', - lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, - privacy_permission: LtiPrivacyPermission.PUBLIC, - resource_link_id: undefined, - }) - .buildWithId(); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); - - const contextId: string = new ObjectId().toHexString(); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ - contextRef: { id: contextId, type: ToolContextType.COURSE }, - }); - - const data: ToolLaunchParams = { - contextExternalTool, - schoolExternalTool, - externalTool, - }; - - const user: UserDO = userDoFactory.buildWithId(); - - userService.findById.mockResolvedValue(user); - - return { - data, - contextId, - }; - }; - - it('should use the context id as resource link id', async () => { - const { data, contextId } = setup(); - - const result: PropertyData[] = await strategy.buildToolLaunchDataFromConcreteConfig('userId', data); - - expect(result).toEqual( - expect.arrayContaining([ - new PropertyData({ - name: 'resource_link_id', - value: contextId, - location: PropertyLocation.BODY, - }), - ]) - ); - }); - }); - describe('when lti privacyPermission is public', () => { const setup = () => { const externalTool: ExternalTool = externalToolFactory @@ -242,7 +189,6 @@ describe('Lti11ToolLaunchStrategy', () => { secret: 'mockSecret', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, privacy_permission: LtiPrivacyPermission.PUBLIC, - resource_link_id: 'resourceLinkId', }) .buildWithId(); const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); @@ -318,7 +264,6 @@ describe('Lti11ToolLaunchStrategy', () => { secret: 'mockSecret', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, privacy_permission: LtiPrivacyPermission.NAME, - resource_link_id: 'resourceLinkId', }) .buildWithId(); const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); @@ -389,7 +334,6 @@ describe('Lti11ToolLaunchStrategy', () => { secret: 'mockSecret', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, privacy_permission: LtiPrivacyPermission.EMAIL, - resource_link_id: 'resourceLinkId', }) .buildWithId(); const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); @@ -461,7 +405,6 @@ describe('Lti11ToolLaunchStrategy', () => { secret: 'mockSecret', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, privacy_permission: LtiPrivacyPermission.PSEUDONYMOUS, - resource_link_id: 'resourceLinkId', }) .buildWithId(); const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); @@ -529,7 +472,6 @@ describe('Lti11ToolLaunchStrategy', () => { secret: 'mockSecret', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, privacy_permission: LtiPrivacyPermission.ANONYMOUS, - resource_link_id: 'resourceLinkId', }) .buildWithId(); const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); @@ -606,6 +548,39 @@ describe('Lti11ToolLaunchStrategy', () => { ); }); }); + + describe('when context external tool id is undefined', () => { + const setup = () => { + const externalTool: ExternalTool = externalToolFactory + .withLti11Config({ + key: 'mockKey', + secret: 'mockSecret', + lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, + privacy_permission: LtiPrivacyPermission.ANONYMOUS, + }) + .buildWithId(); + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); + const contextExternalTool: ContextExternalTool = contextExternalToolFactory.build(); + + const data: ToolLaunchParams = { + contextExternalTool, + schoolExternalTool, + externalTool, + }; + + return { + data, + }; + }; + + it('should throw an InternalServerErrorException', async () => { + const { data } = setup(); + + const func = async () => strategy.buildToolLaunchDataFromConcreteConfig('userId', data); + + await expect(func).rejects.toThrow(new InternalServerErrorException()); + }); + }); }); describe('buildToolLaunchRequestPayload', () => { diff --git a/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.ts b/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.ts index fbc94469c25..26026df9a42 100644 --- a/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.ts +++ b/apps/server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.ts @@ -39,7 +39,6 @@ export class Lti11ToolLaunchStrategy extends AbstractLaunchStrategy { data: ToolLaunchParams ): Promise { const { config } = data.externalTool; - const contextId: EntityId = data.contextExternalTool.contextRef.id; if (!ExternalTool.isLti11Config(config)) { throw new UnprocessableEntityException( @@ -47,6 +46,10 @@ export class Lti11ToolLaunchStrategy extends AbstractLaunchStrategy { ); } + if (!data.contextExternalTool.id) { + throw new InternalServerErrorException(); + } + const user: UserDO = await this.userService.findById(userId); const roleNames: RoleName[] = user.roles.map((roleRef: RoleReference): RoleName => roleRef.name); @@ -60,7 +63,7 @@ export class Lti11ToolLaunchStrategy extends AbstractLaunchStrategy { new PropertyData({ name: 'lti_version', value: 'LTI-1p0', location: PropertyLocation.BODY }), new PropertyData({ name: 'resource_link_id', - value: config.resource_link_id || contextId, + value: data.contextExternalTool.id, location: PropertyLocation.BODY, }), new PropertyData({ diff --git a/apps/server/src/shared/repo/externaltool/external-tool.repo.integration.spec.ts b/apps/server/src/shared/repo/externaltool/external-tool.repo.integration.spec.ts index a17320075d6..c5ee7d38327 100644 --- a/apps/server/src/shared/repo/externaltool/external-tool.repo.integration.spec.ts +++ b/apps/server/src/shared/repo/externaltool/external-tool.repo.integration.spec.ts @@ -204,7 +204,6 @@ describe('ExternalToolRepo', () => { key: 'key', lti_message_type: LtiMessageType.BASIC_LTI_LAUNCH_REQUEST, privacy_permission: LtiPrivacyPermission.PSEUDONYMOUS, - resource_link_id: 'resource_link_id', launch_presentation_locale: 'de-DE', }); const { domainObject } = setupDO(config); diff --git a/apps/server/src/shared/repo/externaltool/external-tool.repo.mapper.ts b/apps/server/src/shared/repo/externaltool/external-tool.repo.mapper.ts index b9113606c7e..37fb4609b29 100644 --- a/apps/server/src/shared/repo/externaltool/external-tool.repo.mapper.ts +++ b/apps/server/src/shared/repo/externaltool/external-tool.repo.mapper.ts @@ -1,4 +1,4 @@ -import { UnprocessableEntityException } from '@nestjs/common'; +import { EntityData } from '@mikro-orm/core'; import { CustomParameter, CustomParameterEntry } from '@modules/tool/common/domain'; import { CustomParameterEntryEntity } from '@modules/tool/common/entity'; import { ToolConfigType } from '@modules/tool/common/enum'; @@ -10,7 +10,7 @@ import { Lti11ToolConfigEntity, Oauth2ToolConfigEntity, } from '@modules/tool/external-tool/entity'; -import { EntityData } from '@mikro-orm/core'; +import { UnprocessableEntityException } from '@nestjs/common'; // TODO: maybe rename because of usage in external tool repo and school external tool repo export class ExternalToolRepoMapper { @@ -70,7 +70,6 @@ export class ExternalToolRepoMapper { key: lti11Config.key, secret: lti11Config.secret, lti_message_type: lti11Config.lti_message_type, - resource_link_id: lti11Config.resource_link_id, privacy_permission: lti11Config.privacy_permission, launch_presentation_locale: lti11Config.launch_presentation_locale, }); @@ -131,7 +130,6 @@ export class ExternalToolRepoMapper { key: lti11Config.key, secret: lti11Config.secret, lti_message_type: lti11Config.lti_message_type, - resource_link_id: lti11Config.resource_link_id, privacy_permission: lti11Config.privacy_permission, launch_presentation_locale: lti11Config.launch_presentation_locale, }); diff --git a/apps/server/src/shared/testing/factory/domainobject/tool/external-tool.factory.ts b/apps/server/src/shared/testing/factory/domainobject/tool/external-tool.factory.ts index ab0ac21c76a..dc1b6aae3da 100644 --- a/apps/server/src/shared/testing/factory/domainobject/tool/external-tool.factory.ts +++ b/apps/server/src/shared/testing/factory/domainobject/tool/external-tool.factory.ts @@ -56,7 +56,6 @@ export const lti11ToolConfigFactory = DoBaseFactory.define