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..cf41a3c5c 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() @@ -89,7 +89,7 @@ class TopLevelDialogController( onDismiss: () -> Unit = {} ) { // Get current error state - val currentErrorState = authState as? AuthState.Error + val currentErrorState = currentAuthState() as? AuthState.Error // If this exact error state has already been shown, skip if (currentErrorState != null && currentErrorState in shownErrorStates) { @@ -162,13 +162,17 @@ 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. */ @Composable fun rememberTopLevelDialogController( stringProvider: AuthUIStringProvider, - authState: AuthState + authState: () -> AuthState ): TopLevelDialogController { - return remember(stringProvider, authState) { + return remember { 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 fffcf5905..c213b56eb 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 @@ -139,7 +139,7 @@ fun FirebaseAuthScreen( 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) } @@ -542,6 +542,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 -> { @@ -632,6 +634,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..3de1876c6 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) } @@ -229,10 +233,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 +265,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 +304,7 @@ fun EmailAuthScreen( } }, onSignInEmailLinkClick = { + emailSignInLinkSentLocal = false coroutineScope.launch { try { if (emailLinkFromDifferentDevice != null) { @@ -327,6 +346,7 @@ fun EmailAuthScreen( } }, onSendResetLinkClick = { + resetLinkSentLocal = false coroutineScope.launch { try { authUI.sendPasswordResetEmail( @@ -346,14 +366,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..6da6e5465 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( @@ -228,10 +231,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..0d43e9750 --- /dev/null +++ b/auth/src/test/java/com/firebase/ui/auth/ui/components/TopLevelDialogControllerTest.kt @@ -0,0 +1,98 @@ +package com.firebase.ui.auth.ui.components + +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithText +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() + } +} 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()