-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[No QA] [ECUK In-App 3DS] Add essential actions, wrappers for the auth keys, challenges logic & BIOMETRICS_TEST scenario
#79477
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
Conversation
BIOMETRICS_TEST scenario
20d80e9 to
1a87b6e
Compare
BIOMETRICS_TEST scenarioBIOMETRICS_TEST scenario
BIOMETRICS_TEST scenarioBIOMETRICS_TEST scenario
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/components/MultifactorAuthentication/config/scenarios/DefaultUserInterface.ts
Show resolved
Hide resolved
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a87b6e913
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
afb40e3 to
68bd50f
Compare
| function signToken(credentialRequestOptions: MultifactorAuthenticationChallengeObject, privateKey: string): SignedChallenge { | ||
| const rawId: Base64URLString = Base64URL.encode(VALUES.KEY_ALIASES.PUBLIC_KEY); | ||
| const type = VALUES.ED25519_TYPE; | ||
| function signToken(credentialRequestOptions: MultifactorAuthenticationChallengeObject, privateKey: string, publicKey: Base64URLString): SignedChallenge { |
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.
I think we should only pass rpId and challenge instead of the full credentialRequestOptions. It would be more reusable.
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.
let's move it to #79473
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.
I think this format is good because we want to standardize the implementation of native biometrics and passkeys to make the implementation easier to review. And these data will be required for passkeys.
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.
Ok, so let's revisit it after passkey implementation
Reviewer Checklist
|
70a873b to
2548bc7
Compare
|
Agreed, let's move forward. |
…r-auth-and-challenges
src/components/MultifactorAuthentication/config/scenarios/BiometricsTest.ts
Show resolved
Hide resolved
| } | ||
|
|
||
| if (isChallengeSigned(this.challenge)) { | ||
| return this.createErrorReturnValue(VALUES.REASON.CHALLENGE.CHALLENGE_ALREADY_SIGNED); |
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.
Reopening this one - I think it would be better to log a warning (and/or record in sentry?), so we can debug the unexpected behavior, but not fail the scenario for the use, since they have what they need to proceed (i.e. a signed challenge). I won't block on it, but it would be great to fix in follow-up.
src/libs/MultifactorAuthentication/Biometrics/SecureStore/index.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * Maps API endpoints to their HTTP status codes and corresponding reason messages. | ||
| */ | ||
| const API_RESPONSE_MAP = { |
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.
I think this will end up being refactored. NAB for this PR, just noting - cc @dariusz-biela
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rafecolton in version: 9.3.10-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.10-6 🚀
|
Explanation of Change
This PR adds:
TroubleshootMultifactorAuthenticationaction and API commandBIOMETRICS_TESTScenario and its configBIOMETRICS_TESTscenario screens & routes + add test biometrics button to the troubleshoot menu #79475Fixed Issues
$ #79376
$ #79059
PROPOSAL: N/A
Tests
N/A
Offline tests
N/A,
D - Full Page Blocking UI Patternfor this project.QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari