Uh oh!
There was an error while loading. Please reload this page.
fix(auth): don't strand phone-only users on email verification after reload - #2474
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces stable test tags (FirebaseAuthTestTags) and exposes them as Android resource IDs to support automated testing with Firebase Test Lab and Robo. It also improves accessibility by adding positional content descriptions to verification code digit boxes and declaring text-input semantics on the digit group to allow crawlers to input verification codes in a single action. Additionally, recovery codes have been completely removed from the Multi-Factor Authentication (MFA) flow across all configurations, UI screens, and localized string resources. Other changes include diagnostic logging for Google Sign-In failures, improved modifier propagation, and comprehensive unit tests validating these updates. I have no feedback to provide as there are no review comments.
a079274 to
3abf81aCompare
russellwheatley
left a comment
There was a problem hiding this comment.
LGTM. The split derivation was the bug, and putting reload behind handleAuthUserState is the right fix.
Two nits, neither blocking:
The unverified-password test does not pin republish the way the other two do. Phone-only and federated call pinToEmailVerification() first, so they fail unless reloadUser() overwrites the stuck internal state. The password test starts from Idle, and authStateFlow() would already emit RequiresEmailVerification from the Firebase listener even if reloadUser() never called updateAuthState. Pinning with email = "" first, then asserting it becomes "test@example.com", would make that test do the same job.
The sign-out-mid-reload test stubs getIdToken(true) to succeed, so the uid check is what saves it. In production, signing out during reload() is more likely to make getIdToken throw, which the screen logs as "Failed to refresh user" before that check runs. I don't think that re-strands anyone: the AuthStateListener already clears stale RequiresEmailVerification / Success on sign-out. An extra uid check between the two awaits would just avoid the misleading log.
3abf81a to
2f12e3dCompareUh oh!
There was an error while loading. Please reload this page.
onReloadUseronAuthSuccessUiContextre-derived "does this user need email verification?" fromisEmailVerifiedalone, dropping theemail != nullandpassword-provider checksFirebaseAuthUIalready applied. A phone-only user is unverified and has no email, so anyauthenticatedContentslot callinguiContext.onReloadUser()publishedRequiresEmailVerification(email = "")— and becauseauthStateFlow()prefers any non-Idle internal state, it stuck there with signing out the only exit. Latent in the stock UI, which wires the callback only to its own verify-email screen.Deriving auth state isn't the screen's job, so the reload moves to
FirebaseAuthUI.reloadUser()and reuses the existinghandleAuthUserStaterule rather than duplicating it. It also revalidates the current user after its two suspension points: signing out mid-reload previously publishedSuccessfor the departed user, made permanent by that same precedence. The screen keeps only the error logging, and now rethrowsCancellationExceptioninstead of logging it as a refresh failure.Tests go in
FirebaseAuthUIAuthStateTest— phone-only, federated, unverified password, and sign-out mid-reload. Verified by mutation rather than assumed: reinstating the old derivation reds the first two, and dropping the revalidation reds the last.Maintainer note: Fixes internal CPRN-407