From e035a60730c5f198bc950edfd0b33a5bc93327e5 Mon Sep 17 00:00:00 2001 From: demolaf Date: Tue, 28 Jul 2026 16:17:47 +0100 Subject: [PATCH 1/3] fix(auth): stale one-off AuthState no longer leaks across screen instances --- .../ui/components/TopLevelDialogController.kt | 32 ++-- .../ui/auth/ui/screens/FirebaseAuthScreen.kt | 9 +- .../auth/ui/screens/email/EmailAuthScreen.kt | 32 +++- .../auth/ui/screens/phone/PhoneAuthScreen.kt | 8 + .../ui/auth/FirebaseAuthUIAuthStateTest.kt | 47 ++++++ .../TopLevelDialogControllerTest.kt | 153 ++++++++++++++++++ .../ui/screens/AnonymousAuthScreenTest.kt | 34 ++-- .../ui/auth/ui/screens/EmailAuthScreenTest.kt | 40 +++-- 8 files changed, 316 insertions(+), 39 deletions(-) create mode 100644 auth/src/test/java/com/firebase/ui/auth/ui/components/TopLevelDialogControllerTest.kt diff --git a/auth/src/main/java/com/firebase/ui/auth/ui/components/TopLevelDialogController.kt b/auth/src/main/java/com/firebase/ui/auth/ui/components/TopLevelDialogController.kt index 4cd0aadd8..e5d22c1a8 100644 --- a/auth/src/main/java/com/firebase/ui/auth/ui/components/TopLevelDialogController.kt +++ b/auth/src/main/java/com/firebase/ui/auth/ui/components/TopLevelDialogController.kt @@ -40,18 +40,18 @@ val LocalTopLevelDialogController = compositionLocalOf * dialogController?.showErrorDialog( @@ -68,7 +68,7 @@ val LocalTopLevelDialogController = compositionLocalOf AuthState ) { private var dialogState by mutableStateOf(null) private val shownErrorStates = mutableSetOf() @@ -78,18 +78,22 @@ class TopLevelDialogController( * Automatically prevents duplicate dialogs for the same AuthState.Error instance. * * @param exception The auth exception to display + * @param errorState The specific [AuthState.Error] instance this call is reacting to, used + * for de-duplication. Pass this explicitly when the caller might not be the only observer of + * the same error: by the time this runs, another observer may have already reset the live + * auth state to `Idle`, so falling back to [currentAuthState] alone would miss the dedup. * @param onRetry Callback when user clicks retry button * @param onRecover Callback when user clicks recover button (e.g., navigate to different screen) * @param onDismiss Callback when dialog is dismissed */ fun showErrorDialog( exception: AuthException, + errorState: AuthState.Error? = null, onRetry: (AuthException) -> Unit = {}, onRecover: ((AuthException) -> Unit)? = null, onDismiss: () -> Unit = {} ) { - // Get current error state - val currentErrorState = authState as? AuthState.Error + val currentErrorState = errorState ?: (currentAuthState() as? AuthState.Error) // If this exact error state has already been shown, skip if (currentErrorState != null && currentErrorState in shownErrorStates) { @@ -162,13 +166,21 @@ class TopLevelDialogController( /** * Creates and remembers a [TopLevelDialogController]. + * + * [authState] is a lambda rather than a snapshot value so the controller can read the + * live auth state on every [TopLevelDialogController.showErrorDialog] call without being + * recreated (and losing its de-duplication history) whenever the auth state changes. + * + * Keyed on [stringProvider] rather than left unkeyed: callers must pass a `remember`ed + * [stringProvider] (stable across recompositions), otherwise the controller — and its + * de-duplication history — would be recreated on every recomposition. */ @Composable fun rememberTopLevelDialogController( stringProvider: AuthUIStringProvider, - authState: AuthState + authState: () -> AuthState ): TopLevelDialogController { - return remember(stringProvider, authState) { + return remember(stringProvider) { TopLevelDialogController(stringProvider, authState) } } diff --git a/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt b/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt index 2d40e98e7..9d43d2b67 100644 --- a/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt +++ b/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt @@ -144,11 +144,11 @@ fun FirebaseAuthScreen( val activity = LocalActivity.current val context = LocalContext.current val coroutineScope = rememberCoroutineScope() - val stringProvider = DefaultAuthUIStringProvider(context) + val stringProvider = remember(context) { DefaultAuthUIStringProvider(context) } val navController = rememberNavController() val authState by remember(authUI) { authUI.authStateFlow() }.collectAsState(AuthState.Idle) - val dialogController = rememberTopLevelDialogController(stringProvider, authState) + val dialogController = rememberTopLevelDialogController(stringProvider) { authState } val lastSuccessfulUserId = remember { mutableStateOf(null) } val pendingLinkingCredential = remember { mutableStateOf(null) } val pendingResolver = remember { mutableStateOf(null) } @@ -556,6 +556,8 @@ fun FirebaseAuthScreen( // Keep external cancellation reporting centralized here so child screens // can handle local navigation without triggering duplicate callbacks. onSignInCancelled() + // Consumed so this doesn't leak to a freshly created screen. + authUI.updateAuthState(AuthState.Idle) } is AuthState.Idle -> { @@ -588,6 +590,7 @@ fun FirebaseAuthScreen( dialogController.showErrorDialog( exception = exception, + errorState = errorState, onRetry = { _ -> // Child screens handle their own retry logic }, @@ -646,6 +649,8 @@ fun FirebaseAuthScreen( // Dialog dismissed } ) + // Consumed immediately so this doesn't leak to a freshly created screen. + authUI.updateAuthState(AuthState.Idle) } } diff --git a/auth/src/main/java/com/firebase/ui/auth/ui/screens/email/EmailAuthScreen.kt b/auth/src/main/java/com/firebase/ui/auth/ui/screens/email/EmailAuthScreen.kt index 667fc364b..c5dae7822 100644 --- a/auth/src/main/java/com/firebase/ui/auth/ui/screens/email/EmailAuthScreen.kt +++ b/auth/src/main/java/com/firebase/ui/auth/ui/screens/email/EmailAuthScreen.kt @@ -24,6 +24,7 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.saveable.rememberSaveable +import androidx.compose.runtime.setValue import com.firebase.ui.auth.AuthException import com.firebase.ui.auth.AuthState import com.firebase.ui.auth.FirebaseAuthUI @@ -167,8 +168,11 @@ fun EmailAuthScreen( val authCredentialForLinking = remember { credentialForLinking } val errorMessage = if (authState is AuthState.Error) (authState as AuthState.Error).exception.message else null - val resetLinkSent = authState is AuthState.PasswordResetLinkSent - val emailSignInLinkSent = authState is AuthState.EmailSignInLinkSent + + // Latched locally since these get consumed (reset to Idle) below — deriving directly from + // authState would close ResetPasswordUI/SignInEmailLinkUI's dialogs as soon as it resets. + var resetLinkSentLocal by rememberSaveable { mutableStateOf(false) } + var emailSignInLinkSentLocal by rememberSaveable { mutableStateOf(false) } // Track if credentials were retrieved from Credential Manager val retrievedCredential = remember { mutableStateOf?>(null) } @@ -187,6 +191,7 @@ fun EmailAuthScreen( onError(exception) dialogController?.showErrorDialog( exception = exception, + errorState = state, onRetry = { ex -> when (ex) { is AuthException.UserNotFoundException -> { @@ -229,10 +234,24 @@ fun EmailAuthScreen( // Dialog dismissed } ) + // Consumed immediately so this doesn't leak to a freshly created screen. + authUI.updateAuthState(AuthState.Idle) } is AuthState.Cancelled -> { onCancel() + // Consumed so this doesn't leak to a freshly created screen. + authUI.updateAuthState(AuthState.Idle) + } + + is AuthState.PasswordResetLinkSent -> { + resetLinkSentLocal = true + authUI.updateAuthState(AuthState.Idle) + } + + is AuthState.EmailSignInLinkSent -> { + emailSignInLinkSentLocal = true + authUI.updateAuthState(AuthState.Idle) } else -> Unit @@ -247,8 +266,8 @@ fun EmailAuthScreen( confirmPassword = confirmPasswordTextValue.value, isLoading = isLoading, error = errorMessage, - resetLinkSent = resetLinkSent, - emailSignInLinkSent = emailSignInLinkSent, + resetLinkSent = resetLinkSentLocal, + emailSignInLinkSent = emailSignInLinkSentLocal, onEmailChange = { email -> emailTextValue.value = email }, @@ -286,6 +305,7 @@ fun EmailAuthScreen( } }, onSignInEmailLinkClick = { + emailSignInLinkSentLocal = false coroutineScope.launch { try { if (emailLinkFromDifferentDevice != null) { @@ -327,6 +347,7 @@ fun EmailAuthScreen( } }, onSendResetLinkClick = { + resetLinkSentLocal = false coroutineScope.launch { try { authUI.sendPasswordResetEmail( @@ -346,14 +367,17 @@ fun EmailAuthScreen( onGoToSignIn = { textValues.forEach { it.value = "" } mode.value = EmailAuthMode.SignIn + emailSignInLinkSentLocal = false }, onGoToResetPassword = { textValues.forEach { it.value = "" } mode.value = EmailAuthMode.ResetPassword + resetLinkSentLocal = false }, onGoToEmailLinkSignIn = { textValues.forEach { it.value = "" } mode.value = EmailAuthMode.EmailLinkSignIn + emailSignInLinkSentLocal = false }, ) diff --git a/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt b/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt index e317c639d..fb3411c81 100644 --- a/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt +++ b/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt @@ -196,6 +196,9 @@ fun PhoneAuthScreen( pendingVerificationPhoneNumber.value = null verificationStartTime.value = null + // Consumed before the async sign-in call so it can't be clobbered by that call's own state. + authUI.updateAuthState(AuthState.Idle) + coroutineScope.launch { try { authUI.signInWithPhoneAuthCredential( @@ -216,6 +219,7 @@ fun PhoneAuthScreen( // Show dialog for phone-specific errors using top-level controller dialogController?.showErrorDialog( exception = exception, + errorState = state, onRetry = { ex -> when (ex) { is AuthException.InvalidCredentialsException -> { @@ -228,10 +232,14 @@ fun PhoneAuthScreen( // Dialog dismissed } ) + // Consumed immediately so this doesn't leak to a freshly created screen. + authUI.updateAuthState(AuthState.Idle) } is AuthState.Cancelled -> { onCancel() + // Consumed so this doesn't leak to a freshly created screen. + authUI.updateAuthState(AuthState.Idle) } else -> Unit diff --git a/auth/src/test/java/com/firebase/ui/auth/FirebaseAuthUIAuthStateTest.kt b/auth/src/test/java/com/firebase/ui/auth/FirebaseAuthUIAuthStateTest.kt index 8a4715c97..78e7f0dd3 100644 --- a/auth/src/test/java/com/firebase/ui/auth/FirebaseAuthUIAuthStateTest.kt +++ b/auth/src/test/java/com/firebase/ui/auth/FirebaseAuthUIAuthStateTest.kt @@ -318,6 +318,53 @@ class FirebaseAuthUIAuthStateTest { assertThat(states[2]).isEqualTo(AuthState.Cancelled) // After second update } + // ============================================================================================= + // Stale one-off AuthState regression tests + // ============================================================================================= + + @Test + fun `Error does not leak to a fresh collector after being consumed`() = runBlocking { + `when`(mockFirebaseAuth.currentUser).thenReturn(null) + + authUI.updateAuthState(AuthState.Error(Exception("boom"))) + authUI.updateAuthState(AuthState.Idle) + + // A brand-new collector (simulating a freshly created Activity) must see Idle. + assertThat(authUI.authStateFlow().first()).isEqualTo(AuthState.Idle) + } + + @Test + fun `Cancelled does not leak to a fresh collector after being consumed`() = runBlocking { + `when`(mockFirebaseAuth.currentUser).thenReturn(null) + + authUI.updateAuthState(AuthState.Cancelled) + authUI.updateAuthState(AuthState.Idle) + + assertThat(authUI.authStateFlow().first()).isEqualTo(AuthState.Idle) + } + + @Test + fun `SMSAutoVerified does not leak to a fresh collector after being consumed`() = runBlocking { + `when`(mockFirebaseAuth.currentUser).thenReturn(null) + val credential = mock(com.google.firebase.auth.PhoneAuthCredential::class.java) + + authUI.updateAuthState(AuthState.SMSAutoVerified(credential)) + authUI.updateAuthState(AuthState.Idle) + + assertThat(authUI.authStateFlow().first()).isEqualTo(AuthState.Idle) + } + + @Test + fun `Error left uncleared still leaks to a fresh collector (pins down the bug being fixed)`() = + runBlocking { + `when`(mockFirebaseAuth.currentUser).thenReturn(null) + + // No consuming reset here — documents the pre-fix leaking behavior. + authUI.updateAuthState(AuthState.Error(Exception("boom"))) + + assertThat(authUI.authStateFlow().first()).isInstanceOf(AuthState.Error::class.java) + } + // ============================================================================================= // AuthState Class Tests // ============================================================================================= diff --git a/auth/src/test/java/com/firebase/ui/auth/ui/components/TopLevelDialogControllerTest.kt b/auth/src/test/java/com/firebase/ui/auth/ui/components/TopLevelDialogControllerTest.kt new file mode 100644 index 000000000..60fa0ba40 --- /dev/null +++ b/auth/src/test/java/com/firebase/ui/auth/ui/components/TopLevelDialogControllerTest.kt @@ -0,0 +1,153 @@ +package com.firebase.ui.auth.ui.components + +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.test.core.app.ApplicationProvider +import com.firebase.ui.auth.AuthException +import com.firebase.ui.auth.AuthState +import com.firebase.ui.auth.configuration.string_provider.DefaultAuthUIStringProvider +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Unit tests for [TopLevelDialogController] and [rememberTopLevelDialogController]. + * + * These cover the fix for a bug where keying `remember` on the live `authState` value recreated + * the controller (and wiped its `shownErrorStates` de-duplication set) on every state change — + * which, combined with screens resetting `AuthState` back to `Idle` immediately after consuming + * an `Error`, would tear down and discard the just-shown dialog on the very next recomposition. + * + * @suppress Internal test class + */ +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, sdk = [34]) +class TopLevelDialogControllerTest { + + @get:Rule + val composeTestRule = createComposeRule() + + private lateinit var stringProvider: DefaultAuthUIStringProvider + + @Test + fun `controller survives an authState change instead of being recreated`() { + stringProvider = DefaultAuthUIStringProvider(ApplicationProvider.getApplicationContext()) + var state: AuthState = AuthState.Idle + lateinit var controller: TopLevelDialogController + + composeTestRule.setContent { + controller = rememberTopLevelDialogController(stringProvider) { state } + controller.CurrentDialog() + } + + val error = AuthState.Error(Exception("boom")) + composeTestRule.runOnIdle { + state = error + controller.showErrorDialog( + exception = AuthException.from(error.exception, stringProvider) + ) + } + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(stringProvider.errorDialogTitle).assertExists() + + // Mirrors the fixed screens resetting authState right after showing the dialog. + composeTestRule.runOnIdle { + state = AuthState.Idle + } + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithText(stringProvider.errorDialogTitle).assertExists() + } + + @Test + fun `showErrorDialog does not re-show the same error state twice`() { + stringProvider = DefaultAuthUIStringProvider(ApplicationProvider.getApplicationContext()) + var state: AuthState = AuthState.Idle + lateinit var controller: TopLevelDialogController + + composeTestRule.setContent { + controller = rememberTopLevelDialogController(stringProvider) { state } + controller.CurrentDialog() + } + + val error = AuthState.Error(Exception("boom")) + val exception = AuthException.from(error.exception, stringProvider) + + composeTestRule.runOnIdle { + state = error + controller.showErrorDialog(exception = exception) + } + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(stringProvider.errorDialogTitle).assertExists() + + composeTestRule.runOnIdle { + controller.dismissDialog() + } + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(stringProvider.errorDialogTitle).assertDoesNotExist() + + // Same Error instance again — must be a no-op, the de-dup set persists across calls. + composeTestRule.runOnIdle { + controller.showErrorDialog(exception = exception) + } + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(stringProvider.errorDialogTitle).assertDoesNotExist() + } + + @Test + fun `second observer of the same error does not overwrite the first observer's dialog`() { + stringProvider = DefaultAuthUIStringProvider(ApplicationProvider.getApplicationContext()) + var state: AuthState = AuthState.Idle + lateinit var controller: TopLevelDialogController + + composeTestRule.setContent { + controller = rememberTopLevelDialogController(stringProvider) { state } + controller.CurrentDialog() + } + + val error = AuthState.Error(Exception("boom")) + val exception = AuthException.from(error.exception, stringProvider) + + var firstOnRetryCalled = false + var secondOnRetryCalled = false + + // Observer #1 (e.g. FirebaseAuthScreen's top-level effect): sees the Error, shows the + // dialog passing the specific errorState, then immediately resets the live state to + // Idle -- mirroring the real screens' consume-then-reset pattern. + composeTestRule.runOnIdle { + state = error + controller.showErrorDialog( + exception = exception, + errorState = error, + onRetry = { firstOnRetryCalled = true } + ) + state = AuthState.Idle + } + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(stringProvider.errorDialogTitle).assertExists() + + // Observer #2 (e.g. EmailAuthScreen's own effect on the same authState emission): its + // LaunchedEffect(authState) still holds the same `error` instance locally, so it passes + // it as errorState even though the controller's live currentAuthState() now reads Idle. + composeTestRule.runOnIdle { + controller.showErrorDialog( + exception = exception, + errorState = error, + onRetry = { secondOnRetryCalled = true } + ) + } + composeTestRule.waitForIdle() + + // Dedup must key off the passed-in errorState, not the (possibly already-reset) live + // state, so observer #2's call is a no-op and observer #1's dialog/callback survives. + composeTestRule.onNodeWithText(stringProvider.retryAction).performClick() + assert(firstOnRetryCalled && !secondOnRetryCalled) { + "Expected observer #1's dialog/callback to survive untouched, but observer #2's " + + "call overwrote it (firstOnRetryCalled=$firstOnRetryCalled, " + + "secondOnRetryCalled=$secondOnRetryCalled)" + } + } +} diff --git a/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/AnonymousAuthScreenTest.kt b/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/AnonymousAuthScreenTest.kt index 743a26db4..6ef77c0b5 100644 --- a/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/AnonymousAuthScreenTest.kt +++ b/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/AnonymousAuthScreenTest.kt @@ -310,9 +310,13 @@ class AnonymousAuthScreenTest { } var currentAuthState: AuthState = AuthState.Idle + var capturedFailure: AuthException? = null composeTestRule.setContent { - TestAuthScreen(configuration = configuration) + TestAuthScreen( + configuration = configuration, + onSignInFailure = { capturedFailure = it }, + ) val authState by authUI.authStateFlow().collectAsState(AuthState.Idle) currentAuthState = authState } @@ -382,12 +386,18 @@ class AnonymousAuthScreenTest { composeTestRule.waitForIdle() shadowOf(Looper.getMainLooper()).idle() - // Step 5: Wait for error state (AccountLinkingRequiredException) + // Step 5: Wait for onSignInFailure to fire with AccountLinkingRequiredException. + // + // This is captured via the onSignInFailure callback rather than polling authStateFlow(): + // the screen resets AuthState back to Idle immediately after consuming the Error (so a + // second, independent authStateFlow() collector — like polling currentAuthState here — + // can miss the transient value entirely per StateFlow's conflation contract), whereas + // onSignInFailure is a direct, synchronous call from the same effect, so it can't race. println("TEST: Waiting for AccountLinkingRequiredException...") composeTestRule.waitUntil(timeoutMillis = AUTH_STATE_WAIT_TIMEOUT_MS) { shadowOf(Looper.getMainLooper()).idle() - println("TEST: Auth state: $currentAuthState") - currentAuthState is AuthState.Error + println("TEST: Captured failure: $capturedFailure") + capturedFailure != null } // Step 6: Verify ErrorRecoveryDialog is displayed @@ -396,24 +406,22 @@ class AnonymousAuthScreenTest { .assertIsDisplayed() // Verify exception - assertThat(currentAuthState).isInstanceOf(AuthState.Error::class.java) - val errorState = currentAuthState as AuthState.Error - assertThat(errorState.exception).isInstanceOf(AuthException.AccountLinkingRequiredException::class.java) + assertThat(capturedFailure).isInstanceOf(AuthException.AccountLinkingRequiredException::class.java) - val linkingException = errorState.exception as AuthException.AccountLinkingRequiredException + val linkingException = capturedFailure as AuthException.AccountLinkingRequiredException assertThat(linkingException.email).isEqualTo(email) } @Composable - private fun TestAuthScreen(configuration: AuthUIConfiguration) { - composeTestRule.waitForIdle() - shadowOf(Looper.getMainLooper()).idle() - + private fun TestAuthScreen( + configuration: AuthUIConfiguration, + onSignInFailure: (AuthException) -> Unit = {}, + ) { FirebaseAuthScreen( configuration = configuration, authUI = authUI, onSignInSuccess = { result -> }, - onSignInFailure = { exception: AuthException -> }, + onSignInFailure = onSignInFailure, onSignInCancelled = {}, authenticatedContent = { state, uiContext -> when (state) { diff --git a/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt b/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt index d438eb45b..7ca09692c 100644 --- a/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt +++ b/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt @@ -20,6 +20,7 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.assertIsNotDisplayed import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollTo @@ -28,6 +29,7 @@ import androidx.credentials.CreatePasswordRequest import androidx.credentials.CredentialManager import androidx.credentials.GetCredentialRequest import androidx.credentials.GetCredentialResponse +import androidx.credentials.exceptions.NoCredentialException import androidx.test.core.app.ActivityScenario import androidx.test.core.app.ApplicationProvider import com.firebase.ui.auth.AuthState @@ -138,6 +140,11 @@ class EmailAuthScreenTest { fun tearDown() { closeable.close() + // Sign out first: the FirebaseAuth SDK instance backing authUI isn't recreated until the + // next test's setUp() deletes/reinitializes the FirebaseApp, so a session left signed in + // here would otherwise still be live when the next test's composition starts. + authUI.auth.signOut() + // Clean up after each test to prevent test pollution FirebaseAuthUI.clearInstanceCache() @@ -585,22 +592,24 @@ class EmailAuthScreenTest { shadowOf(Looper.getMainLooper()).idle() composeAndroidTestRule.waitForIdle() - // Wait for auth state to transition to EmailSignInLinkSent - println("TEST: Waiting for auth state change... Current state: $currentAuthState") + // Wait for the "email link sent" dialog to appear, rather than polling currentAuthState: + // the screen resets AuthState back to Idle immediately after consuming + // EmailSignInLinkSent (so a second, independent authStateFlow() collector — like + // currentAuthState here — can miss the transient value entirely per StateFlow's + // conflation contract), whereas the dialog's visibility is latched in local Compose + // state that isn't reset the same way, so it's a reliable, non-racy signal. + println("TEST: Waiting for email link sent dialog...") composeAndroidTestRule.waitUntil(timeoutMillis = AUTH_STATE_WAIT_TIMEOUT_MS) { shadowOf(Looper.getMainLooper()).idle() - println("TEST: Auth state during wait: $currentAuthState") - currentAuthState is AuthState.EmailSignInLinkSent + composeAndroidTestRule.onAllNodesWithText(stringProvider.emailSignInLinkSentDialogTitle) + .fetchSemanticsNodes().isNotEmpty() } // Ensure final recomposition is complete before assertions shadowOf(Looper.getMainLooper()).idle() composeAndroidTestRule.waitForIdle() - // Verify the auth state and user properties - println("TEST: Verifying auth state: $currentAuthState") - assertThat(currentAuthState) - .isInstanceOf(AuthState.EmailSignInLinkSent::class.java) + // Verify the dialog and user properties assertThat(authUI.auth.currentUser).isNull() composeAndroidTestRule.onNodeWithText(stringProvider.emailSignInLinkSentDialogTitle) .assertIsDisplayed() @@ -842,9 +851,15 @@ class EmailAuthScreenTest { whenever(mockCredentialManager.createCredential(any(), any())) .thenReturn(mock()) - // Mock successful credential retrieval + // No credential exists yet for this account (sign-up hasn't happened), so the very first + // mount's auto-retrieval attempt must find nothing rather than "succeed" with a credential + // for an account that doesn't exist — otherwise it triggers a real sign-in failure (and, + // now that dialogs correctly persist, a real error dialog) before step 1 even runs. + // thenAnswer (not thenThrow) since getCredential's suspend-compiled signature doesn't + // declare GetCredentialException, which Mockito otherwise rejects as an invalid checked + // exception for the method. whenever(mockCredentialManager.getCredential(any(), any())) - .thenReturn(mockCredentialResponse) + .thenAnswer { throw NoCredentialException() } val configuration = authUIConfiguration { context = applicationContext @@ -906,6 +921,11 @@ class EmailAuthScreenTest { verify(mockCredentialManager, times(1)).createCredential(any(), any()) println("TEST: Sign-up complete, credentials saved (createCredential called once)") + // Now that the account actually exists, retrieval can start "succeeding" — matching the + // real scenario this test verifies (auto-sign-in via a previously-saved credential). + whenever(mockCredentialManager.getCredential(any(), any())) + .thenReturn(mockCredentialResponse) + // STEP 2: Sign out println("TEST: Signing out...") authUI.auth.signOut() From 879ef9ae8805578c90bc3640021c02fc5323c36e Mon Sep 17 00:00:00 2001 From: demolaf Date: Wed, 29 Jul 2026 12:38:55 +0100 Subject: [PATCH 2/3] fix(auth): don't bounce back to method picker when consuming a one-off notification --- .../ui/auth/ui/screens/FirebaseAuthScreen.kt | 14 +- .../FirebaseAuthScreenErrorNavigationTest.kt | 152 ++++++++++++++++++ .../ui/auth/ui/screens/EmailAuthScreenTest.kt | 13 +- 3 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 auth/src/test/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreenErrorNavigationTest.kt diff --git a/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt b/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt index 9d43d2b67..733b7f158 100644 --- a/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt +++ b/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt @@ -158,6 +158,9 @@ fun FirebaseAuthScreen( val emailLinkFromDifferentDevice = remember { mutableStateOf(null) } val lastSignInPreference = remember { mutableStateOf(null) } + // Last-processed AuthState, so the Idle branch below can tell a genuine reset apart from + // Idle-as-a-side-effect of consuming a one-off notification (see wasConsumedNotification). + val previousAuthState = remember { mutableStateOf(AuthState.Idle) } val startRoute = remember(configuration.providers, configuration.isProviderChoiceAlwaysShown) { getStartRoute(configuration) } @@ -447,6 +450,8 @@ fun FirebaseAuthScreen( // Synchronise auth state changes with navigation stack. LaunchedEffect(authState) { val state = authState + val previous = previousAuthState.value + previousAuthState.value = state val currentRoute = navController.currentBackStackEntry?.destination?.route when (state) { is AuthState.Success -> { @@ -567,7 +572,14 @@ fun FirebaseAuthScreen( pendingResolver.value = null pendingLinkingCredential.value = null lastSuccessfulUserId.value = null - if (currentRoute != startRoute.route) { + // A one-off notification resets to Idle purely to avoid leaking to a + // freshly created screen — that's not a request to leave the current one. + val wasConsumedNotification = previous is AuthState.Error || + previous is AuthState.Cancelled || + previous is AuthState.SMSAutoVerified || + previous is AuthState.PasswordResetLinkSent || + previous is AuthState.EmailSignInLinkSent + if (!wasConsumedNotification && currentRoute != startRoute.route) { navController.navigate(startRoute.route) { popUpTo(navController.graph.findStartDestination().id) { inclusive = true } launchSingleTop = true diff --git a/auth/src/test/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreenErrorNavigationTest.kt b/auth/src/test/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreenErrorNavigationTest.kt new file mode 100644 index 000000000..ab3c05270 --- /dev/null +++ b/auth/src/test/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreenErrorNavigationTest.kt @@ -0,0 +1,152 @@ +/* + * Copyright 2025 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the + * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.firebase.ui.auth.ui.screens + +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.assertIsNotDisplayed +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.test.core.app.ApplicationProvider +import com.firebase.ui.auth.AuthState +import com.firebase.ui.auth.FirebaseAuthUI +import com.firebase.ui.auth.configuration.authUIConfiguration +import com.firebase.ui.auth.configuration.auth_provider.AuthProvider +import com.firebase.ui.auth.configuration.string_provider.DefaultAuthUIStringProvider +import com.google.firebase.FirebaseApp +import com.google.firebase.FirebaseOptions +import com.google.firebase.auth.FirebaseAuth +import org.junit.After +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.Mockito.`when` +import org.mockito.MockitoAnnotations +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Covers a regression surfaced by code review on the stale-AuthState-reset fix: with multiple + * providers configured, an Error occurring on a provider screen (e.g. Email) must not bounce the + * user back to the method picker once the error dialog is consumed. + * + * @suppress Internal test class + */ +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, sdk = [34]) +class FirebaseAuthScreenErrorNavigationTest { + + @get:Rule + val composeTestRule = createComposeRule() + + @Mock + private lateinit var mockFirebaseAuth: FirebaseAuth + + private lateinit var authUI: FirebaseAuthUI + private lateinit var stringProvider: DefaultAuthUIStringProvider + + @Before + fun setUp() { + MockitoAnnotations.openMocks(this) + + FirebaseAuthUI.clearInstanceCache() + + val context = ApplicationProvider.getApplicationContext() + FirebaseApp.getApps(context).forEach { app -> app.delete() } + + val defaultApp = FirebaseApp.initializeApp( + context, + FirebaseOptions.Builder() + .setApiKey("fake-api-key") + .setApplicationId("fake-app-id") + .setProjectId("fake-project-id") + .build() + )!! + + `when`(mockFirebaseAuth.app).thenReturn(defaultApp) + + authUI = FirebaseAuthUI.create(defaultApp, mockFirebaseAuth) + stringProvider = DefaultAuthUIStringProvider(context) + } + + @After + fun tearDown() { + FirebaseAuthUI.clearInstanceCache() + + val context = ApplicationProvider.getApplicationContext() + FirebaseApp.getApps(context).forEach { app -> app.delete() } + } + + @Test + fun `error on email screen with multiple providers does not navigate back to method picker`() { + val configuration = authUIConfiguration { + context = ApplicationProvider.getApplicationContext() + providers { + provider( + AuthProvider.Email( + emailLinkActionCodeSettings = null, + passwordValidationRules = emptyList() + ) + ) + provider( + AuthProvider.Phone( + defaultNumber = null, + defaultCountryCode = null, + allowedCountries = null + ) + ) + } + } + + composeTestRule.setContent { + FirebaseAuthScreen( + configuration = configuration, + authUI = authUI, + onSignInSuccess = {}, + onSignInFailure = {}, + onSignInCancelled = {}, + ) + } + + // Navigate from the method picker into the Email screen. + composeTestRule.onNodeWithText(stringProvider.signInWithEmail) + .assertIsDisplayed() + .performClick() + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(stringProvider.signInDefault) + .assertIsDisplayed() + + // Trigger a plain error while on the Email screen. + composeTestRule.runOnIdle { + authUI.updateAuthState(AuthState.Error(Exception("boom"))) + } + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(stringProvider.errorDialogTitle) + .assertIsDisplayed() + + // Dismiss the dialog. + composeTestRule.onNodeWithText(stringProvider.dismissAction) + .performClick() + composeTestRule.waitForIdle() + + // We must still be on the Email screen, not bounced back to the method picker. + composeTestRule.onNodeWithText(stringProvider.signInDefault) + .assertIsDisplayed() + composeTestRule.onNodeWithText(stringProvider.signInWithEmail) + .assertIsNotDisplayed() + } +} diff --git a/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt b/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt index 7ca09692c..ff61ae95f 100644 --- a/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt +++ b/e2eTest/src/test/java/com/firebase/ui/auth/ui/screens/EmailAuthScreenTest.kt @@ -499,21 +499,18 @@ class EmailAuthScreenTest { println("TEST: Pumping looper after click...") shadowOf(Looper.getMainLooper()).idle() - // Wait for auth state to transition to PasswordResetLinkSent - println("TEST: Waiting for auth state change... Current state: $currentAuthState") + // Wait for the dialog rather than polling currentAuthState, which the screen resets to + // Idle right after consuming PasswordResetLinkSent (see EmailSignInLinkSent test above). + println("TEST: Waiting for password reset link sent dialog...") composeAndroidTestRule.waitUntil(timeoutMillis = AUTH_STATE_WAIT_TIMEOUT_MS) { shadowOf(Looper.getMainLooper()).idle() - println("TEST: Auth state during wait: $currentAuthState") - currentAuthState is AuthState.PasswordResetLinkSent + composeAndroidTestRule.onAllNodesWithText(stringProvider.recoverPasswordLinkSentDialogTitle) + .fetchSemanticsNodes().isNotEmpty() } // Ensure final recomposition is complete before assertions shadowOf(Looper.getMainLooper()).idle() - // Verify the auth state and user properties - println("TEST: Verifying final auth state: $currentAuthState") - assertThat(currentAuthState) - .isInstanceOf(AuthState.PasswordResetLinkSent::class.java) assertThat(authUI.auth.currentUser).isNull() composeAndroidTestRule.onNodeWithText(stringProvider.recoverPasswordLinkSentDialogTitle) .assertIsDisplayed() From 81cd4840f1c7101dd55cf1404d55c9f7e9ae93ae Mon Sep 17 00:00:00 2001 From: demolaf Date: Wed, 29 Jul 2026 13:58:17 +0100 Subject: [PATCH 3/3] fix(auth): derive Idle-navigation suppression from AuthState.isNotification instead of a hardcoded list --- .../java/com/firebase/ui/auth/AuthState.kt | 21 +++++++++++++++++++ .../ui/auth/ui/screens/FirebaseAuthScreen.kt | 13 ++++-------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/auth/src/main/java/com/firebase/ui/auth/AuthState.kt b/auth/src/main/java/com/firebase/ui/auth/AuthState.kt index 697480213..ccfa4db21 100644 --- a/auth/src/main/java/com/firebase/ui/auth/AuthState.kt +++ b/auth/src/main/java/com/firebase/ui/auth/AuthState.kt @@ -34,10 +34,19 @@ import com.google.firebase.auth.PhoneAuthProvider */ abstract class AuthState private constructor() { + /** + * Whether this is a one-off notification: something a screen shows once (a dialog, a "link + * sent" message) and must reset back to [Idle] immediately after consuming, so it doesn't + * leak to a screen/Activity created later. `abstract` so every new state must explicitly + * decide this rather than silently defaulting one way. + */ + abstract val isNotification: Boolean + /** * Initial state before any authentication operation has been started. */ class Idle internal constructor() : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean = other is Idle override fun hashCode(): Int = javaClass.hashCode() override fun toString(): String = "AuthState.Idle" @@ -49,6 +58,7 @@ abstract class AuthState private constructor() { * @property message Optional message describing what is being loaded */ class Loading(val message: String? = null) : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is Loading) return false @@ -72,6 +82,7 @@ abstract class AuthState private constructor() { val user: FirebaseUser, val isNewUser: Boolean = false ) : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is Success) return false @@ -101,6 +112,7 @@ abstract class AuthState private constructor() { val exception: Exception, val isRecoverable: Boolean = true ) : AuthState() { + override val isNotification: Boolean = true override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is Error) return false @@ -122,6 +134,7 @@ abstract class AuthState private constructor() { * Authentication was cancelled by the user. */ class Cancelled internal constructor() : AuthState() { + override val isNotification: Boolean = true override fun equals(other: Any?): Boolean = other is Cancelled override fun hashCode(): Int = javaClass.hashCode() override fun toString(): String = "AuthState.Cancelled" @@ -137,6 +150,7 @@ abstract class AuthState private constructor() { val resolver: MultiFactorResolver, val hint: String? = null ) : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is RequiresMfa) return false @@ -164,6 +178,7 @@ abstract class AuthState private constructor() { val user: FirebaseUser, val email: String ) : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is RequiresEmailVerification) return false @@ -191,6 +206,7 @@ abstract class AuthState private constructor() { val user: FirebaseUser, val missingFields: List = emptyList() ) : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is RequiresProfileCompletion) return false @@ -221,6 +237,7 @@ abstract class AuthState private constructor() { // Not included in equals/hashCode — lambdas have no meaningful equality. val retryOperation: (suspend (android.content.Context) -> Unit)? = null, ) : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is ReauthenticationRequired) return false @@ -241,6 +258,7 @@ abstract class AuthState private constructor() { * Password reset link has been sent to the user's email. */ class PasswordResetLinkSent : AuthState() { + override val isNotification: Boolean = true override fun equals(other: Any?): Boolean = other is PasswordResetLinkSent override fun hashCode(): Int = javaClass.hashCode() override fun toString(): String = "AuthState.PasswordResetLinkSent" @@ -250,6 +268,7 @@ abstract class AuthState private constructor() { * Email sign in link has been sent to the user's email. */ class EmailSignInLinkSent : AuthState() { + override val isNotification: Boolean = true override fun equals(other: Any?): Boolean = other is EmailSignInLinkSent override fun hashCode(): Int = javaClass.hashCode() override fun toString(): String = "AuthState.EmailSignInLinkSent" @@ -267,6 +286,7 @@ abstract class AuthState private constructor() { * @see PhoneNumberVerificationRequired for the manual verification flow */ class SMSAutoVerified(val credential: PhoneAuthCredential) : AuthState() { + override val isNotification: Boolean = true override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is SMSAutoVerified) return false @@ -305,6 +325,7 @@ abstract class AuthState private constructor() { val verificationId: String, val forceResendingToken: PhoneAuthProvider.ForceResendingToken, ) : AuthState() { + override val isNotification: Boolean = false override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is PhoneNumberVerificationRequired) return false diff --git a/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt b/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt index 733b7f158..f1e501d6a 100644 --- a/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt +++ b/auth/src/main/java/com/firebase/ui/auth/ui/screens/FirebaseAuthScreen.kt @@ -159,7 +159,7 @@ fun FirebaseAuthScreen( val lastSignInPreference = remember { mutableStateOf(null) } // Last-processed AuthState, so the Idle branch below can tell a genuine reset apart from - // Idle-as-a-side-effect of consuming a one-off notification (see wasConsumedNotification). + // Idle-as-a-side-effect of consuming a notification (see AuthState.isNotification). val previousAuthState = remember { mutableStateOf(AuthState.Idle) } val startRoute = remember(configuration.providers, configuration.isProviderChoiceAlwaysShown) { getStartRoute(configuration) @@ -572,14 +572,9 @@ fun FirebaseAuthScreen( pendingResolver.value = null pendingLinkingCredential.value = null lastSuccessfulUserId.value = null - // A one-off notification resets to Idle purely to avoid leaking to a - // freshly created screen — that's not a request to leave the current one. - val wasConsumedNotification = previous is AuthState.Error || - previous is AuthState.Cancelled || - previous is AuthState.SMSAutoVerified || - previous is AuthState.PasswordResetLinkSent || - previous is AuthState.EmailSignInLinkSent - if (!wasConsumedNotification && currentRoute != startRoute.route) { + // A notification resets to Idle purely to avoid leaking to a freshly + // created screen — that's not a request to leave the current one. + if (!previous.isNotification && currentRoute != startRoute.route) { navController.navigate(startRoute.route) { popUpTo(navController.graph.findStartDestination().id) { inclusive = true } launchSingleTop = true