fix(auth): link existing local accounts to OIDC by verified email

[StefanWegener]

Summary

• Local accounts are now linked to OIDC login by verified email instead of creating a duplicate user
• Adds user-facing messaging on the login page when an account link occurs
• Adds English and German translations for the new login page copy

Fixes #1865

Test plan

• Unit tests pass (95%+ coverage)
• Integration tests pass
• Pre-commit hook quality gates pass
• CI Quality Gates pass

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

[steilerDev]

[ux-designer]

Scope reviewed: the two new i18n strings (oidcErrors.oidc_no_matching_account in client/src/i18n/en/auth.json and client/src/i18n/de/auth.json) and the one-line knownCodes addition in client/src/pages/LoginPage/LoginPage.tsx. No CSS changed.

Findings

• Low — Copy tone drift (EN only). client/src/i18n/en/auth.json: the new string reads "Ask an administrator to create an account for you first." All three sibling messages that end in an administrator call-to-action (account_deactivated, and the pattern established across the file) use "Please contact an administrator." The DE string you shipped is actually consistent with the sibling pattern ("Bitte wenden Sie sich an einen Administrator, damit zunächst ein Konto für Sie angelegt wird.") — it's specifically the EN copy that diverges in register/verb choice from its own DE translation and from account_deactivated. Suggest something like "No account was found for your email address. Please contact an administrator to have an account created for you." for consistency. Non-blocking; a copy nit, not a design system violation.

• Informational — DE string length. At ~150 characters it's the longest message this banner has ever carried (vs. ~70-105 chars for the existing siblings). Checked client/src/pages/shared/AuthPage.module.css .errorBanner: no fixed height, no white-space: nowrap, no overflow/text-truncation — it's a plain padded block that wraps naturally, so the longer message will just take 2-3 lines instead of 1-2. No clipping or overflow risk at the .card max-width (28rem desktop, full-width minus padding on mobile per the @media (max-width: 767px) rule). No action needed.

• Confirmed — Accessibility. The banner (<div className={sharedStyles.errorBanner} role="alert"> in LoginPage.tsx) is pre-existing and unchanged; role="alert" already implies an assertive live region, so the new message is announced to screen readers exactly like the other five OIDC error codes that share this same render path. No ARIA changes needed or missing.

• Confirmed — Token adherence / dark mode. No CSS was touched by this PR. .errorBanner uses --color-danger-bg / --color-danger-border / --color-danger-active, which already have light and dark-mode definitions in tokens.css (lines ~140-146 light, ~695-701 dark) and are exercised by five other error codes already in production — no new contrast risk introduced.

• Style Guide impact: none. This is copy-only reuse of an existing, already-documented error banner pattern.

Verdict: APPROVED. Findings are low/informational only — proportionate to the one-line component change plus two translated strings.

[steilerDev]

[frontend-developer]

Review Summary

✅ Approved — The implementation is correct and follows all established patterns.

Verification

Component wiring (LoginPage.tsx):

• ✅ New error code 'oidc_no_matching_account' correctly added to knownCodes array
• ✅ i18n key path t(\login.oidcErrors.${errorCode}`)` resolves correctly to the new key in both locales
• ✅ Existing eslint-disable comment for @eslint-react/set-state-in-effect remains appropriately scoped (only around setApiError call initializing from URL params)
• ✅ email_conflict correctly retained in knownCodes (still used by backend OIDC flow for ConflictError case)

i18n structure (en/auth.json and de/auth.json):

• ✅ login.oidcErrors.oidc_no_matching_account key exists in both locales with identical path structure
• ✅ English: "No account was found for your email address. Ask an administrator to create an account for you first."
• ✅ German: "Für Ihre E-Mail-Adresse wurde kein Konto gefunden. Bitte wenden Sie sich an einen Administrator, damit zunächst ein Konto für Sie angelegt wird."
• ✅ No missing or extra keys between locales

Test coverage (LoginPage.test.tsx):

• ✅ New test follows exact pattern of existing error code tests (lines 109–159)
• ✅ Correct placement in test sequence (between account_deactivated and the "no error in URL" test)
• ✅ Uses window.history.pushState, renderWithAuth, and screen.findByText with case-insensitive regex matching the message content
• ✅ Test validates the rejection path without requiring a second OIDC provider identity

Minor observation (informational):
German translations were added by the contributor, which deviates from CLAUDE.md convention (translator agent owns non-English translations). The translations themselves appear correct and will be validated by the translator agent. No action needed — this is acceptable for external contributions.

[steilerDev]

[security-engineer]

Verdict: Request Changes

Two findings block merge (Critical, High) — both are new attack surface introduced by the linking logic itself, not pre-existing issues. The overall design direction (auto-link on email match, reject unmatched, no manual/password-confirmation step) is correct and matches the accepted trust model from issue #1865 — I am not re-litigating that decision. The problem is that the implementation never actually verifies the email is verified, despite the PR title and issue explicitly promising "verified email."

---

[CRITICAL] No email_verified claim check — unverified email enables full account takeover (including admin)

OWASP: A07 Identification & Authentication Failures / A01 Broken Access Control
Files:

• server/src/services/oidcService.ts (unchanged by this PR) — handleCallback() extracts email straight from tokenResponse.claims() with no check on claims.email_verified.
• server/src/services/userService.ts — new findOrLinkOidcUser() uses that raw, unverified email as the sole credential to link to (and inherit the role of) an existing account.

The PR title says "link ... by verified email," and issue #1865 states plainly: "a verified email match against a pre-existing account is the only thing that authorizes access." But no code anywhere checks email_verified. oidcService.ts isn't even touched by this diff.

Exploit scenario: The trust model assumes the admin's IdP only asserts emails it has verified (true for Google et al.). But Cornerstone doesn't enforce or document that requirement, and many OIDC providers admins might plausibly self-host (Keycloak/Authentik with self-registration enabled, generic IdPs, some enterprise bridges) either omit email_verified or set it false for user-entered addresses that were never confirmed. An attacker who can register/control an identity on the configured IdP sets their email claim to admin@example.com (the real household admin's address, easily guessable/known) and completes SSO. findOrLinkOidcUser silently sets oidcSubject on the admin's existing local account and returns it — role: admin intact, no password check, no confirmation. The attacker is immediately logged in as admin: full household financial/document access.

Remediation:

// oidcService.ts, handleCallback()
const emailVerified = claims.email_verified === true;
const email = typeof claims.email === 'string' && emailVerified ? claims.email : '';

This reuses the existing missing_email rejection path in oidc.ts for unverified emails — no route changes needed.

Risk if unaddressed: Unauthenticated attacker takeover of the admin account on any deployment whose configured IdP doesn't strictly enforce email verification. Directly contradicts the documented trust model this PR claims to implement.

---

[HIGH] Silent re-binding of an already-linked oidcSubject — no conflict check on overwrite

OWASP: A01 Broken Access Control
File: server/src/services/userService.ts, findOrLinkOidcUser() — emailUser = findByEmail(...); if emailUser.oidcSubject is already set to a different value, it is unconditionally overwritten by db.update(users).set({ oidcSubject: sub, ... }) with zero check, zero alert.

Exploit scenario: Combine with the Critical finding above — even after the real admin has already linked their genuine Google account (sub=S1), an attacker exploiting the missing email_verified check can re-run the callback with a spoofed admin@example.com + attacker-controlled sub=S2. The code has no "already linked to a different sub" guard, so it silently rebinds oidcSubject from S1 to S2. The real admin is now locked out of OIDC login (their sub=S1 matches nothing), while the attacker owns persistent OIDC access to the admin account. This also has an independent (lower-likelihood) trigger even with email_verified enforced: IdP email-address recycling (e.g. a Workspace admin reassigns an off-boarded address) would silently hijack the binding the same way.

Remediation: If emailUser.oidcSubject is set and differs from the incoming sub, do not overwrite silently — reject (e.g. a new OIDC_SUBJECT_MISMATCH error) and log at warn/error so it's visible to the admin, rather than treating it as a normal first-link.

---

[MEDIUM] Linking mutation happens before the deactivatedAt guard — unauthenticated write to a deactivated account

File: server/src/routes/oidc.ts — const user = userService.findOrLinkOidcUser(fastify.db, sub, email); performs the DB UPDATE immediately; the if (user.deactivatedAt) { ... } check runs only afterward. Confirmed by this PR's own test (server/src/routes/oidc.test.ts, "redirects to /login?error=account_deactivated for a deactivated linked account"), which explicitly asserts linkedUser?.oidcSubject was set even though login was rejected.

Exploit scenario: An unauthenticated caller who knows a deactivated household member's email (ex-partner, former tenant, previously-removed member) and controls a matching IdP identity (or spoofs it, per the Critical finding) can hit the callback endpoint and permanently bind their sub to that dormant account with no session ever granted — silent, no alarm. If the admin later reactivates that account (the person returns, or a bulk reactivate), the attacker's IdP identity resolves straight into it on their next login.

Remediation: Check deactivatedAt before performing the link mutation — split the read-only lookup from the write, or re-check-and-abort inside findOrLinkOidcUser before the UPDATE.

---

[MEDIUM] idx_users_oidc_lookup unique index no longer matches the new query semantics

File: server/src/db/schema.ts:44-47 — uniqueIndex('idx_users_oidc_lookup').on(table.authProvider, table.oidcSubject).where(isNotNull(table.oidcSubject)). This PR's findByOidcSubject() now queries WHERE oidc_subject = ? alone (no authProvider filter, correctly enabling linked local accounts to be found by sub), but the DB index still partitions uniqueness by authProvider. It does not prevent a legacy pre-#1865 authProvider='oidc' row and a newly-linked authProvider='local' row from holding an identical oidc_subject value; .get() on such a collision returns a non-deterministic row. This is flagged explicitly as a to-do in issue #1865's own notes ("Login-by-sub lookup ... currently filters on authProvider = 'oidc', so this must be reconciled") but wasn't addressed.

Remediation: Add a migration replacing the index with UNIQUE(oidc_subject) WHERE oidc_subject IS NOT NULL (drop authProvider from the key) so the DB enforces the invariant the application code now relies on.

---

[LOW] Email matching is case-sensitive / unnormalized (availability only, not a takeover vector)

File: server/src/services/userService.ts, findByEmail() — eq(users.email, email), no COLLATE NOCASE. If casing differs between the locally-stored account and the IdP's asserted email, linking fails and the user is wrongly told oidc_no_matching_account. This can only cause under-matching (denial of linking), never a wrong-account match, since the existing email column is itself uniquely constrained case-sensitively — no cross-account collision risk. Informational/functional, not blocking.

---

Standard checks — no findings

• No new dependencies added.
• isSafeRedirect() / open-redirect handling in oidc.ts is unchanged and still correct.
• No secrets or credentials introduced.
• Session creation is fresh post-link (no session fixation).
• Distinct oidc_no_matching_account vs account_deactivated vs success responses do allow limited email enumeration by a caller who can obtain arbitrary-email IdP identities — but that capability is exactly the Critical finding above; fixing email_verified closes this too, no separate action needed.
• Logging { error, sub } on rejected callbacks is good practice (audit trail), not a concern.

---

Summary: Please add the email_verified check to oidcService.ts (Critical) and a subject-mismatch guard in findOrLinkOidcUser (High) before merge. The Medium findings (deactivated-account write-before-guard, stale unique index) should be fixed in this PR or as fast immediate follow-ups given they compound with the Critical/High issues above.

[steilerDev]

[backend-developer]

I reviewed the backend changes for Fastify patterns, Drizzle conventions, transactional safety, and error handling. I found one blocker-level finding that must be addressed before approval.

🔴 BLOCKER: Query Determinism Risk in findByOidcSubject — MEDIUM

File: server/src/services/userService.ts lines 187–192 (new code)

The new query removes the authProvider filter:

// OLD
where(and(eq(users.authProvider, 'oidc'), eq(users.oidcSubject, sub)))

// NEW
where(eq(users.oidcSubject, sub))

The Problem:
The schema defines a composite unique index on (authProvider, oidcSubject). This allows multiple rows with the same oidcSubject but different authProvider:

• (authProvider='local', oidcSubject='sub-123') — linked local account ✓
• (authProvider='oidc', oidcSubject='sub-123') — legacy OIDC-only account

If both exist, the query WHERE oidcSubject = 'sub-123' matches both rows. The .get() call returns the first result in undefined order (no ORDER BY), violating determinism.

When Does This Fail?
If the old auto-provisioning code was deployed to production, legacy OIDC-only accounts exist. After this PR, they'd collide with newly linked local accounts, causing non-deterministic lookups.

Fix: Choose one:

1. Add explicit ordering (preferred — prioritize linked local accounts):

db.select().from(users)
  .where(eq(users.oidcSubject, sub))
  .orderBy(desc(sql`CASE WHEN ${users.authProvider} = 'local' THEN 0 ELSE 1 END`))
  .get()

2. Filter to authProvider='local' (if legacy OIDC accounts must never match):

where(and(eq(users.oidcSubject, sub), eq(users.authProvider, 'local')))

3. Document a pre-deployment migration to clean legacy OIDC accounts.

---

📌 LOW: Unused Import Cleanup

File: server/src/services/userService.ts

Verify that and from drizzle-orm is still used elsewhere in the file. The findByOidcSubject refactor removed its only usage. If unused, remove from the import statement.

---

✅ PASSED Checks

Drizzle patterns: Read-after-write pattern matches existing code (updateDisplayName, etc.); non-null assertion is safe in better-sqlite3's synchronous single-threaded model.

Error class: OidcNoMatchingAccountError follows conventions; HTTP 403 is appropriate.

Route handling: Error catch and redirect are correct; ConflictError removal is reasonable.

API contract & i18n: ErrorCode added to types, tests updated, i18n keys in place, LoginPage handles param.

Test coverage: New tests thoroughly cover happy path (linking, repeat login) and rejection (no match = no user created, distinct error).

---

VERDICT: REQUEST_CHANGES. The determinism risk must be fixed before merge. Once resolved, code quality is solid.

[steilerDev]

[product-architect]

Architecture review of the OIDC account-linking change. The core design call — OIDC as an alternate login method for pre-existing accounts, never a self-registration path — is right, and findOrLinkOidcUser implements AC1–AC6 faithfully. The rename from findOrCreateOidcUser is accurate, the new error class follows the AppError pattern, and no new dependencies were added.

Verdict: request changes. One Critical (the "verified email" in the PR title is never actually verified), two High (schema constraint no longer matches the new lookup key, with no migration; wiki documents the exact invariant this PR inverts). Details and remediation below.

---

Critical

C1 — email_verified is never checked, yet the email claim is now the sole authorization gate

server/src/services/oidcService.ts:106-107 (unchanged by this PR):

const sub = claims.sub;
const email = typeof claims.email === 'string' ? claims.email : '';

There is no email_verified check anywhere in the callback path. Before this PR that was tolerable: an email collision with an existing account threw ConflictError and blocked login, and a non-colliding email only ever produced a fresh member. After this PR, a bare email string claim is the one and only thing that grants access to an already-existing account, including the admin account, with its role and password preserved (userService.ts findOrLinkOidcUser, email branch).

Issue #1865 states the trust model as "a verified email match against a pre-existing account is the only thing that authorizes access," and this PR's own title says "by verified email." The code does not implement that. With any IdP that permits a self-asserted or unverified email address on the profile (or a multi-tenant provider where the admin's OAuth client accepts external tenants), an attacker sets their profile email to the admin's address and receives a linked admin session. The deactivatedAt guard does not help — the target account is active by definition.

Remediation — in oidcService.handleCallback, gate the email claim:

const emailVerified = claims.email_verified;
const email =
  typeof claims.email === 'string' && emailVerified === true ? claims.email : '';

email === '' already routes to the existing ?error=missing_email redirect (oidc.ts:118-121), so this fails closed with no new error code. Prefer a distinct code (e.g. oidc_email_unverified) so operators can tell "IdP sent no email" from "IdP sent an unverified email."

Strict === true is deliberate: some IdPs omit email_verified entirely. Omission must fail closed, because the whole model rests on this claim. If you want to support such IdPs, that has to be an explicit, documented opt-out (e.g. OIDC_TRUST_UNVERIFIED_EMAIL=true, default false) — not a silent fallback.

This also needs a route-level test asserting that a callback with email_verified: false creates no session and links no account.

---

High

H1 — idx_users_oidc_lookup no longer matches the lookup key; no migration included

findByOidcSubject drops the authProvider predicate (userService.ts):

return db.select().from(users).where(eq(users.oidcSubject, sub)).get();

But the index is still on the pair (server/src/db/schema.ts:45-47, created in 0001_create_users_and_sessions.sql:17-19):

CREATE UNIQUE INDEX idx_users_oidc_lookup
  ON users (auth_provider, oidc_subject)
  WHERE oidc_subject IS NOT NULL;

Two consequences:

1. Uniqueness of the new correlation key is unenforced. The DB permits (local, 'S') and (oidc, 'S') to coexist; findByOidcSubject then resolves an authentication lookup with an unordered .get() — i.e. non-deterministically. To be fair: I could not construct a path that produces such a pair through findOrLinkOidcUser today, since it short-circuits on findByOidcSubject before ever linking. So this is latent, not exploitable as written. But an auth correlation key should be guarded by the database, not by the current shape of one function.
2. The index cannot serve the query. SQLite requires a leftmost-column prefix, and auth_provider is the leading column, so WHERE oidc_subject = ? full-scans users on every SSO login. Irrelevant at 5 users, but the index named idx_users_oidc_lookup is now vestigial for the lookup it is named for.

No migration file and no schema.ts change are in this PR.

Remediation — add server/src/db/migrations/0045_users_oidc_subject_unique.sql:

-- Issue #1865: OIDC subject is now the sole correlation key for login,
-- independent of auth_provider (a linked local account keeps auth_provider='local').
DROP INDEX IF EXISTS idx_users_oidc_lookup;

CREATE UNIQUE INDEX idx_users_oidc_lookup
  ON users (oidc_subject)
  WHERE oidc_subject IS NOT NULL;

and mirror it in schema.ts:

oidcLookupIdx: uniqueIndex('idx_users_oidc_lookup')
  .on(table.oidcSubject)
  .where(isNotNull(table.oidcSubject)),

The partial predicate must be retained so the many rows with oidc_subject IS NULL are not indexed and do not collide.

On backfill (answering the question in the issue): none is required. Legacy auto-provisioned rows carry auth_provider='oidc' with a non-null oidc_subject, and the new predicate-free lookup finds them unchanged — they keep working, and the tightened index is satisfied by them as long as no duplicate sub already exists (it cannot, given the old pair index plus the fact that only one row per provider was ever written). Do not rewrite them to 'local'.

H2 — Wiki is not updated; one page states the exact invariant this PR inverts

Per CLAUDE.md, schema/API/decision changes update the wiki in the same PR. Nothing in wiki/ changed here. Concretely:

• wiki/Schema.md:50 — "Nullable password_hash / oidc_subject: These are mutually exclusive by auth_provider. Local users have password_hash but no oidc_subject… Application-layer validation enforces this invariant." This PR's entire purpose is to make a local account hold both. This is now actively wrong and needs rewriting plus a Deviation Log entry.
• wiki/Schema.md:32 — oidc_subject described as "only for OIDC users."
• wiki/Schema.md:42, 48, 98-100 — index columns, rationale ("prevents duplicate OIDC subjects per provider"), and DDL all need the H1 change.
• wiki/API-Contract.md:337 — "Looks up the user by (auth_provider='oidc', oidc_subject=sub)" — no longer true. The whole callback description (create-on-miss → link-or-reject) needs rewriting.
• wiki/API-Contract.md:351, 357 — email_conflict redirect param and its explanation are now unreachable; oidc_no_matching_account is missing from that table.
• wiki/API-Contract.md:5869-5870 — global error code table needs an OIDC_NO_MATCHING_ACCOUNT / 403 row.
• wiki/ADR-010-Authentication-Architecture.md:12, 18 — both still assert "automatic user provisioning on first login," which this PR removes.

Remediation: update the four pages above, and amend ADR-010 with a decision record for (a) no OIDC self-registration, (b) auto-link on verified email, (c) auth_provider = how the account was created, oidc_subject IS NOT NULL = OIDC-capable, (d) the tightened index. Given the trust-model reasoning is substantive and security-relevant, a standalone ADR-035-OIDC-Account-Linking.md superseding the relevant part of ADR-010 is also acceptable — either way, link it from ADR-Index.md.

---

Medium

M1 — Silent sub re-binding: accepted, but it must be audit-logged

Issue #1865 asked the architect to rule on whether a ConflictError branch should be retained when the matched email account already carries a different oidcSubject. My decision: keep the current overwrite behavior — do not add a conflict branch. Rationale:

• Under the stated trust model, a verified email match is sufficient to authorize the link. If it authorizes creating one, it authorizes replacing one; a conflict branch would be inconsistent.
• It makes IdP migration self-healing. Changing issuer (Google → Authentik) changes sub. With manual linking/unlinking UI explicitly out of scope, a ConflictError here would permanently lock the admin out of SSO with no in-app recovery — only direct DB surgery.

This decision is contingent on C1 being fixed. Without an email_verified gate, silent re-binding is an account-takeover primitive rather than a convenience.

Required change: the overwrite is currently completely silent — findOrLinkOidcUser emits no log on either first link or re-bind. Re-pointing an existing identity is a security-relevant state change and must be observable:

if (emailUser.oidcSubject && emailUser.oidcSubject !== sub) {
  // caller logs at warn: previousSub -> sub, userId
}

Surface it so oidc.ts can fastify.log.warn({ userId, previousSub, sub }, 'OIDC subject re-bound for existing account'). Record the decision and its email_verified precondition in the ADR from H2.

M2 — authProvider semantics drift is user-visible

auth_provider now means "how the account was created," not "how it authenticates" — but two UI surfaces still present it as the latter:

• client/src/pages/ProfilePage/ProfilePage.tsx:218 — renders profile.authLocal / profile.authOidc
• client/src/pages/UserManagementPage/UserManagementPage.tsx:383-385 — "Auth Provider" column, Local / OIDC

After linking, a user who signs in exclusively via SSO sees "Auth Provider: Local". Your own E2E test now encodes this (e2e/tests/auth/oidc.spec.ts, expect(me.user.authProvider).toBe('local')).

Options: (a) derive a third display value when oidcSubject is non-null (e.g. "Local + SSO"), which needs UserResponse to expose a boolean like oidcLinked — note oidc_subject itself is documented as never exposed (API-Contract.md:508), so expose a derived flag, not the subject; or (b) accept the drift and redefine the label. Either way it needs a decision in the ADR. Not a merge blocker on its own, but do not leave it undecided.

(ProfilePage.tsx:193 isLocalAuth gating the change-password form is correct as-is — a linked account keeps its password.)

M3 — Email matching is case-sensitive and is now the authorization gate

findByEmail uses eq(users.email, email) and email TEXT UNIQUE has no COLLATE NOCASE (schema.ts:29, 0001_*.sql:6). If the account was created as Admin@Example.com and the IdP sends admin@example.com, the link silently fails.

It fails closed, so this is not a security issue. But the resulting message — "Ask an administrator to create an account for you first" — is actively misleading when the person reading it is the administrator, and there is no self-serve recovery. At minimum document it; better, normalize on both sides (compare lowercased) and state the rule in the API contract.

M4 — plan/REQUIREMENTS.md still mandates automatic provisioning

Lines 19, 233, and 301 all specify "automatic user provisioning" as an OIDC requirement. This PR deliberately removes it, and issue #1865 justifies that as a security fix — so I read this as an intentional supersede, not a blocker for this PR. But the requirements document cannot be left contradicting shipped behavior. Please either update those three lines in this PR or open a follow-up issue for product-owner and reference it here.

---

Low

• L1 — email_conflict is now dead code. No code path can produce the redirect param after this change, yet it remains in client/src/pages/LoginPage/LoginPage.tsx:53 (knownCodes), both client/src/i18n/{en,de}/auth.json, and a test at LoginPage.test.tsx:143. Remove all four.
• L2 — EMAIL_CONFLICT is a dead ErrorCode (pre-existing). ConflictError emits code 'CONFLICT'/409 (AppError.ts:57-62), not EMAIL_CONFLICT; nothing in server/src has ever emitted EMAIL_CONFLICT. It survives only in the union (shared/src/types/errors.ts:19), errors.json, and assertions. Not introduced here, but this PR is the natural place to retire it — and shared/src/types/api.test.ts currently adds fresh assertions pinning it in place. If you'd rather not widen scope, leave it and I'll file a cleanup issue.
• L3 — unused name claim. oidc.ts no longer destructures name, but oidcService.handleCallback still computes it and declares it in its return type. Correct given displayName is no longer written, but the now-unused claim extraction can be dropped.
• L4 — no transaction around update-then-select in findOrLinkOidcUser. Fine for SQLite single-writer at this scale; noting only for completeness.

---

Verified clean

• OidcNoMatchingAccountError follows the AppError convention exactly (super(code, status, message) + this.name), and 403 is the right status: the identity authenticated successfully but is not authorized for any account. Correctly distinct from 401 and from EMAIL_CONFLICT/409.
• ErrorCode union addition is placed with the other OIDC_* codes; redirect param oidc_no_matching_account matches the established lowercase convention.
• Naming (camelCase TS, snake_case DB, PascalCase error class), ESM .js import specifiers, and type imports all conform.
• No new dependencies.
• E2E rewrite correctly seeds TEST_MEMBER as a local account first, and the coverage-limitation comment about the single-identity mock provider is honest and well-reasoned.
• AC3/AC4/AC5 (no row inserted on either link or reject) are structurally guaranteed — there is no longer any insert in the OIDC path.

Happy to re-review promptly once C1, H1, and H2 are addressed. C1 is the one I would not merge without.

[steilerDev]

[e2e-test-engineer]

Reviewed e2e/tests/auth/oidc.spec.ts and e2e/fixtures/testData.ts against issue #1865's ACs. Verified against e2e/playwright.config.ts, e2e/auth.setup.ts, e2e/fixtures/apiHelpers.ts, e2e/containers/oidcContainer.ts, server/src/routes/oidc.ts/oidcService.ts, server/src/routes/users.ts, and the shard matrix in .github/workflows/ci.yml. One finding required tracing Playwright's own dispatcher source (node_modules/playwright/lib/runner/index.js) to confirm, cited below.

Findings

Major — beforeAll user-seed is not retry-safe; a Playwright retry after ANY later-test flake fails the whole file with an unrelated 409

e2e/tests/auth/oidc.spec.ts:35-53. test.describe.configure({ mode: 'serial' }) + retries: 1 on CI (playwright.config.ts) interact badly with a beforeAll that has a real side effect keyed on a fixed email.

Traced Playwright 1.61.1's dispatcher (_onDone, node_modules/playwright/lib/runner/index.js ~line 5450): when any test belonging to a serial-mode suite fails, serialSuitesWithFailures collects the whole outer serial suite, and serialSuite.allTests().forEach(t => retryCandidates.add(t)) re-queues every test in the block for retry as one job — not just the failed one. A fresh worker for that retry job re-executes beforeAll from scratch.

Concretely: attempt 1's beforeAll succeeds (POST /api/users creates member@e2e-test.local). If any later test in the file flakes for an unrelated reason (SSO redirect timing, etc.), Playwright retries the entire serial block, and attempt 2's beforeAll POSTs the same email again. server/src/routes/users.ts:181 throws 409 CONFLICT "Email already in use" for a duplicate email, so createLocalUserViaApi's expect(response.ok(), ...).toBeTruthy() (apiHelpers.ts) throws inside beforeAll — which fails every test in the file with a confusing, unrelated error, burning the one retry that was supposed to rescue the original transient flake. This turns any single-test flake in this 6-test file into a guaranteed red CI run.

Contrast with the codebase's own established pattern for this exact helper: both other callers of createLocalUserViaApi (e2e/tests/profile/change-password.spec.ts:38, e2e/tests/i18n/i18n-categories.spec.ts:81) derive the email from testPrefix/Date.now() to guarantee uniqueness rather than reusing a fixed constant — this file can't do that because the mock IdP is hardcoded to one email, but that's exactly why the seed step needs to tolerate being re-run.

Remediation: make the seed idempotent — e.g. GET /api/users?q=<email> first and skip creation if found, or catch the specific 409 in beforeAll and treat it as success (the user already exists in the desired state either way, since nothing else in the suite mutates it before this point).

Major — AC2's local-password-still-works-after-linking is added to the fixture but never asserted

Issue #1865 AC2: "Given a linked account, When the user logs in with their local email + password, Then login succeeds and resolves to the same account (same id)." TEST_MEMBER.localPassword was added to testData.ts:11-18 specifically for this (per its own comment: "this password keeps working alongside SSO"), and LoginPage.login(email, password) already exists as a POM method, but no test in oidc.spec.ts calls it. Only the OIDC→OIDC-again path (AC2's third bullet, via oidcSubject) is covered by "Linked OIDC user retains local account attributes." The local-password path is untested at the E2E layer.

Remediation: after the first successful SSO link (test 3), add an assertion (or a new test) that opens a fresh unauthenticated context, calls loginPage.login(TEST_MEMBER.email, TEST_MEMBER.localPassword), and confirms /api/auth/me's user.id matches the id captured from the OIDC-authenticated session — proving both login methods resolve to the same account, which is the actual point of AC2.

Minor — the new rejection test's second assertion is vacuous and the test largely duplicates the new RTL unit test

e2e/tests/auth/oidc.spec.ts:181-214. Two issues:

1. The comment block (164-180) correctly explains why a genuinely-rejected second IdP identity isn't obtainable from the current mock container (verified: oidcContainer.ts's single requestMappings entry keys only on grant_type, and oidcService.ts buildAuthorizationUrl hardcodes scope: 'openid email profile' with no test-controllable discriminator like login_hint forwarded — so the "infeasible without a real second identity" claim checks out against the current code). Given that, the test navigates straight to ?error=oidc_no_matching_account and asserts the banner — but this is functionally the same assertion as the new unit test added in this same PR (client/src/pages/LoginPage/LoginPage.test.tsx, "shows OIDC error message from URL query parameter"), which renders <LoginPage> from the same URL and asserts the same text via RTL. The E2E version adds real browser navigation but no real integration signal beyond that.
2. The "creates no account" assertion (207-212) checks that no-such-oidc-user@e2e-test.local — an email that was never submitted to the IdP, the callback route, or findOrCreateOidcUser at any point in this test — doesn't exist. That's true of any string and doesn't exercise the rejection code path at all (no OIDC flow is triggered in this test). It doesn't prove AC4's "no auto-provisioning on rejection" guarantee; it just proves an unrelated email was never created, which was never in question.

Not blocking, since the comment is honest that qa-integration-tester covers the real backend path — but the test name/second assertion currently overstate what this test proves. Recommendation: either drop the vacuous second assertion (keep the test purely as a banner-rendering regression guard and rename accordingly), or note explicitly in a comment that it's non-diagnostic filler rather than AC4 coverage.

Verified clean (no action needed)

• beforeAll auth-state timing: desktop project declares dependencies: ['auth-setup'] in playwright.config.ts, which guarantees test-results/.auth/admin.json (written by auth.setup.ts) exists before any desktop-project test — including this file's beforeAll — runs.
• createLocalUserViaApi signature/import: exported from apiHelpers.ts with exactly {email, displayName, password, role?}; the .js import extension is correct per this repo's ESM convention (CLAUDE.md), not a bug.
• Cross-shard/cross-project interference: each of the 16 CI shards runs its own globalSetup (containers/setup.ts), spinning up fresh, isolated Docker containers (app + OIDC + proxy) and therefore a fresh SQLite DB per shard — member@e2e-test.local seeded in one shard cannot leak into another. Within a shard, oidc.spec.ts has no @responsive tag, so it only runs once (desktop project); tablet/mobile grep: /@responsive/ filters skip it entirely — no duplicate seed attempt across projects in the same run. Grepped all of e2e/ for TEST_MEMBER/member@e2e-test.local: no other spec file references it, and the one other spec asserting on user-row counts (admin/search-users.spec.ts) uses toBeGreaterThan(0), not exact equality, so it's unaffected by the extra seeded row.
• Page object conventions: consistent use of LoginPage/UserManagementPage; page.request for API assertions matches the established pattern in change-password.spec.ts/i18n-categories.spec.ts.
• Viewport coverage: correctly out of scope — no @responsive tag (pre-existing, unchanged by this PR), so it only runs on desktop, appropriate for an auth-redirect flow with no responsive-layout concern.

Verdict

Requesting changes on the retry-safety issue (concrete CI-reliability risk, reproducible any time a later test in this serial file flakes) and the AC2 E2E gap (a real acceptance criterion with no browser-level coverage, despite the fixture being built for exactly this purpose). The rejection-test note is a quality suggestion, not a blocker.

[steilerDev]

[qa-integration-tester]

Reviewed test quality and coverage only (server/client unit+integration tests, shared type tests, and the E2E test insofar as it interacts with my scope). Verified against issue #1865's AC1–AC6 using the real merge-base-aware diff (gh pr diff 1866), not a raw file-vs-beta-HEAD comparison (beta has drifted since this branch forked — more on that below).

Findings

MAJOR — AC2 bullet 2 is untested: local login after linking is never exercised

Nothing in this PR logs in with local email+password after OIDC linking to assert it still resolves to the same account id, as AC2 explicitly requires ("When the user logs in with their local email + password, Then login succeeds and resolves to the same account (same id)").

• e2e/fixtures/testData.ts adds TEST_MEMBER.localPassword, but it is used only to seed the local account via createLocalUserViaApi in oidc.spec.ts's beforeAll — never to drive an actual POST /api/auth/login or a UI login with that password.
• server/src/routes/auth.test.ts is unmodified by this PR and has no pre-existing test that logs in locally against a user row that has oidcSubject set.
• server/src/services/userService.test.ts only tests findOrLinkOidcUser — it never calls verifyPassword/the local-login path on a post-link user.

Net effect: the "either login method resolves to the same account" guarantee is only proven for the OIDC→OIDC direction (AC2 bullet 3), not local→same-account (AC2 bullet 2). Recommend one more test (unit in userService.test.ts verifying verifyPassword still succeeds against linked.passwordHash post-link, and/or an integration test in auth.test.ts / oidc.spec.ts that actually submits the local credentials via the login endpoint after linking).

MAJOR — Silently overwriting an existing oidcSubject link is untested (and the old safeguard was removed with no replacement)

Old findOrCreateOidcUser threw ConflictError when the matched email belonged to a different already-OIDC-linked user (throws ConflictError when email is used by different OIDC user, beta userService.test.ts:810-831). The new findOrLinkOidcUser removes this check entirely:

const emailUser = findByEmail(db, email);
if (!emailUser) { throw new OidcNoMatchingAccountError(); }
db.update(users).set({ oidcSubject: sub, updatedAt: now }).where(eq(users.id, emailUser.id)).run();

If emailUser already has a different oidcSubject linked (e.g. a second, distinct IdP sub presenting the same verified email — plausible if the admin's OIDC provider account is recreated, or if a second provider claims the same email), this silently reassigns the link, away from the first sub onto the second, with no error and no test covering it either way. Issue #1865 explicitly flagged this as an open decision ("The architect/dev-team-lead should decide whether the ConflictError branch is retained for any other conflict cases... or removed") — the PR resolves it by deletion, but nothing locks in that this is intentional. Given the trust-model stakes described in the issue (verified-email-match is the only gate), this deserves an explicit test either confirming the re-link is intended behavior or guarding against it.

MAJOR — Agent-memory claim in .claude/agent-memory/qa-integration-tester/environment-setup.md is factually wrong; should not be merged as written

This is my own team's memory file — verified directly rather than taking it on faith. I ran client/src/pages/LoginPage/LoginPage.test.tsx against the current baseline (Node 24.18.0, matching the note's stated environment) two ways:

• node --experimental-vm-modules node_modules/.bin/jest client/src/pages/LoginPage/LoginPage.test.tsx --maxWorkers=1 → 17/17 pass, 0 failures.
• npx jest client/src/pages/LoginPage/LoginPage.test.tsx --coverage --maxWorkers=1 (the contributor's exact invocation, no --experimental-vm-modules) → 9 failed, 8 passed — reproducing their exact symptom (expect(mockGetAuthMe).toHaveBeenCalled() receiving 0 calls).

Root cause: LoginPage.test.tsx uses jest.unstable_mockModule('../../lib/authApi.js', ...). Without the --experimental-vm-modules Node flag (which this repo's own npm test script supplies via node --experimental-vm-modules node_modules/.bin/jest), unstable_mockModule silently no-ops and the real authApi.getAuthMe() runs unmocked. This is the exact, already-documented ESM-mocking gotcha this team tracks elsewhere. The note's diagnosis ("root cause looks like an async-timing/jsdom quirk with the getAuthMe mock resolving after the effect's cleanup") is incorrect — it's an invocation bug, not a code or timing issue, and it isn't specific to this sandbox. If merged, this note would cause future QA sessions to wave off a real future regression in this file as "pre-existing, not a regression." Recommend dropping this file from the PR (it's an internal QA-owned memory file an external contributor shouldn't be editing anyway) or replacing it with the correct root cause.

MEDIUM — AC3 (user count unchanged on successful link) not explicitly asserted

The rejection path explicitly asserts countUsers() before/after (does not create a new user row when no account matches), but the success/link path (links an existing local account by email match on first OIDC login) does not. It's implicitly proven (the update targets emailUser.id and the returned id matches), but doesn't literally assert row count per the AC's wording. Minor — not blocking.

LOW / notes, not blocking

• oidc.spec.ts's rejection test navigates directly to /login?error=oidc_no_matching_account rather than driving a genuinely-unmatched email through the mock IdP (the mock is hardcoded to one fixed identity). Its own comment is transparent about the constraint and correctly defers the real 403/no-account-created assertion to oidc.test.ts's route-level test (confirmed present: redirects to /login?error=oidc_no_matching_account when no account matches by email, asserts countUsers(app.db) === 0). Primarily e2e-test-engineer's call, flagging for their awareness only.
• jest.unstable_mockModule usage in server/src/routes/oidc.test.ts checks out: repo's npm test supplies --experimental-vm-modules; the 6 mocked exports exactly match oidcService.ts's real exports (no missing-export runtime gap); the if (!buildApp) one-time dynamic-import guard is the standard idiom already used elsewhere in this codebase; jest.clearAllMocks() in the outer beforeEach runs before the inner describe's mockResolvedValue setup, so no interference; process.env.SECURE_COOKIES = 'false' follows the identical set/restore pattern already used pre-PR for OIDC_ISSUER etc., correctly restored via process.env = originalEnv in afterEach; app is built per-test and closed in afterEach. No defects.
• Net userService.test.ts swap (+112/−137): the 137 deleted lines are exclusively the removed auto-provisioning branch's tests plus two findByOidcSubject tests whose exact assertion (the authProvider = 'oidc' filter) was the bug being fixed — both deliberately and correctly replaced by equivalent tests for the new behavior. No coverage lost for still-live behavior, except the re-link scenario in the MAJOR finding above.
• shared/src/types/api.test.ts and LoginPage.test.tsx's new assertions are simple literal/type checks, consistent with this codebase's existing pattern for ErrorCode union coverage — not meaningfully tautological beyond what the rest of that file already does.
• Coverage plausibility for findOrLinkOidcUser, findByOidcSubject, OidcNoMatchingAccountError, and the modified oidc.ts callback branches looks like it clears 95% on the touched lines, modulo the untested re-link branch above.
• Housekeeping: comparing PR-HEAD file contents against current origin/beta showed unrelated-looking removals (InvoicesNotClaimableError, RefundExceedsInvoiceError) that are not actually part of this PR's diff per gh pr diff — beta has moved on since this branch forked. Worth a rebase before merge to avoid surprises; not a test-quality issue.

Verdict

Requesting changes: two real AC/behavior coverage gaps (local-login-after-link, and the untested silent re-link/overwrite path) plus one factually incorrect claim landing in a QA-owned memory file that would actively mislead future test triage if merged as-is.

[steilerDev]

[orchestrator] Consolidated full-team review (/review-pr)

Eight reviewers ran against this PR: product-owner (duplicate/requirements gate), product-architect, security-engineer, dev-team-lead, backend-developer, frontend-developer, ux-designer, qa-integration-tester, e2e-test-engineer.

Outcome: changes requested. 6 of 8 requested changes; ux-designer and frontend-developer approved their (small) slices.

First, the good news: the core design is right and faithfully implemented. OIDC-as-alternate-login-for-existing-accounts (never self-registration) is exactly what #1865 asked for. AC1, AC3, AC4, AC5 and AC6 are structurally guaranteed — no INSERT remains anywhere in the OIDC path, and the link is a pure UPDATE of oidcSubject/updatedAt that provably leaves role, passwordHash, authProvider, displayName and id untouched. Reviewers independently verified there are no unused imports after the refactor, no stale callers of the renamed function, correct unstable_mockModule usage, correct env set/restore, and that createLocalUserViaApi, the errorBanner POM locator, and GET /api/users?q= all really exist. The removal of auto-provisioning is a genuine security improvement.

The blockers below are about the new attack surface that linking introduces, plus some reconciliation work the change implies.

---

Critical

C1 — email_verified is never checked; the PR does not do what its title says

server/src/services/oidcService.ts (~L106-107, untouched by this PR) reads claims.email with no verification gate, and findOrLinkOidcUser then uses that email as the sole authorization to link to — and inherit the role of — an existing account, including the admin.

This is a severity escalation created by this PR. Pre-PR, an email collision blocked login (ConflictError). Post-PR it grants it. On any IdP that doesn't strictly enforce email verification (self-hosted Keycloak/Authentik, or a misconfigured tenant), an attacker who can assert admin@example.com in an ID token gets the admin account with its role and password preserved.

The PR title says "by verified email" and #1865's trust model explicitly rests on the email being verified — the code never establishes that. Flagged independently by both product-architect and security-engineer.

Fix: gate on email_verified === true in handleCallback, failing closed when the claim is absent. An empty email already routes to the existing missing_email redirect, so only the verification gate is missing.

---

High

H1 — idx_users_oidc_lookup no longer covers the lookup key, and no migration is included

findByOidcSubject dropped its authProvider = 'oidc' predicate and now queries oidcSubject alone, but the index is still UNIQUE (auth_provider, oidc_subject) WHERE oidc_subject IS NOT NULL (server/src/db/schema.ts:45-47, 0001_create_users_and_sessions.sql:17-19). Two consequences:

• Uniqueness of the new correlation key is unenforced. The DB permits (local,'S') and (oidc,'S') to coexist; .get() then resolves an auth lookup from an unordered multi-row result. Product-architect could not construct a path that produces such a pair through findOrLinkOidcUser today, so this is latent rather than exploitable — but an authentication key should be guarded by the database, not by the absence of a code path.
• SQLite's leftmost-prefix rule means the index cannot serve WHERE oidc_subject = ? at all — every SSO login is now a full table scan.

Fix: add a migration replacing it with a unique index on oidc_subject alone (partial, WHERE oidc_subject IS NOT NULL) and update schema.ts to match. Raised by product-architect, security-engineer and backend-developer independently.

No backfill is required — product-architect checked this explicitly. Legacy auth_provider='oidc' rows are still found unchanged by the predicate-free lookup. Do not rewrite them to 'local'.

H2 — E2E beforeAll seed is not retry-safe and will break the whole spec on any flake

e2e/tests/auth/oidc.spec.ts:35-53. In a test.describe.configure({ mode: 'serial' }) block, Playwright re-queues the entire block including beforeAll on retry, and CI sets retries: 1. So if any later test in the file flakes for an unrelated reason, the retry's beforeAll re-POSTs member@e2e-test.local, server/src/routes/users.ts:181 returns 409 CONFLICT, and createLocalUserViaApi's expect(response.ok()) fails the hook — failing every test in the file with a misleading error and silently defeating the CI retry safety net.

The repo's two other createLocalUserViaApi callers (profile/change-password.spec.ts:38, i18n/i18n-categories.spec.ts:81) uniquify emails with Date.now() precisely to avoid this. This spec can't (the mock IdP is hardcoded to one identity), which is exactly why the seed needs explicit idempotency.

Fix: check-then-create, or catch and ignore the 409.

H3 — Wiki documents the exact invariant this PR inverts

wiki/Schema.md:50 states password_hash/oidc_subject are "mutually exclusive by auth_provider" — holding both is the entire point of this PR. Also stale: Schema.md:32,42,48,98-100; API-Contract.md:337 (documents the removed auth_provider='oidc' lookup), :351,357 (dead email_conflict), :5869-5870 (missing the OIDC_NO_MATCHING_ACCOUNT / 403 row); and ADR-010:12,18, which still assert automatic provisioning.

Fix: amend ADR-010 (or add a superseding ADR) plus the Schema and API-Contract pages.

H4 — The agent-memory note added by this PR is factually wrong and would misdirect future triage

.claude/agent-memory/qa-integration-tester/environment-setup.md gains a note claiming 9/18 LoginPage.test.tsx tests fail locally due to "an async-timing/jsdom quirk," and that CI coverage should be treated as authoritative instead.

qa-integration-tester (whose memory file this is) verified the claim rather than taking it on faith, and it does not hold:

• node --experimental-vm-modules node_modules/.bin/jest client/src/pages/LoginPage/LoginPage.test.tsx --maxWorkers=1 → 17/17 pass, 0 failures
• npx jest client/src/pages/LoginPage/LoginPage.test.tsx --coverage --maxWorkers=1 (the note's exact invocation) → 9 failed, 8 passed, reproducing the reported symptom precisely

Root cause is the known ESM gotcha: LoginPage.test.tsx uses jest.unstable_mockModule, which silently no-ops without the --experimental-vm-modules flag that this repo's own npm test script supplies. The real authApi.getAuthMe() then runs unmocked. It is an invocation error, not a code defect, not a timing quirk, and not sandbox-specific.

Merged as written, this note would teach future QA sessions to wave off a genuine regression in this file as "pre-existing."

Fix: drop this file from the PR. (It is an internal QA-owned memory file in any case.) It is also the only file in this PR that conflicts with beta, so dropping it resolves the merge conflict too.

---

Medium

M1 — Linking mutation happens before the deactivatedAt guard

server/src/routes/oidc.ts performs the linking UPDATE inside findOrLinkOidcUser and only then checks user.deactivatedAt. An unauthenticated caller who knows a deactivated account's email and controls a matching IdP identity permanently binds their sub to it; if an admin later reactivates the account, the attacker's identity resolves straight in. The PR's own test asserts this binding occurs (oidc.test.ts, deactivated case).

Fix: check deactivatedAt before mutating, or re-check inside findOrLinkOidcUser prior to the update.

M2 — Silent oidcSubject re-binding: keep the behavior, but log it and test it

#1865 explicitly asked the architect to decide whether a conflict branch should be retained here. The ruling is to accept the silent overwrite — a verified email match that authorizes creating a link equally authorizes replacing one, and it makes IdP migration self-healing, which matters because manual link/unlink UI is out of scope, so a hard conflict error would lock an admin out of SSO with no in-app recovery.

Two conditions attach:

1. This ruling is contingent on C1. Without email_verified, re-binding is a takeover primitive: security-engineer's High-severity chain (hijack an already-linked admin account, lock out the legitimate admin, retain persistent access) runs entirely through the unverified-email hole. Fix C1 and this reduces to the accepted trust model.
2. The overwrite is currently completely unlogged and untested. Emit a warn recording previousSub → sub, and add a test that locks in the re-link as deliberate (qa-integration-tester notes the old ConflictError safeguard was deleted with nothing asserting the replacement is intentional).

M3 — AC2 bullet 2 is untested: local password login after linking

AC2 requires that after linking, local email+password login still resolves to the same account id. Nothing exercises it. e2e/fixtures/testData.ts adds TEST_MEMBER.localPassword seemingly for exactly this, but it is used only to seed the account — LoginPage.login() is never called with it, server/src/routes/auth.test.ts is unmodified, and userService.test.ts never runs verifyPassword against a post-link user. The guarantee is currently only inferred from passwordHash preservation. Flagged by product-owner, qa-integration-tester and e2e-test-engineer independently.

M4 — authProvider semantics drift is user-visible

authProvider now means "how the account was created," but ProfilePage.tsx:218 and UserManagementPage.tsx:383-385 render it as "how it authenticates." An SSO-only user will display "Local" — the PR's own E2E now asserts authProvider === 'local' for a linked account, encoding the drift.

Fix: expose a derived oidcLinked boolean (not oidc_subject itself — API-Contract.md:508 says it is never exposed), or make an explicit relabel decision.

M5 — Email matching is case-sensitive

findByEmail uses eq(users.email, email) with no COLLATE NOCASE or normalization, and this is now the authorization gate. It fails closed, so it is not a takeover vector, but an IdP returning Admin@example.com against a stored admin@example.com yields "ask an administrator to create an account" — to the administrator, with no self-serve recovery.

M6 — Newly-orphaned email_conflict dead code

Removing the ConflictError branch from oidc.ts makes /login?error=email_conflict unreachable — oidc.ts was its only producer. Left behind: the 'email_conflict' entry in LoginPage.tsx's knownCodes, and the errors.email_conflict key in both en/auth.json and de/auth.json. Per file ownership the de removal belongs to the translator, not the frontend.

(Distinct and out of scope: the uppercase EMAIL_CONFLICT ErrorCode was already dead pre-PR — ConflictError emits 'CONFLICT'/409 and nothing ever constructed EMAIL_CONFLICT. This PR's shared/src/types/api.test.ts newly adds assertions pinning it in place, which is worth reconsidering, but the pre-existing deadness is a separate cleanup.)

M7 — No dedicated unit test for the new error class

server/src/errors/AppError.test.ts follows a strict one-describe-per-error-class pattern (see AccountLockedError), asserting .name, code mapping, status and default message. OidcNoMatchingAccountError gets no such block — it is only exercised indirectly via HTTP-level tests. Line coverage is likely satisfied; the class's own contract is not.

---

Low

• plan/REQUIREMENTS.md:19,233,301 still mandate automatic user provisioning, and closed story #35 ("Automatic User Provisioning on First OIDC Login") is directly contradicted by this PR. Judged an intentional supersede per #1865 rather than a blocker — but it needs either an update here or a follow-up issue, plus a "superseded by #1865" annotation on #35. Separately, #1865 is not on the Projects board.
• The name claim is still computed and returned by handleCallback but no longer consumed anywhere.
• The new E2E rejection test's second assertion ("no account exists for no-such-oidc-user@e2e-test.local") is vacuous — that email is never presented to the IdP, so it cannot demonstrate that rejection creates no account. The route-level test in oidc.test.ts covers this properly. e2e-test-engineer verified the author's "a second mock identity is infeasible" claim is true, so this is not a request to fabricate integration coverage — only to stop the assertion implying something it does not prove.
• AC3's "user count unchanged" is asserted on the rejection path but not on the success/link path. Implicitly proven; not blocking.
• EN copy tone: "Ask an administrator to create an account for you first" diverges from the sibling pattern "Please contact an administrator." Notably the DE translation does match the sibling pattern — only EN drifts. (The DE string is the longest this banner has carried, ~150 chars; ux-designer confirmed .errorBanner has no fixed height, nowrap, or truncation, so it wraps cleanly at every viewport, and the pre-existing role="alert" announces it correctly.)
• findOrLinkOidcUser does read → check → update as three unwrapped statements. Acceptable at 1-5 users; noting for the record.

---

Process items (not code defects — for the maintainer)

1. CI has never run on this PR. The Quality Gates run sits in action_required and there are zero check-runs on head f460b7c. This is a cross-repo PR from a fork, so Actions is waiting on maintainer approval to execute workflows. Nothing here has been validated by CI.
2. scripts/check-trailers.sh fails, so CI's trailer-check job will block merge:
   ERROR: files matching 'client/src/i18n/de/**, glossary.json' changed but no 'Co-Authored-By: Claude translator (...)' trailer found.
   The PR carries only a bare Co-Authored-By: Claude Opus 4.6 with no agent name. The diff touches paths requiring backend-developer, frontend-developer, translator, e2e-test-engineer and qa-integration-tester trailers. As an external contribution this is not the author's defect to fix, but it needs a maintainer decision at merge time.
3. Merge conflict is trivial and self-resolving. git merge-tree confirms exactly one conflicting file: .claude/agent-memory/qa-integration-tester/environment-setup.md. Every production file auto-merges cleanly. Dropping that file per H4 removes the conflict entirely. The branch is 12 days old and beta has since moved on (4 report/invoice commits touched some of the same files) — a rebase is advisable regardless.

---

Thanks for this contribution — the direction is correct and the removal of OIDC self-provisioning genuinely closes a real hole. The blocking work is C1 (the verification gate the design already assumes), H1 (the index/migration), H2 (E2E retry-safety) and H4 (drop the memory note); the rest is reconciliation and coverage.