From ca0937479d3ab0c1b26cd9957d18f5366938aac6 Mon Sep 17 00:00:00 2001 From: Arjan Singh <1598641+arjansingh@users.noreply.github.com> Date: Fri, 22 Aug 2025 11:15:04 -0700 Subject: [PATCH] [fix] #4468 gracefully handle Firebase auth failure (#5144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [fix] gracefully handle Firebase auth failure * [test] Add failing tests to reproduce Firebase Auth network issue #4468 Add test cases that demonstrate the current problematic behavior where Firebase Auth makes network requests when offline without graceful error handling, causing toast error messages and degraded offline experience. Tests reproduce: - getIdToken() throwing auth/network-request-failed instead of returning null - getAuthHeader() failing to fallback gracefully when Firebase token refresh fails These tests currently pass by expecting the error to be thrown. After implementing the fix, the tests should be updated to verify graceful handling (returning null instead of throwing). Related to issue #4468: Firebase Auth makes network requests when offline without evicting token 🤖 Generated with Claude Code Co-Authored-By: Claude * [test] update firebaseAuthStore tests They match the behavior of the implemented solution now * [test] add firebaseAuthStore.getTokenId test for non-network errors * [chore] code review feedback * [test] use FirebaseError Co-authored-by: Alexander Brown * [fix] remove indentation and fix test --------- Co-authored-by: snomiao Co-authored-by: Claude Co-authored-by: Alexander Brown --- src/scripts/app.ts | 3 +- src/stores/firebaseAuthStore.ts | 27 +++- .../tests/store/firebaseAuthStore.test.ts | 119 ++++++++++++++---- 3 files changed, 121 insertions(+), 28 deletions(-) diff --git a/src/scripts/app.ts b/src/scripts/app.ts index ca784b66fc..debcc26278 100644 --- a/src/scripts/app.ts +++ b/src/scripts/app.ts @@ -1303,8 +1303,7 @@ export class ComfyApp { const executionStore = useExecutionStore() executionStore.lastNodeErrors = null - let comfyOrgAuthToken = - (await useFirebaseAuthStore().getIdToken()) ?? undefined + let comfyOrgAuthToken = await useFirebaseAuthStore().getIdToken() let comfyOrgApiKey = useApiKeyAuthStore().getApiKey() try { diff --git a/src/stores/firebaseAuthStore.ts b/src/stores/firebaseAuthStore.ts index b8befb0932..65b468001a 100644 --- a/src/stores/firebaseAuthStore.ts +++ b/src/stores/firebaseAuthStore.ts @@ -1,5 +1,7 @@ +import { FirebaseError } from 'firebase/app' import { type Auth, + AuthErrorCodes, GithubAuthProvider, GoogleAuthProvider, type User, @@ -20,6 +22,7 @@ import { useFirebaseAuth } from 'vuefire' import { COMFY_API_BASE_URL } from '@/config/comfyApi' import { t } from '@/i18n' +import { useDialogService } from '@/services/dialogService' import { useApiKeyAuthStore } from '@/stores/apiKeyAuthStore' import { type AuthHeader } from '@/types/authTypes' import { operations } from '@/types/comfyRegistryTypes' @@ -88,11 +91,27 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { lastBalanceUpdateTime.value = null }) - const getIdToken = async (): Promise => { - if (currentUser.value) { - return currentUser.value.getIdToken() + const getIdToken = async (): Promise => { + if (!currentUser.value) return + try { + return await currentUser.value.getIdToken() + } catch (error: unknown) { + if ( + error instanceof FirebaseError && + error.code === AuthErrorCodes.NETWORK_REQUEST_FAILED + ) { + console.warn( + 'Could not authenticate with Firebase. Features requiring authentication might not work.' + ) + return + } + + useDialogService().showErrorDialog(error, { + title: t('errorDialog.defaultTitle'), + reportType: 'authenticationError' + }) + console.error(error) } - return null } /** diff --git a/tests-ui/tests/store/firebaseAuthStore.test.ts b/tests-ui/tests/store/firebaseAuthStore.test.ts index 7d51685b4c..ffe7a8d995 100644 --- a/tests-ui/tests/store/firebaseAuthStore.test.ts +++ b/tests-ui/tests/store/firebaseAuthStore.test.ts @@ -1,8 +1,10 @@ +import { FirebaseError } from 'firebase/app' import * as firebaseAuth from 'firebase/auth' import { createPinia, setActivePinia } from 'pinia' import { beforeEach, describe, expect, it, vi } from 'vitest' import * as vuefire from 'vuefire' +import { useDialogService } from '@/services/dialogService' import { useFirebaseAuthStore } from '@/stores/firebaseAuthStore' // Mock fetch @@ -46,21 +48,24 @@ vi.mock('vue-i18n', () => ({ }) })) -vi.mock('firebase/auth', () => ({ - signInWithEmailAndPassword: vi.fn(), - createUserWithEmailAndPassword: vi.fn(), - signOut: vi.fn(), - onAuthStateChanged: vi.fn(), - signInWithPopup: vi.fn(), - GoogleAuthProvider: class { - setCustomParameters = vi.fn() - }, - GithubAuthProvider: class { - setCustomParameters = vi.fn() - }, - browserLocalPersistence: 'browserLocalPersistence', - setPersistence: vi.fn().mockResolvedValue(undefined) -})) +vi.mock('firebase/auth', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + signInWithEmailAndPassword: vi.fn(), + createUserWithEmailAndPassword: vi.fn(), + signOut: vi.fn(), + onAuthStateChanged: vi.fn(), + signInWithPopup: vi.fn(), + GoogleAuthProvider: class { + setCustomParameters = vi.fn() + }, + GithubAuthProvider: class { + setCustomParameters = vi.fn() + }, + setPersistence: vi.fn().mockResolvedValue(undefined) + } +}) // Mock useToastStore vi.mock('@/stores/toastStore', () => ({ @@ -70,11 +75,7 @@ vi.mock('@/stores/toastStore', () => ({ })) // Mock useDialogService -vi.mock('@/services/dialogService', () => ({ - useDialogService: () => ({ - showSettingsDialog: vi.fn() - }) -})) +vi.mock('@/services/dialogService') describe('useFirebaseAuthStore', () => { let store: ReturnType @@ -93,6 +94,12 @@ describe('useFirebaseAuthStore', () => { beforeEach(() => { vi.resetAllMocks() + // Setup dialog service mock + vi.mocked(useDialogService, { partial: true }).mockReturnValue({ + showSettingsDialog: vi.fn(), + showErrorDialog: vi.fn() + }) + // Mock useFirebaseAuth to return our mock auth object vi.mocked(vuefire.useFirebaseAuth).mockReturnValue(mockAuth as any) @@ -297,7 +304,7 @@ describe('useFirebaseAuthStore', () => { const token = await store.getIdToken() - expect(token).toBeNull() + expect(token).toBeUndefined() }) it('should return null for token after login and logout sequence', async () => { @@ -329,7 +336,75 @@ describe('useFirebaseAuthStore', () => { // Verify token is null after logout const tokenAfterLogout = await store.getIdToken() - expect(tokenAfterLogout).toBeNull() + expect(tokenAfterLogout).toBeUndefined() + }) + + it('should handle network errors gracefully when offline (reproduces issue #4468)', async () => { + // This test reproduces the issue where Firebase Auth makes network requests when offline + // and fails without graceful error handling, causing toast error messages + + // Simulate a user with an expired token that requires network refresh + mockUser.getIdToken.mockReset() + + // Mock network failure (auth/network-request-failed error from Firebase) + const networkError = new FirebaseError( + firebaseAuth.AuthErrorCodes.NETWORK_REQUEST_FAILED, + 'mock error' + ) + + mockUser.getIdToken.mockRejectedValue(networkError) + + const token = await store.getIdToken() + expect(token).toBeUndefined() // Should return undefined instead of throwing + }) + + it('should show error dialog when getIdToken fails with non-network error', async () => { + // This test verifies that non-network errors trigger the error dialog + mockUser.getIdToken.mockReset() + + // Mock a non-network error using actual Firebase Auth error code + const authError = new FirebaseError( + firebaseAuth.AuthErrorCodes.USER_DISABLED, + 'User account is disabled.' + ) + + mockUser.getIdToken.mockRejectedValue(authError) + + // Should call the error dialog instead of throwing + const token = await store.getIdToken() + const dialogService = useDialogService() + + expect(dialogService.showErrorDialog).toHaveBeenCalledWith(authError, { + title: 'errorDialog.defaultTitle', + reportType: 'authenticationError' + }) + expect(token).toBeUndefined() + }) + }) + + describe('getAuthHeader', () => { + it('should handle network errors gracefully when getting Firebase token (reproduces issue #4468)', async () => { + // This test reproduces the issue where getAuthHeader fails due to network errors + // when Firebase Auth tries to refresh tokens offline + + // Mock useApiKeyAuthStore to return null (no API key fallback) + const mockApiKeyStore = { + getAuthHeader: vi.fn().mockReturnValue(null) + } + vi.doMock('@/stores/apiKeyAuthStore', () => ({ + useApiKeyAuthStore: () => mockApiKeyStore + })) + + // Setup user with network error on token refresh + mockUser.getIdToken.mockReset() + const networkError = new FirebaseError( + firebaseAuth.AuthErrorCodes.NETWORK_REQUEST_FAILED, + 'mock error' + ) + mockUser.getIdToken.mockRejectedValue(networkError) + + const authHeader = await store.getAuthHeader() + expect(authHeader).toBeNull() // Should fallback gracefully }) })