-
Notifications
You must be signed in to change notification settings - Fork 395
DONOTMERGE feat: add retry logic for session cookie creation with token refresh #6540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,276 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { useSessionCookie } from './useSessionCookie' | ||
|
|
||
| // Mock fetch | ||
| const mockFetch = vi.fn() | ||
| vi.stubGlobal('fetch', mockFetch) | ||
|
|
||
| // Mock api.apiURL | ||
| vi.mock('@/scripts/api', () => ({ | ||
| api: { | ||
| apiURL: vi.fn((path: string) => `https://test-api.com${path}`) | ||
| } | ||
| })) | ||
|
|
||
| // Mock isCloud to always return true for testing | ||
| vi.mock('@/platform/distribution/types', () => ({ | ||
| isCloud: true | ||
| })) | ||
|
|
||
| // Mock useFirebaseAuthStore | ||
| const mockGetAuthHeader = vi.fn() | ||
| vi.mock('@/stores/firebaseAuthStore', () => ({ | ||
| useFirebaseAuthStore: vi.fn(() => ({ | ||
| getAuthHeader: mockGetAuthHeader | ||
| })) | ||
| })) | ||
|
|
||
| describe('useSessionCookie', () => { | ||
| beforeEach(() => { | ||
| vi.resetAllMocks() | ||
| mockFetch.mockReset() | ||
| mockGetAuthHeader.mockReset() | ||
| }) | ||
|
|
||
| describe('createSession', () => { | ||
| it('should succeed on first attempt', async () => { | ||
| // Mock successful auth header and API response | ||
| mockGetAuthHeader.mockResolvedValue({ Authorization: 'Bearer token' }) | ||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| status: 200 | ||
| }) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
| await createSession() | ||
|
|
||
| expect(mockGetAuthHeader).toHaveBeenCalledWith(false) // First attempt with cached token | ||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| 'https://test-api.com/auth/session', | ||
| { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| Authorization: 'Bearer token', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| it('should retry with fresh token after 401 error and succeed', async () => { | ||
| // First attempt: auth header success but API returns 401 | ||
| // Second attempt: fresh token and API success | ||
| mockGetAuthHeader | ||
| .mockResolvedValueOnce({ Authorization: 'Bearer cached-token' }) | ||
| .mockResolvedValueOnce({ Authorization: 'Bearer fresh-token' }) | ||
|
|
||
| mockFetch | ||
| .mockResolvedValueOnce({ | ||
| ok: false, | ||
| status: 401, | ||
| statusText: 'Unauthorized', | ||
| json: () => Promise.resolve({ message: 'Token expired' }) | ||
| }) | ||
| .mockResolvedValueOnce({ | ||
| ok: true, | ||
| status: 200 | ||
| }) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
| await createSession() | ||
|
|
||
| expect(mockGetAuthHeader).toHaveBeenCalledTimes(2) | ||
| expect(mockGetAuthHeader).toHaveBeenNthCalledWith(1, false) // First attempt with cached token | ||
| expect(mockGetAuthHeader).toHaveBeenNthCalledWith(2, true) // Second attempt with forced refresh | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledTimes(2) | ||
| expect(mockFetch).toHaveBeenNthCalledWith( | ||
| 1, | ||
| 'https://test-api.com/auth/session', | ||
| { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| Authorization: 'Bearer cached-token', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| } | ||
| ) | ||
| expect(mockFetch).toHaveBeenNthCalledWith( | ||
| 2, | ||
| 'https://test-api.com/auth/session', | ||
| { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| Authorization: 'Bearer fresh-token', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| it('should fail after all retries with API error', async () => { | ||
| // All attempts return auth headers but API always fails | ||
| mockGetAuthHeader.mockResolvedValue({ Authorization: 'Bearer token' }) | ||
| mockFetch.mockResolvedValue({ | ||
| ok: false, | ||
| status: 500, | ||
| statusText: 'Internal Server Error', | ||
| json: () => Promise.resolve({ message: 'Server error' }) | ||
| }) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
|
|
||
| await expect(createSession()).rejects.toThrow( | ||
| 'Failed to create session: Server error' | ||
| ) | ||
|
|
||
| expect(mockGetAuthHeader).toHaveBeenCalledTimes(3) | ||
| expect(mockFetch).toHaveBeenCalledTimes(3) | ||
| }) | ||
|
|
||
| it('should succeed after auth header null then fresh token success', async () => { | ||
| // First attempt: no auth header (token timing issue) | ||
| // Second attempt: fresh token success | ||
| mockGetAuthHeader | ||
| .mockResolvedValueOnce(null) | ||
| .mockResolvedValueOnce({ Authorization: 'Bearer fresh-token' }) | ||
|
|
||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| status: 200 | ||
| }) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
| await createSession() | ||
|
|
||
| expect(mockGetAuthHeader).toHaveBeenCalledTimes(2) | ||
| expect(mockGetAuthHeader).toHaveBeenNthCalledWith(1, false) // First attempt with cached token | ||
| expect(mockGetAuthHeader).toHaveBeenNthCalledWith(2, true) // Second attempt with forced refresh | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledTimes(1) // Only called when auth header is available | ||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| 'https://test-api.com/auth/session', | ||
| { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| Authorization: 'Bearer fresh-token', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| it('should fail when no auth header is available after all retries', async () => { | ||
| // All attempts fail to get auth header (complete auth failure) | ||
| mockGetAuthHeader.mockResolvedValue(null) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
|
|
||
| await expect(createSession()).rejects.toThrow( | ||
| 'No auth header available for session creation after retries' | ||
| ) | ||
|
|
||
| expect(mockGetAuthHeader).toHaveBeenCalledTimes(3) | ||
| expect(mockFetch).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should handle mixed auth header failures and successes', async () => { | ||
| // First attempt: no auth header | ||
| // Second attempt: auth header but API fails | ||
| // Third attempt: auth header and API success | ||
| mockGetAuthHeader | ||
| .mockResolvedValueOnce(null) | ||
| .mockResolvedValueOnce({ Authorization: 'Bearer token' }) | ||
| .mockResolvedValueOnce({ Authorization: 'Bearer fresh-token' }) | ||
|
|
||
| mockFetch | ||
| .mockResolvedValueOnce({ | ||
| ok: false, | ||
| status: 401, | ||
| statusText: 'Unauthorized', | ||
| json: () => Promise.resolve({ message: 'Token expired' }) | ||
| }) | ||
| .mockResolvedValueOnce({ | ||
| ok: true, | ||
| status: 200 | ||
| }) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
| await createSession() | ||
|
|
||
| expect(mockGetAuthHeader).toHaveBeenCalledTimes(3) | ||
| expect(mockFetch).toHaveBeenCalledTimes(2) // Only called when auth header is available | ||
| }) | ||
|
|
||
| it('should handle JSON parsing errors gracefully', async () => { | ||
| mockGetAuthHeader.mockResolvedValue({ Authorization: 'Bearer token' }) | ||
| mockFetch.mockResolvedValue({ | ||
| ok: false, | ||
| status: 500, | ||
| statusText: 'Internal Server Error', | ||
| json: () => Promise.reject(new Error('Invalid JSON')) | ||
| }) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
|
|
||
| await expect(createSession()).rejects.toThrow( | ||
| 'Failed to create session: Internal Server Error' | ||
| ) | ||
| }) | ||
| }) | ||
|
|
||
| describe('deleteSession', () => { | ||
| it('should successfully delete session', async () => { | ||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| status: 200 | ||
| }) | ||
|
|
||
| const { deleteSession } = useSessionCookie() | ||
| await deleteSession() | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| 'https://test-api.com/auth/session', | ||
| { | ||
| method: 'DELETE', | ||
| credentials: 'include' | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| it('should handle delete session errors', async () => { | ||
| mockFetch.mockResolvedValue({ | ||
| ok: false, | ||
| status: 404, | ||
| statusText: 'Not Found', | ||
| json: () => Promise.resolve({ message: 'Session not found' }) | ||
| }) | ||
|
|
||
| const { deleteSession } = useSessionCookie() | ||
|
|
||
| await expect(deleteSession()).rejects.toThrow( | ||
| 'Failed to delete session: Session not found' | ||
| ) | ||
| }) | ||
|
|
||
| it('should handle delete session JSON parsing errors', async () => { | ||
| mockFetch.mockResolvedValue({ | ||
| ok: false, | ||
| status: 500, | ||
| statusText: 'Internal Server Error', | ||
| json: () => Promise.reject(new Error('Invalid JSON')) | ||
| }) | ||
|
|
||
| const { deleteSession } = useSessionCookie() | ||
|
|
||
| await expect(deleteSession()).rejects.toThrow( | ||
| 'Failed to delete session: Internal Server Error' | ||
| ) | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,32 +10,52 @@ export const useSessionCookie = () => { | |
| /** | ||
| * Creates or refreshes the session cookie. | ||
| * Called after login and on token refresh. | ||
| * Implements retry logic with token refresh for handling timing issues. | ||
| */ | ||
| const createSession = async (): Promise<void> => { | ||
| if (!isCloud) return | ||
|
|
||
| const authStore = useFirebaseAuthStore() | ||
| const authHeader = await authStore.getAuthHeader() | ||
|
|
||
| if (!authHeader) { | ||
| throw new Error('No auth header available for session creation') | ||
| } | ||
| // Simple retry with forceRefresh for token timing issues | ||
| for (let attempt = 0; attempt < 3; attempt++) { | ||
viva-jinyi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // First attempt uses cached token, retries force refresh | ||
| const authHeader = await authStore.getAuthHeader(attempt > 0) | ||
|
|
||
| const response = await fetch(api.apiURL('/auth/session'), { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| ...authHeader, | ||
| 'Content-Type': 'application/json' | ||
| if (authHeader) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [architecture] high Priority Issue: Nested conditional and loop complexity increases cognitive load |
||
| // Successfully got auth header, proceed with session creation | ||
| const response = await fetch(api.apiURL('/auth/session'), { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| ...authHeader, | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }) | ||
|
|
||
| if (response.ok) { | ||
| return // Success | ||
| } | ||
|
|
||
| // API error - continue retry loop if not the last attempt | ||
| if (attempt === 2) { | ||
| const errorData = await response.json().catch(() => ({})) | ||
| throw new Error( | ||
viva-jinyi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| `Failed to create session: ${errorData.message || response.statusText}` | ||
| ) | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| const errorData = await response.json().catch(() => ({})) | ||
| throw new Error( | ||
| `Failed to create session: ${errorData.message || response.statusText}` | ||
| ) | ||
| // Exponential backoff before retry (except for last attempt) | ||
| if (attempt < 2) { | ||
| await new Promise((r) => setTimeout(r, Math.pow(2, attempt) * 500)) | ||
viva-jinyi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| // Failed to get auth header after 3 attempts | ||
| throw new Error( | ||
| 'No auth header available for session creation after retries' | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,10 +106,12 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { | |
| } | ||
| }) | ||
|
|
||
| const getIdToken = async (): Promise<string | undefined> => { | ||
| const getIdToken = async ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [quality] high Priority Issue: Missing JSDoc parameter documentation for new forceRefresh parameter |
||
| forceRefresh = false | ||
| ): Promise<string | undefined> => { | ||
| if (!currentUser.value) return | ||
| try { | ||
| return await currentUser.value.getIdToken() | ||
| return await currentUser.value.getIdToken(forceRefresh) | ||
| } catch (error: unknown) { | ||
| if ( | ||
| error instanceof FirebaseError && | ||
|
|
@@ -135,14 +137,17 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { | |
| * 1. Firebase authentication token (if user is logged in) | ||
| * 2. API key (if stored in the browser's credential manager) | ||
| * | ||
| * @param forceRefresh - If true, forces a refresh of the Firebase token | ||
| * @returns {Promise<AuthHeader | null>} | ||
| * - A LoggedInAuthHeader with Bearer token if Firebase authenticated | ||
| * - An ApiKeyAuthHeader with X-API-KEY if API key exists | ||
| * - null if neither authentication method is available | ||
| */ | ||
| const getAuthHeader = async (): Promise<AuthHeader | null> => { | ||
| const getAuthHeader = async ( | ||
viva-jinyi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| forceRefresh = false | ||
| ): Promise<AuthHeader | null> => { | ||
| // If available, set header with JWT used to identify the user to Firebase service | ||
| const token = await getIdToken() | ||
| const token = await getIdToken(forceRefresh) | ||
| if (token) { | ||
| return { | ||
| Authorization: `Bearer ${token}` | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.