Skip to content

Commit 172dc39

Browse files
committed
fix(bd-jv5u083l): expiry timer refresh-only; probe owns logout
The hub validates the sync WS only at upgrade and keeps established connections alive past token expiry, so clearing auth on a schedule only disrupts a still-syncing session (and buys no security). Make the expiry/backstop timers refresh-only and stop logging out in the renewal-failure callbacks; evidence-based logout is now the sync-disconnect probe's job plus the cold mount check.
1 parent e628a18 commit 172dc39

4 files changed

Lines changed: 132 additions & 162 deletions

File tree

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# Expiry timer refresh-only; probe owns logout (bd-jv5u083l)
2+
3+
Follow-up to bd-3o8zmz46.
4+
5+
## Problem
6+
7+
`useAuth` cleared auth on a schedule: the expiry-time `/auth/me` re-check
8+
called `expireSession()` on a 401, and the silent-renewal failure callbacks
9+
(`onError`, `refreshToken` → null/throw) called it too when past `exp`. But the
10+
hub validates the sync WebSocket **only at the upgrade handshake** and never
11+
tears down an established connection on token expiry (`crates/quarto-hub/
12+
src/server.rs:893`). So a session whose cookie has been evicted at `exp` keeps
13+
syncing fine over the still-open socket — and the only HTTP the client makes
14+
during steady editing is the four `/auth/*` endpoints. Tearing the session
15+
down based on a side-channel `/auth/me` 401, while the channel the user is
16+
actually using (the WS) still works, is purely disruptive.
17+
18+
Key insight: **client-side proactive logout buys zero security.** The server is
19+
the boundary and has deliberately chosen to keep the socket alive; a client
20+
logout cannot stop a determined client from editing on the grandfathered
21+
connection. So the expiry logout was only ever UX — and as UX, interrupting a
22+
working edit session (editor unmounts → LoginScreen) is the worse outcome.
23+
True session severing on expiry is server-side work (bd-ey6jg70f).
24+
25+
## Change
26+
27+
- The schedule (`exp − 15 min` refresh timer, `exp` backstop timer) now drives
28+
**refresh only** — both timers call `triggerRefresh()`, never `expireSession()`.
29+
- Silent-renewal failure callbacks no longer log out; a renewal that yields no
30+
session is not evidence the open WS can't sync.
31+
- Logout is now the **sole responsibility of `useAuthProbe`** (the sync-disconnect
32+
probe, two-strike 401 → `expireSession`) plus the cold mount check
33+
(`/auth/me` 401 at startup → login). The visibility handler was already
34+
refresh-only.
35+
- Removed now-dead code: `sessionLapsed`, `expiresAtRef`, `EXPIRY_RECHECK_MS`,
36+
and the `scheduleExpiryCheck` poll/reschedule loop.
37+
38+
## Residual behaviour (intentional, no data loss)
39+
40+
- Editing a project, WS up, `exp` passes, renewal fails → **session retained**
41+
(the fix). Logout happens via the probe once the socket actually drops, or on
42+
reload (mount check).
43+
- Project-list screen (no project open) with a dead cookie: the probe is gated
44+
on `!!project`, so it won't fire there; the dead session is surfaced on reload
45+
or on a project-open attempt (`fetchActorId` 401 → renewal). Acceptable —
46+
broadening the probe's enablement is a separate change.
47+
- No permanent data loss in any case: Automerge changes persist to IndexedDB
48+
and re-sync after re-login.
49+
50+
## Verification
51+
52+
- `useAuth.test.tsx`: schedule-driven-logout tests reframed to assert the new
53+
contract (refresh attempted, session retained on failure; adopt on success;
54+
refocus does not drift the schedule). The visibility "no logout on renewal
55+
failure" test was confirmed red against the old code before implementing.
56+
- hub-client: 572 unit + 66 integration + 81 wasm pass; `npm run build` green.
57+
- **E2E honesty note**: the real flow (Google token aging past 1 h with the WS
58+
staying open, then dropping) was NOT exercised in a browser — same live-session
59+
limitation as bd-3o8zmz46. Manual recipe: sign in, edit, delete the
60+
`quarto_hub_token` cookie in DevTools while the socket stays connected; editing
61+
should keep working until the socket drops, after which the probe lands you on
62+
the login screen with the "session expired" message.

hub-client/src/App.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,10 +85,10 @@ function App() {
8585
const [isOnline, setIsOnline] = useState<boolean>(false);
8686

8787
// While a project's sync is disconnected, check whether the disconnect is
88-
// actually an auth rejection (browsers hide the WS upgrade status). Only
89-
// definitive 401/403 evidence ever clears auth — never network errors.
90-
// Past the token's exp, useAuth's expiry timer logs out on the first 401
91-
// (preempting this probe's two-strike); the probe governs earlier drops.
88+
// actually an auth rejection (browsers hide the WS upgrade status). This is
89+
// the sole evidence-based logout path: useAuth never clears auth on a
90+
// schedule (bd-jv5u083l), so only a definitive 401/403 here — never a
91+
// network error — ends the session.
9292
useAuthProbe({
9393
enabled: AUTH_ENABLED && !!auth && !!project && !isOnline,
9494
triggerRefresh,

hub-client/src/hooks/useAuth.test.tsx

Lines changed: 37 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -226,63 +226,40 @@ describe('useAuth', () => {
226226
await vi.waitFor(() => expect(result.current.auth).toEqual(refreshedUser));
227227
});
228228

229-
it('clears auth when silent renewal fails after visibility-triggered refresh', async () => {
229+
it('does not log out when renewal fails after a visibility-triggered refresh', async () => {
230230
const user = { email: 'a@b.com', name: 'A', picture: null };
231231
mockFetchAuthMe.mockResolvedValueOnce(user); // mount
232232

233233
const { result } = renderHook(() => useAuth(), { wrapper });
234234
await vi.waitFor(() => expect(result.current.auth).toEqual(user));
235235

236-
// Jump Date.now() past cookie lifetime (simulates long idle)
236+
// Long idle past expiry, then refocus with a rejected cookie.
237237
vi.setSystemTime(Date.now() + 3600 * 1000 + 1000);
238-
239-
// Next /auth/me returns null (cookie expired)
240238
mockFetchAuthMe.mockResolvedValueOnce(null);
241239

242240
document.dispatchEvent(new Event('visibilitychange'));
243241
await act(async () => {
244242
await vi.advanceTimersByTimeAsync(0);
245243
});
244+
expect(mockProvider.lastSilentRenewalOpts?.enabled).toBe(true);
246245

247-
// Simulate silent renewal failing (no active IdP session).
246+
// Renewal fails (IdP session gone). Auth is RETAINED: a dead cookie is
247+
// not evidence the still-open WS can't sync. The disconnect probe owns
248+
// evidence-based logout (bd-jv5u083l).
248249
await act(async () => {
249250
mockProvider.lastSilentRenewalOpts?.onError();
250251
});
251-
252-
await vi.waitFor(() => expect(result.current.auth).toBeNull());
253-
});
254-
255-
it('clears auth when refreshToken returns null after visibility-triggered refresh', async () => {
256-
const user = { email: 'a@b.com', name: 'A', picture: null };
257-
mockFetchAuthMe.mockResolvedValueOnce(user); // mount
258-
259-
const { result } = renderHook(() => useAuth(), { wrapper });
260-
await vi.waitFor(() => expect(result.current.auth).toEqual(user));
261-
262-
// Jump Date.now() past cookie lifetime (simulates long idle)
263-
vi.setSystemTime(Date.now() + 3600 * 1000 + 1000);
264-
265-
// Next /auth/me returns null (cookie expired)
266-
mockFetchAuthMe.mockResolvedValueOnce(null);
267-
268-
document.dispatchEvent(new Event('visibilitychange'));
269-
await act(async () => {
270-
await vi.advanceTimersByTimeAsync(0);
271-
});
272-
273-
// Silent renewal returns a credential, but server rejects it.
274-
mockRefreshToken.mockResolvedValue(null);
275-
await act(async () => {
276-
mockProvider.lastSilentRenewalOpts?.onCredential('rejected.jwt');
277-
});
278-
279-
await vi.waitFor(() => expect(result.current.auth).toBeNull());
252+
expect(result.current.auth).toEqual(user);
253+
expect(result.current.sessionExpired).toBe(false);
280254
});
281255
});
282256

283-
// ── Hard expiry (fake timers) ─────────────────────────────
257+
// ── Expiry behaviour (fake timers) ────────────────────────
258+
// The schedule drives refresh, never logout: while the WS stays open it
259+
// keeps syncing past token expiry, so a dead cookie is not grounds to tear
260+
// down a working session (bd-jv5u083l). Logout is the disconnect probe's job.
284261

285-
describe('hard expiry', () => {
262+
describe('expiry behaviour', () => {
286263
beforeEach(() => {
287264
vi.useFakeTimers({ shouldAdvanceTime: true });
288265
});
@@ -291,58 +268,57 @@ describe('useAuth', () => {
291268
vi.useRealTimers();
292269
});
293270

294-
it('clears auth when cookie expires and no refresh in progress', async () => {
271+
it('attempts refresh at expiry and retains the session when renewal fails', async () => {
295272
const user = { email: 'a@b.com', name: 'A', picture: null };
296-
mockFetchAuthMe
297-
.mockResolvedValueOnce(user) // mount check
298-
.mockResolvedValueOnce(null); // expiry re-check
273+
mockFetchAuthMe.mockResolvedValueOnce(user); // mount
299274

300275
const { result } = renderHook(() => useAuth(), { wrapper });
301276
await vi.waitFor(() =>
302277
expect(result.current.auth).toEqual(user),
303278
);
304279

305-
// Advance past the refresh point. The refresh timer sets
306-
// isRefreshing=true and enables silent renewal. Simulate the IdP
307-
// session being gone (onError fires), resetting isRefreshing=false.
280+
// Reach the refresh point — renewal is attempted, then fails.
308281
await act(async () => {
309282
vi.advanceTimersByTime(COOKIE_MAX_AGE_MS - REFRESH_BUFFER_MS + 100);
310283
});
284+
expect(mockProvider.lastSilentRenewalOpts?.enabled).toBe(true);
311285
await act(async () => {
312286
mockProvider.lastSilentRenewalOpts?.onError();
313287
});
314288

315-
// Advance past cookie max-age (remaining buffer).
289+
// Advance past expiry: the session is NOT cleared on schedule.
316290
await act(async () => {
317291
vi.advanceTimersByTime(REFRESH_BUFFER_MS + 100);
318292
});
319-
320-
await vi.waitFor(() =>
321-
expect(result.current.auth).toBeNull(),
322-
);
293+
expect(result.current.auth).toEqual(user);
294+
expect(result.current.sessionExpired).toBe(false);
323295
});
324296

325-
it('keeps auth if server confirms valid cookie at expiry', async () => {
297+
it('adopts the refreshed session when renewal succeeds at expiry', async () => {
326298
const user = { email: 'a@b.com', name: 'A', picture: null };
327299
const freshUser = {
328300
email: 'a@b.com',
329301
name: 'Still Valid',
330302
picture: null,
331303
};
332-
mockFetchAuthMe
333-
.mockResolvedValueOnce(user) // mount check
334-
.mockResolvedValueOnce(freshUser); // expiry re-check (refresh succeeded)
304+
mockFetchAuthMe.mockResolvedValueOnce(user); // mount
335305

336306
const { result } = renderHook(() => useAuth(), { wrapper });
337307
await vi.waitFor(() =>
338308
expect(result.current.auth).toEqual(user),
339309
);
340310

341-
// Advance past cookie max-age
311+
// Reach expiry; the backstop refresh fires.
342312
await act(async () => {
343-
vi.advanceTimersByTime(3600 * 1000 + 100);
313+
vi.advanceTimersByTime(COOKIE_MAX_AGE_MS + 100);
344314
});
315+
expect(mockProvider.lastSilentRenewalOpts?.enabled).toBe(true);
345316

317+
// Renewal delivers a fresh credential → adopt it.
318+
mockRefreshToken.mockResolvedValue(freshUser);
319+
await act(async () => {
320+
mockProvider.lastSilentRenewalOpts?.onCredential('fresh.jwt');
321+
});
346322
await vi.waitFor(() =>
347323
expect(result.current.auth).toEqual(freshUser),
348324
);
@@ -386,19 +362,18 @@ describe('useAuth', () => {
386362
expect(mockProvider.lastSilentRenewalOpts?.enabled).toBe(true);
387363
});
388364

389-
it('does not extend assumed expiry on refocus without a fresh cookie (drift bug)', async () => {
365+
it('does not extend the refresh schedule on refocus without a fresh cookie (drift bug)', async () => {
390366
const expiresAt = Date.now() + 40 * 60 * 1000;
391367
const user = { email: 'a@b.com', name: 'A', picture: null, expiresAt };
392368
mockFetchAuthMe
393369
.mockResolvedValueOnce(user) // mount
394-
.mockResolvedValueOnce({ ...user }) // refocus: same expiry, fresh object
395-
.mockResolvedValueOnce(null); // expiry re-check: token now rejected
370+
.mockResolvedValueOnce({ ...user }); // refocus: same expiry, fresh object
396371

397372
const { result } = renderHook(() => useAuth(), { wrapper });
398373
await vi.waitFor(() => expect(result.current.auth).toEqual(user));
399374

400375
// Refocus at +10 min — server confirms the cookie but its expiry is
401-
// unchanged. This must NOT push the schedule out to +70 min.
376+
// unchanged. This must NOT push the schedule out to +35 min.
402377
await act(async () => {
403378
vi.advanceTimersByTime(10 * 60 * 1000);
404379
});
@@ -407,20 +382,13 @@ describe('useAuth', () => {
407382
await vi.advanceTimersByTimeAsync(0);
408383
});
409384

410-
// Refresh fires at expiresAt − 15 min (+25 min); renewal fails.
385+
// Refresh must still fire at the REAL expiresAt − 15 min (+25 min),
386+
// tracking the token's exp rather than drifting with the refocus.
387+
expect(mockProvider.lastSilentRenewalOpts?.enabled).toBe(false);
411388
await act(async () => {
412389
vi.advanceTimersByTime(15 * 60 * 1000 + 1000);
413390
});
414-
await act(async () => {
415-
mockProvider.lastSilentRenewalOpts?.onError();
416-
});
417-
418-
// At the REAL expiry (+40 min) the server's 401 must clear auth.
419-
await act(async () => {
420-
vi.advanceTimersByTime(15 * 60 * 1000 + 2000);
421-
});
422-
await vi.waitFor(() => expect(result.current.auth).toBeNull());
423-
expect(result.current.sessionExpired).toBe(true);
391+
expect(mockProvider.lastSilentRenewalOpts?.enabled).toBe(true);
424392
});
425393

426394
it('keeps auth when refocus /auth/me fails with a network error (offline)', async () => {
@@ -440,39 +408,6 @@ describe('useAuth', () => {
440408
expect(result.current.auth).toEqual(user);
441409
});
442410

443-
it('keeps auth on expiry-time network error and re-checks later', async () => {
444-
const expiresAt = Date.now() + 20 * 60 * 1000;
445-
const user = { email: 'a@b.com', name: 'A', picture: null, expiresAt };
446-
mockFetchAuthMe.mockResolvedValueOnce(user); // mount
447-
448-
const { result } = renderHook(() => useAuth(), { wrapper });
449-
await vi.waitFor(() => expect(result.current.auth).toEqual(user));
450-
451-
// Refresh fires at +5 min; renewal fails (session not lapsed → keep).
452-
await act(async () => {
453-
vi.advanceTimersByTime(5 * 60 * 1000 + 1000);
454-
});
455-
await act(async () => {
456-
mockProvider.lastSilentRenewalOpts?.onError();
457-
});
458-
459-
// Expiry re-check at +20 min hits a network error → stay logged in.
460-
mockFetchAuthMe.mockRejectedValueOnce(new Error('network'));
461-
await act(async () => {
462-
vi.advanceTimersByTime(15 * 60 * 1000 + 2000);
463-
});
464-
expect(result.current.auth).toEqual(user);
465-
expect(result.current.sessionExpired).toBe(false);
466-
467-
// The next re-check gets a definitive 401 → evidence-based logout.
468-
mockFetchAuthMe.mockResolvedValueOnce(null);
469-
await act(async () => {
470-
vi.advanceTimersByTime(60 * 1000 + 1000);
471-
});
472-
await vi.waitFor(() => expect(result.current.auth).toBeNull());
473-
expect(result.current.sessionExpired).toBe(true);
474-
});
475-
476411
it('does not flag sessionExpired on deliberate logout', async () => {
477412
const user = { email: 'a@b.com', name: 'A', picture: null };
478413
mockFetchAuthMe.mockResolvedValue(user);

0 commit comments

Comments
 (0)