From c189c831ad87c3015f2d2493edbe467e54124ccc Mon Sep 17 00:00:00 2001 From: "Marvin Rode (Cap)" <127723478+marode-cap@users.noreply.github.com> Date: Mon, 26 Aug 2024 15:51:36 +0200 Subject: [PATCH] SPSH-983 Send attributes to keycloak (#624) * Send attributes to keycloak * Fix unrelated files * Fix some tests * Fix coverage * Make external system attributes optional * Remove attributes from keycloak response * Revert some changes --- .../dbseed/domain/db-seed.service.spec.ts | 3 + .../domain/keycloak-user.service.spec.ts | 12 +++ .../domain/keycloak-user.service.ts | 3 + .../keycloak-administration/domain/user.ts | 16 +++- .../person.repository.integration-spec.ts | 20 +++++ .../person/persistence/person.repository.ts | 76 +++++++++++++------ test/utils/do-factory.ts | 1 + 7 files changed, 106 insertions(+), 25 deletions(-) diff --git a/src/console/dbseed/domain/db-seed.service.spec.ts b/src/console/dbseed/domain/db-seed.service.spec.ts index 1616ca443..9c5bc3824 100644 --- a/src/console/dbseed/domain/db-seed.service.spec.ts +++ b/src/console/dbseed/domain/db-seed.service.spec.ts @@ -391,6 +391,9 @@ describe('DbSeedService', () => { 'testusername', 'test@example.com', faker.date.recent(), + { + ID_ITSLEARNING: faker.string.uuid(), + }, ); kcUserService.findOne.mockResolvedValueOnce({ ok: true, value: existingUser }); diff --git a/src/modules/keycloak-administration/domain/keycloak-user.service.spec.ts b/src/modules/keycloak-administration/domain/keycloak-user.service.spec.ts index b4a2423f9..d5b2760ee 100644 --- a/src/modules/keycloak-administration/domain/keycloak-user.service.spec.ts +++ b/src/modules/keycloak-administration/domain/keycloak-user.service.spec.ts @@ -69,6 +69,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }); expect(res).toStrictEqual>({ @@ -90,6 +91,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }, password, ); @@ -99,6 +101,9 @@ describe('KeycloakUserService', () => { email: user.email, enabled: true, credentials: [{ type: 'password', value: password, temporary: false }], + attributes: { + ID_ITSLEARNING: user.externalSystemIDs.ID_ITSLEARNING, + }, }); }); }); @@ -120,6 +125,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }); expect(res).toStrictEqual>({ @@ -172,6 +178,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }, `{BCRYPT}$2b$12$hqG5T3z8v0Ou8Lmmr2mhW.lNP0DQGO9MS6PQT/CzCJP8Fcx GgKOau`, @@ -194,6 +201,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }, `{crypt}$6$M.L8yO/PSWLRRhe6$CXj2g0wgWhiAnfROIdqJROrgbjmcmin02M1 sM1Z25N7H3puT6qlgsDIM.60brf1csn0Zk9GxS8sILpJvmvFi11`, @@ -215,6 +223,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }, `{BCRYPT}xxxxxhqG5T3$z8v0Ou8Lmmr2mhW.lNP0DQGO9MS6PQT/CzCJP8Fcx GgKOau`, @@ -236,6 +245,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }, `{crypt}$$x$$M.L8yO/PSWLRRhe6$CXj2g0wgWhiAnfROIdqJROrgbjmcmin02M1 sM1Z25N7H3puT6qlgsDIM.60brf1csn0Zk9GxS8sILpJvmvFi11`, @@ -257,6 +267,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }, `{notsupported}$6$M.L8yO/PSWLRRhe6$CXj2g0wgWhiAnfROIdqJROrgbjmcmin02M1 sM1Z25N7H3puT6qlgsDIM.60brf1csn0Zk9GxS8sILpJvmvFi11`, @@ -287,6 +298,7 @@ describe('KeycloakUserService', () => { createdDate: undefined, username: user.username, email: user.email, + externalSystemIDs: user.externalSystemIDs, }, `{BCRYPT}$2b$12$hqG5T3z8v0Ou8Lmmr2mhW.lNP0DQGO9MS6PQT/CzCJP8Fcx GgKOau`, diff --git a/src/modules/keycloak-administration/domain/keycloak-user.service.ts b/src/modules/keycloak-administration/domain/keycloak-user.service.ts index eac83ab94..e65c918da 100644 --- a/src/modules/keycloak-administration/domain/keycloak-user.service.ts +++ b/src/modules/keycloak-administration/domain/keycloak-user.service.ts @@ -54,6 +54,7 @@ export class KeycloakUserService { const userRepresentation: UserRepresentation = { username: user.username, enabled: true, + attributes: user.externalSystemIDs, }; if (user.email) { @@ -150,6 +151,7 @@ export class KeycloakUserService { type: 'password', }, ], + attributes: user.externalSystemIDs, }; const response: { id: string } = await kcAdminClientResult.value.users.create(userRepresentation); @@ -269,6 +271,7 @@ export class KeycloakUserService { userReprDto.username, userReprDto.email, new Date(userReprDto.createdTimestamp), + {}, // UserAttributes ); return { ok: true, value: userDo }; diff --git a/src/modules/keycloak-administration/domain/user.ts b/src/modules/keycloak-administration/domain/user.ts index 6b441afde..b338c42e6 100644 --- a/src/modules/keycloak-administration/domain/user.ts +++ b/src/modules/keycloak-administration/domain/user.ts @@ -1,13 +1,22 @@ +export type ExternalSystemIDs = { + ID_ITSLEARNING?: string; +}; + export class User { private constructor( public id: Persisted, public username: string, public email: string | undefined, public createdDate: Persisted, + public externalSystemIDs: ExternalSystemIDs, ) {} - public static createNew(username: string, email: string | undefined): User { - return new User(undefined, username, email, undefined); + public static createNew( + username: string, + email: string | undefined, + externalSystemIDs: ExternalSystemIDs, + ): User { + return new User(undefined, username, email, undefined, externalSystemIDs); } public static construct( @@ -15,7 +24,8 @@ export class User { username: string, email: string | undefined, createdDate: Date, + externalSystemIDs: ExternalSystemIDs, ): User { - return new User(id, username, email, createdDate); + return new User(id, username, email, createdDate, externalSystemIDs); } } diff --git a/src/modules/person/persistence/person.repository.integration-spec.ts b/src/modules/person/persistence/person.repository.integration-spec.ts index 3f475ad24..d6f48348d 100644 --- a/src/modules/person/persistence/person.repository.integration-spec.ts +++ b/src/modules/person/persistence/person.repository.integration-spec.ts @@ -516,6 +516,26 @@ describe('PersonRepository Integration', () => { }); }); }); + + describe('When an unexpected error occurs', () => { + it('should rollback transaction and rethrow', async () => { + usernameGeneratorService.generateUsername.mockResolvedValue({ ok: true, value: 'testusername' }); + const person: Person | DomainError = await Person.createNew(usernameGeneratorService, { + familienname: faker.person.lastName(), + vorname: faker.person.firstName(), + }); + if (person instanceof DomainError) { + throw person; + } + + const dummyError: Error = new Error('Unexpected'); + kcUserServiceMock.create.mockRejectedValueOnce(dummyError); + + const promise: Promise = sut.create(person); + + await expect(promise).rejects.toBe(dummyError); + }); + }); }); describe('update', () => { diff --git a/src/modules/person/persistence/person.repository.ts b/src/modules/person/persistence/person.repository.ts index 7899463bf..e42bfada4 100644 --- a/src/modules/person/persistence/person.repository.ts +++ b/src/modules/person/persistence/person.repository.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'node:crypto'; import { EntityManager, Loaded, RequiredEntityData } from '@mikro-orm/postgresql'; import { Injectable } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; @@ -231,23 +232,50 @@ export class PersonRepository { } public async create(person: Person, hashedPassword?: string): Promise | DomainError> { - let personWithKeycloakUser: Person | DomainError; - if (!hashedPassword) { - personWithKeycloakUser = await this.createKeycloakUser(person, this.kcUserService); - } else { - personWithKeycloakUser = await this.createKeycloakUserWithHashedPassword( - person, - hashedPassword, - this.kcUserService, - ); - } - if (personWithKeycloakUser instanceof DomainError) { - return personWithKeycloakUser; - } - const personEntity: PersonEntity = this.em.create(PersonEntity, mapAggregateToData(personWithKeycloakUser)); - await this.em.persistAndFlush(personEntity); + const transaction: EntityManager = this.em.fork(); + await transaction.begin(); + + try { + // Create DB person + const personEntity: PersonEntity = transaction.create(PersonEntity, mapAggregateToData(person)).assign({ + id: randomUUID(), // Generate ID here instead of at insert-time + }); + transaction.persist(personEntity); + + const persistedPerson: Person = mapEntityToAggregateInplace(personEntity, person); + + // Take ID from person to create keycloak user + let personWithKeycloakUser: Person | DomainError; + if (!hashedPassword) { + personWithKeycloakUser = await this.createKeycloakUser(persistedPerson, this.kcUserService); + } else { + personWithKeycloakUser = await this.createKeycloakUserWithHashedPassword( + persistedPerson, + hashedPassword, + this.kcUserService, + ); + } - return mapEntityToAggregateInplace(personEntity, personWithKeycloakUser); + // -> When keycloak fails, rollback + if (personWithKeycloakUser instanceof DomainError) { + await transaction.rollback(); + return personWithKeycloakUser; + } + + // take ID from keycloak and update user + personEntity.assign(mapAggregateToData(personWithKeycloakUser)); + + // Commit + await transaction.commit(); + + // Return mapped person + return mapEntityToAggregateInplace(personEntity, personWithKeycloakUser); + } catch (e) { + // Any other errors + // -> rollback and rethrow + await transaction.rollback(); + throw e; + } } public async update(person: Person): Promise | DomainError> { @@ -287,15 +315,17 @@ export class PersonRepository { } private async createKeycloakUser( - person: Person, + person: Person, kcUserService: KeycloakUserService, - ): Promise | DomainError> { + ): Promise | DomainError> { if (person.keycloakUserId || !person.newPassword || !person.username) { return new EntityCouldNotBeCreated('Person'); } person.referrer = person.username; - const userDo: User = User.createNew(person.username, undefined); + const userDo: User = User.createNew(person.username, undefined, { + ID_ITSLEARNING: person.id, + }); const creationResult: Result = await kcUserService.create(userDo); if (!creationResult.ok) { @@ -324,15 +354,17 @@ export class PersonRepository { } private async createKeycloakUserWithHashedPassword( - person: Person, + person: Person, hashedPassword: string, kcUserService: KeycloakUserService, - ): Promise | DomainError> { + ): Promise | DomainError> { if (person.keycloakUserId || !person.username) { return new EntityCouldNotBeCreated('Person'); } person.referrer = person.username; - const userDo: User = User.createNew(person.username, undefined); + const userDo: User = User.createNew(person.username, undefined, { + ID_ITSLEARNING: person.id, + }); const creationResult: Result = await kcUserService.createWithHashedPassword( userDo, diff --git a/test/utils/do-factory.ts b/test/utils/do-factory.ts index 27c509cee..43a366392 100644 --- a/test/utils/do-factory.ts +++ b/test/utils/do-factory.ts @@ -79,6 +79,7 @@ export class DoFactory { createdDate: withId ? faker.date.past() : undefined, username: faker.internet.userName(), email: faker.internet.email(), + externalSystemIDs: {}, }; return Object.assign(Object.create(User.prototype) as User, user, props);