Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,18 @@ import kotlinx.coroutines.tasks.await
/**
* Creates a remembered launcher function for anonymous sign-in.
*
* @param config Authentication UI configuration
* @param onSignInFailure Callback invoked with the resulting [AuthException] on failure
* @return A launcher function that starts the anonymous sign-in flow when invoked
*
* @see signInAnonymously
* @see createOrLinkUserWithEmailAndPassword for upgrading anonymous accounts
*/
@Composable
internal fun FirebaseAuthUI.rememberAnonymousSignInHandler(config: AuthUIConfiguration): () -> Unit {
internal fun FirebaseAuthUI.rememberAnonymousSignInHandler(
config: AuthUIConfiguration,
onSignInFailure: (AuthException) -> Unit = {},
): () -> Unit {
Comment on lines +25 to +28

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The remember block on line 31 currently only keys on this (remember(this)). Since config, onSignInFailure, and context are captured inside the lambda but not included in the remember keys, any changes to these parameters (such as a new callback instance or updated configuration) will not be reflected in the returned launcher. This can lead to stale captures and memory leaks.\n\nPlease update the remember keys to include these dependencies:\nkotlin\nreturn remember(this, context, config, onSignInFailure) {\n

val context = androidx.compose.ui.platform.LocalContext.current
val coroutineScope = rememberCoroutineScope()
return remember(this) {
Expand All @@ -31,9 +36,11 @@ internal fun FirebaseAuthUI.rememberAnonymousSignInHandler(config: AuthUIConfigu
} catch (e: AuthException) {
// Already an AuthException, don't re-wrap it
updateAuthState(AuthState.Error(e))
onSignInFailure(e)
} catch (e: Exception) {
val authException = AuthException.from(e, context)
updateAuthState(AuthState.Error(authException))
onSignInFailure(authException)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import kotlinx.coroutines.launch
* @param config The [AuthUIConfiguration] containing authentication settings
* @param provider The [AuthProvider.Facebook] configuration with scopes and credential provider
* @param loginManagerProvider Provides logout operations to clear stale Facebook sessions
* @param onSignInFailure Callback invoked with the resulting [AuthException] on failure
*
* @return A launcher function that starts the Facebook sign-in flow when invoked
*
Expand All @@ -58,6 +59,7 @@ internal fun FirebaseAuthUI.rememberSignInWithFacebookLauncher(
config: AuthUIConfiguration,
provider: AuthProvider.Facebook,
loginManagerProvider: AuthProvider.Facebook.LoginManagerProvider = AuthProvider.Facebook.DefaultLoginManagerProvider(),
onSignInFailure: (AuthException) -> Unit = {},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The DisposableEffect on line 76 is currently keyed only on config. However, the registered FacebookCallback captures context, provider, and onSignInFailure. If any of these parameters change while config remains the same, the callback will use stale references.\n\nPlease update the DisposableEffect keys to include all captured dependencies:\nkotlin\nDisposableEffect(context, config, provider, onSignInFailure) {\n

): () -> Unit {
val coroutineScope = rememberCoroutineScope()
val callbackManager = remember { CallbackManager.Factory.create() }
Expand Down Expand Up @@ -87,9 +89,11 @@ internal fun FirebaseAuthUI.rememberSignInWithFacebookLauncher(
} catch (e: AuthException) {
// Already an AuthException, don't re-wrap it
updateAuthState(AuthState.Error(e))
onSignInFailure(e)
} catch (e: Exception) {
val authException = AuthException.from(e, context)
updateAuthState(AuthState.Error(authException))
onSignInFailure(authException)
}
}
}
Expand All @@ -106,6 +110,7 @@ internal fun FirebaseAuthUI.rememberSignInWithFacebookLauncher(
authException
)
)
onSignInFailure(authException)
}
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import kotlinx.coroutines.launch
* @param context Android context for Credential Manager
* @param config Authentication UI configuration
* @param provider Google provider configuration with server client ID and optional scopes
* @param onSignInFailure Callback invoked with the resulting [AuthException] on failure
* @return A callback function that initiates Google Sign-In when invoked
*
* @see signInWithGoogle
Expand All @@ -57,6 +58,7 @@ internal fun FirebaseAuthUI.rememberGoogleSignInHandler(
context: Context,
config: AuthUIConfiguration,
provider: AuthProvider.Google,
onSignInFailure: (AuthException) -> Unit = {},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The remember block on line 64 currently only keys on this, config. However, the lambda captures context, provider, and onSignInFailure. If any of these captured parameters change (for example, if the context changes due to configuration changes or a new onSignInFailure callback is provided), the returned launcher will continue to use the stale captured references.\n\nPlease update the remember keys to include all captured dependencies:\nkotlin\nreturn remember(this, context, config, provider, onSignInFailure) {\n

): () -> Unit {
val coroutineScope = rememberCoroutineScope()
return remember(this, config) {
Expand All @@ -66,9 +68,11 @@ internal fun FirebaseAuthUI.rememberGoogleSignInHandler(
signInWithGoogle(context, config, provider)
} catch (e: AuthException) {
updateAuthState(AuthState.Error(e))
onSignInFailure(e)
} catch (e: Exception) {
val authException = AuthException.from(e, context)
updateAuthState(AuthState.Error(authException))
onSignInFailure(authException)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import kotlinx.coroutines.tasks.await
*
* @param config Authentication UI configuration
* @param provider OAuth provider configuration
* @param onSignInFailure Callback invoked with the resulting [AuthException] on failure
*
* @return Lambda that triggers OAuth sign-in when invoked
*
Expand All @@ -54,6 +55,7 @@ internal fun FirebaseAuthUI.rememberOAuthSignInHandler(
activity: Activity?,
config: AuthUIConfiguration,
provider: AuthProvider.OAuth,
onSignInFailure: (AuthException) -> Unit = {},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The remember block on line 66 keys on this, provider.providerId, config. However, the lambda captures context, activity, provider, and onSignInFailure.\n\nIn particular, capturing activity without keying on it is highly risky. If the Activity is recreated (e.g., due to screen rotation), the lambda will hold a stale reference to the destroyed activity, causing a memory leak and potentially crashing when attempting to start the OAuth flow.\n\nPlease update the remember keys to include all captured dependencies:\nkotlin\nreturn remember(this, context, activity, config, provider, onSignInFailure) {\n

): () -> Unit {
val coroutineScope = rememberCoroutineScope()
activity ?: throw IllegalStateException(
Expand All @@ -73,9 +75,11 @@ internal fun FirebaseAuthUI.rememberOAuthSignInHandler(
)
} catch (e: AuthException) {
updateAuthState(AuthState.Error(e))
onSignInFailure(e)
} catch (e: Exception) {
val authException = AuthException.from(e, context)
updateAuthState(AuthState.Error(authException))
onSignInFailure(authException)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ fun FirebaseAuthScreen(
)
)
},
onSignInFailure = onSignInFailure,
)
val continueWithProvider: (String) -> Unit = { providerId ->
configuration.providers.find { it.providerId == providerId }?.let { onProviderSelected(it) }
Expand Down Expand Up @@ -994,6 +995,7 @@ private fun FirebaseAuthUI.rememberOnProviderSelected(
config: AuthUIConfiguration,
onNavigate: (AuthRoute) -> Unit,
onUnknownProvider: ((AuthProvider) -> Unit)? = null,
onSignInFailure: (AuthException) -> Unit = {},
): (AuthProvider) -> Unit {
val anonymousProvider = config.providers.filterIsInstance<AuthProvider.Anonymous>().firstOrNull()
val googleProvider = config.providers.filterIsInstance<AuthProvider.Google>().firstOrNull()
Expand All @@ -1005,16 +1007,18 @@ private fun FirebaseAuthUI.rememberOnProviderSelected(
val twitterProvider = config.providers.filterIsInstance<AuthProvider.Twitter>().firstOrNull()
val genericOAuthProviders = config.providers.filterIsInstance<AuthProvider.GenericOAuth>()

val onSignInAnonymously = anonymousProvider?.let { rememberAnonymousSignInHandler(config) }
val onSignInWithGoogle = googleProvider?.let { rememberGoogleSignInHandler(context, config, it) }
val onSignInWithFacebook = facebookProvider?.let { rememberSignInWithFacebookLauncher(context, config, it) }
val onSignInWithApple = appleProvider?.let { rememberOAuthSignInHandler(context, activity, config, it) }
val onSignInWithGithub = githubProvider?.let { rememberOAuthSignInHandler(context, activity, config, it) }
val onSignInWithMicrosoft = microsoftProvider?.let { rememberOAuthSignInHandler(context, activity, config, it) }
val onSignInWithYahoo = yahooProvider?.let { rememberOAuthSignInHandler(context, activity, config, it) }
val onSignInWithTwitter = twitterProvider?.let { rememberOAuthSignInHandler(context, activity, config, it) }
val onSignInAnonymously = anonymousProvider?.let { rememberAnonymousSignInHandler(config, onSignInFailure) }
val onSignInWithGoogle = googleProvider?.let { rememberGoogleSignInHandler(context, config, it, onSignInFailure) }
val onSignInWithFacebook = facebookProvider?.let {
rememberSignInWithFacebookLauncher(context, config, it, onSignInFailure = onSignInFailure)
}
val onSignInWithApple = appleProvider?.let { rememberOAuthSignInHandler(context, activity, config, it, onSignInFailure) }
val onSignInWithGithub = githubProvider?.let { rememberOAuthSignInHandler(context, activity, config, it, onSignInFailure) }
val onSignInWithMicrosoft = microsoftProvider?.let { rememberOAuthSignInHandler(context, activity, config, it, onSignInFailure) }
val onSignInWithYahoo = yahooProvider?.let { rememberOAuthSignInHandler(context, activity, config, it, onSignInFailure) }
val onSignInWithTwitter = twitterProvider?.let { rememberOAuthSignInHandler(context, activity, config, it, onSignInFailure) }
val genericOAuthHandlers = genericOAuthProviders.associateWith {
rememberOAuthSignInHandler(context, activity, config, it)
rememberOAuthSignInHandler(context, activity, config, it, onSignInFailure)
}

return { provider ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.firebase.ui.auth.configuration.auth_provider

import android.content.Context
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.test.core.app.ApplicationProvider
import com.firebase.ui.auth.AuthException
import com.firebase.ui.auth.AuthState
Expand All @@ -38,6 +39,7 @@ import kotlinx.coroutines.launch
import kotlinx.coroutines.test.runTest
import org.junit.After
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentMatchers
Expand All @@ -54,6 +56,9 @@ import org.robolectric.annotation.Config
@Config(manifest = Config.NONE)
class AnonymousAuthProviderFirebaseAuthUITest {

@get:Rule
val composeTestRule = createComposeRule()

@Mock
private lateinit var mockFirebaseAuth: FirebaseAuth

Expand Down Expand Up @@ -214,6 +219,35 @@ class AnonymousAuthProviderFirebaseAuthUITest {
assertThat(errorState.exception).isInstanceOf(AuthException.UnknownException::class.java)
}

// =============================================================================================
// rememberAnonymousSignInHandler - onSignInFailure reporting
// =============================================================================================

@Test
fun `rememberAnonymousSignInHandler reports failure via onSignInFailure immediately, at the source`() {
val networkException = FirebaseNetworkException("Network error")
val taskCompletionSource = TaskCompletionSource<AuthResult>()
taskCompletionSource.setException(networkException)
`when`(mockFirebaseAuth.signInAnonymously()).thenReturn(taskCompletionSource.task)

val instance = FirebaseAuthUI.create(firebaseApp, mockFirebaseAuth)
val reportedFailures = mutableListOf<AuthException>()
var launcher: (() -> Unit)? = null

composeTestRule.setContent {
launcher = instance.rememberAnonymousSignInHandler(
config = config,
onSignInFailure = { reportedFailures.add(it) },
)
}

composeTestRule.runOnIdle { launcher?.invoke() }
composeTestRule.waitForIdle()

assertThat(reportedFailures).hasSize(1)
assertThat(reportedFailures.single()).isInstanceOf(AuthException.NetworkException::class.java)
}

// =============================================================================================
// Anonymous Account Upgrade Tests
// =============================================================================================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.firebase.ui.auth.configuration.auth_provider

import android.content.Context
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.core.net.toUri
import androidx.credentials.CredentialManager
import androidx.test.core.app.ApplicationProvider
Expand All @@ -37,6 +38,7 @@ import kotlinx.coroutines.flow.first
import kotlinx.coroutines.test.runTest
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
Expand Down Expand Up @@ -67,6 +69,9 @@ import org.robolectric.annotation.Config
@Config(manifest = Config.NONE)
class GoogleAuthProviderFirebaseAuthUITest {

@get:Rule
val composeTestRule = createComposeRule()

@Mock
private lateinit var mockFirebaseAuth: FirebaseAuth

Expand Down Expand Up @@ -920,4 +925,59 @@ class GoogleAuthProviderFirebaseAuthUITest {
val finalState = instance.authStateFlow().first { it !is AuthState.Loading }
assertThat(finalState).isEqualTo(AuthState.Success(result = mockAuthResult, user = mockUser, isNewUser = false))
}

// =============================================================================================
// rememberGoogleSignInHandler - onSignInFailure reporting
// =============================================================================================

@Test
fun `rememberGoogleSignInHandler reports failure via onSignInFailure immediately, at the source`() {
val instance = FirebaseAuthUI.create(firebaseApp, mockFirebaseAuth)
val googleProvider = AuthProvider.Google(
serverClientId = "test-client-id",
scopes = emptyList()
)
val config = authUIConfiguration {
context = applicationContext
providers {
provider(googleProvider)
}
}

// A picker-level failure that used to never reach onSignInFailure at all:
// the outer fallback throws AuthException.UnknownException when no Google accounts are found.
instance.testCredentialManagerProvider = object : AuthProvider.Google.CredentialManagerProvider {
override suspend fun getGoogleCredential(
context: Context,
credentialManager: CredentialManager,
serverClientId: String,
filterByAuthorizedAccounts: Boolean,
autoSelectEnabled: Boolean
): AuthProvider.Google.GoogleSignInResult {
throw AuthException.UnknownException(
"No Google accounts available.\n\nPlease add a Google account to your device and try again."
)
}

override suspend fun clearCredentialState(context: Context, credentialManager: CredentialManager) = Unit
}

val reportedFailures = mutableListOf<AuthException>()
var launcher: (() -> Unit)? = null

composeTestRule.setContent {
launcher = instance.rememberGoogleSignInHandler(
context = applicationContext,
config = config,
provider = googleProvider,
onSignInFailure = { reportedFailures.add(it) },
)
}

composeTestRule.runOnIdle { launcher?.invoke() }
composeTestRule.waitForIdle()

assertThat(reportedFailures).hasSize(1)
assertThat(reportedFailures.single()).isInstanceOf(AuthException.UnknownException::class.java)
}
}