-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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. |
👍 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: Next stepsTake a deeper look at the dependencyTake 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 packageIf 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 riskTo ignore an alert, reply with a comment starting with |
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Cal-L I added and E2E run for QA: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2f9a98c4-d5e0-46c2-96de-03bbd047a54b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Development & PR Process
release-xx
label to identify the PR slated for a upcoming release (will be used in release discussion)needs-dev-review
label when work is completedneeds-qa
label when dev review is completedQA Passed
label when QA has signed offDescription
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
Scenario: Should ask to re-authenticate when interrupting biometrics authentication
Scenario: Staying in the background for more than 5 seconds should lock the app with the lock timer set to 5 seconds
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