From 4b05406b4c4cefa2188792087d0faad53d0620b3 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 11 Sep 2026 10:13:34 -0400 Subject: [PATCH 1/2] fix(auth): apply a refreshed operation token to the client, not just the replayed request A direct-connect client keeps the Bearer it was built with, so a memoized one (useInstanceClientIdParams) re-sends the expired token on every poll tick once it expires: a guaranteed 401 plus a replay per tick, indefinitely, and silent because shouldKeepEvent drops 401s. RUM caught 87 such 401s against one instance in 19 minutes on the browse-data view. Fixes #1700 --- ...ecoveredOperationToken.integration.test.ts | 35 ++++++++ ...yExpiredOperationToken.integration.test.ts | 84 +++++++++++++++++++ .../api/retryExpiredOperationToken.test.ts | 42 +++++++++- .../api/retryExpiredOperationToken.ts | 18 +++- 4 files changed, 176 insertions(+), 3 deletions(-) create mode 100644 src/features/auth/store/recoveredOperationToken.integration.test.ts create mode 100644 src/integrations/api/retryExpiredOperationToken.integration.test.ts diff --git a/src/features/auth/store/recoveredOperationToken.integration.test.ts b/src/features/auth/store/recoveredOperationToken.integration.test.ts new file mode 100644 index 000000000..2bd0d04e8 --- /dev/null +++ b/src/features/auth/store/recoveredOperationToken.integration.test.ts @@ -0,0 +1,35 @@ +/** @vitest-environment jsdom */ +import { describe, expect, it, vi } from 'vitest'; + +const createInstanceAuthenticationTokens = vi.fn(); +const refreshInstanceOperationToken = vi.fn(); +const getInstanceUserInfo = vi.fn(); +vi.mock('@/integrations/api/instance/auth/createInstanceAuthenticationTokens', () => ({ + createInstanceAuthenticationTokens: (...args: unknown[]) => createInstanceAuthenticationTokens(...args), + refreshInstanceOperationToken: (...args: unknown[]) => refreshInstanceOperationToken(...args), + mintOperationTokenWithCredentials: vi.fn(), +})); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ + getInstanceUserInfo: (...args: unknown[]) => getInstanceUserInfo(...args), +})); + +const { authStore } = await import('@/features/auth/store/authStore'); + +const ID = 'ins-recovered'; +const OPERATIONS_URL = 'https://ins-recovered.example.com:9925/'; + +describe('a recovered operation token', () => { + it('is what getOperationToken returns once the refresh succeeds', async () => { + createInstanceAuthenticationTokens.mockResolvedValue({ operationToken: 'op-1', refreshToken: 'rt-1' }); + getInstanceUserInfo.mockResolvedValue({ username: 'someone' }); + await authStore.establishFabricConnectAuth({ id: ID, operationsUrl: OPERATIONS_URL }); + expect(authStore.getOperationToken(ID)).toBe('op-1'); + + refreshInstanceOperationToken.mockResolvedValue('op-2'); + + const recovered = await authStore.recoverExpiredOperationToken(ID); + + expect(recovered).toBe('op-2'); + expect(authStore.getOperationToken(ID)).toBe(recovered); + }); +}); diff --git a/src/integrations/api/retryExpiredOperationToken.integration.test.ts b/src/integrations/api/retryExpiredOperationToken.integration.test.ts new file mode 100644 index 000000000..7fb8e407c --- /dev/null +++ b/src/integrations/api/retryExpiredOperationToken.integration.test.ts @@ -0,0 +1,84 @@ +/** @vitest-environment jsdom */ +import { getInstanceClient } from '@/config/getInstanceClient'; +import { authStore } from '@/features/auth/store/authStore'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const INSTANCE_ID = 'ins-123'; +const DIRECT_URL = 'https://my-instance.example.com:9925/'; +const STALE = 'stale-token'; +const FRESH = 'fresh-token'; + +/** Axios only applies `validateStatus` inside its own adapters, so a custom one must reject. */ +function instanceAcceptingOnly(token: string, seen: string[]) { + return async (config: InternalAxiosRequestConfig) => { + const authorization = String(config.headers.Authorization ?? ''); + seen.push(authorization); + if (authorization === `Bearer ${token}`) { + return { status: 200, statusText: 'OK', data: { ok: true }, headers: {}, config }; + } + const response = { status: 401, statusText: 'Unauthorized', data: {}, headers: {}, config }; + throw new AxiosError('Unauthorized', AxiosError.ERR_BAD_REQUEST, config, undefined, response); + }; +} + +function directConnectHolding(operationToken: string, recovered: string) { + vi.spyOn(authStore, 'getOperationToken').mockReturnValue(operationToken); + vi.spyOn(authStore, 'checkForFabricConnect').mockReturnValue(true); + vi.spyOn(authStore, 'checkForBasicAuth').mockReturnValue(undefined); + vi.spyOn(authStore, 'getOperationsUrl').mockReturnValue(DIRECT_URL); + const recover = vi.spyOn(authStore, 'recoverExpiredOperationToken').mockImplementation(async () => { + vi.spyOn(authStore, 'getOperationToken').mockReturnValue(recovered); + return recovered; + }); + return recover; +} + +function staleSends(seen: string[]) { + return seen.filter((authorization) => authorization === `Bearer ${STALE}`).length; +} + +describe('a direct-connect client that outlives its operation token', () => { + afterEach(() => vi.restoreAllMocks()); + + it('stops sending the expired Bearer once a refresh has recovered a fresh one', async () => { + directConnectHolding(STALE, FRESH); + const seen: string[] = []; + const client = getInstanceClient({ id: INSTANCE_ID }); + client.defaults.adapter = instanceAcceptingOnly(FRESH, seen); + + await expect(client.post('/', {})).resolves.toMatchObject({ status: 200 }); + await expect(client.post('/', {})).resolves.toMatchObject({ status: 200 }); + await expect(client.post('/', {})).resolves.toMatchObject({ status: 200 }); + + expect(staleSends(seen)).toBe(1); + }); + + it('pays the stale Bearer once per request in the first burst, then never again', async () => { + directConnectHolding(STALE, FRESH); + const seen: string[] = []; + const client = getInstanceClient({ id: INSTANCE_ID }); + client.defaults.adapter = instanceAcceptingOnly(FRESH, seen); + + await Promise.all([client.post('/', {}), client.post('/', {}), client.post('/', {})]); + expect(staleSends(seen)).toBe(3); + + await client.post('/', {}); + expect(staleSends(seen)).toBe(3); + }); + + it('fails the request outright when the mint lands after the store dropped the token', async () => { + directConnectHolding(STALE, FRESH); + vi.spyOn(authStore, 'recoverExpiredOperationToken').mockImplementation(async () => { + vi.spyOn(authStore, 'getOperationToken').mockReturnValue(undefined); + return FRESH; + }); + const seen: string[] = []; + const client = getInstanceClient({ id: INSTANCE_ID }); + client.defaults.adapter = instanceAcceptingOnly(FRESH, seen); + + await expect(client.post('/', {})).rejects.toMatchObject({ response: { status: 401 } }); + + expect(client.defaults.headers.Authorization).toBe(`Bearer ${STALE}`); + }); +}); diff --git a/src/integrations/api/retryExpiredOperationToken.test.ts b/src/integrations/api/retryExpiredOperationToken.test.ts index 6ed01a24f..788a432a6 100644 --- a/src/integrations/api/retryExpiredOperationToken.test.ts +++ b/src/integrations/api/retryExpiredOperationToken.test.ts @@ -2,8 +2,12 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; const recoverExpiredOperationToken = vi.fn(); +const getOperationToken = vi.fn(); vi.mock('@/features/auth/store/authStore', () => ({ - authStore: { recoverExpiredOperationToken: (...args: unknown[]) => recoverExpiredOperationToken(...args) }, + authStore: { + recoverExpiredOperationToken: (...args: unknown[]) => recoverExpiredOperationToken(...args), + getOperationToken: (...args: unknown[]) => getOperationToken(...args), + }, })); const { curryRecoverExpiredOperationToken } = await import('./retryExpiredOperationToken'); @@ -17,6 +21,7 @@ describe('curryRecoverExpiredOperationToken', () => { it('mints a fresh token and replays the request once with the new Bearer header', async () => { recoverExpiredOperationToken.mockResolvedValue('fresh-token'); + getOperationToken.mockReturnValue('fresh-token'); const request = vi.fn().mockResolvedValue({ data: 'ok' }); const handler = curryRecoverExpiredOperationToken({ request }, 'ins-1'); const config = { headers: { Authorization: 'Bearer stale' } }; @@ -31,6 +36,41 @@ describe('curryRecoverExpiredOperationToken', () => { expect(replayed.__triedOperationTokenRefresh).toBe(true); }); + it('writes the fresh Bearer to the client defaults so later requests skip the 401', async () => { + recoverExpiredOperationToken.mockResolvedValue('fresh-token'); + getOperationToken.mockReturnValue('fresh-token'); + const request = vi.fn().mockResolvedValue({ data: 'ok' }); + const defaults = { headers: { Authorization: 'Bearer stale' } }; + const handler = curryRecoverExpiredOperationToken({ request, defaults }, 'ins-1'); + + await handler(error401({ headers: { Authorization: 'Bearer stale' } })); + + expect(defaults.headers.Authorization).toBe('Bearer fresh-token'); + }); + + it('neither arms nor replays when the store no longer holds the recovered token', async () => { + recoverExpiredOperationToken.mockResolvedValue('fresh-token'); + getOperationToken.mockReturnValue(undefined); + const request = vi.fn().mockResolvedValue({ data: 'ok' }); + const defaults = { headers: { Authorization: 'Bearer stale' } }; + const handler = curryRecoverExpiredOperationToken({ request, defaults }, 'ins-1'); + const err = error401({ headers: { Authorization: 'Bearer stale' } }); + + await expect(handler(err)).rejects.toBe(err); + + expect(defaults.headers.Authorization).toBe('Bearer stale'); + expect(request).not.toHaveBeenCalled(); + }); + + it('still replays for a client that exposes no defaults', async () => { + recoverExpiredOperationToken.mockResolvedValue('fresh-token'); + getOperationToken.mockReturnValue('fresh-token'); + const request = vi.fn().mockResolvedValue({ data: 'ok' }); + const handler = curryRecoverExpiredOperationToken({ request }, 'ins-1'); + + await expect(handler(error401())).resolves.toEqual({ data: 'ok' }); + }); + it('rejects (no retry) when recovery yields no token', async () => { recoverExpiredOperationToken.mockResolvedValue(null); const request = vi.fn(); diff --git a/src/integrations/api/retryExpiredOperationToken.ts b/src/integrations/api/retryExpiredOperationToken.ts index 924b0f4f8..9da2057b4 100644 --- a/src/integrations/api/retryExpiredOperationToken.ts +++ b/src/integrations/api/retryExpiredOperationToken.ts @@ -6,8 +6,14 @@ import { AxiosInstance } from 'axios'; * operation token has most likely expired mid-session; recover a fresh one (via the refresh token, * falling back to a proxy re-mint) and replay the request once with the new Bearer token. A per-request * flag caps this at a single retry so a still-rejected token can't loop. + * + * Clients outlive the token they are built with: `useInstanceClientIdParams` memoizes one per + * mounted view, keyed on route params. */ -export function curryRecoverExpiredOperationToken(client: Pick, id: EntityIds) { +export function curryRecoverExpiredOperationToken( + client: Pick & { defaults?: { headers?: Record } }, + id: EntityIds, +) { // eslint-disable-next-line @typescript-eslint/no-explicit-any return async (error: any) => { const status = error?.response?.status as number | undefined; @@ -21,8 +27,16 @@ export function curryRecoverExpiredOperationToken(client: Pick Date: Mon, 14 Sep 2026 09:31:54 -0400 Subject: [PATCH 2/2] fix(auth): read the direct-connect Bearer at send time, scoped to the connection that minted it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Arming one client's defaults after a refresh left every peer client for the same entity on the expired Bearer: `useInstanceClientIdParams` memoizes per hook call, so one page holds several. Direct-connect clients now stamp the Authorization header from the store on every send, so a single refresh advances all of them. Recovery is also bound to a connection generation. `mode === 'direct'` cannot tell one connection from the next, so a mint racing a disconnect/reconnect could overwrite the new identity's token and replay the old request as the new user. Each request carries the generation it was sent under; a token is committed, and a request replayed, only when that still matches. The refresh exchange opts out of the token lifecycle. It was being built with an operation token present, so it got this same 401 interceptor and re-entered the mint awaiting it — a promise cycle that never settled and hung every later request for that entity. The proxy re-mint fallback is now reachable. Addresses review feedback on #1702 and fixes #1701. Co-Authored-By: Claude Opus 5 (1M context) --- src/config/getInstanceClient.ts | 39 ++- src/features/auth/store/authStore.ts | 71 +++- ...ecoveredOperationToken.integration.test.ts | 35 -- ...yExpiredOperationToken.integration.test.ts | 312 ++++++++++++++---- .../api/retryExpiredOperationToken.test.ts | 59 ++-- .../api/retryExpiredOperationToken.ts | 22 +- 6 files changed, 380 insertions(+), 158 deletions(-) delete mode 100644 src/features/auth/store/recoveredOperationToken.integration.test.ts diff --git a/src/config/getInstanceClient.ts b/src/config/getInstanceClient.ts index 684cbe5eb..b4ef8944e 100644 --- a/src/config/getInstanceClient.ts +++ b/src/config/getInstanceClient.ts @@ -3,7 +3,7 @@ import { authStore, EntityIds, OverallAppSignIn } from '@/features/auth/store/au import { rejectReplicationFailures } from '@/integrations/api/replication'; import { curryRecoverExpiredOperationToken } from '@/integrations/api/retryExpiredOperationToken'; import { curryRetryGatewayErrors } from '@/integrations/api/retryGatewayErrors'; -import axios from 'axios'; +import axios, { AxiosError } from 'axios'; interface InstanceClient { id?: EntityIds; @@ -21,10 +21,17 @@ export function getInstanceClient( forceFabricConnect, disableFabricConnect, forceOperationToken, + disableTokenRecovery, }: InstanceClient & { forceFabricConnect?: boolean; disableFabricConnect?: boolean; forceOperationToken?: boolean; + /** + * Opt this client out of the direct-connect token lifecycle entirely — no request-time Bearer, + * no 401 recovery. Set by the refresh call itself: it carries the refresh token as its own + * Bearer, and a recovery interceptor on it would re-enter the mint that is awaiting it. + */ + disableTokenRecovery?: boolean; } = {}, ) { let baseURL = operationsUrl || authStore.getOperationsUrl(id); @@ -72,13 +79,41 @@ export function getInstanceClient( }, baseURL, }); + // The Bearer comes from the store on every send, not from the header baked in at construction: a + // client outlives its token, so one refresh has to advance every client the page holds. A retry is + // not a new send — `curryRetryGatewayErrors` sleeps up to 20s, long enough to reconnect as someone + // else — so a request keeps the generation of its first send. + if (operationToken && !disableTokenRecovery) { + client.interceptors.request.use((config) => { + const stamped = config as typeof config & { __connectionGeneration?: number }; + const generation = authStore.getConnectionGeneration(id); + if (stamped.__connectionGeneration === undefined) { + stamped.__connectionGeneration = generation; + } else if (stamped.__connectionGeneration !== generation) { + throw new AxiosError( + 'Connection replaced before this request could be retried.', + AxiosError.ERR_CANCELED, + config, + ); + } + const token = authStore.getOperationToken(id); + if (token) { + config.headers.Authorization = `Bearer ${token}`; + } else { + // Signed out: drop the constructor-baked header rather than letting a JWT the store has + // discarded — still valid at the instance for ~a day — ride out on a retained client. + delete config.headers.Authorization; + } + return config; + }); + } client.interceptors.response.use( rejectReplicationFailures, curryRetryGatewayErrors(client), ); // Direct-connect clients recover from a 401 (expired operation token) by minting a fresh token and // replaying once. Registered after the gateway-retry handler, which passes a 401 straight through. - if (operationToken) { + if (operationToken && !disableTokenRecovery) { client.interceptors.response.use(undefined, curryRecoverExpiredOperationToken(client, id)); } return client; diff --git a/src/features/auth/store/authStore.ts b/src/features/auth/store/authStore.ts index d0c5e8b38..d77e4d865 100644 --- a/src/features/auth/store/authStore.ts +++ b/src/features/auth/store/authStore.ts @@ -78,6 +78,9 @@ class AuthStore { >(); private readonly fabricConnectInFlight = new Map>(); private readonly operationTokenRefreshInFlight = new Map>(); + // An entity can be disconnected and reconnected as a different user while a mint is in flight, so + // `mode === 'direct'` alone cannot tell one connection's credential from the next one's. + private readonly directConnectionGeneration = new Map(); // Sign-out generations for the API explorer, per entity plus a global `'*'` slot. The explorer keeps // its credential per browser tab (sessionStorage), which SURVIVES a reload — so an event-only signal @@ -329,6 +332,7 @@ class AuthStore { } else { localStorage.removeItem(this.fabricConnectKeyPrefix + id); this.fabricConnectAuth.delete(id); + this.bumpConnectionGeneration(id); } } @@ -346,6 +350,15 @@ class AuthStore { return value === 'fabric' || value === 'direct' ? value : undefined; } + public getConnectionGeneration(id: EntityIds): number { + return this.directConnectionGeneration.get(id) ?? 0; + } + + private bumpConnectionGeneration(id: EntityIds): void { + this.directConnectionGeneration.set(id, this.getConnectionGeneration(id) + 1); + this.operationTokenRefreshInFlight.delete(id); + } + /** The in-memory Fabric Connect JWT for direct connect, or undefined if not connected directly. */ public getOperationToken(id: EntityIds): string | undefined { const fabric = this.fabricConnectAuth.get(id); @@ -389,23 +402,33 @@ class AuthStore { if (inFlight) { return inFlight; } - const promise = this.mintFreshOperationToken(id, fabric.refreshToken); + const generation = this.getConnectionGeneration(id); + const promise = this.mintFreshOperationToken(id, fabric.refreshToken, generation); this.operationTokenRefreshInFlight.set(id, promise); - return promise.finally(() => this.operationTokenRefreshInFlight.delete(id)); + return promise.finally(() => { + // Only evict our own entry: a reconnect clears the map, and the replacement connection may + // already have registered its refresh by the time this one settles. + if (this.operationTokenRefreshInFlight.get(id) === promise) { + this.operationTokenRefreshInFlight.delete(id); + } + }); } - private async mintFreshOperationToken(id: EntityIds, refreshToken: string | undefined): Promise { + private async mintFreshOperationToken( + id: EntityIds, + refreshToken: string | undefined, + generation: number, + ): Promise { // Cheap path: exchange the refresh token for a new operation token, directly at the instance. // forceOperationToken keeps getInstanceClient on the direct URL (not the proxy) even if a stale // basic-auth entry exists; refreshInstanceOperationToken overrides the Bearer with the refresh token. if (refreshToken) { try { const token = await refreshInstanceOperationToken({ - instanceClient: getInstanceClient({ id, forceOperationToken: true }), + instanceClient: getInstanceClient({ id, forceOperationToken: true, disableTokenRecovery: true }), refreshToken, }); - this.updateDirectOperationToken(id, token, refreshToken); - return token; + return this.updateDirectOperationToken(id, token, refreshToken, generation) ? token : null; } catch (err) { console.debug( 'Operation token refresh failed; re-minting via proxy', @@ -415,23 +438,37 @@ class AuthStore { } // Fall back to minting a fresh pair through the proxy. + if (this.getConnectionGeneration(id) !== generation) { + return null; + } try { const { operationToken, refreshToken: newRefreshToken } = await createInstanceAuthenticationTokens({ instanceClient: getInstanceClient({ id, forceFabricConnect: true }), }); - this.updateDirectOperationToken(id, operationToken, newRefreshToken ?? refreshToken); - return operationToken; + return this.updateDirectOperationToken(id, operationToken, newRefreshToken ?? refreshToken, generation) + ? operationToken + : null; } catch (err) { console.debug('Operation token re-mint failed', err instanceof Error ? err.message : err); return null; } } - /** Update the in-memory direct token, but only if a concurrent logout/flag-off hasn't cleared it. */ - private updateDirectOperationToken(id: EntityIds, token: string, refreshToken: string | undefined): void { - if (this.fabricConnectAuth.get(id)?.mode === 'direct') { - this.fabricConnectAuth.set(id, { mode: 'direct', token, refreshToken }); + /** + * Commit a freshly minted direct token, but only onto the connection that asked for it: a + * drop-and-reconnect leaves `mode` unchanged while the identity behind it changes. + */ + private updateDirectOperationToken( + id: EntityIds, + token: string, + refreshToken: string | undefined, + generation: number, + ): boolean { + if (this.fabricConnectAuth.get(id)?.mode !== 'direct' || this.getConnectionGeneration(id) !== generation) { + return false; } + this.fabricConnectAuth.set(id, { mode: 'direct', token, refreshToken }); + return true; } /** @@ -469,6 +506,7 @@ class AuthStore { // (or the passed URL) is a proxy URL we'd send the instance JWT to the central-manager origin. // In that case skip straight to the proxy fallback. if (isDirectOperationsUrl(operationsUrl)) { + this.bumpConnectionGeneration(id); this.fabricConnectAuth.set(id, { mode: 'direct', token: operationToken, refreshToken }); try { // forceOperationToken so a stale basic-auth entry for this id can't shadow the token we @@ -490,6 +528,7 @@ class AuthStore { } // Proxy fallback: route every operation through the central-manager proxy. + this.bumpConnectionGeneration(id); this.fabricConnectAuth.set(id, { mode: 'proxy' }); const proxyClient = getInstanceClient({ id, forceFabricConnect: true }); const user = await getInstanceUserInfo({ instanceClient: proxyClient }); @@ -500,6 +539,7 @@ class AuthStore { } catch (err) { // Couldn't establish either mode — clear the half-resolved state so we retry next time. this.fabricConnectAuth.delete(id); + this.bumpConnectionGeneration(id); throw err; } } @@ -540,6 +580,10 @@ class AuthStore { instanceClient.defaults.timeout = LOGOUT_TIMEOUT_MS; // No gateway-error retries (5s + 10s + 20s) and no token recovery for a best-effort logout. instanceClient.interceptors.response.clear(); + // The request interceptor re-reads the store at send time, and the caller clears the store + // before posting — leaving it on would strip the very credential the logout must present. + // The constructor-baked header carries it instead. + instanceClient.interceptors.request.clear(); return instanceClient; } catch (err: unknown) { reportLogoutFailure(entityId, err); @@ -577,6 +621,9 @@ class AuthStore { for (const id of Object.keys(this.potentiallyAuthenticated)) { this.signOutLocally(id); } + for (const id of this.fabricConnectAuth.keys()) { + this.bumpConnectionGeneration(id); + } this.fabricConnectAuth.clear(); this.fabricConnectInFlight.clear(); this.operationTokenRefreshInFlight.clear(); diff --git a/src/features/auth/store/recoveredOperationToken.integration.test.ts b/src/features/auth/store/recoveredOperationToken.integration.test.ts deleted file mode 100644 index 2bd0d04e8..000000000 --- a/src/features/auth/store/recoveredOperationToken.integration.test.ts +++ /dev/null @@ -1,35 +0,0 @@ -/** @vitest-environment jsdom */ -import { describe, expect, it, vi } from 'vitest'; - -const createInstanceAuthenticationTokens = vi.fn(); -const refreshInstanceOperationToken = vi.fn(); -const getInstanceUserInfo = vi.fn(); -vi.mock('@/integrations/api/instance/auth/createInstanceAuthenticationTokens', () => ({ - createInstanceAuthenticationTokens: (...args: unknown[]) => createInstanceAuthenticationTokens(...args), - refreshInstanceOperationToken: (...args: unknown[]) => refreshInstanceOperationToken(...args), - mintOperationTokenWithCredentials: vi.fn(), -})); -vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ - getInstanceUserInfo: (...args: unknown[]) => getInstanceUserInfo(...args), -})); - -const { authStore } = await import('@/features/auth/store/authStore'); - -const ID = 'ins-recovered'; -const OPERATIONS_URL = 'https://ins-recovered.example.com:9925/'; - -describe('a recovered operation token', () => { - it('is what getOperationToken returns once the refresh succeeds', async () => { - createInstanceAuthenticationTokens.mockResolvedValue({ operationToken: 'op-1', refreshToken: 'rt-1' }); - getInstanceUserInfo.mockResolvedValue({ username: 'someone' }); - await authStore.establishFabricConnectAuth({ id: ID, operationsUrl: OPERATIONS_URL }); - expect(authStore.getOperationToken(ID)).toBe('op-1'); - - refreshInstanceOperationToken.mockResolvedValue('op-2'); - - const recovered = await authStore.recoverExpiredOperationToken(ID); - - expect(recovered).toBe('op-2'); - expect(authStore.getOperationToken(ID)).toBe(recovered); - }); -}); diff --git a/src/integrations/api/retryExpiredOperationToken.integration.test.ts b/src/integrations/api/retryExpiredOperationToken.integration.test.ts index 7fb8e407c..80cfdf71a 100644 --- a/src/integrations/api/retryExpiredOperationToken.integration.test.ts +++ b/src/integrations/api/retryExpiredOperationToken.integration.test.ts @@ -1,84 +1,278 @@ /** @vitest-environment jsdom */ -import { getInstanceClient } from '@/config/getInstanceClient'; -import { authStore } from '@/features/auth/store/authStore'; -import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; -import { afterEach, describe, expect, it, vi } from 'vitest'; - -const INSTANCE_ID = 'ins-123'; -const DIRECT_URL = 'https://my-instance.example.com:9925/'; -const STALE = 'stale-token'; -const FRESH = 'fresh-token'; - -/** Axios only applies `validateStatus` inside its own adapters, so a custom one must reject. */ -function instanceAcceptingOnly(token: string, seen: string[]) { +import axios, { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const createInstanceAuthenticationTokens = vi.fn(); +const getInstanceUserInfo = vi.fn(); +vi.mock('@/integrations/api/instance/auth/createInstanceAuthenticationTokens', async (importOriginal) => ({ + ...(await importOriginal()), + createInstanceAuthenticationTokens: (...args: unknown[]) => createInstanceAuthenticationTokens(...args), +})); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ + getInstanceUserInfo: (...args: unknown[]) => getInstanceUserInfo(...args), +})); + +const { authStore } = await import('@/features/auth/store/authStore'); +const { getInstanceClient } = await import('@/config/getInstanceClient'); + +const ID = 'ins-123'; +const OPERATIONS_URL = 'https://ins-123.example.com:9925/'; + +function bearer(config: InternalAxiosRequestConfig): string { + return String(config.headers.Authorization ?? ''); +} + +/** Axios applies `validateStatus` inside its own adapters, so a custom one must reject non-2xx. */ +function reject401(config: InternalAxiosRequestConfig): never { + const response = { status: 401, statusText: 'Unauthorized', data: {}, headers: {}, config }; + throw new AxiosError('Unauthorized', AxiosError.ERR_BAD_REQUEST, config, undefined, response); +} + +function ok(config: InternalAxiosRequestConfig, data: unknown = { ok: true }) { + return { status: 200, statusText: 'OK', data, headers: {}, config }; +} + +let seen: string[] = []; + +function record(config: InternalAxiosRequestConfig): string { + const body = typeof config.data === 'string' ? JSON.parse(config.data) : config.data; + const label = (body as { operation?: string } | undefined)?.operation ?? 'call'; + seen.push(`${label}:${bearer(config).replace('Bearer ', '')}`); + return label; +} + +async function connectDirectly(token = 'op-1') { + createInstanceAuthenticationTokens.mockResolvedValue({ operationToken: token, refreshToken: 'rt-1' }); + getInstanceUserInfo.mockResolvedValue({ username: 'someone' }); + await authStore.establishFabricConnectAuth({ id: ID, operationsUrl: OPERATIONS_URL }); + expect(authStore.getOperationToken(ID)).toBe(token); +} + +/** An instance that accepts only `accepted`, and answers a refresh exchange with `mintsTo`. */ +function instance({ accepted, mintsTo }: { accepted: string; mintsTo?: string }) { return async (config: InternalAxiosRequestConfig) => { - const authorization = String(config.headers.Authorization ?? ''); - seen.push(authorization); - if (authorization === `Bearer ${token}`) { - return { status: 200, statusText: 'OK', data: { ok: true }, headers: {}, config }; + const operation = record(config); + if (operation === 'refresh_operation_token') { + return mintsTo ? ok(config, { operation_token: mintsTo }) : reject401(config); } - const response = { status: 401, statusText: 'Unauthorized', data: {}, headers: {}, config }; - throw new AxiosError('Unauthorized', AxiosError.ERR_BAD_REQUEST, config, undefined, response); + return bearer(config) === `Bearer ${accepted}` ? ok(config) : reject401(config); }; } -function directConnectHolding(operationToken: string, recovered: string) { - vi.spyOn(authStore, 'getOperationToken').mockReturnValue(operationToken); - vi.spyOn(authStore, 'checkForFabricConnect').mockReturnValue(true); - vi.spyOn(authStore, 'checkForBasicAuth').mockReturnValue(undefined); - vi.spyOn(authStore, 'getOperationsUrl').mockReturnValue(DIRECT_URL); - const recover = vi.spyOn(authStore, 'recoverExpiredOperationToken').mockImplementation(async () => { - vi.spyOn(authStore, 'getOperationToken').mockReturnValue(recovered); - return recovered; - }); - return recover; +/** One fake instance for every client, including the refresh client the store builds internally. */ +function serve(adapter: ReturnType) { + axios.defaults.adapter = adapter; } -function staleSends(seen: string[]) { - return seen.filter((authorization) => authorization === `Bearer ${STALE}`).length; +function client() { + return getInstanceClient({ id: ID, operationsUrl: OPERATIONS_URL }); } -describe('a direct-connect client that outlives its operation token', () => { - afterEach(() => vi.restoreAllMocks()); +describe('a direct-connect client whose operation token expires mid-session', () => { + const originalAdapter = axios.defaults.adapter; + + beforeEach(() => { + seen = []; + vi.clearAllMocks(); + authStore.flagForFabricConnect(ID, false); + }); + + afterEach(() => { + axios.defaults.adapter = originalAdapter; + }); it('stops sending the expired Bearer once a refresh has recovered a fresh one', async () => { - directConnectHolding(STALE, FRESH); - const seen: string[] = []; - const client = getInstanceClient({ id: INSTANCE_ID }); - client.defaults.adapter = instanceAcceptingOnly(FRESH, seen); + await connectDirectly(); + serve(instance({ accepted: 'op-2', mintsTo: 'op-2' })); + const browse = client(); - await expect(client.post('/', {})).resolves.toMatchObject({ status: 200 }); - await expect(client.post('/', {})).resolves.toMatchObject({ status: 200 }); - await expect(client.post('/', {})).resolves.toMatchObject({ status: 200 }); + await expect(browse.post('/', {})).resolves.toMatchObject({ status: 200 }); + await expect(browse.post('/', {})).resolves.toMatchObject({ status: 200 }); + await expect(browse.post('/', {})).resolves.toMatchObject({ status: 200 }); - expect(staleSends(seen)).toBe(1); + expect(seen.filter((entry) => entry === 'call:op-1')).toHaveLength(1); + expect(seen).toContain('refresh_operation_token:rt-1'); }); - it('pays the stale Bearer once per request in the first burst, then never again', async () => { - directConnectHolding(STALE, FRESH); - const seen: string[] = []; - const client = getInstanceClient({ id: INSTANCE_ID }); - client.defaults.adapter = instanceAcceptingOnly(FRESH, seen); + it('advances every retained client for the entity, not just the one that refreshed', async () => { + await connectDirectly(); + serve(instance({ accepted: 'op-2', mintsTo: 'op-2' })); + const browse = client(); + const peer = client(); + + await expect(browse.post('/', {})).resolves.toMatchObject({ status: 200 }); + seen = []; + await expect(peer.post('/', {})).resolves.toMatchObject({ status: 200 }); + + expect(seen).toEqual(['call:op-2']); + }); + + it('settles rather than deadlocking when the refresh token is rejected too', async () => { + await connectDirectly(); + // No `mintsTo`: the refresh exchange 401s, exactly as an expired refresh token would. + serve(instance({ accepted: 'nothing' })); + const browse = client(); + createInstanceAuthenticationTokens.mockReset(); + createInstanceAuthenticationTokens.mockRejectedValue(new Error('proxy re-mint also rejected')); + + const settled = await Promise.race([ + browse.post('/', {}).then(() => 'resolved', () => 'rejected'), + new Promise((resolve) => setTimeout(() => resolve('HUNG'), 3000)), + ]); + + expect(settled).toBe('rejected'); + // The proxy re-mint fallback is reachable only because the refresh call never re-entered recovery. + expect(createInstanceAuthenticationTokens).toHaveBeenCalledTimes(1); + }, 10_000); + + it('cancels a retry of a request composed under a connection that has since been replaced', async () => { + await connectDirectly(); + serve(instance({ accepted: 'op-other' })); + const browse = client(); + // `curryRetryGatewayErrors` re-sends `error.config` after a backoff of up to 20s — long enough + // for the user to have reconnected as someone else. + const composed = await browse.post('/', {}).then(() => undefined, (error: AxiosError) => error.config); + expect(composed).toBeDefined(); + authStore.flagForFabricConnect(ID, false); + await connectDirectly('op-other'); + seen = []; + + await expect(browse.request(composed!)).rejects.toMatchObject({ code: AxiosError.ERR_CANCELED }); + + expect(seen).toEqual([]); + }); + + it('does not make a new connection wait on the replaced connection\u2019s hung refresh', async () => { + await connectDirectly(); + let hangFirstRefresh: () => void = () => {}; + const stalled = new Promise((resolve) => { + hangFirstRefresh = resolve; + }); + let refreshes = 0; + serve(async (config) => { + const operation = record(config); + if (operation === 'refresh_operation_token') { + if (refreshes++ === 0) { + await stalled; + return reject401(config); + } + return ok(config, { operation_token: 'op-final' }); + } + return bearer(config) === 'Bearer op-final' ? ok(config) : reject401(config); + }); + createInstanceAuthenticationTokens.mockRejectedValue(new Error('proxy re-mint unavailable')); + + const stranded = client().post('/', {}).then(() => 'resolved', () => 'rejected'); + await vi.waitFor(() => expect(refreshes).toBe(1)); + authStore.flagForFabricConnect(ID, false); + await connectDirectly('op-other'); + + const reconnected = await Promise.race([ + client().post('/', {}).then(() => 'resolved', () => 'rejected'), + new Promise((resolve) => setTimeout(() => resolve('HUNG'), 3000)), + ]); + + expect(reconnected).toBe('resolved'); + hangFirstRefresh(); + await expect(stranded).resolves.toBe('rejected'); + }, 10_000); + + it('sends no Bearer at all from a retained client once the store has signed out', async () => { + await connectDirectly(); + serve(async (config) => { + record(config); + return ok(config); + }); + const retained = client(); + await retained.post('/', {}); + seen = []; + authStore.flagForFabricConnect(ID, false); + + await retained.post('/', {}); + + expect(seen).toEqual(['call:']); + }); + + it('does not let a superseded refresh evict the live one, spawning a duplicate mint', async () => { + await connectDirectly(); + const gates: Array<() => void> = []; + let refreshes = 0; + serve(async (config) => { + const operation = record(config); + if (operation === 'refresh_operation_token') { + refreshes++; + await new Promise((resolve) => gates.push(resolve)); + return reject401(config); + } + return reject401(config); + }); + createInstanceAuthenticationTokens.mockRejectedValue(new Error('proxy re-mint unavailable')); + + const stranded = client().post('/', {}).then(() => 'resolved', () => 'rejected'); + await vi.waitFor(() => expect(refreshes).toBe(1)); + authStore.flagForFabricConnect(ID, false); + await connectDirectly('op-2'); + const live = client().post('/', {}).then(() => 'resolved', () => 'rejected'); + await vi.waitFor(() => expect(refreshes).toBe(2)); + + // The superseded refresh settles while the live one is still in flight. + gates[0](); + await expect(stranded).resolves.toBe('rejected'); + const joined = client().post('/', {}).then(() => 'resolved', () => 'rejected'); + + // A third exchange here would mean the live refresh was evicted and this request started its own. + await vi.waitFor(() => expect(gates).toHaveLength(2)); + expect(refreshes).toBe(2); + gates[1](); + await expect(Promise.all([live, joined])).resolves.toEqual(['rejected', 'rejected']); + }, 10_000); + + it('still presents the operation token on the sign-out logout, which posts after the store is cleared', async () => { + await connectDirectly(); + serve(async (config) => { + record(config); + return ok(config, { message: 'logged out' }); + }); + + await authStore.signOutFromPotentiallyAuthenticatedInstances(); + + expect(seen).toContain('logout:op-1'); + }); + + it('discards a refresh that succeeds after the connection it was minted for was replaced', async () => { + await connectDirectly(); + let replaced = false; + serve(async (config) => { + const operation = record(config); + if (operation === 'refresh_operation_token') { + if (!replaced) { + replaced = true; + authStore.flagForFabricConnect(ID, false); + await connectDirectly('op-other'); + } + return ok(config, { operation_token: 'op-stale' }); + } + return reject401(config); + }); - await Promise.all([client.post('/', {}), client.post('/', {}), client.post('/', {})]); - expect(staleSends(seen)).toBe(3); + await expect(client().post('/', {})).rejects.toMatchObject({ response: { status: 401 } }); - await client.post('/', {}); - expect(staleSends(seen)).toBe(3); + expect(authStore.getOperationToken(ID)).toBe('op-other'); }); - it('fails the request outright when the mint lands after the store dropped the token', async () => { - directConnectHolding(STALE, FRESH); - vi.spyOn(authStore, 'recoverExpiredOperationToken').mockImplementation(async () => { - vi.spyOn(authStore, 'getOperationToken').mockReturnValue(undefined); - return FRESH; + it('does not replay a request whose connection was replaced mid-flight', async () => { + await connectDirectly(); + serve(async (config) => { + record(config); + // The user disconnects and reconnects as someone else while this request is in flight. + authStore.flagForFabricConnect(ID, false); + await connectDirectly('op-other'); + return reject401(config); }); - const seen: string[] = []; - const client = getInstanceClient({ id: INSTANCE_ID }); - client.defaults.adapter = instanceAcceptingOnly(FRESH, seen); - await expect(client.post('/', {})).rejects.toMatchObject({ response: { status: 401 } }); + await expect(client().post('/', {})).rejects.toMatchObject({ response: { status: 401 } }); - expect(client.defaults.headers.Authorization).toBe(`Bearer ${STALE}`); + expect(seen).toEqual(['call:op-1']); + expect(authStore.getOperationToken(ID)).toBe('op-other'); }); }); diff --git a/src/integrations/api/retryExpiredOperationToken.test.ts b/src/integrations/api/retryExpiredOperationToken.test.ts index 788a432a6..7f62c8344 100644 --- a/src/integrations/api/retryExpiredOperationToken.test.ts +++ b/src/integrations/api/retryExpiredOperationToken.test.ts @@ -2,77 +2,63 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; const recoverExpiredOperationToken = vi.fn(); -const getOperationToken = vi.fn(); +const getConnectionGeneration = vi.fn(); vi.mock('@/features/auth/store/authStore', () => ({ authStore: { recoverExpiredOperationToken: (...args: unknown[]) => recoverExpiredOperationToken(...args), - getOperationToken: (...args: unknown[]) => getOperationToken(...args), + getConnectionGeneration: (...args: unknown[]) => getConnectionGeneration(...args), }, })); const { curryRecoverExpiredOperationToken } = await import('./retryExpiredOperationToken'); -function error401(config: Record = { headers: {} }) { +function error401(config: Record = { headers: {}, __connectionGeneration: 1 }) { return { response: { status: 401 }, config }; } describe('curryRecoverExpiredOperationToken', () => { afterEach(() => vi.clearAllMocks()); - it('mints a fresh token and replays the request once with the new Bearer header', async () => { + it('mints a fresh token and replays the request once', async () => { recoverExpiredOperationToken.mockResolvedValue('fresh-token'); - getOperationToken.mockReturnValue('fresh-token'); + getConnectionGeneration.mockReturnValue(1); const request = vi.fn().mockResolvedValue({ data: 'ok' }); const handler = curryRecoverExpiredOperationToken({ request }, 'ins-1'); - const config = { headers: { Authorization: 'Bearer stale' } }; - const result = await handler(error401(config)); + const result = await handler(error401()); expect(result).toEqual({ data: 'ok' }); expect(recoverExpiredOperationToken).toHaveBeenCalledWith('ins-1'); expect(request).toHaveBeenCalledTimes(1); - const replayed = request.mock.calls[0][0]; - expect(replayed.headers.Authorization).toBe('Bearer fresh-token'); - expect(replayed.__triedOperationTokenRefresh).toBe(true); + expect(request.mock.calls[0][0].__triedOperationTokenRefresh).toBe(true); }); - it('writes the fresh Bearer to the client defaults so later requests skip the 401', async () => { - recoverExpiredOperationToken.mockResolvedValue('fresh-token'); - getOperationToken.mockReturnValue('fresh-token'); - const request = vi.fn().mockResolvedValue({ data: 'ok' }); - const defaults = { headers: { Authorization: 'Bearer stale' } }; - const handler = curryRecoverExpiredOperationToken({ request, defaults }, 'ins-1'); - - await handler(error401({ headers: { Authorization: 'Bearer stale' } })); - - expect(defaults.headers.Authorization).toBe('Bearer fresh-token'); - }); - - it('neither arms nor replays when the store no longer holds the recovered token', async () => { - recoverExpiredOperationToken.mockResolvedValue('fresh-token'); - getOperationToken.mockReturnValue(undefined); - const request = vi.fn().mockResolvedValue({ data: 'ok' }); - const defaults = { headers: { Authorization: 'Bearer stale' } }; - const handler = curryRecoverExpiredOperationToken({ request, defaults }, 'ins-1'); - const err = error401({ headers: { Authorization: 'Bearer stale' } }); + it('does not recover a request sent under a superseded connection', async () => { + getConnectionGeneration.mockReturnValue(2); + const request = vi.fn(); + const handler = curryRecoverExpiredOperationToken({ request }, 'ins-1'); + const err = error401({ headers: {}, __connectionGeneration: 1 }); await expect(handler(err)).rejects.toBe(err); - - expect(defaults.headers.Authorization).toBe('Bearer stale'); + expect(recoverExpiredOperationToken).not.toHaveBeenCalled(); expect(request).not.toHaveBeenCalled(); }); - it('still replays for a client that exposes no defaults', async () => { + it('does not replay when the connection is replaced while recovery is in flight', async () => { + getConnectionGeneration.mockReturnValueOnce(1).mockReturnValue(2); recoverExpiredOperationToken.mockResolvedValue('fresh-token'); - getOperationToken.mockReturnValue('fresh-token'); - const request = vi.fn().mockResolvedValue({ data: 'ok' }); + const request = vi.fn(); const handler = curryRecoverExpiredOperationToken({ request }, 'ins-1'); + const err = error401(); - await expect(handler(error401())).resolves.toEqual({ data: 'ok' }); + await expect(handler(err)).rejects.toBe(err); + expect(recoverExpiredOperationToken).toHaveBeenCalledTimes(1); + expect(request).not.toHaveBeenCalled(); }); it('rejects (no retry) when recovery yields no token', async () => { recoverExpiredOperationToken.mockResolvedValue(null); + getConnectionGeneration.mockReturnValue(1); const request = vi.fn(); const handler = curryRecoverExpiredOperationToken({ request }, 'ins-1'); const err = error401(); @@ -91,9 +77,10 @@ describe('curryRecoverExpiredOperationToken', () => { }); it('does not retry twice (guards against a loop when the fresh token is also rejected)', async () => { + getConnectionGeneration.mockReturnValue(1); const request = vi.fn(); const handler = curryRecoverExpiredOperationToken({ request }, 'ins-1'); - const err = error401({ headers: {}, __triedOperationTokenRefresh: true }); + const err = error401({ headers: {}, __connectionGeneration: 1, __triedOperationTokenRefresh: true }); await expect(handler(err)).rejects.toBe(err); expect(recoverExpiredOperationToken).not.toHaveBeenCalled(); diff --git a/src/integrations/api/retryExpiredOperationToken.ts b/src/integrations/api/retryExpiredOperationToken.ts index 9da2057b4..0563c90b6 100644 --- a/src/integrations/api/retryExpiredOperationToken.ts +++ b/src/integrations/api/retryExpiredOperationToken.ts @@ -7,13 +7,11 @@ import { AxiosInstance } from 'axios'; * falling back to a proxy re-mint) and replay the request once with the new Bearer token. A per-request * flag caps this at a single retry so a still-rejected token can't loop. * - * Clients outlive the token they are built with: `useInstanceClientIdParams` memoizes one per - * mounted view, keyed on route params. + * A request is only recovered and replayed on the connection it was sent under. The entity can be + * disconnected and reconnected as a different user while the request is in flight, and replaying + * then would run one user's operation as another. */ -export function curryRecoverExpiredOperationToken( - client: Pick & { defaults?: { headers?: Record } }, - id: EntityIds, -) { +export function curryRecoverExpiredOperationToken(client: Pick, id: EntityIds) { // eslint-disable-next-line @typescript-eslint/no-explicit-any return async (error: any) => { const status = error?.response?.status as number | undefined; @@ -22,21 +20,17 @@ export function curryRecoverExpiredOperationToken( return Promise.reject(error); } - const token = await authStore.recoverExpiredOperationToken(id); - if (!token) { + const generation = config.__connectionGeneration as number | undefined; + if (generation !== authStore.getConnectionGeneration(id)) { return Promise.reject(error); } - if (authStore.getOperationToken(id) !== token) { + const token = await authStore.recoverExpiredOperationToken(id); + if (!token || generation !== authStore.getConnectionGeneration(id)) { return Promise.reject(error); } - const authorization = `Bearer ${token}`; - if (client.defaults?.headers) { - client.defaults.headers.Authorization = authorization; - } config.__triedOperationTokenRefresh = true; - config.headers = { ...config.headers, Authorization: authorization }; return client.request(config); }; }