From a99ef1daae96f2dccc12a976d5cbea8d51e3e5b7 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Sun, 7 Jun 2026 15:37:18 -0400 Subject: [PATCH] refactor(260607-l6l): extract shared deriveDisplayName helper (BUG 2 DRY) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The displayName claim-preference logic (name → preferred_username → email → sub fallback) was duplicated verbatim in me.ts and events.ts resolveUserId. Extract it to auth/user.ts as deriveDisplayName and use it in both call sites, so the rule has one definition. Update the events.test.ts user.js mock to keep the real helper (spread importActual) while stubbing only upsertUser. --- apps/api/src/auth/user.ts | 28 ++++++++++++++++++++++++++++ apps/api/src/routes/events.ts | 16 +++++----------- apps/api/src/routes/me.ts | 22 +++++----------------- apps/api/tests/routes/events.test.ts | 14 ++++++++++---- 4 files changed, 48 insertions(+), 32 deletions(-) diff --git a/apps/api/src/auth/user.ts b/apps/api/src/auth/user.ts index 5e423af..1814a62 100644 --- a/apps/api/src/auth/user.ts +++ b/apps/api/src/auth/user.ts @@ -26,6 +26,34 @@ export const COLOR_PALETTE: string[] = [ '#3AAFA9', // teal ] +/** Coerce an OIDC claim to a trimmed non-empty string, else undefined. */ +const claimStr = (v: unknown): string | undefined => + typeof v === 'string' && v.trim() !== '' ? v.trim() : undefined + +/** + * Derive the best available display name from OIDC claims, in preference order: + * 1. name — full name set by the IdP (most human-friendly) + * 2. preferred_username — often the login handle; still readable + * 3. email — readable but reveals contact info; acceptable fallback + * 4. `Member ` — sub is always present; never blank + * + * Shared by every call site that upserts a user (me.ts, events.ts resolveUserId) + * so a write-path upsert never overwrites a correctly-derived name with a worse + * one. Whether Authelia emits name/preferred_username is an operator config + * concern (userinfo scope + claim mappings) — out of scope here. + */ +export function deriveDisplayName( + claims: { name?: unknown; preferred_username?: unknown; email?: unknown }, + sub: string, +): string { + return ( + claimStr(claims.name) ?? + claimStr(claims.preferred_username) ?? + claimStr(claims.email) ?? + `Member ${String(sub).slice(0, 8)}` + ) +} + /** * Upsert a user by their OIDC identity (iss + sub). * diff --git a/apps/api/src/routes/events.ts b/apps/api/src/routes/events.ts index 8037922..5581da0 100644 --- a/apps/api/src/routes/events.ts +++ b/apps/api/src/routes/events.ts @@ -31,7 +31,7 @@ import { db } from '../db/client.js' import { calendarEvents, calendars, users, calendarOutbox } from '../db/schema.js' import { expandOccurrences } from '../broker/expand.js' import { getAuth } from '../auth/middleware.js' -import { upsertUser } from '../auth/user.js' +import { upsertUser, deriveDisplayName } from '../auth/user.js' // Side-effect import: brings in the ContextVariableMap augmentation for c.get('user') import '../auth/devBypass.js' @@ -66,16 +66,10 @@ async function resolveUserId(c: any): Promise { const iss = (auth.iss as string | undefined) ?? '' const sub = auth.sub ?? '' - // Derive displayName with same preference order as me.ts (name → preferred_username - // → email → sub fallback). Both call sites must agree so a write-path upsert does not - // overwrite a correctly-derived name with a worse one. - const claimStr = (v: unknown): string | undefined => - typeof v === 'string' && v.trim() !== '' ? v.trim() : undefined - const displayName = - claimStr(auth.name) ?? - claimStr(auth.preferred_username) ?? - claimStr(auth.email) ?? - `Member ${String(sub).slice(0, 8)}` + // Derive displayName via the shared helper (name → preferred_username → email + // → sub fallback) so the write-path upsert agrees with me.ts and never + // overwrites a correctly-derived name with a worse one. + const displayName = deriveDisplayName(auth, sub) const user = await upsertUser(iss, sub, displayName) return user?.id ?? null diff --git a/apps/api/src/routes/me.ts b/apps/api/src/routes/me.ts index ec86a0c..8b6e60e 100644 --- a/apps/api/src/routes/me.ts +++ b/apps/api/src/routes/me.ts @@ -21,7 +21,7 @@ import { Hono } from 'hono' import { getAuth } from '../auth/middleware.js' -import { upsertUser } from '../auth/user.js' +import { upsertUser, deriveDisplayName } from '../auth/user.js' // Side-effect import: brings in the ContextVariableMap augmentation for c.get('user') import '../auth/devBypass.js' @@ -53,22 +53,10 @@ meRouter.get('/', async (c) => { const iss = (auth.iss as string | undefined) ?? '' const sub = auth.sub ?? '' - // Derive the best available display name from OIDC claims, in preference order: - // 1. name — full name set by the IdP (most human-friendly) - // 2. preferred_username — often the login handle; still readable - // 3. email — readable but reveals contact info; acceptable fallback - // 4. sub — always present; not human-friendly but never blank - // - // Each candidate is tested defensively — Authelia may omit or blank-out any claim. - // Whether Authelia emits name/preferred_username is an operator configuration concern - // (e.g. userinfo scope, claim mappings in authelia config) — out of scope here. - const claimStr = (v: unknown): string | undefined => - typeof v === 'string' && v.trim() !== '' ? v.trim() : undefined - const displayName = - claimStr(auth.name) ?? - claimStr(auth.preferred_username) ?? - claimStr(auth.email) ?? - `Member ${String(sub).slice(0, 8)}` + // Derive the best available display name from OIDC claims (name → + // preferred_username → email → sub fallback). Shared helper keeps every + // upsert call site in agreement (see deriveDisplayName). + const displayName = deriveDisplayName(auth, sub) const user = await upsertUser(iss, sub, displayName) diff --git a/apps/api/tests/routes/events.test.ts b/apps/api/tests/routes/events.test.ts index 883b59f..2116943 100644 --- a/apps/api/tests/routes/events.test.ts +++ b/apps/api/tests/routes/events.test.ts @@ -49,10 +49,16 @@ vi.mock('@hono/oidc-auth', () => ({ // Mock upsertUser for the OIDC path — CR-06 tests override mockUpsertUser.fn. const mockUpsertUserFn = vi.fn() -vi.mock('../../src/auth/user.js', () => ({ - upsertUser: (...args: unknown[]) => mockUpsertUserFn(...args), - COLOR_PALETTE: ['#4A90D9', '#E8734A', '#5BA85A', '#9B6DC5', '#E8A840', '#3AAFA9'], -})) +vi.mock('../../src/auth/user.js', async (importActual) => { + // Keep deriveDisplayName (a pure helper) real so the displayName the handler + // passes to upsertUser reflects production claim-preference logic; stub only + // upsertUser (the DB-touching function). + const actual = await importActual() + return { + ...actual, + upsertUser: (...args: unknown[]) => mockUpsertUserFn(...args), + } +}) // --------------------------------------------------------------------------- // Configurable mock for the DB.