diff --git a/AGENTS.md b/AGENTS.md index e22d73dbf..2ba94b760 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -784,8 +784,8 @@ Three traps when reading this from RUM data: A boot-time redirect does **not** cost you that first view, and the deciding factor is whether `getAllConnections()` can answer synchronously. It synthesizes `{ user: null, isLoading: false }` -for `OverallAppSignIn` only when the `Studio:PotentiallyAuthenticated` localStorage record has no -entry for it; with an entry it returns the record untouched, so the key is simply **absent**. +for `OverallAppSignIn` only when no `Studio:PotentiallyAuthenticated:OverallAppSignIn` marker is +stored; with one it returns the record untouched, so the key is simply **absent**. `dashboardLayout.beforeLoad` guards on `auth && !auth.isLoading && !auth.user`, so those are two different outcomes: a signed-out deep link redirects _during_ the router's initial load and the root component never commits the deep-link location — one `startView`, named `/sign-in/` — while an diff --git a/DESIGN.md b/DESIGN.md index c3fec3c59..4522c6b2b 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -109,9 +109,32 @@ Central manager only ever sends the boolean, never the other organizations' ids; its own route: TanStack resolves it against the _current_ location, so a row that outlives a pending navigation re-resolves its href one level too deep (HarperFast/studio#1710, #1711). `EntityMenuItem.to` accepts only the helper's `AbsolutePath`, so a relative menu target fails `tsc`. -- **Remembered instance/cluster sign-ins are checked on demand**: `Studio:PotentiallyAuthenticated` spans every - organization ever visited, so the root auth subscription checks only the app sign-in; an entity is checked by +- **Remembered instance/cluster sign-ins are checked on demand**: the sign-in markers span every organization ever + visited, so the root auth subscription checks only the app sign-in; an entity is checked by `useInstanceAuth`/`listenToEntity` or the instance-layout guard (`entityAuthCheckScope.test.ts`, HarperFast/studio#1329). +- **Each sign-in marker has its own key, `Studio:PotentiallyAuthenticated:`, holding the URL signed into** + (`signInMarkers.ts`, HarperFast/studio#1968). A tab writes only the entity it signs in or out, so tabs can't erase or + restore each other's markers; app and full sign-outs act on every stored marker, at each URL an entity is marked at + here or in storage. A check never undoes another tab's change: a passing one of a sign-in this tab already marked + writes nothing, and a failing one removes only the marker it checked. So a marker removed elsewhere stays removed + while this tab still holds that sign-in, until this tab signs into the entity again, which always writes; for the + app sign-in, which every `/User/current` answer re-signs, those answers count as checks. A check's answer, or a + `/User/current` answer, is dropped when this tab signed in again while it was out (`crossTabMarkers.test.ts`, + `getCurrentUser.test.ts`). A sign-in storage refuses to record still signs in, and its next sign-in or check writes + it; a marker storage won't remove is retried at every marker write, unless another tab has rewritten it since + (`localSignOutStorageFailure.test.ts`). +- **The previous release's one shared record is kept in step for one more release.** Its tabs read + `Studio:PotentiallyAuthenticated` once at load and write their whole copy back, and know nothing of the per-entity + keys. So this release copies that record into the keys once per storage (a marker already stored per entity wins), + mirrors every marker change into it, and reads it in every sign-out sweep. A previous-release tab, or a rollback, + therefore still sees this release's sign-ins and sign-outs, and an app sign-out here reaches a sign-in only that tab + recorded. Its sign-in reaches this release's keys only at that first copy, and its sign-out of an entity doesn't + remove the entity's key, which matters only if its logout also failed; its full sign-out wipes both. The copy is + never repeated, because that tab's whole-copy writes would bring back what this release signed out, so the sign-out + wipe keeps the flag that records it. Until the copy lands, as when a full quota refuses it, the record also seeds + this tab's markers (`legacySignInMarkers.test.ts`, `account-menu.anon.spec.ts`). Every load also removes a record + that isn't one, which the previous release can't start on, and drops entries that aren't markers; a sign-in leaves + one it can't read alone. The release after this one stops the mirror and removes the record (HarperFast/studio#1970). - **`Dashboard` renders nothing under it without a signed-in user.** Route data loads in parallel with the page load's `/User/current` check (waiting for it costs +262/680 ms p75 on live org/cluster cold loads), and central manager answers a request with no session with 403, so a deep link on an expired session has its route data fail before the @@ -132,9 +155,9 @@ Central manager only ever sends the boolean, never the other organizations' ids; from their next request. A tab follows only when the name is its own app user: a late 401 or event from user U's session must not sign out a tab that has since signed in as V. A tab whose check is still out remembers the user, and if the check comes back as them, asks again: central manager may have confirmed them just before the session ended. - Tabs follow that key, not the marker record, because a tab also drops the app sign-in from the record when its own - check merely fails (a timeout, a 5xx). A tab that follows writes no `localStorage`: its copy of the markers is a - snapshot from its own load, and writing it back could undo the other tab's next sign-in (`crossTabSignOut.test.tsx`). + Tabs follow that key, not the app sign-in's marker, because a tab also drops that marker when its own check merely + fails (a timeout, a 5xx). A tab that follows writes no `localStorage`: the other tab already wiped it, and a write + could undo that tab's next sign-in (`crossTabSignOut.test.tsx`). - **Data fetching**: TanStack React Query 5. Query keys must be instance-scoped when the request targets a specific instance. - **Polling**: every `refetchInterval` on a query that can reject against an instance or a cluster goes through [`pollUnlessForbidden`](src/react-query/pollUnlessForbidden.ts), which stops the timer on a 403 and backs off a sustained 400 or 5xx; a raw number polls a failing endpoint at full rate for as long as the page is open. When the interval depends on the data, compute it and hand it over per tick: `(query) => pollUnlessForbidden(base(query))(query)` (the back-off state is per query, not per wrapper). - **Forms**: react-hook-form with a zod `zodResolver`. A form whose submit button waits on `formState.isValid` must not use the default `onSubmit` mode, which keeps `isValid` current but shows errors only after a submit the disabled button never allows, so the user is blocked with no reason given (HarperFast/studio#1550). Use `mode: 'onTouched'`: a field's error appears once it is first left and then follows each change, while `isValid` still tracks every keystroke (`onBlur` recomputes `isValid` only on blur, so the button lags the text). Enter neither submits past a disabled button nor leaves the field, so give the `
` `onKeyDown={revealFieldErrorOnEnter(form)}`, which treats Enter in a text input as leaving it. A refine that compares two fields reports on one of them; re-check that field from the other's `onChange` once it has been touched (`getFieldState(name).isTouched`, then `trigger(name)`), because `rules={{ deps }}` fires on the other field's first blur, before the user reaches it. react-hook-form only updates the `formState` fields a render has read, and `disabled={!isDirty || !isValid}` skips `isValid` while the form is pristine, so where one change can complete the form (a single field, a paste, a checkbox) the button stays disabled on a valid form. Read the fields before the JSX (`const { isDirty, isValid } = form.formState;`) and pin it with a test that makes one change and checks the button. diff --git a/e2e/README.md b/e2e/README.md index b17b5fb1e..85a49b0ab 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -121,7 +121,7 @@ Naming drives which project runs a spec: `*.anon.spec.ts` = no session, - **Hash routing.** Routes are `/#/sign-in`, `/#/verifying`, `/#//users`. - **No test-ids** in these flows — drive by role/label/text (`getByRole`, `getByLabel`). The only `data-testid`s in the app are in instance analytics. -- **Auth is a cookie** (`POST /Login/`); the `Studio:PotentiallyAuthenticated` +- **Auth is a cookie** (`POST /Login/`); the `Studio:PotentiallyAuthenticated:` localStorage flag is only a hint. `storageState` captures the cookie. - **Sign-in and sign-up render every submit failure inline** (`p[role="alert"][data-slot="form-message"]`). **Forgot-password renders only CAPTCHA rejections diff --git a/e2e/tests/account-menu.anon.spec.ts b/e2e/tests/account-menu.anon.spec.ts index cfb838708..8f7b1820c 100644 --- a/e2e/tests/account-menu.anon.spec.ts +++ b/e2e/tests/account-menu.anon.spec.ts @@ -1,4 +1,5 @@ import { expect, test } from '@playwright/test'; +import { appSignInMarker, rememberAppSignIn } from './signInMarkers'; const role = (name: string) => ({ organizationName: name, role: 'owner', permission: { super_user: true } }); const user = { @@ -18,9 +19,7 @@ test.beforeEach(async ({ page }) => { ['fetch', 'xhr'].includes(route.request().resourceType()) ? route.fulfill({ status: 404, json: { error: 'Unmocked test request' } }) : route.continue()); - await page.addInitScript(() => - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })) - ); + await rememberAppSignIn(page); await page.route('**/User/current', route => route.fulfill({ json: user })); await page.route('**/Organization/*', route => { const id = route.request().url().split('/').at(-1)!; @@ -87,7 +86,56 @@ test('account menu groups profile, organization switching, appearance and sign o await page.getByRole('menuitem', { name: 'Sign Out', exact: true }).click(); await expect(page).toHaveURL(/#\/sign-in/); expect(signedOut).toBe(true); - expect(await page.evaluate(() => localStorage.getItem('Studio:PotentiallyAuthenticated'))).toBeNull(); + expect(await page.evaluate((key) => localStorage.getItem(key), appSignInMarker)).toBeNull(); +}); + +test.describe('alongside a tab still on the previous release, which keeps every sign-in in one record', () => { + const legacyRecord = 'Studio:PotentiallyAuthenticated'; + + test('stays signed in on the sign-in that release recorded', async ({ page }) => { + await page.addInitScript(({ marker, legacy }) => { + if (!localStorage.getItem('Studio:PotentiallyAuthenticatedMigrated')) { + localStorage.removeItem(marker); + localStorage.setItem(legacy, JSON.stringify({ OverallAppSignIn: 'fixture' })); + } + }, { marker: appSignInMarker, legacy: legacyRecord }); + + await page.goto('/#/org-a'); + + await expect(page.getByRole('button', { name: 'Account menu' })).toBeVisible(); + expect(await page.evaluate((key) => localStorage.getItem(key), appSignInMarker)).toBeTruthy(); + }); + + test('signs out of a server only that tab signed into', async ({ page }) => { + const oldTabSignIn = 'https://ins-old.example.test:9925/'; + const instanceRequests: string[] = []; + await page.route(/ins-old\.example\.test/, route => { + instanceRequests.push(`${route.request().method()} ${route.request().postData() ?? ''}`); + return route.fulfill({ + json: { message: 'ok' }, + headers: { + 'Access-Control-Allow-Origin': route.request().headers()['origin'] ?? '*', + 'Access-Control-Allow-Credentials': 'true', + 'Access-Control-Allow-Headers': 'content-type', + }, + }); + }); + await page.route(/\/Logout\/?$/, route => route.fulfill({ json: {} })); + await page.goto('/#/org-a'); + await expect(page.getByRole('button', { name: 'Account menu' })).toBeVisible(); + // That tab writes its whole record back after this one has loaded. + await page.evaluate( + ({ legacy, url }) => + localStorage.setItem(legacy, JSON.stringify({ OverallAppSignIn: 'fixture', 'ins-old': url })), + { legacy: legacyRecord, url: oldTabSignIn }, + ); + + await page.getByRole('button', { name: 'Account menu' }).click(); + await page.getByRole('menuitem', { name: 'Sign Out', exact: true }).click(); + + await expect(page).toHaveURL(/#\/sign-in/); + expect(instanceRequests).toContain('POST {"operation":"logout"}'); + }); }); test('account controls remain reachable on a narrow screen with keyboard focus return', async ({ page }) => { diff --git a/e2e/tests/auth.setup.ts b/e2e/tests/auth.setup.ts index 69341f268..954014c37 100644 --- a/e2e/tests/auth.setup.ts +++ b/e2e/tests/auth.setup.ts @@ -14,7 +14,7 @@ const password = process.env.PLAYWRIGHT_USER_PASSWORD; /** * Logs in through the real UI once and persists the session (the cookie set by - * POST /Login/ — the localStorage `Studio:PotentiallyAuthenticated` flag is only + * POST /Login/ — the localStorage `Studio:PotentiallyAuthenticated:*` flags are only * a hint, so cookie-bearing storageState is what actually authenticates). * * Doubles as the sign-in happy-path smoke test. diff --git a/e2e/tests/cluster-form.anon.spec.ts b/e2e/tests/cluster-form.anon.spec.ts index 3c604e81d..69718415d 100644 --- a/e2e/tests/cluster-form.anon.spec.ts +++ b/e2e/tests/cluster-form.anon.spec.ts @@ -1,4 +1,5 @@ import { expect, test } from '@playwright/test'; +import { rememberAppSignIn } from './signInMarkers'; const region = { id: 'us-1', region: 'US', latencyDescription: '83ms', instanceCount: 2 }; const plan = { @@ -33,9 +34,7 @@ const organizationWith = (clusterRecord: typeof cluster) => ({ }); test.beforeEach(async ({ page }) => { - await page.addInitScript(() => - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })) - ); + await rememberAppSignIn(page); await page.route( '**/*', route => diff --git a/e2e/tests/clusters.anon.spec.ts b/e2e/tests/clusters.anon.spec.ts index 83fa07c30..723b61116 100644 --- a/e2e/tests/clusters.anon.spec.ts +++ b/e2e/tests/clusters.anon.spec.ts @@ -1,4 +1,5 @@ import { expect, test } from '@playwright/test'; +import { rememberAppSignIn } from './signInMarkers'; const clusters = [ { id: 'clu-production', name: 'Production', status: 'RUNNING', regionId: 'reg-east' }, @@ -26,9 +27,7 @@ const user = { }; test.beforeEach(async ({ page }) => { - await page.addInitScript(() => { - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })); - }); + await rememberAppSignIn(page); await page.route('**/User/current', route => route.fulfill({ json: user })); await page.route('**/Organization/org-fixture', route => route.fulfill({ json: organization })); await page.route('**/Cluster/clu-production', route => route.fulfill({ json: { ...clusters[0], instances: [] } })); diff --git a/e2e/tests/global-search.anon.spec.ts b/e2e/tests/global-search.anon.spec.ts index 24d33aa3b..69e252124 100644 --- a/e2e/tests/global-search.anon.spec.ts +++ b/e2e/tests/global-search.anon.spec.ts @@ -1,10 +1,9 @@ import { expect, test } from '@playwright/test'; +import { rememberAppSignIn } from './signInMarkers'; const role = (organizationName: string) => ({ organizationName, role: 'owner', permission: { super_user: true } }); test.beforeEach(async ({ page }) => { - await page.addInitScript(() => - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })) - ); + await rememberAppSignIn(page); await page.route( '**/*', route => diff --git a/e2e/tests/new-org.anon.spec.ts b/e2e/tests/new-org.anon.spec.ts index a80be010b..18ad24fdc 100644 --- a/e2e/tests/new-org.anon.spec.ts +++ b/e2e/tests/new-org.anon.spec.ts @@ -1,11 +1,10 @@ import { expect, test } from '@playwright/test'; +import { rememberAppSignIn } from './signInMarkers'; const user = { id: 'new-org-user', email: 'org@example.test', firstname: 'Taylor', lastname: 'River', roles: {} }; test.beforeEach(async ({ page }) => { - await page.addInitScript(() => - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })) - ); + await rememberAppSignIn(page); await page.route('**/*', route => ['fetch', 'xhr'].includes(route.request().resourceType()) ? route.fulfill({ status: 404, json: {} }) diff --git a/e2e/tests/organizations.anon.spec.ts b/e2e/tests/organizations.anon.spec.ts index c3b9a87a9..8d7976d40 100644 --- a/e2e/tests/organizations.anon.spec.ts +++ b/e2e/tests/organizations.anon.spec.ts @@ -1,4 +1,5 @@ import { expect, test } from '@playwright/test'; +import { rememberAppSignIn } from './signInMarkers'; const member = (organizationName: string, role = 'owner') => ({ organizationName, @@ -19,9 +20,7 @@ const user = { }; test.beforeEach(async ({ page }) => { - await page.addInitScript(() => - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })) - ); + await rememberAppSignIn(page); await page.route( '**/*', route => diff --git a/e2e/tests/profile.anon.spec.ts b/e2e/tests/profile.anon.spec.ts index 338b2d4a7..88ac67900 100644 --- a/e2e/tests/profile.anon.spec.ts +++ b/e2e/tests/profile.anon.spec.ts @@ -1,11 +1,10 @@ import { expect, test } from '@playwright/test'; +import { rememberAppSignIn } from './signInMarkers'; const user = { id: 'profile-user', email: 'profile@example.test', firstname: 'Taylor', lastname: 'River', roles: {} }; test.beforeEach(async ({ page }) => { - await page.addInitScript(() => - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })) - ); + await rememberAppSignIn(page); await page.route( '**/*', route => diff --git a/e2e/tests/sidebar-help.anon.spec.ts b/e2e/tests/sidebar-help.anon.spec.ts index 5373b689e..ebf9e3d8e 100644 --- a/e2e/tests/sidebar-help.anon.spec.ts +++ b/e2e/tests/sidebar-help.anon.spec.ts @@ -1,9 +1,8 @@ import { expect, test } from '@playwright/test'; +import { rememberAppSignIn } from './signInMarkers'; test.beforeEach(async ({ page }) => { - await page.addInitScript(() => - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'fixture' })) - ); + await rememberAppSignIn(page); await page.route( '**/User/current', route => diff --git a/e2e/tests/signInMarkers.ts b/e2e/tests/signInMarkers.ts new file mode 100644 index 000000000..07266ddae --- /dev/null +++ b/e2e/tests/signInMarkers.ts @@ -0,0 +1,9 @@ +import type { Page } from '@playwright/test'; + +/** Must match `signInMarkerKey(OverallAppSignIn)` in `src/features/auth/store/signInMarkers.ts`, which e2e can't import. */ +export const appSignInMarker = 'Studio:PotentiallyAuthenticated:OverallAppSignIn'; + +/** Marks the app signed in before the page loads, so it checks the mocked session instead of showing sign-in. */ +export function rememberAppSignIn(page: Page) { + return page.addInitScript((key) => localStorage.setItem(key, 'fixture'), appSignInMarker); +} diff --git a/src/features/auth/handlers/clearAuthStateLocally.ts b/src/features/auth/handlers/clearAuthStateLocally.ts index 4c8050046..fc56cafbb 100644 --- a/src/features/auth/handlers/clearAuthStateLocally.ts +++ b/src/features/auth/handlers/clearAuthStateLocally.ts @@ -5,8 +5,8 @@ import { queryClient } from '@/react-query/queryClient'; /** * Full LOCAL sign-out: clears all in-memory auth connections/tokens/flags, persisted storage, and BOTH * React Query caches, WITHOUT posting a logout to CM or instances. None of it may survive into a - * same-tab sign-in as another user, except the explorer's sign-out generation (see - * `authStore.wipeLocalStorageForSignOut`). + * same-tab sign-in as another user, except the explorer's sign-out generation and the sign-in markers' migration flag + * (see `authStore.wipeLocalStorageForSignOut`). */ export function clearAuthStateLocally(): void { runEvery([ diff --git a/src/features/auth/handlers/signOutHandlers.test.ts b/src/features/auth/handlers/signOutHandlers.test.ts index 4df256b09..79386c803 100644 --- a/src/features/auth/handlers/signOutHandlers.test.ts +++ b/src/features/auth/handlers/signOutHandlers.test.ts @@ -1,5 +1,6 @@ /** @vitest-environment jsdom */ import { authStore, OverallAppSignIn } from '@/features/auth/store/authStore'; +import { seedSignInMarkers, storedSignInMarkers } from '@/features/auth/store/signInMarkersTestHelpers'; import { queryClient } from '@/react-query/queryClient'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { clearAuthStateAfterSignOutElsewhere, clearAuthStateLocally } from './clearAuthStateLocally'; @@ -88,7 +89,8 @@ describe.each( expect(authStore.getConnectionById(OverallAppSignIn).user).toBeNull(); expect(authStore.getConnectionById('ins-a').user).toBeNull(); expectCachesEmpty(); - expect(localStorageKeys()).toEqual(['Studio:ExplorerAuthEpoch']); + // The migration flag holds no account data, and keeps a previous-release tab's write-back from being migrated. + expect(localStorageKeys().sort()).toEqual(['Studio:ExplorerAuthEpoch', 'Studio:PotentiallyAuthenticatedMigrated']); expect(sessionStorage.length).toBe(0); }); @@ -234,7 +236,7 @@ describe.each( await signOut(); expectCachesEmpty(); - expect(localStorage.length).toBe(0); + expect(localStorageKeys()).toEqual(['Studio:PotentiallyAuthenticatedMigrated']); expect(sessionStorage.length).toBe(0); expect(consoleError).toHaveBeenCalledWith(failure); }); @@ -244,7 +246,7 @@ describe('clearAuthStateAfterSignOutElsewhere', () => { it('signs this tab out and wipes both caches and its sessionStorage, but leaves the shared localStorage', () => { seedSignedInTab(); localStorage.clear(); - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn })); + seedSignInMarkers({ OverallAppSignIn }); clearAuthStateAfterSignOutElsewhere(); @@ -253,7 +255,7 @@ describe('clearAuthStateAfterSignOutElsewhere', () => { expectCachesEmpty(); expect(sessionStorage.length).toBe(0); expect(localStorage.length).toBe(1); - expect(localStorage.getItem('Studio:PotentiallyAuthenticated')).toBe(JSON.stringify({ OverallAppSignIn })); + expect(storedSignInMarkers()).toEqual({ OverallAppSignIn }); }); it('still wipes the caches and sessionStorage when the store teardown throws', () => { diff --git a/src/features/auth/queries/getCurrentUser.test.ts b/src/features/auth/queries/getCurrentUser.test.ts index 88785247d..d85e3c31e 100644 --- a/src/features/auth/queries/getCurrentUser.test.ts +++ b/src/features/auth/queries/getCurrentUser.test.ts @@ -1,30 +1,27 @@ // @vitest-environment jsdom import { authStore, OverallAppSignIn } from '@/features/auth/store/authStore'; -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { getCurrentUser } from './getCurrentUser'; -vi.mock('@/config/apiClient', () => ({ - apiClient: { - get: vi.fn().mockResolvedValue({ - data: { - id: 'usr-test', - email: 'admin@example.com', - fabricRole: 'fabric_admin', - roles: {}, - }, - }), - }, -})); +const { get } = vi.hoisted(() => ({ get: vi.fn() })); +vi.mock('@/config/apiClient', () => ({ apiClient: { get } })); + +const current = { id: 'usr-test', email: 'admin@example.com', fabricRole: 'fabric_admin', roles: {} }; + +function signIn(user: object) { + authStore.setUserForIdAndKey(OverallAppSignIn, OverallAppSignIn, user as never); +} + +beforeEach(() => { + get.mockResolvedValue({ data: current }); +}); describe('getCurrentUser', () => { it('syncs the fetched user into the auth store', async () => { // The /Login/ response seeds the store with a slimmer user (no // fabricRole); fetching /User/current must replace it so admin-only UI // appears without a page refresh. - authStore.setUserForIdAndKey(OverallAppSignIn, OverallAppSignIn, { - id: 'usr-test', - email: 'admin@example.com', - } as never); + signIn({ id: 'usr-test', email: 'admin@example.com' }); expect(authStore.getConnectionById(OverallAppSignIn).user).not.toHaveProperty('fabricRole'); const user = await getCurrentUser(); @@ -32,4 +29,21 @@ describe('getCurrentUser', () => { expect(user.fabricRole).toBe('fabric_admin'); expect(authStore.getConnectionById(OverallAppSignIn).user).toBe(user); }); + + it('leaves a sign-in made while it was in flight, whose session its answer may predate', async () => { + signIn({ id: 'usr-test' }); + let answer: (response: unknown) => void = () => undefined; + get.mockReturnValue( + new Promise((resolve) => { + answer = resolve; + }), + ); + const fetched = getCurrentUser(); + + signIn({ id: 'usr-other' }); + answer({ data: current }); + await fetched; + + expect(authStore.getConnectionById(OverallAppSignIn).user).toEqual({ id: 'usr-other' }); + }); }); diff --git a/src/features/auth/queries/getCurrentUser.ts b/src/features/auth/queries/getCurrentUser.ts index 1939e35f7..d0ffc88d8 100644 --- a/src/features/auth/queries/getCurrentUser.ts +++ b/src/features/auth/queries/getCurrentUser.ts @@ -1,22 +1,32 @@ import { apiClient } from '@/config/apiClient'; -// Circular with authStore.ts (it calls getCurrentUser); safe because neither +// Circular with authStore.ts (it calls fetchCurrentUser); safe because neither // module touches the other's exports at module-evaluation time. import { authStore, OverallAppSignIn } from '@/features/auth/store/authStore'; import { User } from '@/integrations/api/api.patch'; import { queryOptions } from '@tanstack/react-query'; -export async function getCurrentUser(): Promise { - const signOutGeneration = authStore.getSignOutGeneration(OverallAppSignIn); +/** The signed-in user, without syncing it into the auth store: the store's own check applies it itself. */ +export async function fetchCurrentUser(): Promise { const { data } = await apiClient.get('/User/current' as '/User/{id}'); // TODO: The roles the API returns is a map instead of an array, so we have to map through unknown. - const user = data as unknown as User; + return data as unknown as User; +} + +export async function getCurrentUser(): Promise { + const signOutGeneration = authStore.getSignOutGeneration(OverallAppSignIn); + const signInRevision = authStore.getSignInRevision(OverallAppSignIn); + const user = await fetchCurrentUser(); // The /Login/ response seeds the auth store with a slimmer user than // /User/current returns (it lacks fabricRole, among other fields), so sync // the authoritative record into the store whenever we fetch it — unless a - // sign-out landed while it was in flight. Resolved, not rejected, then: a - // rejection would reach the query's global error toast. - if (authStore.getSignOutGeneration(OverallAppSignIn) === signOutGeneration) { - authStore.setUserForEntity(OverallAppSignIn, user); + // sign-out landed while it was in flight, or a sign-in its answer may + // predate. Resolved, not rejected, then: a rejection would reach the + // query's global error toast. + if ( + authStore.getSignOutGeneration(OverallAppSignIn) === signOutGeneration + && authStore.getSignInRevision(OverallAppSignIn) === signInRevision + ) { + authStore.syncAppUser(user); } return user; } diff --git a/src/features/auth/store/appSignOutElsewhere.test.ts b/src/features/auth/store/appSignOutElsewhere.test.ts index 6f29eb750..bcfc7f55c 100644 --- a/src/features/auth/store/appSignOutElsewhere.test.ts +++ b/src/features/auth/store/appSignOutElsewhere.test.ts @@ -14,10 +14,12 @@ vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getIn import { apiClient } from '@/config/apiClient'; import { authStore, OverallAppSignIn } from './authStore'; +import { signInMarkerKey } from './signInMarkers'; +import { seedSignInMarkers, storedSignInMarkers } from './signInMarkersTestHelpers'; type SetArgs = Parameters; -const MARKER = 'Studio:PotentiallyAuthenticated'; +const MARKER = signInMarkerKey(OverallAppSignIn); const SIGNAL = 'Studio:AppSignOut'; const DIRECT_URL = 'https://ins-direct.example.com:9925/'; @@ -56,8 +58,8 @@ describe('authStore.onAppSignedOutElsewhere', () => { const signedOut = vi.fn(); const stop = authStore.onAppSignedOutElsewhere(signedOut); - storageEventFromAnotherTab(MARKER, JSON.stringify({ OverallAppSignIn })); - storageEventFromAnotherTab(MARKER, '{}'); + storageEventFromAnotherTab(MARKER, OverallAppSignIn); + storageEventFromAnotherTab(MARKER, null); storageEventFromAnotherTab('Studio:ExplorerAuthEpoch', JSON.stringify({ '*': 1 })); storageEventFromAnotherTab(null, null); storageEventFromAnotherTab(SIGNAL, signOutOf('usr_v')); @@ -220,7 +222,7 @@ describe('authStore.forgetAppSignInEndedElsewhere', () => { authStore.setUserForIdAndKey('ins-a', 'https://ins-a.example.com', { username: 'u' } as SetArgs[2]); // What the other tab wrote after signing out: a sign-in, perhaps as someone else. localStorage.clear(); - localStorage.setItem(MARKER, JSON.stringify({ OverallAppSignIn })); + seedSignInMarkers({ OverallAppSignIn }); const writes = [ vi.spyOn(Storage.prototype, 'setItem'), vi.spyOn(Storage.prototype, 'removeItem'), @@ -234,7 +236,7 @@ describe('authStore.forgetAppSignInEndedElsewhere', () => { expect(authStore.getOperationsUrl('ins-a')).toBeUndefined(); const localWrites = writes.flatMap((spy) => spy.mock.contexts).filter((storage) => storage === localStorage); expect(localWrites).toEqual([]); - expect(localStorage.getItem(MARKER)).toBe(JSON.stringify({ OverallAppSignIn })); + expect(storedSignInMarkers()).toEqual({ OverallAppSignIn }); }); it('drops a direct-connect token, and a sign-in check still in flight', async () => { diff --git a/src/features/auth/store/authStore.ts b/src/features/auth/store/authStore.ts index bddaedbae..ee612bfcd 100644 --- a/src/features/auth/store/authStore.ts +++ b/src/features/auth/store/authStore.ts @@ -1,6 +1,6 @@ import { isLocalStudio, localStudioDevUrl } from '@/config/constants'; import { getInstanceClient } from '@/config/getInstanceClient'; -import { getCurrentUser } from '@/features/auth/queries/getCurrentUser'; +import { fetchCurrentUser } from '@/features/auth/queries/getCurrentUser'; import { forgetAllEntitySettings, forgetEntitySettings, @@ -22,6 +22,15 @@ import { isInstance } from '@/lib/types/isInstance'; import { getOperationsUrlForCluster } from '@/lib/urls/getOperationsUrlForCluster'; import { getOperationsUrlForInstance } from '@/lib/urls/getOperationsUrlForInstance'; import { isDirectOperationsUrl } from '@/lib/urls/isDirectOperationsUrl'; +import { + legacyMigratedKey, + markLegacySignInMarkersMigrated, + migrateLegacySignInMarkers, + mirrorLegacySignInMarker, + readLegacySignInMarkers, + readSignInMarkers, + signInMarkerKey, +} from './signInMarkers'; type AuthStoreListenerCleanup = () => void; @@ -61,13 +70,18 @@ class AuthStore { Array<(connection: AuthenticatedConnection, id: EntityIds) => void> > = {}; - private readonly potentiallyAuthenticatedKey = 'Studio:PotentiallyAuthenticated'; private readonly basicAuthKeyPrefix = 'Studio:BasicAuth:'; private readonly fabricConnectKeyPrefix = 'Studio:FabricConnect:'; private readonly lastConnectModeKeyPrefix = 'Studio:LastConnectMode:'; // Storage can refuse a removal yet still answer reads, so a credential it kept belongs to the session // that ended. This tab reads such a key as absent until a write replaces it. private readonly unremovedKeys = new Set(); + // Sign-ins storage refused to record, which the next sign-in or check of them writes again. + private readonly unrecordedSignIns = new Set(); + // Markers storage refused to remove, by key, with the value they held then (null when it couldn't be read). + private readonly unremovedSignInMarkers = new Map(); + // Bumped by every sign-in, so a check can tell its answer is about one that has since been replaced. + private readonly signInRevisions = new Map(); private readonly potentiallyAuthenticated: Record; private readonly checkedAuthentication: Record = {}; private readonly allConnections: Record = {}; @@ -111,7 +125,9 @@ class AuthStore { private signOutHeardDuringCheck: string | null = null; constructor() { - this.potentiallyAuthenticated = JSON.parse(localStorage.getItem(this.potentiallyAuthenticatedKey) || '{}'); + // Until a migration lands (a full quota refuses one), the previous release's record still holds the sign-ins. + const unmigrated = migrateLegacySignInMarkers() ? {} : readLegacySignInMarkers(); + this.potentiallyAuthenticated = { ...unmigrated, ...Object.fromEntries(readSignInMarkers()) }; } private readExplorerGenerations(): Record { @@ -185,12 +201,13 @@ class AuthStore { /** * Keeps the explorer's sign-out generation: a tab that missed the sign-out compares its stored explorer - * credential against it when it reloads, so removing it would make that credential current again. Then, - * with room made, writes what a full store refused. + * credential against it when it reloads, so removing it would make that credential current again. Keeps the + * sign-in markers' migration flag too (`signInMarkers.ts`). Then, with room made, writes what a full store refused. */ public wipeLocalStorageForSignOut(): void { try { - clearLocalStorage([this.explorerAuthEpochKey]); + clearLocalStorage([this.explorerAuthEpochKey, legacyMigratedKey]); + markLegacySignInMarkersMigrated(); } finally { this.retrySignOutWritesAfterWipe(); } @@ -354,6 +371,7 @@ class AuthStore { user: AuthenticatedConnection['user'], ): void { if (user) { + this.signInRevisions.set(id, (this.signInRevisions.get(id) ?? 0) + 1); this.flagKeyAsSignedIn(id, key); this.updateConnectionIfChanged(id, false, user); } else { @@ -363,6 +381,15 @@ class AuthStore { } } + /** + * A `/User/current` answer: the current session's fuller record, not a new sign-in. So, like a check, it writes no + * marker this tab already holds and leaves the sign-in revision alone. + */ + public syncAppUser(user: User): void { + this.flagKeyAsSignedIn(OverallAppSignIn, OverallAppSignIn, true); + this.updateConnectionIfChanged(OverallAppSignIn, false, user); + } + public updateUserForEntity(entity: EntityTypes, changes: Partial): void { const id = this.calculateIdFromEntity(entity); if (!id) { @@ -451,6 +478,11 @@ class AuthStore { return value === 'fabric' || value === 'direct' ? value : undefined; } + /** Advances at every sign-in, so an answer fetched before one can tell it is about a session that ended. */ + public getSignInRevision(id: EntityIds): number { + return this.signInRevisions.get(id) ?? 0; + } + /** * Moves on every sign-out of `id`, and of everything. An auth resolution reads it before its first * await and commits only if it has not moved, so a result that outlived its session is dropped. @@ -694,35 +726,31 @@ class AuthStore { } public async signOutFromPotentiallyAuthenticatedInstances() { - // Dooms connects for entities not yet in `potentiallyAuthenticated`, which the loop skips. + // Dooms connects for entities not yet marked, which the loop skips. this.bumpSignOutGeneration('*'); forgetAllEntitySettings(); - const logouts: Array> = []; - for (const entityId of Object.keys(this.potentiallyAuthenticated)) { - // The clears below drop the operations URL and token the client resolves from. - const instanceClient = this.buildLogoutClient(entityId); + const signIns = this.rememberedSignIns(); + // Built first: the clears below drop the credentials and token they are built with. + const clients = signIns.map(([entityId, operationsUrl]) => this.buildLogoutClient(entityId, operationsUrl)); + for (const entityId of new Set(signIns.map(([entityId]) => entityId))) { this.signOutLocally(entityId); - if (instanceClient) { - logouts.push( - onInstanceLogoutSubmit({ entityId, instanceClient }).then( - () => undefined, - (err: unknown) => reportLogoutFailure(entityId, err), - ), - ); - } } + const logouts = signIns.flatMap(([entityId], index) => { + const instanceClient = clients[index]; + return instanceClient ? [this.sendBestEffortLogout(entityId, instanceClient)] : []; + }); // After the logout clients are built, which carry their credential in a header of their own. A // failed CM logout skips `signOutAllLocally`, so this is the only clear those connects get. this.dropFabricConnectState(); await Promise.all(logouts); } - private buildLogoutClient(entityId: EntityIds): ReturnType | null { + private buildLogoutClient(entityId: EntityIds, operationsUrl?: string): ReturnType | null { if (entityId === OverallAppSignIn) { return null; } try { - const instanceClient = getInstanceClient({ id: entityId }); + const instanceClient = getInstanceClient({ id: entityId, operationsUrl }); 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(); @@ -761,6 +789,67 @@ class AuthStore { } } + /** For a server that outlives Studio's record of it. Never rejects. */ + public async signOutEntities(ids: Iterable): Promise { + const unique = [...new Set(ids)]; + const signIns = this.rememberedSignIns(); + const logouts = unique.flatMap((entityId) => + signIns.filter(([id]) => id === entityId).map(([, operationsUrl]) => ({ + entityId, + instanceClient: this.buildLogoutClient(entityId, operationsUrl), + })) + ); + this.signOutEntitiesLocally(unique); + await Promise.all( + logouts.map(({ entityId, instanceClient }) => + instanceClient ? this.sendBestEffortLogout(entityId, instanceClient) : Promise.resolve() + ), + ); + } + + /** Sent after the caller has forgotten `entityId`, so whatever is recorded for it by the answer, in any tab, is newer. */ + private sendBestEffortLogout( + entityId: EntityIds, + instanceClient: ReturnType, + ): Promise { + const sentWith = this.signInSnapshot(entityId); + const signedInSince = () => this.signInSnapshot(entityId) !== sentWith; + return onInstanceLogoutSubmit({ entityId, instanceClient, signedInSince }).then( + () => undefined, + (err: unknown) => reportLogoutFailure(entityId, err), + ); + } + + /** + * This tab's sign-ins, every stored one, and every one a previous-release tab recorded, once per URL: another tab + * may have signed in again at a newer URL, and the older one's session is still open. A sweep that reaches more + * only signs more out. + */ + private rememberedSignIns(): Array<[EntityIds, AuthenticatedConnectionKey]> { + const signIns = [ + ...Object.entries(this.potentiallyAuthenticated), + ...readSignInMarkers(), + ...Object.entries(readLegacySignInMarkers() ?? {}), + ]; + return signIns.filter(([id, key], index) => signIns.findIndex(([i, k]) => i === id && k === key) === index); + } + + /** Read raw, not through `readStoredItem`, because another tab may have written them. */ + private signInSnapshot(id: EntityIds): string { + const stored = (key: string) => { + try { + return localStorage.getItem(key); + } catch { + return null; + } + }; + return JSON.stringify([ + stored(this.basicAuthKeyPrefix + id), + stored(this.fabricConnectKeyPrefix + id), + this.getConnectionGeneration(id), + ]); + } + private clearSignIn(id: EntityIds): void { this.bumpSignOutGeneration(id); // That attempt will now reject; the next connect for this id must not join it. @@ -798,7 +887,7 @@ class AuthStore { this.bumpSignOutGeneration('*'); forgetAllEntitySettings(); // Snapshot keys first — signOutLocally mutates the flags as it goes. - for (const id of Object.keys(this.potentiallyAuthenticated)) { + for (const id of new Set(this.rememberedSignIns().map(([id]) => id))) { this.signOutLocally(id); } this.dropFabricConnectState(); @@ -895,25 +984,77 @@ class AuthStore { void this.updateListeners(id); } - private flagKeyAsSignedIn(id: EntityIds, key: AuthenticatedConnectionKey) { - if (this.potentiallyAuthenticated[id] !== key) { - this.potentiallyAuthenticated[id] = key; - localStorage.setItem(this.potentiallyAuthenticatedKey, JSON.stringify(this.potentiallyAuthenticated)); + /** + * Other tabs, and the next load, read a marker this tab couldn't remove, so every marker write tries again. One + * rewritten since is a newer sign-in, and is left alone. + */ + private retryUnremovedSignInMarkers(): void { + for (const [key, value] of this.unremovedSignInMarkers) { + try { + if (value === null || localStorage.getItem(key) === value) { + localStorage.removeItem(key); + } + this.unremovedSignInMarkers.delete(key); + this.unremovedKeys.delete(key); + } catch { + // Storage still refuses, so the entry stays queued. + } + } + } + + /** + * `checked`: a check's passing answer for a sign-in this tab already marked writes nothing, so it can't put back a + * marker another tab's sign-out removed. A sign-in itself always writes. + */ + private flagKeyAsSignedIn(id: EntityIds, key: AuthenticatedConnectionKey, checked = false) { + if (checked && this.potentiallyAuthenticated[id] === key && !this.unrecordedSignIns.has(id)) { + return; } + this.potentiallyAuthenticated[id] = key; + const markerKey = signInMarkerKey(id); + // A marker storage refused to remove that already holds this URL is this sign-in's, even if the write below fails. + if (this.unremovedSignInMarkers.get(markerKey) === key) { + this.unremovedSignInMarkers.delete(markerKey); + } + this.retryUnremovedSignInMarkers(); + try { + this.writeStoredItem(markerKey, key); + this.unremovedSignInMarkers.delete(markerKey); + this.unrecordedSignIns.delete(id); + } catch (error) { + // Not thrown: the sign-in itself worked, and unrecorded it only looks signed out on the next load. + this.unrecordedSignIns.add(id); + console.debug('Could not record a sign-in', error); + } + mirrorLegacySignInMarker(id, key); } - private flagKeyAsSignedOut(id: EntityIds) { - if (this.potentiallyAuthenticated[id]) { - delete this.potentiallyAuthenticated[id]; - try { - localStorage.setItem(this.potentiallyAuthenticatedKey, JSON.stringify(this.potentiallyAuthenticated)); - } catch { - // Fail closed. A marker left behind makes the next page load probe this entity with its - // cookies, and a session that outlived the sign-out would sign it back in; losing every - // marker only makes the other entities look signed out on that load. - this.removeStoredItem(this.potentiallyAuthenticatedKey); + /** `checkedKey`: the URL a failed check used, which says nothing about a newer one another tab stored. */ + private flagKeyAsSignedOut(id: EntityIds, checkedKey?: AuthenticatedConnectionKey) { + delete this.potentiallyAuthenticated[id]; + this.unrecordedSignIns.delete(id); + this.retryUnremovedSignInMarkers(); + const markerKey = signInMarkerKey(id); + let stored: string | null = null; + let unreadable = false; + try { + stored = localStorage.getItem(markerKey); + } catch { + unreadable = true; + } + // A failed check is a probe, not a sign-out: it removes only the marker it checked. + if (checkedKey !== undefined && (unreadable || (stored && stored !== checkedKey))) { + return; + } + if (stored || unreadable) { + // Blanks or hides a marker storage won't remove: one left behind makes the next page load probe this + // entity with its cookies, and a session that outlived the sign-out would sign it back in. + this.removeStoredItem(markerKey); + if (this.unremovedKeys.has(markerKey)) { + this.unremovedSignInMarkers.set(markerKey, stored); } } + mirrorLegacySignInMarker(id, null, checkedKey); } private async updateListeners(id: EntityIds): Promise { @@ -972,6 +1113,7 @@ class AuthStore { private async loadUser(id: EntityIds): Promise { const key = this.potentiallyAuthenticated[id]; const signOutGeneration = this.getSignOutGeneration(id); + const signInRevision = this.signInRevisions.get(id); this.updateConnectionIfChanged(id, true, null); let user: AuthenticatedConnection['user'] = null; try { @@ -979,7 +1121,7 @@ class AuthStore { if (isLocalStudio) { user = await getInstanceUserInfo({ instanceClient: getInstanceClient() }); } else { - user = await getCurrentUser(); + user = await fetchCurrentUser(); } } else if (id) { user = await getInstanceUserInfo({ instanceClient: getInstanceClient({ id }) }); @@ -996,10 +1138,13 @@ class AuthStore { if (this.getSignOutGeneration(id) !== signOutGeneration) { return null; } + if (this.signInRevisions.get(id) !== signInRevision) { + return this.getConnectionById(id).user; + } if (user) { - this.flagKeyAsSignedIn(id, key); + this.flagKeyAsSignedIn(id, key, true); } else { - this.flagKeyAsSignedOut(id); + this.flagKeyAsSignedOut(id, key); } this.updateConnectionIfChanged(id, false, user); if (signOutHeard !== null && signOutHeard === this.appUser()) { diff --git a/src/features/auth/store/crossTabMarkers.test.ts b/src/features/auth/store/crossTabMarkers.test.ts new file mode 100644 index 000000000..36f037dd4 --- /dev/null +++ b/src/features/auth/store/crossTabMarkers.test.ts @@ -0,0 +1,319 @@ +/** @vitest-environment jsdom */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { authStore, OverallAppSignIn } from './authStore'; +import { legacySignInMarkersKey, signInMarkerKey } from './signInMarkers'; +import { seedSignInMarkers, storedSignInMarkers } from './signInMarkersTestHelpers'; + +const { fetchCurrentUser, getInstanceUserInfo, onInstanceLogoutSubmit } = vi.hoisted(() => ({ + fetchCurrentUser: vi.fn(), + getInstanceUserInfo: vi.fn(), + onInstanceLogoutSubmit: vi.fn(), +})); +vi.mock('@/features/auth/queries/getCurrentUser', () => ({ fetchCurrentUser })); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getInstanceUserInfo })); +vi.mock('@/integrations/api/instance/auth/onInstanceLogoutSubmit', () => ({ onInstanceLogoutSubmit })); + +type SetArgs = Parameters; +const ENTITIES = ['ins-here', 'ins-other-tab']; +const url = (id: string) => `https://${id}.example.com:9925/`; +const newer = 'https://ins-here-2.example.com:9925/'; + +function signInHere(id: string, at = url(id)) { + authStore.setUserForIdAndKey(id as SetArgs[0], at as SetArgs[1], { username: 'admin' } as SetArgs[2]); +} + +function signInInAnotherTab(id: string) { + seedSignInMarkers({ [id]: url(id) }); + localStorage.setItem(`Studio:BasicAuth:${id}`, btoa(JSON.stringify({ username: 'admin', password: 'hunter2' }))); +} + +function loggedOut() { + return onInstanceLogoutSubmit.mock.calls.map(([{ entityId, instanceClient }]) => [ + entityId, + instanceClient.defaults.baseURL, + ]); +} + +beforeEach(() => { + localStorage.clear(); + onInstanceLogoutSubmit.mockResolvedValue({ message: 'ok' }); + vi.spyOn(console, 'debug').mockImplementation(() => undefined); +}); + +afterEach(() => { + vi.resetAllMocks(); + vi.restoreAllMocks(); + [...ENTITIES, OverallAppSignIn].forEach((id) => authStore.signOutLocally(id)); + localStorage.clear(); +}); + +describe('sign-in markers another tab added', () => { + it('survive a sign-in in this tab', () => { + signInInAnotherTab('ins-other-tab'); + + signInHere('ins-here'); + + expect(storedSignInMarkers()).toEqual({ 'ins-other-tab': url('ins-other-tab'), 'ins-here': url('ins-here') }); + }); + + it('survive a sign-out in this tab', () => { + signInHere('ins-here'); + signInInAnotherTab('ins-other-tab'); + + authStore.signOutLocally('ins-here'); + + expect(storedSignInMarkers()).toEqual({ 'ins-other-tab': url('ins-other-tab') }); + }); + + it('go when this tab signs the same entity out', () => { + signInInAnotherTab('ins-other-tab'); + + authStore.signOutLocally('ins-other-tab'); + + expect(storedSignInMarkers()).toEqual({}); + }); + + it('get a logout, and go, at an app sign-out from this tab', async () => { + signInHere('ins-here'); + signInInAnotherTab('ins-other-tab'); + + await authStore.signOutFromPotentiallyAuthenticatedInstances(); + + expect(loggedOut()).toEqual([['ins-here', url('ins-here')], ['ins-other-tab', url('ins-other-tab')]]); + expect(storedSignInMarkers()).toEqual({}); + expect(authStore.checkForBasicAuth('ins-other-tab')).toBeUndefined(); + }); + + it('go, with their credentials, at a local full sign-out', () => { + signInInAnotherTab('ins-other-tab'); + + authStore.signOutAllLocally(); + + expect(storedSignInMarkers()).toEqual({}); + expect(authStore.checkForBasicAuth('ins-other-tab')).toBeUndefined(); + }); +}); + +describe("a sign-in's marker", () => { + it('is the only one its sign-in writes, beside the legacy record', () => { + signInInAnotherTab('ins-other-tab'); + const setItem = vi.spyOn(Storage.prototype, 'setItem'); + + signInHere('ins-here'); + + expect(setItem.mock.calls.map(([key]) => key)).toEqual([signInMarkerKey('ins-here'), legacySignInMarkersKey]); + }); + + it('is the only one its sign-out removes', () => { + signInHere('ins-here'); + signInInAnotherTab('ins-other-tab'); + const removeItem = vi.spyOn(Storage.prototype, 'removeItem'); + + authStore.signOutLocally('ins-here'); + + expect(removeItem.mock.calls.map(([key]) => key)).not.toContain(signInMarkerKey('ins-other-tab')); + }); +}); + +describe('a sign-in this tab already remembers', () => { + it('records itself again when another tab signed it out', () => { + signInHere('ins-here'); + localStorage.removeItem(signInMarkerKey('ins-here')); + + signInHere('ins-here'); + + expect(storedSignInMarkers()).toEqual({ 'ins-here': url('ins-here') }); + }); +}); + +describe('the app sign-in', () => { + const signInApp = (user: object) => + authStore.setUserForIdAndKey(OverallAppSignIn, OverallAppSignIn, user as SetArgs[2]); + + it("isn't put back by a /User/current answer after another tab's failed check dropped it", () => { + signInApp({ id: 'usr_a' }); + localStorage.removeItem(signInMarkerKey(OverallAppSignIn)); + + authStore.syncAppUser({ id: 'usr_a' } as never); + + expect(storedSignInMarkers()).toEqual({}); + }); + + it('records an explicit sign-in again, even while this tab still holds the marker', () => { + signInApp({ id: 'usr_a' }); + localStorage.removeItem(signInMarkerKey(OverallAppSignIn)); + + signInApp({ id: 'usr_a' }); + + expect(storedSignInMarkers()).toEqual({ [OverallAppSignIn]: OverallAppSignIn }); + }); + + it('still applies its own answer when a /User/current answer lands while it is out', async () => { + signInApp({ id: 'usr_a' }); + let answer: (user: unknown) => void = () => undefined; + fetchCurrentUser.mockReturnValue( + new Promise((resolve) => { + answer = resolve; + }), + ); + const check = authStore.reloadUser(OverallAppSignIn); + const checked = { id: 'usr_a', checked: true }; + + authStore.syncAppUser({ id: 'usr_a', synced: true } as never); + answer(checked); + await check; + + expect(authStore.getConnectionById(OverallAppSignIn).user).toBe(checked); + }); + + it('stays signed in when a check that was out during a new sign-in fails', async () => { + signInApp({ id: 'usr_a' }); + let answer: (reason: unknown) => void = () => undefined; + fetchCurrentUser.mockReturnValue( + new Promise((_, reject) => { + answer = reject; + }), + ); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + const check = authStore.reloadUser(OverallAppSignIn); + + signInApp({ id: 'usr_b' }); + answer(new Error('timeout of 60000ms exceeded')); + await check; + + expect(authStore.getConnectionById(OverallAppSignIn).user).toEqual({ id: 'usr_b' }); + expect(storedSignInMarkers()).toHaveProperty(OverallAppSignIn); + }); + + it("drops a passing check's answer about the session a sign-in made meanwhile replaced", async () => { + signInApp({ id: 'usr_a' }); + let answer: (user: unknown) => void = () => undefined; + fetchCurrentUser.mockReturnValue( + new Promise((resolve) => { + answer = resolve; + }), + ); + const check = authStore.reloadUser(OverallAppSignIn); + + signInApp({ id: 'usr_b' }); + answer({ id: 'usr_a' }); + await check; + + expect(authStore.getConnectionById(OverallAppSignIn).user).toEqual({ id: 'usr_b' }); + }); +}); + +describe('a sign-in another tab made again at a newer URL', () => { + function loggedOutAt() { + return onInstanceLogoutSubmit.mock.calls.map(([{ instanceClient }]) => instanceClient.defaults.baseURL); + } + + beforeEach(() => { + signInHere('ins-here'); + seedSignInMarkers({ 'ins-here': newer }); + }); + + it('gets a logout at both URLs at an app sign-out', async () => { + await authStore.signOutFromPotentiallyAuthenticatedInstances(); + + expect(loggedOutAt()).toEqual([url('ins-here'), newer]); + }); + + it('gets a logout at both URLs from signOutEntities', async () => { + await authStore.signOutEntities(['ins-here']); + + expect(loggedOutAt()).toEqual([url('ins-here'), newer]); + }); +}); + +describe('a check of a sign-in this tab remembers', () => { + beforeEach(() => { + getInstanceUserInfo.mockResolvedValue({ username: 'admin' }); + }); + + it('leaves out, when it passes, the marker of another tab that signed it out', async () => { + signInHere('ins-here'); + localStorage.removeItem(signInMarkerKey('ins-here')); + + await authStore.reloadUser('ins-here'); + + expect(storedSignInMarkers()).toEqual({}); + }); + + it('keeps, when it passes, the operations URL another tab has signed in with since', async () => { + signInHere('ins-here'); + seedSignInMarkers({ 'ins-here': newer }); + + await authStore.reloadUser('ins-here'); + + expect(storedSignInMarkers()).toEqual({ 'ins-here': newer }); + }); + + it("keeps that newer URL when it fails against this tab's older one", async () => { + signInHere('ins-here'); + seedSignInMarkers({ 'ins-here': newer }); + getInstanceUserInfo.mockRejectedValue(new Error('Network Error')); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + + await authStore.reloadUser('ins-here'); + + expect(storedSignInMarkers()).toEqual({ 'ins-here': newer }); + }); + + it('leaves a sign-in this tab made at another URL while it was out', async () => { + signInHere('ins-here'); + let answer: (reason: unknown) => void = () => undefined; + getInstanceUserInfo.mockReturnValue( + new Promise((_, reject) => { + answer = reject; + }), + ); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + const check = authStore.reloadUser('ins-here'); + + signInHere('ins-here', newer); + answer(new Error('Network Error')); + await check; + + expect(authStore.getOperationsUrl('ins-here')).toBe(newer); + expect(authStore.getConnectionById('ins-here').user).toEqual({ username: 'admin' }); + expect(storedSignInMarkers()).toEqual({ 'ins-here': newer }); + }); + + it("leaves the marker alone when it fails and can't read it", async () => { + signInHere('ins-here'); + getInstanceUserInfo.mockRejectedValue(new Error('Network Error')); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + const getItem = Storage.prototype.getItem; + vi.spyOn(Storage.prototype, 'getItem').mockImplementation(function(this: Storage, key: string) { + if (key === signInMarkerKey('ins-here')) { + throw new DOMException('Storage refused.', 'SecurityError'); + } + return getItem.call(this, key); + }); + + await authStore.reloadUser('ins-here'); + vi.mocked(Storage.prototype.getItem).mockRestore(); + + expect(storedSignInMarkers()).toEqual({ 'ins-here': url('ins-here') }); + }); + + it('leaves a sign-in this tab made at the same URL while it was out', async () => { + signInHere('ins-here'); + let answer: (reason: unknown) => void = () => undefined; + getInstanceUserInfo.mockReturnValue( + new Promise((_, reject) => { + answer = reject; + }), + ); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + const check = authStore.reloadUser('ins-here'); + + signInHere('ins-here'); + answer(new Error('Network Error')); + await check; + + expect(authStore.getConnectionById('ins-here').user).toEqual({ username: 'admin' }); + expect(storedSignInMarkers()).toEqual({ 'ins-here': url('ins-here') }); + }); +}); diff --git a/src/features/auth/store/entityAuthCheckScope.test.ts b/src/features/auth/store/entityAuthCheckScope.test.ts index 1cb3382aa..4333c2fa7 100644 --- a/src/features/auth/store/entityAuthCheckScope.test.ts +++ b/src/features/auth/store/entityAuthCheckScope.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; // Transformed here, while the file is collected, which has no timeout, rather than inside the first test's 5s. // Each test still evaluates a fresh copy. import '@/features/instance/instanceLayoutRoute'; +import { seedSignInMarkers, storedSignInMarkers } from './signInMarkersTestHelpers'; const { get, getInstanceUserInfo, createInstanceAuthenticationTokens, onInstanceLogoutSubmit, routeParams } = vi .hoisted(() => ({ @@ -54,14 +55,11 @@ function guardParams(clusterId: string) { beforeEach(() => { localStorage.clear(); sessionStorage.clear(); - localStorage.setItem( - 'Studio:PotentiallyAuthenticated', - JSON.stringify({ - OverallAppSignIn: 'OverallAppSignIn', - [OTHER_ORG_CLUSTER]: OTHER_ORG_URL, - [THIS_ORG_CLUSTER]: THIS_ORG_URL, - }), - ); + seedSignInMarkers({ + OverallAppSignIn: 'OverallAppSignIn', + [OTHER_ORG_CLUSTER]: OTHER_ORG_URL, + [THIS_ORG_CLUSTER]: THIS_ORG_URL, + }); get.mockResolvedValue({ data: { id: 'cloud-user' } }); getInstanceUserInfo.mockResolvedValue({ username: 'admin' }); onInstanceLogoutSubmit.mockResolvedValue({ message: 'ok' }); @@ -137,7 +135,7 @@ describe('which remembered sign-ins a page load checks (#1329)', () => { expect(onInstanceLogoutSubmit.mock.calls.map(([{ entityId }]) => entityId).sort()) .toEqual([OTHER_ORG_CLUSTER, THIS_ORG_CLUSTER]); - expect(localStorage.getItem('Studio:PotentiallyAuthenticated')).toBe('{}'); + expect(storedSignInMarkers()).toEqual({}); }); it('checks the entity of the route a component is on when it names none', async () => { diff --git a/src/features/auth/store/isKnownSignedOut.test.ts b/src/features/auth/store/isKnownSignedOut.test.ts index 8686115b1..42f56850f 100644 --- a/src/features/auth/store/isKnownSignedOut.test.ts +++ b/src/features/auth/store/isKnownSignedOut.test.ts @@ -1,6 +1,7 @@ /** @vitest-environment jsdom */ // jsdom: authStore touches localStorage at module load and throughout. import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { seedSignInMarkers } from './signInMarkersTestHelpers'; const user = { id: 'usr_a', email: 'a@example.com', roles: {} }; @@ -26,7 +27,7 @@ describe('authStore.isKnownSignedOut', () => { }); it('knows at once when a sign-out clears the session', async () => { - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'OverallAppSignIn' })); + seedSignInMarkers({ OverallAppSignIn: 'OverallAppSignIn' }); const { authStore, OverallAppSignIn } = await freshStore(); authStore.setUserForIdAndKey(OverallAppSignIn, OverallAppSignIn, user as never); diff --git a/src/features/auth/store/lateLogoutAnswer.test.ts b/src/features/auth/store/lateLogoutAnswer.test.ts new file mode 100644 index 000000000..b8f714912 --- /dev/null +++ b/src/features/auth/store/lateLogoutAnswer.test.ts @@ -0,0 +1,96 @@ +/** @vitest-environment jsdom */ +import axios, { type AxiosAdapter, type AxiosResponse } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { authStore } from './authStore'; + +const { createInstanceAuthenticationTokens, getInstanceUserInfo } = vi.hoisted(() => ({ + createInstanceAuthenticationTokens: vi.fn(), + getInstanceUserInfo: vi.fn(), +})); +vi.mock('@/integrations/api/instance/auth/createInstanceAuthenticationTokens', () => ({ + createInstanceAuthenticationTokens, + refreshInstanceOperationToken: vi.fn(), +})); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getInstanceUserInfo })); + +type SetArgs = Parameters; +const ID = 'ins-late'; +const URL = 'https://ins-late.example.com:9925/'; +const OLD = { username: 'admin', password: 'old' }; +const NEW = { username: 'admin', password: 'new' }; +const defaultAdapter = axios.defaults.adapter; +let answerLogout: (() => void) | undefined; + +beforeEach(() => { + localStorage.clear(); + answerLogout = undefined; + axios.defaults.adapter = ((config) => + new Promise((resolve) => { + answerLogout = () => resolve({ status: 200, statusText: 'OK', data: { message: 'ok' }, headers: {}, config }); + })) as AxiosAdapter; + authStore.setUserForIdAndKey(ID, URL as SetArgs[1], { username: 'admin' } as SetArgs[2]); + authStore.flagForBasicAuth(ID, OLD); + vi.spyOn(console, 'debug').mockImplementation(() => undefined); +}); + +afterEach(() => { + axios.defaults.adapter = defaultAdapter; + vi.resetAllMocks(); + vi.restoreAllMocks(); + authStore.signOutLocally(ID); +}); + +describe.each([ + { path: 'an app sign-out', signOut: () => authStore.signOutFromPotentiallyAuthenticatedInstances() }, + { path: 'removing its cluster', signOut: () => authStore.signOutEntities([ID]) }, +])('a logout from $path that answers after a new sign-in', ({ signOut }) => { + async function signInWhileTheLogoutIsOut(signIn: () => Promise | void) { + const done = signOut(); + await vi.waitFor(() => expect(answerLogout).toBeDefined()); + await signIn(); + answerLogout!(); + await done; + } + + it('keeps the new Basic Auth credential', async () => { + await signInWhileTheLogoutIsOut(() => authStore.flagForBasicAuth(ID, NEW)); + + expect(authStore.checkForBasicAuth(ID)).toEqual(NEW); + }); + + it('keeps a Basic Auth credential another tab stored', async () => { + await signInWhileTheLogoutIsOut(() => localStorage.setItem(`Studio:BasicAuth:${ID}`, btoa(JSON.stringify(NEW)))); + + expect(authStore.checkForBasicAuth(ID)).toEqual(NEW); + }); + + it('clears again on the answer when nothing replaced the sign-in', async () => { + const flagForBasicAuth = vi.spyOn(authStore, 'flagForBasicAuth'); + + await signInWhileTheLogoutIsOut(() => undefined); + + expect(flagForBasicAuth.mock.calls).toEqual([[ID, null], [ID, null]]); + }); + + it('keeps the new Fabric Connect flag', async () => { + await signInWhileTheLogoutIsOut(() => authStore.flagForFabricConnect(ID, true)); + + expect(authStore.checkForFabricConnect(ID)).toBe(true); + }); + + it('keeps the token of a Fabric Connect that is still verifying when the answer lands', async () => { + createInstanceAuthenticationTokens.mockResolvedValue({ operationToken: 'new-token' }); + let verify: (user: unknown) => void = () => undefined; + getInstanceUserInfo.mockReturnValue(new Promise((resolve) => (verify = resolve))); + let connect: Promise = Promise.resolve(); + + await signInWhileTheLogoutIsOut(async () => { + connect = authStore.establishFabricConnectAuth({ id: ID, operationsUrl: URL }); + await vi.waitFor(() => expect(getInstanceUserInfo).toHaveBeenCalled()); + }); + verify({ username: 'admin' }); + await connect; + + expect(authStore.getOperationToken(ID)).toBe('new-token'); + }); +}); diff --git a/src/features/auth/store/legacySignInMarkers.test.ts b/src/features/auth/store/legacySignInMarkers.test.ts new file mode 100644 index 000000000..c87a363e8 --- /dev/null +++ b/src/features/auth/store/legacySignInMarkers.test.ts @@ -0,0 +1,319 @@ +/** @vitest-environment jsdom */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { legacySignInMarkersKey, migrateLegacySignInMarkers } from './signInMarkers'; +import { seedSignInMarkers, storedSignInMarkers } from './signInMarkersTestHelpers'; + +const { getInstanceUserInfo, onInstanceLogoutSubmit } = vi.hoisted(() => ({ + getInstanceUserInfo: vi.fn(), + onInstanceLogoutSubmit: vi.fn(), +})); +vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getInstanceUserInfo })); +vi.mock('@/integrations/api/instance/auth/onInstanceLogoutSubmit', () => ({ onInstanceLogoutSubmit })); + +type Store = Awaited>; +type SetArgs = Parameters; +const url = (id: string) => `https://${id}.example.com:9925/`; + +/** The store reads its markers at construction, so each load is a fresh one. */ +async function loadPage() { + vi.resetModules(); + return (await import('./authStore')).authStore; +} + +function legacy(): unknown { + const raw = localStorage.getItem(legacySignInMarkersKey); + return raw === null ? null : JSON.parse(raw); +} + +// What a tab on the previous release writes: its whole load-time copy, every time. +function writeFromPreviousRelease(markers: Record) { + localStorage.setItem(legacySignInMarkersKey, JSON.stringify(markers)); +} + +function signIn(store: Store, id: string) { + store.setUserForIdAndKey(id as SetArgs[0], url(id) as SetArgs[1], { username: 'admin' } as SetArgs[2]); +} + +beforeEach(() => { + localStorage.clear(); + onInstanceLogoutSubmit.mockResolvedValue({ message: 'ok' }); + vi.spyOn(console, 'debug').mockImplementation(() => undefined); +}); + +afterEach(() => { + vi.resetAllMocks(); + vi.restoreAllMocks(); +}); + +describe('the first load of this release', () => { + it("moves the previous release's markers into one key per entity", async () => { + writeFromPreviousRelease({ OverallAppSignIn: 'OverallAppSignIn', 'ins-a': url('ins-a') }); + + const store = await loadPage(); + + expect(storedSignInMarkers()).toEqual({ OverallAppSignIn: 'OverallAppSignIn', 'ins-a': url('ins-a') }); + expect(store.getOperationsUrl('ins-a')).toBe(url('ins-a')); + }); + + it('keeps a marker already stored for an entity over the legacy one', async () => { + seedSignInMarkers({ 'ins-a': 'https://ins-a-2.example.com:9925/' }); + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + + await loadPage(); + + expect(storedSignInMarkers()).toEqual({ 'ins-a': 'https://ins-a-2.example.com:9925/' }); + }); + + it('writes the same markers when another tab migrates at the same time', () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a'), 'ins-b': url('ins-b') }); + + migrateLegacySignInMarkers(); + const once = storedSignInMarkers(); + localStorage.removeItem('Studio:PotentiallyAuthenticatedMigrated'); + migrateLegacySignInMarkers(); + + expect(storedSignInMarkers()).toEqual(once); + }); + + it.each([ + ['an array', JSON.stringify([url('ins-a')])], + ['not JSON', 'ins-a'], + ['null', 'null'], + ])('removes a legacy record that is %s, and moves nothing', async (_, junk) => { + localStorage.setItem(legacySignInMarkersKey, junk); + + await loadPage(); + + expect(localStorage.getItem(legacySignInMarkersKey)).toBeNull(); + expect(storedSignInMarkers()).toEqual({}); + }); + + it("drops entries that aren't markers from the legacy record", async () => { + localStorage.setItem(legacySignInMarkersKey, JSON.stringify({ 'ins-a': url('ins-a'), 'ins-b': 123, 'ins-c': '' })); + + await loadPage(); + + expect(legacy()).toEqual({ 'ins-a': url('ins-a') }); + }); + + it("still signs in on the previous release's markers when storage refuses the migration", async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { + throw new DOMException('The quota has been exceeded.', 'QuotaExceededError'); + }); + + const store = await loadPage(); + + expect(store.getOperationsUrl('ins-a')).toBe(url('ins-a')); + }); +}); + +describe('a later load', () => { + it('brings back nothing a previous-release tab wrote back after this release signed it out', async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const store = await loadPage(); + store.signOutLocally('ins-a'); + + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + await loadPage(); + + expect(storedSignInMarkers()).toEqual({}); + }); + + it('brings back nothing written after a full sign-out, which keeps the record migrated', async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const store = await loadPage(); + store.signOutAllLocally(); + store.wipeLocalStorageForSignOut(); + + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + await loadPage(); + + expect(storedSignInMarkers()).toEqual({}); + }); + + it('keeps the record migrated through a full sign-out whose writes storage refuses', async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const store = await loadPage(); + store.signOutAllLocally(); + vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { + throw new DOMException('The quota has been exceeded.', 'QuotaExceededError'); + }); + vi.spyOn(console, 'debug').mockImplementation(() => undefined); + store.wipeLocalStorageForSignOut(); + vi.restoreAllMocks(); + + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + await loadPage(); + + expect(storedSignInMarkers()).toEqual({}); + }); + + it("brings back nothing written after a full sign-out, even when the first migration didn't land", async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const refused = vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { + throw new DOMException('The quota has been exceeded.', 'QuotaExceededError'); + }); + const store = await loadPage(); + refused.mockRestore(); + store.signOutAllLocally(); + store.wipeLocalStorageForSignOut(); + + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const next = await loadPage(); + + expect(next.getOperationsUrl('ins-a')).toBeUndefined(); + }); + + it("doesn't sign back in on a write-back when it can't clean the record", async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const store = await loadPage(); + store.signOutLocally('ins-a'); + localStorage.setItem(legacySignInMarkersKey, JSON.stringify({ 'ins-a': url('ins-a'), junk: 1 })); + vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { + throw new DOMException('The quota has been exceeded.', 'QuotaExceededError'); + }); + + const next = await loadPage(); + + expect(next.getOperationsUrl('ins-a')).toBeUndefined(); + }); + + it("doesn't sign back in on a write-back when it can't read the migration flag", async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const store = await loadPage(); + store.signOutLocally('ins-a'); + writeFromPreviousRelease({ 'ins-a': url('ins-a') }); + const getItem = Storage.prototype.getItem; + vi.spyOn(Storage.prototype, 'getItem').mockImplementation(function(this: Storage, key: string) { + if (key === 'Studio:PotentiallyAuthenticatedMigrated') { + throw new DOMException('Storage refused.', 'SecurityError'); + } + return getItem.call(this, key); + }); + + const next = await loadPage(); + + expect(next.getOperationsUrl('ins-a')).toBeUndefined(); + }); + + it('still removes a legacy record that is not one', async () => { + await loadPage(); + localStorage.setItem(legacySignInMarkersKey, JSON.stringify([url('ins-a')])); + + await loadPage(); + + expect(localStorage.getItem(legacySignInMarkersKey)).toBeNull(); + }); +}); + +describe('for tabs still on the previous release', () => { + it('mirrors each sign-in and sign-out into the legacy record, leaving its other markers', async () => { + writeFromPreviousRelease({ 'ins-old': url('ins-old') }); + const store = await loadPage(); + + signIn(store, 'ins-a'); + expect(legacy()).toEqual({ 'ins-old': url('ins-old'), 'ins-a': url('ins-a') }); + + store.signOutLocally('ins-a'); + expect(legacy()).toEqual({ 'ins-old': url('ins-old') }); + }); + + it('creates the legacy record when there is none, so a rollback still sees the sign-in', async () => { + const store = await loadPage(); + + signIn(store, 'ins-a'); + + expect(legacy()).toEqual({ 'ins-a': url('ins-a') }); + }); + + it("keeps a newer URL that tab recorded when a check of this tab's older one fails", async () => { + const store = await loadPage(); + signIn(store, 'ins-a'); + writeFromPreviousRelease({ 'ins-a': 'https://ins-a-2.example.com:9925/' }); + getInstanceUserInfo.mockRejectedValue(new Error('Network Error')); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + + await store.reloadUser('ins-a'); + + expect(legacy()).toEqual({ 'ins-a': 'https://ins-a-2.example.com:9925/' }); + }); + + it("leaves a legacy record it can't read alone at a sign-in", async () => { + const store = await loadPage(); + localStorage.setItem(legacySignInMarkersKey, 'not a record'); + + signIn(store, 'ins-a'); + + expect(localStorage.getItem(legacySignInMarkersKey)).toBe('not a record'); + }); + + it("keeps the whole legacy record when it can't mirror a failed check", async () => { + writeFromPreviousRelease({ 'ins-old': url('ins-old') }); + const store = await loadPage(); + signIn(store, 'ins-a'); + getInstanceUserInfo.mockRejectedValue(new Error('Network Error')); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + const setItem = Storage.prototype.setItem; + vi.spyOn(Storage.prototype, 'setItem').mockImplementation(function(this: Storage, key: string, value: string) { + if (key === legacySignInMarkersKey) { + throw new DOMException('The quota has been exceeded.', 'QuotaExceededError'); + } + setItem.call(this, key, value); + }); + + await store.reloadUser('ins-a'); + + expect(legacy()).toEqual({ 'ins-old': url('ins-old'), 'ins-a': url('ins-a') }); + }); + + it('takes the whole legacy record when it cannot mirror a sign-out', async () => { + writeFromPreviousRelease({ 'ins-a': url('ins-a'), 'ins-old': url('ins-old') }); + const store = await loadPage(); + const setItem = Storage.prototype.setItem; + vi.spyOn(Storage.prototype, 'setItem').mockImplementation(function(this: Storage, key: string, value: string) { + if (key === legacySignInMarkersKey) { + throw new DOMException('The quota has been exceeded.', 'QuotaExceededError'); + } + setItem.call(this, key, value); + }); + + store.signOutLocally('ins-a'); + + expect(legacy()).toBeNull(); + }); + + it('gives an app sign-out a sign-in only a previous-release tab recorded, and forgets it', async () => { + const store = await loadPage(); + writeFromPreviousRelease({ 'ins-old': url('ins-old') }); + localStorage.setItem('Studio:BasicAuth:ins-old', btoa(JSON.stringify({ username: 'admin', password: 'hunter2' }))); + + await store.signOutFromPotentiallyAuthenticatedInstances(); + + expect(onInstanceLogoutSubmit.mock.calls.map(([{ entityId, instanceClient }]) => [ + entityId, + instanceClient.defaults.baseURL, + ])).toEqual([['ins-old', url('ins-old')]]); + expect(legacy()).toEqual({}); + expect(store.checkForBasicAuth('ins-old')).toBeUndefined(); + }); + + it('gives signOutEntities a sign-in only a previous-release tab recorded', async () => { + const store = await loadPage(); + writeFromPreviousRelease({ 'ins-old': url('ins-old') }); + + await store.signOutEntities(['ins-old']); + + expect(onInstanceLogoutSubmit.mock.calls.map(([{ entityId }]) => entityId)).toEqual(['ins-old']); + expect(legacy()).toEqual({}); + }); + + it('gives an app sign-out no entity from a legacy record that is not one', async () => { + const store = await loadPage(); + localStorage.setItem(legacySignInMarkersKey, JSON.stringify([url('ins-old')])); + + await store.signOutFromPotentiallyAuthenticatedInstances(); + + expect(onInstanceLogoutSubmit).not.toHaveBeenCalled(); + }); +}); diff --git a/src/features/auth/store/localSignOutStorageFailure.test.ts b/src/features/auth/store/localSignOutStorageFailure.test.ts index f340278e8..b79181ff0 100644 --- a/src/features/auth/store/localSignOutStorageFailure.test.ts +++ b/src/features/auth/store/localSignOutStorageFailure.test.ts @@ -2,6 +2,7 @@ import { readEntitySettings, writeEntitySettings } from '@/features/instance/apis/explorer/settings'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { authStore, OverallAppSignIn } from './authStore'; +import { seedSignInMarkers, storedSignInMarkers } from './signInMarkersTestHelpers'; const { createInstanceAuthenticationTokens, getInstanceUserInfo } = vi.hoisted(() => ({ createInstanceAuthenticationTokens: vi.fn(), @@ -15,8 +16,6 @@ vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getIn type SetArgs = Parameters; -const MARKERS = 'Studio:PotentiallyAuthenticated'; - function signIn(id: string) { authStore.setUserForIdAndKey(id as SetArgs[0], `https://${id}` as SetArgs[1], { username: 'u' } as SetArgs[2]); } @@ -53,15 +52,16 @@ describe('authStore local sign-out when storage writes throw', () => { expect(readEntitySettings('ins-a')).toEqual({}); }); - it('drops the whole marker record rather than leave a signed-out entity marked', () => { + it("removes a signed-out entity's marker, and only it, while writes fail", () => { signIn('ins-a'); signIn('ins-b'); failEveryStorageWrite(); authStore.signOutLocally('ins-a'); - expect(localStorage.getItem(MARKERS)).toBeNull(); + expect(storedSignInMarkers()).toEqual({ 'ins-b': 'https://ins-b' }); expect(authStore.getOperationsUrl('ins-b')).toBe('https://ins-b'); + authStore.signOutLocally('ins-b'); }); it('drops the in-memory direct-connect token even when storage refuses removal', async () => { @@ -126,9 +126,8 @@ describe('authStore local sign-out when storage writes throw', () => { expect(authStore.checkForFabricConnect('ins-a')).toBe(true); }); - it('reports a marker record it could neither rewrite nor remove', () => { + it('reports a marker storage would neither remove nor blank', () => { signIn('ins-a'); - signIn('ins-b'); failEveryStorageWrite(); const refusal = new DOMException('Storage refused.', 'SecurityError'); vi.spyOn(Storage.prototype, 'removeItem').mockImplementation(() => { @@ -138,10 +137,150 @@ describe('authStore local sign-out when storage writes throw', () => { expect(() => authStore.signOutLocally('ins-a')).not.toThrow(); - expect(JSON.parse(localStorage.getItem(MARKERS) ?? '{}')).toHaveProperty('ins-a'); + expect(storedSignInMarkers()).toHaveProperty('ins-a'); expect(consoleError).toHaveBeenCalledWith(refusal); }); + describe("once storage works again after keeping a signed-out entity's marker", () => { + beforeEach(() => { + signIn('ins-p'); + failEveryStorageWrite(); + vi.spyOn(Storage.prototype, 'removeItem').mockImplementation(() => { + throw new DOMException('Storage refused.', 'SecurityError'); + }); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + authStore.signOutLocally('ins-p'); + vi.restoreAllMocks(); + }); + + afterEach(() => { + authStore.signOutLocally('ins-r'); + }); + + it.each([ + ['sign-out of another entity', () => authStore.signOutLocally('ins-never-signed-in')], + ['sign-in', () => signIn('ins-r')], + ])('removes it at the next %s', (_, write) => { + write(); + + expect(storedSignInMarkers()).not.toHaveProperty('ins-p'); + }); + + it('leaves it once another tab has signed in there again', () => { + seedSignInMarkers({ 'ins-p': 'https://ins-p-2' }); + + signIn('ins-r'); + + expect(storedSignInMarkers()).toHaveProperty('ins-p', 'https://ins-p-2'); + }); + + it('reaches it at a full sign-out after another tab signs it back in', () => { + seedSignInMarkers({ 'ins-p': 'https://ins-p' }); + localStorage.setItem('Studio:BasicAuth:ins-p', btoa(JSON.stringify({ username: 'u', password: 'p' }))); + + authStore.signOutAllLocally(); + + // Raw: this tab still reads the credential it could not remove at the first sign-out as absent. + expect(localStorage.getItem('Studio:BasicAuth:ins-p')).toBeNull(); + }); + }); + + it('keeps a marker this tab signed back into while storage still refused its removal', () => { + signIn('ins-v'); + failEveryStorageWrite(); + vi.spyOn(Storage.prototype, 'removeItem').mockImplementation(() => { + throw new DOMException('Storage refused.', 'SecurityError'); + }); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + vi.spyOn(console, 'debug').mockImplementation(() => undefined); + authStore.signOutLocally('ins-v'); + signIn('ins-v'); + vi.restoreAllMocks(); + + signIn('ins-w'); + + expect(storedSignInMarkers()).toHaveProperty('ins-v'); + ['ins-v', 'ins-w'].forEach((id) => authStore.signOutLocally(id)); + }); + + it('still owes the removal of a marker this tab signed back into at another URL while writes were refused', async () => { + authStore.setUserForIdAndKey('ins-x' as SetArgs[0], 'https://ins-x' as SetArgs[1], { username: 'u' } as SetArgs[2]); + failEveryStorageWrite(); + vi.spyOn(Storage.prototype, 'removeItem').mockImplementation(() => { + throw new DOMException('Storage refused.', 'SecurityError'); + }); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + vi.spyOn(console, 'debug').mockImplementation(() => undefined); + authStore.signOutLocally('ins-x'); + authStore.setUserForIdAndKey( + 'ins-x' as SetArgs[0], + 'https://ins-x-2' as SetArgs[1], + { username: 'u' } as SetArgs[2], + ); + vi.restoreAllMocks(); + getInstanceUserInfo.mockRejectedValue(new Error('Network Error')); + vi.spyOn(console, 'error').mockImplementation(() => undefined); + await authStore.reloadUser('ins-x'); + + signIn('ins-y'); + + expect(storedSignInMarkers()).not.toHaveProperty('ins-x'); + authStore.signOutLocally('ins-y'); + }); + + it('still removes a marker it cannot read at sign-out', () => { + signIn('ins-u'); + const getItem = Storage.prototype.getItem; + vi.spyOn(Storage.prototype, 'getItem').mockImplementation(function(this: Storage, key: string) { + if (key === 'Studio:PotentiallyAuthenticated:ins-u') { + throw new DOMException('Storage refused.', 'SecurityError'); + } + return getItem.call(this, key); + }); + + authStore.signOutLocally('ins-u'); + vi.restoreAllMocks(); + + expect(storedSignInMarkers()).not.toHaveProperty('ins-u'); + }); + + describe('a sign-in storage refused to record', () => { + afterEach(() => { + authStore.signOutLocally('ins-refused'); + }); + + it('still signs in, without throwing', () => { + failEveryStorageWrite(); + + expect(() => signIn('ins-refused')).not.toThrow(); + + expect(authStore.getConnectionById('ins-refused').user).toEqual({ username: 'u' }); + }); + + it('is recorded at the next sign-in', () => { + failEveryStorageWrite(); + signIn('ins-refused'); + vi.restoreAllMocks(); + + signIn('ins-refused'); + + expect(storedSignInMarkers()).toHaveProperty('ins-refused'); + }); + + it('is recorded at its next check, and a check that still cannot record it settles', async () => { + getInstanceUserInfo.mockResolvedValue({ username: 'u' }); + failEveryStorageWrite(); + signIn('ins-refused'); + + await expect(authStore.reloadUser('ins-refused')).resolves.toEqual({ username: 'u' }); + expect(authStore.getConnectionById('ins-refused').isLoading).toBe(false); + + vi.restoreAllMocks(); + await authStore.reloadUser('ins-refused'); + expect(storedSignInMarkers()).toHaveProperty('ins-refused'); + }); + }); + it('lets signOutAllLocally reach every entity and the cloud slot', () => { authStore.setUserForIdAndKey(OverallAppSignIn, OverallAppSignIn, { id: 'usr_a' } as SetArgs[2]); signIn('ins-a'); diff --git a/src/features/auth/store/signInMarkers.ts b/src/features/auth/store/signInMarkers.ts new file mode 100644 index 000000000..76e5e169c --- /dev/null +++ b/src/features/auth/store/signInMarkers.ts @@ -0,0 +1,155 @@ +import type { AuthenticatedConnectionKey, EntityIds } from './authStore'; + +/** + * Sign-in markers: one `localStorage` key per entity, holding the operations URL it was signed into. Each tab writes + * only the entity it signs in or out, so it can't erase or restore another tab's (HarperFast/studio#1968). + * + * The previous release kept every marker in one record, which its tabs still read once at load and write back whole. + * While those tabs can still be open, or a rollback can bring that release back, this one mirrors each change into the + * record too and reads it in sign-out sweeps (DESIGN.md). + */ +const signInMarkerPrefix = 'Studio:PotentiallyAuthenticated:'; +export const legacySignInMarkersKey = 'Studio:PotentiallyAuthenticated'; +/** Survives the sign-out wipe: a previous-release tab writing its copy back afterwards must not be migrated again. */ +export const legacyMigratedKey = 'Studio:PotentiallyAuthenticatedMigrated'; + +type SignIns = Record; + +export function signInMarkerKey(id: EntityIds): string { + return signInMarkerPrefix + id; +} + +/** Raw, since other tabs write them too. */ +export function readSignInMarkers(): Array<[EntityIds, AuthenticatedConnectionKey]> { + const markers: Array<[EntityIds, AuthenticatedConnectionKey]> = []; + try { + for (let index = 0; index < localStorage.length; index++) { + const key = localStorage.key(index); + const url = key?.startsWith(signInMarkerPrefix) ? localStorage.getItem(key) : null; + if (key && url) { + markers.push([key.slice(signInMarkerPrefix.length), url]); + } + } + } catch { + // Unreadable storage reads as signed out. + } + return markers; +} + +/** Null when the record is absent or is not one; entries that aren't markers are dropped. */ +function parseLegacySignInMarkers(raw: string | null): SignIns | null { + try { + const markers: unknown = raw ? JSON.parse(raw) : null; + if (!markers || typeof markers !== 'object' || Array.isArray(markers)) { + return null; + } + return Object.fromEntries( + Object.entries(markers).filter((entry): entry is [string, string] => typeof entry[1] === 'string' && !!entry[1]), + ); + } catch { + return null; + } +} + +export function readLegacySignInMarkers(): SignIns | null { + try { + return parseLegacySignInMarkers(localStorage.getItem(legacySignInMarkersKey)); + } catch { + return null; + } +} + +/** + * Copies the legacy record into per-entity keys once per storage, leaving any marker already stored per entity as it + * is. Later loads don't read it again: the previous release's tabs write their whole load-time copy back into it, so + * it would bring back what this release signed out. Two tabs migrating at once write the same values. Returns whether + * the record has been migrated, now or before. + * + * Every load also cleans the record, since the previous release fails to start on one that isn't a record: it is + * removed, and entries that aren't markers are dropped from one that is. + */ +export function migrateLegacySignInMarkers(): boolean { + let migrated = false; + let raw: string | null = null; + try { + migrated = !!localStorage.getItem(legacyMigratedKey); + raw = localStorage.getItem(legacySignInMarkersKey); + } catch { + // Reported as migrated: the record could be a previous-release write-back, which must not seed this load. + return true; + } + const legacy = parseLegacySignInMarkers(raw); + try { + if (raw !== null && !legacy) { + localStorage.removeItem(legacySignInMarkersKey); + } else if (legacy && JSON.stringify(legacy) !== raw) { + localStorage.setItem(legacySignInMarkersKey, JSON.stringify(legacy)); + } + } catch { + // Cleaned at the next load instead. + } + if (migrated) { + return true; + } + try { + for (const [id, url] of Object.entries(legacy ?? {})) { + if (!localStorage.getItem(signInMarkerKey(id))) { + localStorage.setItem(signInMarkerKey(id), url); + } + } + localStorage.setItem(legacyMigratedKey, '1'); + return true; + } catch { + return false; + } +} + +/** After the sign-out wipe nothing of the legacy record is left, so a write-back after it is never migrated. */ +export function markLegacySignInMarkersMigrated(): void { + try { + localStorage.setItem(legacyMigratedKey, '1'); + } catch { + // The wipe kept the flag if it was already set. + } +} + +/** + * Keeps the legacy record current for the previous release's tabs. Best effort: one that misses a sign-in only shows + * it signed out. A removal it can't write takes the whole record, as that release does itself, since a marker it kept + * would make its next load probe the entity with its cookies. A sign-in leaves a record it can't read alone, rather + * than replace other tabs' markers with this one. `removing` limits a removal to the URL a failed check used, and a + * failed check, being a probe rather than a sign-out, never takes the whole record. + */ +export function mirrorLegacySignInMarker( + id: EntityIds, + url: AuthenticatedConnectionKey | null, + removing?: AuthenticatedConnectionKey, +): void { + try { + const raw = localStorage.getItem(legacySignInMarkersKey); + const markers = raw === null ? {} : parseLegacySignInMarkers(raw); + if (!markers) { + throw new Error('The legacy sign-in record is not one'); + } + const unchanged = url === null + ? !Object.hasOwn(markers, id) || (removing !== undefined && markers[id] !== removing) + : markers[id] === url; + if (unchanged) { + return; + } + if (url === null) { + delete markers[id]; + } else { + markers[id] = url; + } + localStorage.setItem(legacySignInMarkersKey, JSON.stringify(markers)); + } catch { + if (url === null && removing === undefined) { + try { + localStorage.removeItem(legacySignInMarkersKey); + } catch { + // Storage that refuses this too keeps the record, as the previous release's own fallback would. + } + } + } +} diff --git a/src/features/auth/store/signInMarkersTestHelpers.ts b/src/features/auth/store/signInMarkersTestHelpers.ts new file mode 100644 index 000000000..9cf9cfed2 --- /dev/null +++ b/src/features/auth/store/signInMarkersTestHelpers.ts @@ -0,0 +1,11 @@ +import { readSignInMarkers, signInMarkerKey } from './signInMarkers'; + +export function seedSignInMarkers(markers: Record): void { + for (const [id, url] of Object.entries(markers)) { + localStorage.setItem(signInMarkerKey(id), url); + } +} + +export function storedSignInMarkers(): Record { + return Object.fromEntries(readSignInMarkers()); +} diff --git a/src/features/auth/store/signOutEntities.test.ts b/src/features/auth/store/signOutEntities.test.ts new file mode 100644 index 000000000..0ba335f3c --- /dev/null +++ b/src/features/auth/store/signOutEntities.test.ts @@ -0,0 +1,120 @@ +/** @vitest-environment jsdom */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { authStore } from './authStore'; +import { seedSignInMarkers, storedSignInMarkers } from './signInMarkersTestHelpers'; + +const { onInstanceLogoutSubmit } = vi.hoisted(() => ({ onInstanceLogoutSubmit: vi.fn() })); +vi.mock('@/integrations/api/instance/auth/onInstanceLogoutSubmit', () => ({ onInstanceLogoutSubmit })); + +type SetArgs = Parameters; +const ENTITIES = ['clu-1', 'ins-1', 'clu-2']; + +function remember(id: string) { + const user = { username: 'admin' } as SetArgs[2]; + authStore.setUserForIdAndKey(id as SetArgs[0], `https://${id}.example.com:9925/` as SetArgs[1], user); + authStore.flagForBasicAuth(id, { username: 'admin', password: 'hunter2' }); +} + +function rememberedEntities() { + return ENTITIES.filter((id) => !!authStore.getOperationsUrl(id) || !!authStore.checkForBasicAuth(id)); +} + +function loggedOut() { + return onInstanceLogoutSubmit.mock.calls.map(([{ entityId, instanceClient }]) => ({ + entityId, + baseURL: instanceClient.defaults.baseURL, + timeout: instanceClient.defaults.timeout, + auth: instanceClient.defaults.auth, + })); +} + +beforeEach(() => { + localStorage.clear(); + ENTITIES.forEach(remember); + onInstanceLogoutSubmit.mockResolvedValue({ message: 'ok' }); + vi.spyOn(console, 'debug').mockImplementation(() => undefined); +}); + +afterEach(() => { + vi.resetAllMocks(); + vi.restoreAllMocks(); + ENTITIES.forEach((id) => authStore.signOutLocally(id)); +}); + +describe('authStore.signOutEntities', () => { + it('sends a logout for each remembered sign-in to its own server, with its credential and a short timeout, and forgets it', async () => { + await authStore.signOutEntities(['clu-1', 'ins-1', 'clu-1']); + + const credential = { username: 'admin', password: 'hunter2' }; + expect(loggedOut()).toEqual([ + { entityId: 'clu-1', baseURL: 'https://clu-1.example.com:9925/', timeout: 10_000, auth: credential }, + { entityId: 'ins-1', baseURL: 'https://ins-1.example.com:9925/', timeout: 10_000, auth: credential }, + ]); + expect(rememberedEntities()).toEqual(['clu-2']); + }); + + it('forgets the sign-ins, and resolves, when a logout fails', async () => { + const consoleError = vi.spyOn(console, 'error'); + onInstanceLogoutSubmit.mockRejectedValue(new Error('connect ECONNREFUSED')); + + await expect(authStore.signOutEntities(['clu-1', 'ins-1'])).resolves.toBeUndefined(); + + expect(rememberedEntities()).toEqual(['clu-2']); + expect(console.debug).toHaveBeenCalledWith(expect.stringContaining('clu-1'), 'connect ECONNREFUSED'); + expect(consoleError).not.toHaveBeenCalled(); + }); + + it('forgets the sign-ins before a slow logout answers', async () => { + let answer: () => void = () => undefined; + onInstanceLogoutSubmit.mockReturnValue(new Promise((resolve) => (answer = () => resolve({ message: 'ok' })))); + let settled = false; + + const signOut = authStore.signOutEntities(['clu-1']).then(() => (settled = true)); + + expect(rememberedEntities()).toEqual(['ins-1', 'clu-2']); + await new Promise((resolve) => setTimeout(resolve)); + expect(settled).toBe(false); + answer(); + await signOut; + expect(settled).toBe(true); + }); + + it('logs out a session-cookie sign-in, which stores no Basic Auth credential', async () => { + const user = { username: 'admin' } as SetArgs[2]; + authStore.setUserForIdAndKey( + 'ins-cookie' as SetArgs[0], + 'https://ins-cookie.example.com:9925/' as SetArgs[1], + user, + ); + + await authStore.signOutEntities(['ins-cookie']); + + expect(loggedOut()).toEqual([ + { entityId: 'ins-cookie', baseURL: 'https://ins-cookie.example.com:9925/', timeout: 10_000, auth: undefined }, + ]); + expect(authStore.getOperationsUrl('ins-cookie')).toBeUndefined(); + }); + + it('logs out, and forgets, a sign-in another tab made after this one read the markers', async () => { + const markers = storedSignInMarkers(); + seedSignInMarkers({ 'ins-other-tab': 'https://ins-other-tab.example.com:9925/' }); + + await authStore.signOutEntities(['ins-other-tab']); + + expect(loggedOut()).toEqual([ + { + entityId: 'ins-other-tab', + baseURL: 'https://ins-other-tab.example.com:9925/', + timeout: 10_000, + auth: undefined, + }, + ]); + expect(storedSignInMarkers()).toEqual(markers); + }); + + it('posts no logout for an id with nothing remembered', async () => { + await authStore.signOutEntities(['ins-unknown']); + + expect(onInstanceLogoutSubmit).not.toHaveBeenCalled(); + }); +}); diff --git a/src/features/auth/store/signOutRaces.test.ts b/src/features/auth/store/signOutRaces.test.ts index 1c60461d8..e30dfaeb0 100644 --- a/src/features/auth/store/signOutRaces.test.ts +++ b/src/features/auth/store/signOutRaces.test.ts @@ -17,6 +17,7 @@ vi.mock('@/integrations/api/instance/status/getInstanceUserInfo', () => ({ getIn import { getCurrentUser } from '@/features/auth/queries/getCurrentUser'; import { authStore, OverallAppSignIn } from './authStore'; +import { storedSignInMarkers } from './signInMarkersTestHelpers'; type SetArgs = Parameters; @@ -69,7 +70,7 @@ describe('auth resolutions that outlive a sign-out', () => { await expect(reload).resolves.toBeNull(); expectNothingRestored('ins-load'); - expect(localStorage.getItem('Studio:PotentiallyAuthenticated') ?? '').not.toContain('ins-load'); + expect(storedSignInMarkers()).not.toHaveProperty('ins-load'); }); it('getCurrentUser hands back its answer but does not sign the cloud slot back in', async () => { diff --git a/src/features/cluster/ClusterHome.signIns.test.tsx b/src/features/cluster/ClusterHome.signIns.test.tsx index fc255c013..ab0784b46 100644 --- a/src/features/cluster/ClusterHome.signIns.test.tsx +++ b/src/features/cluster/ClusterHome.signIns.test.tsx @@ -1,4 +1,5 @@ /** @vitest-environment jsdom */ +import { seedSignInMarkers } from '@/features/auth/store/signInMarkersTestHelpers'; import type { Cluster } from '@/integrations/api/api.patch'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { cleanup, render, screen } from '@testing-library/react'; @@ -58,10 +59,7 @@ async function renderOverview(status: string) { beforeEach(() => { localStorage.clear(); - localStorage.setItem( - 'Studio:PotentiallyAuthenticated', - JSON.stringify({ 'clu-1': 'https://clu-1.example.com:9925/' }), - ); + seedSignInMarkers({ 'clu-1': 'https://clu-1.example.com:9925/' }); getInstanceUserInfo.mockResolvedValue({ username: 'admin' }); }); diff --git a/src/features/cluster/DESIGN.md b/src/features/cluster/DESIGN.md index cf4ce1b2e..734552ef3 100644 --- a/src/features/cluster/DESIGN.md +++ b/src/features/cluster/DESIGN.md @@ -94,8 +94,11 @@ which is not final. Never from a route guard: guards also run on hover preloads, credentials. A sign-in left behind is not harmless. Each app sign-out posts a logout to every remembered host, and a stored Basic Auth credential would stay in localStorage for good. -A self-hosted cluster removed from Fabric keeps its sign-ins. Its servers may still run, and forgetting them locally -would leave their cookie sessions open; kept, the next app sign-out still logs them out. +A self-hosted cluster removed from Fabric is never cleared on render. Its servers may still run, and forgetting their +sign-ins locally would leave their cookie sessions open. Removing one from Studio sends each remembered sign-in's server a +logout the way app sign-out does (`signOutEntities`: one attempt, a 10s timeout, not awaited by the removal), and forgets +the sign-in without waiting for the answer. So a failed logout still forgets it, and a server unreachable at that moment +keeps its session. Removed elsewhere, its sign-ins stay until an app sign-out logs them out. None of the hidden pages can do anything there. Central manager's `assertClusterMutable` refuses every update to a cluster that isn't `RUNNING`, and the instances are torn down, along with the certificates issued on them. Nor is anything left diff --git a/src/features/cluster/terminatedClusterSignIns.ts b/src/features/cluster/terminatedClusterSignIns.ts index c5a5ae9ab..0e19fca33 100644 --- a/src/features/cluster/terminatedClusterSignIns.ts +++ b/src/features/cluster/terminatedClusterSignIns.ts @@ -1,13 +1,14 @@ import { Cluster } from '@/integrations/api/api.patch'; import { clusterIsSelfManaged } from '@/integrations/api/clusterIsSelfManaged'; +export function clusterSignInIds(cluster: Cluster): string[] { + return [cluster.id, ...(cluster.instances ?? []).map((instance) => instance.id)].filter(Boolean); +} + /** - * None for a self-hosted cluster removed from Fabric: its servers may still run, so its sign-ins stay remembered and an - * app sign-out still ends their sessions. + * None for a self-hosted cluster removed from Fabric: its servers may still run, and forgetting their sign-ins without a + * logout would leave their sessions open. */ export function terminatedClusterSignIns(cluster: Cluster): string[] { - if (clusterIsSelfManaged(cluster)) { - return []; - } - return [cluster.id, ...(cluster.instances ?? []).map((instance) => instance.id)].filter(Boolean); + return clusterIsSelfManaged(cluster) ? [] : clusterSignInIds(cluster); } diff --git a/src/features/clusters/mutations/terminateCluster.test.ts b/src/features/clusters/mutations/terminateCluster.test.ts index 4255b75db..d53e80efa 100644 --- a/src/features/clusters/mutations/terminateCluster.test.ts +++ b/src/features/clusters/mutations/terminateCluster.test.ts @@ -1,9 +1,15 @@ /** @vitest-environment jsdom */ import { authStore } from '@/features/auth/store/authStore'; +import { seedSignInMarkers, storedSignInMarkers } from '@/features/auth/store/signInMarkersTestHelpers'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { apiDelete, apiGet } = vi.hoisted(() => ({ apiDelete: vi.fn(), apiGet: vi.fn() })); +const { apiDelete, apiGet, onInstanceLogoutSubmit } = vi.hoisted(() => ({ + apiDelete: vi.fn(), + apiGet: vi.fn(), + onInstanceLogoutSubmit: vi.fn(), +})); vi.mock('@/config/apiClient', () => ({ apiClient: { delete: apiDelete, get: apiGet } })); +vi.mock('@/integrations/api/instance/auth/onInstanceLogoutSubmit', () => ({ onInstanceLogoutSubmit })); const { terminateCluster } = await import('./terminateCluster'); @@ -12,6 +18,7 @@ const ENTITIES = ['clu-1', 'ins-1', 'ins-2', 'clu-2']; const DELETED = { data: { id: 'clu-1', deleted: true } }; // The organization's cluster list carries no instances; the cluster record does. const RECORD = { data: { id: 'clu-1', status: 'RUNNING', instances: [{ id: 'ins-1' }, { id: 'ins-2' }] } }; +const SELF_HOSTED = { data: { ...RECORD.data, plans: [{ planId: 'self-hosted-free' }] } }; function remember(id: string) { const user = { username: 'admin' } as SetArgs[2]; @@ -20,14 +27,23 @@ function remember(id: string) { } function rememberedEntities() { + return ENTITIES.filter((id) => !!authStore.getOperationsUrl(id) || !!authStore.checkForBasicAuth(id)); +} + +function fullyRememberedEntities() { return ENTITIES.filter((id) => !!authStore.getOperationsUrl(id) && !!authStore.checkForBasicAuth(id)); } +function loggedOut() { + return onInstanceLogoutSubmit.mock.calls.map(([{ entityId }]) => entityId); +} + beforeEach(() => { localStorage.clear(); ENTITIES.forEach(remember); apiDelete.mockResolvedValue(DELETED); apiGet.mockResolvedValue(RECORD); + onInstanceLogoutSubmit.mockResolvedValue({ message: 'ok' }); vi.spyOn(console, 'debug').mockImplementation(() => undefined); }); @@ -43,6 +59,19 @@ describe(terminateCluster, () => { await vi.waitFor(() => expect(rememberedEntities()).toEqual(['clu-2'])); expect(apiGet).toHaveBeenCalledWith('/Cluster/clu-1'); + expect(onInstanceLogoutSubmit).not.toHaveBeenCalled(); + }); + + it('forgets sign-ins only another tab remembers', async () => { + const markers = storedSignInMarkers(); + const otherTab = { 'clu-9': 'https://clu-9.example.com:9925/', 'ins-9': 'https://ins-9.example.com:9925/' }; + seedSignInMarkers(otherTab); + apiDelete.mockResolvedValue({ data: { id: 'clu-9', deleted: true } }); + apiGet.mockResolvedValue({ data: { id: 'clu-9', status: 'RUNNING', instances: [{ id: 'ins-9' }] } }); + + await terminateCluster('clu-9'); + + await vi.waitFor(() => expect(storedSignInMarkers()).toEqual(markers)); }); it('reads the instances back after the terminate when the read alongside it failed', async () => { @@ -60,25 +89,47 @@ describe(terminateCluster, () => { await expect(terminateCluster('clu-1')).resolves.toEqual(DELETED.data); await vi.waitFor(() => expect(console.debug).toHaveBeenCalledTimes(2)); - expect(rememberedEntities()).toEqual(ENTITIES); + expect(fullyRememberedEntities()).toEqual(ENTITIES); + expect(onInstanceLogoutSubmit).not.toHaveBeenCalled(); }); - it("keeps a removed self-hosted cluster's sign-ins, whose servers may still run", async () => { - apiGet.mockResolvedValue({ data: { ...RECORD.data, plans: [{ planId: 'self-hosted-free' }] } }); + it('forgets nothing when the terminate fails', async () => { + apiDelete.mockRejectedValue(new Error('Request failed with status code 400')); - await terminateCluster('clu-1'); + await expect(terminateCluster('clu-1')).rejects.toThrow('400'); await new Promise((resolve) => setTimeout(resolve)); - expect(apiGet).toHaveBeenCalled(); - expect(rememberedEntities()).toEqual(ENTITIES); + expect(fullyRememberedEntities()).toEqual(ENTITIES); }); +}); - it('forgets nothing when the terminate fails', async () => { - apiDelete.mockRejectedValue(new Error('Request failed with status code 400')); +describe('removing a self-hosted cluster, whose servers may still run', () => { + beforeEach(() => { + apiGet.mockResolvedValue(SELF_HOSTED); + }); - await expect(terminateCluster('clu-1')).rejects.toThrow('400'); + it('sends the cluster and every instance a logout, and forgets them', async () => { + await terminateCluster('clu-1'); - await new Promise((resolve) => setTimeout(resolve)); - expect(rememberedEntities()).toEqual(ENTITIES); + await vi.waitFor(() => expect(rememberedEntities()).toEqual(['clu-2'])); + expect(loggedOut()).toEqual(['clu-1', 'ins-1', 'ins-2']); + }); + + it('still forgets them when a logout fails', async () => { + onInstanceLogoutSubmit.mockRejectedValue(new Error('timeout of 10000ms exceeded')); + + await expect(terminateCluster('clu-1')).resolves.toEqual(DELETED.data); + + await vi.waitFor(() => expect(rememberedEntities()).toEqual(['clu-2'])); + expect(loggedOut()).toEqual(['clu-1', 'ins-1', 'ins-2']); + }); + + it('finishes the removal without waiting for the logouts', async () => { + onInstanceLogoutSubmit.mockReturnValue(new Promise(() => undefined)); + + await expect(terminateCluster('clu-1')).resolves.toEqual(DELETED.data); + + await vi.waitFor(() => expect(loggedOut()).toEqual(['clu-1', 'ins-1', 'ins-2'])); + expect(rememberedEntities()).toEqual(['clu-2']); }); }); diff --git a/src/features/clusters/mutations/terminateCluster.ts b/src/features/clusters/mutations/terminateCluster.ts index edd6d7345..ca7d72f4d 100644 --- a/src/features/clusters/mutations/terminateCluster.ts +++ b/src/features/clusters/mutations/terminateCluster.ts @@ -1,8 +1,9 @@ import { apiClient } from '@/config/apiClient'; import { authStore } from '@/features/auth/store/authStore'; import { getClusterInfo } from '@/features/cluster/queries/getClusterInfoQuery'; -import { terminatedClusterSignIns } from '@/features/cluster/terminatedClusterSignIns'; +import { clusterSignInIds } from '@/features/cluster/terminatedClusterSignIns'; import { Cluster } from '@/integrations/api/api.patch'; +import { clusterIsSelfManaged } from '@/integrations/api/clusterIsSelfManaged'; import { useMutation } from '@tanstack/react-query'; export async function terminateCluster(clusterId: string) { @@ -15,12 +16,18 @@ export async function terminateCluster(clusterId: string) { /** * The organization's cluster list carries no instances, so they come from the cluster record: read alongside the DELETE, * or read back after it, since central manager keeps a terminated cluster's record and its instances. Without either - * read nothing is forgotten, since the cluster might be self-hosted; `ClusterLayout` forgets them on the next visit. + * read nothing is forgotten, since the cluster might be self-hosted: a Fabric cluster's go on its next visit + * (`ClusterLayout`), a self-hosted one's at the next app sign-out. */ async function forgetSignIns(clusterId: string, before: Promise): Promise { const cluster = await before ?? await readCluster(clusterId); - if (cluster) { - authStore.signOutEntitiesLocally(terminatedClusterSignIns(cluster)); + if (!cluster) { + return; + } + if (clusterIsSelfManaged(cluster)) { + await authStore.signOutEntities(clusterSignInIds(cluster)); + } else { + authStore.signOutEntitiesLocally(clusterSignInIds(cluster)); } } diff --git a/src/integrations/api/instance/auth/onInstanceLogoutSubmit.ts b/src/integrations/api/instance/auth/onInstanceLogoutSubmit.ts index 12f71aa47..16b4fad2e 100644 --- a/src/integrations/api/instance/auth/onInstanceLogoutSubmit.ts +++ b/src/integrations/api/instance/auth/onInstanceLogoutSubmit.ts @@ -6,13 +6,15 @@ interface LogoutInfoResponse { } export async function onInstanceLogoutSubmit( - { instanceClient, entityId }: InstanceClientIdConfig, + { instanceClient, entityId, signedInSince }: InstanceClientIdConfig & { signedInSince?: () => boolean }, ): Promise { const { data } = await instanceClient.post('/', { operation: 'logout', }); - authStore.flagForBasicAuth(entityId, null); - authStore.flagForFabricConnect(entityId, false); + if (!signedInSince?.()) { + authStore.flagForBasicAuth(entityId, null); + authStore.flagForFabricConnect(entityId, false); + } if (data) { return data; } else { diff --git a/src/router/__tests__/crossTabSignOut.test.tsx b/src/router/__tests__/crossTabSignOut.test.tsx index f1222b5fe..f6848405d 100644 --- a/src/router/__tests__/crossTabSignOut.test.tsx +++ b/src/router/__tests__/crossTabSignOut.test.tsx @@ -1,4 +1,5 @@ // @vitest-environment jsdom +import { seedSignInMarkers, storedSignInMarkers } from '@/features/auth/store/signInMarkersTestHelpers'; import { type QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { cleanup, render, screen, waitFor } from '@testing-library/react'; import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; @@ -295,7 +296,7 @@ function reportedAuthorizationFailures() { beforeEach(() => { localStorage.clear(); sessionStorage.clear(); - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'OverallAppSignIn' })); + seedSignInMarkers({ OverallAppSignIn: 'OverallAppSignIn' }); window.history.replaceState(null, '', `#${PAGE}`); session = 'live'; requests.length = 0; @@ -445,13 +446,13 @@ describe('a sign-out in another tab', () => { const thisTab = await openClusterPage(); const otherTab = await openAnotherTab(); // A reload during a central-manager blip: that tab reads the failed check as signed out, and drops the app - // sign-in from the marker record both tabs share. + // sign-in's marker, which both tabs share. otherTab.apiClient.defaults.adapter = async (config) => { throw new AxiosError('timeout of 60000ms exceeded', AxiosError.ECONNABORTED, config); }; await otherTab.run(() => otherTab.authStore.reloadUser('OverallAppSignIn')); - expect(localStorage.getItem('Studio:PotentiallyAuthenticated')).toBe('{}'); + expect(storedSignInMarkers()).toEqual({}); await expectStillSignedIn(thisTab); }); diff --git a/src/router/__tests__/endedSessionRouteFailures.test.tsx b/src/router/__tests__/endedSessionRouteFailures.test.tsx index 92f0625d9..3e40f9b4f 100644 --- a/src/router/__tests__/endedSessionRouteFailures.test.tsx +++ b/src/router/__tests__/endedSessionRouteFailures.test.tsx @@ -1,4 +1,5 @@ // @vitest-environment jsdom +import { seedSignInMarkers } from '@/features/auth/store/signInMarkersTestHelpers'; import { type QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { cleanup, render, screen, waitFor } from '@testing-library/react'; import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; @@ -177,7 +178,7 @@ function reportedAuthorizationFailures() { beforeEach(() => { localStorage.clear(); sessionStorage.clear(); - localStorage.setItem('Studio:PotentiallyAuthenticated', JSON.stringify({ OverallAppSignIn: 'OverallAppSignIn' })); + seedSignInMarkers({ OverallAppSignIn: 'OverallAppSignIn' }); // Not `location.hash =`: jsdom dispatches its popstate a task later, and a render that beats it gets a second // router load wherever that lands. Between the sign-out and the router hearing of it, that is the load the // last case starts on purpose. diff --git a/src/router/__tests__/rememberedClusterDeepLink.test.ts b/src/router/__tests__/rememberedClusterDeepLink.test.ts index ff683b50c..dd5852d9b 100644 --- a/src/router/__tests__/rememberedClusterDeepLink.test.ts +++ b/src/router/__tests__/rememberedClusterDeepLink.test.ts @@ -1,4 +1,5 @@ // @vitest-environment jsdom +import { seedSignInMarkers } from '@/features/auth/store/signInMarkersTestHelpers'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; // Transformed here, while the file is collected, which has no timeout, rather than inside the first test's 5s. // Each test still evaluates a fresh copy. @@ -71,10 +72,7 @@ beforeEach(() => { cluster = { status: 'RUNNING' }; localStorage.clear(); sessionStorage.clear(); - localStorage.setItem( - 'Studio:PotentiallyAuthenticated', - JSON.stringify({ OverallAppSignIn: 'OverallAppSignIn', [CLUSTER_ID]: CLUSTER_URL }), - ); + seedSignInMarkers({ OverallAppSignIn: 'OverallAppSignIn', [CLUSTER_ID]: CLUSTER_URL }); get.mockImplementation(async (url: string) => { if (url === '/User/current') { return { data: cloudUser }; @@ -136,10 +134,7 @@ describe('a deep link into a remembered cluster that has been terminated since', }); function remember(entity: string, url: string) { - localStorage.setItem( - 'Studio:PotentiallyAuthenticated', - JSON.stringify({ OverallAppSignIn: 'OverallAppSignIn', [CLUSTER_ID]: CLUSTER_URL, [entity]: url }), - ); + seedSignInMarkers({ [entity]: url }); } it.each(remembered)('sends /$page to the overview before checking the sign-in or loading the page', async ({ diff --git a/src/router/__tests__/terminatedClusterDeepLink.test.tsx b/src/router/__tests__/terminatedClusterDeepLink.test.tsx index 1d0eff223..a1104660c 100644 --- a/src/router/__tests__/terminatedClusterDeepLink.test.tsx +++ b/src/router/__tests__/terminatedClusterDeepLink.test.tsx @@ -1,4 +1,5 @@ // @vitest-environment jsdom +import { seedSignInMarkers, storedSignInMarkers } from '@/features/auth/store/signInMarkersTestHelpers'; import { QueryClientProvider } from '@tanstack/react-query'; import { cleanup, render, screen } from '@testing-library/react'; import type { InternalAxiosRequestConfig } from 'axios'; @@ -116,7 +117,7 @@ async function openBookmark(path: string, cluster: object = {}) { } function rememberedEntities() { - const markers = JSON.parse(localStorage.getItem('Studio:PotentiallyAuthenticated') || '{}'); + const markers = storedSignInMarkers(); return Object.keys(SIGN_INS).filter((id) => markers[id] || localStorage.getItem(`Studio:BasicAuth:${id}`) || localStorage.getItem(`Studio:FabricConnect:${id}`) ); @@ -125,10 +126,7 @@ function rememberedEntities() { beforeEach(() => { localStorage.clear(); sessionStorage.clear(); - localStorage.setItem( - 'Studio:PotentiallyAuthenticated', - JSON.stringify({ OverallAppSignIn: 'OverallAppSignIn', ...SIGN_INS }), - ); + seedSignInMarkers({ OverallAppSignIn: 'OverallAppSignIn', ...SIGN_INS }); for (const id of Object.keys(SIGN_INS)) { localStorage.setItem(`Studio:BasicAuth:${id}`, btoa(JSON.stringify({ username: 'admin', password: 'hunter2' }))); localStorage.setItem(`Studio:FabricConnect:${id}`, 'true');