feat: implement legacyFetchSignInWithEmail behaviour for Angular and React#1343
feat: implement legacyFetchSignInWithEmail behaviour for Angular and React#1343russellwheatley wants to merge 32 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust mechanism for handling authentication errors where a user attempts to sign in with a different provider than previously used for the same email. It provides a new behavior that fetches and presents alternative sign-in methods, along with corresponding UI components for React and Angular, to guide users through the recovery process. This significantly improves the user experience by preventing dead ends during sign-in and offering clear paths to resolve credential conflicts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new legacyFetchSignInWithEmail behavior to handle OAuth account mismatch flows, allowing users to recover their accounts by suggesting previously used sign-in methods. The changes include core logic for error handling and state management, new UI components for React and Angular to display recovery options, and updated translations. My feedback suggests a minor improvement to the attachEmailToError function to ensure safer cloning of the customData object.
russellwheatley
changed the title
jhuleatt
left a comment
jhuleatt
left a comment
There was a problem hiding this comment.
Overall LGTM, I just want to confirm that all defaults and demos have this legacy behavior disabled (except for the demo that explicitly shows this flow). This won't work by default on any Firebase project created after 2023
|
Hello, this PR has been open for more than 28 days with no activity. If you think this is a mistake, please comment to keep this open. Thanks for contributing!
|
setPendingState cleared legacySignInRecovery synchronously at the start of every sign-in attempt, so clicking a recovery button in the modal dismissed it immediately instead of on success, hiding any error if the attempt failed. Move the clear into handlePendingCredential, which only runs after sign-in actually succeeds.
… and behavior Extract the duplicated list of error codes that trigger the legacyFetchSignInWithEmail recovery flow into a single exported LEGACY_SIGN_IN_RECOVERY_ERROR_CODES constant plus an isLegacySignInRecoveryErrorCode() helper, used by both handleFirebaseError and buildRecovery so the two lists can't drift.
# Conflicts: # packages/core/src/auth.ts # packages/core/src/errors.ts
…in provider.ts FirebaseUIType from core already types legacySignInRecovery and clearLegacySignInRecovery correctly, so the local LegacySignInRecovery/FirebaseUIWithLegacyRecovery types and the `as FirebaseUIWithLegacyRecovery` casts were redundant. Now imports LegacySignInRecovery from @firebase-oss/ui-core and accesses the fields directly, matching the React package's approach.
Cover default and consumer-managed recovery flows in the React and Angular examples, including provider linking without duplicate accounts.
…nable legacyFetchSignInWithEmailHandler previously called setLegacySignInRecovery whenever fetchSignInMethodsForEmail resolved, even when there was nothing useful to recover to: - A password typo on a password-only account returns signInMethods: ["password"] - the SAME method the user just tried, not a different one. - An empty array (e.g. Email Enumeration Protection enabled) was preserved as-is. Both cases opened the recovery modal with misleading "different method" copy and no actionable sign-in buttons. Add hasActionableRecoveryMethod() and clear recovery state instead of setting it when no sign-in method differs from the one just attempted, mirroring the existing "no email"/"fetch failed" branches.
The "pendingCred" sessionStorage key literal was duplicated across legacy-fetch-sign-in-with-email.ts, errors.ts, and auth.ts with no shared source of truth, risking silent drift if one call site were renamed without the others. Export PENDING_CREDENTIAL_STORAGE_KEY from legacy-fetch-sign-in-with-email.ts and reuse it at every read/write site (and in the corresponding tests). Also comment the one remaining literal in the e2e spec, which can't import the constant since page.evaluate runs in the browser context.
…SignInWithEmail fetchSignInMethodsForEmail() only returns real data when Email Enumeration Protection is disabled on the Firebase project (the default for projects created after September 15, 2023 is enabled, making this behavior a no-op). When enumeration protection is off, this behavior also actively surfaces which sign-in methods exist for an email on every failed password attempt, not just OAuth conflicts, which directly opposes the enumeration protection Firebase's generic error codes otherwise provide. Add a security note to the README so integrators can make an informed choice before enabling it.
…overy UI The default legacy sign-in recovery UI rendered an OAuth button for any provider present in signInMethods, without checking it against attemptedProviderId. Add a defensive canOfferMethod() check (React) / canOfferMethod() (Angular) that excludes the just-attempted provider, so the recovery UI can never offer to "sign in with X" as the fix for a failed attempt with X, even under an edge case where Firebase's API includes it in signInMethods.
…redentials persistPendingCredential() stores a serialized OAuth credential (which may include access/ID tokens) in sessionStorage as plaintext. This is an accepted, pre-existing trade-off, not an oversight, so document it inline at both the write site (legacy-fetch-sign-in-with-email.ts) and read/consume site (auth.ts's handlePendingCredential), and call it out in the README next to the related Email Enumeration Protection note.
…th.ts auth.test.ts mocks ./errors at module scope, so it only proves handleFirebaseError was called - it can never prove the caller's promise actually rejects, since the mock never rejects. Every catch block in auth.ts used to call `handleFirebaseError(ui, error);` without `await`/`return`, which turned the rejection into an unhandled promise rejection instead of propagating it, silently resolving the caller with undefined. Add a test file that does NOT mock ./errors, exercising the real implementation end-to-end, so any future regression of `return await handleFirebaseError(...)` is caught by the test suite.
showLegacySignInRecovery defaulted to true on OAuthScreen and SignInAuthScreen in both React and Angular, so registering the legacyFetchSignInWithEmail behavior caused the recovery modal to render on every screen (not just the dedicated demo), addressing reviewer feedback that all defaults should have it disabled.
…idance Explains what the behavior does, links to the relevant Firebase docs on email/password vs. email-link auth and email enumeration protection, and recommends migrating away from it once protection is enabled.
…egacy recovery demo description Applies reviewer-suggested wording noting the demo only works on projects with email enumeration protection disabled.
…n protection caveat Extends the wording already applied to examples/react/src/routes.ts to the remaining route description and on-page copy for the Angular and React legacy recovery demo screens, for consistency.
5 participants
legacyFetchSignInWithEmailcore behavior: on OAuth conflicts (auth/account-exists-with-different-credential) and password-mismatch errors (auth/wrong-password,auth/invalid-credential,auth/invalid-login-credentials), it callsfetchSignInMethodsForEmail()and stores recovery state (email, available methods, attempted/pending provider) on the FirebaseUI instance — but only when there's an actually different method to offer, so a password typo or Email Enumeration Protection (empty methods) never pops up an empty dialog.auth.tsswallowed thrownFirebaseUIErrors — catch blocks calledhandleFirebaseError(ui, error);withoutawait/return, so sign-in/sign-up/phone-verification/MFA errors were never surfaced to callers. All call sites nowreturn await handleFirebaseError(...).sessionStorage(documented plaintext trade-off, pre-existing) so it can be linked once the user signs back in with their original method.LegacySignInRecovery) and Angular (fui-legacy-sign-in-recovery), plususeLegacySignInRecovery()/injectLegacySignInRecovery()for apps that want their own UI instead.showLegacySignInRecoveryonSignInAuthScreen/OAuthScreendefaults tofalse— opt in explicitly.e2e/tests/legacy-sign-in-recovery.spec.ts, react + angular) covering the full emulator-backed recovery-and-link flow and the custom/suppressed recovery UI.closes #1313