Skip to content

Commit adcb2c2

Browse files
authored
chore: Update reauthenticate method with proper error (#24213)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> Update `reauthenticate` method to throw more verbose errors. For example, when password is not set using biometrics. This also fixes an issue where reset password screen would show the incorrect error message when attempting to use biometrics. The correct behavior is that it should not show incorrect message when password is not stored using biometrics. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: MetaMask/MetaMask-planning#6008 ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> https://github.com/user-attachments/assets/2681f8f9-b8d5-4aa7-ac8c-5daf47972520 ### **After** <!-- [screenshots/recordings] --> https://github.com/user-attachments/assets/19538c61-66ba-4642-be0e-6dbe5675c8fa ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Reworks reauthentication to emit explicit PASSWORD_NOT_SET_WITH_BIOMETRICS/BIOMETRIC_ERROR and updates UI/tests to handle no-credentials flows without showing incorrect-password warnings. > > - **Authentication**: > - Refactor `Authentication.reauthenticate` to use stored keychain credentials; throw `PASSWORD_NOT_SET_WITH_BIOMETRICS` when absent and wrap keychain failures as `BIOMETRIC_ERROR`. > - Update `updateAuthPreference` to convert `PASSWORD_NOT_SET_WITH_BIOMETRICS` into `AUTHENTICATION_APP_TRIGGERED_AUTH_NO_CREDENTIALS` and keep lock-time handling. > - Add `ReauthenticateErrorType` values (`PASSWORD_NOT_SET_WITH_BIOMETRICS`, `BIOMETRIC_ERROR`). > - **UI**: > - `Views/ResetPassword`: suppress incorrect-password warning when `PASSWORD_NOT_SET_WITH_BIOMETRICS`; always resolve loading state in `finally`. > - `Views/RevealPrivateCredential`: suppress incorrect-password warning on `PASSWORD_NOT_SET_WITH_BIOMETRICS`. > - **Tests**: > - Update tests to expect `PASSWORD_NOT_SET_WITH_BIOMETRICS`; remove legacy `BIOMETRIC_NOT_ENABLED` expectations and related storage checks; add mapping test to `AUTHENTICATION_APP_TRIGGERED_AUTH_NO_CREDENTIALS`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8a8b246. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent e03727e commit adcb2c2

5 files changed

Lines changed: 43 additions & 37 deletions

File tree

app/components/Views/ResetPassword/index.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ import {
7979
AuthConnection,
8080
SeedlessOnboardingControllerErrorMessage,
8181
} from '@metamask/seedless-onboarding-controller';
82+
import { ReauthenticateErrorType } from '../../../core/Authentication/types';
8283

8384
// Constants
8485
const PASSCODE_NOT_SET_ERROR = 'Error: Passcode not set.';
@@ -656,9 +657,23 @@ class ResetPassword extends PureComponent {
656657
view: RESET_PASSWORD,
657658
});
658659
} catch (e) {
660+
// Don't show warning if password is not set with biometrics
661+
if (
662+
e.message.includes(
663+
ReauthenticateErrorType.PASSWORD_NOT_SET_WITH_BIOMETRICS,
664+
)
665+
) {
666+
return;
667+
}
668+
669+
// Show warning if password is incorrect
659670
const msg = strings('reveal_credential.warning_incorrect_password');
660671
this.setState({
661672
warningIncorrectPassword: msg,
673+
});
674+
} finally {
675+
// Resolve UI loading state
676+
this.setState({
662677
ready: true,
663678
});
664679
}

app/components/Views/RevealPrivateCredential/RevealPrivateCredential.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,8 +206,12 @@ const RevealPrivateCredential = ({
206206
// TODO: Replace "any" with type
207207
// eslint-disable-next-line @typescript-eslint/no-explicit-any
208208
} catch (e: any) {
209-
// we should not show the error message if the error is because biometric is not enabled
210-
if (e.message.includes(ReauthenticateErrorType.BIOMETRIC_NOT_ENABLED)) {
209+
// we should not show the error message if the error is because password is not set with biometrics
210+
if (
211+
e.message.includes(
212+
ReauthenticateErrorType.PASSWORD_NOT_SET_WITH_BIOMETRICS,
213+
)
214+
) {
211215
return;
212216
}
213217
let msg = strings('reveal_credential.warning_incorrect_password');

app/core/Authentication/Authentication.test.ts

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3984,10 +3984,10 @@ describe('Authentication', () => {
39843984
alertSpy.mockRestore();
39853985
});
39863986

3987-
it('converts BIOMETRIC_NOT_ENABLED error to AUTHENTICATION_APP_TRIGGERED_AUTH_NO_CREDENTIALS', async () => {
3988-
// Mock reauthenticate to throw BIOMETRIC_NOT_ENABLED error
3987+
it('converts PASSWORD_NOT_SET_WITH_BIOMETRICS error to AUTHENTICATION_APP_TRIGGERED_AUTH_NO_CREDENTIALS', async () => {
3988+
// Mock reauthenticate to throw PASSWORD_NOT_SET_WITH_BIOMETRICS error
39893989
const biometricNotEnabledError = new Error(
3990-
`${ReauthenticateErrorType.BIOMETRIC_NOT_ENABLED}: Biometric is not enabled`,
3990+
`${ReauthenticateErrorType.PASSWORD_NOT_SET_WITH_BIOMETRICS}: No password stored with biometrics in keychain.`,
39913991
);
39923992
jest
39933993
.spyOn(Authentication, 'reauthenticate')
@@ -4230,39 +4230,18 @@ describe('Authentication', () => {
42304230
expect(result.password).toBe('test-password');
42314231
});
42324232

4233-
it('throws PASSWORD_REQUIRED error when no biometric credentials are available', async () => {
4233+
it('throws PASSWORD_NOT_SET_WITH_BIOMETRICS error when password is not set using biometric credentials', async () => {
42344234
const verifyPasswordSpy = Engine.context.KeyringController.verifyPassword;
4235-
const getItemSpy = jest
4236-
.spyOn(StorageWrapper, 'getItem')
4237-
.mockResolvedValueOnce(null as never);
42384235
const getPasswordSpy = jest
42394236
.spyOn(Authentication, 'getPassword')
42404237
.mockResolvedValueOnce(null);
42414238

42424239
await expect(Authentication.reauthenticate()).rejects.toThrow(
4243-
ReauthenticateErrorType.BIOMETRIC_NOT_ENABLED,
4240+
ReauthenticateErrorType.PASSWORD_NOT_SET_WITH_BIOMETRICS,
42444241
);
42454242

4246-
expect(getItemSpy).toHaveBeenCalledWith(BIOMETRY_CHOICE);
4247-
expect(getPasswordSpy).not.toHaveBeenCalled();
4248-
expect(verifyPasswordSpy).not.toHaveBeenCalled();
4249-
});
4250-
4251-
it('uses stored biometric password when no password is provided', async () => {
4252-
const verifyPasswordSpy = Engine.context.KeyringController.verifyPassword;
4253-
await StorageWrapper.setItem(BIOMETRY_CHOICE, TRUE);
4254-
const getPasswordSpy = jest
4255-
.spyOn(Authentication, 'getPassword')
4256-
.mockResolvedValue({
4257-
password: 'stored-password',
4258-
} as unknown as Keychain.UserCredentials);
4259-
4260-
const result = await Authentication.reauthenticate();
4261-
4262-
expect(StorageWrapper.getItem).toHaveBeenCalledWith(BIOMETRY_CHOICE);
42634243
expect(getPasswordSpy).toHaveBeenCalled();
4264-
expect(verifyPasswordSpy).toHaveBeenCalledWith('stored-password');
4265-
expect(result.password).toBe('stored-password');
4244+
expect(verifyPasswordSpy).not.toHaveBeenCalled();
42664245
});
42674246

42684247
it('propagates error when password verification fails', async () => {

app/core/Authentication/Authentication.ts

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import {
88
SEED_PHRASE_HINTS,
99
OPTIN_META_METRICS_UI_SEEN,
1010
PREVIOUS_AUTH_TYPE_BEFORE_REMEMBER_ME,
11-
BIOMETRY_CHOICE,
1211
} from '../../constants/storage';
1312
import {
1413
authSuccess,
@@ -1521,11 +1520,11 @@ class AuthenticationService {
15211520
} catch (e) {
15221521
const errorWithMessage = e as { message: string };
15231522

1524-
// Check if the error is because biometrics are not enabled
1523+
// Check if the error is because password is not set with biometrics
15251524
// Convert it to AUTHENTICATION_APP_TRIGGERED_AUTH_NO_CREDENTIALS so UI can handle it
15261525
if (
15271526
errorWithMessage.message.includes(
1528-
ReauthenticateErrorType.BIOMETRIC_NOT_ENABLED,
1527+
ReauthenticateErrorType.PASSWORD_NOT_SET_WITH_BIOMETRICS,
15291528
)
15301529
) {
15311530
throw new AuthenticationError(
@@ -1571,20 +1570,28 @@ class AuthenticationService {
15711570

15721571
// if no password is provided, try to use the stored biometric/remember-me password
15731572
if (!passwordToVerify) {
1574-
const biometryChoice = await StorageWrapper.getItem(BIOMETRY_CHOICE);
1575-
if (biometryChoice) {
1573+
try {
15761574
const credentials = await this.getPassword();
15771575
if (credentials) {
15781576
passwordToVerify = credentials.password;
15791577
}
1578+
} catch (e) {
1579+
const error = e as Error;
1580+
// TODO: May want to triage errors here and throw different errors based on the error type
1581+
// For example, getPassword throws with `User canceled the operation` when biometrics is canceled or fails
1582+
// Disallowing biometrics on system level will not throw an error and just return empty credentials
1583+
throw new Error(
1584+
`${ReauthenticateErrorType.BIOMETRIC_ERROR}: ${error.message}`,
1585+
);
15801586
}
15811587

15821588
// If there is no biometric choice configured or no stored credentials,
15831589
// throw a specific error instead of attempting to verify an empty password.
15841590
if (!passwordToVerify) {
1585-
const biometricNotEnabledErrorMessage = 'Biometric is not enabled';
1591+
const passwordNotSetWithBiometricsErrorMessage =
1592+
'No password stored with biometrics in keychain.';
15861593
throw new Error(
1587-
`${ReauthenticateErrorType.BIOMETRIC_NOT_ENABLED}: ${biometricNotEnabledErrorMessage}`,
1594+
`${ReauthenticateErrorType.PASSWORD_NOT_SET_WITH_BIOMETRICS}: ${passwordNotSetWithBiometricsErrorMessage}`,
15881595
);
15891596
}
15901597
}

app/core/Authentication/types.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
export enum ReauthenticateErrorType {
2-
BIOMETRIC_NOT_ENABLED = 'BIOMETRIC_NOT_ENABLED',
2+
PASSWORD_NOT_SET_WITH_BIOMETRICS = 'PASSWORD_NOT_SET_WITH_BIOMETRICS',
3+
BIOMETRIC_ERROR = 'BIOMETRIC_ERROR',
34
}

0 commit comments

Comments
 (0)