fix(auth): clear MFA enrollment flow state on every entry - #2463
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a state reset mechanism to the MFA enrollment screen upon successful enrollment, resetting it back to the SelectFactor step and clearing consumed inputs before invoking onComplete. To prevent the UI from prematurely resetting during the back-stack pop transition, a MfaEnrollmentDestination wrapper with a RetainedMfaEnrollmentState has been introduced to freeze and retain the completed state during the transition. Additionally, single-factor auto-advance is now suppressed after a completed enrollment to avoid re-triggering the flow upon state restoration. Comprehensive unit tests have been added to verify these behaviors. I have no feedback to provide as there are no review comments.
837d1b8 to
bcb4a97
Compare
ab4f6dd to
f3f02fd
Compare
bcb4a97 to
5a7ea86
Compare
f3f02fd to
ce24ffa
Compare
russellwheatley
left a comment
There was a problem hiding this comment.
Frozen Verify stays tappable through the pop fade. Those callbacks close over the already-reset live state, so a tap can surface onError after enrollment already succeeded.
Suggested a copy in freeze() that disables the form, and an isLoading pin on the retention test.
ce24ffa to
7e2ad77
Compare
7e2ad77 to
56f7e51
Compare
56f7e51 to
d40fa39
Compare
548c396 to
a38bb4f
Compare
After a successful MFA enrollment the flow state was never cleared, so re-entering landed the user on a form pre-filled from the previous attempt — phone number, verification code and a consumed
smsSessionall still live. #2467 moved that state above theNavHostand made most of itrememberSaveable, so it now outlives both step navigation and process death: longer-lived rather than cleaner. Across a sign-out, that is the previous user's data.MfaEnrollmentFlowState.reset()returns all nine fields to the valuesrememberMfaEnrollmentFlowState()starts them at, and the host calls it on flow entry rather than on completion, through a newenterMfaEnrollmentextension. Resetting on completion would mutate state a leaving step still reads while it is composed for the exit transition; on the way in there is no such step.Every entry now goes through that helper, including one that previously did not.
AuthRoute.MfaEnrollmentandAuthRoute.MfaEnrollment.SelectFactorare different objects reporting the sameroutestring, soonNavigate(AuthRoute.MfaEnrollment.SelectFactor)looked equivalent toonNavigate(AuthRoute.MfaEnrollment)but fell through to a plainnavigatewith no clear. A step named that way now clears and still lands on the step the host asked for, rather than being redirected through the resolved start step — a host deliberately showing the picker under a single-factor configuration keeps that behaviour.This replaces the earlier approach on this branch, which reset inside
MfaEnrollmentScreenand retained the pre-reset state through the pop transition. @russellwheatley found that the frozen snapshot's callbacks still closed over the cleared state, so Verify stayed tappable through the fade and a tap surfacedonErrorafter enrollment had already succeeded. Per-step navigation removes the reset window altogether, so all ~117 lines of retention machinery are dropped rather than patched.Adds 8 tests. Each clause of the widened entry condition is independently pinned by mutation, as is the step-resolution path and every field of
reset(): three mutants, each killing exactly the test that names the behaviour it breaks. Note the one-clause "simplification" compiles cleanly, so only a test catches it.A single-pop bug in this area — steps push, but completion popped one entry and landed on
ConfigureSmsinstead of leaving the flow — was found while writing these tests and is being fixed in #2467 instead, since it propagates up the nav stack and a fix landing only here would be silently reintroduced by #2469.Maintainer note: Fixes internal CPRN-384