From 9b65164af5f2f49e1ae939214e8d7619e73987c7 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 24 Apr 2024 00:16:15 +0200 Subject: [PATCH 1/9] - check email in provisioning - throw loggable --- .../email-already-exists.loggable.spec.ts | 40 +++++++++++++++ .../loggable/email-already-exists.loggable.ts | 36 +++++++++++++ .../modules/provisioning/loggable/index.ts | 1 + ...ulconnex-user-provisioning.service.spec.ts | 50 ++++++++++++++++++- .../schulconnex-user-provisioning.service.ts | 23 ++++++++- 5 files changed, 147 insertions(+), 3 deletions(-) create mode 100644 apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts create mode 100644 apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts diff --git a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts new file mode 100644 index 00000000000..5c7e14b5be4 --- /dev/null +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts @@ -0,0 +1,40 @@ +import { EmailAlreadyExistsLoggable } from '@modules/provisioning/loggable/email-already-exists.loggable'; + +describe('EmailAlreadyExistsLoggableException', () => { + describe('getLogMessage', () => { + const setup = () => { + const email = 'mock-email'; + const systemId = '123'; + const schoolId = '456'; + const externalId = '789'; + + const exception = new EmailAlreadyExistsLoggable(email, systemId, schoolId, externalId); + + return { + exception, + email, + systemId, + schoolId, + externalId, + }; + }; + + it('should return the correct log message', () => { + const { exception, email, systemId, schoolId, externalId } = setup(); + + const message = exception.getLogMessage(); + + expect(message).toEqual({ + type: 'EMAIL_ALREADY_EXISTS', + message: 'The Email to be provisioned already exists.', + stack: expect.any(String), + data: { + email, + systemId, + schoolId, + externalId, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts new file mode 100644 index 00000000000..23e556aa7fd --- /dev/null +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts @@ -0,0 +1,36 @@ +import { HttpStatus } from '@nestjs/common'; +import { BusinessError } from '@shared/common'; +import { EntityId } from '@shared/domain/types'; +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; + +export class EmailAlreadyExistsLoggable extends BusinessError implements Loggable { + constructor( + private readonly email: string, + private readonly systemId: EntityId, + private readonly schoolId?: string, + private readonly externalId?: string + ) { + super( + { + type: 'EMAIL_ALREADY_EXISTS', + title: 'Email already Exists', + defaultMessage: 'The Email to be provisioned already exists.', + }, + HttpStatus.INTERNAL_SERVER_ERROR + ); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: this.type, + message: this.message, + stack: this.stack, + data: { + email: this.email, + systemId: this.systemId, + schoolId: this.schoolId, + externalId: this.externalId, + }, + }; + } +} diff --git a/apps/server/src/modules/provisioning/loggable/index.ts b/apps/server/src/modules/provisioning/loggable/index.ts index a790e846f2d..23542bf1df2 100644 --- a/apps/server/src/modules/provisioning/loggable/index.ts +++ b/apps/server/src/modules/provisioning/loggable/index.ts @@ -1,3 +1,4 @@ export * from './user-for-group-not-found.loggable'; export * from './school-for-group-not-found.loggable'; export * from './group-role-unknown.loggable'; +export { EmailAlreadyExistsLoggable } from './email-already-exists.loggable'; diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts index a9cd98abe0e..6eb4b225801 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts @@ -1,5 +1,6 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { AccountService, AccountSave } from '@modules/account'; +import { AccountSave, AccountService } from '@modules/account'; +import { EmailAlreadyExistsLoggable } from '@modules/provisioning/loggable'; import { RoleService } from '@modules/role'; import { RoleDto } from '@modules/role/service/dto/role.dto'; import { UserService } from '@modules/user'; @@ -71,6 +72,19 @@ describe(SchulconnexUserProvisioningService.name, () => { }, 'userId' ); + const otherUserWithSameEmail: UserDO = userDoFactory + .withRoles([{ id: 'existingRoleId', name: RoleName.USER }]) + .buildWithId( + { + firstName: 'OtherFirstName', + lastName: 'OtherLastName', + email: 'email', + schoolId: 'OtherSchoolId', + externalId: 'OtherUserId', + birthday: new Date('2023-11-15'), + }, + 'otherId' + ); const savedUser: UserDO = userDoFactory.withRoles([{ id: 'roleId', name: RoleName.USER }]).buildWithId( { firstName: 'firstName', @@ -109,6 +123,7 @@ describe(SchulconnexUserProvisioningService.name, () => { return { existingUser, + otherUserWithSameEmail, savedUser, externalUser, userRole, @@ -123,6 +138,7 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); + userService.findByEmail.mockResolvedValue([]); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -133,6 +149,7 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); + userService.findByEmail.mockResolvedValue([]); const result: UserDO = await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -149,6 +166,7 @@ describe(SchulconnexUserProvisioningService.name, () => { } as AccountSave; userService.findByExternalId.mockResolvedValue(null); + userService.findByEmail.mockResolvedValue([]); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -160,12 +178,26 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser } = setupUser(); userService.findByExternalId.mockResolvedValue(null); + userService.findByEmail.mockResolvedValue([]); const promise: Promise = service.provisionExternalUser(externalUser, 'systemId', undefined); await expect(promise).rejects.toThrow(UnprocessableEntityException); }); }); + + describe('when the external user has an email, that already exists in SVS', () => { + it('should throw EmailAlreadyExistsLoggable', async () => { + const { externalUser, systemId, schoolId, otherUserWithSameEmail } = setupUser(); + + userService.findByExternalId.mockResolvedValue(null); + userService.findByEmail.mockResolvedValue([otherUserWithSameEmail]); + + const promise: Promise = service.provisionExternalUser(externalUser, systemId, schoolId); + + await expect(promise).rejects.toThrow(EmailAlreadyExistsLoggable); + }); + }); }); describe('when the user already exists', () => { @@ -173,6 +205,7 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, existingUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); + userService.findByEmail.mockResolvedValue([existingUser]); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -183,6 +216,7 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, existingUser, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); + userService.findByEmail.mockResolvedValue([existingUser]); const result: UserDO = await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -193,11 +227,25 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, systemId, existingUser } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); + userService.findByEmail.mockResolvedValue([existingUser]); await service.provisionExternalUser(externalUser, systemId, schoolId); expect(accountService.saveWithValidation).not.toHaveBeenCalled(); }); + + describe('when the external user has an email, that already exists ijn SVS', () => { + it('should throw EmailAlreadyExistsLoggable', async () => { + const { externalUser, systemId, schoolId, otherUserWithSameEmail, existingUser } = setupUser(); + + userService.findByExternalId.mockResolvedValue(existingUser); + userService.findByEmail.mockResolvedValue([otherUserWithSameEmail]); + + const promise: Promise = service.provisionExternalUser(externalUser, systemId, schoolId); + + await expect(promise).rejects.toThrow(EmailAlreadyExistsLoggable); + }); + }); }); }); }); diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts index 7352ab7323f..a21209ea43a 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts @@ -1,4 +1,5 @@ -import { AccountService, AccountSave } from '@modules/account'; +import { AccountSave, AccountService } from '@modules/account'; +import { EmailAlreadyExistsLoggable } from '@modules/provisioning/loggable'; import { RoleDto, RoleService } from '@modules/role'; import { UserService } from '@modules/user'; import { Injectable, UnprocessableEntityException } from '@nestjs/common'; @@ -20,13 +21,17 @@ export class SchulconnexUserProvisioningService { systemId: EntityId, schoolId?: string ): Promise { + const existingUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); + if (externalUser.email) { + await this.checkUniqueEmail(externalUser.email, systemId, schoolId, existingUser?.externalId); + } + let roleRefs: RoleReference[] | undefined; if (externalUser.roles) { const roles: RoleDto[] = await this.roleService.findByNames(externalUser.roles); roleRefs = roles.map((role: RoleDto): RoleReference => new RoleReference({ id: role.id || '', name: role.name })); } - const existingUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); let user: UserDO; let createNewAccount = false; if (existingUser) { @@ -70,4 +75,18 @@ export class SchulconnexUserProvisioningService { return savedUser; } + + private async checkUniqueEmail( + email: string, + systemId: string, + schoolId?: string, + externalId?: string + ): Promise { + const foundUsers: UserDO[] = await this.userService.findByEmail(email); + const unmatchedUsers: UserDO[] = foundUsers.filter((user: UserDO) => user.externalId !== externalId); + + if (unmatchedUsers.length || (!externalId && foundUsers.length)) { + throw new EmailAlreadyExistsLoggable(email, systemId, schoolId, externalId); + } + } } From e5ab7d9c9dc5c013e49ff88b8dfa582d4c69bb72 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 24 Apr 2024 10:11:12 +0200 Subject: [PATCH 2/9] requested changes 1 --- .../loggable/email-already-exists.loggable.spec.ts | 10 ++-------- .../loggable/email-already-exists.loggable.ts | 10 +--------- .../service/schulconnex-user-provisioning.service.ts | 11 +++-------- 3 files changed, 6 insertions(+), 25 deletions(-) diff --git a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts index 5c7e14b5be4..694a3154868 100644 --- a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts @@ -4,23 +4,19 @@ describe('EmailAlreadyExistsLoggableException', () => { describe('getLogMessage', () => { const setup = () => { const email = 'mock-email'; - const systemId = '123'; - const schoolId = '456'; const externalId = '789'; - const exception = new EmailAlreadyExistsLoggable(email, systemId, schoolId, externalId); + const exception = new EmailAlreadyExistsLoggable(email, externalId); return { exception, email, - systemId, - schoolId, externalId, }; }; it('should return the correct log message', () => { - const { exception, email, systemId, schoolId, externalId } = setup(); + const { exception, email, externalId } = setup(); const message = exception.getLogMessage(); @@ -30,8 +26,6 @@ describe('EmailAlreadyExistsLoggableException', () => { stack: expect.any(String), data: { email, - systemId, - schoolId, externalId, }, }); diff --git a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts index 23e556aa7fd..7d1e92fff00 100644 --- a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts @@ -1,15 +1,9 @@ import { HttpStatus } from '@nestjs/common'; import { BusinessError } from '@shared/common'; -import { EntityId } from '@shared/domain/types'; import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; export class EmailAlreadyExistsLoggable extends BusinessError implements Loggable { - constructor( - private readonly email: string, - private readonly systemId: EntityId, - private readonly schoolId?: string, - private readonly externalId?: string - ) { + constructor(private readonly email: string, private readonly externalId?: string) { super( { type: 'EMAIL_ALREADY_EXISTS', @@ -27,8 +21,6 @@ export class EmailAlreadyExistsLoggable extends BusinessError implements Loggabl stack: this.stack, data: { email: this.email, - systemId: this.systemId, - schoolId: this.schoolId, externalId: this.externalId, }, }; diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts index a21209ea43a..66ffd76a2d1 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts @@ -23,7 +23,7 @@ export class SchulconnexUserProvisioningService { ): Promise { const existingUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); if (externalUser.email) { - await this.checkUniqueEmail(externalUser.email, systemId, schoolId, existingUser?.externalId); + await this.checkUniqueEmail(externalUser.email, existingUser?.externalId); } let roleRefs: RoleReference[] | undefined; @@ -76,17 +76,12 @@ export class SchulconnexUserProvisioningService { return savedUser; } - private async checkUniqueEmail( - email: string, - systemId: string, - schoolId?: string, - externalId?: string - ): Promise { + private async checkUniqueEmail(email: string, externalId?: string): Promise { const foundUsers: UserDO[] = await this.userService.findByEmail(email); const unmatchedUsers: UserDO[] = foundUsers.filter((user: UserDO) => user.externalId !== externalId); if (unmatchedUsers.length || (!externalId && foundUsers.length)) { - throw new EmailAlreadyExistsLoggable(email, systemId, schoolId, externalId); + throw new EmailAlreadyExistsLoggable(email, externalId); } } } From 021f8db724bd70f2200c569b78e4c7649c54d9f4 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 24 Apr 2024 13:32:43 +0200 Subject: [PATCH 3/9] change logic to: - allow login - not change email if not unique - log a warning TODO: give info to client to show problem --- .../email-already-exists.loggable.spec.ts | 10 +- .../loggable/email-already-exists.loggable.ts | 19 +-- ...ulconnex-user-provisioning.service.spec.ts | 79 +++++----- .../schulconnex-user-provisioning.service.ts | 32 ++-- .../modules/user/service/user.service.spec.ts | 145 ++++++++++++++++-- .../src/modules/user/service/user.service.ts | 57 ++++--- 6 files changed, 231 insertions(+), 111 deletions(-) diff --git a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts index 694a3154868..87833c6967c 100644 --- a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts @@ -6,24 +6,22 @@ describe('EmailAlreadyExistsLoggableException', () => { const email = 'mock-email'; const externalId = '789'; - const exception = new EmailAlreadyExistsLoggable(email, externalId); + const loggable = new EmailAlreadyExistsLoggable(email, externalId); return { - exception, + loggable, email, externalId, }; }; it('should return the correct log message', () => { - const { exception, email, externalId } = setup(); + const { loggable, email, externalId } = setup(); - const message = exception.getLogMessage(); + const message = loggable.getLogMessage(); expect(message).toEqual({ - type: 'EMAIL_ALREADY_EXISTS', message: 'The Email to be provisioned already exists.', - stack: expect.any(String), data: { email, externalId, diff --git a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts index 7d1e92fff00..1e4cd7ed77d 100644 --- a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts @@ -1,24 +1,11 @@ -import { HttpStatus } from '@nestjs/common'; -import { BusinessError } from '@shared/common'; import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; -export class EmailAlreadyExistsLoggable extends BusinessError implements Loggable { - constructor(private readonly email: string, private readonly externalId?: string) { - super( - { - type: 'EMAIL_ALREADY_EXISTS', - title: 'Email already Exists', - defaultMessage: 'The Email to be provisioned already exists.', - }, - HttpStatus.INTERNAL_SERVER_ERROR - ); - } +export class EmailAlreadyExistsLoggable implements Loggable { + constructor(private readonly email: string, private readonly externalId?: string) {} getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { return { - type: this.type, - message: this.message, - stack: this.stack, + message: 'The Email to be provisioned already exists.', data: { email: this.email, externalId: this.externalId, diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts index 6eb4b225801..877ae9356a7 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts @@ -1,6 +1,5 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { AccountSave, AccountService } from '@modules/account'; -import { EmailAlreadyExistsLoggable } from '@modules/provisioning/loggable'; import { RoleService } from '@modules/role'; import { RoleDto } from '@modules/role/service/dto/role.dto'; import { UserService } from '@modules/user'; @@ -9,6 +8,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { UserDO } from '@shared/domain/domainobject'; import { RoleName } from '@shared/domain/interface'; import { userDoFactory } from '@shared/testing'; +import { Logger } from '@src/core/logger'; import CryptoJS from 'crypto-js'; import { ExternalUserDto } from '../../../dto'; import { SchulconnexUserProvisioningService } from './schulconnex-user-provisioning.service'; @@ -22,6 +22,7 @@ describe(SchulconnexUserProvisioningService.name, () => { let userService: DeepMocked; let roleService: DeepMocked; let accountService: DeepMocked; + let logger: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -39,6 +40,10 @@ describe(SchulconnexUserProvisioningService.name, () => { provide: AccountService, useValue: createMock(), }, + { + provide: Logger, + useValue: createMock(), + }, ], }).compile(); @@ -46,6 +51,7 @@ describe(SchulconnexUserProvisioningService.name, () => { userService = module.get(UserService); roleService = module.get(RoleService); accountService = module.get(AccountService); + logger = module.get(Logger); }); afterAll(async () => { @@ -72,19 +78,6 @@ describe(SchulconnexUserProvisioningService.name, () => { }, 'userId' ); - const otherUserWithSameEmail: UserDO = userDoFactory - .withRoles([{ id: 'existingRoleId', name: RoleName.USER }]) - .buildWithId( - { - firstName: 'OtherFirstName', - lastName: 'OtherLastName', - email: 'email', - schoolId: 'OtherSchoolId', - externalId: 'OtherUserId', - birthday: new Date('2023-11-15'), - }, - 'otherId' - ); const savedUser: UserDO = userDoFactory.withRoles([{ id: 'roleId', name: RoleName.USER }]).buildWithId( { firstName: 'firstName', @@ -123,7 +116,6 @@ describe(SchulconnexUserProvisioningService.name, () => { return { existingUser, - otherUserWithSameEmail, savedUser, externalUser, userRole, @@ -134,11 +126,21 @@ describe(SchulconnexUserProvisioningService.name, () => { }; describe('when the user does not exist yet', () => { + it('should call user service to check uniqueness of email', async () => { + const { externalUser, schoolId, systemId } = setupUser(); + + userService.findByExternalId.mockResolvedValue(null); + + await service.provisionExternalUser(externalUser, systemId, schoolId); + + expect(userService.isEmailFromExternalSourceUnique).toHaveBeenCalledWith(externalUser.email, undefined); + }); + it('should call the user service to save the user', async () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.findByEmail.mockResolvedValue([]); + userService.isEmailFromExternalSourceUnique.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -149,7 +151,6 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.findByEmail.mockResolvedValue([]); const result: UserDO = await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -166,7 +167,6 @@ describe(SchulconnexUserProvisioningService.name, () => { } as AccountSave; userService.findByExternalId.mockResolvedValue(null); - userService.findByEmail.mockResolvedValue([]); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -178,7 +178,6 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.findByEmail.mockResolvedValue([]); const promise: Promise = service.provisionExternalUser(externalUser, 'systemId', undefined); @@ -187,25 +186,40 @@ describe(SchulconnexUserProvisioningService.name, () => { }); describe('when the external user has an email, that already exists in SVS', () => { - it('should throw EmailAlreadyExistsLoggable', async () => { - const { externalUser, systemId, schoolId, otherUserWithSameEmail } = setupUser(); + it('should log EmailAlreadyExistsLoggable', async () => { + const { externalUser, systemId, schoolId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.findByEmail.mockResolvedValue([otherUserWithSameEmail]); + userService.isEmailFromExternalSourceUnique.mockResolvedValue(false); - const promise: Promise = service.provisionExternalUser(externalUser, systemId, schoolId); + await service.provisionExternalUser(externalUser, systemId, schoolId); - await expect(promise).rejects.toThrow(EmailAlreadyExistsLoggable); + expect(logger.warning).toHaveBeenCalledWith({ + email: externalUser.email, + externalId: externalUser.externalId, + }); }); }); }); describe('when the user already exists', () => { + it('should call user service to check uniqueness of email', async () => { + const { externalUser, schoolId, systemId, existingUser } = setupUser(); + + userService.findByExternalId.mockResolvedValue(existingUser); + + await service.provisionExternalUser(externalUser, systemId, schoolId); + + expect(userService.isEmailFromExternalSourceUnique).toHaveBeenCalledWith( + externalUser.email, + existingUser.externalId + ); + }); + it('should call the user service to save the user', async () => { const { externalUser, schoolId, existingUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); - userService.findByEmail.mockResolvedValue([existingUser]); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -216,7 +230,6 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, existingUser, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); - userService.findByEmail.mockResolvedValue([existingUser]); const result: UserDO = await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -227,25 +240,11 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, systemId, existingUser } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); - userService.findByEmail.mockResolvedValue([existingUser]); await service.provisionExternalUser(externalUser, systemId, schoolId); expect(accountService.saveWithValidation).not.toHaveBeenCalled(); }); - - describe('when the external user has an email, that already exists ijn SVS', () => { - it('should throw EmailAlreadyExistsLoggable', async () => { - const { externalUser, systemId, schoolId, otherUserWithSameEmail, existingUser } = setupUser(); - - userService.findByExternalId.mockResolvedValue(existingUser); - userService.findByEmail.mockResolvedValue([otherUserWithSameEmail]); - - const promise: Promise = service.provisionExternalUser(externalUser, systemId, schoolId); - - await expect(promise).rejects.toThrow(EmailAlreadyExistsLoggable); - }); - }); }); }); }); diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts index 66ffd76a2d1..ed330af9c1e 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts @@ -5,6 +5,7 @@ import { UserService } from '@modules/user'; import { Injectable, UnprocessableEntityException } from '@nestjs/common'; import { RoleReference, UserDO } from '@shared/domain/domainobject'; import { EntityId } from '@shared/domain/types'; +import { Logger } from '@src/core/logger'; import CryptoJS from 'crypto-js'; import { ExternalUserDto } from '../../../dto'; @@ -13,7 +14,8 @@ export class SchulconnexUserProvisioningService { constructor( private readonly userService: UserService, private readonly roleService: RoleService, - private readonly accountService: AccountService + private readonly accountService: AccountService, + private readonly logger: Logger ) {} public async provisionExternalUser( @@ -22,8 +24,17 @@ export class SchulconnexUserProvisioningService { schoolId?: string ): Promise { const existingUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); + + let isUniqueEmail = true; if (externalUser.email) { - await this.checkUniqueEmail(externalUser.email, existingUser?.externalId); + isUniqueEmail = await this.userService.isEmailFromExternalSourceUnique( + externalUser.email, + existingUser?.externalId + ); + + if (!isUniqueEmail) { + this.logger.warning(new EmailAlreadyExistsLoggable(externalUser.email, externalUser.externalId)); + } } let roleRefs: RoleReference[] | undefined; @@ -42,6 +53,10 @@ export class SchulconnexUserProvisioningService { user.roles = roleRefs ?? existingUser.roles; user.schoolId = schoolId ?? existingUser.schoolId; user.birthday = externalUser.birthday ?? existingUser.birthday; + + if (!isUniqueEmail) { + user.email = existingUser.email; + } } else { createNewAccount = true; @@ -60,6 +75,10 @@ export class SchulconnexUserProvisioningService { schoolId, birthday: externalUser.birthday, }); + + if (!isUniqueEmail) { + user.email = ''; + } } const savedUser: UserDO = await this.userService.save(user); @@ -75,13 +94,4 @@ export class SchulconnexUserProvisioningService { return savedUser; } - - private async checkUniqueEmail(email: string, externalId?: string): Promise { - const foundUsers: UserDO[] = await this.userService.findByEmail(email); - const unmatchedUsers: UserDO[] = foundUsers.filter((user: UserDO) => user.externalId !== externalId); - - if (unmatchedUsers.length || (!externalId && foundUsers.length)) { - throw new EmailAlreadyExistsLoggable(email, externalId); - } - } } diff --git a/apps/server/src/modules/user/service/user.service.spec.ts b/apps/server/src/modules/user/service/user.service.spec.ts index 88778dfb5a7..7c709d28774 100644 --- a/apps/server/src/modules/user/service/user.service.spec.ts +++ b/apps/server/src/modules/user/service/user.service.spec.ts @@ -1,35 +1,35 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { EntityManager } from '@mikro-orm/core'; import { ObjectId } from '@mikro-orm/mongodb'; -import { AccountService, Account } from '@modules/account'; +import { Account, AccountService } from '@modules/account'; import { OauthCurrentUser } from '@modules/authentication/interface'; +import { + DataDeletedEvent, + DeletionErrorLoggableException, + DomainDeletionReportBuilder, + DomainName, + DomainOperationReportBuilder, + OperationType, +} from '@modules/deletion'; +import { deletionRequestFactory } from '@modules/deletion/domain/testing'; +import { RegistrationPinService } from '@modules/registration-pin'; import { RoleService } from '@modules/role'; import { NotFoundException } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; +import { EventBus } from '@nestjs/cqrs'; import { Test, TestingModule } from '@nestjs/testing'; import { UserDO } from '@shared/domain/domainobject/user.do'; -import { EntityId } from '@shared/domain/types'; import { Role, User } from '@shared/domain/entity'; import { IFindOptions, LanguageType, Permission, RoleName, SortOrder } from '@shared/domain/interface'; +import { EntityId } from '@shared/domain/types'; import { UserRepo } from '@shared/repo'; import { UserDORepo } from '@shared/repo/user/user-do.repo'; import { roleFactory, setupEntities, userDoFactory, userFactory } from '@shared/testing'; import { Logger } from '@src/core/logger'; -import { EventBus } from '@nestjs/cqrs'; -import { RegistrationPinService } from '@modules/registration-pin'; -import { - DomainDeletionReportBuilder, - DomainName, - DomainOperationReportBuilder, - OperationType, - DataDeletedEvent, - DeletionErrorLoggableException, -} from '@modules/deletion'; -import { deletionRequestFactory } from '@modules/deletion/domain/testing'; import { CalendarService } from '@src/infra/calendar'; -import { UserService } from './user.service'; -import { UserQuery } from './user-query.type'; import { UserDto } from '../uc/dto/user.dto'; +import { UserQuery } from './user-query.type'; +import { UserService } from './user.service'; describe('UserService', () => { let service: UserService; @@ -942,4 +942,119 @@ describe('UserService', () => { }); }); }); + + describe('isEmailFromExternalSourceUnique', () => { + describe('when email does not exist in SVS', () => { + const setup = () => { + const email = 'email'; + + userDORepo.findByEmail.mockResolvedValue([]); + + return { + email, + }; + }; + + it('should return true', async () => { + const { email } = setup(); + + const result: boolean = await service.isEmailFromExternalSourceUnique(email, undefined); + + expect(result).toBe(true); + }); + }); + + describe('when an existing user is found', () => { + describe('when existing user is the external user', () => { + const setup = () => { + const email = 'email'; + const externalId = 'externalId'; + const existingUser: UserDO = userDoFactory.build({ email, externalId }); + + userDORepo.findByEmail.mockResolvedValue([existingUser]); + + return { + email, + externalId, + }; + }; + + it('should return true', async () => { + const { email, externalId } = setup(); + + const result: boolean = await service.isEmailFromExternalSourceUnique(email, externalId); + + expect(result).toBe(true); + }); + }); + + describe('when existing user is not the external user', () => { + const setup = () => { + const email = 'email'; + const externalId = 'externalId'; + const otherUserWithSameEmail: UserDO = userDoFactory.build({ email }); + + userDORepo.findByEmail.mockResolvedValue([otherUserWithSameEmail]); + + return { + email, + externalId, + }; + }; + + it('should return false', async () => { + const { email, externalId } = setup(); + + const result: boolean = await service.isEmailFromExternalSourceUnique(email, externalId); + + expect(result).toBe(false); + }); + }); + + describe('when existing user is not the external user and external user is not already provisioned.', () => { + const setup = () => { + const email = 'email'; + const otherUserWithSameEmail: UserDO = userDoFactory.build({ email }); + + userDORepo.findByEmail.mockResolvedValue([otherUserWithSameEmail]); + + return { + email, + }; + }; + + it('should return false', async () => { + const { email } = setup(); + + const result: boolean = await service.isEmailFromExternalSourceUnique(email, undefined); + + expect(result).toBe(false); + }); + }); + }); + + describe('when multiple users are found', () => { + const setup = () => { + const email = 'email'; + const externalId = 'externalId'; + const existingUser: UserDO = userDoFactory.build({ email, externalId }); + const otherUserWithSameEmail: UserDO = userDoFactory.build({ email }); + + userDORepo.findByEmail.mockResolvedValue([existingUser, otherUserWithSameEmail]); + + return { + email, + externalId, + }; + }; + + it('should return false', async () => { + const { email, externalId } = setup(); + + const result: boolean = await service.isEmailFromExternalSourceUnique(email, externalId); + + expect(result).toBe(false); + }); + }); + }); }); diff --git a/apps/server/src/modules/user/service/user.service.ts b/apps/server/src/modules/user/service/user.service.ts index 1a5a3337880..de33a4c5cd4 100644 --- a/apps/server/src/modules/user/service/user.service.ts +++ b/apps/server/src/modules/user/service/user.service.ts @@ -1,39 +1,39 @@ -import { AccountService, Account } from '@modules/account'; +import { Account, AccountService } from '@modules/account'; // invalid import import { OauthCurrentUser } from '@modules/authentication/interface'; import { CurrentUserMapper } from '@modules/authentication/mapper'; -import { RoleDto, RoleService } from '@modules/role'; -import { BadRequestException, Injectable } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; -import { Page, RoleReference, UserDO } from '@shared/domain/domainobject'; -import { EntityId } from '@shared/domain/types'; -import { UserRepo } from '@shared/repo'; -import { UserDORepo } from '@shared/repo/user/user-do.repo'; -import { Logger } from '@src/core/logger'; -import { EventBus, EventsHandler, IEventHandler } from '@nestjs/cqrs'; -import { RegistrationPinService } from '@modules/registration-pin'; -import { User } from '@shared/domain/entity'; -import { IFindOptions, LanguageType } from '@shared/domain/interface'; import { - UserDeletedEvent, - DeletionService, DataDeletedEvent, - DomainDeletionReport, DataDeletionDomainOperationLoggable, - DomainName, + DeletionErrorLoggableException, + DeletionService, + DomainDeletionReport, DomainDeletionReportBuilder, + DomainName, + DomainOperationReport, DomainOperationReportBuilder, + OperationReportHelper, OperationType, - DeletionErrorLoggableException, - DomainOperationReport, StatusModel, - OperationReportHelper, + UserDeletedEvent, } from '@modules/deletion'; +import { RegistrationPinService } from '@modules/registration-pin'; +import { RoleDto, RoleService } from '@modules/role'; +import { BadRequestException, Injectable } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; +import { EventBus, EventsHandler, IEventHandler } from '@nestjs/cqrs'; +import { Page, RoleReference, UserDO } from '@shared/domain/domainobject'; +import { User } from '@shared/domain/entity'; +import { IFindOptions, LanguageType } from '@shared/domain/interface'; +import { EntityId } from '@shared/domain/types'; +import { UserRepo } from '@shared/repo'; +import { UserDORepo } from '@shared/repo/user/user-do.repo'; +import { Logger } from '@src/core/logger'; import { CalendarService } from '@src/infra/calendar'; -import { UserQuery } from './user-query.type'; -import { UserDto } from '../uc/dto/user.dto'; -import { UserMapper } from '../mapper/user.mapper'; import { UserConfig } from '../interfaces'; +import { UserMapper } from '../mapper/user.mapper'; +import { UserDto } from '../uc/dto/user.dto'; +import { UserQuery } from './user-query.type'; @Injectable() @EventsHandler(UserDeletedEvent) @@ -280,4 +280,15 @@ export class UserService implements DeletionService, IEventHandler { + const foundUsers: UserDO[] = await this.findByEmail(email); + const unmatchedUsers: UserDO[] = foundUsers.filter((user: UserDO) => user.externalId !== externalId); + + if (unmatchedUsers.length || (!externalId && foundUsers.length)) { + return false; + } + + return true; + } } From 061c9980ad0953168ef8ffbe1bc0ae262e3f8d71 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 24 Apr 2024 14:13:11 +0200 Subject: [PATCH 4/9] fix email provisioning TODO: give info to client to show problem --- .../service/schulconnex-user-provisioning.service.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts index ed330af9c1e..ad1f9021d91 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts @@ -47,16 +47,18 @@ export class SchulconnexUserProvisioningService { let createNewAccount = false; if (existingUser) { user = existingUser; + + if (!isUniqueEmail) { + user.email = existingUser.email; + } else { + user.email = externalUser.email ?? existingUser.email; + } + user.firstName = externalUser.firstName ?? existingUser.firstName; user.lastName = externalUser.lastName ?? existingUser.lastName; - user.email = externalUser.email ?? existingUser.email; user.roles = roleRefs ?? existingUser.roles; user.schoolId = schoolId ?? existingUser.schoolId; user.birthday = externalUser.birthday ?? existingUser.birthday; - - if (!isUniqueEmail) { - user.email = existingUser.email; - } } else { createNewAccount = true; From d1a47273612e67191e67247c76fa5053361aac73 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 24 Apr 2024 15:04:10 +0200 Subject: [PATCH 5/9] coverage up - hopefully --- .../oidc/service/schulconnex-user-provisioning.service.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts index 877ae9356a7..d9991fd3ea2 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts @@ -130,6 +130,7 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); + userService.isEmailFromExternalSourceUnique.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); From 17c6c2cde024d1b074d6d63f8a591a2a419a41bc Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 24 Apr 2024 15:50:55 +0200 Subject: [PATCH 6/9] coverage up - hopefully 2 --- .../oidc/service/schulconnex-user-provisioning.service.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts index d9991fd3ea2..19898f6479c 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts @@ -130,7 +130,6 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.isEmailFromExternalSourceUnique.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -208,6 +207,7 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, systemId, existingUser } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); + userService.isEmailFromExternalSourceUnique.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); From fe1a8f019817060c556b125c9d1a6652e3580866 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 25 Apr 2024 22:58:31 +0200 Subject: [PATCH 7/9] requested changes, readability refactoring --- ...ulconnex-user-provisioning.service.spec.ts | 16 +-- .../schulconnex-user-provisioning.service.ts | 120 +++++++++++------- .../modules/user/service/user.service.spec.ts | 14 +- .../src/modules/user/service/user.service.ts | 9 +- 4 files changed, 92 insertions(+), 67 deletions(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts index 19898f6479c..c94168b7def 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts @@ -133,14 +133,14 @@ describe(SchulconnexUserProvisioningService.name, () => { await service.provisionExternalUser(externalUser, systemId, schoolId); - expect(userService.isEmailFromExternalSourceUnique).toHaveBeenCalledWith(externalUser.email, undefined); + expect(userService.isEmailUniqueForExternal).toHaveBeenCalledWith(externalUser.email, undefined); }); it('should call the user service to save the user', async () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.isEmailFromExternalSourceUnique.mockResolvedValue(true); + userService.isEmailUniqueForExternal.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -185,18 +185,17 @@ describe(SchulconnexUserProvisioningService.name, () => { }); }); - describe('when the external user has an email, that already exists in SVS', () => { + describe('when the external user has an email, that already exists', () => { it('should log EmailAlreadyExistsLoggable', async () => { const { externalUser, systemId, schoolId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.isEmailFromExternalSourceUnique.mockResolvedValue(false); + userService.isEmailUniqueForExternal.mockResolvedValue(false); await service.provisionExternalUser(externalUser, systemId, schoolId); expect(logger.warning).toHaveBeenCalledWith({ email: externalUser.email, - externalId: externalUser.externalId, }); }); }); @@ -207,14 +206,11 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, systemId, existingUser } = setupUser(); userService.findByExternalId.mockResolvedValue(existingUser); - userService.isEmailFromExternalSourceUnique.mockResolvedValue(true); + userService.isEmailUniqueForExternal.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); - expect(userService.isEmailFromExternalSourceUnique).toHaveBeenCalledWith( - externalUser.email, - existingUser.externalId - ); + expect(userService.isEmailUniqueForExternal).toHaveBeenCalledWith(externalUser.email, existingUser.externalId); }); it('should call the user service to save the user', async () => { diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts index ad1f9021d91..f451486e0a7 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts @@ -4,6 +4,7 @@ import { RoleDto, RoleService } from '@modules/role'; import { UserService } from '@modules/user'; import { Injectable, UnprocessableEntityException } from '@nestjs/common'; import { RoleReference, UserDO } from '@shared/domain/domainobject'; +import { RoleName } from '@shared/domain/interface'; import { EntityId } from '@shared/domain/types'; import { Logger } from '@src/core/logger'; import CryptoJS from 'crypto-js'; @@ -23,64 +24,25 @@ export class SchulconnexUserProvisioningService { systemId: EntityId, schoolId?: string ): Promise { - const existingUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); + const foundUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); - let isUniqueEmail = true; - if (externalUser.email) { - isUniqueEmail = await this.userService.isEmailFromExternalSourceUnique( - externalUser.email, - existingUser?.externalId - ); + const isEmailUnique: boolean = await this.checkUniqueEmail(externalUser.email, foundUser?.externalId); - if (!isUniqueEmail) { - this.logger.warning(new EmailAlreadyExistsLoggable(externalUser.email, externalUser.externalId)); - } - } - - let roleRefs: RoleReference[] | undefined; - if (externalUser.roles) { - const roles: RoleDto[] = await this.roleService.findByNames(externalUser.roles); - roleRefs = roles.map((role: RoleDto): RoleReference => new RoleReference({ id: role.id || '', name: role.name })); - } + const roleRefs: RoleReference[] | undefined = await this.createRoleReferences(externalUser.roles); - let user: UserDO; let createNewAccount = false; - if (existingUser) { - user = existingUser; - - if (!isUniqueEmail) { - user.email = existingUser.email; - } else { - user.email = externalUser.email ?? existingUser.email; - } - - user.firstName = externalUser.firstName ?? existingUser.firstName; - user.lastName = externalUser.lastName ?? existingUser.lastName; - user.roles = roleRefs ?? existingUser.roles; - user.schoolId = schoolId ?? existingUser.schoolId; - user.birthday = externalUser.birthday ?? existingUser.birthday; + let user: UserDO; + if (foundUser) { + user = this.updateUser(externalUser, foundUser, isEmailUnique, roleRefs, schoolId); } else { - createNewAccount = true; - if (!schoolId) { throw new UnprocessableEntityException( `Unable to create new external user ${externalUser.externalId} without a school` ); } - user = new UserDO({ - externalId: externalUser.externalId, - firstName: externalUser.firstName ?? '', - lastName: externalUser.lastName ?? '', - email: externalUser.email ?? '', - roles: roleRefs ?? [], - schoolId, - birthday: externalUser.birthday, - }); - - if (!isUniqueEmail) { - user.email = ''; - } + createNewAccount = true; + user = this.createUser(externalUser, isEmailUnique, schoolId, roleRefs); } const savedUser: UserDO = await this.userService.save(user); @@ -96,4 +58,68 @@ export class SchulconnexUserProvisioningService { return savedUser; } + + private async checkUniqueEmail(email?: string, externalId?: string): Promise { + if (email) { + const isEmailUnique: boolean = await this.userService.isEmailUniqueForExternal(email, externalId); + + if (!isEmailUnique) { + this.logger.warning(new EmailAlreadyExistsLoggable(email, externalId)); + } + + return isEmailUnique; + } + + return true; + } + + private async createRoleReferences(roles?: RoleName[]): Promise { + if (roles) { + const foundRoles: RoleDto[] = await this.roleService.findByNames(roles); + const roleRefs = foundRoles.map( + (role: RoleDto): RoleReference => new RoleReference({ id: role.id || '', name: role.name }) + ); + + return roleRefs; + } + + return undefined; + } + + private updateUser( + externalUser: ExternalUserDto, + foundUser: UserDO, + isEmailUnique: boolean, + roleRefs?: RoleReference[], + schoolId?: string + ): UserDO { + const user: UserDO = foundUser; + user.firstName = externalUser.firstName ?? foundUser.firstName; + user.lastName = externalUser.lastName ?? foundUser.lastName; + user.email = isEmailUnique ? externalUser.email ?? foundUser.email : foundUser.email; + user.roles = roleRefs ?? foundUser.roles; + user.schoolId = schoolId ?? foundUser.schoolId; + user.birthday = externalUser.birthday ?? foundUser.birthday; + + return user; + } + + private createUser( + externalUser: ExternalUserDto, + isEmailUnique: boolean, + schoolId: string, + roleRefs?: RoleReference[] + ): UserDO { + const user: UserDO = new UserDO({ + externalId: externalUser.externalId, + firstName: externalUser.firstName ?? '', + lastName: externalUser.lastName ?? '', + email: isEmailUnique ? externalUser.email ?? '' : '', + roles: roleRefs ?? [], + schoolId, + birthday: externalUser.birthday, + }); + + return user; + } } diff --git a/apps/server/src/modules/user/service/user.service.spec.ts b/apps/server/src/modules/user/service/user.service.spec.ts index 7c709d28774..1a5766e743b 100644 --- a/apps/server/src/modules/user/service/user.service.spec.ts +++ b/apps/server/src/modules/user/service/user.service.spec.ts @@ -943,8 +943,8 @@ describe('UserService', () => { }); }); - describe('isEmailFromExternalSourceUnique', () => { - describe('when email does not exist in SVS', () => { + describe('isEmailUniqueForExternal', () => { + describe('when email does not exist', () => { const setup = () => { const email = 'email'; @@ -958,7 +958,7 @@ describe('UserService', () => { it('should return true', async () => { const { email } = setup(); - const result: boolean = await service.isEmailFromExternalSourceUnique(email, undefined); + const result: boolean = await service.isEmailUniqueForExternal(email, undefined); expect(result).toBe(true); }); @@ -982,7 +982,7 @@ describe('UserService', () => { it('should return true', async () => { const { email, externalId } = setup(); - const result: boolean = await service.isEmailFromExternalSourceUnique(email, externalId); + const result: boolean = await service.isEmailUniqueForExternal(email, externalId); expect(result).toBe(true); }); @@ -1005,7 +1005,7 @@ describe('UserService', () => { it('should return false', async () => { const { email, externalId } = setup(); - const result: boolean = await service.isEmailFromExternalSourceUnique(email, externalId); + const result: boolean = await service.isEmailUniqueForExternal(email, externalId); expect(result).toBe(false); }); @@ -1026,7 +1026,7 @@ describe('UserService', () => { it('should return false', async () => { const { email } = setup(); - const result: boolean = await service.isEmailFromExternalSourceUnique(email, undefined); + const result: boolean = await service.isEmailUniqueForExternal(email, undefined); expect(result).toBe(false); }); @@ -1051,7 +1051,7 @@ describe('UserService', () => { it('should return false', async () => { const { email, externalId } = setup(); - const result: boolean = await service.isEmailFromExternalSourceUnique(email, externalId); + const result: boolean = await service.isEmailUniqueForExternal(email, externalId); expect(result).toBe(false); }); diff --git a/apps/server/src/modules/user/service/user.service.ts b/apps/server/src/modules/user/service/user.service.ts index de33a4c5cd4..3433527a631 100644 --- a/apps/server/src/modules/user/service/user.service.ts +++ b/apps/server/src/modules/user/service/user.service.ts @@ -281,11 +281,14 @@ export class UserService implements DeletionService, IEventHandler { + public async isEmailUniqueForExternal(email: string, externalId?: string): Promise { const foundUsers: UserDO[] = await this.findByEmail(email); - const unmatchedUsers: UserDO[] = foundUsers.filter((user: UserDO) => user.externalId !== externalId); + if (!externalId && foundUsers.length) { + return false; + } - if (unmatchedUsers.length || (!externalId && foundUsers.length)) { + const unmatchedUsers: UserDO[] = foundUsers.filter((user: UserDO) => user.externalId !== externalId); + if (unmatchedUsers.length) { return false; } From 11e5c4aeaf9c130ffff77898f61f922e1f3b9f35 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 25 Apr 2024 23:44:00 +0200 Subject: [PATCH 8/9] coverage up --- ...schulconnex-user-provisioning.service.spec.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts index c94168b7def..29faf026c2b 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts @@ -97,6 +97,7 @@ describe(SchulconnexUserProvisioningService.name, () => { roles: [RoleName.USER], birthday, }); + const minimalViableExternalUser: ExternalUserDto = new ExternalUserDto({ externalId: 'externalUserId' }); const userRole: RoleDto = new RoleDto({ id: 'roleId', name: RoleName.USER, @@ -118,6 +119,7 @@ describe(SchulconnexUserProvisioningService.name, () => { existingUser, savedUser, externalUser, + minimalViableExternalUser, userRole, schoolId, systemId, @@ -126,10 +128,23 @@ describe(SchulconnexUserProvisioningService.name, () => { }; describe('when the user does not exist yet', () => { + describe('when the external user has no email or roles', () => { + it('should return the saved user', async () => { + const { minimalViableExternalUser, schoolId, savedUser, systemId } = setupUser(); + + userService.findByExternalId.mockResolvedValue(null); + + const result: UserDO = await service.provisionExternalUser(minimalViableExternalUser, systemId, schoolId); + + expect(result).toEqual(savedUser); + }); + }); + it('should call user service to check uniqueness of email', async () => { const { externalUser, schoolId, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); + userService.isEmailUniqueForExternal.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -140,7 +155,6 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.isEmailUniqueForExternal.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); From 6348271b84e45c64c1485a8437166aa6c82782a3 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 26 Apr 2024 08:52:15 +0200 Subject: [PATCH 9/9] fix test --- .../oidc/service/schulconnex-user-provisioning.service.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts index 29faf026c2b..08f3f70bb38 100644 --- a/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts +++ b/apps/server/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts @@ -144,7 +144,6 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); - userService.isEmailUniqueForExternal.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -155,6 +154,7 @@ describe(SchulconnexUserProvisioningService.name, () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); userService.findByExternalId.mockResolvedValue(null); + userService.isEmailUniqueForExternal.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId);