[ECUK In-App 3DS] refactor MFA REASON#87975
[ECUK In-App 3DS] refactor MFA REASON#87975dariusz-biela wants to merge 37 commits intoExpensify:mainfrom
Conversation
…UTHN/HSM to SERVER_ERRORS/CLIENT_ERRORS/LOCAL_ERRORS Reorganize error grouping by HTTP status type instead of technology source: - BACKEND.* → CLIENT_ERRORS.* (all are 4xx API responses) - GENERIC.CLIENT_ERROR/SERVER_ERROR/LOCAL_ERROR → CLIENT_ERRORS/SERVER_ERRORS/LOCAL_ERRORS.UNHANDLED - WEBAUTHN.* → LOCAL_ERRORS.WEBAUTHN.* (nested under LOCAL_ERRORS) - HSM.* → LOCAL_ERRORS.HSM.* (nested under LOCAL_ERRORS) Also remove unused REASON/BACKEND_MESSAGE values, remove reason from success result types, improve helpers.ts fallback logic with case-insensitive matching, and update ReasonValue type to handle 3-level nesting via DeepLeafValues.
- Remove DeepLeafValues duplication: define once in VALUES.ts, export ReasonValue for types.ts to alias as MultifactorAuthenticationReason - Move CLIENT_ERRORS.UNHANDLED from ROUTINE_FAILURES to ANOMALOUS_FAILURES since unexpected 4xx responses may indicate a bug or API change - Return undefined reason for 200 success responses without SUCCESS map entry instead of misleading LOCAL_ERRORS.UNHANDLED; update parseHttpRequest, ProcessResult, ChallengeResponse, and ScenarioResponse types accordingly - Add fallback reasons in SET_ERROR dispatches and null guard in failureScreens lookup for type safety - Pass httpStatusCode and message to all SET_ERROR dispatches that use a generic fallback reason - Document empty API_RESPONSE_MAP entries as relying on fallback reasons
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
… UNHANDLED_ERROR to UNHANDLED_EXCEPTION UNHANDLED and UNHANDLED_ERROR were ambiguous. UNHANDLED_API_RESPONSE clarifies it's the HTTP no-status fallback, while UNHANDLED_EXCEPTION clarifies it catches code exceptions and invalid states.
…RANSACTION_UNAVAILABLE as ANOMALOUS These rare edge cases may indicate bugs and were previously unclassified, meaning they would be logged as UNCLASSIFIED instead of at 'error' level.
Better describes intent: function determines screen routing, not error classification.
- DENY_TRANSACTION SUCCESS branch (typeof responseMapEntry === 'string')
- Empty source map fallback (CHANGE_CARD_PIN with 4xx)
- endsWith prefix tolerance ('Error: Registration required' matches 'Registration required')
REASON.LOCAL_ERRORS.WEBAUTHN values no longer mirror DOMException.name strings. A dedicated DOM_EXCEPTION_NAME_MAP in WebAuthn.ts maps DOMException names to REASON constants (same pattern HSM already uses). Also forwards DOMException.message for known errors.
Make vague fallback reasons more descriptive so they are self-explanatory in logs without needing to cross-reference code.
Prefix every REASON string with its origin category (Server, Client, Local, Local WebAuthn, Local HSM) so the source is immediately visible in logs without needing to look up the constant key.
This reason was never dispatched by any code path. The case it was meant to cover (transaction disappearing from Onyx mid-review) is handled directly in AuthorizeTransactionPage without going through the MFA state machine.
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
Good job overall, left some comments
…rors Replace scattered reason/httpStatusCode/message properties with a single MfaError type using factory functions (local, fromApiResponse) and a MfaResult<TData> discriminated union. This eliminates redundant fallback logic, removes manual object construction at dispatch sites, and ensures every error carries a descriptive message.
addMFABreadcrumb now accepts MfaError directly, eliminating manual destructuring at call sites. Removed unused payload property from ErrorState since it was never read anywhere.
mapAuthTypeNumber returning undefined is a local validation failure (client cannot interpret HSM sign result), not a server 4xx response. Add LOCAL_ERRORS.HSM.UNRECOGNIZED_AUTH_TYPE and use it instead of CLIENT_ERRORS.BAD_REQUEST.
TRANSACTION_DENIED represents a successful deny API call (HTTP 200), not a server 4xx error. Placing it under CLIENT_ERRORS contradicted the naming convention. New REASON.FLOW_OUTCOMES category holds results that are completed flow outcomes rather than errors.
Extract resolveReason to collapse 4 return paths into one. Rename helpers for clarity: - parseHttpRequest → parseHttpResponse (parses response, not request) - getHttpStatusCategory → categorizeHttpStatus - getCategoryFallbackReason → getFallbackReason (accepts undefined, eliminating dead code branch) - httpStatusCategoryIsDefined → hasHttpStatusCategory - findMessageInSource → findReasonByBackendMessage - ParseHTTPSource → ResponseToReasonMap
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Tests were asserting flat `{ success: false, reason }` but the actual
MfaResult failure type nests error details under `error` property:
`{ success: false, error: { reason, message } }`.
Also corrected the mapAuthTypeNumber failure test to expect
UNRECOGNIZED_AUTH_TYPE (matching production code) instead of BAD_REQUEST.
String(httpStatusCode).startsWith('2') incorrectly returns true for
values like 2, 20, or 2000. Use proper HTTP range check (200-299).
Instead of hardcoding individual reason constants, use Object.values() so new SERVER_ERRORS entries are automatically picked up.
TRANSACTION_DENIED is a flow outcome, not a failure. Let it fall through to 'unclassified' severity rather than mislabel it as routine.
…tion map Complete coverage of production-relevant DOMException names per WebAuthn spec. Each gets a dedicated REASON for observability. TimeoutError is classified as routine; DataError and UnknownError as anomalous. Also reverts accidental ROUTINE_FAILURES rename from previous commit.
The authorize error handler only checked KEY_ACCESS_FAILED and KEY_NOT_FOUND, but the hooks now emit NO_MATCHING_LOCAL_CREDENTIAL when the local credential is absent from the challenge's allowCredentials list. Without this condition, users see a terminal failure screen instead of automatic re-registration recovery.
…lse error telemetry TRANSACTION_DENIED was moved to FLOW_OUTCOMES but not included in any classification set, causing trackMFAFlowOutcome to log it as unclassified at error level. Add ALTERNATIVE_OUTCOMES set for valid non-success terminal states and classify them at info level.
|
@codex review |
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments! I'll leave the rest up to C+, Codex and the internal team.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
dariusz-biela
changed the title
No code path ever dispatched this reason — it only existed in its definition and the ANOMALOUS_FAILURES classification set.
melvin-bot
Bot
requested review from
cretadn22 and
heyjennahay
and removed request for
a team and
heyjennahay
|
@cretadn22 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const result = await state.scenario.onCancel(state.payload); | ||
| addMFABreadcrumb('Flow cancelled with onCancel', {reason: result.reason}, 'warning'); | ||
| dispatch({type: 'SET_ERROR', payload: {reason: result.reason, payload: result.payload}}); | ||
| dispatch({type: 'SET_ERROR', payload: {reason: result.reason, message: undefined}}); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
Every other SET_ERROR dispatch in this file creates error payloads using either MfaError.local(...), MfaError.fromApiResponse(...), or passes through an existing .error object from a result. This single instance constructs a raw object literal {reason: result.reason, message: undefined} instead of using the MfaError.local() factory, breaking the consistent pattern.
Use the factory function to be consistent with the rest of the file:
dispatch({type: 'SET_ERROR', payload: MfaError.local(result.reason, undefined)});Reviewed at: aa7c675 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Here we have an error passed from a callback scenario, and I'm not sure if it's a local error or one from the API.
Additionally, we had an issue where the payload was being written to the error object, which was unsafe because it could have contained sensitive
There was a problem hiding this comment.
Or actually, I'll tweak it a bit so that we also include the httpStatus and message here.
dariusz-biela
force-pushed
the
dariusz-biela/refactor/3ds/clean-up-reasons
branch
from
2492f94 to
05820ab
Compare
chuckdries
requested review from
chuckdries and
rafecolton
and removed request for
cretadn22
There was a problem hiding this comment.
I'm a little unclear on the semantic meaning of the MfaResult type. We return it in src/libs/actions/MultifactorAuthentication/index.ts from requestAuthorizationChallenge and requestRegistrationChallenge. But, none of the actions which actually are MFA calls - actions which take a signedChallenge parameter - return it. Is that intentional?
| * `fromApiResponse` — errors derived from an API response; defaults reason to UNHANDLED_API_RESPONSE when undefined. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-redeclare -- Intentional: companion object shares name with MfaError type (type+value merge pattern) | ||
| const MfaError = { |
There was a problem hiding this comment.
| const MfaError = { | |
| const MFAError = { |
There was a problem hiding this comment.
Please update in all places
There was a problem hiding this comment.
I prefer to use another name and remove the disable lint. E.g. MFAErrorHandler
| }; | ||
|
|
||
| /** | ||
| * Factory functions for creating MfaError instances. |
There was a problem hiding this comment.
| * Factory functions for creating MfaError instances. | |
| * Factory functions for creating MFAError instances. |
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-redeclare -- Intentional: companion object shares name with MfaError type (type+value merge pattern) | ||
| const MfaError = { | ||
| local(reason: MultifactorAuthenticationReason, message: string | undefined): MfaError { |
There was a problem hiding this comment.
handle
| local(reason: MultifactorAuthenticationReason, message: string | undefined): MfaError { | |
| handleLocalError(reason: MultifactorAuthenticationReason, message: string | undefined): MfaError { |
| return {reason, message}; | ||
| }, | ||
|
|
||
| fromApiResponse(httpStatusCode: number | undefined, reason: MultifactorAuthenticationReason | undefined, message?: string): MfaError { |
There was a problem hiding this comment.
| fromApiResponse(httpStatusCode: number | undefined, reason: MultifactorAuthenticationReason | undefined, message?: string): MfaError { | |
| hanldeAPIError(httpStatusCode: number | undefined, reason: MultifactorAuthenticationReason | undefined, message: string | undefined): MfaError { |
| // eslint-disable-next-line @typescript-eslint/no-empty-object-type -- {} means "no additional data fields" as default generic parameter | ||
| type MfaResult<TData = {}> = ({success: true} & TData) | {success: false; error: MfaError}; |
There was a problem hiding this comment.
| // eslint-disable-next-line @typescript-eslint/no-empty-object-type -- {} means "no additional data fields" as default generic parameter | |
| type MfaResult<TData = {}> = ({success: true} & TData) | {success: false; error: MfaError}; | |
| type MfaResult<TData = Record<string, never>> = ({success: true} & TData) | {success: false; error: MfaError}; |
There was a problem hiding this comment.
I should limit use lint disableWe should limit the use of eslint-disable.
| let reason = mapLibraryErrorToReason(error); | ||
| if (reason === undefined) { | ||
| reason = VALUES.REASON.HSM.KEY_CREATION_FAILED; | ||
| reason = VALUES.REASON.LOCAL_ERRORS.HSM.KEY_CREATION_FAILED; |
There was a problem hiding this comment.
I think we should use VALUES.REASON.LOCAL_ERRORS.HSM.UNRECOGNIZED here for consistency. This way, when we see this error, it means that it hasn’t been handled by mapLibraryErrorToReason. It will be easier to debug
Wdyt?
There was a problem hiding this comment.
Alternatively, what do you think about using UNRECOGNIZED as a prefix, like UNRECOGNIZED_KEY_CREATION_FAILED?
Use UNRECOGNIZED to indicate that HSM doesn’t handle this case.
There was a problem hiding this comment.
If we use VALUES.REASON.LOCAL_ERRORS.HSM.UNRECOGNIZED as a fallback, we can move this fallback handler into mapLibraryErrorToReason function as we did with decodeWebAuthnError
| [CONST.MULTIFACTOR_AUTHENTICATION.REASON.CLIENT_ERRORS.ALREADY_DENIED_APPROVE_ATTEMPTED]: <AlreadyReviewedFailureScreen />, | ||
| [CONST.MULTIFACTOR_AUTHENTICATION.REASON.CLIENT_ERRORS.ALREADY_REVIEWED]: <AlreadyReviewedFailureScreen />, | ||
|
|
||
| // Client-side errors (not returned by the backend API) |
There was a problem hiding this comment.
| // Client-side errors (not returned by the backend API) | |
| // Local errors (not returned by the backend API) |
| httpStatusCode?: number; | ||
| message?: string; | ||
| }; | ||
| type ErrorState = MfaError; |
There was a problem hiding this comment.
Please use MfaError directly and remove ErrorState
|
@dariusz-biela For future PRs, if possible, could we avoid combining too many changes into one? It would be better to split them into smaller PRs |
Actually, it did cross my mind, but at that point I figured it might be too much for this PR. But now that you mention it, I’ll go ahead and do it. But I’m warning you, this is the last feature, because Dylan is going to kill me. 😄
Sorry This PR was supposed to be half as big, but Jakub noticed some issues and I wanted to fix them properly, so it ended up getting a bit bigger. |
4 participants
Explanation of Change
Refactors MFA
REASONconstants to improve clarity, reduce dead code, and make error origin obvious from value strings alone.Categories & values:
BACKEND/GENERIC/WEBAUTHN/HSM) → HTTP-status-based (SERVER_ERRORS/CLIENT_ERRORS/LOCAL_ERRORSwithWEBAUTHN/HSMnested underLOCAL_ERRORS)"Client: Registration required","Local WebAuthn: Operation not allowed") so error origin is identifiable in logs without looking up constant nameTRANSACTION_DENIEDasalternative_outcometo prevent false error telemetry — it represents a successful deny API call (HTTP 200), not a client errorREQUESTED_TRANSACTION_UNAVAILABLEType system & error handling:
MfaErrordiscriminated union andMfaResult<TData>type (shared/MfaResult.ts) with factory functions — replaces scatteredreason/httpStatusCode/messagepropertiesaddMFABreadcrumbnow acceptsMfaErrordirectly; remove deadErrorState.payloadDeepLeafValuesutility type for 3-levelREASONnestingDecoupling & naming:
DOMException.namestrings viaDOM_EXCEPTION_NAME_MAPUNHANDLED→UNRECOGNIZED,UNHANDLED_ERROR→UNHANDLED_EXCEPTION,isServerOrLocalError→shouldShowServerFailureScreen, etc.parseHttpResponsehelpers: clearer names, extractresolveReason, case-insensitive message matchingBug fixes:
NO_MATCHING_LOCAL_CREDENTIALreasonsisHttpSuccessinstead of string prefixSIGNATURE_MISSING,UNHANDLED_EXCEPTIONasANOMALOUSfor error-level loggingTests & misc: updated unit tests for renamed values + new branch coverage,
cspell.jsonadditions.Fixed Issues
$ #83036
PROPOSAL:
Tests
Biometrics registration (iOS / Android native):
registeredlabel is now displayedBiometrics authorization (iOS / Android native):
Passkeys registration (Web / mWeb):
registeredlabel is now displayedPasskeys authorization (Web / mWeb):
21. Repeat step 15
22. Verify that the validate code step is no longer required
23. Authenticate using the passkey prompt
24. Verify that the Authentication was successful
Failure scenarios:
25. Cancel the passkey/biometrics prompt during registration (step 6 or 18) — verify the Authentication failed
26. Cancel the passkey/biometrics prompt during authorization (step 11 or 23) — verify the Authentication failed
27. Exit the flow using the back button or by clicking the overlay — verify a confirmation modal is displayed
28. Enter a wrong validate code during registration — verify an error text is displayed and the magic code input is shown again to allow re-entry
Offline tests
N/A,
D - Full Page Blocking UI Patternfor this project.QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
iOS: Native
Screen.Recording.2026-04-21.at.17.17.19.mp4
MacOS: Chrome / Safari
Screen.Recording.2026-04-21.at.16.57.49.mp4