-
Notifications
You must be signed in to change notification settings - Fork 397
Cloud Auth Backport #6195
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
Cloud Auth Backport #6195
Changes from all commits
1a84fcf
b1dc9c9
aecaf0e
02be1ce
8791eac
860e05d
048034c
74ba8bf
c34e34f
0b868bf
71c8606
bd051ee
4b80ee9
ec23f33
428ec17
02cbbca
e77f24a
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 |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { promiseTimeout, until } from '@vueuse/core' | ||
| import axios from 'axios' | ||
| import { get } from 'es-toolkit/compat' | ||
|
|
||
|
|
@@ -6,6 +7,7 @@ import type { | |
| ModelFile, | ||
| ModelFolderInfo | ||
| } from '@/platform/assets/schemas/assetSchema' | ||
| import { isCloud } from '@/platform/distribution/types' | ||
| import { useToastStore } from '@/platform/updates/common/toastStore' | ||
| import { type WorkflowTemplates } from '@/platform/workflow/templates/types/template' | ||
| import type { | ||
|
|
@@ -42,6 +44,8 @@ import type { | |
| UserDataFullInfo | ||
| } from '@/schemas/apiSchema' | ||
| import type { ComfyNodeDef } from '@/schemas/nodeDefSchema' | ||
| import type { useFirebaseAuthStore } from '@/stores/firebaseAuthStore' | ||
| import type { AuthHeader } from '@/types/authTypes' | ||
| import type { NodeExecutionId } from '@/types/nodeIdentification' | ||
|
|
||
| interface QueuePromptRequestBody { | ||
|
|
@@ -200,6 +204,16 @@ type SimpleApiEvents = keyof PickNevers<ApiEventTypes> | |
| /** Keys (names) of API events that pass a {@link CustomEvent} `detail` object. */ | ||
| type ComplexApiEvents = keyof NeverNever<ApiEventTypes> | ||
|
|
||
| function addHeaderEntry(headers: HeadersInit, key: string, value: string) { | ||
| if (Array.isArray(headers)) { | ||
| headers.push([key, value]) | ||
| } else if (headers instanceof Headers) { | ||
| headers.set(key, value) | ||
| } else { | ||
| headers[key] = value | ||
| } | ||
| } | ||
|
|
||
| /** EventTarget typing has no generic capability. */ | ||
| export interface ComfyApi extends EventTarget { | ||
| addEventListener<TEvent extends keyof ApiEvents>( | ||
|
|
@@ -263,6 +277,11 @@ export class ComfyApi extends EventTarget { | |
| user: string | ||
| socket: WebSocket | null = null | ||
|
|
||
| /** | ||
| * Cache Firebase auth store composable function. | ||
| */ | ||
| private authStoreComposable?: typeof useFirebaseAuthStore | ||
|
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. note (no actionable): So I guess caching the function vs. the returned store is essentially the same since Pinia store is singleton by nature. |
||
|
|
||
| reportedUnknownMessageTypes = new Set<string>() | ||
|
|
||
| /** | ||
|
|
@@ -317,25 +336,77 @@ export class ComfyApi extends EventTarget { | |
| return this.api_base + route | ||
| } | ||
|
|
||
| fetchApi(route: string, options?: RequestInit) { | ||
| if (!options) { | ||
| options = {} | ||
| } | ||
| if (!options.headers) { | ||
| options.headers = {} | ||
| /** | ||
| * Gets the Firebase auth store instance using cached composable function. | ||
| * Caches the composable function on first call, then reuses it. | ||
| * Returns null for non-cloud distributions. | ||
| * @returns The Firebase auth store instance, or null if not in cloud | ||
| */ | ||
| private async getAuthStore() { | ||
| if (isCloud) { | ||
| if (!this.authStoreComposable) { | ||
| const module = await import('@/stores/firebaseAuthStore') | ||
|
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. question (non-blocking): Preventing circular import or is there a timing issue? 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. Circular import. |
||
| this.authStoreComposable = module.useFirebaseAuthStore | ||
| } | ||
|
|
||
| return this.authStoreComposable() | ||
| } | ||
| if (!options.cache) { | ||
| options.cache = 'no-cache' | ||
| } | ||
|
|
||
| /** | ||
| * Waits for Firebase auth to be initialized before proceeding. | ||
| * Includes 10-second timeout to prevent infinite hanging. | ||
| */ | ||
| private async waitForAuthInitialization(): Promise<void> { | ||
| if (isCloud) { | ||
| const authStore = await this.getAuthStore() | ||
| if (!authStore) return | ||
|
|
||
| if (authStore.isInitialized) return | ||
|
|
||
| try { | ||
| await Promise.race([ | ||
| until(authStore.isInitialized), | ||
| promiseTimeout(10000) | ||
| ]) | ||
| } catch { | ||
| console.warn('Firebase auth initialization timeout after 10 seconds') | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async fetchApi(route: string, options?: RequestInit) { | ||
| const headers: HeadersInit = options?.headers ?? {} | ||
|
|
||
| if (isCloud) { | ||
| await this.waitForAuthInitialization() | ||
|
|
||
| if (Array.isArray(options.headers)) { | ||
| options.headers.push(['Comfy-User', this.user]) | ||
| } else if (options.headers instanceof Headers) { | ||
| options.headers.set('Comfy-User', this.user) | ||
| } else { | ||
| options.headers['Comfy-User'] = this.user | ||
| // Get Firebase JWT token if user is logged in | ||
| const getAuthHeaderIfAvailable = async (): Promise<AuthHeader | null> => { | ||
arjansingh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| try { | ||
| const authStore = await this.getAuthStore() | ||
| return authStore ? await authStore.getAuthHeader() : null | ||
| } catch (error) { | ||
| console.warn('Failed to get auth header:', error) | ||
| return null | ||
| } | ||
| } | ||
|
|
||
| const authHeader = await getAuthHeaderIfAvailable() | ||
|
|
||
| if (authHeader) { | ||
| for (const [key, value] of Object.entries(authHeader)) { | ||
| addHeaderEntry(headers, key, value) | ||
| } | ||
| } | ||
| } | ||
| return fetch(this.apiURL(route), options) | ||
|
|
||
| addHeaderEntry(headers, 'Comfy-User', this.user) | ||
| return fetch(this.apiURL(route), { | ||
| cache: 'no-cache', | ||
| ...options, | ||
| headers | ||
|
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. double check for override test cases
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. LGTM |
||
| }) | ||
| } | ||
|
|
||
| override addEventListener<TEvent extends keyof ApiEvents>( | ||
|
|
@@ -402,19 +473,44 @@ export class ComfyApi extends EventTarget { | |
| * Creates and connects a WebSocket for realtime updates | ||
| * @param {boolean} isReconnect If the socket is connection is a reconnect attempt | ||
| */ | ||
| #createSocket(isReconnect?: boolean) { | ||
| private async createSocket(isReconnect?: boolean) { | ||
| if (this.socket) { | ||
| return | ||
| } | ||
|
|
||
| let opened = false | ||
| let existingSession = window.name | ||
|
|
||
| // Build WebSocket URL with query parameters | ||
| const params = new URLSearchParams() | ||
|
|
||
| if (existingSession) { | ||
| existingSession = '?clientId=' + existingSession | ||
| params.set('clientId', existingSession) | ||
| } | ||
| this.socket = new WebSocket( | ||
| `ws${window.location.protocol === 'https:' ? 's' : ''}://${this.api_host}${this.api_base}/ws${existingSession}` | ||
| ) | ||
|
|
||
| // Get auth token and set cloud params if available | ||
| if (isCloud) { | ||
| try { | ||
| const authStore = await this.getAuthStore() | ||
| const authToken = await authStore?.getIdToken() | ||
| if (authToken) { | ||
| params.set('token', authToken) | ||
| } | ||
| } catch (error) { | ||
| // Continue without auth token if there's an error | ||
| console.warn( | ||
| 'Could not get auth token for WebSocket connection:', | ||
| error | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| const protocol = window.location.protocol === 'https:' ? 'wss' : 'ws' | ||
| const baseUrl = `${protocol}://${this.api_host}${this.api_base}/ws` | ||
| const query = params.toString() | ||
| const wsUrl = query ? `${baseUrl}?${query}` : baseUrl | ||
|
|
||
| this.socket = new WebSocket(wsUrl) | ||
| this.socket.binaryType = 'arraybuffer' | ||
|
|
||
| this.socket.addEventListener('open', () => { | ||
|
|
@@ -441,9 +537,9 @@ export class ComfyApi extends EventTarget { | |
| }) | ||
|
|
||
| this.socket.addEventListener('close', () => { | ||
| setTimeout(() => { | ||
| setTimeout(async () => { | ||
| this.socket = null | ||
| this.#createSocket(true) | ||
| await this.createSocket(true) | ||
| }, 300) | ||
| if (opened) { | ||
| this.dispatchCustomEvent('status', null) | ||
|
|
@@ -579,7 +675,7 @@ export class ComfyApi extends EventTarget { | |
| * Initialises sockets and realtime updates | ||
| */ | ||
| init() { | ||
| this.#createSocket() | ||
| this.createSocket() | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: In the future, we will route to the onboarding flow from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That is what I am thinking. Replace the modal with @viva-jinyi's login and auth pages after history v2.