From 69b41d5e044b59daa2e8575ee19935394f8764d2 Mon Sep 17 00:00:00 2001 From: Maximilian Kreuzkam Date: Mon, 13 Jan 2025 16:15:04 +0100 Subject: [PATCH 1/2] EW-1056: Remove for await and improve legibility. --- .../infra/sync/tsp/tsp-oauth-data.mapper.ts | 157 +++++++++++------- .../sync/tsp/tsp-sync-migration.service.ts | 1 + .../service/tsp-provisioning.service.spec.ts | 22 ++- .../service/tsp-provisioning.service.ts | 75 +++++---- .../provisioning/strategy/tsp/tsp.strategy.ts | 83 +++++---- 5 files changed, 219 insertions(+), 119 deletions(-) diff --git a/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts b/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts index dcadb75014..a644ad3754 100644 --- a/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts +++ b/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts @@ -33,11 +33,21 @@ export class TspOauthDataMapper { provisioningStrategy: SystemProvisioningStrategy.TSP, }); - const externalSchools = new Map(); - const externalClasses = new Map(); - const teacherForClasses = new Map>(); + const externalSchools = this.createMapOfExternalSchoolDtos(schools); + const { externalClasses, teacherForClasses } = this.createMapsOfClasses(tspClasses); const oauthDataDtos: OauthDataDto[] = []; + oauthDataDtos.push( + ...this.mapTspTeacherToOauthDataDtos(tspTeachers, systemDto, externalSchools, externalClasses, teacherForClasses) + ); + oauthDataDtos.push(...this.mapTspStudentsToOauthDataDtos(tspStudents, systemDto, externalSchools, externalClasses)); + + return oauthDataDtos; + } + + private createMapOfExternalSchoolDtos(schools: School[]): Map { + const externalSchools = new Map(); + schools.forEach((school) => { if (!school.externalId) { throw new BadDataLoggableException(`School ${school.id} has no externalId`); @@ -51,6 +61,16 @@ export class TspOauthDataMapper { ); }); + return externalSchools; + } + + private createMapsOfClasses(tspClasses: RobjExportKlasse[]): { + externalClasses: Map; + teacherForClasses: Map>; + } { + const externalClasses = new Map(); + const teacherForClasses = new Map>(); + tspClasses.forEach((tspClass) => { if (!tspClass.klasseId) { this.logger.info(new TspMissingExternalIdLoggable('class')); @@ -71,62 +91,87 @@ export class TspOauthDataMapper { } }); - tspTeachers.forEach((tspTeacher) => { - if (!tspTeacher.lehrerUid) { - this.logger.info(new TspMissingExternalIdLoggable('teacher')); - return; - } - - const externalUser = new ExternalUserDto({ - externalId: tspTeacher.lehrerUid, - firstName: tspTeacher.lehrerVorname, - lastName: tspTeacher.lehrerNachname, - roles: [RoleName.TEACHER], - }); - - const classIds = teacherForClasses.get(tspTeacher.lehrerUid) ?? []; - const classes: ExternalClassDto[] = classIds - .map((classId) => externalClasses.get(classId)) - .filter((externalClass: ExternalClassDto | undefined): externalClass is ExternalClassDto => !!externalClass); - - const externalSchool = tspTeacher.schuleNummer == null ? undefined : externalSchools.get(tspTeacher.schuleNummer); - - const oauthDataDto = new OauthDataDto({ - system: systemDto, - externalUser, - externalSchool, - externalClasses: classes, - }); - - oauthDataDtos.push(oauthDataDto); - }); - - tspStudents.forEach((tspStudent) => { - if (!tspStudent.schuelerUid) { - this.logger.info(new TspMissingExternalIdLoggable('student')); - return; - } - - const externalUser = new ExternalUserDto({ - externalId: tspStudent.schuelerUid, - firstName: tspStudent.schuelerVorname, - lastName: tspStudent.schuelerNachname, - roles: [RoleName.STUDENT], - }); - - const classStudent = tspStudent.klasseId == null ? undefined : externalClasses.get(tspStudent.klasseId); + return { externalClasses, teacherForClasses }; + } - const externalSchool = tspStudent.schuleNummer == null ? undefined : externalSchools.get(tspStudent.schuleNummer); + private mapTspTeacherToOauthDataDtos( + tspTeachers: RobjExportLehrer[], + systemDto: ProvisioningSystemDto, + externalSchools: Map, + externalClasses: Map, + teacherForClasses: Map> + ): OauthDataDto[] { + const oauthDataDtos = tspTeachers + .map((tspTeacher) => { + if (!tspTeacher.lehrerUid) { + this.logger.info(new TspMissingExternalIdLoggable('teacher')); + return null; + } + + const externalUser = new ExternalUserDto({ + externalId: tspTeacher.lehrerUid, + firstName: tspTeacher.lehrerVorname, + lastName: tspTeacher.lehrerNachname, + roles: [RoleName.TEACHER], + }); + + const classIds = teacherForClasses.get(tspTeacher.lehrerUid) ?? []; + const classes: ExternalClassDto[] = classIds + .map((classId) => externalClasses.get(classId)) + .filter((externalClass: ExternalClassDto | undefined): externalClass is ExternalClassDto => !!externalClass); + + const externalSchool = + tspTeacher.schuleNummer == null ? undefined : externalSchools.get(tspTeacher.schuleNummer); + + const oauthDataDto = new OauthDataDto({ + system: systemDto, + externalUser, + externalSchool, + externalClasses: classes, + }); + + return oauthDataDto; + }) + .filter((oauthDataDto) => oauthDataDto !== null); - const oauthDataDto = new OauthDataDto({ - system: systemDto, - externalUser, - externalSchool, - externalClasses: classStudent ? [classStudent] : [], - }); + return oauthDataDtos; + } - oauthDataDtos.push(oauthDataDto); - }); + private mapTspStudentsToOauthDataDtos( + tspStudents: RobjExportSchueler[], + systemDto: ProvisioningSystemDto, + externalSchools: Map, + externalClasses: Map + ): OauthDataDto[] { + const oauthDataDtos = tspStudents + .map((tspStudent) => { + if (!tspStudent.schuelerUid) { + this.logger.info(new TspMissingExternalIdLoggable('student')); + return null; + } + + const externalUser = new ExternalUserDto({ + externalId: tspStudent.schuelerUid, + firstName: tspStudent.schuelerVorname, + lastName: tspStudent.schuelerNachname, + roles: [RoleName.STUDENT], + }); + + const classStudent = tspStudent.klasseId == null ? undefined : externalClasses.get(tspStudent.klasseId); + + const externalSchool = + tspStudent.schuleNummer == null ? undefined : externalSchools.get(tspStudent.schuleNummer); + + const oauthDataDto = new OauthDataDto({ + system: systemDto, + externalUser, + externalSchool, + externalClasses: classStudent ? [classStudent] : [], + }); + + return oauthDataDto; + }) + .filter((oauthDataDto) => oauthDataDto !== null); return oauthDataDtos; } diff --git a/apps/server/src/infra/sync/tsp/tsp-sync-migration.service.ts b/apps/server/src/infra/sync/tsp/tsp-sync-migration.service.ts index a5d54f56b5..652c4566a5 100644 --- a/apps/server/src/infra/sync/tsp/tsp-sync-migration.service.ts +++ b/apps/server/src/infra/sync/tsp/tsp-sync-migration.service.ts @@ -145,6 +145,7 @@ export class TspSyncMigrationService { } private async saveUsersAndAccounts(users: UserDO[], accounts: Account[]): Promise { + // These statement should probably not be combined with Promise.all() because last time I tried the performance massively decreased. await this.userService.saveAll(users); await this.accountService.saveAll(accounts); } diff --git a/apps/server/src/modules/provisioning/service/tsp-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/service/tsp-provisioning.service.spec.ts index b42ff993b9..0fd91fdbd4 100644 --- a/apps/server/src/modules/provisioning/service/tsp-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/service/tsp-provisioning.service.spec.ts @@ -87,6 +87,8 @@ describe('TspProvisioningService', () => { beforeEach(() => { jest.resetAllMocks(); + jest.restoreAllMocks(); + jest.clearAllMocks(); }); it('should be defined', () => { @@ -133,7 +135,7 @@ describe('TspProvisioningService', () => { }); describe('provisionClasses', () => { - describe('when user ID is missing', () => { + describe('when user ID is missing and class exists', () => { const setup = () => { const school = schoolFactory.build(); const classes = [setupExternalClass()]; @@ -149,6 +151,24 @@ describe('TspProvisioningService', () => { }); }); + describe('when user ID is missing and class does not exist', () => { + const setup = () => { + const school = schoolFactory.build(); + const classes = [setupExternalClass()]; + const user = userDoFactory.build(); + + classServiceMock.findClassWithSchoolIdAndExternalId.mockResolvedValueOnce(null); + + return { school, classes, user }; + }; + + it('should throw', async () => { + const { school, classes, user } = setup(); + + await expect(sut.provisionClasses(school, classes, user)).rejects.toThrow(BadDataLoggableException); + }); + }); + describe('when class exists', () => { const setup = () => { const school = schoolFactory.build(); diff --git a/apps/server/src/modules/provisioning/service/tsp-provisioning.service.ts b/apps/server/src/modules/provisioning/service/tsp-provisioning.service.ts index d494829298..8b18d4eddf 100644 --- a/apps/server/src/modules/provisioning/service/tsp-provisioning.service.ts +++ b/apps/server/src/modules/provisioning/service/tsp-provisioning.service.ts @@ -1,5 +1,5 @@ import { AccountSave, AccountService } from '@modules/account'; -import { ClassFactory, ClassService, ClassSourceOptions } from '@modules/class'; +import { Class, ClassFactory, ClassService, ClassSourceOptions } from '@modules/class'; import { RoleService } from '@modules/role'; import { Injectable } from '@nestjs/common'; import { NotFoundLoggableException } from '@shared/common/loggable-exception'; @@ -45,41 +45,60 @@ export class TspProvisioningService { } public async provisionClasses(school: School, classes: ExternalClassDto[], user: UserDO): Promise { - if (!user.id) + const promises = classes.map(async (clazz) => { + const currentClass = await this.classService.findClassWithSchoolIdAndExternalId(school.id, clazz.externalId); + + if (currentClass) { + await this.updateClass(currentClass, clazz, school, user); + } else { + await this.createClass(clazz, school, user); + } + }); + + await Promise.all(promises); + } + + private async updateClass(currentClass: Class, clazz: ExternalClassDto, school: School, user: UserDO): Promise { + if (!user.id) { throw new BadDataLoggableException('User ID is missing', { externalId: user.externalId, }); + } - for await (const clazz of classes) { - const currentClass = await this.classService.findClassWithSchoolIdAndExternalId(school.id, clazz.externalId); + currentClass.schoolId = school.id; + currentClass.name = clazz.name ?? currentClass.name; + currentClass.year = school.currentYear?.id; + currentClass.source = this.ENTITY_SOURCE; + currentClass.sourceOptions = new ClassSourceOptions({ tspUid: clazz.externalId }); - if (currentClass) { - // Case: Class exists -> update class - currentClass.schoolId = school.id; - currentClass.name = clazz.name ?? currentClass.name; - currentClass.year = school.currentYear?.id; - currentClass.source = this.ENTITY_SOURCE; - currentClass.sourceOptions = new ClassSourceOptions({ tspUid: clazz.externalId }); + if (user.roles.some((role) => role.name === RoleName.TEACHER)) { + currentClass.addTeacher(user.id); + } + if (user.roles.some((role) => role.name === RoleName.STUDENT)) { + currentClass.addUser(user.id); + } - if (user.roles.some((role) => role.name === RoleName.TEACHER)) currentClass.addTeacher(user.id); - if (user.roles.some((role) => role.name === RoleName.STUDENT)) currentClass.addUser(user.id); + await this.classService.save(currentClass); + } - await this.classService.save(currentClass); - } else { - // Case: Class does not exist yet -> create new class - const newClass = ClassFactory.create({ - name: clazz.name, - schoolId: school.id, - year: school.currentYear?.id, - teacherIds: user.roles.some((role) => role.name === RoleName.TEACHER) ? [user.id] : [], - userIds: user.roles.some((role) => role.name === RoleName.STUDENT) ? [user.id] : [], - source: this.ENTITY_SOURCE, - sourceOptions: new ClassSourceOptions({ tspUid: clazz.externalId }), - }); - - await this.classService.save(newClass); - } + private async createClass(clazz: ExternalClassDto, school: School, user: UserDO): Promise { + if (!user.id) { + throw new BadDataLoggableException('User ID is missing', { + externalId: user.externalId, + }); } + + const newClass = ClassFactory.create({ + name: clazz.name, + schoolId: school.id, + year: school.currentYear?.id, + teacherIds: user.roles.some((role) => role.name === RoleName.TEACHER) ? [user.id] : [], + userIds: user.roles.some((role) => role.name === RoleName.STUDENT) ? [user.id] : [], + source: this.ENTITY_SOURCE, + sourceOptions: new ClassSourceOptions({ tspUid: clazz.externalId }), + }); + + await this.classService.save(newClass); } public async provisionUser(data: OauthDataDto, school: School): Promise { diff --git a/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts b/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts index e9dc4d7b8b..ccba8d22e0 100644 --- a/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts +++ b/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts @@ -28,40 +28,8 @@ export class TspProvisioningStrategy extends ProvisioningStrategy { } public override async getData(input: OauthDataStrategyInputDto): Promise { - const decodedAccessToken: JwtPayload | null = jwt.decode(input.accessToken, { json: true }); - - if (!decodedAccessToken) { - throw new IdTokenExtractionFailureLoggableException('sub'); - } - - const payload = new TspJwtPayload(decodedAccessToken); - const errors = await validate(payload); - - if (errors.length > 0) { - throw new IdTokenExtractionFailureLoggableException(errors.map((error) => error.property).join(', ')); - } - - const externalUserDto = new ExternalUserDto({ - externalId: payload.sub, - firstName: payload.personVorname, - lastName: payload.personNachname, - roles: payload.ptscListRolle - .split(',') - .map((role) => this.mapRoles(role)) - .filter(Boolean) as RoleName[], - }); - - if (externalUserDto.roles && externalUserDto.roles.length < 1) { - throw new IdTokenExtractionFailureLoggableException('ptscListRolle'); - } - - const externalSchoolDto = new ExternalSchoolDto({ - externalId: payload.ptscSchuleNummer, - }); - - const externalClassDtoList = payload.ptscListKlasseId - ? payload.ptscListKlasseId.split(',').map((externalId) => new ExternalClassDto({ externalId })) - : []; + const payload = await this.parseAndValidateToken(input); + const { externalUserDto, externalSchoolDto, externalClassDtoList } = this.extractDateFromPayload(payload); const oauthDataDto = new OauthDataDto({ system: input.system, @@ -107,4 +75,51 @@ export class TspProvisioningStrategy extends ProvisioningStrategy { return null; } } + + private async parseAndValidateToken(input: OauthDataStrategyInputDto): Promise { + const decodedAccessToken: JwtPayload | null = jwt.decode(input.accessToken, { json: true }); + + if (!decodedAccessToken) { + throw new IdTokenExtractionFailureLoggableException('sub'); + } + + const payload = new TspJwtPayload(decodedAccessToken); + const errors = await validate(payload); + + if (errors.length > 0) { + throw new IdTokenExtractionFailureLoggableException(errors.map((error) => error.property).join(', ')); + } + + return payload; + } + + private extractDateFromPayload(payload: TspJwtPayload): { + externalUserDto: ExternalUserDto; + externalSchoolDto: ExternalSchoolDto; + externalClassDtoList: ExternalClassDto[]; + } { + const externalUserDto = new ExternalUserDto({ + externalId: payload.sub, + firstName: payload.personVorname, + lastName: payload.personNachname, + roles: payload.ptscListRolle + .split(',') + .map((role) => this.mapRoles(role)) + .filter(Boolean) as RoleName[], + }); + + if (externalUserDto.roles && externalUserDto.roles.length < 1) { + throw new IdTokenExtractionFailureLoggableException('ptscListRolle'); + } + + const externalSchoolDto = new ExternalSchoolDto({ + externalId: payload.ptscSchuleNummer, + }); + + const externalClassDtoList = payload.ptscListKlasseId + ? payload.ptscListKlasseId.split(',').map((externalId) => new ExternalClassDto({ externalId })) + : []; + + return { externalUserDto, externalSchoolDto, externalClassDtoList }; + } } From 410750bf179377c3a814134462383d20309fd0c0 Mon Sep 17 00:00:00 2001 From: Maximilian Kreuzkam Date: Wed, 15 Jan 2025 09:30:53 +0100 Subject: [PATCH 2/2] Fix typos. --- apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts | 4 ++-- .../src/modules/provisioning/strategy/tsp/tsp.strategy.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts b/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts index a644ad3754..f748477315 100644 --- a/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts +++ b/apps/server/src/infra/sync/tsp/tsp-oauth-data.mapper.ts @@ -38,7 +38,7 @@ export class TspOauthDataMapper { const oauthDataDtos: OauthDataDto[] = []; oauthDataDtos.push( - ...this.mapTspTeacherToOauthDataDtos(tspTeachers, systemDto, externalSchools, externalClasses, teacherForClasses) + ...this.mapTspTeachersToOauthDataDtos(tspTeachers, systemDto, externalSchools, externalClasses, teacherForClasses) ); oauthDataDtos.push(...this.mapTspStudentsToOauthDataDtos(tspStudents, systemDto, externalSchools, externalClasses)); @@ -94,7 +94,7 @@ export class TspOauthDataMapper { return { externalClasses, teacherForClasses }; } - private mapTspTeacherToOauthDataDtos( + private mapTspTeachersToOauthDataDtos( tspTeachers: RobjExportLehrer[], systemDto: ProvisioningSystemDto, externalSchools: Map, diff --git a/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts b/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts index ccba8d22e0..36ebfbd250 100644 --- a/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts +++ b/apps/server/src/modules/provisioning/strategy/tsp/tsp.strategy.ts @@ -29,7 +29,7 @@ export class TspProvisioningStrategy extends ProvisioningStrategy { public override async getData(input: OauthDataStrategyInputDto): Promise { const payload = await this.parseAndValidateToken(input); - const { externalUserDto, externalSchoolDto, externalClassDtoList } = this.extractDateFromPayload(payload); + const { externalUserDto, externalSchoolDto, externalClassDtoList } = this.extractDataFromPayload(payload); const oauthDataDto = new OauthDataDto({ system: input.system, @@ -93,7 +93,7 @@ export class TspProvisioningStrategy extends ProvisioningStrategy { return payload; } - private extractDateFromPayload(payload: TspJwtPayload): { + private extractDataFromPayload(payload: TspJwtPayload): { externalUserDto: ExternalUserDto; externalSchoolDto: ExternalSchoolDto; externalClassDtoList: ExternalClassDto[];