[HOLD] test PR for 79473 #81619
[HOLD] test PR for 79473 #81619
Conversation
# Conflicts: # src/components/TestToolMenu.tsx
Replace complex step-based MFA implementation with a simplified state-driven approach: - Add StateContext for centralized state management with individual setters - Replace Context.tsx with useEffect-based process flow - Consolidate useNativeBiometrics directory into a single hook file - Add processRegistration and processScenario helpers for backend calls - Update page imports to use new exports from index.ts - Remove outdated test files for old useNativeBiometrics API The new implementation: - Uses React state + useEffect instead of status/step objects - Separates concerns: StateContext (state), Context (logic), useNativeBiometrics (native ops) - Always fetches private key from SecureStore (no chainedPrivateKey) - Uses callback-based register/authorize in useNativeBiometrics hook
… flow" This reverts commit 3ca4f3c.
Adds a Guards context that prevents direct URL access to MFA screens that should only be accessible during an active authentication flow. - Create Guards.tsx with canAccessMagicCode, canAccessPrompt, canAccessOutcome - Wrap MFA pages with FullPageNotFoundView showing when guards fail - Guards check: scenario is set, flow not complete, no error state
- Remove refresh function from useNativeBiometrics hook - Get isAnyDeviceRegistered from ONYXKEYS.ACCOUNT.multifactorAuthenticationPublicKeyIDs - Remove unnecessary key reset logic from MFA context - Update tests to reflect new Onyx-based state management
- Create Provider.tsx for provider hierarchy composition - Rename MultifactorAuthenticationProviderInner to MultifactorAuthenticationContextProvider - Rename MultifactorAuthenticationProvider to MultifactorAuthenticationContextProviders - Update index.ts and usages in RightModalNavigator
- Add publicKey to multifactorAuthenticationPublicKeyIDs immediately on registration - Use makeRequestWithSideEffects onyxData for automatic rollback on failure - Add currentPublicKeyIDs and registeredPublicKeyIDs to track existing keys
…ys during registration
This reverts commit 9f11501.
…he soft prompt since they last reinstalled the app
melvin-bot
bot
requested review from
JmillsExpensify and
nkuoch
and removed request for
a team
|
🚧 @chuckdries has triggered a test Expensify/App build. You can view the workflow run here. |
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index 14357132..91482490 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -736,8 +736,8 @@ const translations: TranslationDeepObject<typeof en> = {
looksLikeYouRanOutOfTime: 'Anscheinend ist deine Zeit abgelaufen! Bitte versuche es noch einmal beim Händler.',
youRanOutOfTime: 'Die Zeit ist abgelaufen',
letsVerifyItsYou: 'Lass uns bestätigen, dass du es bist',
- nowLetsAuthenticateYou: 'Lassen Sie uns Sie jetzt authentifizieren …',
- letsAuthenticateYou: 'Lass uns dich authentifizieren …',
+ nowLetsAuthenticateYou: 'Jetzt authentifizieren wir dich ...',
+ letsAuthenticateYou: 'Wir authentifizieren dich …',
verifyYourself: {
biometrics: 'Bestätige dich mit deinem Gesicht oder Fingerabdruck',
},
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index 25b81f4e..0cf28144 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -740,7 +740,7 @@ const translations: TranslationDeepObject<typeof en> = {
youRanOutOfTime: 'Vous n’avez plus de temps',
letsVerifyItsYou: 'Vérifions qu’il s’agit bien de vous',
nowLetsAuthenticateYou: 'Maintenant, procédons à votre authentification…',
- letsAuthenticateYou: 'Authentifions votre identité…',
+ letsAuthenticateYou: 'Authentification en cours...',
verifyYourself: {
biometrics: 'Vérifiez votre identité avec votre visage ou votre empreinte digitale',
},
@@ -761,7 +761,7 @@ const translations: TranslationDeepObject<typeof en> = {
},
unsupportedDevice: {
unsupportedDevice: 'Appareil non pris en charge',
- pleaseDownloadMobileApp: `<centered-text><muted-text> Cette action n'est pas prise en charge sur votre appareil. Veuillez télécharger l'application Expensify depuis l'<a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> ou le <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a> et réessayer.</muted-text></centered-text>`,
+ pleaseDownloadMobileApp: `<centered-text><muted-text> Cette action n’est pas prise en charge sur votre appareil. Veuillez télécharger l’application Expensify depuis l’<a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> ou le <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a>, puis réessayez.</muted-text></centered-text>`,
},
},
validateCodeModal: {
diff --git a/src/languages/it.ts b/src/languages/it.ts
index e60ae65e..572f9dcd 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -736,8 +736,8 @@ const translations: TranslationDeepObject<typeof en> = {
looksLikeYouRanOutOfTime: 'Sembra che il tempo sia scaduto! Riprova presso l’esercente.',
youRanOutOfTime: 'Il tempo è scaduto',
letsVerifyItsYou: 'Verifichiamo che sia tu',
- nowLetsAuthenticateYou: 'Ora procediamo con l’autenticazione...',
- letsAuthenticateYou: 'Autentichiamo la tua identità...',
+ nowLetsAuthenticateYou: 'Ora procediamo con l’autenticazione…',
+ letsAuthenticateYou: 'Autentichiamoti...',
verifyYourself: {
biometrics: 'Verificati con il volto o l’impronta digitale',
},
@@ -758,7 +758,7 @@ const translations: TranslationDeepObject<typeof en> = {
},
unsupportedDevice: {
unsupportedDevice: 'Dispositivo non supportato',
- pleaseDownloadMobileApp: `<centered-text><muted-text> Questa azione non è supportata sul tuo dispositivo. Scarica l'app Expensify dall'<a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> o da <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a> e riprova.</muted-text></centered-text>`,
+ pleaseDownloadMobileApp: `<centered-text><muted-text> Questa azione non è supportata sul tuo dispositivo. Scarica l'app Expensify dall'<a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> o dal <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a> e riprova.</muted-text></centered-text>`,
},
},
validateCodeModal: {
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index 0543ec44..34e686c4 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -736,7 +736,7 @@ const translations: TranslationDeepObject<typeof en> = {
looksLikeYouRanOutOfTime: '時間切れになったようです。加盟店で再度お試しください。',
youRanOutOfTime: '時間切れです',
letsVerifyItsYou: 'ご本人確認を行いましょう',
- nowLetsAuthenticateYou: 'では、ご本人確認を行いましょう…',
+ nowLetsAuthenticateYou: 'では、認証を行いましょう…',
letsAuthenticateYou: '認証を行っています…',
verifyYourself: {
biometrics: '顔または指紋で本人確認を行ってください',
@@ -755,8 +755,8 @@ const translations: TranslationDeepObject<typeof en> = {
error: 'リクエストに失敗しました。後でもう一度お試しください。',
},
unsupportedDevice: {
- unsupportedDevice: '未対応のデバイス',
- pleaseDownloadMobileApp: `<centered-text><muted-text> この操作はお使いのデバイスではサポートされていません。<a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> または <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Playストア</a> からExpensifyアプリをダウンロードして、もう一度お試しください。</muted-text></centered-text>`,
+ unsupportedDevice: 'サポートされていないデバイス',
+ pleaseDownloadMobileApp: `<centered-text><muted-text> この操作はお使いのデバイスではサポートされていません。<a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> または <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play ストア</a> から Expensify アプリをダウンロードして、もう一度お試しください。</muted-text></centered-text>`,
},
},
validateCodeModal: {
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index 8ba638a7..79935baf 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -736,7 +736,7 @@ const translations: TranslationDeepObject<typeof en> = {
looksLikeYouRanOutOfTime: 'Het lijkt erop dat je tijd op is! Probeer het alsjeblieft opnieuw bij de handelaar.',
youRanOutOfTime: 'Je tijd is op',
letsVerifyItsYou: 'Laten we controleren of jij het bent',
- nowLetsAuthenticateYou: 'Laten we je nu verifiëren...',
+ nowLetsAuthenticateYou: 'Nu gaan we je verifiëren...',
letsAuthenticateYou: 'We gaan je authenticeren...',
verifyYourself: {
biometrics: 'Verifieer jezelf met je gezicht of vingerafdruk',
@@ -757,7 +757,7 @@ const translations: TranslationDeepObject<typeof en> = {
},
unsupportedDevice: {
unsupportedDevice: 'Niet-ondersteund apparaat',
- pleaseDownloadMobileApp: `<centered-text><muted-text> Deze actie wordt niet ondersteund op jouw apparaat. Download de Expensify-app uit de <a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> of de <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a> en probeer het opnieuw.</muted-text></centered-text>`,
+ pleaseDownloadMobileApp: `<centered-text><muted-text> Deze actie wordt niet ondersteund op je apparaat. Download de Expensify-app vanuit de <a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> of de <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a> en probeer het opnieuw.</muted-text></centered-text>`,
},
},
validateCodeModal: {
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index 175a45bc..b8b74404 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -736,7 +736,7 @@ const translations: TranslationDeepObject<typeof en> = {
looksLikeYouRanOutOfTime: 'Wygląda na to, że skończył ci się czas! Spróbuj ponownie u sprzedawcy.',
youRanOutOfTime: 'Czas się skończył',
letsVerifyItsYou: 'Zweryfikujmy, czy to na pewno Ty',
- nowLetsAuthenticateYou: 'Teraz Cię uwierzytelnimy…',
+ nowLetsAuthenticateYou: 'Teraz Cię uwierzytelnimy...',
letsAuthenticateYou: 'Uwierzytelnijmy Cię…',
verifyYourself: {
biometrics: 'Zweryfikuj się za pomocą twarzy lub odcisku palca',
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index dd6e404e..2b597237 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -756,7 +756,7 @@ const translations: TranslationDeepObject<typeof en> = {
},
unsupportedDevice: {
unsupportedDevice: 'Dispositivo não compatível',
- pleaseDownloadMobileApp: `<centered-text><muted-text> Esta ação não é compatível com seu dispositivo. Baixe o app do Expensify na <a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> ou na <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a> e tente novamente.</muted-text></centered-text>`,
+ pleaseDownloadMobileApp: `<centered-text><muted-text> Esta ação não é compatível com o seu dispositivo. Baixe o app Expensify na <a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> ou na <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play Store</a> e tente novamente.</muted-text></centered-text>`,
},
},
validateCodeModal: {
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index b23ebb15..ef3e8e0d 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -720,7 +720,7 @@ const translations: TranslationDeepObject<typeof en> = {
areYouSureToReject: '您确定吗？如果您关闭此界面，此次身份验证尝试将被拒绝。',
rejectAuthentication: '拒绝认证',
test: '测试',
- biometricsAuthentication: '生物识别认证',
+ biometricsAuthentication: '生物识别身份验证',
},
pleaseEnableInSystemSettings: {
start: '请在您的设备中启用面部/指纹验证或设置设备密码',
@@ -731,8 +731,8 @@ const translations: TranslationDeepObject<typeof en> = {
looksLikeYouRanOutOfTime: '看起来您的操作已超时！请在商家处重试。',
youRanOutOfTime: '你的时间用完了',
letsVerifyItsYou: '让我们验证一下您的身份',
- nowLetsAuthenticateYou: '现在，让我们为你进行身份验证…',
- letsAuthenticateYou: '正在验证您的身份…',
+ nowLetsAuthenticateYou: '现在，让我们验证您的身份…',
+ letsAuthenticateYou: '正在为你进行身份验证…',
verifyYourself: {
biometrics: '使用面部或指纹验证您的身份',
},
@@ -751,7 +751,7 @@ const translations: TranslationDeepObject<typeof en> = {
},
unsupportedDevice: {
unsupportedDevice: '不支持的设备',
- pleaseDownloadMobileApp: `<centered-text><muted-text> 您的设备不支持此操作。请从<a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a>或<a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play 商店</a>下载 Expensify 应用，然后重试。</muted-text></centered-text>`,
+ pleaseDownloadMobileApp: `<centered-text><muted-text> 您的设备不支持此操作。请从 <a href="${CONST.APP_DOWNLOAD_LINKS.IOS}">App Store</a> 或 <a href="${CONST.APP_DOWNLOAD_LINKS.ANDROID}">Google Play 商店</a> 下载 Expensify 应用后重试。</muted-text></centered-text>`,
},
},
validateCodeModal: {
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Navigation.navigate(ROUTES.MULTIFACTOR_AUTHENTICATION_PROMPT.getRoute(CONST.MULTIFACTOR_AUTHENTICATION.PROMPT.ENABLE_BIOMETRICS), {forceReplace: true}); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-12 (docs)
The Onyx.connectWithoutView connection is never cleaned up, creating a memory leak. This connection persists for the lifetime of the module and continues to receive updates even after the component unmounts.
Fix: Move this connection inside the component and clean it up in useEffect:
function MultifactorAuthenticationContextProvider({children}: MultifactorAuthenticationContextProviderProps) {
const [deviceBiometricsState, setDeviceBiometricsState] = useState<OnyxEntry<DeviceBiometrics>>();
useEffect(() => {
const connection = Onyx.connect({
key: ONYXKEYS.DEVICE_BIOMETRICS,
callback: setDeviceBiometricsState,
});
return () => {
Onyx.disconnect(connection);
};
}, []);
// ... rest of component
}
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const {challenge, reason: challengeReason} = await requestAuthorizationChallenge(); | ||
|
|
||
| if (!challenge) { | ||
| dispatch({type: 'SET_ERROR', payload: {reason: challengeReason}}); |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
Multiple async operations in the process() function lack error handling. If any of these operations throw (network errors, promise rejections), the error will be unhandled and the MFA flow will be stuck.
Critical operations without error handling:
biometrics.areLocalCredentialsKnownToServer()(line 287)requestRegistrationChallenge(validateCode)(line 299)requestAuthorizationChallenge()(line 377)biometrics.register()callback (line 328)biometrics.authorize()callback (line 395)
Fix: Wrap the entire process() function body in try-catch:
const process = useCallback(async () => {
try {
const { error, continuableError, scenario, ... } = state;
// ... existing logic ...
} catch (error) {
dispatch({
type: 'SET_ERROR',
payload: {
reason: CONST.MULTIFACTOR_AUTHENTICATION.REASON.GENERIC.UNKNOWN_ERROR,
message: error instanceof Error ? error.message : 'Unknown error'
},
});
}
}, [biometrics, dispatch, isOffline, state, isWeb]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| if (continuableError.reason !== VALUES.REASON.BACKEND.INVALID_VALIDATE_CODE) { | ||
| // Cannot handle this error - convert to regular error which will stop the flow | ||
| dispatch({type: 'SET_ERROR', payload: {reason: continuableError.reason, message: continuableError.message}}); | ||
| } |
There was a problem hiding this comment.
❌ PERF-8 (docs)
This useEffect converts continuable errors to regular errors based on the error reason. However, this logic should run immediately when the error is set, not in a separate effect.
Fix: Move this validation into the State reducer where the continuable error is set:
// In State.tsx reducer
case 'SET_ERROR': {
if (action.payload === undefined) {
return {...state, error: undefined, continuableError: undefined};
}
// Determine if error is continuable based on its reason
const isContinuableError = action.payload.reason === CONST.MULTIFACTOR_AUTHENTICATION.REASON.BACKEND.INVALID_VALIDATE_CODE;
return {
...state,
error: isContinuableError ? undefined : action.payload,
continuableError: isContinuableError ? action.payload : undefined,
};
}This eliminates the effect and makes error handling immediate and predictable.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| useEffect(() => { | ||
| // Show a short loading state so the RHP transition feels smooth, then move to the magic code flow | ||
| const timeoutId = setTimeout(() => Navigation.navigate(ROUTES.MULTIFACTOR_AUTHENTICATION_MAGIC_CODE), LOADING_DELAY_MS); | ||
| if (isOffline) { |
There was a problem hiding this comment.
❌ PERF-8 (docs)
The useEffect calls executeScenario() on mount/when coming back online. This should be handled in the component that navigates to this page, not in an effect.
Issue: The useEffect has an incomplete dependency array (missing executeScenario), which is acknowledged with an eslint-disable comment. This creates a stale closure where executeScenario could be outdated.
Fix: Remove the effect and call executeScenario before navigating to this page:
// In the component that opens BiometricsTestPage:
const handleOpenBiometricsTest = async () => {
await executeScenario(CONST.MULTIFACTOR_AUTHENTICATION.SCENARIO.BIOMETRICS_TEST);
Navigation.navigate(ROUTES.BIOMETRICS_TEST_PAGE);
};
// BiometricsTestPage becomes a simple loading/offline indicator
function MultifactorAuthenticationBiometricsTestPage() {
const {isOffline} = useNetwork();
return (
<ScreenWrapper>
{isOffline ? <OfflineMessage /> : <FullScreenLoadingIndicator />}
</ScreenWrapper>
);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| ): Promise<ProcessResult> { | ||
| const currentScenario = MULTIFACTOR_AUTHENTICATION_SCENARIO_CONFIG[scenario] as MultifactorAuthenticationScenarioConfig; | ||
|
|
||
| if (!params.signedChallenge) { |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The -8 is a COSE algorithm identifier for EdDSA (Ed25519). While documented in the type comment, it should be defined as a named constant for clarity and maintainability.
Fix: Define the constant in CONST or a dedicated file:
// In CONST.ts or a crypto constants file
const COSE_ALGORITHM_EDDSA = -8; // EdDSA with Ed25519 curve per RFC 8152
// Then use it:
biometric: {
publicKey,
algorithm: COSE_ALGORITHM_EDDSA,
}This makes the code self-documenting and easier to maintain if crypto standards change.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
| }; | ||
|
|
||
| const resendValidationCode = () => { |
There was a problem hiding this comment.
❌ PERF-13 (docs)
Two separate dispatch() calls in sequence will trigger two separate state updates and potentially two re-renders. While React may batch these in some cases, it's better to handle both state changes in a single action for guaranteed atomicity.
Fix: Create a compound action or handle both state clears in the reducer:
// Option 1: Single compound action
dispatch({type: 'CLEAR_VALIDATE_CODE_AND_ERROR'});
// Option 2: Handle both in existing CLEAR_CONTINUABLE_ERROR action
case 'CLEAR_CONTINUABLE_ERROR':
return {...state, continuableError: undefined, validateCode: undefined};This guarantees a single state update and re-render, and ensures both changes happen atomically.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dd4f6ea05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| }, [ | ||
| // Error states - need to handle failures and navigate to outcome screens |
There was a problem hiding this comment.
The state machine exits early when isOffline is true, but the effect that drives process() does not depend on network state, so coming back online does not trigger another run. If the user reaches any step while offline (for example right after entering a valid code), the flow can remain stuck because no tracked state field changes and re-submitting the same code also won’t change state. Include connectivity in the trigger path (or explicitly re-dispatch) so the flow resumes automatically after reconnect.
Useful? React with 👍 / 👎.
| if (!deviceBiometricsState?.hasAcceptedSoftPrompt) { | ||
| Navigation.navigate(ROUTES.MULTIFACTOR_AUTHENTICATION_PROMPT.getRoute(CONST.MULTIFACTOR_AUTHENTICATION.PROMPT.ENABLE_BIOMETRICS), {forceReplace: true}); | ||
| return; |
There was a problem hiding this comment.
This check reads deviceBiometricsState from a module-level connectWithoutView variable, but that value is not part of React state, so updates to DEVICE_BIOMETRICS do not trigger process() to run again. If the first run happens before the Onyx callback hydrates, returning users can be sent to the prompt path and remain there (showing the loading state) with no state transition to continue. Read this value via useOnyx/state in the provider so prompt gating is reactive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentional
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
6 participants
This PR is a copy of #79473 on our remote so I can write to it. Please direct all feedback there.