-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(auth): stale one-off AuthState no longer leaks across screen instances #2415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: version-10.0.0-beta04
Are you sure you want to change the base?
Changes from all commits
00640cb
89bf319
cb6aa67
55a5744
4b6658a
a6848c2
07af5ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,18 +40,18 @@ val LocalTopLevelDialogController = compositionLocalOf<TopLevelDialogController? | |||||||||||||
| * **Usage:** | ||||||||||||||
| * ```kotlin | ||||||||||||||
| * // At the root of your auth flow (FirebaseAuthScreen): | ||||||||||||||
| * val dialogController = rememberTopLevelDialogController(stringProvider) | ||||||||||||||
| * | ||||||||||||||
| * val dialogController = rememberTopLevelDialogController(stringProvider) { authState } | ||||||||||||||
| * | ||||||||||||||
| * CompositionLocalProvider(LocalTopLevelDialogController provides dialogController) { | ||||||||||||||
| * // Your auth screens... | ||||||||||||||
| * | ||||||||||||||
| * | ||||||||||||||
| * // Show dialog at root level (only one instance) | ||||||||||||||
| * dialogController.CurrentDialog() | ||||||||||||||
| * } | ||||||||||||||
| * | ||||||||||||||
| * | ||||||||||||||
| * // In any child screen (EmailAuthScreen, PhoneAuthScreen, etc.): | ||||||||||||||
| * val dialogController = LocalTopLevelDialogController.current | ||||||||||||||
| * | ||||||||||||||
| * | ||||||||||||||
| * LaunchedEffect(error) { | ||||||||||||||
| * error?.let { exception -> | ||||||||||||||
| * dialogController?.showErrorDialog( | ||||||||||||||
|
|
@@ -68,7 +68,7 @@ val LocalTopLevelDialogController = compositionLocalOf<TopLevelDialogController? | |||||||||||||
| */ | ||||||||||||||
| class TopLevelDialogController( | ||||||||||||||
| private val stringProvider: AuthUIStringProvider, | ||||||||||||||
| private val authState: AuthState | ||||||||||||||
| private val currentAuthState: () -> AuthState | ||||||||||||||
| ) { | ||||||||||||||
| private var dialogState by mutableStateOf<DialogState?>(null) | ||||||||||||||
| private val shownErrorStates = mutableSetOf<AuthState.Error>() | ||||||||||||||
|
|
@@ -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) | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+175
to
177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In We should key the
Suggested change
|
||||||||||||||
| } | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please wrap val stringProvider = remember(context) { DefaultAuthUIStringProvider(context) } |
||
| val lastSuccessfulUserId = remember { mutableStateOf<String?>(null) } | ||
| val pendingLinkingCredential = remember { mutableStateOf<AuthCredential?>(null) } | ||
| val pendingResolver = remember { mutableStateOf<MultiFactorResolver?>(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) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a subtle race condition in the error dialog deduplication logic. Because
authUI.updateAuthState(AuthState.Idle)is called immediately aftershowErrorDialogin the observing screens (e.g.,EmailAuthScreenandFirebaseAuthScreen), the first observer to run will reset the auth state toIdle. When the second observer runs and callsshowErrorDialog,currentAuthState()will returnAuthState.Idleinstead ofAuthState.Error. This causescurrentErrorStateto benull, bypassing the deduplication check and allowing the second call to overwrite the dialog state (potentially losing customonRetryoronRecovercallbacks).To fix this, consider adding an optional
errorState: AuthState.Error? = nullparameter toshowErrorDialogand using it for deduplication: