Claude finished @Yeraze's task —— View job

PR Review: feat(auth): native OIDC group → role mapping + SSO docs fixes

• Gather context
• Read changed files
• Review code quality and security
• Post review feedback

This is a well-structured PR that fills a genuine gap in the auth stack. The core logic is clean and the factoring into claims.ts is the right call. A few observations worth addressing:

Security

getNestedValue accepts arbitrary paths from env config (low risk, but worth noting)

src/server/auth/claims.ts:17-28 — the obj[path] lookup for URL-style claims passes path directly as an object key. Since path comes from OIDC_GROUPS_CLAIM (operator-controlled env var), this isn't a user-controlled injection vector, but it's worth a code comment clarifying the trust boundary to prevent future confusion.

Username derived from sub with no sanitization (oidcAuth.ts:190)

const username = preferredUsername || email?.split('@')[0] || sub.substring(0, 20);

The sub.substring(0, 20) fallback could produce usernames with characters that are problematic depending on other parts of the system (e.g. /, \, or null bytes if sub is exotic). A light sanitization or character allowlist on this path would be defensive. This is pre-existing code, but the new OIDC paths make it more reachable.

OIDC_CLIENT_SECRET not logged (good), but OIDC_ADMIN_GROUPS is logged in plaintext

environment.ts:737 logs the configured admin group names. This is intentional and matches PROXY_AUTH_ADMIN_GROUPS behavior. Just confirming the parity is deliberate.

Code Quality

Unnecessary null initialization and re-assignment (oidcAuth.ts:211-212)

let user: User | null = null;
user = await databaseService.auth.getUserByOidcSubject(sub) as User | null;

This is a pre-existing pattern, but the new code around line 239-245 repeats it for existingUser:

let existingUser: User | null = null;
existingUser = await databaseService.findUserByUsernameAsync(username) as User | null;

These can be const with a direct await — no functional impact but cleaner.

databaseService.auditLogAsync is fire-and-forget without error handling (oidcAuth.ts:269-275, 346-352)

databaseService.auditLogAsync(
  user!.id,
  'user_migrated_to_oidc',
  ...
);

This is a pre-existing pattern in the file, but it means audit log failures are silent. A void prefix or .catch(logger.error) would make the intentionality explicit.

getEnvironmentConfig() called at callback time, not once at module load

oidcAuth.ts:195 — env config is re-fetched on every OIDC callback. This is fine for production (the config is a cached singleton), but it differs from the pattern in proxyAuth.ts where getEnvironmentConfig() is called inside each function rather than cached at the top. Consistent, so no issue — just noting it.

Test Coverage

The claims.test.ts suite (src/server/auth/claims.test.ts) covers the pure functions well. Two gaps worth considering:

1. Integration: no test for the oidcAuth.handleOIDCCallback path with group restrictions. The OIDC callback logic (oidcAuth.ts:196-208) — particularly the !groupRole.allowed rejection — is only exercised by the pure resolveGroupRole test. An integration-level test for handleOIDCCallback with a mock databaseService and mock token response would catch regressions in the wiring (e.g. if someone reorders the allowed-check and user-lookup).

2. Missing: test for normalizeGroups with a non-string, non-array, non-{name} object (e.g. { value: 'admin' }). Currently normalizeGroups({value:'admin'}) silently returns [] — that's the right fail-closed behavior, but an explicit test makes it a contract rather than an accident.

Minor / Nits

• oidcAuth.ts:224: The condition groupRole.adminGroupsConfigured && groupRole.isAdmin !== user.isAdmin only updates isAdmin on change. This avoids a no-op DB write, which is good, but means a demotion from true→false requires isAdmin !== user.isAdmin to be truthy — which it is. Logic is correct but a comment would help readers.

• claims.ts:48: The { name: unknown } object detection is Keycloak/SCIM specific. If other IdPs use { value: string } (SCIM complexObjects), this would silently drop those groups. Not a blocker, but worth a doc comment.

• environment.ts:576-583: oidcAdminGroups and oidcAllowedGroups don't have a *Provided boolean in the interface, unlike most other array configs. This is inconsistent with the pattern (compare proxyAuthAdminGroups — also no Provided bool). Consistent omission, acceptable.

Summary

The PR is solid: the abstraction layer in claims.ts is clean, the fail-closed behavior (allowed=false when groups list is configured but user isn't in it) is correct, and the backward-compatibility guarantee (no change when OIDC_ADMIN_GROUPS is unset) is well-tested. The main actionable items are:

1. Consider adding an integration test for handleOIDCCallback with the allowed-groups gate to catch wiring regressions.
2. Sanitize or validate the sub.substring(0,20) username fallback.
3. Add void or .catch() to the fire-and-forget auditLogAsync calls to make intentionality explicit.

Items 2 and 3 are pre-existing patterns — whether to fix them in this PR is up to the maintainer.