-
Notifications
You must be signed in to change notification settings - Fork 442
Fix session cookie creation race: skip initial token refresh, wrap extension auth hooks #6563
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
Changes from 1 commit
3719a46
93c3c3f
b4cfaf7
fd75975
17f2048
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 |
|---|---|---|
|
|
@@ -11,12 +11,14 @@ import { useMenuItemStore } from '@/stores/menuItemStore' | |
| import { useWidgetStore } from '@/stores/widgetStore' | ||
| import { useBottomPanelStore } from '@/stores/workspace/bottomPanelStore' | ||
| import type { ComfyExtension } from '@/types/comfy' | ||
| import type { AuthUserInfo } from '@/types/authTypes' | ||
|
|
||
| export const useExtensionService = () => { | ||
| const extensionStore = useExtensionStore() | ||
| const settingStore = useSettingStore() | ||
| const keybindingStore = useKeybindingStore() | ||
| const { wrapWithErrorHandling } = useErrorHandling() | ||
| const { wrapWithErrorHandling, wrapWithErrorHandlingAsync } = | ||
| useErrorHandling() | ||
|
|
||
| /** | ||
| * Loads all extensions from the API into the window in parallel | ||
|
|
@@ -77,22 +79,31 @@ export const useExtensionService = () => { | |
|
|
||
| if (extension.onAuthUserResolved) { | ||
| const { onUserResolved } = useCurrentUser() | ||
| const handleUserResolved = wrapWithErrorHandlingAsync( | ||
benceruleanlu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| (user: AuthUserInfo) => extension.onAuthUserResolved?.(user, app) | ||
| ) | ||
| onUserResolved((user) => { | ||
| void extension.onAuthUserResolved?.(user, app) | ||
| void handleUserResolved(user) | ||
benceruleanlu marked this conversation as resolved.
Show resolved
Hide resolved
Member
Author
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. Addressed: the async wrapper now adds structured error context for auth hooks — we log the extension name and the hook (e.g., |
||
| }) | ||
| } | ||
|
|
||
| if (extension.onAuthTokenRefreshed) { | ||
| const { onTokenRefreshed } = useCurrentUser() | ||
| const handleTokenRefreshed = wrapWithErrorHandlingAsync(() => | ||
| extension.onAuthTokenRefreshed?.() | ||
| ) | ||
| onTokenRefreshed(() => { | ||
| void extension.onAuthTokenRefreshed?.() | ||
| void handleTokenRefreshed() | ||
|
Member
Author
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. Clarification: the wrapper’s return is intentionally discarded ( |
||
| }) | ||
| } | ||
|
|
||
| if (extension.onAuthUserLogout) { | ||
| const { onUserLogout } = useCurrentUser() | ||
| const handleUserLogout = wrapWithErrorHandlingAsync(() => | ||
| extension.onAuthUserLogout?.() | ||
| ) | ||
| onUserLogout(() => { | ||
| void extension.onAuthUserLogout?.() | ||
| void handleUserLogout() | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,11 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { | |
|
|
||
| // Token refresh trigger - increments when token is refreshed | ||
| const tokenRefreshTrigger = ref(0) | ||
| /** | ||
| * The user ID for which the initial ID token has been observed. | ||
| * When a token changes for the same user, that is a refresh. | ||
| */ | ||
| const lastTokenUserId = ref<string | null>(null) | ||
|
|
||
| // Providers | ||
| const googleProvider = new GoogleAuthProvider() | ||
|
|
@@ -93,6 +98,9 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { | |
| onAuthStateChanged(auth, (user) => { | ||
| currentUser.value = user | ||
| isInitialized.value = true | ||
| if (user === null) { | ||
| lastTokenUserId.value = null | ||
| } | ||
|
|
||
| // Reset balance when auth state changes | ||
| balance.value = null | ||
|
|
@@ -102,6 +110,11 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { | |
| // Listen for token refresh events | ||
| onIdTokenChanged(auth, (user) => { | ||
benceruleanlu marked this conversation as resolved.
Show resolved
Hide resolved
Member
Author
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. Clarification: the Pinia store is a singleton for the app’s lifetime, so we register one |
||
| if (user && isCloud) { | ||
| // Skip initial token change | ||
| if (lastTokenUserId.value !== user.uid) { | ||
benceruleanlu marked this conversation as resolved.
Show resolved
Hide resolved
Member
Author
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. Clarification: |
||
| lastTokenUserId.value = user.uid | ||
| return | ||
| } | ||
| tokenRefreshTrigger.value++ | ||
| } | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { createPinia, setActivePinia } from 'pinia' | ||
|
|
||
| vi.mock('@/platform/distribution/types', () => ({ | ||
| isCloud: true | ||
| })) | ||
|
|
||
| vi.mock('vuefire', () => ({ | ||
| useFirebaseAuth: vi.fn() | ||
| })) | ||
|
|
||
| vi.mock('firebase/auth', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('firebase/auth')>() | ||
| return { | ||
| ...actual, | ||
| onAuthStateChanged: vi.fn(), | ||
| onIdTokenChanged: vi.fn(), | ||
| setPersistence: vi.fn().mockResolvedValue(undefined), | ||
| GoogleAuthProvider: class { | ||
| addScope = vi.fn() | ||
| setCustomParameters = vi.fn() | ||
| }, | ||
| GithubAuthProvider: class { | ||
| addScope = vi.fn() | ||
| setCustomParameters = vi.fn() | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| import * as firebaseAuth from 'firebase/auth' | ||
| import * as vuefire from 'vuefire' | ||
|
|
||
| type MinimalUser = { uid: string } | ||
|
|
||
| /** Create a minimal user-like object with a stable uid */ | ||
| function makeUser(uid: string): MinimalUser { | ||
| return { uid } | ||
| } | ||
|
|
||
| describe('firebaseAuthStore token refresh gating', () => { | ||
| let onAuthStateChangedCallback: | ||
| | ((user: MinimalUser | null) => void) | ||
| | undefined | ||
| let onIdTokenChangedCallback: ((user: MinimalUser | null) => void) | undefined | ||
| let store: any | ||
|
|
||
| beforeEach(async () => { | ||
| vi.resetModules() | ||
| vi.resetAllMocks() | ||
| setActivePinia(createPinia()) | ||
|
|
||
| const authInstance = {} | ||
| vi.mocked(vuefire.useFirebaseAuth).mockReturnValue(authInstance as any) | ||
|
|
||
| vi.mocked(firebaseAuth.onAuthStateChanged).mockImplementation((...args) => { | ||
| const callback = args[1] as (user: MinimalUser | null) => void | ||
| onAuthStateChangedCallback = callback | ||
| return vi.fn() | ||
| }) | ||
|
|
||
| vi.mocked(firebaseAuth.onIdTokenChanged).mockImplementation((...args) => { | ||
| const callback = args[1] as (user: MinimalUser | null) => void | ||
| onIdTokenChangedCallback = callback | ||
| return vi.fn() | ||
| }) | ||
|
|
||
| const { useFirebaseAuthStore } = await import('@/stores/firebaseAuthStore') | ||
| store = useFirebaseAuthStore() | ||
| }) | ||
|
|
||
| it('skips initial token for a user and increments on subsequent refresh', () => { | ||
| const user = makeUser('user-123') | ||
|
|
||
| onIdTokenChangedCallback?.(user) | ||
| expect(store.tokenRefreshTrigger).toBe(0) | ||
|
|
||
| onIdTokenChangedCallback?.(user) | ||
| expect(store.tokenRefreshTrigger).toBe(1) | ||
| }) | ||
|
|
||
| it('does not increment when uid changes; increments on next refresh for new user', () => { | ||
| const userA = makeUser('user-a') | ||
| const userB = makeUser('user-b') | ||
|
|
||
| onIdTokenChangedCallback?.(userA) | ||
| expect(store.tokenRefreshTrigger).toBe(0) | ||
|
|
||
| onIdTokenChangedCallback?.(userB) | ||
| expect(store.tokenRefreshTrigger).toBe(0) | ||
|
|
||
| onIdTokenChangedCallback?.(userB) | ||
| expect(store.tokenRefreshTrigger).toBe(1) | ||
| }) | ||
|
|
||
| it('resets gating after logout; first token after logout is skipped', () => { | ||
| const user = makeUser('user-x') | ||
|
|
||
| onIdTokenChangedCallback?.(user) | ||
| onIdTokenChangedCallback?.(user) | ||
| expect(store.tokenRefreshTrigger).toBe(1) | ||
|
|
||
| onAuthStateChangedCallback?.(null) | ||
|
|
||
| onIdTokenChangedCallback?.(user) | ||
| expect(store.tokenRefreshTrigger).toBe(1) | ||
|
|
||
| onIdTokenChangedCallback?.(user) | ||
| expect(store.tokenRefreshTrigger).toBe(2) | ||
| }) | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.