From 6c6fd2b85946da2dded21808234c2ed2b2854475 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Tue, 1 Sep 2026 14:17:58 +0200 Subject: [PATCH] refactor(server-nestjs): route Keycloak group creation through shared ensure util Refs: #2618 Signed-off-by: William Phetsinorath Change-Id: I05b26d3c6452b05e7c6243852fd35c116a6a6964 --- .../keycloak/keycloak-client.service.ts | 55 ++++++++----------- .../src/modules/keycloak/keycloak.utils.ts | 34 ++++++++++++ 2 files changed, 57 insertions(+), 32 deletions(-) diff --git a/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts b/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts index 22e42eaf09..6f0a0ad96b 100644 --- a/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts +++ b/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts @@ -12,7 +12,7 @@ import { keycloakConfigFactory } from '../../config/keycloak.config' import { getErrorResponseStatus } from '../../utils/http.utils' import { StartActiveSpan } from '../infrastructure/telemetry/telemetry.decorator' import { ADMIN_AUTH_REALM, ADMIN_TOKEN_REFRESH_INTERVAL_MS, CONSOLE_GROUP_NAME, PASSWORD_GRANT_TYPE, REFRESH_TOKEN_GRANT_TYPE, SUBGROUPS_PAGINATE_QUERY_MAX } from './keycloak.constants' -import { groupSchema, splitGroupPath } from './keycloak.utils' +import { ensure, groupSchema, splitGroupPath } from './keycloak.utils' export const KEYCLOAK_ADMIN_CLIENT = Symbol('KEYCLOAK_ADMIN_CLIENT') @@ -129,21 +129,16 @@ export class KeycloakClientService implements OnModuleInit { const span = trace.getActiveSpan() span?.setAttribute('group.name', name) this.logger.debug(`Creating Keycloak group ${name}`) - try { - await this.client.groups.create({ name }) - } catch (err) { - // A concurrent reconciliation (e.g. the cron sync and a project upsert) - // may have created the root group between the read and the create; treat - // the 409 as "already exists" and re-fetch it. - if (getErrorResponseStatus(err) !== 409) throw err - this.logger.verbose(`Keycloak group ${name} was created concurrently, fetching it`) - const existing = await this.getRootGroupByName(name) - if (!existing) throw err - return existing - } - const created = await this.getRootGroupByName(name) - if (!created) throw new Error(`Created Keycloak group ${name} but could not fetch it back`) - return created + return ensure({ + create: async () => { + await this.client.groups.create({ name }) + const created = await this.getRootGroupByName(name) + if (!created) throw new Error(`Created Keycloak group ${name} but could not fetch it back`) + return created + }, + reload: () => this.getRootGroupByName(name), + onCollision: () => this.logger.verbose(`Keycloak group ${name} was created concurrently, fetching it`), + }) } async addUserToGroup(userId: string, groupId: string) { @@ -213,22 +208,18 @@ export class KeycloakClientService implements OnModuleInit { if (existing) return existing this.logger.debug(`Creating Keycloak subgroup ${name} under parentId=${parentId}`) - try { - // createChildGroup only returns the new id; re-list the parent to hand - // back the representation Keycloak computed (name, path, subGroups...) - await this.client.groups.createChildGroup({ id: parentId }, { name }) - const created = await this.getSubGroupByName(parentId, name) - if (!created) throw new Error(`Created Keycloak subgroup ${name} under parentId=${parentId} but could not fetch it back`) - return created - } catch (err) { - // A concurrent reconciliation may have created the subgroup between the - // scan and the create; treat the 409 as "already exists" and re-fetch it - if (getErrorResponseStatus(err) !== 409) throw err - this.logger.verbose(`Keycloak subgroup ${name} was created concurrently under parentId=${parentId}, fetching it`) - const subgroup = await this.getSubGroupByName(parentId, name) - if (!subgroup) throw err - return subgroup - } + return ensure({ + create: async () => { + // createChildGroup only returns the new id; re-list the parent to hand + // back the representation Keycloak computed (name, path, subGroups...) + await this.client.groups.createChildGroup({ id: parentId }, { name }) + const created = await this.getSubGroupByName(parentId, name) + if (!created) throw new Error(`Created Keycloak subgroup ${name} under parentId=${parentId} but could not fetch it back`) + return created + }, + reload: () => this.getSubGroupByName(parentId, name), + onCollision: () => this.logger.verbose(`Keycloak subgroup ${name} was created concurrently under parentId=${parentId}, fetching it`), + }) } async getOrCreateConsoleGroup(projectGroup: GroupRepresentationWith<'id'>) { diff --git a/apps/server-nestjs/src/modules/keycloak/keycloak.utils.ts b/apps/server-nestjs/src/modules/keycloak/keycloak.utils.ts index 00199085af..7796c874a6 100644 --- a/apps/server-nestjs/src/modules/keycloak/keycloak.utils.ts +++ b/apps/server-nestjs/src/modules/keycloak/keycloak.utils.ts @@ -1,7 +1,9 @@ import type GroupRepresentation from '@keycloak/keycloak-admin-client/lib/defs/groupRepresentation' import type UserRepresentation from '@keycloak/keycloak-admin-client/lib/defs/userRepresentation' import type { ProjectWithDetails } from './keycloak-datastore.service' +import { HttpStatus } from '@nestjs/common' import z from 'zod' +import { getErrorResponseStatus } from '../../utils/http.utils' import { CONSOLE_GROUP_NAME } from './keycloak.constants' type With = T & Required> @@ -47,3 +49,35 @@ export function toRoleRelativeGroupPath( ): string { return role.oidcGroup.replace(consoleGroup.path, '') } + +// Whether a Keycloak admin-client error signals an entity already existing +// (race collision): a 409 conflict on the create call. +export function isKeycloakConflict(error: unknown): boolean { + return getErrorResponseStatus(error) === HttpStatus.CONFLICT +} + +// Runs an idempotent write: tries `create`, and on a Keycloak race collision +// reloads via `reload` and returns the existing entity instead of failing. +// `onCollision` is invoked once when a collision is detected. If the reload +// finds nothing, the original error is rethrown so genuine failures are not +// swallowed. +export async function ensure({ + create, + reload, + onCollision, +}: { + create: () => Promise + reload: () => Promise + onCollision?: (error: unknown) => void +}): Promise { + try { + return await create() + } catch (error) { + if (isKeycloakConflict(error)) { + onCollision?.(error) + const existing = await reload() + if (existing) return existing + } + throw error + } +}