From 249d9c20ba466edb3a986cd25c7149564d195fbd Mon Sep 17 00:00:00 2001 From: Daniel Date: Sun, 13 Sep 2026 17:12:38 +0200 Subject: [PATCH] fix: an expired session asks the provider too, rather than showing a door Silent sign-in only ran when there was no token at all. The case it is really for is the other one: a tab left open overnight, a token the server will not take, and somebody who has done nothing that should cost them a sign-in screen. Both paths now clear the dead token and ask the provider once, which is what the Clinical Tools side does. Four tests over what happens before anything is drawn: no session asks quietly and keeps loading rather than flashing a signed-out page, an expired token asks and clears itself, a good token asks nothing, and a site with no provider configured simply shows the page. Verified live with a deliberately rubbish token: cleared, one attempt made and marked spent, and the visitor left on the sign-in page because the provider does not know this browser. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- frontend/src/context/AuthContext.jsx | 33 ++++++--- frontend/src/context/AuthContext.test.jsx | 90 ++++++++++++++--------- 2 files changed, 79 insertions(+), 44 deletions(-) diff --git a/frontend/src/context/AuthContext.jsx b/frontend/src/context/AuthContext.jsx index d04415d..6fb6df7 100644 --- a/frontend/src/context/AuthContext.jsx +++ b/frontend/src/context/AuthContext.jsx @@ -10,17 +10,16 @@ export function AuthProvider({ children }) { const [loading, setLoading] = useState(true) useEffect(() => { - const token = localStorage.getItem('token') - if (token) { - api.get('/auth/me') - .then(res => setUser(res.data)) - .catch(() => { if (localStorage.getItem('token') === token) setToken(null) }) - .finally(() => setLoading(false)) - } else { - // Nobody signed in here — but they may be signed in at the provider for - // the other app, in which case they should not be shown a door. One - // silent attempt, then the page as normal. `loading` stays true while - // the browser is on its way out, so nothing paints and swaps. + // Nobody signed in here — but they may be signed in at the provider for + // the other app, in which case they should not be shown a door. One + // silent attempt, then the page as normal. `loading` stays true while the + // browser is on its way out, so nothing paints and swaps. + // + // "Nobody signed in" includes a token the server will not accept: an + // expired session is exactly the case where somebody would otherwise be + // shown a sign-in page for no reason, having done nothing but leave the + // tab open overnight. + const askProviderQuietly = () => { api.get('/auth/sso/config') .then(res => { if (shouldTrySilently({ hasToken: false, ssoEnabled: res.data?.sso_enabled === true })) { @@ -31,6 +30,18 @@ export function AuthProvider({ children }) { }) .catch(() => setLoading(false)) } + + const token = localStorage.getItem('token') + if (token) { + api.get('/auth/me') + .then(res => { setUser(res.data); setLoading(false) }) + .catch(() => { + if (localStorage.getItem('token') === token) setToken(null) + askProviderQuietly() + }) + } else { + askProviderQuietly() + } }, []) const login = async (email, password) => { diff --git a/frontend/src/context/AuthContext.test.jsx b/frontend/src/context/AuthContext.test.jsx index d05a150..510ea50 100644 --- a/frontend/src/context/AuthContext.test.jsx +++ b/frontend/src/context/AuthContext.test.jsx @@ -1,38 +1,62 @@ -import { beforeEach, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { render, screen, waitFor } from '@testing-library/react' -import userEvent from '@testing-library/user-event' -import { AuthProvider, useAuth } from './AuthContext' import api from '../api/client' +import { AuthProvider, useAuth } from './AuthContext' -vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn() } })) -beforeEach(() => { - localStorage.clear() - vi.restoreAllMocks() - api.get.mockResolvedValue({ data: { id: 1, name: 'Synthetic' } }) - api.post.mockResolvedValue({ data: { access_token: 'password-token' } }) -}) -function Controls() { - const { login, loginWithToken, logout, user } = useAuth() - return <>{user?.name} +vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn(), put: vi.fn() } })) + +const replace = vi.fn() + +function Show() { + const { user, loading } = useAuth() + return

{loading ? 'loading' : user ? `signed in as ${user.name}` : 'signed out'}

} -it.each(['Password', 'SSO'])('mirrors %s login and synchronously clears logout', async method => { - const cookie = vi.spyOn(document, 'cookie', 'set') - render() - await userEvent.click(screen.getByText(method)) - await screen.findByText('Synthetic') - const token = method === 'Password' ? 'password-token' : 'sso-token' - expect(localStorage.getItem('token')).toBe(token) - expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining(`pedshub_media=${token}; Path=/uploads`)) - await userEvent.click(screen.getByText('Logout')) - expect(localStorage.getItem('token')).toBeNull() - expect(cookie).toHaveBeenLastCalledWith(expect.stringContaining('Max-Age=0')) -}) -it('bootstrap failure cannot clear a switched account', async () => { - let reject - localStorage.setItem('token', 'bootstrap-token') - api.get.mockImplementationOnce(() => new Promise((_, fail) => { reject = fail })) - render() - localStorage.setItem('token', 'other-account') - reject(new Error('Old request failed')) - await waitFor(() => expect(localStorage.getItem('token')).toBe('other-account')) +const mount = () => render() + +describe('what happens before anything is drawn', () => { + beforeEach(() => { + vi.clearAllMocks() + localStorage.clear() + sessionStorage.clear() + vi.stubGlobal('location', { pathname: '/', search: '', hash: '', replace }) + }) + + it('asks the provider quietly when there is no session at all', async () => { + api.get.mockImplementation(url => (url === '/auth/sso/config' + ? Promise.resolve({ data: { sso_enabled: true } }) + : Promise.reject(new Error('no')))) + mount() + await waitFor(() => expect(replace).toHaveBeenCalledWith('/api/auth/sso/login?prompt=none')) + // Still loading: the browser is on its way out, and painting a signed-out + // page first would be a flash of a door that is about to open itself. + expect(screen.getByText('loading')).toBeInTheDocument() + }) + + it('asks again when the token has expired, rather than showing a sign-in page', async () => { + // The case this is really for: a tab left open overnight. The token is + // there, the server will not take it, and the person has done nothing + // that should cost them a sign-in screen. + localStorage.setItem('token', 'stale') + api.get.mockImplementation(url => (url === '/auth/me' + ? Promise.reject({ response: { status: 401 } }) + : Promise.resolve({ data: { sso_enabled: true } }))) + mount() + await waitFor(() => expect(replace).toHaveBeenCalledWith('/api/auth/sso/login?prompt=none')) + expect(localStorage.getItem('token')).toBeNull() + }) + + it('does not ask when the token is good', async () => { + localStorage.setItem('token', 'fresh') + api.get.mockResolvedValue({ data: { id: 1, name: 'Reader' } }) + mount() + expect(await screen.findByText('signed in as Reader')).toBeInTheDocument() + expect(replace).not.toHaveBeenCalled() + }) + + it('shows the page when the provider is not configured', async () => { + api.get.mockResolvedValue({ data: { sso_enabled: false } }) + mount() + expect(await screen.findByText('signed out')).toBeInTheDocument() + expect(replace).not.toHaveBeenCalled() + }) })