From 97301af75f562fec7812f6ccdac03801bd9d6e8a Mon Sep 17 00:00:00 2001 From: Greg Date: Tue, 25 Aug 2026 09:56:04 -0400 Subject: [PATCH 1/7] [DT-4012] Map the fixed consent /api/user/me contract in the BFF and client Consent now answers /api/user/me with distinct statuses (DT-3997 #3021, DT-4011 #3032): 401 rejected token, 404 authenticated but unregistered, 409 Sam sub-provider conflict with an actionable message. - server/src/auth/me.ts: 401 destroys the session (no more profileSeen disambiguation), 404 keeps the "authenticated, no user" answer, and 409 forwards the upstream message as { error: 'provider_conflict', message } after destroying the session. - server/src/types/session.ts: drop the now-dead profileSeen flag (and its once-per-session store write). - src/libs/auth/session.ts: the probe treats 409 as an authoritative signed-out answer and shows the conflict message instead of failing sign-in generically. - src/libs/auth/postSignIn.ts: a 409 from getMe signs the user out with the message instead of attempting a registration that cannot succeed; the legacy "azureb2c authentication error" substring check stays for older consent builds. Co-Authored-By: Claude Fable 5 --- server/src/auth/me.ts | 96 ++++++++++++++++++------------- server/src/types/session.ts | 5 -- server/test/me.test.ts | 84 ++++++++++++++------------- src/libs/auth/postSignIn.ts | 8 ++- src/libs/auth/session.ts | 20 +++++++ test/libs/auth/postSignIn.spec.ts | 24 +++++++- test/libs/auth/session.spec.ts | 52 +++++++++++++++++ 7 files changed, 199 insertions(+), 90 deletions(-) diff --git a/server/src/auth/me.ts b/server/src/auth/me.ts index d45ca1559..a875521db 100644 --- a/server/src/auth/me.ts +++ b/server/src/auth/me.ts @@ -5,41 +5,35 @@ import { REFRESH_WINDOW_SECONDS, RefreshFailedError, refreshAccessToken } from ' const UPSTREAM_TIMEOUT_MS = 5000 /** - * Answers an upstream 401/404. The DUOS API conflates "bad token" with "no - * DUOS profile for this email" (both 401 — DuosUserAuthenticator turns the - * lookup's NotFoundException into an empty principal), so the session's - * `profileSeen` flag is the disambiguator this endpoint controls. + * Shown when the upstream 409 arrives without a usable message — the client + * needs something actionable even if the body was empty or malformed. */ -async function answerNoProfile(request: FastifyRequest, reply: FastifyReply): Promise { - if (request.session.profileSeen) { - // This session has served a profile before, so "no profile" cannot be - // the explanation. The upstream is rejecting the token itself — revoked - // mid-lifetime (before refresh-before-forward would touch it) or the - // account was disabled. The terminal 401 is the honest answer; leaving - // the session alive would send the client into re-registering an - // existing user. - try { - await request.session.destroy() - } - catch (err: unknown) { - request.log.error({ err }, '[auth] upstream rejected a profile-seen session but it could not be destroyed — returning 401 anyway') - } - reply.clearCookie('sessionId').status(401).send({ authenticated: false }) - return +const PROVIDER_CONFLICT_FALLBACK_MESSAGE + = 'You may have previously signed in with a different authentication provider (Google or Microsoft). Please sign in with that provider.' + +/** + * Ends a session the upstream has authoritatively rejected. + */ +async function destroySession(request: FastifyRequest, reply: FastifyReply): Promise { + try { + await request.session.destroy() } - // Never seen a profile: authenticated but not yet registered. Treating - // this 401 as a dead session destroyed every brand-new user's session on - // their first probe and made registration unreachable. In the rare case - // of a token revoked before first contact, the client's registration - // attempt fails too, which signs the session out. (404 kept deliberately: - // DT-3997 restores the upstream's 404 for unregistered users.) - reply.send({ authenticated: true, idp: request.session.idp }) + catch (err: unknown) { + request.log.error({ err }, '[auth] upstream rejected the session but it could not be destroyed — answering as signed out anyway') + } + reply.clearCookie('sessionId') } /** * Confirms the user is authenticated against the upstream Consent API. * Forwards the upstream user profile and the active sub-provider — never the * tokens themselves, which stay server-side in the session. + * + * The upstream /api/user/me contract: + * - 200 registered profile + * - 401 rejected token + * - 404 authenticated but unregistered + * - 409 Sam sub-provider conflict */ export async function getMe(request: FastifyRequest, reply: FastifyReply): Promise { // The answer is per-session and now gates the whole SPA: no intermediary @@ -98,13 +92,46 @@ export async function getMe(request: FastifyRequest, reply: FastifyReply): Promi return } - if (res.status === 401 || res.status === 404) { - await answerNoProfile(request, reply) + if (res.status === 401) { + // The upstream rejected the token itself — revoked mid-lifetime (before refresh-before-forward would + // touch it) or the account was disabled. The terminal 401 is final so the session must be destroyed. + // The client still needs to know it is signed out, so the reply goes out after the session is destroyed. + await destroySession(request, reply) + reply.status(401).send({ authenticated: false }) + return + } + + if (res.status === 404) { + // Authenticated but not yet registered — the session is valid, and the + // client's post-sign-in bootstrap needs authenticated:true (with user + // absent) to reach its registration flow. + reply.send({ authenticated: true, idp: request.session.idp }) + return + } + + if (res.status === 409) { + // Sam sub-provider conflict: the account exists under the other B2C sub-provider and Sam rejects it. + // Registration cannot succeed and the session cannot become usable, so end it — but forward the + // upstream's actionable message (sign in with the other provider, plus the support link) instead + // of a generic failure. + let message = PROVIDER_CONFLICT_FALLBACK_MESSAGE + try { + const body: unknown = await res.json() + const upstreamMessage = (body as { message?: unknown } | null)?.message + if (typeof upstreamMessage === 'string' && upstreamMessage.length > 0) { + message = upstreamMessage + } + } + catch { + // Unparseable body — the fallback message stands. + } + await destroySession(request, reply) + reply.status(409).send({ authenticated: false, error: 'provider_conflict', message }) return } if (!res.ok) { - // A non-401 failure (5xx, upstream outage) says nothing about whether the + // A non-4xx failure (5xx, upstream outage) says nothing about whether the // token itself is still valid — don't destroy the session or parse an // error body as if it were a user profile. reply.status(502).send({ authenticated: false, error: 'upstream_unavailable' }) @@ -120,15 +147,6 @@ export async function getMe(request: FastifyRequest, reply: FastifyReply): Promi return } - if (!request.session.profileSeen) { - request.session.profileSeen = true - // Saved explicitly before the reply, once per session (the flag never - // flips back): with rolling off, the onSend hook only skips its async - // save when the session is unmodified — see index.ts on the - // ERR_HTTP_HEADERS_SENT hazard a post-reply async save creates. - await request.session.save() - } - reply.send({ authenticated: true, user, diff --git a/server/src/types/session.ts b/server/src/types/session.ts index 42f7163d5..46778fd89 100644 --- a/server/src/types/session.ts +++ b/server/src/types/session.ts @@ -23,10 +23,5 @@ declare module 'fastify' { // regardless — this field exists for the audit trail and observability, // not client selection. idp?: 'google' | 'microsoft' - // Set once /auth/me has served a DUOS profile on this session. The - // upstream conflates "no profile" with "bad token" (both 401), and this - // is the disambiguator: a 401/404 on a profile-seen session cannot mean - // "unregistered", so it is treated as a terminal token verdict. - profileSeen?: boolean } } diff --git a/server/test/me.test.ts b/server/test/me.test.ts index 54d77a739..e0c768c4c 100644 --- a/server/test/me.test.ts +++ b/server/test/me.test.ts @@ -16,21 +16,18 @@ const ENV = { DUOS_API_URL: 'https://consent.dsde-dev.broadinstitute.org' } // tests exercise the forward path untouched. const FRESH_EXPIRY = () => Math.floor(Date.now() / 1000) + 3600 -function makeRequest(overrides: { accessToken?: string, idp?: 'google' | 'microsoft', tokenExpiry?: number, profileSeen?: boolean } = {}) { +function makeRequest(overrides: { accessToken?: string, idp?: 'google' | 'microsoft', tokenExpiry?: number } = {}) { const destroy = vi.fn().mockResolvedValue(undefined) - const save = vi.fn().mockResolvedValue(undefined) const request = { session: { accessToken: overrides.accessToken, idp: overrides.idp, tokenExpiry: overrides.tokenExpiry ?? FRESH_EXPIRY(), - profileSeen: overrides.profileSeen, destroy, - save, }, log: { error: vi.fn(), info: vi.fn() }, } - return { request: request as unknown as FastifyRequest, destroy, save } + return { request: request as unknown as FastifyRequest, destroy } } function makeReply() { @@ -106,33 +103,12 @@ describe('getMe', () => { }) }) - it('marks the session profile-seen on the first served profile, with an explicit pre-reply save', async () => { - vi.mocked(fetch).mockResolvedValue(makeFetchResponse(200, { email: 'user@example.com' }) as never) - const { request, save } = makeRequest({ accessToken: 'test-access-token' }) - - await getMe(request, makeReply()) - - expect(request.session.profileSeen).toBe(true) - expect(save).toHaveBeenCalledOnce() - }) - - it('does not re-save a session already marked profile-seen', async () => { - // Focus revalidation makes this path hot — the flag costs one DB write - // per session, not one per probe. - vi.mocked(fetch).mockResolvedValue(makeFetchResponse(200, { email: 'user@example.com' }) as never) - const { request, save } = makeRequest({ accessToken: 'test-access-token', profileSeen: true }) - - await getMe(request, makeReply()) - - expect(save).not.toHaveBeenCalled() - }) - - it.each([401, 404])('destroys a profile-seen session on an upstream %i — "no profile" cannot explain it', async (status) => { + it('destroys the session on an upstream 401', async () => { // A registered user's token revoked mid-lifetime forwards (no refresh due) // and 401s. Answering "authenticated, no user" here sent the client into // re-registering an existing account; the terminal 401 is the honest end. - vi.mocked(fetch).mockResolvedValue(makeFetchResponse(status, {}) as never) - const { request, destroy } = makeRequest({ accessToken: 'fresh-access-token', idp: 'google', profileSeen: true }) + vi.mocked(fetch).mockResolvedValue(makeFetchResponse(401, {}) as never) + const { request, destroy } = makeRequest({ accessToken: 'fresh-access-token', idp: 'google' }) const reply = makeReply() await getMe(request, reply) @@ -143,6 +119,19 @@ describe('getMe', () => { expect(reply.send).toHaveBeenCalledWith({ authenticated: false }) }) + it('still answers 401 when the rejected session cannot be destroyed', async () => { + vi.mocked(fetch).mockResolvedValue(makeFetchResponse(401, {}) as never) + const { request, destroy } = makeRequest({ accessToken: 'fresh-access-token' }) + destroy.mockRejectedValue(new Error('store unavailable')) + const reply = makeReply() + + await getMe(request, reply) + + expect(reply.clearCookie).toHaveBeenCalledWith('sessionId') + expect(reply.status).toHaveBeenCalledWith(401) + expect(reply.send).toHaveBeenCalledWith({ authenticated: false }) + }) + it('marks every answer uncacheable — the profile must never be replayed across sessions', async () => { vi.mocked(fetch).mockResolvedValue(makeFetchResponse(200, { email: 'user@example.com' }) as never) const { request } = makeRequest({ accessToken: 'test-access-token', idp: 'google' }) @@ -163,20 +152,35 @@ describe('getMe', () => { expect(reply.header).toHaveBeenCalledWith('cache-control', 'no-store') }) - it('reports authenticated with no user on an upstream 401 — DUOS conflates "no profile" with "bad token"', async () => { - // DuosUserAuthenticator turns an unregistered email's NotFoundException - // into an empty principal → Dropwizard 401. The token itself was just - // refreshed, so this must NOT destroy the session: it is a brand-new - // user who needs the registration bootstrap, not a dead session. - vi.mocked(fetch).mockResolvedValue(makeFetchResponse(401, {}) as never) - const { request, destroy } = makeRequest({ accessToken: 'fresh-access-token', idp: 'google' }) + it('forwards the upstream 409 message and destroys the session — a Sam sub-provider conflict cannot be registered through', async () => { + const message = 'Email: user@example.com. You may have previously signed in with a different authentication provider (Google or Microsoft). Please sign in with that provider.' + vi.mocked(fetch).mockResolvedValue(makeFetchResponse(409, { message, code: 409 }) as never) + const { request, destroy } = makeRequest({ accessToken: 'fresh-access-token', idp: 'microsoft' }) const reply = makeReply() await getMe(request, reply) - expect(destroy).not.toHaveBeenCalled() - expect(reply.clearCookie).not.toHaveBeenCalled() - expect(reply.send).toHaveBeenCalledWith({ authenticated: true, idp: 'google' }) + expect(destroy).toHaveBeenCalledOnce() + expect(reply.clearCookie).toHaveBeenCalledWith('sessionId') + expect(reply.status).toHaveBeenCalledWith(409) + expect(reply.send).toHaveBeenCalledWith({ authenticated: false, error: 'provider_conflict', message }) + }) + + it('answers the 409 with a fallback message when the upstream body is unusable', async () => { + const badBody = { status: 409, ok: false, json: vi.fn().mockRejectedValue(new Error('invalid JSON')) } + vi.mocked(fetch).mockResolvedValue(badBody as never) + const { request, destroy } = makeRequest({ accessToken: 'fresh-access-token' }) + const reply = makeReply() + + await getMe(request, reply) + + expect(destroy).toHaveBeenCalledOnce() + expect(reply.status).toHaveBeenCalledWith(409) + expect(reply.send).toHaveBeenCalledWith({ + authenticated: false, + error: 'provider_conflict', + message: expect.stringContaining('different authentication provider'), + }) }) it('does not refresh when the access token is comfortably fresh', async () => { @@ -248,7 +252,7 @@ describe('getMe', () => { expect(reply.send).toHaveBeenCalledWith({ authenticated: true, idp: 'microsoft' }) }) - it('returns 502 without destroying the session when the upstream API errors with a non-401 status', async () => { + it('returns 502 without destroying the session when the upstream API errors with an unmapped status', async () => { vi.mocked(fetch).mockResolvedValue(makeFetchResponse(503, {}) as never) const { request, destroy } = makeRequest({ accessToken: 'test-access-token' }) const reply = makeReply() diff --git a/src/libs/auth/postSignIn.ts b/src/libs/auth/postSignIn.ts index 2af22d3a3..01fb27718 100644 --- a/src/libs/auth/postSignIn.ts +++ b/src/libs/auth/postSignIn.ts @@ -232,10 +232,12 @@ export const completeSignIn = async ({ navigate, queryClient, redirectPath, isCa } catch (error) { if (cancelled()) return 'cancelled' - // Explicitly handle AzureB2C errors from Sam const errorMessage = extractError(error) - if (errorMessage.toLowerCase().includes('azureb2c authentication error')) { - Notifications.showError({ text: errorMessage }) + // A 409 from getMe is the Sam sub-provider conflict the account lives under the other provider, + // so registration cannot succeed — surface the actionable message instead of attempting it. + if (errorStatus(error) === 409 || errorMessage.toLowerCase().includes('azureb2c authentication error')) { + // Long timeout: the message carries instructions and a support link. + Notifications.showError({ text: errorMessage, timeout: 30000 }) await Auth.signOut() return 'signed-out' } diff --git a/src/libs/auth/session.ts b/src/libs/auth/session.ts index a8241db16..650319108 100644 --- a/src/libs/auth/session.ts +++ b/src/libs/auth/session.ts @@ -1,5 +1,6 @@ import { Config } from 'src/libs/config' import { Storage } from 'src/libs/storage' +import { ToastNotifications } from 'src/libs/ToastNotifications' import { DuosUser } from 'src/types/model' /** @@ -65,6 +66,25 @@ const probeBffSession = async (): Promise => { lastAuthoritativeAnswer = { authenticated: false } return lastAuthoritativeAnswer } + if (res.status === 409) { + // Sam sub-provider conflict (the account lives under the other provider). The BFF has already destroyed + // the session, so this fires once per sign-in attempt. Show the upstream's actionable message (sign in + // with the other provider, plus the support link) instead of failing sign-in generically. + let message = 'You may have previously signed in with a different authentication provider (Google or Microsoft). Please sign in with that provider.' + try { + const body = await res.json() as { message?: string } + if (typeof body.message === 'string' && body.message.length > 0) { + message = body.message + } + } + catch { + // Unparseable body — the fallback message stands. + } + // Long timeout: the message carries instructions and a support link. + ToastNotifications.showError({ text: message, timeout: 30000 }) + lastAuthoritativeAnswer = { authenticated: false } + return lastAuthoritativeAnswer + } if (!res.ok) { // Transient upstream failure (e.g. 502): hold the last real answer for // this ask, but drop the cache so the next ask retries instead of diff --git a/test/libs/auth/postSignIn.spec.ts b/test/libs/auth/postSignIn.spec.ts index 16b78b34d..c798a7307 100644 --- a/test/libs/auth/postSignIn.spec.ts +++ b/test/libs/auth/postSignIn.spec.ts @@ -383,13 +383,31 @@ describe('completeSignIn', () => { }) }) - describe('AzureB2C errors from Sam', () => { - it('shows the error and signs the user out', async () => { + describe('Sam sub-provider conflicts', () => { + it('shows the 409 conflict message and signs the user out instead of attempting registration', async () => { + // Consent DT-4011 answers the conflict with a 409 and an actionable + // message — registration cannot succeed, so it must not be attempted. + const message = 'Email: test@user.com. You may have previously signed in with a different authentication provider (Google or Microsoft). Please sign in with that provider.' + vi.mocked(User.getMe).mockRejectedValue(adapterHttpError(409, message)) + + await expect(run('/')).resolves.toBe('signed-out') + + expect(vi.mocked(Notifications.showError)).toHaveBeenCalledWith( + expect.objectContaining({ text: expect.stringContaining('different authentication provider') }), + ) + expect(vi.mocked(Auth.signOut)).toHaveBeenCalled() + expect(vi.mocked(User.registerUser)).not.toHaveBeenCalled() + }) + + it('still recognizes the legacy 500 "AzureB2C authentication error" message', async () => { + // Older consent builds answer the conflict with a 500 and this message. vi.mocked(User.getMe).mockRejectedValue(new Error('AzureB2C authentication error: bad tenant')) await expect(run('/')).resolves.toBe('signed-out') - expect(vi.mocked(Notifications.showError)).toHaveBeenCalledWith({ text: 'AzureB2C authentication error: bad tenant' }) + expect(vi.mocked(Notifications.showError)).toHaveBeenCalledWith( + expect.objectContaining({ text: 'AzureB2C authentication error: bad tenant' }), + ) expect(vi.mocked(Auth.signOut)).toHaveBeenCalled() expect(vi.mocked(User.registerUser)).not.toHaveBeenCalled() }) diff --git a/test/libs/auth/session.spec.ts b/test/libs/auth/session.spec.ts index 170fe6ecf..65bb37440 100644 --- a/test/libs/auth/session.spec.ts +++ b/test/libs/auth/session.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { getSessionInfo, resetSessionCache, resetSessionProbeState, revalidateSessionInfo, userIsLogged } from 'src/libs/auth/session' import { Config } from 'src/libs/config' import { Storage } from 'src/libs/storage' +import { ToastNotifications } from 'src/libs/ToastNotifications' vi.mock('src/libs/config', async importOriginal => ({ ...(await importOriginal()), @@ -51,6 +52,57 @@ describe('session probe', () => { expect(fetchMock).toHaveBeenCalledTimes(1) }) + it('returns unauthenticated on 409 and shows the provider-conflict message — a real answer, not a transient failure', async () => { + // The BFF forwards consent's Sam sub-provider conflict (DT-4012) as a 409 + // with an actionable message, and has already destroyed the session. + const message = 'Email: user@example.com. You may have previously signed in with a different authentication provider (Google or Microsoft). Please sign in with that provider.' + const showError = vi.spyOn(ToastNotifications, 'showError').mockImplementation(() => undefined) + fetchMock.mockResolvedValue( + new Response(JSON.stringify({ authenticated: false, error: 'provider_conflict', message }), { + status: 409, + headers: { 'content-type': 'application/json' }, + }), + ) + + await expect(getSessionInfo()).resolves.toEqual({ authenticated: false }) + expect(showError).toHaveBeenCalledWith(expect.objectContaining({ text: message })) + // Cached like any authoritative answer — one failed sign-in, one toast. + await expect(getSessionInfo()).resolves.toEqual({ authenticated: false }) + expect(fetchMock).toHaveBeenCalledTimes(1) + expect(showError).toHaveBeenCalledTimes(1) + }) + + it('falls back to a generic provider-conflict message when the 409 body is unusable', async () => { + const showError = vi.spyOn(ToastNotifications, 'showError').mockImplementation(() => undefined) + fetchMock.mockResolvedValue(new Response('not json', { status: 409 })) + + await expect(getSessionInfo()).resolves.toEqual({ authenticated: false }) + expect(showError).toHaveBeenCalledWith(expect.objectContaining({ + text: expect.stringContaining('different authentication provider'), + })) + }) + + it('lets an authoritative 409 overwrite the held signed-in answer', async () => { + vi.spyOn(ToastNotifications, 'showError').mockImplementation(() => undefined) + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ authenticated: true, idp: 'google' }), { status: 200 }), + ) + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ authenticated: false, error: 'provider_conflict', message: 'conflict' }), { status: 409 }), + ) + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ authenticated: false, error: 'upstream_unavailable' }), { status: 502 }), + ) + + await expect(getSessionInfo()).resolves.toMatchObject({ authenticated: true }) + resetSessionCache() + await expect(getSessionInfo()).resolves.toEqual({ authenticated: false }) + resetSessionCache() + + // The later transient failure reports the 409 verdict, not the stale sign-in. + await expect(getSessionInfo()).resolves.toEqual({ authenticated: false }) + }) + it('returns unauthenticated on upstream outage (502) but retries on the next ask', async () => { fetchMock.mockResolvedValueOnce( new Response(JSON.stringify({ authenticated: false, error: 'upstream_unavailable' }), { status: 502 }), From 6274d03e33abf20014935d9fa2878a9c988cb3c3 Mon Sep 17 00:00:00 2001 From: Greg Date: Tue, 25 Aug 2026 11:29:58 -0400 Subject: [PATCH 2/7] [DT-4012] Land B2C authorization-response errors in the SPA, not on raw 500 JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B2C can answer the authorization request with an error instead of a code — the known case is a Microsoft identity outside the accepted client ids (e.g. a personal Live account), plus the user's own cancel (access_denied). authorizationCodeGrant then throws AuthorizationResponseError, and the thrown 500 answered the browser's top-level /auth/callback navigation with raw JSON, stranding the user. - server/src/auth/callback.ts: catch AuthorizationResponseError; a cancel redirects home silently, anything else redirects to /?signInError=provider. Other failures (state mismatch, bad token) still throw. - src/App.tsx: show a fixed error toast for the signInError marker and strip it from the URL. The marker is a key, never reflected text. Co-Authored-By: Claude Fable 5 --- server/src/auth/callback.ts | 23 +++++++++++++++++++---- server/test/authCrypto.test.ts | 34 ++++++++++++++++++++++++++++++++++ src/App.tsx | 18 ++++++++++++++++++ test/components/App.spec.tsx | 27 +++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/server/src/auth/callback.ts b/server/src/auth/callback.ts index d9c6bf276..151b7ee2c 100644 --- a/server/src/auth/callback.ts +++ b/server/src/auth/callback.ts @@ -17,10 +17,25 @@ export async function handleCallback(request: FastifyRequest, reply: FastifyRepl // inspected; DUOS_OAUTH_REDIRECT_URI supplies the base so it parses as an // absolute URL. const currentUrl = new URL(request.url, requireEnv('DUOS_OAUTH_REDIRECT_URI')) - const tokens = await oidc.authorizationCodeGrant(config, currentUrl, { - pkceCodeVerifier: request.session.pkceVerifier, - expectedState: request.session.pkceState, - }) + let tokens: Awaited> + try { + tokens = await oidc.authorizationCodeGrant(config, currentUrl, { + pkceCodeVerifier: request.session.pkceVerifier, + expectedState: request.session.pkceState, + }) + } + catch (err: unknown) { + if (err instanceof oidc.AuthorizationResponseError) { + // B2C answered the authorization request with an error instead of a + // code — the user canceled on the B2C page (access_denied), or chose + // an identity the tenant's policy rejects. Land back in the + // SPA instead; a cancel is the user's own action and stays silent. + request.log.warn({ error: err.error, description: err.error_description }, '[auth] B2C authorization response is an error') + reply.redirect(err.error === 'access_denied' ? '/' : '/?signInError=provider') + return + } + throw err + } const claims = tokens.claims() // undefined when no id_token is present diff --git a/server/test/authCrypto.test.ts b/server/test/authCrypto.test.ts index 9b11e21c1..f9cbfcbf4 100644 --- a/server/test/authCrypto.test.ts +++ b/server/test/authCrypto.test.ts @@ -310,6 +310,40 @@ describe('B2C OAuth callback (real openid-client validation against a fake B2C)' expect(res.statusCode).toBe(500) }) + it('redirects to /?signInError=provider when B2C answers with an error instead of a code', async () => { + // Edge case: the user picked an identity the tenant's policy + // rejects (e.g. a personal Microsoft Live account). B2C redirects back + // with error=server_error — the real authorizationCodeGrant throws + // AuthorizationResponseError, which must land the browser in the SPA + const { cookie, state } = await login() + + const res = await app.inject({ + method: 'GET', + url: `/auth/callback?error=server_error&error_description=AADB2C90085&state=${state}`, + headers: { cookie }, + }) + + expect(res.statusCode).toBe(302) + expect(res.headers.location).toBe('/?signInError=provider') + const sess = [...rows.values()][0].sess as Record + expect(sess.accessToken).toBeUndefined() + }) + + it('redirects home silently when the user cancels on the B2C page (access_denied)', async () => { + const { cookie, state } = await login() + + const res = await app.inject({ + method: 'GET', + url: `/auth/callback?error=access_denied&error_description=AADB2C90091&state=${state}`, + headers: { cookie }, + }) + + expect(res.statusCode).toBe(302) + expect(res.headers.location).toBe('/') + const sess = [...rows.values()][0].sess as Record + expect(sess.accessToken).toBeUndefined() + }) + it('returns 400 token_missing_email_claim when a valid id_token has no email', async () => { // The token is cryptographically valid — the grant succeeds — so this // exercises the handler guard on top of real validation, not a crypto error. diff --git a/src/App.tsx b/src/App.tsx index 1613b2bcf..51473e5a5 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -49,6 +49,24 @@ function App() { // mount instead of on every render. }, []) + /** + * The BFF /auth/callback lands here with ?signInError=provider when B2C answered the authorization request with an + * error instead of a code. The known case is when Microsoft provides login options that use an unsupported client id + */ + useEffect(() => { + const queryParams = new URLSearchParams(location.search) + if (queryParams.get('signInError') === null) return + Notifications.showError({ + text: 'Sign in could not be completed because the identity provider reported an error. ' + + 'If you chose a personal Microsoft account (such as an Outlook.com or Live account), that account type is not supported — ' + + 'please sign in with Google or with a Microsoft work or school account.', + // Long timeout: the message carries instructions the user must read. + timeout: 30000, + }) + queryParams.delete('signInError') + navigate({ pathname: location.pathname, search: queryParams.toString() }, { replace: true }) + }, [navigate, location.pathname, location.search]) + /** * Check for RAS Authentication URL params. If we have a code and state, we will call ECM APIs to get redirect * information and user linkage information. With that, we can sync the users account linkage and then redirect the diff --git a/test/components/App.spec.tsx b/test/components/App.spec.tsx index 706d65b41..bc23043fa 100644 --- a/test/components/App.spec.tsx +++ b/test/components/App.spec.tsx @@ -133,6 +133,33 @@ describe('Main App Functions', () => { await waitFor(() => expect(document.querySelector('[data-cy="notification-alert"]')).toBeVisible()) }) + it('shows the sign-in error toast and strips the marker when the BFF callback lands with ?signInError', async () => { + // The BFF /auth/callback redirects here when B2C answers the + // authorization request with an error instead of a code (e.g. a + // rejected Microsoft authentication attempt). + vi.mocked(useSessionInfo).mockReturnValue({ authenticated: false }) + const searchSpy = vi.fn() + const SearchSpy = () => { + const location = useLocation() + React.useEffect(() => { + searchSpy(location.search) + }, [location]) + return null + } + + render( + + + + , + ) + + await waitFor(() => expect(document.querySelector('[data-cy="notification-alert"]')).toBeVisible()) + expect(document.querySelector('[data-cy="notification-alert"]')?.textContent).toContain('identity provider') + // Stripped so a reload or bookmark does not repeat the toast. + await waitFor(() => expect(searchSpy).toHaveBeenCalledWith('')) + }) + it('should process RAS query params (code, state) and navigate to the profile page when the parameter specifies it', async () => { vi.mocked(AuthenticateNIH.getECMProviderLinkInfo).mockResolvedValue(linkInfo as never) vi.mocked(AuthenticateNIH.getSyncedUser).mockResolvedValue(duosUser as never) From 8b778d2fbbe1c47b53842e0e366c3b2dc083ef5d Mon Sep 17 00:00:00 2001 From: Greg Date: Tue, 25 Aug 2026 11:48:02 -0400 Subject: [PATCH 3/7] [DT-4012] Extract getMe helpers to satisfy Sonar cognitive complexity (S3776) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sonar flagged getMe at 18 against the allowed 15 after the 401/404/409 mapping landed. The refresh-before-forward block moves to refreshedIfExpiring() and the 409 body parse to providerConflictMessage() — same behavior, getMe now reads as one status-to-answer mapping. Co-Authored-By: Claude Fable 5 --- server/src/auth/me.ts | 86 +++++++++++++++++++++++++------------------ 1 file changed, 51 insertions(+), 35 deletions(-) diff --git a/server/src/auth/me.ts b/server/src/auth/me.ts index a875521db..1e8f12be6 100644 --- a/server/src/auth/me.ts +++ b/server/src/auth/me.ts @@ -24,6 +24,55 @@ async function destroySession(request: FastifyRequest, reply: FastifyReply): Pro reply.clearCookie('sessionId') } +/** + * Refresh-before-forward, mirroring the API proxy: an idle tab can outlive + * the access token while the refresh token and session are still perfectly + * valid. Forwarding the expired token would 401 upstream and destroy a + * session that only needed a refresh — and the client's focus revalidation + * makes this exact path hot. + * + * Returns false when the refresh failed and the reply has already gone out. + */ +async function refreshedIfExpiring(request: FastifyRequest, reply: FastifyReply): Promise { + const secondsRemaining = (request.session.tokenExpiry ?? 0) - Math.floor(Date.now() / 1000) + if (secondsRemaining >= REFRESH_WINDOW_SECONDS) return true + try { + await refreshAccessToken(request) + return true + } + catch (err: unknown) { + if (err instanceof RefreshFailedError) { + // Terminal: B2C rejected the refresh token and refreshAccessToken has + // already destroyed the session — clear the dead cookie. + reply.clearCookie('sessionId').status(401).send({ authenticated: false }) + return false + } + // Transient (network blip, B2C 5xx, store error) — the session is + // intact, so this must not read as signed out permanently: 502 tells + // the client probe to retry on the next ask. + reply.status(502).send({ authenticated: false, error: 'upstream_unavailable' }) + return false + } +} + +/** + * The actionable message from the upstream 409 body ({ message, code }), or + * the fallback when the body is empty, malformed, or not a string. + */ +async function providerConflictMessage(res: Response): Promise { + try { + const body: unknown = await res.json() + const upstreamMessage = (body as { message?: unknown } | null)?.message + if (typeof upstreamMessage === 'string' && upstreamMessage.length > 0) { + return upstreamMessage + } + } + catch { + // Unparseable body — the fallback message stands. + } + return PROVIDER_CONFLICT_FALLBACK_MESSAGE +} + /** * Confirms the user is authenticated against the upstream Consent API. * Forwards the upstream user profile and the active sub-provider — never the @@ -47,30 +96,7 @@ export async function getMe(request: FastifyRequest, reply: FastifyReply): Promi return } - // Refresh-before-forward, mirroring the API proxy: an idle tab can outlive - // the access token while the refresh token and session are still perfectly - // valid. Forwarding the expired token would 401 upstream and destroy a - // session that only needed a refresh — and the client's focus revalidation - // makes this exact path hot. - const secondsRemaining = (request.session.tokenExpiry ?? 0) - Math.floor(Date.now() / 1000) - if (secondsRemaining < REFRESH_WINDOW_SECONDS) { - try { - await refreshAccessToken(request) - } - catch (err: unknown) { - if (err instanceof RefreshFailedError) { - // Terminal: B2C rejected the refresh token and refreshAccessToken has - // already destroyed the session — clear the dead cookie. - reply.clearCookie('sessionId').status(401).send({ authenticated: false }) - return - } - // Transient (network blip, B2C 5xx, store error) — the session is - // intact, so this must not read as signed out permanently: 502 tells - // the client probe to retry on the next ask. - reply.status(502).send({ authenticated: false, error: 'upstream_unavailable' }) - return - } - } + if (!(await refreshedIfExpiring(request, reply))) return const url = `${requireEnv('DUOS_API_URL')}/api/user/me` @@ -114,17 +140,7 @@ export async function getMe(request: FastifyRequest, reply: FastifyReply): Promi // Registration cannot succeed and the session cannot become usable, so end it — but forward the // upstream's actionable message (sign in with the other provider, plus the support link) instead // of a generic failure. - let message = PROVIDER_CONFLICT_FALLBACK_MESSAGE - try { - const body: unknown = await res.json() - const upstreamMessage = (body as { message?: unknown } | null)?.message - if (typeof upstreamMessage === 'string' && upstreamMessage.length > 0) { - message = upstreamMessage - } - } - catch { - // Unparseable body — the fallback message stands. - } + const message = await providerConflictMessage(res) await destroySession(request, reply) reply.status(409).send({ authenticated: false, error: 'provider_conflict', message }) return From d6d001e7f58774a592ffac114c44a474e56b58d3 Mon Sep 17 00:00:00 2001 From: Greg Date: Tue, 25 Aug 2026 12:31:27 -0400 Subject: [PATCH 4/7] [DT-4012] Address Copilot review: unmapped-status comment, cancel log level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - me.ts: the !res.ok comment claimed "non-4xx", but the branch also catches contract-undefined 4xx (400/403/429) — reworded to match the behavior, which stays deliberately transient. - callback.ts: log the user's own B2C cancel (access_denied) at info so it stays out of warn-based alerting; real provider errors stay warn. Co-Authored-By: Claude Fable 5 --- server/src/auth/callback.ts | 7 +++++-- server/src/auth/me.ts | 7 ++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/server/src/auth/callback.ts b/server/src/auth/callback.ts index 151b7ee2c..7edbd59e8 100644 --- a/server/src/auth/callback.ts +++ b/server/src/auth/callback.ts @@ -30,8 +30,11 @@ export async function handleCallback(request: FastifyRequest, reply: FastifyRepl // code — the user canceled on the B2C page (access_denied), or chose // an identity the tenant's policy rejects. Land back in the // SPA instead; a cancel is the user's own action and stays silent. - request.log.warn({ error: err.error, description: err.error_description }, '[auth] B2C authorization response is an error') - reply.redirect(err.error === 'access_denied' ? '/' : '/?signInError=provider') + // A cancel is routine — info keeps it out of warn-based alerting; + // real provider errors stay at warn. + const cancelled = err.error === 'access_denied' + request.log[cancelled ? 'info' : 'warn']({ error: err.error, description: err.error_description }, '[auth] B2C authorization response is an error') + reply.redirect(cancelled ? '/' : '/?signInError=provider') return } throw err diff --git a/server/src/auth/me.ts b/server/src/auth/me.ts index 1e8f12be6..7505bc54e 100644 --- a/server/src/auth/me.ts +++ b/server/src/auth/me.ts @@ -147,9 +147,10 @@ export async function getMe(request: FastifyRequest, reply: FastifyReply): Promi } if (!res.ok) { - // A non-4xx failure (5xx, upstream outage) says nothing about whether the - // token itself is still valid — don't destroy the session or parse an - // error body as if it were a user profile. + // An unmapped status — 5xx, an upstream outage, or a 4xx the contract + // does not define (400/403/429) — says nothing about whether the token + // itself is still valid: don't destroy the session or parse an error + // body as if it were a user profile. 502 tells the probe to retry. reply.status(502).send({ authenticated: false, error: 'upstream_unavailable' }) return } From ee4cda7b09ba924713b289b6a123f05741a4bb7e Mon Sep 17 00:00:00 2001 From: Greg Date: Tue, 25 Aug 2026 13:12:59 -0400 Subject: [PATCH 5/7] [DT-4012] Reattribute the B2C authorization error and genericize the toast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server_error on /auth/callback is not a rejected account type: the non-prod B2C tenant's federation client secret to the upstream Microsoft provider expired (AADB2C90289 / invalid_client), which fails every Microsoft sign-in in those environments. The old toast told users their account type was unsupported — wrong advice for a provider-side fault. It now says the provider reported an error, try again, contact Terra support; the server log keeps the B2C error and description. Comments and the test's error_description now name the real cause. Co-Authored-By: Claude Fable 5 --- server/src/auth/callback.ts | 12 +++++++----- server/test/authCrypto.test.ts | 7 ++++--- src/App.tsx | 7 ++++--- test/components/App.spec.tsx | 4 ++-- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/server/src/auth/callback.ts b/server/src/auth/callback.ts index 7edbd59e8..07998273f 100644 --- a/server/src/auth/callback.ts +++ b/server/src/auth/callback.ts @@ -27,11 +27,13 @@ export async function handleCallback(request: FastifyRequest, reply: FastifyRepl catch (err: unknown) { if (err instanceof oidc.AuthorizationResponseError) { // B2C answered the authorization request with an error instead of a - // code — the user canceled on the B2C page (access_denied), or chose - // an identity the tenant's policy rejects. Land back in the - // SPA instead; a cancel is the user's own action and stays silent. - // A cancel is routine — info keeps it out of warn-based alerting; - // real provider errors stay at warn. + // code — the user canceled on the B2C page (access_denied), or B2C + // itself failed (error=server_error; observed as AADB2C90289 when a + // tenant's federation client secret to the upstream Microsoft provider + // expired, which fails EVERY Microsoft sign-in in that environment). + // Land back in the SPA instead; a cancel is the user's own action + // and stays silent. A cancel is also routine — info keeps it out of + // warn-based alerting; real provider errors stay at warn. const cancelled = err.error === 'access_denied' request.log[cancelled ? 'info' : 'warn']({ error: err.error, description: err.error_description }, '[auth] B2C authorization response is an error') reply.redirect(cancelled ? '/' : '/?signInError=provider') diff --git a/server/test/authCrypto.test.ts b/server/test/authCrypto.test.ts index f9cbfcbf4..dfb447982 100644 --- a/server/test/authCrypto.test.ts +++ b/server/test/authCrypto.test.ts @@ -311,15 +311,16 @@ describe('B2C OAuth callback (real openid-client validation against a fake B2C)' }) it('redirects to /?signInError=provider when B2C answers with an error instead of a code', async () => { - // Edge case: the user picked an identity the tenant's policy - // rejects (e.g. a personal Microsoft Live account). B2C redirects back + // Edge case: B2C cannot complete the federated sign-in (observed when a + // tenant's federation client secret to the upstream Microsoft provider + // has expired — every Microsoft account then fails). B2C redirects back // with error=server_error — the real authorizationCodeGrant throws // AuthorizationResponseError, which must land the browser in the SPA const { cookie, state } = await login() const res = await app.inject({ method: 'GET', - url: `/auth/callback?error=server_error&error_description=AADB2C90085&state=${state}`, + url: `/auth/callback?error=server_error&error_description=AADB2C90289&state=${state}`, headers: { cookie }, }) diff --git a/src/App.tsx b/src/App.tsx index 51473e5a5..0ad785395 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -51,15 +51,16 @@ function App() { /** * The BFF /auth/callback lands here with ?signInError=provider when B2C answered the authorization request with an - * error instead of a code. The known case is when Microsoft provides login options that use an unsupported client id + * error instead of a code. The known case is B2C failing to connect to the chosen Microsoft provider (an expired + * federation client secret in the tenant fails every Microsoft sign-in). The message stays generic because the + * cause is on the provider side — the server log carries the B2C error and description. */ useEffect(() => { const queryParams = new URLSearchParams(location.search) if (queryParams.get('signInError') === null) return Notifications.showError({ text: 'Sign in could not be completed because the identity provider reported an error. ' - + 'If you chose a personal Microsoft account (such as an Outlook.com or Live account), that account type is not supported — ' - + 'please sign in with Google or with a Microsoft work or school account.', + + 'Please try again. If the problem continues, contact Terra support.', // Long timeout: the message carries instructions the user must read. timeout: 30000, }) diff --git a/test/components/App.spec.tsx b/test/components/App.spec.tsx index bc23043fa..8c8c0012c 100644 --- a/test/components/App.spec.tsx +++ b/test/components/App.spec.tsx @@ -135,8 +135,8 @@ describe('Main App Functions', () => { it('shows the sign-in error toast and strips the marker when the BFF callback lands with ?signInError', async () => { // The BFF /auth/callback redirects here when B2C answers the - // authorization request with an error instead of a code (e.g. a - // rejected Microsoft authentication attempt). + // authorization request with an error instead of a code (e.g. B2C could + // not complete a federated Microsoft sign-in). vi.mocked(useSessionInfo).mockReturnValue({ authenticated: false }) const searchSpy = vi.fn() const SearchSpy = () => { From e5d4b65093d58492629b3ff0c66d807f10c7a0d8 Mon Sep 17 00:00:00 2001 From: Greg Date: Tue, 25 Aug 2026 13:16:17 -0400 Subject: [PATCH 6/7] [DT-4012] Point the sign-in error toast at DUOS support, not Terra Co-Authored-By: Claude Fable 5 --- src/App.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/App.tsx b/src/App.tsx index 0ad785395..8785eecdf 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -60,7 +60,7 @@ function App() { if (queryParams.get('signInError') === null) return Notifications.showError({ text: 'Sign in could not be completed because the identity provider reported an error. ' - + 'Please try again. If the problem continues, contact Terra support.', + + 'Please try again. If the problem continues, contact DUOS support.', // Long timeout: the message carries instructions the user must read. timeout: 30000, }) From 2396d50cc625b581d3dbaae8ca2657d23fafb069 Mon Sep 17 00:00:00 2001 From: Greg Date: Tue, 25 Aug 2026 13:30:16 -0400 Subject: [PATCH 7/7] doc: prune comments --- server/src/auth/callback.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/server/src/auth/callback.ts b/server/src/auth/callback.ts index 07998273f..0a4ec852c 100644 --- a/server/src/auth/callback.ts +++ b/server/src/auth/callback.ts @@ -28,12 +28,9 @@ export async function handleCallback(request: FastifyRequest, reply: FastifyRepl if (err instanceof oidc.AuthorizationResponseError) { // B2C answered the authorization request with an error instead of a // code — the user canceled on the B2C page (access_denied), or B2C - // itself failed (error=server_error; observed as AADB2C90289 when a - // tenant's federation client secret to the upstream Microsoft provider - // expired, which fails EVERY Microsoft sign-in in that environment). - // Land back in the SPA instead; a cancel is the user's own action - // and stays silent. A cancel is also routine — info keeps it out of - // warn-based alerting; real provider errors stay at warn. + // itself failed. Land back in the SPA instead; a cancel is the user's + // own action and stays silent. A cancel is also routine — info keeps + // it out of warn-based alerting; real provider errors stay at warn. const cancelled = err.error === 'access_denied' request.log[cancelled ? 'info' : 'warn']({ error: err.error, description: err.error_description }, '[auth] B2C authorization response is an error') reply.redirect(cancelled ? '/' : '/?signInError=provider')