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

fix: #893 fix immediate lock timer #6653

Merged
merged 70 commits into from
Jul 28, 2023
Merged

fix: #893 fix immediate lock timer #6653

merged 70 commits into from
Jul 28, 2023

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Jun 21, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
The PR fixes showing the biometrics prompt after backgrounding/foregrounding the app with the lock timer set to immediate. Prior to the fix, the lock timer wasn't respected when backgrounding in the middle of authentication via biometrics. After the fix, the biometrics prompt should show again even after backgrounding while authenticating. Other videos show the timers being respected as well. Lastly, the lock screen, for the most part should not affect the underlying screen after re-authenticating after foregrounding. You'll see in some videos where that is the case. The videos below demonstrate some of the behavior as mentioned.

Some examples of tests:

  • Scenario: Should remain on same screen after being locked out and re-authenticating

    • Given: The lock timer is set to immediate and biometrics is enabled
    • When: I background the app and foreground it
    • And: I re-authenticate using biometrics
    • Then: I should see that I am on the same screen that I was on prior to backgrounding
  • Scenario: Should ask to re-authenticate when interrupting biometrics authentication

    • Given: The lock timer is set to immediate and biometrics is enabled
    • When: I background the app and foreground it
    • And: I re-authenticate using biometrics
    • And: I background in the middle of biometrics verification
    • And: I foreground the app
    • Then: I should be prompted to re-authenticate
  • Scenario: Staying in the background for more than 5 seconds should lock the app with the lock timer set to 5 seconds

    • Given: The lock timer is set to 5 seconds and biometrics is enabled
    • When: I background the app and wait 6 seconds
    • When: I foreground the app
    • Then: I should be prompted to re-authenticate

Video using TouchID on iOS with timer set to immediate
https://github.com/MetaMask/metamask-mobile/assets/10508597/35ee4bf0-7f2a-495f-a290-ec463eb4eb2b

Video using TouchID on iOS with timer set to 5 seconds
https://github.com/MetaMask/metamask-mobile/assets/10508597/e4d50499-62aa-49c2-adf4-71eceb9be994

Video using FaceID on iOS with timer set to immediate
https://github.com/MetaMask/metamask-mobile/assets/10508597/206f0834-0149-4991-bf31-c9fe8030090e

Video using FaceID on iOS with timer set to 5 seconds
https://github.com/MetaMask/metamask-mobile/assets/10508597/8b0c1528-0985-47ad-b780-26cac29f4077

Video with biometrics disabled on iOS with timer set to immediate
https://github.com/MetaMask/metamask-mobile/assets/10508597/c2a68764-1e61-4b04-8e0a-4012a4990a24

Video with biometrics disabled on iOS with timer set to 5 seconds
https://github.com/MetaMask/metamask-mobile/assets/10508597/4751a6cc-ed00-4d7a-8063-c860a4a05d9e

Video with TouchID on Android with timer set to immediate
https://recordit.co/CphtqALsck

Video with TouchID on Android with timer set to 5 seconds
https://recordit.co/JR7Q0rKtpA

Caveat - Since it was a risk to completely refactor authentication code, we kept most of the existing logic and just introduced a state machine to handle the backgrounding/foregrounding logic with respect to the lock timer. As a result, we will not see the complete metamask fox animation since the log in logic is now dependent on the state machine instead of the completion of the animation.

Issue
#893

@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Cal-L Cal-L added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-client labels Jun 22, 2023
@Cal-L Cal-L marked this pull request as ready for review June 22, 2023 06:57
@Cal-L Cal-L requested a review from a team as a code owner June 22, 2023 06:57
@gauthierpetetin gauthierpetetin added ready-for-dev and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jun 22, 2023
@socket-security
Copy link

socket-security bot commented Jun 22, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: @redux-saga/deferred@1.2.1, @redux-saga/delay-p@1.2.1, @redux-saga/is@1.1.3, @redux-saga/symbols@1.1.3, typescript-compare@0.0.2, typescript-logic@0.0.0, typescript-tuple@2.2.1

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@socket-security
Copy link

socket-security bot commented Jun 22, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
redux-saga 1.2.3 eval, environment +9 735 kB redux-saga-release-bot

sethkfman
sethkfman previously approved these changes Jul 26, 2023
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman
Copy link
Contributor

sethkfman
sethkfman previously approved these changes Jul 27, 2023
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jul 27, 2023
sethkfman
sethkfman previously approved these changes Jul 27, 2023
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

Andepande
Andepande previously approved these changes Jul 28, 2023
Copy link
Member

@Andepande Andepande left a comment

Choose a reason for hiding this comment

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

LGTM, QA-Passed, built PR locally on physical Android device and IOS sim

@Andepande Andepande added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 28, 2023
@Cal-L Cal-L dismissed stale reviews from Andepande and sethkfman via 2c6b5f3 July 28, 2023 21:13
@sonarcloud
Copy link

sonarcloud bot commented Jul 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

67.6% 67.6% Coverage
0.0% 0.0% Duplication

Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

Looks good.

@cortisiko cortisiko merged commit 5258fe9 into main Jul 28, 2023
11 checks passed
@cortisiko cortisiko deleted the fix/893-biometrics-timer branch July 28, 2023 21:58
@github-actions github-actions bot locked and limited conversation to collaborators Jul 28, 2023
@metamaskbot metamaskbot added the release-7.6.0 Issue or pull request that will be included in release 7.6.0 label Jul 28, 2023
@Cal-L Cal-L restored the fix/893-biometrics-timer branch August 18, 2023 03:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.6.0 Issue or pull request that will be included in release 7.6.0 team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants