From 2cba8e4d29d4d2c884a2a6e7af3f0b1e5bcede89 Mon Sep 17 00:00:00 2001 From: Igor Richter <93926487+IgorCapCoder@users.noreply.github.com> Date: Fri, 26 Apr 2024 13:51:23 +0200 Subject: [PATCH] N21-1888- check email in provisioning (#4955) - check email in provisioning - log warning --- .../email-already-exists.loggable.spec.ts | 32 ++++ .../loggable/email-already-exists.loggable.ts | 15 ++ .../modules/provisioning/loggable/index.ts | 1 + ...ulconnex-user-provisioning.service.spec.ts | 60 +++++++- .../schulconnex-user-provisioning.service.ts | 108 +++++++++---- .../modules/user/service/user.service.spec.ts | 145 ++++++++++++++++-- .../src/modules/user/service/user.service.ts | 60 +++++--- 7 files changed, 354 insertions(+), 67 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..87833c6967c --- /dev/null +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts @@ -0,0 +1,32 @@ +import { EmailAlreadyExistsLoggable } from '@modules/provisioning/loggable/email-already-exists.loggable'; + +describe('EmailAlreadyExistsLoggableException', () => { + describe('getLogMessage', () => { + const setup = () => { + const email = 'mock-email'; + const externalId = '789'; + + const loggable = new EmailAlreadyExistsLoggable(email, externalId); + + return { + loggable, + email, + externalId, + }; + }; + + it('should return the correct log message', () => { + const { loggable, email, externalId } = setup(); + + const message = loggable.getLogMessage(); + + expect(message).toEqual({ + message: 'The Email to be provisioned already exists.', + 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 new file mode 100644 index 00000000000..1e4cd7ed77d --- /dev/null +++ b/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts @@ -0,0 +1,15 @@ +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; + +export class EmailAlreadyExistsLoggable implements Loggable { + constructor(private readonly email: string, private readonly externalId?: string) {} + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + message: 'The Email to be provisioned already exists.', + data: { + email: this.email, + 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..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 @@ -1,5 +1,5 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { AccountService, AccountSave } from '@modules/account'; +import { AccountSave, AccountService } from '@modules/account'; import { RoleService } from '@modules/role'; import { RoleDto } from '@modules/role/service/dto/role.dto'; import { UserService } from '@modules/user'; @@ -8,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'; @@ -21,6 +22,7 @@ describe(SchulconnexUserProvisioningService.name, () => { let userService: DeepMocked; let roleService: DeepMocked; let accountService: DeepMocked; + let logger: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -38,6 +40,10 @@ describe(SchulconnexUserProvisioningService.name, () => { provide: AccountService, useValue: createMock(), }, + { + provide: Logger, + useValue: createMock(), + }, ], }).compile(); @@ -45,6 +51,7 @@ describe(SchulconnexUserProvisioningService.name, () => { userService = module.get(UserService); roleService = module.get(RoleService); accountService = module.get(AccountService); + logger = module.get(Logger); }); afterAll(async () => { @@ -90,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, @@ -111,6 +119,7 @@ describe(SchulconnexUserProvisioningService.name, () => { existingUser, savedUser, externalUser, + minimalViableExternalUser, userRole, schoolId, systemId, @@ -119,10 +128,33 @@ 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); + + await service.provisionExternalUser(externalUser, systemId, schoolId); + + 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.isEmailUniqueForExternal.mockResolvedValue(true); await service.provisionExternalUser(externalUser, systemId, schoolId); @@ -166,9 +198,35 @@ describe(SchulconnexUserProvisioningService.name, () => { await expect(promise).rejects.toThrow(UnprocessableEntityException); }); }); + + 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.isEmailUniqueForExternal.mockResolvedValue(false); + + await service.provisionExternalUser(externalUser, systemId, schoolId); + + expect(logger.warning).toHaveBeenCalledWith({ + email: externalUser.email, + }); + }); + }); }); 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); + userService.isEmailUniqueForExternal.mockResolvedValue(true); + + await service.provisionExternalUser(externalUser, systemId, schoolId); + + expect(userService.isEmailUniqueForExternal).toHaveBeenCalledWith(externalUser.email, existingUser.externalId); + }); + it('should call the user service to save the user', async () => { const { externalUser, schoolId, existingUser, systemId } = setupUser(); 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..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 @@ -1,9 +1,12 @@ -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'; 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'; import { ExternalUserDto } from '../../../dto'; @@ -12,7 +15,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( @@ -20,41 +24,25 @@ export class SchulconnexUserProvisioningService { systemId: EntityId, schoolId?: string ): Promise { - 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 foundUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); + + const isEmailUnique: boolean = await this.checkUniqueEmail(externalUser.email, foundUser?.externalId); + + const roleRefs: RoleReference[] | undefined = await this.createRoleReferences(externalUser.roles); - const existingUser: UserDO | null = await this.userService.findByExternalId(externalUser.externalId, systemId); - let user: UserDO; let createNewAccount = false; - if (existingUser) { - user = existingUser; - 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; + 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, - }); + createNewAccount = true; + user = this.createUser(externalUser, isEmailUnique, schoolId, roleRefs); } const savedUser: UserDO = await this.userService.save(user); @@ -70,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 88778dfb5a7..1a5766e743b 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('isEmailUniqueForExternal', () => { + describe('when email does not exist', () => { + const setup = () => { + const email = 'email'; + + userDORepo.findByEmail.mockResolvedValue([]); + + return { + email, + }; + }; + + it('should return true', async () => { + const { email } = setup(); + + const result: boolean = await service.isEmailUniqueForExternal(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.isEmailUniqueForExternal(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.isEmailUniqueForExternal(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.isEmailUniqueForExternal(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.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 1a5a3337880..3433527a631 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,18 @@ export class UserService implements DeletionService, IEventHandler { + const foundUsers: UserDO[] = await this.findByEmail(email); + if (!externalId && foundUsers.length) { + return false; + } + + const unmatchedUsers: UserDO[] = foundUsers.filter((user: UserDO) => user.externalId !== externalId); + if (unmatchedUsers.length) { + return false; + } + + return true; + } }