From 3630dd5a80d30af367c9aecf75613ca5835926c0 Mon Sep 17 00:00:00 2001 From: Michael Chihlas Date: Thu, 7 May 2026 01:32:53 -0400 Subject: [PATCH] fix(auth): mark store authenticated after OAuth setTokens setTokens() previously only set { token } without flipping isAuthenticated, so after the OAuth callback exchange the store had fresh tokens but ProtectedRoute still saw isAuthenticated === false and bounced the user to /landing before fetchUser() could complete. Storing tokens implies an active session, so set isAuthenticated: true inside setTokens. The other caller (refresh interceptor in api/client.ts) runs from an already-authenticated session, so the flag flip is a no-op there. Tests: - new src/store/authStore.test.ts covers the setTokens contract - src/pages/__tests__/OAuthCallbackPage.test.tsx adds a successful- callback case asserting setTokens + fetchUser are invoked with the exchanged tokens Co-Authored-By: Claude Opus 4.7 --- .../__tests__/OAuthCallbackPage.test.tsx | 44 +++++++++++- frontend/src/store/authStore.test.ts | 68 +++++++++++++++++++ frontend/src/store/authStore.ts | 8 ++- 3 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 frontend/src/store/authStore.test.ts diff --git a/frontend/src/pages/__tests__/OAuthCallbackPage.test.tsx b/frontend/src/pages/__tests__/OAuthCallbackPage.test.tsx index 392e9403..ecb0f341 100644 --- a/frontend/src/pages/__tests__/OAuthCallbackPage.test.tsx +++ b/frontend/src/pages/__tests__/OAuthCallbackPage.test.tsx @@ -13,10 +13,13 @@ vi.mock('@/api/auth', () => ({ }, })) +const mockSetTokens = vi.fn() +const mockFetchUser = vi.fn().mockResolvedValue(undefined) + vi.mock('@/store/authStore', () => ({ useAuthStore: () => ({ - setTokens: vi.fn(), - fetchUser: vi.fn().mockResolvedValue(undefined), + setTokens: mockSetTokens, + fetchUser: mockFetchUser, }), })) @@ -79,3 +82,40 @@ describe('OAuthCallbackPage CSRF state validation', () => { expect(authApi.googleCallback).not.toHaveBeenCalled() }) }) + +describe('OAuthCallbackPage successful callback', () => { + beforeEach(() => { + sessionStorage.clear() + vi.clearAllMocks() + localStorage.clear() + }) + + afterEach(() => { + sessionStorage.clear() + localStorage.clear() + }) + + it('persists tokens via setTokens (which marks the store authenticated) and fetches the user', async () => { + sessionStorage.setItem('rf-oauth-state', 'csrf-value') + ;(authApi.googleCallback as ReturnType).mockResolvedValue({ + access_token: 'access-123', + refresh_token: 'refresh-456', + token_type: 'bearer', + is_new_user: false, + }) + + renderAt('/auth/google/callback?code=auth-code-123&state=csrf-value') + + await waitFor(() => { + expect(mockSetTokens).toHaveBeenCalledWith({ + access_token: 'access-123', + refresh_token: 'refresh-456', + token_type: 'bearer', + }) + }) + expect(mockFetchUser).toHaveBeenCalled() + // Tokens are also persisted for the apiClient interceptor. + expect(localStorage.getItem('access_token')).toBe('access-123') + expect(localStorage.getItem('refresh_token')).toBe('refresh-456') + }) +}) diff --git a/frontend/src/store/authStore.test.ts b/frontend/src/store/authStore.test.ts new file mode 100644 index 00000000..765b1d8e --- /dev/null +++ b/frontend/src/store/authStore.test.ts @@ -0,0 +1,68 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { useAuthStore } from './authStore' +import type { Token } from '@/types' + +// Avoid pulling in real analytics / Sentry side effects during tests. +vi.mock('@/lib/analytics', () => ({ + identifyUser: vi.fn(), + resetAnalytics: vi.fn(), + analytics: { + loginSuccess: vi.fn(), + accountCreated: vi.fn(), + }, +})) + +vi.mock('@sentry/react', () => ({ + setUser: vi.fn(), +})) + +describe('authStore.setTokens', () => { + beforeEach(() => { + // Reset store to initial state between tests. + useAuthStore.setState({ + user: null, + token: null, + account: null, + subscription: null, + isAuthenticated: false, + isLoading: false, + error: null, + }) + }) + + it('marks the store as authenticated and persists the token', () => { + const fakeToken: Token = { + access_token: 'access-abc', + refresh_token: 'refresh-xyz', + token_type: 'bearer', + } + + useAuthStore.getState().setTokens(fakeToken) + + const state = useAuthStore.getState() + expect(state.token).toEqual(fakeToken) + expect(state.isAuthenticated).toBe(true) + }) + + it('keeps isAuthenticated true when called again (refresh-token path)', () => { + // Simulate an already-authenticated session (refresh interceptor case). + useAuthStore.setState({ + token: { + access_token: 'old', + refresh_token: 'old-r', + token_type: 'bearer', + }, + isAuthenticated: true, + }) + + useAuthStore.getState().setTokens({ + access_token: 'new', + refresh_token: 'new-r', + token_type: 'bearer', + }) + + const state = useAuthStore.getState() + expect(state.token?.access_token).toBe('new') + expect(state.isAuthenticated).toBe(true) + }) +}) diff --git a/frontend/src/store/authStore.ts b/frontend/src/store/authStore.ts index 68e40459..fb6f3ba3 100644 --- a/frontend/src/store/authStore.ts +++ b/frontend/src/store/authStore.ts @@ -131,7 +131,13 @@ export const useAuthStore = create()( } }, - setTokens: (token: Token) => set({ token }), + // Storing tokens implies an active session — mark the store as + // authenticated so doesn't bounce the user back to + // /landing while fetchUser() is still inflight (e.g. immediately after + // the OAuth callback exchange). The refresh interceptor in api/client.ts + // also calls this; that path is already authenticated, so flipping the + // flag has no effect there. + setTokens: (token: Token) => set({ token, isAuthenticated: true }), clearError: () => set({ error: null }), setLoading: (loading: boolean) => set({ isLoading: loading }), }),