feat(auth): add reauthentication flow with automatic operation retry#2332
feat(auth): add reauthentication flow with automatic operation retry#2332demolaf wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a reauthentication flow to the FirebaseUI Auth library, which is required before performing sensitive operations such as account deletion. It adds a new AuthState.ReauthenticationRequired state, a createReauthFlow method in FirebaseAuthUI, and updates the email and OAuth providers to support reauthentication. Additionally, it integrates a reauthentication bottom sheet in FirebaseAuthScreen and updates the demo application to demonstrate account deletion. The review feedback highlights three key improvement opportunities: wrapping the retry(context) invocation in a try-catch block within FirebaseAuthScreen to prevent potential crashes from unhandled exceptions, and replacing unsafe double-bang (!!) operators on auth.currentUser with defensive null checks in both EmailAuthProvider+FirebaseAuthUI.kt and OAuthProvider+FirebaseAuthUI.kt.
| // Lock the state to Loading before launching the retry so no | ||
| // intermediate Success emission can navigate to AuthRoute.Success. | ||
| authUI.updateAuthState(AuthState.Loading()) | ||
| coroutineScope.launch { retry(context) } |
There was a problem hiding this comment.
Launching retry(context) directly inside coroutineScope.launch without a try-catch block can lead to an unhandled exception and crash the application if the retried operation (such as delete(context)) throws an exception (e.g., due to network issues or another authentication error). Since delete explicitly re-throws exceptions after updating the state, we should safely catch any exceptions thrown during the retry.
coroutineScope.launch {
try {
retry(context)
} catch (e: Exception) {
Log.e(\"FirebaseAuthScreen\", \"Retrying operation failed\", e)
}
}| ): AuthResult? = if (config.isReauthenticationMode) { | ||
| auth.currentUser!!.reauthenticate(credential).await() | ||
| null | ||
| } else { |
There was a problem hiding this comment.
Using the double-bang operator (!!) on auth.currentUser is unsafe and can lead to a NullPointerException if the user session is concurrently cleared or invalidated. It is safer to use a defensive null check and throw a descriptive AuthException.UserNotFoundException if the user is null.
): AuthResult? = if (config.isReauthenticationMode) {
val currentUser = auth.currentUser ?: throw AuthException.UserNotFoundException(
message = \"No user is currently signed in for reauthentication\"
)
currentUser.reauthenticate(credential).await()
null
} else {| config.isReauthenticationMode -> | ||
| auth.currentUser!!.startActivityForReauthenticateWithProvider(activity, oauthProvider).await() |
There was a problem hiding this comment.
Using the double-bang operator (!!) on auth.currentUser is unsafe and can lead to a NullPointerException if the user session is concurrently cleared or invalidated. It is safer to use a defensive null check and throw a descriptive AuthException.UserNotFoundException if the user is null.
config.isReauthenticationMode -> {
val currentUser = auth.currentUser ?: throw AuthException.UserNotFoundException(
message = \"No user is currently signed in for reauthentication\"
)
currentUser.startActivityForReauthenticateWithProvider(activity, oauthProvider).await()
}
Closes #563 .