From eb7d0a0ea05939636233e3edb91ee98375f4f75e Mon Sep 17 00:00:00 2001 From: Arne Gnisa Date: Thu, 5 Oct 2023 14:54:34 +0200 Subject: [PATCH 1/2] N21-1332 changes group provisioning to add only current user instead of whole group --- .../src/modules/group/domain/group.spec.ts | 64 +++++++++++++++++++ apps/server/src/modules/group/domain/group.ts | 6 ++ .../service/oidc-provisioning.service.spec.ts | 2 +- .../oidc/service/oidc-provisioning.service.ts | 13 ++-- .../sanis/sanis-response.mapper.spec.ts | 4 -- .../strategy/sanis/sanis-response.mapper.ts | 7 +- 6 files changed, 82 insertions(+), 14 deletions(-) create mode 100644 apps/server/src/modules/group/domain/group.spec.ts diff --git a/apps/server/src/modules/group/domain/group.spec.ts b/apps/server/src/modules/group/domain/group.spec.ts new file mode 100644 index 00000000000..fd50ddb02db --- /dev/null +++ b/apps/server/src/modules/group/domain/group.spec.ts @@ -0,0 +1,64 @@ +import { groupFactory } from '@shared/testing'; + +import { ObjectId } from 'bson'; +import { GroupUser } from './group-user'; + +describe('Group (Domain Object)', () => { + describe('addUser', () => { + describe('when the user already exists in the group', () => { + const setup = () => { + const existingUser: GroupUser = new GroupUser({ + userId: new ObjectId().toHexString(), + roleId: new ObjectId().toHexString(), + }); + const group = groupFactory.build({ + users: [existingUser], + }); + + return { + group, + existingUser, + }; + }; + + it('should not add the user', () => { + const { group, existingUser } = setup(); + + group.addUser(existingUser); + + expect(group.users.length).toEqual(1); + }); + }); + + describe('when the user does not exist in the group', () => { + const setup = () => { + const newUser: GroupUser = new GroupUser({ + userId: new ObjectId().toHexString(), + roleId: new ObjectId().toHexString(), + }); + const group = groupFactory.build({ + users: [ + { + userId: new ObjectId().toHexString(), + roleId: new ObjectId().toHexString(), + }, + ], + }); + + return { + group, + newUser, + }; + }; + + it('should add the user', () => { + const { group, newUser } = setup(); + + group.addUser(newUser); + + expect(group.users).toContain(newUser); + expect(group.users.length).toEqual(2); + }); + }); + }); +}); diff --git a/apps/server/src/modules/group/domain/group.ts b/apps/server/src/modules/group/domain/group.ts index cbc5a416ffe..fcabef50f44 100644 --- a/apps/server/src/modules/group/domain/group.ts +++ b/apps/server/src/modules/group/domain/group.ts @@ -37,4 +37,10 @@ export class Group extends DomainObject { get organizationId(): string | undefined { return this.props.organizationId; } + + addUser(user: GroupUser): void { + if (!this.users.find((u) => u.userId === user.userId)) { + this.users.push(user); + } + } } diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.spec.ts index 1084f21bbf4..9383ee66362 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.spec.ts @@ -461,7 +461,7 @@ describe('OidcProvisioningService', () => { describe('when provision group', () => { const setup = () => { - const group: Group = groupFactory.build(); + const group: Group = groupFactory.build({ users: [] }); groupService.findByExternalSource.mockResolvedValue(group); const school: LegacySchoolDo = legacySchoolDoFactory.build({ id: 'schoolId' }); diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.ts index 1a16b8578c9..07dedeef02f 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/oidc-provisioning.service.ts @@ -118,10 +118,6 @@ export class OidcProvisioningService { } async provisionExternalGroup(externalGroup: ExternalGroupDto, systemId: EntityId): Promise { - if (externalGroup.users.length === 0) { - return; - } - const existingGroup: Group | null = await this.groupService.findByExternalSource( externalGroup.externalId, systemId @@ -144,6 +140,10 @@ export class OidcProvisioningService { const users: GroupUser[] = await this.getFilteredGroupUsers(externalGroup, systemId); + if (!users.length) { + return; + } + const group: Group = new Group({ id: existingGroup ? existingGroup.id : new ObjectId().toHexString(), name: externalGroup.name, @@ -155,8 +155,9 @@ export class OidcProvisioningService { organizationId, validFrom: externalGroup.from, validUntil: externalGroup.until, - users, + users: existingGroup ? existingGroup.users : [], }); + users.forEach((user: GroupUser) => group.addUser(user)); await this.groupService.save(group); } @@ -167,7 +168,7 @@ export class OidcProvisioningService { const user: UserDO | null = await this.userService.findByExternalId(externalGroupUser.externalUserId, systemId); const roles: RoleDto[] = await this.roleService.findByNames([externalGroupUser.roleName]); - if (!user || !user.id || roles.length !== 1 || !roles[0].id) { + if (!user?.id || roles.length !== 1 || !roles[0].id) { this.logger.info(new UserForGroupNotFoundLoggable(externalGroupUser)); return null; } diff --git a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts index 902e2e850f2..94cfbf7301c 100644 --- a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts @@ -171,10 +171,6 @@ describe('SanisResponseMapper', () => { until: group.gruppe.laufzeit.bis, externalId: group.gruppe.id, users: [ - { - externalUserId: group.sonstige_gruppenzugehoerige![0].ktid, - roleName: RoleName.STUDENT, - }, { externalUserId: personenkontext.id, roleName: RoleName.TEACHER, diff --git a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts index 34a7ff4029c..beaa2927a22 100644 --- a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts +++ b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts @@ -66,7 +66,7 @@ export class SanisResponseMapper { } mapToExternalGroupDtos(source: SanisResponse): ExternalGroupDto[] | undefined { - const groups: SanisGruppenResponse[] | undefined = source.personenkontexte[0].gruppen; + const groups: SanisGruppenResponse[] | undefined = source.personenkontexte[0]?.gruppen; if (!groups) { return undefined; @@ -81,12 +81,13 @@ export class SanisResponseMapper { } const sanisGroupUsers: SanisSonstigeGruppenzugehoerigeResponse[] = [ - ...(group.sonstige_gruppenzugehoerige ?? []), { ktid: source.personenkontexte[0].id, rollen: group.gruppenzugehoerigkeit.rollen, }, - ].sort((a, b) => a.ktid.localeCompare(b.ktid)); + ] + .filter((sanisGroupUser) => sanisGroupUser.ktid && sanisGroupUser.rollen) + .sort((a, b) => a.ktid.localeCompare(b.ktid)); const gruppenzugehoerigkeiten: ExternalGroupUserDto[] = sanisGroupUsers .map((relation): ExternalGroupUserDto | null => this.mapToExternalGroupUser(relation)) From 0ffb63a8677e49580ace99faf5c1f80f82c3c85e Mon Sep 17 00:00:00 2001 From: Arne Gnisa Date: Fri, 6 Oct 2023 07:33:34 +0200 Subject: [PATCH 2/2] N21-1332 test adjustment --- .../strategy/sanis/sanis-response.mapper.spec.ts | 6 ++---- .../provisioning/strategy/sanis/sanis-response.mapper.ts | 4 +--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts index 94cfbf7301c..c52e654155b 100644 --- a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.spec.ts @@ -202,9 +202,7 @@ describe('SanisResponseMapper', () => { describe('when a group role mapping is missing', () => { const setup = () => { const { sanisResponse } = setupSanisResponse(); - sanisResponse.personenkontexte[0].gruppen![0]!.sonstige_gruppenzugehoerige![0].rollen = [ - SanisGroupRole.SCHOOL_SUPPORT, - ]; + sanisResponse.personenkontexte[0].gruppen![0]!.gruppenzugehoerigkeit.rollen = [SanisGroupRole.SCHOOL_SUPPORT]; return { sanisResponse, @@ -216,7 +214,7 @@ describe('SanisResponseMapper', () => { const result: ExternalGroupDto[] | undefined = mapper.mapToExternalGroupDtos(sanisResponse); - expect(result![0].users).toHaveLength(1); + expect(result![0].users).toHaveLength(0); }); }); diff --git a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts index beaa2927a22..e11e57e9068 100644 --- a/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts +++ b/apps/server/src/modules/provisioning/strategy/sanis/sanis-response.mapper.ts @@ -85,9 +85,7 @@ export class SanisResponseMapper { ktid: source.personenkontexte[0].id, rollen: group.gruppenzugehoerigkeit.rollen, }, - ] - .filter((sanisGroupUser) => sanisGroupUser.ktid && sanisGroupUser.rollen) - .sort((a, b) => a.ktid.localeCompare(b.ktid)); + ].filter((sanisGroupUser) => sanisGroupUser.ktid && sanisGroupUser.rollen); const gruppenzugehoerigkeiten: ExternalGroupUserDto[] = sanisGroupUsers .map((relation): ExternalGroupUserDto | null => this.mapToExternalGroupUser(relation))