refactor(auth)!: decouple reauthentication from the FirebaseAuthUI shared state flow - #2478
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the reauthentication flow in FirebaseUI Auth to use a composition-scoped AuthFlowScope and coroutine-based suspension (CompletableDeferred) instead of process-local callbacks on FirebaseAuthUI. This prevents sensitive operations from being lost or executed multiple times during Activity recreation. Various auth providers have been updated to emit states through this new scope. Feedback is provided to improve code safety in authUserState by avoiding the !! operator on user.email through smart casting.
bf937e9 to
c3c7802
Compare
…and a screen-scoped phase holder
… FirebaseAuthUI singleton
… a stale composition value
… of returning quietly
…ad of calling it arming
…tead of the shared state flow
c3c7802 to
d918b91
Compare
russellwheatley
left a comment
There was a problem hiding this comment.
Went through this end to end (checked out the branch, read the actual files rather than just the diff, since a phase-machine/coroutine refactor like this is easy to get wrong on diff alone).
The decoupling itself looks solid: AuthFlowScope genuinely walls provider code off from the old singleton flow, all six provider files migrated consistently, and the nine deleted FirebaseAuthUI members have no leftover references anywhere.
Found one hang I think is real (see inline) plus a smaller state-coherence issue on failed retries, and a couple of stale doc comments left over from the refactor. Details inline.
…not just skips the retry
FirebaseAuthUIwas the SDK entry point, the process-wideAuthStatechannel and the reauthentication session store all at once. Reauthentication requests now travel on their own channel, provider code emits to anAuthFlowScopesink instead of the singleton's flow, and the phase machine lives in a composition-scoped holder — nine members deleted fromFirebaseAuthUI, none added.withReauthno longer retains the caller's lambda on a process-scoped singleton: it raises a request, suspends on a resolver, and retries on the caller's own coroutine.delete()goes through the same path instead of publishingRequiredand throwing.AuthFlowScope.kt: provider extensions move offFirebaseAuthUI, so provider code can no longer reach the public state flow at all — the isolation is compiler-held rather than review-held.ReauthFlowState.kt: the phase machine, scoped to oneFirebaseAuthScreen's composition.Abortednow clears the phase and resolves the caller under both hosts.SignInStateSequenceTestandReauthFlowStateTest, plus emulator-backed e2e coverage for a password change and an account deletion through reauthentication.AuthState.Reauthentication.Required.retryOperationis removed outright — pre-GA, zero external usage, no shim.AuthState.Reauthentication.Required's(user, reason)constructor is nowinternal. Raising a request iswithReauth/delete()'s job; constructing one yourself no longer did anything.AuthException.AuthCancelledExceptioninstead of returning quietly, anddelete()reports that rather thanInvalidCredentialsException.AuthState.Reauthentication.RequiredtoauthStateFlow()yourself no longer raises a request; onlywithReauthanddelete()do.Maintainer note: Fixes internal CPRN-413