From 752b69e3bf7e124b7a758041562fd50c0193c92d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marvin=20=C3=96hlerking?= Date: Wed, 17 Jul 2024 15:00:10 +0200 Subject: [PATCH] adjust provisioning and add schoolId to public teacher --- .../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 | 49 -------- .../schulconnex-user-provisioning.service.ts | 37 +----- .../modules/user/service/user.service.spec.ts | 115 ------------------ .../src/modules/user/service/user.service.ts | 14 --- .../src/shared/domain/entity/user.entity.ts | 1 - src/services/user/hooks/publicTeachers.js | 2 +- 9 files changed, 7 insertions(+), 259 deletions(-) delete mode 100644 apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts delete 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 deleted file mode 100644 index 87833c6967c..00000000000 --- a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts +++ /dev/null @@ -1,32 +0,0 @@ -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 deleted file mode 100644 index 1e4cd7ed77d..00000000000 --- a/apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts +++ /dev/null @@ -1,15 +0,0 @@ -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 48efe5c4f67..89b4baf9b7a 100644 --- a/apps/server/src/modules/provisioning/loggable/index.ts +++ b/apps/server/src/modules/provisioning/loggable/index.ts @@ -1,5 +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'; export { SchoolExternalToolCreatedLoggable } from './school-external-tool-created.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 08f3f70bb38..05e6fa3d3ad 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 @@ -22,7 +22,6 @@ describe(SchulconnexUserProvisioningService.name, () => { let userService: DeepMocked; let roleService: DeepMocked; let accountService: DeepMocked; - let logger: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -51,7 +50,6 @@ describe(SchulconnexUserProvisioningService.name, () => { userService = module.get(UserService); roleService = module.get(RoleService); accountService = module.get(AccountService); - logger = module.get(Logger); }); afterAll(async () => { @@ -140,27 +138,6 @@ describe(SchulconnexUserProvisioningService.name, () => { }); }); - 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); - - expect(userService.save).toHaveBeenCalledWith(new UserDO({ ...savedUser, id: undefined })); - }); - it('should return the saved user', async () => { const { externalUser, schoolId, savedUser, systemId } = setupUser(); @@ -198,35 +175,9 @@ 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 f451486e0a7..783076572de 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,12 +1,10 @@ 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'; @@ -15,8 +13,7 @@ export class SchulconnexUserProvisioningService { constructor( private readonly userService: UserService, private readonly roleService: RoleService, - private readonly accountService: AccountService, - private readonly logger: Logger + private readonly accountService: AccountService ) {} public async provisionExternalUser( @@ -26,14 +23,12 @@ export class SchulconnexUserProvisioningService { ): Promise { 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); let createNewAccount = false; let user: UserDO; if (foundUser) { - user = this.updateUser(externalUser, foundUser, isEmailUnique, roleRefs, schoolId); + user = this.updateUser(externalUser, foundUser, roleRefs, schoolId); } else { if (!schoolId) { throw new UnprocessableEntityException( @@ -42,7 +37,7 @@ export class SchulconnexUserProvisioningService { } createNewAccount = true; - user = this.createUser(externalUser, isEmailUnique, schoolId, roleRefs); + user = this.createUser(externalUser, schoolId, roleRefs); } const savedUser: UserDO = await this.userService.save(user); @@ -59,20 +54,6 @@ 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); @@ -89,14 +70,13 @@ export class SchulconnexUserProvisioningService { 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.email = externalUser.email ?? foundUser.email; user.roles = roleRefs ?? foundUser.roles; user.schoolId = schoolId ?? foundUser.schoolId; user.birthday = externalUser.birthday ?? foundUser.birthday; @@ -104,17 +84,12 @@ export class SchulconnexUserProvisioningService { return user; } - private createUser( - externalUser: ExternalUserDto, - isEmailUnique: boolean, - schoolId: string, - roleRefs?: RoleReference[] - ): UserDO { + private createUser(externalUser: ExternalUserDto, schoolId: string, roleRefs?: RoleReference[]): UserDO { const user: UserDO = new UserDO({ externalId: externalUser.externalId, firstName: externalUser.firstName ?? '', lastName: externalUser.lastName ?? '', - email: isEmailUnique ? externalUser.email ?? '' : '', + email: externalUser.email ?? '', roles: roleRefs ?? [], schoolId, birthday: externalUser.birthday, 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 98bd755ef61..eee9f447616 100644 --- a/apps/server/src/modules/user/service/user.service.spec.ts +++ b/apps/server/src/modules/user/service/user.service.spec.ts @@ -946,119 +946,4 @@ 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 f474bdfbefc..33394ff30dc 100644 --- a/apps/server/src/modules/user/service/user.service.ts +++ b/apps/server/src/modules/user/service/user.service.ts @@ -283,18 +283,4 @@ 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; - } } diff --git a/apps/server/src/shared/domain/entity/user.entity.ts b/apps/server/src/shared/domain/entity/user.entity.ts index a253766e29a..cfb27773c5b 100644 --- a/apps/server/src/shared/domain/entity/user.entity.ts +++ b/apps/server/src/shared/domain/entity/user.entity.ts @@ -51,7 +51,6 @@ interface UserInfo { export class User extends BaseEntityWithTimestamps implements EntityWithSchool { @Property() @Index() - // @Unique() email: string; @Property() diff --git a/src/services/user/hooks/publicTeachers.js b/src/services/user/hooks/publicTeachers.js index c44e7eba5cf..9a59e3a6fdd 100644 --- a/src/services/user/hooks/publicTeachers.js +++ b/src/services/user/hooks/publicTeachers.js @@ -26,7 +26,7 @@ const mapRoleFilterQuery = (hook) => { const filterForPublicTeacher = (hook) => { // Limit accessible fields - hook.params.query.$select = ['_id', 'firstName', 'lastName']; + hook.params.query.$select = ['_id', 'firstName', 'lastName', 'schoolId']; // Limit accessible user (only teacher which are discoverable) hook.params.query.roles = ['teacher'];