N21-1768 fix CTL LTI resource_link_id (#4784)

hpi-schul-cloud · Feb 22, 2024 · e74db83 · e74db83
1 parent b6457fe
commit e74db83
Show file tree

Hide file tree

Showing 17 changed files with 79 additions and 107 deletions.
diff --git a/apps/server/src/migrations/mikro-orm/Migration20240221131029.ts 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<void> {
+		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<void> {
+		// do nothing
+		console.error(`Migration down not implemented. You might need to restore database from backup!`);
+	}
+}
diff --git a/...dules/tool/external-tool/controller/dto/request/config/lti11-tool-config-create.params.ts b/...dules/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/...dules/tool/external-tool/controller/dto/request/config/lti11-tool-config-update.params.ts b/...dules/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/...c/modules/tool/external-tool/controller/dto/response/config/lti11-tool-config.response.ts b/...c/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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/...r/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.spec.ts b/...r/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/...server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.ts b/...server/src/modules/tool/tool-launch/service/launch-strategy/lti11-tool-launch.strategy.ts
@@ -39,14 +39,17 @@ export class Lti11ToolLaunchStrategy extends AbstractLaunchStrategy {
 		data: ToolLaunchParams
 	): Promise<PropertyData[]> {
 		const { config } = data.externalTool;
-		const contextId: EntityId = data.contextExternalTool.contextRef.id;
 
 		if (!ExternalTool.isLti11Config(config)) {
 			throw new UnprocessableEntityException(
 				`Unable to build LTI 1.1 launch data. Tool configuration is of type ${config.type}. Expected "lti11"`
 			);
 		}
 
+		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
@@ -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);