Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(ios) App Access Control: Device passcode fallback #478

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Conversation

robbiehanson
Copy link
Contributor

Partially addresses issue #441

Phoenix should provide more options for app access control, each with their own trade-offs:

  1. biometrics authentication: the recommended option. It enables fine-grained control on a device that is shared between several users (e.g. a family), reasonably secure, but less private and prone to hardware malfunction
  2. the device's PIN/password: no fine control and probably less secure, but it's robust

Option 2 was missing from iOS, but fixed via this PR.

 

If the user:

  • enables biometrics (Face ID / Touch ID)
  • but does not enable the passcode fallback
  • and has not backed up their recovery phrase

then we display this warning:

@robbiehanson robbiehanson requested a review from dpad85 November 28, 2023 16:11
Copy link
Member

@dpad85 dpad85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to let the user only use the Passcode option, without having to enable FaceID/TouchID? It does not really matter since the correct solution for users who don't want biometrics will eventually use the custom password feature (in a future PR), but it could be a valid option.

if the user ... then we display this warning:

I think the dialog is a bit too much:

  • if the risk is so severe that a specific full-screen warning is needed, then we should not let the user disable that fallback option - or at least make it opt-out instead of opt-in.
  • the warning should be contextual, in the App Access screen, since it really applies depending on the Passcode fallback option being enabled or not.

For example the help message in the Allow passcode fallback setting could be updated and contain a warning if needed.

But honestly, I think we can just remove that warning altogether : users need to backup their seed regardless of the app-control setting. It's true that a broken biometrics hardware can lock the user out if the passcode fallback was disabled, but that's just an edge case among many and we can't handle all of them.

But we could regularly remind the user that backing up their seed is important, even if they already checked the boxes.

…scode fallback, and hasn't backed up their recovery phrase.
@robbiehanson
Copy link
Contributor Author

I think we can just remove that warning altogether : users need to backup their seed regardless of the app-control setting. It's true that a broken biometrics hardware can lock the user out if the passcode fallback was disabled, but that's just an edge case among many and we can't handle all of them.

I agree. And that screen already displays a warning icon next to the "Recovery Phrase", if the user hasn't performed a backup.

Done in 880114a

@robbiehanson
Copy link
Contributor Author

Is it possible to let the user only use the Passcode option, without having to enable FaceID/TouchID?

I do not believe this is possible. My understanding is that you have to use LAContext to prompt for iOS authentication. And the supported policies only allow you to determine whether-or-not to allow passcode fallback.

@robbiehanson robbiehanson requested a review from dpad85 November 30, 2023 16:50
@robbiehanson
Copy link
Contributor Author

Addresses issue #455

Copy link
Member

@dpad85 dpad85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants