From dcb221df5f10e1a757175c95928aa289e5e6beda Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 28 Sep 2023 11:51:26 +0200 Subject: [PATCH 1/7] remove from group: - strategy + spec - service WIP --- .../strategy/oidc/oidc.strategy.spec.ts | 20 +++++++++++++++++++ .../strategy/oidc/oidc.strategy.ts | 6 +++++- .../oidc/service/oidc-provisioning.service.ts | 15 ++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts index 80bdfdf4c88..86192dac09b 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts @@ -198,6 +198,18 @@ describe('OidcStrategy', () => { }; }; + it('should call the OidcProvisioningService.removeUserFromExternalGroups', async () => { + const { oauthData } = setup(); + + await strategy.apply(oauthData); + + expect(oidcProvisioningService.removeUserFromExternalGroups).toHaveBeenCalledWith( + oauthData.externalUser.externalId, + oauthData.externalGroups, + oauthData.system.systemId + ); + }); + it('should call the OidcProvisioningService.provisionExternalGroup for each group', async () => { const { oauthData } = setup(); @@ -241,6 +253,14 @@ describe('OidcStrategy', () => { }; }; + it('should not call the OidcProvisioningService.removeUserFromExternalGroups', async () => { + const { oauthData } = setup(); + + await strategy.apply(oauthData); + + expect(oidcProvisioningService.removeUserFromExternalGroups).not.toHaveBeenCalled(); + }); + it('should not call the OidcProvisioningService.provisionExternalGroup', async () => { const { oauthData } = setup(); diff --git a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts index f51fc49abed..7fe361bfabb 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts @@ -24,7 +24,11 @@ export abstract class OidcProvisioningStrategy extends ProvisioningStrategy { ); if (Configuration.get('FEATURE_SANIS_GROUP_PROVISIONING_ENABLED') && data.externalGroups) { - // TODO: N21-1212 remove user from groups + await this.oidcProvisioningService.removeUserFromExternalGroups( + data.externalUser.externalId, + data.externalGroups, + data.system.systemId + ); await Promise.all( data.externalGroups.map((externalGroup) => 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..179af9eb370 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 @@ -185,4 +185,19 @@ export class OidcProvisioningService { return filteredUsers; } + + async removeUserFromExternalGroups( + externalUserId: EntityId, + externalGroups: ExternalGroupDto[], + systemId: EntityId + ): Promise { + const existingGroupsOfUser: Group[] = await this.groupService.findByUserId(externalUserId); // TODO implement service and repo function + + const groupsWithoutUser: Group[] = await Promise.all( + existingGroupsOfUser.map(async (existingGroup: Group): Promise => { + // TODO check for existingGroup not in externalGroups[] and remove user from this group + }) + ); + // TODO remove all groupsWithoutUser.length === 0 + } } From 976fa19b0192b4efe1f6fdbb9344ff770e8abfe2 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 29 Sep 2023 03:53:47 +0200 Subject: [PATCH 2/7] orchestration removal of group and groupUsers --- .../oidc/service/oidc-provisioning.service.ts | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) 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 179af9eb370..ec6d17e0e61 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 @@ -193,11 +193,30 @@ export class OidcProvisioningService { ): Promise { const existingGroupsOfUser: Group[] = await this.groupService.findByUserId(externalUserId); // TODO implement service and repo function - const groupsWithoutUser: Group[] = await Promise.all( - existingGroupsOfUser.map(async (existingGroup: Group): Promise => { - // TODO check for existingGroup not in externalGroups[] and remove user from this group + const externalGroupsExternalSources: ExternalSource[] = externalGroups.forEach( + (externalGroup: ExternalGroupDto): ExternalSource => { + const externalGroupId: ExternalSource = { externalId: externalGroup.externalId, systemId }; + + return externalGroupId; + } + ); + + await Promise.all( + existingGroupsOfUser.map(async (existingGroup: Group): Promise => { + if (existingGroup.externalSource) { + const isGroupWithoutUser = !externalGroupsExternalSources.includes(existingGroup.externalSource); + + if (isGroupWithoutUser) { + await this.groupService.deleteUserFromGroup(externalUserId, existingGroup.externalSource); // TODO implement service and repo function + + const groupUsers: GroupUser[] = await this.groupService.getUsersOfGroup(existingGroup.externalSource); // TODO implement service and repo function + + if (groupUsers.length === 0) { + await this.groupService.deleteGroup(existingGroup.externalSource); // TODO implement service and repo function + } + } + } }) ); - // TODO remove all groupsWithoutUser.length === 0 } } From 3150ac8d115896643c57c24edc5216f4367a758d Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 29 Sep 2023 15:48:33 +0200 Subject: [PATCH 3/7] extract into private function --- .../oidc/service/oidc-provisioning.service.ts | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) 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 ec6d17e0e61..7da1c11f43d 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 @@ -201,22 +201,24 @@ export class OidcProvisioningService { } ); - await Promise.all( - existingGroupsOfUser.map(async (existingGroup: Group): Promise => { - if (existingGroup.externalSource) { - const isGroupWithoutUser = !externalGroupsExternalSources.includes(existingGroup.externalSource); + existingGroupsOfUser.map(async (existingGroup: Group): Promise => { + if (existingGroup.externalSource) { + const isGroupWithoutUser = !externalGroupsExternalSources.includes(existingGroup.externalSource); - if (isGroupWithoutUser) { - await this.groupService.deleteUserFromGroup(externalUserId, existingGroup.externalSource); // TODO implement service and repo function + if (isGroupWithoutUser) { + await this.removeUserFromGroup(externalUserId, existingGroup.externalSource); + } + } + }); + } - const groupUsers: GroupUser[] = await this.groupService.getUsersOfGroup(existingGroup.externalSource); // TODO implement service and repo function + private async removeUserFromGroup(externalUserId: string, externalSource: ExternalSource): Promise { + await this.groupService.deleteUserFromGroup(externalUserId, externalSource); // TODO implement service and repo function - if (groupUsers.length === 0) { - await this.groupService.deleteGroup(existingGroup.externalSource); // TODO implement service and repo function - } - } - } - }) - ); + const groupUsers: GroupUser[] = await this.groupService.getUsersOfGroup(externalSource); // TODO implement service and repo function + + if (groupUsers.length === 0) { + await this.groupService.deleteGroup(externalSource); // TODO implement service and repo function + } } } From db059a7a5924bfc8f9ecf048effbb1594a550c87 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 4 Oct 2023 15:30:00 +0200 Subject: [PATCH 4/7] extract into private function --- .../src/modules/group/domain/group.spec.ts | 138 ++++++++++++ apps/server/src/modules/group/domain/group.ts | 10 +- .../src/modules/group/repo/group.repo.spec.ts | 69 +++++- .../src/modules/group/repo/group.repo.ts | 18 +- .../group/service/group.service.spec.ts | 47 ++++- .../modules/group/service/group.service.ts | 8 +- .../service/oidc-provisioning.service.spec.ts | 199 +++++++++++++++++- .../oidc/service/oidc-provisioning.service.ts | 47 +++-- .../not-found.loggable-exception.ts | 2 +- 9 files changed, 509 insertions(+), 29 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..b5ae8a03321 --- /dev/null +++ b/apps/server/src/modules/group/domain/group.spec.ts @@ -0,0 +1,138 @@ +import { groupFactory, roleFactory, userDoFactory } from '@shared/testing'; + +import { ObjectId } from 'bson'; +import { RoleReference, UserDO } from '@shared/domain'; +import { Group } from './group'; +import { GroupUser } from './group-user'; + +describe('Group (Domain Object)', () => { + describe('removeUser', () => { + describe('when the user is in the group', () => { + const setup = () => { + const user: UserDO = userDoFactory.buildWithId(); + const groupUser1 = new GroupUser({ + userId: user.id as string, + roleId: new ObjectId().toHexString(), + }); + const groupUser2 = new GroupUser({ + userId: new ObjectId().toHexString(), + roleId: new ObjectId().toHexString(), + }); + const group: Group = groupFactory.build({ + users: [groupUser1, groupUser2], + }); + + return { + user, + groupUser1, + groupUser2, + group, + }; + }; + + it('should remove the user', () => { + const { user, group, groupUser1 } = setup(); + + group.removeUser(user); + + expect(group.users).not.toContain(groupUser1); + }); + + it('should keep all other users', () => { + const { user, group, groupUser2 } = setup(); + + group.removeUser(user); + + expect(group.users).toContain(groupUser2); + }); + }); + + describe('when the user is not in the group', () => { + const setup = () => { + const user: UserDO = userDoFactory.buildWithId(); + const groupUser2 = new GroupUser({ + userId: new ObjectId().toHexString(), + roleId: new ObjectId().toHexString(), + }); + const group: Group = groupFactory.build({ + users: [groupUser2], + }); + + return { + user, + groupUser2, + group, + }; + }; + + it('should do nothing', () => { + const { user, group, groupUser2 } = setup(); + + group.removeUser(user); + + expect(group.users).toEqual([groupUser2]); + }); + }); + + describe('when the group is empty', () => { + const setup = () => { + const user: UserDO = userDoFactory.buildWithId(); + const group: Group = groupFactory.build({ users: [] }); + + return { + user, + group, + }; + }; + + it('should stay empty', () => { + const { user, group } = setup(); + + group.removeUser(user); + + expect(group.users).toEqual([]); + }); + }); + }); + + describe('isEmpty', () => { + describe('when no users in group exist', () => { + const setup = () => { + const group: Group = groupFactory.build({ users: [] }); + + return { + group, + }; + }; + + it('should return true', () => { + const { group } = setup(); + + const isEmpty = group.isEmpty(); + + expect(isEmpty).toEqual(true); + }); + }); + + describe('when users in group exist', () => { + const setup = () => { + const externalUserId = 'externalUserId'; + const role: RoleReference = roleFactory.buildWithId(); + const user: UserDO = userDoFactory.buildWithId({ roles: [role], externalId: externalUserId }); + const group: Group = groupFactory.build({ users: [{ userId: user.id as string, roleId: role.id }] }); + + return { + group, + }; + }; + + it('should return false', () => { + const { group } = setup(); + + const isEmpty = group.isEmpty(); + + expect(isEmpty).toEqual(false); + }); + }); + }); +}); diff --git a/apps/server/src/modules/group/domain/group.ts b/apps/server/src/modules/group/domain/group.ts index cbc5a416ffe..049043618ac 100644 --- a/apps/server/src/modules/group/domain/group.ts +++ b/apps/server/src/modules/group/domain/group.ts @@ -1,4 +1,4 @@ -import { EntityId, ExternalSource } from '@shared/domain'; +import { EntityId, ExternalSource, type UserDO } from '@shared/domain'; import { AuthorizableObject, DomainObject } from '@shared/domain/domain-object'; import { GroupTypes } from './group-types'; import { GroupUser } from './group-user'; @@ -37,4 +37,12 @@ export class Group extends DomainObject { get organizationId(): string | undefined { return this.props.organizationId; } + + removeUser(user: UserDO): void { + this.props.users = this.props.users.filter((groupUser: GroupUser): boolean => groupUser.userId !== user.id); + } + + isEmpty(): boolean { + return this.props.users.length === 0; + } } diff --git a/apps/server/src/modules/group/repo/group.repo.spec.ts b/apps/server/src/modules/group/repo/group.repo.spec.ts index 6b7c9daf741..8c15ce15fe9 100644 --- a/apps/server/src/modules/group/repo/group.repo.spec.ts +++ b/apps/server/src/modules/group/repo/group.repo.spec.ts @@ -1,11 +1,20 @@ import { EntityManager, ObjectId } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; -import { ExternalSource, SchoolEntity } from '@shared/domain'; +import { ExternalSource, SchoolEntity, UserDO, User } from '@shared/domain'; import { MongoMemoryDatabaseModule } from '@shared/infra/database'; -import { cleanupCollections, groupEntityFactory, groupFactory, schoolFactory } from '@shared/testing'; +import { + cleanupCollections, + groupEntityFactory, + groupFactory, + roleFactory, + schoolFactory, + userDoFactory, + userFactory, +} from '@shared/testing'; import { Group, GroupProps, GroupTypes, GroupUser } from '../domain'; import { GroupEntity, GroupEntityTypes } from '../entity'; import { GroupRepo } from './group.repo'; +import { SortHelper } from '../util'; describe('GroupRepo', () => { let module: TestingModule; @@ -82,6 +91,62 @@ describe('GroupRepo', () => { }); }); + describe('findByUser', () => { + describe('when the user has groups', () => { + const setup = async () => { + const userEntity: User = userFactory.buildWithId(); + const user: UserDO = userDoFactory.build({ id: userEntity.id }); + const groups: GroupEntity[] = groupEntityFactory.buildListWithId(3, { + users: [{ user: userEntity, role: roleFactory.buildWithId() }], + }); + + const otherGroups: GroupEntity[] = groupEntityFactory.buildListWithId(2); + + await em.persistAndFlush([userEntity, ...groups, ...otherGroups]); + em.clear(); + + return { + user, + groups, + }; + }; + + it('should return the groups', async () => { + const { user, groups } = await setup(); + + const result: Group[] = await repo.findByUser(user); + + expect(result.map((group) => group.id).sort((a, b) => a.localeCompare(b))).toEqual( + groups.map((group) => group.id).sort((a, b) => a.localeCompare(b)) + ); + }); + }); + + describe('when the user has no groups exists', () => { + const setup = async () => { + const userEntity: User = userFactory.buildWithId(); + const user: UserDO = userDoFactory.build({ id: userEntity.id }); + + const otherGroups: GroupEntity[] = groupEntityFactory.buildListWithId(2); + + await em.persistAndFlush([userEntity, ...otherGroups]); + em.clear(); + + return { + user, + }; + }; + + it('should return an empty array', async () => { + const { user } = await setup(); + + const result: Group[] = await repo.findByUser(user); + + expect(result).toHaveLength(0); + }); + }); + }); + describe('findClassesForSchool', () => { describe('when groups of type class for the school exist', () => { const setup = async () => { diff --git a/apps/server/src/modules/group/repo/group.repo.ts b/apps/server/src/modules/group/repo/group.repo.ts index 2c920b9a39d..6f96fd02da5 100644 --- a/apps/server/src/modules/group/repo/group.repo.ts +++ b/apps/server/src/modules/group/repo/group.repo.ts @@ -1,6 +1,8 @@ -import { EntityManager } from '@mikro-orm/mongodb'; +import { EntityManager, ObjectId } from '@mikro-orm/mongodb'; import { Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain/types'; +import { type UserDO } from '@shared/domain'; +import { FilterQuery } from '@mikro-orm/core'; import { Group, GroupProps } from '../domain'; import { GroupEntity, GroupEntityProps, GroupEntityTypes } from '../entity'; import { GroupDomainMapper } from './group-domain.mapper'; @@ -42,6 +44,20 @@ export class GroupRepo { return domainObject; } + public async findByUser(user: UserDO): Promise { + const entities: GroupEntity[] = await this.em.find(GroupEntity, { + users: { user: new ObjectId(user.id) }, + }); + + const domainObjects = entities.map((entity) => { + const props: GroupProps = GroupDomainMapper.mapEntityToDomainObjectProperties(entity); + + return new Group(props); + }); + + return domainObjects; + } + public async findClassesForSchool(schoolId: EntityId): Promise { const entities: GroupEntity[] = await this.em.find(GroupEntity, { type: GroupEntityTypes.CLASS, diff --git a/apps/server/src/modules/group/service/group.service.spec.ts b/apps/server/src/modules/group/service/group.service.spec.ts index 71cc9eaeb6a..19c66f266ee 100644 --- a/apps/server/src/modules/group/service/group.service.spec.ts +++ b/apps/server/src/modules/group/service/group.service.spec.ts @@ -2,7 +2,8 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { ObjectId } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; import { NotFoundLoggableException } from '@shared/common/loggable-exception'; -import { groupFactory } from '@shared/testing'; +import { groupFactory, userDoFactory } from '@shared/testing'; +import { UserDO } from '@shared/domain'; import { Group } from '../domain'; import { GroupRepo } from '../repo'; import { GroupService } from './group.service'; @@ -120,6 +121,50 @@ describe('GroupService', () => { }); }); + describe('findByUser', () => { + describe('when groups with the user exists', () => { + const setup = () => { + const user: UserDO = userDoFactory.buildWithId(); + const groups: Group[] = groupFactory.buildList(2); + + groupRepo.findByUser.mockResolvedValue(groups); + + return { + user, + groups, + }; + }; + + it('should return the groups', async () => { + const { user, groups } = setup(); + + const result: Group[] = await service.findByUser(user); + + expect(result).toEqual(groups); + }); + }); + + describe('when no groups with the user exists', () => { + const setup = () => { + const user: UserDO = userDoFactory.buildWithId(); + + groupRepo.findByUser.mockResolvedValue([]); + + return { + user, + }; + }; + + it('should return empty array', async () => { + const { user } = setup(); + + const result: Group[] = await service.findByUser(user); + + expect(result).toEqual([]); + }); + }); + }); + describe('findClassesForSchool', () => { describe('when the school has groups of type class', () => { const setup = () => { diff --git a/apps/server/src/modules/group/service/group.service.ts b/apps/server/src/modules/group/service/group.service.ts index dcba9377de3..f3ce6a287e8 100644 --- a/apps/server/src/modules/group/service/group.service.ts +++ b/apps/server/src/modules/group/service/group.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; import { NotFoundLoggableException } from '@shared/common/loggable-exception'; -import { EntityId } from '@shared/domain'; +import { EntityId, type UserDO } from '@shared/domain'; import { AuthorizationLoaderServiceGeneric } from '@src/modules/authorization'; import { Group } from '../domain'; import { GroupRepo } from '../repo'; @@ -31,6 +31,12 @@ export class GroupService implements AuthorizationLoaderServiceGeneric { return group; } + public async findByUser(user: UserDO): Promise { + const groups: Group[] = await this.groupRepo.findByUser(user); + + return groups; + } + public async findClassesForSchool(schoolId: EntityId): Promise { const group: Group[] = await this.groupRepo.findClassesForSchool(schoolId); 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..97104cfd3b4 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 @@ -1,7 +1,7 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { UnprocessableEntityException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; -import { LegacySchoolDo, RoleName, SchoolFeatures } from '@shared/domain'; +import { ExternalSource, LegacySchoolDo, RoleName, RoleReference, SchoolFeatures } from '@shared/domain'; import { UserDO } from '@shared/domain/domainobject/user.do'; import { externalGroupDtoFactory, @@ -11,6 +11,7 @@ import { legacySchoolDoFactory, schoolYearFactory, userDoFactory, + roleFactory, } from '@shared/testing'; import { Logger } from '@src/core/logger'; import { AccountService } from '@src/modules/account/services/account.service'; @@ -21,6 +22,7 @@ import { RoleDto } from '@src/modules/role/service/dto/role.dto'; import { FederalStateService, LegacySchoolService, SchoolYearService } from '@src/modules/legacy-school'; import { UserService } from '@src/modules/user'; import CryptoJS from 'crypto-js'; +import { NotFoundLoggableException } from '@shared/common/loggable-exception'; import { ExternalGroupDto, ExternalSchoolDto, ExternalUserDto } from '../../../dto'; import { SchoolForGroupNotFoundLoggable, UserForGroupNotFoundLoggable } from '../../../loggable'; import { OidcProvisioningService } from './oidc-provisioning.service'; @@ -342,6 +344,201 @@ describe('OidcProvisioningService', () => { }); describe('provisionExternalGroup', () => { + describe('when group membership of user has not changed', () => { + const setup = () => { + const systemId = 'systemId'; + const externalUserId = 'externalUserId'; + const role: RoleReference = roleFactory.buildWithId(); + const user: UserDO = userDoFactory.buildWithId({ roles: [role], externalId: externalUserId }); + + const existingGroups: Group[] = groupFactory.buildList(2, { + users: [{ userId: user.id as string, roleId: role.id }], + }); + + const firstExternalGroup: ExternalGroupDto = externalGroupDtoFactory.build({ + externalId: existingGroups[0].externalSource?.externalId, + users: [{ externalUserId, roleName: role.name }], + }); + const secondExternalGroup: ExternalGroupDto = externalGroupDtoFactory.build({ + externalId: existingGroups[1].externalSource?.externalId, + users: [{ externalUserId, roleName: role.name }], + }); + const externalGroups: ExternalGroupDto[] = [firstExternalGroup, secondExternalGroup]; + + userService.findByExternalId.mockResolvedValue(user); + groupService.findByUser.mockResolvedValue(existingGroups); + + return { + externalGroups, + systemId, + externalUserId, + }; + }; + + it('should not call groupService.save', async () => { + const { externalGroups, systemId, externalUserId } = setup(); + + await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + + expect(groupService.save).not.toHaveBeenCalled(); + }); + + it('should not call groupService.delete', async () => { + const { externalGroups, systemId, externalUserId } = setup(); + + await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + + expect(groupService.delete).not.toHaveBeenCalled(); + }); + }); + + describe('when user is not part of a group anymore', () => { + describe('when group is empty after removal of the User', () => { + const setup = () => { + const systemId = 'systemId'; + const externalUserId = 'externalUserId'; + const role: RoleReference = roleFactory.buildWithId(); + const user: UserDO = userDoFactory.buildWithId({ roles: [role], externalId: externalUserId }); + + const firstExistingGroup: Group = groupFactory.build({ + users: [{ userId: user.id as string, roleId: role.id }], + externalSource: new ExternalSource({ + externalId: 'externalId-1', + systemId, + }), + }); + const secondExistingGroup: Group = groupFactory.build({ + users: [{ userId: user.id as string, roleId: role.id }], + externalSource: new ExternalSource({ + externalId: 'externalId-2', + systemId, + }), + }); + const existingGroups = [firstExistingGroup, secondExistingGroup]; + + const firstExternalGroup: ExternalGroupDto = externalGroupDtoFactory.build({ + externalId: existingGroups[0].externalSource?.externalId, + users: [{ externalUserId, roleName: role.name }], + }); + const externalGroups: ExternalGroupDto[] = [firstExternalGroup]; + + userService.findByExternalId.mockResolvedValue(user); + groupService.findByUser.mockResolvedValue(existingGroups); + + return { + externalGroups, + systemId, + externalUserId, + existingGroups, + }; + }; + + it('should call groupService.delete', async () => { + const { externalGroups, systemId, externalUserId, existingGroups } = setup(); + + await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + + expect(groupService.delete).toHaveBeenCalledWith(existingGroups[1]); + }); + it('should not call groupService.save', async () => { + const { externalGroups, systemId, externalUserId } = setup(); + + await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + + expect(groupService.save).not.toHaveBeenCalled(); + }); + }); + describe('when group is not empty after removal of the User', () => { + const setup = () => { + const systemId = 'systemId'; + const externalUserId = 'externalUserId'; + const anotherExternalUserId = 'anotherExternalUserId'; + const role: RoleReference = roleFactory.buildWithId(); + const user: UserDO = userDoFactory.buildWithId({ roles: [role], externalId: externalUserId }); + const anotherUser: UserDO = userDoFactory.buildWithId({ roles: [role], externalId: anotherExternalUserId }); + + const firstExistingGroup: Group = groupFactory.build({ + users: [ + { userId: user.id as string, roleId: role.id }, + { userId: anotherUser.id as string, roleId: role.id }, + ], + externalSource: new ExternalSource({ + externalId: `externalId-1`, + systemId, + }), + }); + + const secondExistingGroup: Group = groupFactory.build({ + users: [ + { userId: user.id as string, roleId: role.id }, + { userId: anotherUser.id as string, roleId: role.id }, + ], + externalSource: new ExternalSource({ + externalId: `externalId-2`, + systemId, + }), + }); + + const existingGroups: Group[] = [firstExistingGroup, secondExistingGroup]; + + const firstExternalGroup: ExternalGroupDto = externalGroupDtoFactory.build({ + externalId: existingGroups[0].externalSource?.externalId, + users: [{ externalUserId, roleName: role.name }], + }); + const externalGroups: ExternalGroupDto[] = [firstExternalGroup]; + + userService.findByExternalId.mockResolvedValue(user); + groupService.findByUser.mockResolvedValue(existingGroups); + + return { + externalGroups, + systemId, + externalUserId, + existingGroups, + }; + }; + + it('should call groupService.save', async () => { + const { externalGroups, systemId, externalUserId, existingGroups } = setup(); + + await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + + expect(groupService.save).toHaveBeenCalledWith(existingGroups[1]); + }); + it('should not call groupService.delete', async () => { + const { externalGroups, systemId, externalUserId } = setup(); + + await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + + expect(groupService.delete).not.toHaveBeenCalled(); + }); + }); + }); + + describe('when user could not be found', () => { + const setup = () => { + const systemId = 'systemId'; + const externalUserId = 'externalUserId'; + const externalGroups: ExternalGroupDto[] = [externalGroupDtoFactory.build()]; + + userService.findByExternalId.mockResolvedValue(null); + + return { + systemId, + externalUserId, + externalGroups, + }; + }; + + it('should throw NotFoundLoggableException', async () => { + const { externalGroups, systemId, externalUserId } = setup(); + + const func = async () => service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + + await expect(func).rejects.toThrow(new NotFoundLoggableException('User', 'externalId', externalUserId)); + }); + }); + describe('when the group has no users', () => { const setup = () => { const externalGroupDto: ExternalGroupDto = externalGroupDtoFactory.build({ users: [] }); 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 7da1c11f43d..eca41da6c46 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 @@ -12,6 +12,8 @@ import { RoleDto } from '@src/modules/role/service/dto/role.dto'; import { UserService } from '@src/modules/user'; import { ObjectId } from 'bson'; import CryptoJS from 'crypto-js'; +import { notFound } from '@feathersjs/express'; +import { NotFoundLoggableException } from '@shared/common/loggable-exception'; import { ExternalGroupDto, ExternalGroupUserDto, ExternalSchoolDto, ExternalUserDto } from '../../../dto'; import { SchoolForGroupNotFoundLoggable, UserForGroupNotFoundLoggable } from '../../../loggable'; @@ -191,34 +193,37 @@ export class OidcProvisioningService { externalGroups: ExternalGroupDto[], systemId: EntityId ): Promise { - const existingGroupsOfUser: Group[] = await this.groupService.findByUserId(externalUserId); // TODO implement service and repo function + const user: UserDO | null = await this.userService.findByExternalId(externalUserId, systemId); - const externalGroupsExternalSources: ExternalSource[] = externalGroups.forEach( - (externalGroup: ExternalGroupDto): ExternalSource => { - const externalGroupId: ExternalSource = { externalId: externalGroup.externalId, systemId }; + if (!user) { + throw new NotFoundLoggableException('User', 'externalId', externalUserId); + } - return externalGroupId; - } + const existingGroupsOfUser: Group[] = await this.groupService.findByUser(user); + + const groupsFromSystem: Group[] = existingGroupsOfUser.filter( + (existingGroup: Group) => existingGroup.externalSource?.systemId === systemId ); - existingGroupsOfUser.map(async (existingGroup: Group): Promise => { - if (existingGroup.externalSource) { - const isGroupWithoutUser = !externalGroupsExternalSources.includes(existingGroup.externalSource); + const groupsWithoutUser: Group[] = groupsFromSystem.filter((existingGroupFromSystem: Group) => { + const isUserInGroup = externalGroups.some( + (externalGroup: ExternalGroupDto) => + externalGroup.externalId === existingGroupFromSystem.externalSource?.externalId + ); - if (isGroupWithoutUser) { - await this.removeUserFromGroup(externalUserId, existingGroup.externalSource); - } - } + return !isUserInGroup; }); - } - - private async removeUserFromGroup(externalUserId: string, externalSource: ExternalSource): Promise { - await this.groupService.deleteUserFromGroup(externalUserId, externalSource); // TODO implement service and repo function - const groupUsers: GroupUser[] = await this.groupService.getUsersOfGroup(externalSource); // TODO implement service and repo function + await Promise.all( + groupsWithoutUser.map(async (group: Group) => { + group.removeUser(user); - if (groupUsers.length === 0) { - await this.groupService.deleteGroup(externalSource); // TODO implement service and repo function - } + if (group.isEmpty()) { + await this.groupService.delete(group); + } else { + await this.groupService.save(group); + } + }) + ); } } diff --git a/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts b/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts index 261f4161a30..18207e99e02 100644 --- a/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts +++ b/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts @@ -7,7 +7,7 @@ export class NotFoundLoggableException extends NotFoundException implements Logg constructor( private readonly resourceName: string, private readonly identifierName: string, - private readonly resourceId: EntityId + private readonly resourceId: string ) { super(); } From c33c869077c99ac4bcb6c69400857203a98a84d5 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 5 Oct 2023 12:33:49 +0200 Subject: [PATCH 5/7] change scope of unique members assertion to fix current classes --- .../strategy/oidc/oidc.strategy.spec.ts | 8 ++--- .../strategy/oidc/oidc.strategy.ts | 2 +- .../service/oidc-provisioning.service.spec.ts | 29 ++++++++++--------- .../oidc/service/oidc-provisioning.service.ts | 4 +-- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts index 86192dac09b..34d176dde41 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts @@ -198,12 +198,12 @@ describe('OidcStrategy', () => { }; }; - it('should call the OidcProvisioningService.removeUserFromExternalGroups', async () => { + it('should call the OidcProvisioningService.removeExternalGroupsAndAffiliation', async () => { const { oauthData } = setup(); await strategy.apply(oauthData); - expect(oidcProvisioningService.removeUserFromExternalGroups).toHaveBeenCalledWith( + expect(oidcProvisioningService.removeExternalGroupsAndAffiliation).toHaveBeenCalledWith( oauthData.externalUser.externalId, oauthData.externalGroups, oauthData.system.systemId @@ -253,12 +253,12 @@ describe('OidcStrategy', () => { }; }; - it('should not call the OidcProvisioningService.removeUserFromExternalGroups', async () => { + it('should not call the OidcProvisioningService.removeExternalGroupsAndAffiliation', async () => { const { oauthData } = setup(); await strategy.apply(oauthData); - expect(oidcProvisioningService.removeUserFromExternalGroups).not.toHaveBeenCalled(); + expect(oidcProvisioningService.removeExternalGroupsAndAffiliation).not.toHaveBeenCalled(); }); it('should not call the OidcProvisioningService.provisionExternalGroup', async () => { diff --git a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts index 7fe361bfabb..7804f2190f9 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.ts @@ -24,7 +24,7 @@ export abstract class OidcProvisioningStrategy extends ProvisioningStrategy { ); if (Configuration.get('FEATURE_SANIS_GROUP_PROVISIONING_ENABLED') && data.externalGroups) { - await this.oidcProvisioningService.removeUserFromExternalGroups( + await this.oidcProvisioningService.removeExternalGroupsAndAffiliation( data.externalUser.externalId, data.externalGroups, data.system.systemId 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 97104cfd3b4..c4b27cac34b 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 @@ -375,18 +375,18 @@ describe('OidcProvisioningService', () => { }; }; - it('should not call groupService.save', async () => { + it('should not save the group', async () => { const { externalGroups, systemId, externalUserId } = setup(); - await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + await service.removeExternalGroupsAndAffiliation(externalUserId, externalGroups, systemId); expect(groupService.save).not.toHaveBeenCalled(); }); - it('should not call groupService.delete', async () => { + it('should not delete the group', async () => { const { externalGroups, systemId, externalUserId } = setup(); - await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + await service.removeExternalGroupsAndAffiliation(externalUserId, externalGroups, systemId); expect(groupService.delete).not.toHaveBeenCalled(); }); @@ -433,21 +433,23 @@ describe('OidcProvisioningService', () => { }; }; - it('should call groupService.delete', async () => { + it('should delete the group', async () => { const { externalGroups, systemId, externalUserId, existingGroups } = setup(); - await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + await service.removeExternalGroupsAndAffiliation(externalUserId, externalGroups, systemId); expect(groupService.delete).toHaveBeenCalledWith(existingGroups[1]); }); - it('should not call groupService.save', async () => { + + it('should not save the group', async () => { const { externalGroups, systemId, externalUserId } = setup(); - await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + await service.removeExternalGroupsAndAffiliation(externalUserId, externalGroups, systemId); expect(groupService.save).not.toHaveBeenCalled(); }); }); + describe('when group is not empty after removal of the User', () => { const setup = () => { const systemId = 'systemId'; @@ -498,17 +500,18 @@ describe('OidcProvisioningService', () => { }; }; - it('should call groupService.save', async () => { + it('should save the group', async () => { const { externalGroups, systemId, externalUserId, existingGroups } = setup(); - await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + await service.removeExternalGroupsAndAffiliation(externalUserId, externalGroups, systemId); expect(groupService.save).toHaveBeenCalledWith(existingGroups[1]); }); - it('should not call groupService.delete', async () => { + + it('should not delete the group', async () => { const { externalGroups, systemId, externalUserId } = setup(); - await service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + await service.removeExternalGroupsAndAffiliation(externalUserId, externalGroups, systemId); expect(groupService.delete).not.toHaveBeenCalled(); }); @@ -533,7 +536,7 @@ describe('OidcProvisioningService', () => { it('should throw NotFoundLoggableException', async () => { const { externalGroups, systemId, externalUserId } = setup(); - const func = async () => service.removeUserFromExternalGroups(externalUserId, externalGroups, systemId); + const func = async () => service.removeExternalGroupsAndAffiliation(externalUserId, externalGroups, systemId); await expect(func).rejects.toThrow(new NotFoundLoggableException('User', 'externalId', externalUserId)); }); 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 eca41da6c46..ea7e81f3c84 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 @@ -188,7 +188,7 @@ export class OidcProvisioningService { return filteredUsers; } - async removeUserFromExternalGroups( + async removeExternalGroupsAndAffiliation( externalUserId: EntityId, externalGroups: ExternalGroupDto[], systemId: EntityId @@ -196,7 +196,7 @@ export class OidcProvisioningService { const user: UserDO | null = await this.userService.findByExternalId(externalUserId, systemId); if (!user) { - throw new NotFoundLoggableException('User', 'externalId', externalUserId); + throw new NotFoundLoggableException(UserDO.name, 'externalId', externalUserId); } const existingGroupsOfUser: Group[] = await this.groupService.findByUser(user); From 91b3c2dc35f2bed9663ee07207ac56bd47190487 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 5 Oct 2023 12:56:25 +0200 Subject: [PATCH 6/7] fix imports --- apps/server/src/modules/group/repo/group.repo.spec.ts | 1 - apps/server/src/modules/group/repo/group.repo.ts | 1 - .../strategy/oidc/service/oidc-provisioning.service.ts | 1 - .../common/loggable-exception/not-found.loggable-exception.ts | 1 - 4 files changed, 4 deletions(-) diff --git a/apps/server/src/modules/group/repo/group.repo.spec.ts b/apps/server/src/modules/group/repo/group.repo.spec.ts index 8c15ce15fe9..358b3c13983 100644 --- a/apps/server/src/modules/group/repo/group.repo.spec.ts +++ b/apps/server/src/modules/group/repo/group.repo.spec.ts @@ -14,7 +14,6 @@ import { import { Group, GroupProps, GroupTypes, GroupUser } from '../domain'; import { GroupEntity, GroupEntityTypes } from '../entity'; import { GroupRepo } from './group.repo'; -import { SortHelper } from '../util'; describe('GroupRepo', () => { let module: TestingModule; diff --git a/apps/server/src/modules/group/repo/group.repo.ts b/apps/server/src/modules/group/repo/group.repo.ts index 6f96fd02da5..920647ee7e8 100644 --- a/apps/server/src/modules/group/repo/group.repo.ts +++ b/apps/server/src/modules/group/repo/group.repo.ts @@ -2,7 +2,6 @@ import { EntityManager, ObjectId } from '@mikro-orm/mongodb'; import { Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain/types'; import { type UserDO } from '@shared/domain'; -import { FilterQuery } from '@mikro-orm/core'; import { Group, GroupProps } from '../domain'; import { GroupEntity, GroupEntityProps, GroupEntityTypes } from '../entity'; import { GroupDomainMapper } from './group-domain.mapper'; 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 ea7e81f3c84..0aef3fdecb9 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 @@ -12,7 +12,6 @@ import { RoleDto } from '@src/modules/role/service/dto/role.dto'; import { UserService } from '@src/modules/user'; import { ObjectId } from 'bson'; import CryptoJS from 'crypto-js'; -import { notFound } from '@feathersjs/express'; import { NotFoundLoggableException } from '@shared/common/loggable-exception'; import { ExternalGroupDto, ExternalGroupUserDto, ExternalSchoolDto, ExternalUserDto } from '../../../dto'; import { SchoolForGroupNotFoundLoggable, UserForGroupNotFoundLoggable } from '../../../loggable'; diff --git a/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts b/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts index 18207e99e02..4ffd8e5b70e 100644 --- a/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts +++ b/apps/server/src/shared/common/loggable-exception/not-found.loggable-exception.ts @@ -1,5 +1,4 @@ import { NotFoundException } from '@nestjs/common'; -import { EntityId } from '@shared/domain'; import { Loggable } from '@src/core/logger/interfaces'; import { ErrorLogMessage } from '@src/core/logger/types'; From 39533d5ed829b895d3b2b2a1e446e102d2971df8 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 5 Oct 2023 15:26:25 +0200 Subject: [PATCH 7/7] rename testcases --- .../provisioning/strategy/oidc/oidc.strategy.spec.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts index 34d176dde41..2b838159524 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/oidc.strategy.spec.ts @@ -98,7 +98,7 @@ describe('OidcStrategy', () => { }; }; - it('should call the OidcProvisioningService.provisionExternalSchool', async () => { + it('should provision school', async () => { const { oauthData } = setup(); await strategy.apply(oauthData); @@ -150,7 +150,7 @@ describe('OidcStrategy', () => { }; }; - it('should call the OidcProvisioningService.provisionExternalUser', async () => { + it('should provision external user', async () => { const { oauthData, schoolId } = setup(); await strategy.apply(oauthData); @@ -198,7 +198,7 @@ describe('OidcStrategy', () => { }; }; - it('should call the OidcProvisioningService.removeExternalGroupsAndAffiliation', async () => { + it('should remove external groups and affiliation', async () => { const { oauthData } = setup(); await strategy.apply(oauthData); @@ -210,7 +210,7 @@ describe('OidcStrategy', () => { ); }); - it('should call the OidcProvisioningService.provisionExternalGroup for each group', async () => { + it('should provision every external group', async () => { const { oauthData } = setup(); await strategy.apply(oauthData); @@ -253,7 +253,7 @@ describe('OidcStrategy', () => { }; }; - it('should not call the OidcProvisioningService.removeExternalGroupsAndAffiliation', async () => { + it('should not remove external groups and affiliation', async () => { const { oauthData } = setup(); await strategy.apply(oauthData); @@ -261,7 +261,7 @@ describe('OidcStrategy', () => { expect(oidcProvisioningService.removeExternalGroupsAndAffiliation).not.toHaveBeenCalled(); }); - it('should not call the OidcProvisioningService.provisionExternalGroup', async () => { + it('should not provision groups', async () => { const { oauthData } = setup(); await strategy.apply(oauthData);