Skip to content

Commit

Permalink
fix: Onboarding failing biometrics locks screen for user instead of d…
Browse files Browse the repository at this point in the history
…isabling biometrics and continuing with the onboarding (#12120)

## **Description**

During the course of onboarding, if the user enables Face ID to log in
to the app, but the Face ID is unsuccessful in reading the users face,
or the user cancels the Face ID check, the user is then locked out of
the app and kicked out of the onboarding flow. When they sign back in
they are taken to the wallet screen.

I have added a fix so that if the Face ID fails or the user cancels the
Face ID check, Face ID will be disabled and the user will have to log in
with their password. The onboarding process will also continue without
locking the user out of the app.

The fix isn't 'ideal' as we use a `catch` block to catch an error and
then perform some state setting, when ideally a `catch` should be used
exclusively for error handling. However, when Biometrics fail, React
Native throws an error that ends up in the `catch`. I also would prefer
an `enum` to be used but we are in a JS file instead of a TS file.

To test the issue repeat the following:

1. Install the app on device.
2. Start the onboarding and enter a new password.
3. Enable Face ID (iOS) and cover the camera so the Face ID fails.
4. Then hit cancel, or repeat the process and hit cancel when the second
alert appears.
5. You should then be pushed through to the next onboarding screen and
you should not be sent back to the lock screen and be asked to log in
again.

## **Related issues**

Fixes:
https://github.com/orgs/MetaMask/projects/60/views/16?pane=issue&itemId=84422135&issue=MetaMask%7Cmetamask-mobile%7C11964

## **Manual testing steps**

1. Install the app on device.
2. Start the onboarding and enter a new password.
3. Enable Face ID (iOS) and cover the camera so the Face ID fails.
4. Then hit cancel, or repeat the process and hit cancel when the second
alert appears.
5. You should then be pushed through to the next onboarding screen and
you should not be sent back to the lock screen and be asked to log in
again.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**


https://github.com/user-attachments/assets/39dc4b1f-3099-433f-ac7b-4aff9e6ca742

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
  • Loading branch information
Daniel-Cross authored Nov 7, 2024
1 parent e446dc1 commit b2c9110
Show file tree
Hide file tree
Showing 2 changed files with 231 additions and 1 deletion.
30 changes: 29 additions & 1 deletion app/core/SecureKeychain.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export default {
const metrics = MetaMetrics.getInstance();
if (type === this.TYPES.BIOMETRICS) {
authOptions.accessControl = Keychain.ACCESS_CONTROL.BIOMETRY_CURRENT_SET;

await metrics.addTraitsToUser({
[UserProfileProperty.AUTHENTICATION_TYPE]:
AUTHENTICATION_TYPE.BIOMETRIC,
Expand Down Expand Up @@ -155,7 +156,34 @@ export default {
// If the user enables biometrics, we're trying to read the password
// immediately so we get the permission prompt
if (Platform.OS === 'ios') {
await this.getGenericPassword();
try {
await this.getGenericPassword();
} catch (error) {
// Specifically check for user cancellation
if (error.message === 'User canceled the operation.') {
// Store password without biometrics
const encryptedPassword = await instance.encryptPassword(password);
await Keychain.setGenericPassword(
'metamask-user',
encryptedPassword,
{
...defaultOptions,
},
);

// Update storage to reflect disabled biometrics
await StorageWrapper.removeItem(BIOMETRY_CHOICE);
await StorageWrapper.setItem(BIOMETRY_CHOICE_DISABLED, TRUE);

// Update metrics
await metrics.addTraitsToUser({
[UserProfileProperty.AUTHENTICATION_TYPE]:
AUTHENTICATION_TYPE.PASSWORD,
});

return;
}
}
}
} else if (type === this.TYPES.PASSCODE) {
await StorageWrapper.removeItem(BIOMETRY_CHOICE);
Expand Down
202 changes: 202 additions & 0 deletions app/core/SecureKeychain.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
import SecureKeychain from './SecureKeychain';
import * as Keychain from 'react-native-keychain'; // eslint-disable-line import/no-namespace
import StorageWrapper from '../store/storage-wrapper';
import { Platform } from 'react-native';
import {
BIOMETRY_CHOICE,
BIOMETRY_CHOICE_DISABLED,
PASSCODE_CHOICE,
PASSCODE_DISABLED,
TRUE,
} from '../constants/storage';
import { UserProfileProperty } from '../util/metrics/UserSettingsAnalyticsMetaData/UserProfileAnalyticsMetaData.types';
import AUTHENTICATION_TYPE from '../constants/userProperties';

jest.mock('../../locales/i18n', () => ({
strings: jest.fn((key) => key),
}));

jest.mock('../store/storage-wrapper', () => ({
__esModule: true,
default: {
setItem: jest.fn(),
removeItem: jest.fn(),
getItem: jest.fn().mockResolvedValue(null),
},
}));

jest.mock('react-native-keychain', () => ({
ACCESSIBLE: {
WHEN_UNLOCKED_THIS_DEVICE_ONLY: 'WHEN_UNLOCKED_THIS_DEVICE_ONLY',
},
ACCESS_CONTROL: {
BIOMETRY_CURRENT_SET: 'BIOMETRY_CURRENT_SET',
DEVICE_PASSCODE: 'DEVICE_PASSCODE',
},
setGenericPassword: jest.fn(),
getGenericPassword: jest.fn(),
resetGenericPassword: jest.fn(),
}));

jest.mock('../store/storage-wrapper', () => ({
__esModule: true,
default: {
setItem: jest.fn(),
removeItem: jest.fn(),
},
}));

const mockAddTraitsToUser = jest.fn();
jest.mock('../core/Analytics', () => ({
MetaMetrics: {
getInstance: jest.fn(() => ({
addTraitsToUser: mockAddTraitsToUser,
trackEvent: jest.fn(),
})),
},
}));

describe('SecureKeychain - setGenericPassword', () => {
const mockPassword = 'test_password';

beforeEach(() => {
jest.clearAllMocks();
SecureKeychain.init('test_salt');
});

it('should set biometric authentication correctly', async () => {
await SecureKeychain.setGenericPassword(
mockPassword,
SecureKeychain.TYPES.BIOMETRICS,
);

expect(Keychain.setGenericPassword).toHaveBeenCalledWith(
'metamask-user',
expect.any(String),
expect.objectContaining({
accessControl: Keychain.ACCESS_CONTROL.BIOMETRY_CURRENT_SET,
}),
);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(BIOMETRY_CHOICE, TRUE);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(
PASSCODE_DISABLED,
TRUE,
);
expect(mockAddTraitsToUser).toHaveBeenCalledWith(
expect.objectContaining({
[UserProfileProperty.AUTHENTICATION_TYPE]:
AUTHENTICATION_TYPE.BIOMETRIC,
}),
);
});

it('should set passcode authentication correctly', async () => {
await SecureKeychain.setGenericPassword(
mockPassword,
SecureKeychain.TYPES.PASSCODE,
);

expect(Keychain.setGenericPassword).toHaveBeenCalledWith(
'metamask-user',
expect.any(String),
expect.objectContaining({
accessControl: Keychain.ACCESS_CONTROL.DEVICE_PASSCODE,
}),
);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(PASSCODE_CHOICE, TRUE);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(
BIOMETRY_CHOICE_DISABLED,
TRUE,
);
});

it('should set remember me correctly', async () => {
await SecureKeychain.setGenericPassword(
mockPassword,
SecureKeychain.TYPES.REMEMBER_ME,
);

expect(Keychain.setGenericPassword).toHaveBeenCalledWith(
'metamask-user',
expect.any(String),
expect.not.objectContaining({
accessControl: expect.anything(),
}),
);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(
PASSCODE_DISABLED,
TRUE,
);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(
BIOMETRY_CHOICE_DISABLED,
TRUE,
);
});

it('should reset password when no type is provided', async () => {
const resetSpy = jest.spyOn(SecureKeychain, 'resetGenericPassword');
await SecureKeychain.setGenericPassword(mockPassword);

expect(resetSpy).toHaveBeenCalled();
expect(Keychain.setGenericPassword).not.toHaveBeenCalled();
});

describe('iOS Biometric Handling', () => {
beforeEach(() => {
Platform.OS = 'ios';
});

it('should handle user cancellation of biometric prompt', async () => {
(Keychain.getGenericPassword as jest.Mock).mockRejectedValueOnce(
new Error('User canceled the operation.'),
);

await SecureKeychain.setGenericPassword(
mockPassword,
SecureKeychain.TYPES.BIOMETRICS,
);

expect(StorageWrapper.removeItem).toHaveBeenCalledWith(BIOMETRY_CHOICE);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(
BIOMETRY_CHOICE_DISABLED,
TRUE,
);
expect(mockAddTraitsToUser).toHaveBeenLastCalledWith(
expect.objectContaining({
[UserProfileProperty.AUTHENTICATION_TYPE]:
AUTHENTICATION_TYPE.PASSWORD,
}),
);
});

it('should successfully set up biometric authentication', async () => {
(Keychain.getGenericPassword as jest.Mock).mockResolvedValueOnce({
password: 'encrypted_password',
});

await SecureKeychain.setGenericPassword(
mockPassword,
SecureKeychain.TYPES.BIOMETRICS,
);

expect(StorageWrapper.setItem).toHaveBeenCalledWith(
BIOMETRY_CHOICE,
TRUE,
);
expect(StorageWrapper.setItem).toHaveBeenCalledWith(
PASSCODE_DISABLED,
TRUE,
);
expect(StorageWrapper.removeItem).toHaveBeenCalledWith(PASSCODE_CHOICE);
expect(StorageWrapper.removeItem).toHaveBeenCalledWith(
BIOMETRY_CHOICE_DISABLED,
);
expect(mockAddTraitsToUser).toHaveBeenCalledWith(
expect.objectContaining({
[UserProfileProperty.AUTHENTICATION_TYPE]:
AUTHENTICATION_TYPE.BIOMETRIC,
}),
);
});
});
});

0 comments on commit b2c9110

Please sign in to comment.