diff --git a/docs/dev/deploy.md b/docs/dev/deploy.md index 1792af219..3fa66cc6c 100644 --- a/docs/dev/deploy.md +++ b/docs/dev/deploy.md @@ -66,7 +66,7 @@ gcloud app services set-traffic default --splits==1 ### Canary deploy + Firebase login: `pnpm deploy:canary` -The `--no-promote` flow above lets you `curl` the canary, but you cannot actually **log in** to it: Firebase OAuth (`signInWithRedirect` via `auth.simlin.com`) rejects any origin not in Firebase's *Authorized domains*, and the canary is reachable only at a versioned `https://-dot-...appspot.com` URL that isn't on that list. So a full end-to-end test (including Google sign-in, the new-user flow, and saving a model) isn't possible on a bare `--no-promote` version. +The `--no-promote` flow above lets you `curl` the canary, but you cannot actually **log in** to it: Firebase OAuth (`signInWithPopup` via `auth.simlin.com`) rejects any origin not in Firebase's *Authorized domains*, and the canary is reachable only at a versioned `https://-dot-...appspot.com` URL that isn't on that list. So a full end-to-end test (including Google sign-in, the new-user flow, and saving a model) isn't possible on a bare `--no-promote` version. [`scripts/deploy-canary.mjs`](/scripts/deploy-canary.mjs) (`pnpm deploy:canary`) closes that gap. It: @@ -85,7 +85,7 @@ pnpm deploy:canary --cleanup # de-authorize the host + delete the ve Cleanup is the inverse: it removes the host from `authorizedDomains` and the API key's referrer allowlist (same read-modify-write; it refuses to empty the referrer list, so it can never un-restrict the key) and deletes the version (freeing its slot toward the GAE version cap). It deletes rather than `stop`s because production uses `automatic_scaling`, for which `gcloud app versions stop` is rejected. The delete is guarded by the version's current traffic share: if you have already promoted the canary it is now production, so cleanup refuses to delete it (de-authorizing the host only) and tells you to delete the previous, now-idle version instead -- cleanup can never cause an outage. -**Known limitation: Google/Apple sign-in cannot complete on the canary host.** The redirect flow initiates and returns fine, but `signInWithRedirect`'s result pickup loads an `auth.simlin.com` iframe that must read storage written during the top-level redirect -- and browsers partition cross-**site** iframe storage, so under an `appspot.com` top-level site the iframe sees an empty bucket and the SDK silently reports no result (no error anywhere). No domain/key authorization can fix this; it is inherent to the host. `app.simlin.com` is same-site with the auth domain, so OAuth works there. On the canary, verify login with **email/password** (same identitytoolkit + `/session` path); verify Google/Apple on `app.simlin.com` immediately after promoting, with the rollback split ready. +**OAuth flow: popup, with redirect only as a popup-blocked fallback.** Google/Apple sign-in uses `signInWithPopup`, which hands the result back to the opener via `postMessage` and needs no storage shared with `auth.simlin.com`. The previous `signInWithRedirect` flow silently failed on Safari/iOS even on `app.simlin.com`: its result pickup loads an `auth.simlin.com` iframe that must read storage written during the top-level redirect, and WebKit partitions iframe storage per-**origin** (Chromium partitions per-site, which is why Chrome worked). The same mechanism silently broke OAuth on the cross-site canary host in every browser. With the popup flow, OAuth should complete on the canary too (the host is added to *Authorized domains* by `deploy-canary.mjs`); the redirect fallback still cannot deliver a result there, so if a popup blocker forces the fallback on the canary, expect a silent bounce back to login. Verify Google/Apple on `app.simlin.com` (Safari included) after promoting, with the rollback split ready; email/password exercises the same identitytoolkit + `/session` path if popups are unavailable. This deliberately uses **your own** credentials (`gcloud auth print-access-token`), not the CI deploy service account: mutating Firebase auth config needs `roles/firebaseauth.admin`, an operator-level grant intentionally kept off the CI SA. The deploy/authorize and the traffic promote are separate, explicit steps so traffic is never switched implicitly. Keep the canary host authorized only while you are testing it -- leaving stale appspot hosts in `authorizedDomains` needlessly widens the OAuth surface. diff --git a/src/app/Login.tsx b/src/app/Login.tsx index 01aa5e946..a4e4a2f56 100644 --- a/src/app/Login.tsx +++ b/src/app/Login.tsx @@ -6,9 +6,11 @@ import * as React from 'react'; import clsx from 'clsx'; import { + signInWithPopup, signInWithRedirect, GoogleAuthProvider, OAuthProvider, + AuthProvider, Auth as FirebaseAuth, createUserWithEmailAndPassword, updateProfile, @@ -130,28 +132,48 @@ export function Login(props: LoginProps): React.JSX.Element { ); latest.current = { props, state }; - // Surface OAuth redirect failures (provider misconfig, popup blocked, - // network errors, expired auth domain) into emailError so the user sees them. - const appleLoginClick = async () => { - const provider = appleProvider(); + // OAuth uses the POPUP flow: the result comes back to the opener via + // postMessage, which needs no storage shared with auth.simlin.com. The + // redirect flow's result pickup reads storage the auth.simlin.com helper + // wrote while it was the top-level page, through an auth.simlin.com iframe + // embedded under app.simlin.com -- and WebKit partitions storage per-ORIGIN + // (Chromium partitions per-site), so on Safari/iOS that iframe sees an empty + // partition and the sign-in silently never completes (firebase-js-sdk #7824). + // Redirect remains only as the fallback when a popup blocker rejects the + // popup; a user cancelling the popup is not an error. Real failures + // (provider misconfig, network errors, unauthorized domain) surface into + // emailError so the user sees them. + const oauthLoginClick = async (provider: AuthProvider, genericError: string) => { + // A fresh attempt starts with a clean slate: without this, a cancelled + // popup's early return below would leave a PREVIOUS attempt's failure on + // screen, misattributed to the action the user just harmlessly dismissed. + setState({ emailError: undefined }); try { - await signInWithRedirect(latest.current.props.auth, provider); - } catch (err) { + await signInWithPopup(latest.current.props.auth, provider); + } catch (popupErr) { + let err: unknown = popupErr; + const code = firebaseErrorCode(err); + if (code === 'auth/popup-closed-by-user' || code === 'auth/cancelled-popup-request') { + return; + } + if (code === 'auth/popup-blocked') { + try { + await signInWithRedirect(latest.current.props.auth, provider); + return; + } catch (redirectErr) { + err = redirectErr; + } + } setState({ - emailError: err instanceof Error ? err.message : 'Sign in with Apple failed', + emailError: err instanceof Error ? err.message : genericError, }); } }; - const googleLoginClick = async () => { + const appleLoginClick = () => oauthLoginClick(appleProvider(), 'Sign in with Apple failed'); + const googleLoginClick = () => { const provider = new GoogleAuthProvider(); provider.addScope('profile'); - try { - await signInWithRedirect(latest.current.props.auth, provider); - } catch (err) { - setState({ - emailError: err instanceof Error ? err.message : 'Sign in with Google failed', - }); - } + return oauthLoginClick(provider, 'Sign in with Google failed'); }; // Entering the email flow lands directly on the combined sign-in card. Clear // any stale OAuth error from the landing screen so it doesn't reappear under @@ -510,9 +532,9 @@ export function Login(props: LoginProps): React.JSX.Element { Sign in with email {/* Visible error sink for OAuth click handlers. Without this, a - * rejected signInWithRedirect (popup blocked, provider misconfig, - * network failure) would set emailError but stay invisible because - * no email-flow card is mounted to render it. */} + * rejected signInWithPopup (provider misconfig, unauthorized + * domain, network failure) would set emailError but stay invisible + * because no email-flow card is mounted to render it. */} {state.emailError !== undefined && (

{state.emailError} diff --git a/src/app/tests/login.test.tsx b/src/app/tests/login.test.tsx index fa1bf7e16..7bd10fa04 100644 --- a/src/app/tests/login.test.tsx +++ b/src/app/tests/login.test.tsx @@ -12,6 +12,7 @@ // reaches for the deprecated, enumeration-leaking API again (issue #692). rs.mock('@firebase/auth', () => ({ + signInWithPopup: rs.fn(), signInWithRedirect: rs.fn(), GoogleAuthProvider: class { addScope() {} @@ -103,6 +104,7 @@ import { Login, GoogleIcon } from '../Login'; // rs.mock replaced the module above, so importing it normally yields the mock. // (jest.requireMock had no async equivalent -- rs.importMock returns a promise.) const firebaseAuth = firebaseAuthModule as unknown as { + signInWithPopup: Mock; signInWithRedirect: Mock; fetchSignInMethodsForEmail: Mock; sendPasswordResetEmail: Mock; @@ -125,6 +127,7 @@ function firebaseError(code: string, message = code): Error { } function resetAllMocks(): void { + firebaseAuth.signInWithPopup.mockReset(); firebaseAuth.signInWithRedirect.mockReset(); firebaseAuth.fetchSignInMethodsForEmail.mockReset(); firebaseAuth.sendPasswordResetEmail.mockReset(); @@ -147,37 +150,64 @@ function openSignInCard(email?: string, password?: string): void { describe('Login OAuth click handlers (landing)', () => { beforeEach(resetAllMocks); - test('Google sign-in surfaces an error to UI when signInWithRedirect rejects', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce(new Error('popup blocked')); + // signInWithPopup, not signInWithRedirect: the redirect flow's result pickup + // reads storage the auth.simlin.com helper wrote while it was the top-level + // page, through an iframe of that origin embedded under app.simlin.com. + // WebKit partitions storage per-ORIGIN (not per-site like Chromium), so on + // Safari/iOS that iframe sees an empty partition and the sign-in silently + // never completes. The popup flow hands the result back via postMessage and + // needs no shared storage, so it works everywhere. + test('Google sign-in uses the popup flow, not the redirect flow', async () => { + firebaseAuth.signInWithPopup.mockResolvedValueOnce({ user: {} }); render(); fireEvent.click(screen.getByText('Sign in with Google')); await waitFor(() => { - expect(firebaseAuth.signInWithRedirect).toHaveBeenCalledTimes(1); - expect(screen.queryByText(/popup blocked/i)).not.toBeNull(); + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); }); + expect(firebaseAuth.signInWithRedirect).not.toHaveBeenCalled(); }); - test('Apple sign-in surfaces an error to UI when signInWithRedirect rejects', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce(new Error('apple unavailable')); + test('Apple sign-in uses the popup flow, not the redirect flow', async () => { + firebaseAuth.signInWithPopup.mockResolvedValueOnce({ user: {} }); render(); fireEvent.click(screen.getByText('Sign in with Apple')); await waitFor(() => { - expect(firebaseAuth.signInWithRedirect).toHaveBeenCalledTimes(1); + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); + }); + expect(firebaseAuth.signInWithRedirect).not.toHaveBeenCalled(); + }); + + test('Google sign-in surfaces an error to UI when signInWithPopup rejects', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(new Error('provider misconfigured')); + + render(); + fireEvent.click(screen.getByText('Sign in with Google')); + + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); + expect(screen.queryByText(/provider misconfigured/i)).not.toBeNull(); + }); + }); + + test('Apple sign-in surfaces an error to UI when signInWithPopup rejects', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(new Error('apple unavailable')); + + render(); + fireEvent.click(screen.getByText('Sign in with Apple')); + + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); expect(screen.queryByText(/apple unavailable/i)).not.toBeNull(); }); }); - test('Google sign-in awaits the redirect promise (no fire-and-forget setTimeout)', async () => { - let resolveSignIn: () => void = () => {}; - firebaseAuth.signInWithRedirect.mockReturnValueOnce( - new Promise((resolve) => { - resolveSignIn = resolve; - }), - ); + test('a blocked popup falls back to the redirect flow without showing an error', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError('auth/popup-blocked')); + firebaseAuth.signInWithRedirect.mockResolvedValueOnce(undefined); render(); fireEvent.click(screen.getByText('Sign in with Google')); @@ -185,8 +215,57 @@ describe('Login OAuth click handlers (landing)', () => { await waitFor(() => { expect(firebaseAuth.signInWithRedirect).toHaveBeenCalledTimes(1); }); + expect(screen.queryByRole('alert')).toBeNull(); + }); + + test('a rejected redirect fallback still surfaces its error', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError('auth/popup-blocked')); + firebaseAuth.signInWithRedirect.mockRejectedValueOnce(new Error('redirect also failed')); + + render(); + fireEvent.click(screen.getByText('Sign in with Google')); + + await waitFor(() => { + expect(screen.queryByText(/redirect also failed/i)).not.toBeNull(); + }); + }); + + // A new attempt must clear the previous attempt's error up front: without + // that, a failed Google attempt followed by a CANCELLED attempt (even an + // Apple one) leaves the old failure on screen, misattributing it to the + // action the user just harmlessly dismissed. + test('starting a new OAuth attempt clears the previous attempt error, even when cancelled', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(new Error('provider misconfigured')); + + render(); + fireEvent.click(screen.getByText('Sign in with Google')); + await waitFor(() => { + expect(screen.queryByText(/provider misconfigured/i)).not.toBeNull(); + }); - resolveSignIn(); + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError('auth/popup-closed-by-user')); + fireEvent.click(screen.getByText('Sign in with Apple')); + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(2); + }); + expect(screen.queryByText(/provider misconfigured/i)).toBeNull(); + }); + + test('closing the popup is treated as a cancel, not an error', async () => { + for (const code of ['auth/popup-closed-by-user', 'auth/cancelled-popup-request']) { + firebaseAuth.signInWithPopup.mockReset(); + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError(code)); + + const { unmount } = render(); + fireEvent.click(screen.getByText('Sign in with Google')); + + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); + }); + expect(screen.queryByRole('alert')).toBeNull(); + expect(firebaseAuth.signInWithRedirect).not.toHaveBeenCalled(); + unmount(); + } }); }); @@ -597,7 +676,7 @@ describe('Login miscellaneous error paths', () => { }); test('a non-Error OAuth rejection falls back to a generic provider message', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce('nope'); + firebaseAuth.signInWithPopup.mockRejectedValueOnce('nope'); render(); fireEvent.click(screen.getByText('Sign in with Google')); @@ -608,7 +687,7 @@ describe('Login miscellaneous error paths', () => { }); test('a non-Error Apple OAuth rejection falls back to a generic provider message', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce('nope'); + firebaseAuth.signInWithPopup.mockRejectedValueOnce('nope'); render(); fireEvent.click(screen.getByText('Sign in with Apple'));