diff --git a/e2e/auth/session-expiry.spec.ts b/e2e/auth/session-expiry.spec.ts new file mode 100644 index 00000000..774afa6f --- /dev/null +++ b/e2e/auth/session-expiry.spec.ts @@ -0,0 +1,90 @@ +import { test, expect, type Page } from '../support/fixtures.js'; +import { waitForHydration } from '../support/helpers.js'; + +/** Issue a same-origin fetch from the page and wait for the session check it triggers. */ +async function fetchAndAwaitSessionCheck(page: Page, url: string) { + const sessionCheck = page.waitForResponse((res) => res.url().includes('/api/auth/get-session')); + const status = await page.evaluate(async (u) => (await fetch(u)).status, url); + await sessionCheck; + return status; +} + +test.describe('Session expiry', () => { + test.use({ locale: 'en-US' }); + + test.describe('without a session', () => { + test.use({ storageState: { cookies: [], origins: [] } }); + + test('API routes return a JSON 401', async ({ request }) => { + const res = await request.get('/api/trino/catalog?level=catalogs', { maxRedirects: 0 }); + expect(res.status()).toBe(401); + expect(await res.json()).toEqual({ message: 'Authentication required' }); + }); + }); + + test('shows a blocking modal when an API call fails after the session is gone', async ({ + page + }) => { + await page.goto('/trino?tab=1'); + await waitForHydration(page); + + const dialog = page.getByRole('dialog', { name: 'Session expired' }); + await expect(dialog).toBeHidden(); + + await page.context().clearCookies(); + const status = await fetchAndAwaitSessionCheck(page, '/api/trino/catalog?level=catalogs'); + expect(status).toBe(401); + + await expect(dialog).toBeVisible(); + await expect(dialog.getByText('Your session has expired.')).toBeVisible(); + + const signIn = dialog.getByRole('link', { name: 'Sign in again' }); + + // The modal cannot be dismissed. Press Escape twice: browsers may skip the + // cancellable `cancel` event on a repeated Escape and close the dialog. + await page.keyboard.press('Escape'); + await page.keyboard.press('Escape'); + await expect(dialog).toBeVisible(); + await expect(signIn).toBeFocused(); + + await expect(signIn).toHaveAttribute( + 'href', + '/auth/login?redirectTo=' + encodeURIComponent('/trino?tab=1') + ); + + // Signing in again returns the user to where they were. This relies on the + // mock OIDC provider signing in without a login form. + await signIn.click(); + await expect(page).toHaveURL(/\/auth\/login/); + await waitForHydration(page); + await page.getByRole('button', { name: /sign in with sso/i }).click(); + await expect(page).toHaveURL('/trino?tab=1'); + await expect(dialog).toBeHidden(); + }); + + test('shows the modal when the user returns to the tab after the session is gone', async ({ + page + }) => { + await page.goto('/'); + await waitForHydration(page); + + await page.context().clearCookies(); + const sessionCheck = page.waitForResponse((res) => res.url().includes('/api/auth/get-session')); + await page.evaluate(() => document.dispatchEvent(new Event('visibilitychange'))); + await sessionCheck; + + await expect(page.getByRole('dialog', { name: 'Session expired' })).toBeVisible(); + }); + + test('does not show the modal for a 401 while the session is valid', async ({ page }) => { + await page.goto('/'); + await waitForHydration(page); + + // The storage API answers 401 when no storage connection header is sent + // (requires the storage browser to be enabled, as in .env.test). + const status = await fetchAndAwaitSessionCheck(page, '/api/storage/buckets'); + expect(status).toBe(401); + + await expect(page.getByRole('dialog', { name: 'Session expired' })).toBeHidden(); + }); +}); diff --git a/e2e/trino/query-api.spec.ts b/e2e/trino/query-api.spec.ts index b9e3bf00..77878ad0 100644 --- a/e2e/trino/query-api.spec.ts +++ b/e2e/trino/query-api.spec.ts @@ -16,11 +16,11 @@ test.describe('Trino API', () => { '/api/trino/catalog?level=catalogs', '/api/trino/completion/metadata?level=catalogs' ]) { - test(`redirects ${path.split('?')[0]} to the login page`, async ({ page }) => { + test(`rejects ${path.split('?')[0]} with a JSON 401`, async ({ page }) => { const res = await page.request.get(path, { maxRedirects: 0 }); - expect(res.status()).toBe(302); - expect(res.headers()['location']).toMatch(/^\/auth\/login\?redirectTo=/); + expect(res.status()).toBe(401); + expect(await res.json()).toEqual({ message: 'Authentication required' }); }); } }); diff --git a/messages/de.json b/messages/de.json index 6452b783..5ea684c5 100644 --- a/messages/de.json +++ b/messages/de.json @@ -47,6 +47,9 @@ "auth_login_subtitle": "Verwenden Sie den SSO-Anbieter Ihrer Organisation, um auf die Plattform zuzugreifen.", "auth_login_button": "Mit SSO anmelden", "auth_login_error": "Anmeldung fehlgeschlagen. Bitte versuchen Sie es erneut.", + "session_expired_title": "Sitzung abgelaufen", + "session_expired_message": "Ihre Sitzung ist abgelaufen. Melden Sie sich erneut an, um fortzufahren.", + "session_expired_sign_in": "Erneut anmelden", "header_sign_out": "Abmelden", "trino_results_empty": "Keine Ergebnisse", "trino_query_error": "Abfragefehler", diff --git a/messages/en.json b/messages/en.json index 3d9d06a4..62ece893 100644 --- a/messages/en.json +++ b/messages/en.json @@ -47,6 +47,9 @@ "auth_login_subtitle": "Use your organisation's SSO provider to access the platform.", "auth_login_button": "Sign in with SSO", "auth_login_error": "Sign-in failed. Please try again.", + "session_expired_title": "Session expired", + "session_expired_message": "Your session has expired. Sign in again to continue.", + "session_expired_sign_in": "Sign in again", "header_sign_out": "Sign out", "trino_results_empty": "No results", "trino_query_error": "Query error", diff --git a/src/hooks.client.ts b/src/hooks.client.ts new file mode 100644 index 00000000..4de7dab2 --- /dev/null +++ b/src/hooks.client.ts @@ -0,0 +1,28 @@ +import type { ClientInit } from '@sveltejs/kit'; +import { checkSession } from '$lib/client/session.svelte'; + +/** + * Wrap `window.fetch` so that any same-origin 401 triggers a session check. + * A 401 does not always mean the session has expired (e.g. the storage API + * uses it for a missing connection), so the response itself is passed through + * unchanged and `checkSession` decides whether the session is really gone. + */ +export const init: ClientInit = () => { + const originalFetch = window.fetch; + window.fetch = async (input, requestInit) => { + const response = await originalFetch(input, requestInit); + if (response.status === 401 && isSameOrigin(response.url)) { + void checkSession(); + } + return response; + }; +}; + +function isSameOrigin(url: string): boolean { + if (!url) return false; + try { + return new URL(url).origin === window.location.origin; + } catch { + return false; + } +} diff --git a/src/hooks.server.ts b/src/hooks.server.ts index 8fb81a2b..086772ae 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -1,16 +1,11 @@ import { paraglideMiddleware } from '$lib/paraglide/server'; import { httpRequestDuration } from '$lib/server/metrics'; import { building, dev } from '$app/environment'; -import { - error, - redirect, - type Handle, - type HandleServerError, - type ServerInit -} from '@sveltejs/kit'; +import { error, type Handle, type HandleServerError, type ServerInit } from '@sveltejs/kit'; import { sequence } from '@sveltejs/kit/hooks'; import { svelteKitHandler } from 'better-auth/svelte-kit'; import { auth, oidcEnabled } from '$lib/server/auth'; +import { unauthenticatedResponse } from '$lib/server/auth-guard'; import { requestLogger, logger } from '$lib/server/logging'; import { getConnectionFromHeader } from '$lib/server/storage/connection.js'; import { STORAGE_CONNECTION_ID_HEADER } from '$lib/storage/connection-id-header.js'; @@ -62,8 +57,8 @@ const handleAuthGuard: Handle = async ({ event, resolve }) => { const isPublic = PUBLIC_PATHS.some((p) => event.url.pathname.startsWith(p)); if (!isPublic && !event.locals.user) { - const redirectTo = encodeURIComponent(event.url.pathname + event.url.search); - throw redirect(302, `/auth/login?redirectTo=${redirectTo}`); + event.locals.logger.debug({ path: event.url.pathname }, 'Rejecting unauthenticated request'); + return unauthenticatedResponse(event.url); } return resolve(event); diff --git a/src/lib/client/session.svelte.spec.ts b/src/lib/client/session.svelte.spec.ts new file mode 100644 index 00000000..3f894f4a --- /dev/null +++ b/src/lib/client/session.svelte.spec.ts @@ -0,0 +1,66 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { checkSession, sessionState } from './session.svelte.js'; + +const { getSession } = vi.hoisted(() => ({ getSession: vi.fn() })); +vi.mock('$lib/auth-client', () => ({ authClient: { getSession } })); + +describe('checkSession', () => { + beforeEach(() => { + getSession.mockReset(); + sessionState.active = true; + sessionState.expired = false; + }); + + it('marks the session expired when the server reports no session', async () => { + getSession.mockResolvedValue({ data: null, error: null }); + + await checkSession(); + + expect(sessionState.expired).toBe(true); + expect(getSession).toHaveBeenCalledWith({ query: { disableRefresh: true } }); + }); + + it('keeps the session when the server returns one', async () => { + getSession.mockResolvedValue({ data: { user: {}, session: {} }, error: null }); + + await checkSession(); + + expect(sessionState.expired).toBe(false); + }); + + it('clears the flag once the session is restored', async () => { + sessionState.expired = true; + getSession.mockResolvedValue({ data: { user: {}, session: {} }, error: null }); + + await checkSession(); + + expect(sessionState.expired).toBe(false); + }); + + it('ignores server and network errors', async () => { + getSession.mockResolvedValueOnce({ data: null, error: { status: 500 } }); + getSession.mockRejectedValueOnce(new TypeError('Failed to fetch')); + + await checkSession(); + await checkSession(); + + expect(sessionState.expired).toBe(false); + }); + + it('shares one request between concurrent checks', async () => { + getSession.mockResolvedValue({ data: { user: {}, session: {} }, error: null }); + + await Promise.all([checkSession(), checkSession(), checkSession()]); + + expect(getSession).toHaveBeenCalledTimes(1); + }); + + it('does nothing while inactive', async () => { + sessionState.active = false; + + await checkSession(); + + expect(getSession).not.toHaveBeenCalled(); + expect(sessionState.expired).toBe(false); + }); +}); diff --git a/src/lib/client/session.svelte.ts b/src/lib/client/session.svelte.ts new file mode 100644 index 00000000..02b21baf --- /dev/null +++ b/src/lib/client/session.svelte.ts @@ -0,0 +1,36 @@ +import { authClient } from '$lib/auth-client'; + +/** + * Client-side view of the user's session. `active` is set while a signed-in + * layout is mounted; without it (e.g. OIDC disabled) no checks are made. + */ +export const sessionState = $state({ active: false, expired: false }); + +let pending: Promise | null = null; + +/** + * Ask the server whether the current session is still valid and update + * `sessionState.expired`. Concurrent calls share one request. A session that + * is restored (e.g. by signing in again in another tab) clears the flag. + * + * Network and server errors are ignored: only an explicit "no session" answer + * marks the session as expired. + * + * Note: `disableRefresh` only prevents extending a database-backed session. + * In stateless mode (no database, as configured today) a check made shortly + * before the session cookie expires still renews it, like any other request. + */ +export function checkSession(): Promise { + if (!sessionState.active) return Promise.resolve(); + pending ??= authClient + .getSession({ query: { disableRefresh: true } }) + .then(({ data, error }) => { + // Drop the result if the signed-in layout unmounted in the meantime. + if (!error && sessionState.active) sessionState.expired = !data; + }) + .catch(() => {}) + .finally(() => { + pending = null; + }); + return pending; +} diff --git a/src/lib/components/Modal.svelte b/src/lib/components/Modal.svelte index 9bd44369..b8d417bc 100644 --- a/src/lib/components/Modal.svelte +++ b/src/lib/components/Modal.svelte @@ -20,6 +20,7 @@ children, variant = 'default', class: className = '', + dismissible = true, closeguard, ...restProps }: { @@ -32,6 +33,8 @@ variant?: 'default' | 'fullscreen'; /** Extra classes, appended to the variant's own. */ class?: string; + /** When false, Escape and backdrop clicks do not close the modal. */ + dismissible?: boolean; closeguard?: () => boolean; } & Omit< import('svelte/elements').HTMLDialogAttributes, @@ -64,11 +67,17 @@ // component was destroyed in the meantime, the shared `open` binding may // already belong to a newly opened dialog, which must not be closed. if (e.currentTarget !== dialogEl) return; + // Browsers may close a dialog despite a cancelled `cancel` event (e.g. a + // repeated Escape press), so reopen it if it must stay open. + if (!dismissible && open) { + dialogEl?.showModal(); + return; + } open = false; } function handleCancel(e: Event) { - if (closeguard && !closeguard()) { + if (!dismissible || (closeguard && !closeguard())) { e.preventDefault(); } } @@ -103,6 +112,7 @@ // by the browser on Escape; the dialog itself must stay open. if (dialogEl.querySelector('[popover]:not([popover="manual"]):popover-open')) return; e.preventDefault(); + if (!dismissible) return; if (closeguard && !closeguard()) return; open = false; } @@ -113,7 +123,7 @@ function handleBackdropClick(e: MouseEvent) { // The click target is the element itself only when the backdrop is // clicked; clicks inside the content bubble up to child elements instead. - if (e.target !== dialogEl) return; + if (e.target !== dialogEl || !dismissible) return; if (closeguard && !closeguard()) { e.preventDefault(); } else { diff --git a/src/lib/components/layout/SessionExpiredModal.svelte b/src/lib/components/layout/SessionExpiredModal.svelte new file mode 100644 index 00000000..c64dd2eb --- /dev/null +++ b/src/lib/components/layout/SessionExpiredModal.svelte @@ -0,0 +1,53 @@ + + + + + diff --git a/src/lib/server/auth-guard.test.ts b/src/lib/server/auth-guard.test.ts new file mode 100644 index 00000000..4052be07 --- /dev/null +++ b/src/lib/server/auth-guard.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import { isRedirect } from '@sveltejs/kit'; +import { unauthenticatedResponse } from './auth-guard.js'; + +describe('unauthenticatedResponse', () => { + it('returns a JSON 401 for API routes', async () => { + const res = unauthenticatedResponse( + new URL('http://localhost/api/trino/catalog?level=catalogs') + ); + + expect(res.status).toBe(401); + expect(res.headers.get('content-type')).toContain('application/json'); + expect(await res.json()).toEqual({ message: 'Authentication required' }); + }); + + it('redirects page routes to the login page, preserving the target', () => { + try { + unauthenticatedResponse(new URL('http://localhost/trino?tab=1')); + expect.unreachable('expected a redirect to be thrown'); + } catch (err) { + expect(isRedirect(err)).toBe(true); + if (!isRedirect(err)) return; + expect(err.status).toBe(302); + expect(err.location).toBe('/auth/login?redirectTo=%2Ftrino%3Ftab%3D1'); + } + }); + + it('does not treat paths that merely start with "api" as API routes', () => { + expect(() => unauthenticatedResponse(new URL('http://localhost/apis'))).toThrow(); + }); +}); diff --git a/src/lib/server/auth-guard.ts b/src/lib/server/auth-guard.ts new file mode 100644 index 00000000..70d679e5 --- /dev/null +++ b/src/lib/server/auth-guard.ts @@ -0,0 +1,18 @@ +import { json, redirect } from '@sveltejs/kit'; + +/** + * Build the response for a request that requires authentication but has no + * valid session. + * + * API routes get a JSON 401 so that client-side `fetch` callers receive a + * parseable error. + * The client treats a 401 as a hint to re-check the session (see + * `src/hooks.client.ts`). All other routes redirect to the login page. + */ +export function unauthenticatedResponse(url: URL): Response { + if (url.pathname.startsWith('/api/')) { + return json({ message: 'Authentication required' }, { status: 401 }); + } + const redirectTo = encodeURIComponent(url.pathname + url.search); + throw redirect(302, `/auth/login?redirectTo=${redirectTo}`); +} diff --git a/src/routes/(app)/+layout.svelte b/src/routes/(app)/+layout.svelte index f538f0f6..944903b7 100644 --- a/src/routes/(app)/+layout.svelte +++ b/src/routes/(app)/+layout.svelte @@ -5,6 +5,7 @@ import Sidebar from '$lib/components/layout/sidebar/Sidebar.svelte'; import Header from '$lib/components/layout/header/Header.svelte'; import ToastHost from '$lib/components/ToastHost.svelte'; + import SessionExpiredModal from '$lib/components/layout/SessionExpiredModal.svelte'; let { children, data } = $props(); @@ -64,3 +65,8 @@ + + +{#if data.user} + +{/if}