mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-09-30 03:01:21 +02:00
fix(auth): don't discard a valid stored token on a transient load-time error (#1889)
On mount, AuthContext.checkAuthStatus restores the persisted "Remember Me"
token from localStorage and validates it via GET /auth/me. The catch around
that call cleared the token on ANY failure, not just a definitive 401
invalid-token — so a brief backend-not-ready or reverse-proxy hiccup during
page load (plausible right after a container restart, e.g. on Unraid) would
delete a still-valid token. Because the token was deleted, a reload couldn't
recover it and the user was bounced to the login screen.
Token validation now retries transient failures (up to 3 attempts with short
backoff) and only discards the token on a definitive 401 — which request()
already handles (clears the token and dispatches auth:expired). Transient /
5xx / network errors leave the persisted token intact so the session survives
a slow load. "Remember Me" stays client-storage only; it does not extend the
server-side JWT lifetime (session_max_hours, default 24h).
Adds AuthContext tests: transient /auth/me failure keeps the token, a
definitive 401 clears it, and a valid token loads the user. Rebuilt frontend
bundle.
This commit is contained in:
@@ -2,7 +2,7 @@
|
||||
* Tests for the AuthContext permission helpers.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
||||
import { act, renderHook, waitFor } from '@testing-library/react';
|
||||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
|
||||
import { BrowserRouter } from 'react-router-dom';
|
||||
@@ -11,7 +11,7 @@ import { server } from '../mocks/server';
|
||||
import { AuthProvider, useAuth } from '../../contexts/AuthContext';
|
||||
import { ThemeProvider } from '../../contexts/ThemeContext';
|
||||
import { ToastProvider } from '../../contexts/ToastContext';
|
||||
import { setAuthToken, type Permission } from '../../api/client';
|
||||
import { getAuthToken, setAuthToken, type Permission } from '../../api/client';
|
||||
|
||||
function createWrapper() {
|
||||
const queryClient = new QueryClient({
|
||||
@@ -325,4 +325,76 @@ describe('AuthContext', () => {
|
||||
}).not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe('token validation on mount (#1889)', () => {
|
||||
beforeEach(() => {
|
||||
// Persisted "Remember Me" token — lives in localStorage.
|
||||
setAuthToken('valid-token', 'persistent');
|
||||
server.use(
|
||||
http.get('/api/v1/auth/status', () =>
|
||||
HttpResponse.json({ auth_enabled: true, requires_setup: false })
|
||||
)
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
setAuthToken(null);
|
||||
localStorage.removeItem('auth_token');
|
||||
});
|
||||
|
||||
it('keeps the persisted token when /auth/me fails transiently (does not force re-login)', async () => {
|
||||
// Backend not ready yet / brief blip → 500 on every attempt.
|
||||
server.use(http.get('/api/v1/auth/me', () => new HttpResponse(null, { status: 500 })));
|
||||
vi.mocked(window.localStorage.removeItem).mockClear();
|
||||
|
||||
const { result } = renderHook(() => useAuth(), { wrapper: createWrapper() });
|
||||
|
||||
await waitFor(() => expect(result.current.loading).toBe(false), { timeout: 4000 });
|
||||
|
||||
// No user this load, but the token MUST survive so a reload can recover —
|
||||
// the pre-#1889 blanket catch deleted it, making the session unrecoverable.
|
||||
expect(result.current.user).toBeNull();
|
||||
expect(getAuthToken()).toBe('valid-token');
|
||||
// Persisted copy must not be wiped (setAuthToken(null) removes it).
|
||||
expect(window.localStorage.removeItem).not.toHaveBeenCalledWith('auth_token');
|
||||
});
|
||||
|
||||
it('clears the token on a definitive 401 invalid-token response', async () => {
|
||||
server.use(
|
||||
http.get('/api/v1/auth/me', () =>
|
||||
HttpResponse.json({ detail: 'Could not validate credentials' }, { status: 401 })
|
||||
)
|
||||
);
|
||||
vi.mocked(window.localStorage.removeItem).mockClear();
|
||||
|
||||
const { result } = renderHook(() => useAuth(), { wrapper: createWrapper() });
|
||||
|
||||
await waitFor(() => expect(result.current.loading).toBe(false), { timeout: 4000 });
|
||||
|
||||
expect(result.current.user).toBeNull();
|
||||
expect(getAuthToken()).toBeNull();
|
||||
// Definitive invalid-token → persisted copy is removed.
|
||||
expect(window.localStorage.removeItem).toHaveBeenCalledWith('auth_token');
|
||||
});
|
||||
|
||||
it('loads the user when the persisted token is valid', async () => {
|
||||
server.use(
|
||||
http.get('/api/v1/auth/me', () =>
|
||||
HttpResponse.json({
|
||||
id: 1,
|
||||
username: 'alice',
|
||||
is_active: true,
|
||||
permissions: [],
|
||||
groups: [],
|
||||
})
|
||||
)
|
||||
);
|
||||
|
||||
const { result } = renderHook(() => useAuth(), { wrapper: createWrapper() });
|
||||
|
||||
await waitFor(() => expect(result.current.user).not.toBeNull());
|
||||
expect(result.current.user?.username).toBe('alice');
|
||||
expect(getAuthToken()).toBe('valid-token');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import React, { createContext, useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react';
|
||||
import { api, getAuthToken, setAuthToken } from '../api/client';
|
||||
import { ApiError, api, getAuthToken, setAuthToken } from '../api/client';
|
||||
import type { LoginResponse, Permission, TokenPersistence, UserResponse } from '../api/client';
|
||||
|
||||
interface AuthContextType {
|
||||
@@ -58,18 +58,51 @@ export function AuthProvider({ children }: { children: React.ReactNode }) {
|
||||
if (status.auth_enabled) {
|
||||
const token = getAuthToken();
|
||||
if (token) {
|
||||
try {
|
||||
const currentUser = await api.getCurrentUser();
|
||||
if (!mountedRef.current) return;
|
||||
// Validate the stored token. A transient failure here (backend not
|
||||
// yet ready after a container/proxy restart, a brief network blip)
|
||||
// must NOT discard a valid persisted token — doing so logs the user
|
||||
// out and, because the token is deleted, a reload can't recover it
|
||||
// (#1889). Only a definitive 401 invalid-token response clears the
|
||||
// token, and `request()` already does that clearing + dispatches
|
||||
// `auth:expired`; here we just retry the transient cases and keep the
|
||||
// token so the session survives a slow load.
|
||||
let currentUser: UserResponse | null = null;
|
||||
let definitiveAuthFailure = false;
|
||||
const maxAttempts = 3;
|
||||
for (let attempt = 1; attempt <= maxAttempts; attempt++) {
|
||||
try {
|
||||
currentUser = await api.getCurrentUser();
|
||||
break;
|
||||
} catch (err) {
|
||||
if (!mountedRef.current) return;
|
||||
// 401 invalid-token → genuinely logged out. `request()` has
|
||||
// already cleared the token; stop retrying.
|
||||
if (err instanceof ApiError && err.status === 401) {
|
||||
definitiveAuthFailure = true;
|
||||
break;
|
||||
}
|
||||
// Transient (network / 5xx / other) → back off and retry. Leave
|
||||
// the token in place so a subsequent load can recover.
|
||||
if (attempt < maxAttempts) {
|
||||
await new Promise((r) => setTimeout(r, 400 * attempt));
|
||||
}
|
||||
}
|
||||
}
|
||||
if (!mountedRef.current) return;
|
||||
if (currentUser) {
|
||||
setUser(currentUser);
|
||||
// Persist kiosk token only after the server confirms it is valid.
|
||||
if (urlToken && token === urlToken) {
|
||||
setAuthToken(urlToken, 'persistent');
|
||||
}
|
||||
} catch {
|
||||
// Token invalid, clear it (removes from both sessionStorage and localStorage)
|
||||
setAuthToken(null);
|
||||
if (!mountedRef.current) return;
|
||||
} else {
|
||||
// No user: either a definitive 401 (token already cleared by
|
||||
// request()) or transient failures exhausted their retries. In the
|
||||
// transient case we deliberately keep the token so a reload retries
|
||||
// rather than forcing a re-login.
|
||||
if (definitiveAuthFailure) {
|
||||
setAuthToken(null);
|
||||
}
|
||||
setUser(null);
|
||||
}
|
||||
} else {
|
||||
|
||||
File diff suppressed because one or more lines are too long
+1
-1
@@ -26,7 +26,7 @@
|
||||
|
||||
<!-- Splash screens for iOS -->
|
||||
<link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
|
||||
<script type="module" crossorigin src="/assets/index-0IaClY0k.js"></script>
|
||||
<script type="module" crossorigin src="/assets/index-CaHy_042.js"></script>
|
||||
<link rel="stylesheet" crossorigin href="/assets/index-BxVhuRti.css">
|
||||
</head>
|
||||
<body>
|
||||
|
||||
Reference in New Issue
Block a user