-
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 all commits
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,17 @@ 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, | ||
| toastErrorHandler | ||
| } = useErrorHandling() | ||
|
|
||
| /** | ||
| * Loads all extensions from the API into the window in parallel | ||
|
|
@@ -77,22 +82,55 @@ export const useExtensionService = () => { | |
|
|
||
| if (extension.onAuthUserResolved) { | ||
| const { onUserResolved } = useCurrentUser() | ||
| const handleUserResolved = wrapWithErrorHandlingAsync( | ||
| (user: AuthUserInfo) => extension.onAuthUserResolved?.(user, app), | ||
| (error) => { | ||
| console.error('[Extension Auth Hook Error]', { | ||
| extension: extension.name, | ||
| hook: 'onAuthUserResolved', | ||
| error | ||
| }) | ||
| toastErrorHandler(error) | ||
| } | ||
| ) | ||
| 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?.(), | ||
| (error) => { | ||
| console.error('[Extension Auth Hook Error]', { | ||
| extension: extension.name, | ||
| hook: 'onAuthTokenRefreshed', | ||
| error | ||
| }) | ||
| toastErrorHandler(error) | ||
| } | ||
| ) | ||
| 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?.(), | ||
| (error) => { | ||
| console.error('[Extension Auth Hook Error]', { | ||
| extension: extension.name, | ||
| hook: 'onAuthUserLogout', | ||
| error | ||
| }) | ||
| toastErrorHandler(error) | ||
| } | ||
| ) | ||
| 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++ | ||
| } | ||
| }) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.