refactor(260607-l6l): extract shared deriveDisplayName helper (BUG 2 DRY)

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.
This commit is contained in:
Lucas Berger
2026-06-07 15:37:18 -04:00
parent 509f4b26e0
commit a99ef1daae
4 changed files with 48 additions and 32 deletions
+28
View File
@@ -26,6 +26,34 @@ export const COLOR_PALETTE: string[] = [
'#3AAFA9', // teal '#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>` — 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). * Upsert a user by their OIDC identity (iss + sub).
* *
+5 -11
View File
@@ -31,7 +31,7 @@ import { db } from '../db/client.js'
import { calendarEvents, calendars, users, calendarOutbox } from '../db/schema.js' import { calendarEvents, calendars, users, calendarOutbox } from '../db/schema.js'
import { expandOccurrences } from '../broker/expand.js' import { expandOccurrences } from '../broker/expand.js'
import { getAuth } from '../auth/middleware.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') // Side-effect import: brings in the ContextVariableMap augmentation for c.get('user')
import '../auth/devBypass.js' import '../auth/devBypass.js'
@@ -66,16 +66,10 @@ async function resolveUserId(c: any): Promise<number | null> {
const iss = (auth.iss as string | undefined) ?? '' const iss = (auth.iss as string | undefined) ?? ''
const sub = auth.sub ?? '' const sub = auth.sub ?? ''
// Derive displayName with same preference order as me.ts (name → preferred_username // Derive displayName via the shared helper (name → preferred_username → email
// → email → sub fallback). Both call sites must agree so a write-path upsert does not // → sub fallback) so the write-path upsert agrees with me.ts and never
// overwrite a correctly-derived name with a worse one. // overwrites a correctly-derived name with a worse one.
const claimStr = (v: unknown): string | undefined => const displayName = deriveDisplayName(auth, sub)
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)}`
const user = await upsertUser(iss, sub, displayName) const user = await upsertUser(iss, sub, displayName)
return user?.id ?? null return user?.id ?? null
+5 -17
View File
@@ -21,7 +21,7 @@
import { Hono } from 'hono' import { Hono } from 'hono'
import { getAuth } from '../auth/middleware.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') // Side-effect import: brings in the ContextVariableMap augmentation for c.get('user')
import '../auth/devBypass.js' import '../auth/devBypass.js'
@@ -53,22 +53,10 @@ meRouter.get('/', async (c) => {
const iss = (auth.iss as string | undefined) ?? '' const iss = (auth.iss as string | undefined) ?? ''
const sub = auth.sub ?? '' const sub = auth.sub ?? ''
// Derive the best available display name from OIDC claims, in preference order: // Derive the best available display name from OIDC claims (name →
// 1. name — full name set by the IdP (most human-friendly) // preferred_username → email → sub fallback). Shared helper keeps every
// 2. preferred_username — often the login handle; still readable // upsert call site in agreement (see deriveDisplayName).
// 3. email — readable but reveals contact info; acceptable fallback const displayName = deriveDisplayName(auth, sub)
// 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)}`
const user = await upsertUser(iss, sub, displayName) const user = await upsertUser(iss, sub, displayName)
+10 -4
View File
@@ -49,10 +49,16 @@ vi.mock('@hono/oidc-auth', () => ({
// Mock upsertUser for the OIDC path — CR-06 tests override mockUpsertUser.fn. // Mock upsertUser for the OIDC path — CR-06 tests override mockUpsertUser.fn.
const mockUpsertUserFn = vi.fn() const mockUpsertUserFn = vi.fn()
vi.mock('../../src/auth/user.js', () => ({ vi.mock('../../src/auth/user.js', async (importActual) => {
upsertUser: (...args: unknown[]) => mockUpsertUserFn(...args), // Keep deriveDisplayName (a pure helper) real so the displayName the handler
COLOR_PALETTE: ['#4A90D9', '#E8734A', '#5BA85A', '#9B6DC5', '#E8A840', '#3AAFA9'], // passes to upsertUser reflects production claim-preference logic; stub only
})) // upsertUser (the DB-touching function).
const actual = await importActual<typeof import('../../src/auth/user.js')>()
return {
...actual,
upsertUser: (...args: unknown[]) => mockUpsertUserFn(...args),
}
})
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
// Configurable mock for the DB. // Configurable mock for the DB.