fix(auth): dedupe and retry token refresh to prevent spurious logouts (#2948)
This commit is contained in:
Tink
GitHub
parent
02d46944ac
commit
f4bbe80144
|
|
@ -0,0 +1,153 @@
|
|||
import {describe, it, expect, vi, beforeEach, afterEach} from 'vitest'
|
||||
|
||||
import {refreshToken, removeToken} from './auth'
|
||||
|
||||
// Count how many times the refresh endpoint is actually POSTed. The whole point
|
||||
// of the in-flight dedup is that concurrent refreshToken() calls share a single
|
||||
// underlying POST, independent of the Web Locks API.
|
||||
let postCallCount = 0
|
||||
let resolvePost: ((value: unknown) => void) | null = null
|
||||
|
||||
vi.mock('@/helpers/fetcher', () => ({
|
||||
HTTPFactory: () => ({
|
||||
post: vi.fn(() => {
|
||||
postCallCount++
|
||||
return new Promise((resolve) => {
|
||||
resolvePost = resolve
|
||||
})
|
||||
}),
|
||||
}),
|
||||
}))
|
||||
|
||||
vi.mock('@/helpers/desktopAuth', () => ({
|
||||
isDesktopApp: () => false,
|
||||
refreshDesktopToken: vi.fn(),
|
||||
}))
|
||||
|
||||
const FAKE_TOKEN = 'header.payload.signature'
|
||||
|
||||
function settlePost() {
|
||||
resolvePost?.({data: {token: FAKE_TOKEN}})
|
||||
}
|
||||
|
||||
describe('refreshToken in-flight dedup', () => {
|
||||
const originalLocks = navigator.locks
|
||||
|
||||
beforeEach(() => {
|
||||
postCallCount = 0
|
||||
resolvePost = null
|
||||
removeToken()
|
||||
localStorage.clear()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
Object.defineProperty(navigator, 'locks', {
|
||||
value: originalLocks,
|
||||
configurable: true,
|
||||
writable: true,
|
||||
})
|
||||
})
|
||||
|
||||
it('coalesces concurrent calls into a single POST when Web Locks is available', async () => {
|
||||
// Stub a minimal Web Locks API: happy-dom leaves navigator.locks
|
||||
// undefined, so without this the test would silently fall through to
|
||||
// the insecure-HTTP branch and never exercise navigator.locks.request.
|
||||
const requestSpy = vi.fn((_name: string, cb: () => unknown) => cb())
|
||||
Object.defineProperty(navigator, 'locks', {
|
||||
value: {request: requestSpy},
|
||||
configurable: true,
|
||||
writable: true,
|
||||
})
|
||||
|
||||
const p1 = refreshToken(true)
|
||||
const p2 = refreshToken(true)
|
||||
|
||||
// Both calls share one underlying request.
|
||||
expect(postCallCount).toBe(1)
|
||||
|
||||
settlePost()
|
||||
await Promise.all([p1, p2])
|
||||
|
||||
// The Web Locks branch actually ran...
|
||||
expect(requestSpy).toHaveBeenCalledWith('vikunja-token-refresh', expect.any(Function))
|
||||
// ...and the in-flight dedup still collapsed both calls into one POST.
|
||||
expect(postCallCount).toBe(1)
|
||||
})
|
||||
|
||||
it('coalesces concurrent calls into a single POST on insecure HTTP (no Web Locks)', async () => {
|
||||
// Simulate an insecure HTTP context where navigator.locks is undefined.
|
||||
Object.defineProperty(navigator, 'locks', {
|
||||
value: undefined,
|
||||
configurable: true,
|
||||
writable: true,
|
||||
})
|
||||
|
||||
const p1 = refreshToken(true)
|
||||
const p2 = refreshToken(true)
|
||||
const p3 = refreshToken(true)
|
||||
|
||||
expect(postCallCount).toBe(1)
|
||||
|
||||
settlePost()
|
||||
await Promise.all([p1, p2, p3])
|
||||
|
||||
expect(postCallCount).toBe(1)
|
||||
})
|
||||
|
||||
it('allows a fresh refresh after the previous one settled', async () => {
|
||||
const p1 = refreshToken(true)
|
||||
settlePost()
|
||||
await p1
|
||||
expect(postCallCount).toBe(1)
|
||||
|
||||
// The in-flight promise was reset, so a later refresh runs anew.
|
||||
const p2 = refreshToken(true)
|
||||
expect(postCallCount).toBe(2)
|
||||
settlePost()
|
||||
await p2
|
||||
})
|
||||
|
||||
it('does not re-persist the token when logout happens during an in-flight refresh', async () => {
|
||||
const p1 = refreshToken(true)
|
||||
expect(postCallCount).toBe(1)
|
||||
|
||||
// User logs out while the refresh POST is still in flight.
|
||||
removeToken()
|
||||
|
||||
// The in-flight POST resolves afterwards — it must not undo the logout.
|
||||
settlePost()
|
||||
await p1
|
||||
|
||||
expect(localStorage.getItem('token')).toBeNull()
|
||||
})
|
||||
|
||||
it('an older refresh settling does not clobber a newer in-flight one', async () => {
|
||||
// Refresh A starts and stays in flight.
|
||||
const pA = refreshToken(true)
|
||||
expect(postCallCount).toBe(1)
|
||||
const resolveA = resolvePost
|
||||
|
||||
// User logs out, which drops the in-flight reference to A.
|
||||
removeToken()
|
||||
|
||||
// Refresh B starts; it must claim the in-flight slot.
|
||||
const pB = refreshToken(true)
|
||||
expect(postCallCount).toBe(2)
|
||||
const resolveB = resolvePost
|
||||
|
||||
// A settles after B started. Its cleanup must NOT null the in-flight
|
||||
// slot, since that slot now belongs to B. Without the `=== p` guard,
|
||||
// A's .finally would clobber B and let a concurrent caller fire a
|
||||
// second parallel POST.
|
||||
resolveA?.({data: {token: FAKE_TOKEN}})
|
||||
await pA
|
||||
|
||||
// A concurrent caller while B is still in flight must dedup to B —
|
||||
// no third POST.
|
||||
const pB2 = refreshToken(true)
|
||||
expect(postCallCount).toBe(2)
|
||||
|
||||
resolveB?.({data: {token: FAKE_TOKEN}})
|
||||
await Promise.all([pB, pB2])
|
||||
})
|
||||
})
|
||||
|
|
@ -33,18 +33,53 @@ export const removeToken = () => {
|
|||
savedToken = null
|
||||
localStorage.removeItem('token')
|
||||
localStorage.removeItem('desktopOAuthRefreshToken')
|
||||
|
||||
// Bump the epoch and drop the in-flight refresh so a refresh that started
|
||||
// before this logout can't re-persist a token after we cleared it.
|
||||
authEpoch++
|
||||
inFlightRefresh = null
|
||||
}
|
||||
|
||||
// Coalesces concurrent same-tab refreshes into one POST. Web Locks (below) is
|
||||
// secure-context-only, so on insecure HTTP there's no cross-tab coordination —
|
||||
// without this guard, refreshes firing close together each spend the single-use
|
||||
// cookie and all but one get a 401.
|
||||
let inFlightRefresh: Promise<void> | null = null
|
||||
|
||||
// Incremented on every removeToken()/logout. A refresh captures the epoch when
|
||||
// it starts and only persists its result if the epoch is unchanged, so a
|
||||
// refresh that resolves after a logout can't undo it.
|
||||
let authEpoch = 0
|
||||
|
||||
/**
|
||||
* Refreshes an auth token while ensuring it is updated everywhere.
|
||||
* The refresh token is sent automatically as an HttpOnly cookie.
|
||||
* The server rotates the cookie on every call.
|
||||
*
|
||||
* Uses the Web Locks API to coordinate across browser tabs. Only one tab
|
||||
* performs the actual refresh; other tabs waiting for the lock detect that
|
||||
* the token in localStorage was already updated and adopt it directly.
|
||||
* Same-tab concurrent calls share one in-flight refresh (always-on dedup); the
|
||||
* Web Locks API inside adds cross-tab coordination only in secure contexts.
|
||||
*/
|
||||
export async function refreshToken(persist: boolean): Promise<void> {
|
||||
if (inFlightRefresh) {
|
||||
return inFlightRefresh
|
||||
}
|
||||
const p = doRefresh(persist)
|
||||
inFlightRefresh = p
|
||||
// Only clear if it still points to this promise — a logout (or a newer
|
||||
// refresh started after it) may have replaced inFlightRefresh meanwhile.
|
||||
p.finally(() => {
|
||||
if (inFlightRefresh === p) {
|
||||
inFlightRefresh = null
|
||||
}
|
||||
})
|
||||
return p
|
||||
}
|
||||
|
||||
async function doRefresh(persist: boolean): Promise<void> {
|
||||
// Snapshot the epoch so we can tell if a logout happened while we awaited.
|
||||
const epochAtStart = authEpoch
|
||||
const loggedOutSinceStart = () => authEpoch !== epochAtStart
|
||||
|
||||
// In desktop mode, refresh via IPC to the Electron main process
|
||||
if (isDesktopApp()) {
|
||||
const storedRefreshToken = localStorage.getItem('desktopOAuthRefreshToken')
|
||||
|
|
@ -53,6 +88,9 @@ export async function refreshToken(persist: boolean): Promise<void> {
|
|||
}
|
||||
try {
|
||||
const tokens = await refreshDesktopToken(window.API_URL, storedRefreshToken)
|
||||
if (loggedOutSinceStart()) {
|
||||
return
|
||||
}
|
||||
saveToken(tokens.access_token, persist)
|
||||
localStorage.setItem('desktopOAuthRefreshToken', tokens.refresh_token)
|
||||
} catch (e) {
|
||||
|
|
@ -65,7 +103,13 @@ export async function refreshToken(persist: boolean): Promise<void> {
|
|||
// if another tab refreshed while we were queued.
|
||||
const tokenBeforeLock = localStorage.getItem('token')
|
||||
|
||||
const doRefresh = async () => {
|
||||
const refreshUnderLock = async () => {
|
||||
// A logout may have happened while we waited for the lock — don't
|
||||
// re-adopt or re-fetch a token after the user signed out.
|
||||
if (loggedOutSinceStart()) {
|
||||
return
|
||||
}
|
||||
|
||||
// If the token in localStorage changed while waiting for the lock,
|
||||
// another tab already refreshed. Just adopt the new token.
|
||||
const currentToken = localStorage.getItem('token')
|
||||
|
|
@ -78,6 +122,9 @@ export async function refreshToken(persist: boolean): Promise<void> {
|
|||
const HTTP = HTTPFactory()
|
||||
try {
|
||||
const response = await HTTP.post('user/token/refresh')
|
||||
if (loggedOutSinceStart()) {
|
||||
return
|
||||
}
|
||||
saveToken(response.data.token, persist)
|
||||
} catch (e) {
|
||||
throw new Error('Error renewing token: ', {cause: e})
|
||||
|
|
@ -85,10 +132,10 @@ export async function refreshToken(persist: boolean): Promise<void> {
|
|||
}
|
||||
|
||||
if (navigator.locks) {
|
||||
await navigator.locks.request('vikunja-token-refresh', doRefresh)
|
||||
await navigator.locks.request('vikunja-token-refresh', refreshUnderLock)
|
||||
} else {
|
||||
// Fallback for environments without Web Locks (e.g. insecure HTTP)
|
||||
await doRefresh()
|
||||
await refreshUnderLock()
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,139 @@
|
|||
import {describe, it, expect, beforeEach, vi} from 'vitest'
|
||||
import {setActivePinia, createPinia} from 'pinia'
|
||||
|
||||
import {useAuthStore} from './auth'
|
||||
import {AUTH_TYPES} from '@/modelTypes/IUser'
|
||||
|
||||
const {refreshTokenMock, routerPushMock, getTokenMock} = vi.hoisted(() => ({
|
||||
refreshTokenMock: vi.fn(),
|
||||
routerPushMock: vi.fn(),
|
||||
getTokenMock: vi.fn(() => null as string | null),
|
||||
}))
|
||||
|
||||
vi.mock('@/helpers/auth', () => ({
|
||||
refreshToken: refreshTokenMock,
|
||||
getToken: getTokenMock,
|
||||
saveToken: vi.fn(),
|
||||
removeToken: vi.fn(),
|
||||
}))
|
||||
|
||||
vi.mock('@/router', () => ({
|
||||
default: {push: routerPushMock},
|
||||
}))
|
||||
|
||||
vi.mock('@/composables/useWebSocket', () => ({
|
||||
useWebSocket: () => ({disconnect: vi.fn(), connect: vi.fn()}),
|
||||
}))
|
||||
|
||||
function fakeHttp() {
|
||||
return {
|
||||
post: vi.fn().mockResolvedValue({data: {}}),
|
||||
get: vi.fn().mockResolvedValue({data: {}}),
|
||||
request: vi.fn().mockResolvedValue({data: {}}),
|
||||
interceptors: {
|
||||
request: {use: vi.fn()},
|
||||
response: {use: vi.fn()},
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
vi.mock('@/helpers/fetcher', () => ({
|
||||
HTTPFactory: () => fakeHttp(),
|
||||
AuthenticatedHTTPFactory: () => fakeHttp(),
|
||||
getApiBaseUrl: () => 'http://localhost/api/v1/',
|
||||
}))
|
||||
|
||||
vi.mock('@/helpers/redirectToProvider', () => ({
|
||||
getRedirectUrlFromCurrentFrontendPath: vi.fn(),
|
||||
redirectToProvider: vi.fn(),
|
||||
redirectToProviderOnLogout: vi.fn(),
|
||||
}))
|
||||
|
||||
// A refresh failure that looks like a real network/HTTP error so renewToken's
|
||||
// "is this a genuine logout?" check (it inspects the error cause's status) fires.
|
||||
function refreshError() {
|
||||
return new Error('Error renewing token: ', {
|
||||
cause: {response: {status: 401}},
|
||||
})
|
||||
}
|
||||
|
||||
// A JWT carrying a not-yet-expired user session, so the checkAuth() call that
|
||||
// renewToken() runs after a successful refresh treats the session as live.
|
||||
function freshUserJwt() {
|
||||
const payload = {
|
||||
id: 1,
|
||||
type: AUTH_TYPES.USER,
|
||||
exp: Math.floor(Date.now() / 1000) + 3600,
|
||||
}
|
||||
const encoded = btoa(JSON.stringify(payload))
|
||||
return `header.${encoded}.signature`
|
||||
}
|
||||
|
||||
describe('auth store renewToken retry (issue #2863)', () => {
|
||||
beforeEach(() => {
|
||||
setActivePinia(createPinia())
|
||||
refreshTokenMock.mockReset()
|
||||
routerPushMock.mockReset()
|
||||
getTokenMock.mockReset().mockReturnValue(null)
|
||||
})
|
||||
|
||||
function setupExpiredUserSession(store: ReturnType<typeof useAuthStore>) {
|
||||
store.setAuthenticated(true)
|
||||
// Expired exp so renewToken treats a refresh failure as a real logout.
|
||||
store.setUser({
|
||||
id: 1,
|
||||
type: AUTH_TYPES.USER,
|
||||
exp: Math.floor(Date.now() / 1000) - 60,
|
||||
} as never, false)
|
||||
}
|
||||
|
||||
it('does NOT log out when the first refresh fails but the retry succeeds', async () => {
|
||||
const store = useAuthStore()
|
||||
setupExpiredUserSession(store)
|
||||
|
||||
// The retry "succeeds" only if it actually leaves a usable token behind:
|
||||
// renewToken() runs checkAuth() afterwards, which reads getToken(). Start
|
||||
// with no token, then hand back a fresh JWT once the refresh resolves.
|
||||
getTokenMock.mockReturnValue(null)
|
||||
refreshTokenMock
|
||||
.mockRejectedValueOnce(refreshError())
|
||||
.mockImplementationOnce(async () => {
|
||||
getTokenMock.mockReturnValue(freshUserJwt())
|
||||
})
|
||||
|
||||
await store.renewToken()
|
||||
|
||||
// Two refresh attempts: the initial one and the single retry.
|
||||
expect(refreshTokenMock).toHaveBeenCalledTimes(2)
|
||||
// The retry recovered the session: the user is still authenticated...
|
||||
expect(store.authenticated).toBe(true)
|
||||
// ...and was not bounced to login.
|
||||
expect(routerPushMock).not.toHaveBeenCalledWith({name: 'user.login'})
|
||||
})
|
||||
|
||||
it('logs out when BOTH the refresh and its retry fail', async () => {
|
||||
const store = useAuthStore()
|
||||
setupExpiredUserSession(store)
|
||||
|
||||
refreshTokenMock
|
||||
.mockRejectedValueOnce(refreshError())
|
||||
.mockRejectedValueOnce(refreshError())
|
||||
|
||||
await store.renewToken()
|
||||
|
||||
expect(refreshTokenMock).toHaveBeenCalledTimes(2)
|
||||
expect(routerPushMock).toHaveBeenCalledWith({name: 'user.login'})
|
||||
})
|
||||
|
||||
it('retries exactly once (no infinite loop) when the session is genuinely dead', async () => {
|
||||
const store = useAuthStore()
|
||||
setupExpiredUserSession(store)
|
||||
|
||||
refreshTokenMock.mockRejectedValue(refreshError())
|
||||
|
||||
await store.renewToken()
|
||||
|
||||
// Initial attempt + exactly one retry — never more.
|
||||
expect(refreshTokenMock).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
})
|
||||
|
|
@ -55,6 +55,17 @@ function redirectToSpecifiedProvider() {
|
|||
}
|
||||
}
|
||||
|
||||
// A race-loser's refresh fails but the rotated cookie is already valid, so a
|
||||
// second attempt succeeds — recovering what would otherwise be a spurious
|
||||
// logout. Exactly one retry: a genuinely dead session still logs out, no loop.
|
||||
async function refreshTokenWithRetry(persist: boolean): Promise<void> {
|
||||
try {
|
||||
await refreshToken(persist)
|
||||
} catch {
|
||||
await refreshToken(persist)
|
||||
}
|
||||
}
|
||||
|
||||
function getLoggedInVia(): string | null {
|
||||
return localStorage.getItem('loggedInViaProvider')
|
||||
}
|
||||
|
|
@ -352,7 +363,7 @@ export const useAuthStore = defineStore('auth', () => {
|
|||
// refresh before giving up. This lets users who reopen the app
|
||||
// after the short JWT TTL seamlessly resume their session.
|
||||
try {
|
||||
await refreshToken(true)
|
||||
await refreshTokenWithRetry(true)
|
||||
const freshJwt = getToken()
|
||||
if (freshJwt) {
|
||||
const b64 = freshJwt.split('.')[1].replace(/-/g, '+').replace(/_/g, '/')
|
||||
|
|
@ -512,7 +523,7 @@ export const useAuthStore = defineStore('auth', () => {
|
|||
saveToken(response.data.token, false)
|
||||
} else {
|
||||
// User sessions renew via the refresh-token cookie.
|
||||
await refreshToken(true)
|
||||
await refreshTokenWithRetry(true)
|
||||
}
|
||||
await checkAuth()
|
||||
} catch (e) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue