Skip to content

Commit

Permalink
N21-2057 Team invitations with duplicate emails (#5114)
Browse files Browse the repository at this point in the history
  • Loading branch information
@MarvinOehlerkingCap
MarvinOehlerkingCap authored Jul 18, 2024
1 parent 41b019b commit 8a11bc9
Show file tree
Hide file tree
Showing 9 changed files with 7 additions and 259 deletions.
32 changes: 0 additions & 32 deletions apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.spec.ts
View file

This file was deleted.

15 changes: 0 additions & 15 deletions apps/server/src/modules/provisioning/loggable/email-already-exists.loggable.ts
View file

This file was deleted.

1 change: 0 additions & 1 deletion apps/server/src/modules/provisioning/loggable/index.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
49 changes: 0 additions & 49 deletions .../modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ describe(SchulconnexUserProvisioningService.name, () => {
let userService: DeepMocked<UserService>;
let roleService: DeepMocked<RoleService>;
let accountService: DeepMocked<AccountService>;
let logger: DeepMocked<Logger>;

beforeAll(async () => {
module = await Test.createTestingModule({
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down
37 changes: 6 additions & 31 deletions ...r/src/modules/provisioning/strategy/oidc/service/schulconnex-user-provisioning.service.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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(
Expand All @@ -26,14 +23,12 @@ export class SchulconnexUserProvisioningService {
): Promise<UserDO> {
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(
Expand All @@ -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);
Expand All @@ -59,20 +54,6 @@ export class SchulconnexUserProvisioningService {
return savedUser;
}

private async checkUniqueEmail(email?: string, externalId?: string): Promise<boolean> {
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<RoleReference[] | undefined> {
if (roles) {
const foundRoles: RoleDto[] = await this.roleService.findByNames(roles);
Expand All @@ -89,32 +70,26 @@ 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;

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,
Expand Down
115 changes: 0 additions & 115 deletions apps/server/src/modules/user/service/user.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
});
14 changes: 0 additions & 14 deletions apps/server/src/modules/user/service/user.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,18 +283,4 @@ export class UserService implements DeletionService, IEventHandler<UserDeletedEv

return DomainDeletionReportBuilder.build(DomainName.CALENDAR, extractedOperationReport);
}

public async isEmailUniqueForExternal(email: string, externalId?: string): Promise<boolean> {
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;
}
}
1 change: 0 additions & 1 deletion apps/server/src/shared/domain/entity/user.entity.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ interface UserInfo {
export class User extends BaseEntityWithTimestamps implements EntityWithSchool {
@Property()
@Index()
// @Unique()
email: string;

@Property()
Expand Down
2 changes: 1 addition & 1 deletion src/services/user/hooks/publicTeachers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down

0 comments on commit 8a11bc9

Please sign in to comment.