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/features/instance/databases/DESIGN.md b/src/features/instance/databases/DESIGN.md index e715373da..364ab11af 100644 --- a/src/features/instance/databases/DESIGN.md +++ b/src/features/instance/databases/DESIGN.md @@ -27,7 +27,15 @@ schema load. Add Records, Import and Export CSV need the key itself, so they wai that names one (`hasKnownPrimaryKey`, in the toolbar and in [`TableContextMenuItems`](components/TableContextMenuItems.tsx), the tree and overview menus). A table nothing has described yet gets the same answer as one without a key, rather than an Export -that then reports "no primary key found" (#1941). +that then reports "no primary key found" (#1941). The menus' Drop Table waits for the map to hold +the table too: one the map no longer holds may already have been dropped. The table view needs no +such gate, because `resolveDatabasesRedirect` leaves a table the map no longer holds. Browse reads +the describe map and the schema-relationship map by database and table name only through +[`describedTable.ts`](functions/describedTable.ts), which takes their own keys, so a table named like +an `Object.prototype` member (`constructor`, `toString`) is never found there by mistake. + +A Radix menu item runs its `onClick` even while it is disabled, and only CSS keeps a pointer off it, +so an item that can be disabled takes its action through `onSelect`, which Radix gates. `$id` is not a stand-in key. These are live-probe observations on `harperdb:4.7.36` and `harper-pro:5.2.13` (2026-09), not covered by a test here: @@ -156,6 +164,16 @@ so it is judged by that table's attribute. Typed filters and deep-linked ones bo carries `allowFullScan: true`, and so does a CSV export of that filter. A new way to apply a filter has to go through that function. +## Browse asks in a Studio dialog, and a question belongs to where it was asked + +The full-scan question, Cleanup Orphan Blobs, Delete Selected and the row editor's discard all ask in +an `AlertDialog`, not `window.confirm()` (#1924). Nothing blocks while it is open, so each question +is a pending request that carries what it would act on: the filter's conditions, the selected keys. +`DatabaseTableView` stays mounted when the route moves to another table, so its request also carries +the view's context (instance, database, table, deep-link filters) and is cleared, not answered, once +that changes. The editor's discard question is cleared once its draft is gone. A filter an index can +answer never asks and applies at once, as before. + ## An import refreshes Browse from the mutation, not from the modal Every Browse read has a 60s `staleTime`, so an import into the open table shows nothing new until diff --git a/src/features/instance/databases/components/DatabaseActionModals.tsx b/src/features/instance/databases/components/DatabaseActionModals.tsx index deb672de4..04131d47d 100644 --- a/src/features/instance/databases/components/DatabaseActionModals.tsx +++ b/src/features/instance/databases/components/DatabaseActionModals.tsx @@ -1,4 +1,9 @@ import { useInstanceClientIdParams } from '@/config/useInstanceClient'; +import { + declaredRelationships, + describedDatabase, + describedTable, +} from '@/features/instance/databases/functions/describedTable'; import { getSchemaRelationshipsQueryOptions } from '@/features/instance/databases/functions/schemaRelationships'; import { AddTableRowModal } from '@/features/instance/databases/modals/AddTableRowModal'; import { CreateNewTableModal } from '@/features/instance/databases/modals/CreateNewTableModal'; @@ -55,7 +60,7 @@ export function DatabaseActionModals({ instanceDatabaseMap }: { instanceDatabase ); const addTableFromMap = addTarget - ? instanceDatabaseMap?.[addTarget.databaseName]?.[addTarget.tableName] + ? describedTable(instanceDatabaseMap, addTarget.databaseName, addTarget.tableName) : undefined; const [describedAddTable, setDescribedAddTable] = useState<{ target: AddTarget; table: InstanceTable }>(); const addInstanceTable = addTableFromMap @@ -123,8 +128,12 @@ export function DatabaseActionModals({ instanceDatabaseMap }: { instanceDatabase { if (!open) { diff --git a/src/features/instance/databases/components/DatabaseOverview.tsx b/src/features/instance/databases/components/DatabaseOverview.tsx index dcdc4f10a..0615a8298 100644 --- a/src/features/instance/databases/components/DatabaseOverview.tsx +++ b/src/features/instance/databases/components/DatabaseOverview.tsx @@ -3,6 +3,7 @@ import { DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuTrigge import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { TableRowContextMenu } from '@/features/instance/databases/components/TableRowContextMenu'; +import { describedDatabase, describedTableNames } from '@/features/instance/databases/functions/describedTable'; import { summarizeDatabaseCounts } from '@/features/instance/databases/functions/summarizeDatabaseCounts'; import { useInstanceBrowseManagePermission, useInstanceImportOperationsPermission } from '@/hooks/usePermissions'; import { InstanceDatabaseMap } from '@/integrations/api/api.patch'; @@ -58,8 +59,8 @@ export function DatabaseOverview({ instanceDatabaseMap, databaseName }: { const canManage = useInstanceBrowseManagePermission(); const canImport = useInstanceImportOperationsPermission(); - const tables = instanceDatabaseMap?.[databaseName] ?? {}; - const tableNames = Object.keys(tables).sort(); + const tables = describedDatabase(instanceDatabaseMap, databaseName) ?? {}; + const tableNames = describedTableNames(instanceDatabaseMap, databaseName); // Sizes/schema are already in the fast (count-skipping) map, so they render instantly. Row counts // aren't, so fetch them per-table via describe_table's cheap estimate path -- but only for the diff --git a/src/features/instance/databases/components/DatabaseTableView.deepLinkRelationship.test.tsx b/src/features/instance/databases/components/DatabaseTableView.deepLinkRelationship.test.tsx index 8efefe665..15814b38b 100644 --- a/src/features/instance/databases/components/DatabaseTableView.deepLinkRelationship.test.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.deepLinkRelationship.test.tsx @@ -4,7 +4,7 @@ import { InstanceDatabaseMap, InstanceTable } from '@/integrations/api/api.patch'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, cleanup, render, screen } from '@testing-library/react'; -import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { DatabaseTableView } from './DatabaseTableView'; vi.mock('@tanstack/react-router', () => { @@ -142,15 +142,10 @@ beforeAll(() => { } }); -const confirm = vi.fn<(message?: string) => boolean>(() => true); -beforeEach(() => { - vi.stubGlobal('confirm', confirm); -}); +const fullScanQuestion = () => screen.queryByRole('alertdialog'); afterEach(() => { cleanup(); - vi.unstubAllGlobals(); - confirm.mockClear(); instance.searches = []; instance.holdComponents = true; }); @@ -162,7 +157,7 @@ async function expectForeignKeySearch() { expect(instance.searches[0].conditions).toEqual([ { search_attribute: 'authorId', search_type: 'equals', search_value: 'a1' }, ]); - expect(confirm).not.toHaveBeenCalled(); + expect(fullScanQuestion()).toBeNull(); await screen.findByRole('cell', { name: 'b1' }, { timeout: 10_000 }); expect(screen.queryByRole('cell', { name: 'b2' })).toBeNull(); } @@ -203,7 +198,7 @@ describe('DatabaseTableView deep-linked filter on a relationship only the compon await vi.waitFor(() => expect(queryClient.getQueryData(['instance-1', 'schema-relationships'])).toBeDefined()); await act(async () => {}); expect(instance.searches).toEqual([]); - expect(confirm).not.toHaveBeenCalled(); + expect(fullScanQuestion()).toBeNull(); rerender(view(databaseMap)); diff --git a/src/features/instance/databases/components/DatabaseTableView.fullScanFilter.test.tsx b/src/features/instance/databases/components/DatabaseTableView.fullScanFilter.test.tsx index a1f97ebf2..d9bf8f8c1 100644 --- a/src/features/instance/databases/components/DatabaseTableView.fullScanFilter.test.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.fullScanFilter.test.tsx @@ -4,7 +4,7 @@ import { InstanceTable } from '@/integrations/api/api.patch'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { cleanup, fireEvent, render, screen } from '@testing-library/react'; -import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { DatabaseTableView } from './DatabaseTableView'; const route = vi.hoisted(() => ({ search: {} as { filters?: Record } })); @@ -110,19 +110,16 @@ beforeAll(() => { } }); -const confirm = vi.fn<(message?: string) => boolean>(); -beforeEach(() => { - vi.stubGlobal('confirm', confirm); -}); - afterEach(() => { cleanup(); - vi.unstubAllGlobals(); - confirm.mockReset(); instance.searches = []; route.search = {}; }); +const scanQuestion = () => screen.queryByRole('alertdialog', { name: /^Scan every record in/ }); +const findScanQuestion = () => screen.findByRole('alertdialog', { name: /^Scan every record in/ }); +const answer = (name: 'Scan anyway' | 'Cancel') => fireEvent.click(screen.getByRole('button', { name })); + function renderView() { return render( @@ -152,21 +149,26 @@ describe('DatabaseTableView filtering a column no index covers', { timeout: 30_0 }); it('asks first, and runs nothing when the user declines', async () => { - confirm.mockReturnValue(false); renderView(); await filterBy('note', 'beta'); - expect(confirm).toHaveBeenCalledTimes(1); - expect(confirm.mock.calls[0][0]).toContain('every record in "Thing"'); + const question = await findScanQuestion(); + expect(question.querySelector('h2')!.textContent).toBe('Scan every record in "Thing"?'); + expect(question.textContent).toContain('this filter on "note"'); + expect(instance.searches).toEqual([]); + answer('Cancel'); + + expect(scanQuestion()).toBeNull(); await findCell('t3'); expect(instance.searches).toEqual([]); expect(screen.getByRole('button', { name: /apply filters/i })).toBeTruthy(); }); it('runs the search, marked as allowed to scan, once the user agrees', async () => { - confirm.mockReturnValue(true); renderView(); await filterBy('note', 'beta'); + await findScanQuestion(); + answer('Scan anyway'); await vi.waitFor(() => expect(instance.searches).toHaveLength(1)); expect(instance.searches[0]).toMatchObject({ @@ -182,17 +184,17 @@ describe('DatabaseTableView filtering a column no index covers', { timeout: 30_0 await filterBy('name', 'John'); await vi.waitFor(() => expect(instance.searches).toHaveLength(1)); - expect(confirm).not.toHaveBeenCalled(); + expect(scanQuestion()).toBeNull(); expect(instance.searches[0].allowFullScan).toBeUndefined(); }); it('reads *x as ends-with and *x* as contains, and asks before either', async () => { - confirm.mockReturnValue(true); renderView(); await filterBy('name', '*hn'); + expect((await findScanQuestion()).textContent).toContain('"name"'); + answer('Scan anyway'); await vi.waitFor(() => expect(instance.searches).toHaveLength(1)); - expect(confirm.mock.calls[0][0]).toContain('"name"'); expect(instance.searches[0]).toMatchObject({ conditions: [{ search_attribute: 'name', search_type: 'ends_with', search_value: 'hn' }], allowFullScan: true, @@ -200,8 +202,9 @@ describe('DatabaseTableView filtering a column no index covers', { timeout: 30_0 fireEvent.change(filterInput('name'), { target: { value: '*oh*' } }); fireEvent.keyDown(filterInput('name'), { key: 'Enter' }); + await findScanQuestion(); + answer('Scan anyway'); await vi.waitFor(() => expect(instance.searches).toHaveLength(2)); - expect(confirm).toHaveBeenCalledTimes(2); expect(instance.searches[1].conditions).toEqual([ { search_attribute: 'name', search_type: 'contains', search_value: 'oh' }, ]); @@ -234,21 +237,91 @@ describe('DatabaseTableView filtering a column no index covers', { timeout: 30_0 it('asks once for a deep link’s filter, and runs it on yes', async () => { route.search = { filters: { note: 'beta' } }; - confirm.mockReturnValue(true); renderView(); + await findScanQuestion(); + expect(instance.searches).toEqual([]); + answer('Scan anyway'); + await vi.waitFor(() => expect(instance.searches).toHaveLength(1)); await findCell('t2'); - expect(instance.searches).toHaveLength(1); expect(instance.searches[0].allowFullScan).toBe(true); - expect(confirm).toHaveBeenCalledTimes(1); + expect(scanQuestion()).toBeNull(); + await vi.waitFor(() => expect(screen.queryByRole('button', { name: /apply filters/i })).toBeNull()); + }); + + it('shows exactly the deep link’s filter once it runs, not one typed before it', async () => { + const { rerender } = renderView(); + await filterBy('name', 'John'); + await vi.waitFor(() => expect(instance.searches).toHaveLength(1)); + + route.search = { filters: { note: 'beta' } }; + rerender( + + + , + ); + await findScanQuestion(); + answer('Scan anyway'); + + await vi.waitFor(() => expect(instance.searches).toHaveLength(2)); + expect(instance.searches[1].conditions).toEqual([ + { search_attribute: 'note', search_type: 'equals', search_value: 'beta' }, + ]); + await vi.waitFor(() => expect(filterInput('note').value).toBe('beta')); + expect(filterInput('name').value).toBe(''); + expect(screen.queryByRole('button', { name: /apply filters/i })).toBeNull(); + }); + + it('keeps no earlier filter in the inputs when the deep link’s question is declined', async () => { + const { rerender } = renderView(); + await filterBy('name', 'John'); + await vi.waitFor(() => expect(instance.searches).toHaveLength(1)); + + route.search = { filters: { note: 'beta' } }; + rerender( + + + , + ); + await findScanQuestion(); + answer('Cancel'); + + await vi.waitFor(() => expect(filterInput('note').value).toBe('beta')); + expect(filterInput('name').value).toBe(''); + fireEvent.click(screen.getByRole('button', { name: /apply filters/i })); + await findScanQuestion(); + answer('Scan anyway'); + await vi.waitFor(() => expect(instance.searches).toHaveLength(2)); + expect(instance.searches[1].conditions).toEqual([ + { search_attribute: 'note', search_type: 'equals', search_value: 'beta' }, + ]); + }); + + // The view stays mounted across deep links, so a yes to one must not apply it to the next. + it('drops the question for a deep link the user has since left', async () => { + route.search = { filters: { note: 'beta' } }; + const { rerender } = renderView(); + await findScanQuestion(); + + route.search = {}; + rerender( + + + , + ); + + expect(scanQuestion()).toBeNull(); + await findCell('t3'); + expect(instance.searches).toEqual([]); }); it('asks before a deep link’s filter runs too', async () => { route.search = { filters: { note: 'beta' } }; - confirm.mockReturnValue(false); renderView(); + await findScanQuestion(); + fireEvent.keyDown(scanQuestion()!, { key: 'Escape' }); - await vi.waitFor(() => expect(confirm).toHaveBeenCalledTimes(1)); + expect(scanQuestion()).toBeNull(); await findCell('t3'); expect(instance.searches).toEqual([]); expect(filterInput('note').value).toBe('beta'); diff --git a/src/features/instance/databases/components/DatabaseTableView.test.tsx b/src/features/instance/databases/components/DatabaseTableView.test.tsx index 4b95e4963..754d1c89c 100644 --- a/src/features/instance/databases/components/DatabaseTableView.test.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.test.tsx @@ -19,17 +19,15 @@ const permissionState = vi.hoisted(() => ({ canDeleteRecords: true, })); -vi.mock('@tanstack/react-router', () => { - // Stable references: the component keys effects off these objects' identity, and the real - // router hooks only produce a new one when params/search actually change. - const params = {}; - const search = {}; - return { - useParams: () => params, - useSearch: () => search, - Link: ({ children }: { children?: React.ReactNode }) => <>{children}, - }; -}); +// Stable references: the component keys effects off these objects' identity, and the real +// router hooks only produce a new one when params/search actually change. +const routerState = vi.hoisted(() => ({ params: {} as Record, search: {} })); + +vi.mock('@tanstack/react-router', () => ({ + useParams: () => routerState.params, + useSearch: () => routerState.search, + Link: ({ children }: { children?: React.ReactNode }) => <>{children}, +})); const instanceClientState = vi.hoisted(() => ({ entityId: 'instance-1' })); @@ -41,8 +39,15 @@ vi.mock('@/config/useInstanceClient', () => ({ }), })); +const staffState = vi.hoisted(() => ({ isInstanceOperator: false })); + vi.mock('@/hooks/useAuth', () => ({ - useStaffPermission: () => false, + useStaffPermission: () => staffState.isInstanceOperator, +})); + +const cleanupOrphanBlobs = vi.hoisted(() => ({ mutate: vi.fn(), isPending: false })); +vi.mock('@/integrations/api/instance/database/cleanupOrphanBlobs', () => ({ + useCleanupOrphanBlobsMutation: () => ({ mutate: cleanupOrphanBlobs.mutate, isPending: cleanupOrphanBlobs.isPending }), })); vi.mock('@/hooks/usePermissions', () => ({ @@ -168,9 +173,16 @@ afterEach(() => { editorProps.syntheticAttributes = undefined; watchedValues.calls = []; deleteRecords.mutate.mockReset(); + cleanupOrphanBlobs.mutate.mockReset(); + cleanupOrphanBlobs.isPending = false; + routerState.params = {}; + staffState.isInstanceOperator = false; vi.restoreAllMocks(); }); +const question = (name: string | RegExp) => screen.queryByRole('alertdialog', { name }); +const answer = (name: string) => fireEvent.click(screen.getByRole('button', { name })); + const dogTable = { attributes: [{ attribute: 'id', type: 'string', is_primary_key: true, indexed: true }], primary_key: 'id', @@ -355,12 +367,15 @@ describe('DatabaseTableView bulk delete', () => { }); it('deletes every selected key once confirmed, then clears the selection', () => { - vi.spyOn(window, 'confirm').mockReturnValue(true); renderView(); selectRecords(['abc', 'def']); fireEvent.click(deleteSelectedButton()!); + expect(deleteRecords.mutate).not.toHaveBeenCalled(); + expect(question('Delete 2 records from "dog"?')).not.toBeNull(); + answer('Delete'); + expect(question(/^Delete/)).toBeNull(); expect(deleteRecords.mutate).toHaveBeenCalledTimes(1); const [payload, handlers] = deleteRecords.mutate.mock.calls[0]; // Raw primary-key values, not stringified row ids -- `delete` addresses records by the value @@ -373,12 +388,13 @@ describe('DatabaseTableView bulk delete', () => { }); it('does not delete anything when the confirmation is declined', () => { - vi.spyOn(window, 'confirm').mockReturnValue(false); renderView(); selectRecords(['abc']); fireEvent.click(deleteSelectedButton()!); + answer('Cancel'); + expect(question(/^Delete/)).toBeNull(); expect(deleteRecords.mutate).not.toHaveBeenCalled(); // The selection survives a declined confirmation -- the user backed out of the delete, not // out of the rows they had picked. @@ -396,7 +412,6 @@ describe('DatabaseTableView selection scope', () => { const deleteSelectedButton = () => screen.queryByRole('button', { name: /Delete Selected/ }); it('arms the delete with every key the grid reports as selectable', () => { - vi.spyOn(window, 'confirm').mockReturnValue(true); renderView(); // Select-all hands the grid's page keys straight through, so the delete is aimed at the same @@ -405,6 +420,7 @@ describe('DatabaseTableView selection scope', () => { expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (3)'); fireEvent.click(deleteSelectedButton()!); + answer('Delete'); expect(deleteRecords.mutate.mock.calls[0][0]).toMatchObject({ hashValues: [1, 2, 3] }); @@ -472,13 +488,13 @@ describe('DatabaseTableView selection scope', () => { it('reports a delete the server only partly applied instead of claiming success', () => { // `delete` answers 200 while naming the records it couldn't address. Reporting that as a clean // success is what #1643 was about on the update path. - vi.spyOn(window, 'confirm').mockReturnValue(true); const warned = vi.spyOn(toast, 'error').mockImplementation(() => ''); const succeeded = vi.spyOn(toast, 'success').mockImplementation(() => ''); renderView(); act(() => tableViewSelection.current!.toggleAll(['a', 'b'], true)); fireEvent.click(deleteSelectedButton()!); + answer('Delete'); act(() => deleteRecords.mutate.mock.calls[0][1].onSuccess({ deleted_hashes: ['a'], skipped_hashes: ['b'] })); expect(succeeded).not.toHaveBeenCalled(); @@ -486,13 +502,13 @@ describe('DatabaseTableView selection scope', () => { }); it('treats an unreadable delete answer as unproven rather than successful', () => { - vi.spyOn(window, 'confirm').mockReturnValue(true); const warned = vi.spyOn(toast, 'error').mockImplementation(() => ''); const succeeded = vi.spyOn(toast, 'success').mockImplementation(() => ''); renderView(); act(() => tableViewSelection.current!.toggleRow('a')); fireEvent.click(deleteSelectedButton()!); + answer('Delete'); // Present but not an array: that responder does answer `delete`, and its answer is unreadable. act(() => deleteRecords.mutate.mock.calls[0][1].onSuccess({ skipped_hashes: null })); @@ -525,7 +541,6 @@ describe('DatabaseTableView selection follows the rows on screen', () => { }); it('deletes only the keys still on the page', () => { - vi.spyOn(window, 'confirm').mockReturnValue(true); const { rerender } = renderView({ rows: [{ id: 'abc' }, { id: 'def' }] }); act(() => tableViewSelection.current!.toggleAll(['abc', 'def'], true)); expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (2)'); @@ -539,6 +554,7 @@ describe('DatabaseTableView selection follows the rows on screen', () => { expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (1)'); fireEvent.click(deleteSelectedButton()!); + answer('Delete'); // 'abc' is still in the stored set, but it is no longer a row anyone can see. expect(deleteRecords.mutate.mock.calls[0][0]).toMatchObject({ hashValues: ['def'] }); @@ -598,3 +614,97 @@ describe('DatabaseTableView row editor write exclusions', () => { expect(editorProps.syntheticAttributes).toEqual(['tracks']); }); }); + +describe('DatabaseTableView asking before an action', () => { + it('cleans up orphan blobs only after the user agrees, once', () => { + staffState.isInstanceOperator = true; + routerState.params = { instanceId: 'instance-1' }; + renderView(); + + openTableOptions(); + fireEvent.click(screen.getByRole('menuitem', { name: 'Cleanup Orphan Blobs' })); + expect(question('Clean up orphan blobs?')).not.toBeNull(); + answer('Cancel'); + expect(cleanupOrphanBlobs.mutate).not.toHaveBeenCalled(); + // Opened from a menu: closing it must not leave the page unclickable. + expect(document.body.style.pointerEvents).not.toBe('none'); + + openTableOptions(); + fireEvent.click(screen.getByRole('menuitem', { name: 'Cleanup Orphan Blobs' })); + answer('Clean Up'); + + expect(cleanupOrphanBlobs.mutate).toHaveBeenCalledTimes(1); + expect(cleanupOrphanBlobs.mutate.mock.calls[0][0]).toMatchObject({ databaseName: 'data' }); + expect(document.body.style.pointerEvents).not.toBe('none'); + }); + + it('does not ask again while a cleanup is already running', () => { + staffState.isInstanceOperator = true; + routerState.params = { instanceId: 'instance-1' }; + cleanupOrphanBlobs.isPending = true; + renderView(); + + openTableOptions(); + const cleanup = screen.getByRole('menuitem', { name: 'Cleanup Orphan Blobs' }); + expect(cleanup.getAttribute('aria-disabled')).toBe('true'); + fireEvent.click(cleanup); + fireEvent.keyDown(cleanup, { key: 'Enter' }); + + expect(question('Clean up orphan blobs?')).toBeNull(); + expect(cleanupOrphanBlobs.mutate).not.toHaveBeenCalled(); + }); + + // The view stays mounted when the route moves to another table, so a question asked about one + // must not be answerable once the view shows the other. + it('drops a question once the view is showing another table, and does not bring it back', () => { + staffState.isInstanceOperator = true; + routerState.params = { instanceId: 'instance-1' }; + const { rerender } = renderView(); + const show = (databaseName: string, tableName: string) => + rerender( + + + , + ); + const ask = () => { + openTableOptions(); + fireEvent.click(screen.getByRole('menuitem', { name: 'Cleanup Orphan Blobs' })); + expect(question('Clean up orphan blobs?')).not.toBeNull(); + }; + + ask(); + show('data', 'cat'); + expect(question('Clean up orphan blobs?')).toBeNull(); + show('data', 'dog'); + expect(question('Clean up orphan blobs?')).toBeNull(); + + ask(); + show('other', 'dog'); + expect(question('Clean up orphan blobs?')).toBeNull(); + show('data', 'dog'); + + ask(); + instanceClientState.entityId = 'instance-2'; + show('data', 'dog'); + expect(question('Clean up orphan blobs?')).toBeNull(); + expect(cleanupOrphanBlobs.mutate).not.toHaveBeenCalled(); + }); + + it('drops the delete question once its rows leave the screen', () => { + const { rerender } = renderView({ rows: [{ id: 'abc' }, { id: 'def' }] }); + act(() => tableViewSelection.current!.toggleRow('abc')); + fireEvent.click(screen.getByRole('button', { name: /Delete Selected/ })); + expect(question('Delete 1 record from "dog"?')).not.toBeNull(); + + // A focus refetch, or another writer's record, moving the row onto the next page. + pageRows.current = [{ id: 'def' }, { id: 'ghi' }]; + rerender( + + + , + ); + + expect(question(/^Delete/)).toBeNull(); + expect(deleteRecords.mutate).not.toHaveBeenCalled(); + }); +}); diff --git a/src/features/instance/databases/components/DatabaseTableView.tsx b/src/features/instance/databases/components/DatabaseTableView.tsx index 5c3800fe0..77f86cdb7 100644 --- a/src/features/instance/databases/components/DatabaseTableView.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.tsx @@ -1,4 +1,15 @@ +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from '@/components/ui/alertDialog'; import { Button } from '@/components/ui/button'; +import { buttonVariants } from '@/components/ui/buttonVariants'; import { DropdownMenu, DropdownMenuContent, @@ -7,6 +18,12 @@ import { DropdownMenuTrigger, } from '@/components/ui/dropdownMenu'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; +import { + declaredRelationships, + describedDatabase, + describedTable, + describedTableNames, +} from '@/features/instance/databases/functions/describedTable'; import { ExtraRecordAttribute, extraRecordAttributes, @@ -16,7 +33,7 @@ import { browsePrimaryKey, formatBrowseDataTableHeader, } from '@/features/instance/databases/functions/formatBrowseDataTableHeader'; -import { fullScanAttributes, fullScanWarning } from '@/features/instance/databases/functions/fullScanFilter'; +import { FullScan, fullScanAttributes, fullScanQuestion } from '@/features/instance/databases/functions/fullScanFilter'; import { randomizableAttributes } from '@/features/instance/databases/functions/generateRandomRecords'; import { buildSyntheticGetAttributes, @@ -107,6 +124,16 @@ const EMPTY_SELECTION: ReadonlySet = new Set(); const NO_ROWS: Record[] = []; const NO_EXTRA_ATTRIBUTES: ExtraRecordAttribute[] = []; +type FilterValues = Record; + +type PendingConfirmation = + & { context: string } + & ( + | { kind: 'fullScan'; conditions: SearchCondition[]; scans: FullScan[]; formValues: FilterValues } + | { kind: 'cleanupOrphanBlobs' } + | { kind: 'deleteSelected'; hashValues: unknown[] } + ); + export function DatabaseTableView({ instanceDatabaseMap, instanceDatabaseMapLoading = false, @@ -148,8 +175,8 @@ export function DatabaseTableView({ // describe_table backfills this table's count asynchronously. Render schema from whichever arrives first // -- the map is usually ready before describe_table -- so columns and records are never gated on the // (slower) count scan. describe_table wins once present: it's the per-table, refresh-invalidated copy. - const databaseTables = instanceDatabaseMap?.[databaseName]; - const tableFromMap = databaseTables?.[tableName]; + const databaseTables = describedDatabase(instanceDatabaseMap, databaseName); + const tableFromMap = describedTable(instanceDatabaseMap, databaseName, tableName); const instanceTable = describeTableData ?? tableFromMap; // What the empty state may actually invite, each gated on the source it would use rather than on @@ -174,7 +201,7 @@ export function DatabaseTableView({ const hasReplaceGrants = useInstanceTablePutPermission(instanceId ?? clusterId, databaseName, tableName); const replaceBlockedReason = replaceRecordsBlockedReason(registrationInfo?.version, hasReplaceGrants); const canReplaceRecords = replaceBlockedReason === undefined; - const schemaRelationships = schemaRelationshipMap?.[databaseName]?.[tableName]; + const schemaRelationships = declaredRelationships(schemaRelationshipMap, databaseName, tableName); // Read the attributes from the same map the modal's Random Data option reads (NOT the // describe-first `instanceTable`): where the two disagree, offering Seed reopens the empty-dropdown // dead end this gate exists to close. @@ -226,8 +253,8 @@ export function DatabaseTableView({ // answers `false` when it simply doesn't know, which would offer an irreversible action on a // guess): dropping a table needs positive evidence that another one remains. const canDropTable = useMemo( - () => canManageBrowseInstance && !!databaseTables && Object.keys(databaseTables).length > 1, - [canManageBrowseInstance, databaseTables], + () => canManageBrowseInstance && describedTableNames(instanceDatabaseMap, databaseName).length > 1, + [canManageBrowseInstance, instanceDatabaseMap, databaseName], ); const { toggled: filtersToggled, toggleOn: showFilters, toggleOff: hideFilters } = useToggler(false); @@ -273,23 +300,47 @@ export function DatabaseTableView({ return conditions; }, [attributesMap, relationshipInfoMap]); + const { filters: urlFilters }: { filters?: Record } = useSearch({ strict: false }); + const urlFiltersKey = JSON.stringify([databaseName, tableName, urlFilters ?? null]); + const confirmationContext = JSON.stringify([instanceParams.entityId, urlFiltersKey]); + const [pendingConfirmation, setPendingConfirmation] = useState(null); + if (pendingConfirmation && pendingConfirmation.context !== confirmationContext) { + setPendingConfirmation(null); + } + + // Empty rather than left out: an input left out of a reset falls back to an earlier filter's value. + const { getValues: getFilterValues } = columnFiltersForm; + const onlyThese = useCallback( + (formValues: FilterValues) => ({ + ...Object.fromEntries(Object.keys(getFilterValues()).map((name) => [name, ''])), + ...formValues, + }), + [getFilterValues], + ); + const commitSearchConditions = useCallback( + (conditions: SearchCondition[], allowFullScan: boolean, formValues: FilterValues) => { + setFullScanAllowed(allowFullScan); + setAppliedSearchConditions(conditions.length ? conditions : null); + resetFiltersForm(onlyThese(formValues)); + }, + [setFullScanAllowed, setAppliedSearchConditions, onlyThese, resetFiltersForm], + ); + // The only way a filter is applied, typed or deep-linked, so one that no index can answer never runs // without the user agreeing to read the whole table. - const applySearchConditions = useCallback((conditions: SearchCondition[]) => { - const scanned = fullScanAttributes(conditions, attributesMap, relationshipInfoMap); - if (scanned.length && !confirm(fullScanWarning(tableName, scanned))) { + const applySearchConditions = useCallback((conditions: SearchCondition[], formValues: FilterValues) => { + const scans = fullScanAttributes(conditions, attributesMap, relationshipInfoMap); + if (scans.length) { + setPendingConfirmation({ kind: 'fullScan', context: confirmationContext, conditions, scans, formValues }); return false; } - setFullScanAllowed(scanned.length > 0); - setAppliedSearchConditions(conditions.length ? conditions : null); + commitSearchConditions(conditions, false, formValues); return true; - }, [attributesMap, relationshipInfoMap, tableName, setFullScanAllowed, setAppliedSearchConditions]); + }, [attributesMap, relationshipInfoMap, confirmationContext, commitSearchConditions]); const applyFilters = useCallback(() => { - if (applySearchConditions(translateFilterValues(columnFiltersValues))) { - resetFiltersForm({ ...columnFiltersValues }); - } - }, [applySearchConditions, translateFilterValues, resetFiltersForm, columnFiltersValues]); + applySearchConditions(translateFilterValues(columnFiltersValues), columnFiltersValues); + }, [applySearchConditions, translateFilterValues, columnFiltersValues]); const clearFilters = useCallback(() => { // Note sure why we need to resetFiltersForm twice here... resetFiltersForm({}, { keepValues: false, keepDirtyValues: false, keepDefaultValues: false }); @@ -311,8 +362,6 @@ export function DatabaseTableView({ // When the URL filters go away (navigating to another table, or back to the bare table), the filter // inputs are cleared too so they don't linger stale — `appliedSearchConditions` already resets on // table change, and the visible inputs should stay in step with it. - const { filters: urlFilters }: { filters?: Record } = useSearch({ strict: false }); - const urlFiltersKey = JSON.stringify([databaseName, tableName, urlFilters ?? null]); const appliedUrlFiltersKey = useRef(null); useEffect(function syncFiltersFromUrl() { if ( @@ -325,11 +374,9 @@ export function DatabaseTableView({ const isInitialSync = appliedUrlFiltersKey.current === null; appliedUrlFiltersKey.current = urlFiltersKey; if (urlFilters) { - if (applySearchConditions(translateFilterValues(urlFilters))) { - resetFiltersForm({ ...urlFilters }); - } else { - // Kept dirty, so Apply Filters stays on offer. - resetFiltersForm({ ...urlFilters }, { keepDefaultValues: true }); + if (!applySearchConditions(translateFilterValues(urlFilters), urlFilters)) { + // Kept dirty until the user agrees, so Apply Filters stays on offer if they don't. + resetFiltersForm(onlyThese(urlFilters), { keepDefaultValues: true }); } showFilters(); } else if (!isInitialSync) { @@ -344,6 +391,7 @@ export function DatabaseTableView({ schemaRelationshipsLoading, instanceDatabaseMapLoading, resetFiltersForm, + onlyThese, translateFilterValues, applySearchConditions, showFilters, @@ -637,11 +685,10 @@ export function DatabaseTableView({ [queryClient, instanceParams.entityId, databaseName, tableName], ); - const onCleanupOrphanBlobs = useCallback(async () => { - if (!confirm(`Are you sure you want to cleanup orphan blobs for database "${databaseName}"?`)) { - return; - } - + const onCleanupOrphanBlobs = useCallback(() => { + setPendingConfirmation({ kind: 'cleanupOrphanBlobs', context: confirmationContext }); + }, [confirmationContext]); + const cleanupOrphanBlobsNow = useCallback(() => { cleanupOrphanBlobs({ ...instanceParams, databaseName, @@ -785,13 +832,19 @@ export function DatabaseTableView({ // Bulk delete from the toolbar. Unlike the editor's single-record delete there is no record in // front of the user to check against, so it confirms first; both read the answer the same way. const onDeleteSelected = useCallback(() => { - const hashValues = visibleSelectedKeys; - if (!hashValues.length) { - return; - } - if (!confirm(`Permanently delete ${pluralize(hashValues.length, 'record', 'records')} from "${tableName}"?`)) { - return; + if (visibleSelectedKeys.length) { + setPendingConfirmation({ kind: 'deleteSelected', context: confirmationContext, hashValues: visibleSelectedKeys }); } + }, [visibleSelectedKeys, confirmationContext]); + // The question names the rows on screen; once a refetch moves them it names rows nobody can see. + if ( + pendingConfirmation?.kind === 'deleteSelected' + && (pendingConfirmation.hashValues.length !== visibleSelectedKeys.length + || pendingConfirmation.hashValues.some((key) => !visibleSelectedKeys.includes(key))) + ) { + setPendingConfirmation(null); + } + const deleteSelectedNow = useCallback((hashValues: unknown[]) => { deleteTableRecords( { ...instanceParams, @@ -815,15 +868,24 @@ export function DatabaseTableView({ }, }, ); - }, [ - deleteTableRecords, - instanceParams, - databaseName, - tableName, - refreshTable, - refreshOpenRecord, - visibleSelectedKeys, - ]); + }, [deleteTableRecords, instanceParams, databaseName, tableName, refreshTable, refreshOpenRecord]); + + const onConfirmed = () => { + const confirmation = pendingConfirmation; + setPendingConfirmation(null); + switch (confirmation?.kind) { + case 'fullScan': + commitSearchConditions(confirmation.conditions, true, confirmation.formValues); + return; + case 'cleanupOrphanBlobs': + cleanupOrphanBlobsNow(); + return; + case 'deleteSelected': + deleteSelectedNow(confirmation.hashValues); + return; + } + }; + const confirmationCopy = pendingConfirmation && describeConfirmation(pendingConfirmation, databaseName, tableName); // Point the editor at a record on the page the grid is showing. The row is kept as well as its // id because the editor falls back to it when the record can't be fetched by that id. @@ -1101,7 +1163,7 @@ export function DatabaseTableView({ && !!instanceId && ( @@ -1204,6 +1266,59 @@ export function DatabaseTableView({ isUpdateTableRecordsPending={isUpdateTableRecordsPending || isPutTableRecordsPending} isDeleteTableRecordsPending={isDeleteTableRecordsPending} /> + { + if (!open) { + setPendingConfirmation(null); + } + }} + > + {confirmationCopy && ( + + + {confirmationCopy.title} + {confirmationCopy.description} + + + Cancel + + {confirmationCopy.action} + + + + )} + ); } + +function describeConfirmation( + confirmation: PendingConfirmation, + databaseName: string, + tableName: string, +): { title: string; description: string; action: string; variant?: 'destructive' | 'warning' } { + switch (confirmation.kind) { + case 'fullScan': + return { ...fullScanQuestion(tableName, confirmation.scans), action: 'Scan anyway' }; + case 'cleanupOrphanBlobs': + return { + title: 'Clean up orphan blobs?', + description: `This starts an orphan blob cleanup for the "${databaseName}" database.`, + action: 'Clean Up', + variant: 'warning', + }; + case 'deleteSelected': { + const records = pluralize(confirmation.hashValues.length, 'record', 'records'); + return { + title: `Delete ${records} from "${tableName}"?`, + description: `The ${records} will be permanently deleted.`, + action: 'Delete', + variant: 'destructive', + }; + } + } +} diff --git a/src/features/instance/databases/components/DatabasesTree/buildItems.test.ts b/src/features/instance/databases/components/DatabasesTree/buildItems.test.ts index bedcf4798..1add35ff7 100644 --- a/src/features/instance/databases/components/DatabasesTree/buildItems.test.ts +++ b/src/features/instance/databases/components/DatabasesTree/buildItems.test.ts @@ -37,6 +37,12 @@ describe('buildItems', () => { expect(items['empty'].children).toEqual([]); }); + it('lists only databases the map has a describe for', () => { + const { items } = buildItems({ ...map, hole: null } as unknown as InstanceDatabaseMap, { canManage: true }); + expect(items[rootId].children).toEqual([createTableId, 'alpha', 'beta', 'empty']); + expect(items['hole']).toBeUndefined(); + }); + it('gives tables composite ids so duplicate names across databases do not collide', () => { const { items } = buildItems(map, { canManage: true }); expect(items['alpha/shared'].data).toEqual({ kind: 'table', databaseName: 'alpha', tableName: 'shared' }); diff --git a/src/features/instance/databases/components/DatabasesTree/buildItems.ts b/src/features/instance/databases/components/DatabasesTree/buildItems.ts index 64576655e..8dc1e562c 100644 --- a/src/features/instance/databases/components/DatabasesTree/buildItems.ts +++ b/src/features/instance/databases/components/DatabasesTree/buildItems.ts @@ -1,3 +1,4 @@ +import { describedDatabaseNames, describedTableNames } from '@/features/instance/databases/functions/describedTable'; import { InstanceDatabaseMap } from '@/integrations/api/api.patch'; import type { TreeItem } from 'react-complex-tree'; import { createTableId, rootId } from './specialItems'; @@ -28,10 +29,10 @@ export function buildItems( { canManage }: { canManage: boolean }, ): { items: Record>; rootId: string } { const items: Record> = {}; - const databaseNames = Object.keys(instanceDatabaseMap || {}).sort(); + const databaseNames = describedDatabaseNames(instanceDatabaseMap); for (const databaseName of databaseNames) { - const tableNames = Object.keys(instanceDatabaseMap?.[databaseName] || {}).sort(); + const tableNames = describedTableNames(instanceDatabaseMap, databaseName); items[databaseName] = { index: databaseName, isFolder: true, diff --git a/src/features/instance/databases/components/TableContextMenuItems.test.tsx b/src/features/instance/databases/components/TableContextMenuItems.test.tsx index d069bfa95..6ad7a1e98 100644 --- a/src/features/instance/databases/components/TableContextMenuItems.test.tsx +++ b/src/features/instance/databases/components/TableContextMenuItems.test.tsx @@ -32,12 +32,12 @@ function renderMenu(rolePermission: Record, dog: Record, instanceDatabaseMap: unknown) { +function renderMenuOver(rolePermission: Record, instanceDatabaseMap: unknown, tableName = 'dog') { permission.current = rolePermission as unknown as LocalRolePermission; const menu = (map: unknown) => ( - + ); @@ -109,9 +109,14 @@ describe('TableContextMenuItems on a table with no primary key', () => { }); describe('TableContextMenuItems on a table the map does not describe', () => { + const dropTable = () => screen.queryByRole('menuitem', { name: 'Drop Table' }); + // Two other tables, so the database's last table is not what withholds Drop Table. + const otherTables = { cat: keyedTable, cow: keyedTable }; + function expectNothingOffered() { expect(addRecords()).toBeNull(); expect(importData()).toBeNull(); + expect(dropTable()).toBeNull(); expect(exportCsv().hasAttribute('data-disabled')).toBe(true); fireEvent.click(exportCsv()); expect(exportCsvSpy).not.toHaveBeenCalled(); @@ -123,17 +128,23 @@ describe('TableContextMenuItems on a table the map does not describe', () => { }); it('offers nothing that needs its key when the map lacks the table', () => { - renderMenuOver({ super_user: true }, { data: { cat: keyedTable } }); + renderMenuOver({ super_user: true }, { data: otherTables }); + expectNothingOffered(); + }); + + it('reads only the map’s own tables, whatever the table is called', () => { + renderMenuOver({ super_user: true }, { data: otherTables }, 'constructor'); expectNothingOffered(); }); // Both menus list tables from the map, so an open one meets an undescribed table when a refetch // lands without it: another tab or user dropped it. it('withdraws them from an open menu once a refetched map drops the table', () => { - const { rerenderOver } = renderMenuOver({ super_user: true }, { data: { dog: keyedTable, cat: keyedTable } }); + const { rerenderOver } = renderMenuOver({ super_user: true }, { data: { dog: keyedTable, ...otherTables } }); expect(exportCsv().hasAttribute('data-disabled')).toBe(false); + expect(dropTable()).not.toBeNull(); - rerenderOver({ data: { cat: keyedTable } }); + rerenderOver({ data: otherTables }); expectNothingOffered(); }); }); diff --git a/src/features/instance/databases/components/TableContextMenuItems.tsx b/src/features/instance/databases/components/TableContextMenuItems.tsx index b2439aef4..d0aa53046 100644 --- a/src/features/instance/databases/components/TableContextMenuItems.tsx +++ b/src/features/instance/databases/components/TableContextMenuItems.tsx @@ -1,4 +1,9 @@ import { ContextMenuItem, ContextMenuSeparator } from '@/components/ui/contextMenu'; +import { + describedDatabase, + describedTable, + describedTableNames, +} from '@/features/instance/databases/functions/describedTable'; import { browsePrimaryKey } from '@/features/instance/databases/functions/formatBrowseDataTableHeader'; import { useExportTableCsv } from '@/features/instance/databases/hooks/useExportTableCsv'; import { @@ -28,8 +33,9 @@ export function TableContextMenuItems({ databaseName, tableName, instanceDatabas const canImport = useInstanceImportDataPermission(instanceId ?? clusterId, databaseName, tableName); const { exportCsv } = useExportTableCsv(); - const isLastTable = Object.keys(instanceDatabaseMap?.[databaseName] || {}).length <= 1; - const instanceTable = instanceDatabaseMap?.[databaseName]?.[tableName]; + const databaseTables = describedDatabase(instanceDatabaseMap, databaseName); + const isLastTable = describedTableNames(instanceDatabaseMap, databaseName).length <= 1; + const instanceTable = describedTable(instanceDatabaseMap, databaseName, tableName); const primaryKey = browsePrimaryKey(instanceTable); // Same withdrawal as the table view's toolbar: rows added here couldn't be listed there. const hasKnownPrimaryKey = !!primaryKey; @@ -41,7 +47,7 @@ export function TableContextMenuItems({ databaseName, tableName, instanceDatabas primaryKey, conditions: null, instanceTable, - databaseTables: instanceDatabaseMap?.[databaseName], + databaseTables, }); }; @@ -63,7 +69,7 @@ export function TableContextMenuItems({ databaseName, tableName, instanceDatabas Export CSV - {canManage && !isLastTable && ( + {canManage && instanceTable && !isLastTable && ( <> { + it('return the map’s own entries', () => { + expect(describedDatabase(map, 'data')).toEqual({ dog }); + expect(describedTable(map, 'data', 'dog')).toBe(dog); + expect(declaredRelationships(relationshipMap, 'data', 'dog')).toBe(relationships); + }); + + it('return nothing for a name the map lacks, or before the map loads', () => { + expect(describedDatabase(undefined, 'data')).toBeUndefined(); + expect(describedTable(undefined, 'data', 'dog')).toBeUndefined(); + expect(describedTable(map, 'data', 'cat')).toBeUndefined(); + expect(describedTable(map, 'other', 'dog')).toBeUndefined(); + expect(declaredRelationships(undefined, 'data', 'dog')).toBeUndefined(); + expect(ownEntry(null, 'data')).toBeUndefined(); + expect(ownEntry({ data: null }, 'data')).toBeUndefined(); + }); + + it('name the databases and tables that have a describe, sorted', () => { + const withHoles = { beta: { b: dog, a: dog, hole: null }, alpha: {}, hole: null } as unknown as InstanceDatabaseMap; + expect(describedDatabaseNames(withHoles)).toEqual(['alpha', 'beta']); + expect(describedDatabaseNames(undefined)).toEqual([]); + expect(describedTableNames(withHoles, 'beta')).toEqual(['a', 'b']); + expect(describedTableNames(withHoles, 'hole')).toEqual([]); + expect(describedTableNames(undefined, 'beta')).toEqual([]); + }); + + it('never find an Object.prototype member', () => { + expect(describedDatabase(map, 'constructor')).toBeUndefined(); + expect(describedTable(map, 'data', 'toString')).toBeUndefined(); + expect(describedTable(map, 'constructor', 'name')).toBeUndefined(); + expect(declaredRelationships(relationshipMap, 'data', 'constructor')).toBeUndefined(); + }); +}); diff --git a/src/features/instance/databases/functions/describedTable.ts b/src/features/instance/databases/functions/describedTable.ts new file mode 100644 index 000000000..ec7c4710b --- /dev/null +++ b/src/features/instance/databases/functions/describedTable.ts @@ -0,0 +1,41 @@ +import { SchemaRelationship, SchemaRelationshipMap } from '@/features/instance/databases/functions/schemaRelationships'; +import { InstanceDatabaseMap, InstanceDatabaseTableMap, InstanceTable } from '@/integrations/api/api.patch'; + +export function ownEntry(record: Readonly> | null | undefined, key: string): T | undefined { + return record && Object.hasOwn(record, key) ? (record[key] ?? undefined) : undefined; +} + +export function describedDatabase( + instanceDatabaseMap: InstanceDatabaseMap | undefined, + databaseName: string, +): InstanceDatabaseTableMap | undefined { + return ownEntry(instanceDatabaseMap, databaseName); +} + +export function describedDatabaseNames(instanceDatabaseMap: InstanceDatabaseMap | undefined): string[] { + return Object.keys(instanceDatabaseMap ?? {}).filter((name) => !!describedDatabase(instanceDatabaseMap, name)).sort(); +} + +export function describedTableNames( + instanceDatabaseMap: InstanceDatabaseMap | undefined, + databaseName: string, +): string[] { + const databaseTables = describedDatabase(instanceDatabaseMap, databaseName); + return Object.keys(databaseTables ?? {}).filter((name) => !!ownEntry(databaseTables, name)).sort(); +} + +export function describedTable( + instanceDatabaseMap: InstanceDatabaseMap | undefined, + databaseName: string, + tableName: string, +): InstanceTable | undefined { + return ownEntry(describedDatabase(instanceDatabaseMap, databaseName), tableName); +} + +export function declaredRelationships( + schemaRelationshipMap: SchemaRelationshipMap | undefined, + databaseName: string, + tableName: string, +): SchemaRelationship[] | undefined { + return ownEntry(ownEntry(schemaRelationshipMap, databaseName), tableName); +} diff --git a/src/features/instance/databases/functions/fullScanFilter.test.ts b/src/features/instance/databases/functions/fullScanFilter.test.ts index 631caa4c7..a21a0fc25 100644 --- a/src/features/instance/databases/functions/fullScanFilter.test.ts +++ b/src/features/instance/databases/functions/fullScanFilter.test.ts @@ -2,7 +2,7 @@ import { RelationshipAttributeInfo } from '@/features/instance/databases/functio import { InstanceAttribute } from '@/integrations/api/api.patch'; import { SearchCondition } from '@/integrations/api/instance/database/getSearchByConditions'; import { describe, expect, it } from 'vitest'; -import { fullScanAttributes, fullScanWarning } from './fullScanFilter'; +import { fullScanAttributes, fullScanQuestion } from './fullScanFilter'; // harperdb 4.7.40 describe_table for `type Thing @table { id: ID @primaryKey name: String @indexed // note: String category: String brand: Brand @relationship(from: brandId) }`: an indexed attribute @@ -66,19 +66,20 @@ describe('fullScanAttributes', () => { }); }); -describe('fullScanWarning', () => { +describe('fullScanQuestion', () => { it('names the attributes and the table they make Harper read', () => { - const message = fullScanWarning('Thing', [{ attribute: 'note' }, { attribute: 'category' }]); - expect(message).toContain('"note", "category"'); - expect(message).toContain('every record in "Thing" to'); - expect(message).toMatch(/Run it anyway\?$/); + const { title, description } = fullScanQuestion('Thing', [{ attribute: 'note' }, { attribute: 'category' }]); + expect(title).toBe('Scan every record in "Thing"?'); + expect(description).toContain('"note", "category"'); + expect(description).toContain('every record in "Thing" to'); }); it('names the related table for a join', () => { - const message = fullScanWarning('Thing', [ + const { title, description } = fullScanQuestion('Thing', [ { attribute: 'note' }, { attribute: 'brand.description', relatedTableName: 'Brand' }, ]); - expect(message).toContain('every record in "Thing" and "Brand"'); + expect(title).toBe('Scan every record in "Thing" and "Brand"?'); + expect(description).toContain('every record in "Thing" and "Brand"'); }); }); diff --git a/src/features/instance/databases/functions/fullScanFilter.ts b/src/features/instance/databases/functions/fullScanFilter.ts index ad8a9ae3d..8d5b0ad73 100644 --- a/src/features/instance/databases/functions/fullScanFilter.ts +++ b/src/features/instance/databases/functions/fullScanFilter.ts @@ -45,10 +45,17 @@ export function fullScanAttributes( return [...scans.values()]; } -export function fullScanWarning(tableName: string, scans: readonly FullScan[]): string { +export function fullScanQuestion( + tableName: string, + scans: readonly FullScan[], +): { title: string; description: string } { const attributes = scans.map(({ attribute }) => `"${attribute}"`).join(', '); const tables = [...new Set(scans.map((scan) => scan.relatedTableName ?? tableName))] .map((name) => `"${name}"`) .join(' and '); - return `No index can answer this filter on ${attributes}, so Harper will read every record in ${tables} to find the matches. On a large table that can take a long time and slow the server down for everyone using it.\n\nRun it anyway?`; + return { + title: `Scan every record in ${tables}?`, + description: + `No index can answer this filter on ${attributes}, so Harper will read every record in ${tables} to find the matches. On a large table that can take a long time and slow the server down for everyone using it.`, + }; } diff --git a/src/features/instance/databases/functions/relationshipAttributes.ts b/src/features/instance/databases/functions/relationshipAttributes.ts index a2809bea9..ac63ecb91 100644 --- a/src/features/instance/databases/functions/relationshipAttributes.ts +++ b/src/features/instance/databases/functions/relationshipAttributes.ts @@ -1,3 +1,4 @@ +import { ownEntry } from '@/features/instance/databases/functions/describedTable'; import { SchemaRelationship } from '@/features/instance/databases/functions/schemaRelationships'; import { InstanceAttribute, InstanceDatabaseTableMap, InstanceTable } from '@/integrations/api/api.patch'; @@ -152,7 +153,7 @@ export function getRelationshipInfoMap( } } for (const declared of schemaRelationships ?? []) { - const relatedTable = databaseTables?.[declared.relatedTableName]; + const relatedTable = ownEntry(databaseTables, declared.relatedTableName); const relatedPrimaryKey = relatedTable?.primary_key ?? relatedTable?.hash_attribute; if (!relatedTable || !relatedPrimaryKey || !databaseTables) { continue; diff --git a/src/features/instance/databases/functions/resolveDatabasesRedirect.test.ts b/src/features/instance/databases/functions/resolveDatabasesRedirect.test.ts index d412e5d3a..868406bc5 100644 --- a/src/features/instance/databases/functions/resolveDatabasesRedirect.test.ts +++ b/src/features/instance/databases/functions/resolveDatabasesRedirect.test.ts @@ -44,6 +44,27 @@ describe('resolveDatabasesRedirect', () => { expect(resolveDatabasesRedirect(map, { databaseName: 'alpha', tableName: 'widgets' })).toBeNull(); }); + it('treats a database or table named like an Object.prototype member as missing unless the map has it', () => { + expect(resolveDatabasesRedirect(map, { databaseName: 'constructor' })).toEqual({ + databaseName: 'alpha', + tableName: undefined, + }); + expect(resolveDatabasesRedirect(map, { databaseName: 'alpha', tableName: 'toString' })).toEqual({ + databaseName: 'alpha', + tableName: undefined, + }); + }); + + it('treats a database with no describe as missing rather than throwing', () => { + const withHole = { alpha: {}, broken: null } as unknown as InstanceDatabaseMap; + expect(resolveDatabasesRedirect(withHole, { databaseName: 'broken', tableName: 'orders' })).toEqual({ + databaseName: 'alpha', + tableName: undefined, + }); + const holeFirst = { aaa: null, beta: {} } as unknown as InstanceDatabaseMap; + expect(resolveDatabasesRedirect(holeFirst, {})).toEqual({ databaseName: 'beta', tableName: undefined }); + }); + it('stays put on an empty database (shows its overview, no redirect loop)', () => { expect(resolveDatabasesRedirect(map, { databaseName: 'empty' })).toBeNull(); }); diff --git a/src/features/instance/databases/functions/resolveDatabasesRedirect.ts b/src/features/instance/databases/functions/resolveDatabasesRedirect.ts index 02639a13b..8e20c1069 100644 --- a/src/features/instance/databases/functions/resolveDatabasesRedirect.ts +++ b/src/features/instance/databases/functions/resolveDatabasesRedirect.ts @@ -1,3 +1,8 @@ +import { + describedDatabase, + describedDatabaseNames, + describedTable, +} from '@/features/instance/databases/functions/describedTable'; import { InstanceDatabaseMap } from '@/integrations/api/api.patch'; /** @@ -17,9 +22,9 @@ export function resolveDatabasesRedirect( if (!instanceDatabaseMap) { return null; } - const databaseExists = !!(params.databaseName && instanceDatabaseMap[params.databaseName]); + const databaseExists = !!params.databaseName && !!describedDatabase(instanceDatabaseMap, params.databaseName); if (!databaseExists) { - const firstDatabaseName = Object.keys(instanceDatabaseMap).sort()[0]; + const firstDatabaseName = describedDatabaseNames(instanceDatabaseMap)[0]; // No redirect when there are no databases at all, or the fallback would land on the same // (nonexistent) name we're already on -- both would otherwise loop. if (firstDatabaseName && firstDatabaseName !== params.databaseName) { @@ -27,7 +32,7 @@ export function resolveDatabasesRedirect( } return null; } - if (params.tableName && !instanceDatabaseMap[params.databaseName!][params.tableName]) { + if (params.tableName && !describedTable(instanceDatabaseMap, params.databaseName!, params.tableName)) { return { databaseName: params.databaseName!, tableName: undefined }; } return null; diff --git a/src/features/instance/databases/functions/schemaRelationships.test.ts b/src/features/instance/databases/functions/schemaRelationships.test.ts index 5e27d110d..2339ccfe1 100644 --- a/src/features/instance/databases/functions/schemaRelationships.test.ts +++ b/src/features/instance/databases/functions/schemaRelationships.test.ts @@ -1,5 +1,6 @@ +import { QueryClient } from '@tanstack/react-query'; import { describe, expect, it } from 'vitest'; -import { parseSchemaRelationships } from './schemaRelationships'; +import { getSchemaRelationshipsQueryOptions, parseSchemaRelationships } from './schemaRelationships'; // The live anvils app schema this feature was built against. const anvilsSchema = `type Category @table @export @sealed { @@ -70,6 +71,65 @@ type Post @table(database: "blog") { ]); }); + it('keeps a database named __proto__ or constructor as its own entry, touching no prototype', () => { + const schemaIn = (database: string) => + `type Owner @table(database: "${database}") { + id: ID @primaryKey + pets: [Pet] @relationship(to: "ownerId") +} + +type Pet @table(database: "${database}") { + id: ID @primaryKey + ownerId: ID @indexed +} +`; + const map = parseSchemaRelationships([schemaIn('__proto__'), schemaIn('constructor')]); + + expect(Object.keys(map).sort()).toEqual(['__proto__', 'constructor']); + for (const database of ['__proto__', 'constructor']) { + expect(map[database].Owner[0]).toMatchObject({ attribute: 'pets', relatedTableName: 'Pet' }); + } + + const tableNamedProto = `type Owner @table(table: "__proto__") { + id: ID @primaryKey + pets: [Pet] @relationship(to: "ownerId") +} + +type Pet @table { + id: ID @primaryKey + ownerId: ID @indexed +} +`; + const tables = parseSchemaRelationships([tableNamedProto]).data; + expect(Object.keys(tables)).toEqual(['__proto__']); + expect(Object.getPrototypeOf(tables)).toBeNull(); + expect(({} as Record).Owner).toBeUndefined(); + expect((Object as unknown as Record).Owner).toBeUndefined(); + }); + + it('keeps a __proto__ database an entry across a refetch that changes the map', () => { + const ownerIn = (database: string, pets: string) => + `type Owner @table(database: "${database}") { + id: ID @primaryKey + ${pets}: [Pet] @relationship(to: "ownerId") +} + +type Pet @table(database: "${database}") { + id: ID @primaryKey + ownerId: ID @indexed +} +`; + const options = getSchemaRelationshipsQueryOptions({ entityId: 'instance-1' } as never); + const queryClient = new QueryClient(); + queryClient.getQueryCache().build(queryClient, options as never); + queryClient.setQueryData(options.queryKey, parseSchemaRelationships([ownerIn('__proto__', 'pets')])); + queryClient.setQueryData(options.queryKey, parseSchemaRelationships([ownerIn('__proto__', 'animals')])); + + const map = queryClient.getQueryData(options.queryKey)!; + expect(Object.hasOwn(map, '__proto__')).toBe(true); + expect(map['__proto__'].Owner[0]).toMatchObject({ attribute: 'animals' }); + }); + it('ignores unparseable sources and tables without relationships', () => { expect(parseSchemaRelationships(['type Broken @table {'])).toEqual({}); expect(parseSchemaRelationships(['type Plain @table { id: ID @primaryKey }'])).toEqual({}); diff --git a/src/features/instance/databases/functions/schemaRelationships.ts b/src/features/instance/databases/functions/schemaRelationships.ts index 5575c04be..f1dc745d0 100644 --- a/src/features/instance/databases/functions/schemaRelationships.ts +++ b/src/features/instance/databases/functions/schemaRelationships.ts @@ -41,6 +41,9 @@ export function getSchemaRelationshipsQueryOptions(params: InstanceClientIdConfi queryKey: [params.entityId, 'schema-relationships'] as const, staleTime: 60_000, retry: false, + // Structural sharing would rebuild changed levels as plain objects, where a `__proto__` key + // sets the prototype instead of staying an entry. + structuralSharing: false, enabled: params.enabled !== false, queryFn: async (): Promise => { // Best-effort: browse must keep working for users who can't read component files @@ -123,7 +126,9 @@ export function parseSchemaRelationships(sources: string[]): SchemaRelationshipM } } - const map: SchemaRelationshipMap = {}; + // Without a prototype, a database or table named `__proto__` or `constructor` is an ordinary key + // rather than a write into Object.prototype or onto Object itself. + const map: SchemaRelationshipMap = Object.create(null); for (const table of tables) { if (!table.relationships.length) { continue; @@ -140,7 +145,7 @@ export function parseSchemaRelationships(sources: string[]): SchemaRelationshipM } } if (resolved.length) { - (map[table.database] ??= {})[table.tableName] = resolved; + (map[table.database] ??= Object.create(null))[table.tableName] = resolved; } } return map; diff --git a/src/features/instance/databases/hooks/useExportTableCsv.ts b/src/features/instance/databases/hooks/useExportTableCsv.ts index 96fafe91b..c371f0348 100644 --- a/src/features/instance/databases/hooks/useExportTableCsv.ts +++ b/src/features/instance/databases/hooks/useExportTableCsv.ts @@ -1,4 +1,5 @@ import { useInstanceClientIdParams } from '@/config/useInstanceClient'; +import { declaredRelationships } from '@/features/instance/databases/functions/describedTable'; import { getSchemaRelationshipsQueryOptions } from '@/features/instance/databases/functions/schemaRelationships'; import { buildExportGetAttributes, @@ -78,7 +79,7 @@ export function useExportTableCsv() { records, instanceTable, databaseTables, - schemaRelationshipMap[databaseName]?.[tableName], + declaredRelationships(schemaRelationshipMap, databaseName, tableName), ); const blob = tableCsvBlob(records, columns); const url = URL.createObjectURL(blob); diff --git a/src/features/instance/databases/modals/EditTableRowModal.test.tsx b/src/features/instance/databases/modals/EditTableRowModal.test.tsx index e616efb55..221f459d6 100644 --- a/src/features/instance/databases/modals/EditTableRowModal.test.tsx +++ b/src/features/instance/databases/modals/EditTableRowModal.test.tsx @@ -556,6 +556,7 @@ describe('EditTableRowModal', () => { }); const previousButton = () => screen.getByRole('button', { name: 'Previous record' }); const nextButton = () => screen.getByRole('button', { name: 'Next record' }); + const discardQuestion = () => screen.queryByRole('alertdialog', { name: 'Discard your unsaved changes?' }); it('has no navigation when the caller has no result set to step through', () => { renderModal(); @@ -662,52 +663,88 @@ describe('EditTableRowModal', () => { // silently, the way Escape used to before it was blocked for the same reason. it('asks before a step discards unsaved edits, and stays put when refused', () => { const onNext = vi.fn(); - const confirmed = vi.spyOn(window, 'confirm').mockReturnValue(false); renderModal({ recordNavigation: navigation({ onNext }) }); edit('[{"name":"Grace"}]'); fireEvent.click(nextButton()); + expect(discardQuestion()).not.toBeNull(); + fireEvent.click(screen.getByRole('button', { name: 'Keep Editing' })); - expect(confirmed).toHaveBeenCalled(); + expect(discardQuestion()).toBeNull(); expect(onNext).not.toHaveBeenCalled(); - confirmed.mockRestore(); + expect((screen.getByTestId('editor') as HTMLTextAreaElement).value).toBe('[{"name":"Grace"}]'); }); it('steps once the discard is confirmed', () => { const onNext = vi.fn(); - const confirmed = vi.spyOn(window, 'confirm').mockReturnValue(true); renderModal({ recordNavigation: navigation({ onNext }) }); edit('[{"name":"Grace"}]'); fireEvent.click(nextButton()); + fireEvent.click(screen.getByRole('button', { name: 'Discard Changes' })); expect(onNext).toHaveBeenCalledTimes(1); - confirmed.mockRestore(); + expect(discardQuestion()).toBeNull(); + }); + + it('asks once for the arrow keys too, and Escape answers no without closing the editor', () => { + const onNext = vi.fn(); + const setIsModalOpen = vi.fn(); + renderModal({ recordNavigation: navigation({ onNext }), setIsModalOpen }); + + edit('[{"name":"Grace"}]'); + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowRight' }); + const question = discardQuestion()!; + // The question's own keys stay with it rather than stepping the record behind it. + fireEvent.keyDown(question, { key: 'ArrowRight' }); + fireEvent.keyDown(question, { key: 'Escape' }); + + expect(discardQuestion()).toBeNull(); + expect(onNext).not.toHaveBeenCalled(); + expect(setIsModalOpen).not.toHaveBeenCalled(); + }); + + // A refetch of the record resets the draft the question was about, so a yes must not step on + // the strength of edits that are already gone. + it('drops the question once the record it was about is replaced', () => { + const onNext = vi.fn(); + const recordNavigation = navigation({ onNext }); + const { rerender } = renderAddressableModal({ recordNavigation, data: [{ id: 'a', name: 'A' }] }); + + edit('[{"id":"a","name":"edited A"}]'); + fireEvent.click(nextButton()); + expect(discardQuestion()).not.toBeNull(); + rerender(modal({ primaryKey: 'id', recordNavigation, data: [{ id: 'a', name: 'A, saved elsewhere' }] })); + expect(discardQuestion()).toBeNull(); + + // Edits typed after that are a new draft, which the old question never asked about. + edit('[{"id":"a","name":"edited again"}]'); + expect(discardQuestion()).toBeNull(); + expect(onNext).not.toHaveBeenCalled(); }); it('does not ask when there is nothing to discard', () => { - const confirmed = vi.spyOn(window, 'confirm').mockReturnValue(true); - renderModal({ recordNavigation: navigation() }); + const onNext = vi.fn(); + renderModal({ recordNavigation: navigation({ onNext }) }); fireEvent.click(nextButton()); - expect(confirmed).not.toHaveBeenCalled(); - confirmed.mockRestore(); + expect(discardQuestion()).toBeNull(); + expect(onNext).toHaveBeenCalledTimes(1); }); // The record arriving after a step is a different record, not this one changing underfoot — // the warning for that would be describing something that never happened. it('does not warn about a changed record when the user stepped to another one', () => { - const confirmed = vi.spyOn(window, 'confirm').mockReturnValue(true); const recordNavigation = navigation(); const { rerender } = renderModal({ recordNavigation, data: [{ id: 'a', name: 'A' }] }); edit('[{"id":"a","name":"edited A"}]'); fireEvent.click(nextButton()); + fireEvent.click(screen.getByRole('button', { name: 'Discard Changes' })); rerender(modal({ recordNavigation, data: [{ id: 'b', name: 'B' }] })); expect(screen.queryByText('This record changed while you were editing it')).toBeNull(); - confirmed.mockRestore(); }); }); @@ -784,7 +821,6 @@ describe('EditTableRowModal', () => { // further step must not stop to ask (Escape is refused on the same flag). it('leaves nothing to discard while the next record is still loading', () => { const onNext = vi.fn(); - const confirmed = vi.spyOn(window, 'confirm').mockReturnValue(true); const recordNavigation = { position: 1, total: 3, @@ -798,9 +834,8 @@ describe('EditTableRowModal', () => { rerender(modal({ primaryKey: 'id', recordNavigation, data: undefined })); fireEvent.click(screen.getByRole('button', { name: 'Next record' })); - expect(confirmed).not.toHaveBeenCalled(); + expect(screen.queryByRole('alertdialog')).toBeNull(); expect(onNext).toHaveBeenCalledTimes(1); - confirmed.mockRestore(); }); // The echo is recognised by "we just pushed this", not by "the text matches the record": a diff --git a/src/features/instance/databases/modals/EditTableRowModal.tsx b/src/features/instance/databases/modals/EditTableRowModal.tsx index a698fa85e..aa884fa96 100644 --- a/src/features/instance/databases/modals/EditTableRowModal.tsx +++ b/src/features/instance/databases/modals/EditTableRowModal.tsx @@ -1,6 +1,17 @@ import { Loading } from '@/components/Loading'; import { Alert, AlertDescription, AlertTitle } from '@/components/ui/alert'; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from '@/components/ui/alertDialog'; import { Button } from '@/components/ui/button'; +import { buttonVariants } from '@/components/ui/buttonVariants'; import { Dialog, DialogContent, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog'; import { primaryKeyMismatch } from '@/features/instance/databases/functions/primaryKeyMismatch'; import { @@ -180,14 +191,28 @@ export function EditTableRowModal({ // dealt with first (Escape is blocked for the same reason). Dropping the draft here rather than // leaving it for the reset above also keeps that reset's warning meaning what it says: the // stored record moved, not the user stepping away from it. - const stepToRecord = (step: () => void) => { - if (madeChanges && !confirm('Discard your unsaved changes to this record?')) { + const takeStep = (direction: 'previous' | 'next') => { + const allowed = direction === 'previous' ? recordNavigation?.hasPrevious : recordNavigation?.hasNext; + if (!recordNavigation || !allowed || isWritePending) { return; } setUpdatedTableRecordData(undefined); setMadeChanges(false); setDiscardedUnsavedEdits(false); - step(); + (direction === 'previous' ? recordNavigation.onPrevious : recordNavigation.onNext)(); + }; + // The question is about this draft. A draft that is gone (the record was replaced, or the dialog + // closed) takes the question with it, so a yes can't discard edits made after it was asked. + const [pendingStep, setPendingStep] = useState<'previous' | 'next'>(); + if (pendingStep && !(isModalOpen && madeChanges)) { + setPendingStep(undefined); + } + const stepToRecord = (direction: 'previous' | 'next') => { + if (madeChanges) { + setPendingStep(direction); + return; + } + takeStep(direction); }; const onNavigationKeyDown = (event: KeyboardEvent) => { if ( @@ -197,16 +222,16 @@ export function EditTableRowModal({ ) { return; } - const step = event.key === 'ArrowLeft' || event.key === 'ArrowUp' - ? (recordNavigation.hasPrevious ? recordNavigation.onPrevious : undefined) + const direction = event.key === 'ArrowLeft' || event.key === 'ArrowUp' + ? (recordNavigation.hasPrevious ? 'previous' : undefined) : event.key === 'ArrowRight' || event.key === 'ArrowDown' - ? (recordNavigation.hasNext ? recordNavigation.onNext : undefined) + ? (recordNavigation.hasNext ? 'next' : undefined) : undefined; - if (!step) { + if (!direction) { return; } event.preventDefault(); - stepToRecord(step); + stepToRecord(direction); }; return ( @@ -340,8 +365,8 @@ export function EditTableRowModal({ stepToRecord(recordNavigation.onPrevious)} - onNext={() => stepToRecord(recordNavigation.onNext)} + onPrevious={() => stepToRecord('previous')} + onNext={() => stepToRecord('next')} /> )}
@@ -455,6 +480,39 @@ export function EditTableRowModal({
+ {/* Outside the editor's content, so its keys never reach the record navigation. */} + { + if (!open) { + setPendingStep(undefined); + } + }} + > + + + Discard your unsaved changes? + + Moving to another record replaces the editor's contents, so your edits to this record will be lost. + + + + Keep Editing + { + const direction = pendingStep; + setPendingStep(undefined); + if (direction) { + takeStep(direction); + } + }} + > + Discard Changes + + + + ); } diff --git a/src/features/instance/databases/modals/ImportDataModal.permissions.test.tsx b/src/features/instance/databases/modals/ImportDataModal.permissions.test.tsx index 6123f3b5d..f89be1703 100644 --- a/src/features/instance/databases/modals/ImportDataModal.permissions.test.tsx +++ b/src/features/instance/databases/modals/ImportDataModal.permissions.test.tsx @@ -143,4 +143,13 @@ describe('ImportDataModal name fields', () => { expect(screen.getByText('Maximum 75 characters reached.')).toBeTruthy(); expect(screen.getByText('Maximum 250 characters reached.')).toBeTruthy(); }); + + it('says a table named like an Object.prototype member will be created, as for any new name', () => { + renderModal({ super_user: true }); + expect(screen.queryByText('New table')).toBeNull(); + + fireEvent.change(screen.getByLabelText('Table'), { target: { value: 'constructor' } }); + + expect(screen.getByText('New table')).toBeTruthy(); + }); }); diff --git a/src/features/instance/databases/modals/ImportDataModal.tsx b/src/features/instance/databases/modals/ImportDataModal.tsx index 6a55d6699..f0462a8ca 100644 --- a/src/features/instance/databases/modals/ImportDataModal.tsx +++ b/src/features/instance/databases/modals/ImportDataModal.tsx @@ -28,6 +28,11 @@ import { } from '@/components/ui/select'; import { Separator } from '@/components/ui/separator'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; +import { + declaredRelationships, + describedDatabase, + describedTable, +} from '@/features/instance/databases/functions/describedTable'; import { generateRandomRecords, randomizableAttributes, @@ -193,15 +198,15 @@ export function ImportDataModal({ const watchedTable = form.watch('table'); const selectedDataset = sampleDatasets.find((dataset) => dataset.id === datasetId); - const targetTable = instanceDatabaseMap?.[watchedDatabase || 'data']?.[watchedTable]; + const targetTable = describedTable(instanceDatabaseMap, watchedDatabase || 'data', watchedTable); const tableExists = !!targetTable; const tableWillBeCreated = !!instanceDatabaseMap && !!watchedTable && !tableExists; // Random data needs existing columns to model values on, so the option only appears // when the target table already has some beyond the primary key and system fields. const fillableAttributes = randomizableAttributes( targetTable?.attributes, - instanceDatabaseMap?.[watchedDatabase || 'data'], - schemaRelationshipMap?.[watchedDatabase || 'data']?.[watchedTable], + describedDatabase(instanceDatabaseMap, watchedDatabase || 'data'), + declaredRelationships(schemaRelationshipMap, watchedDatabase || 'data', watchedTable), ); const canGenerateRandom = fillableAttributes.length > 0; const isRandomDataset = datasetId === RANDOM_DATASET_ID; @@ -256,9 +261,9 @@ export function ImportDataModal({ let source: ImportSource; if (values.method === 'sample' && values.datasetId === RANDOM_DATASET_ID) { const attributes = randomizableAttributes( - instanceDatabaseMap?.[database]?.[table]?.attributes, - instanceDatabaseMap?.[database], - schemaRelationshipMap?.[database]?.[table], + describedTable(instanceDatabaseMap, database, table)?.attributes, + describedDatabase(instanceDatabaseMap, database), + declaredRelationships(schemaRelationshipMap, database, table), ); if (attributes.length === 0) { form.setError('datasetId', { 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');