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

biometric-ed25519 bug fix on supporting Bitwarden #1339

Merged
merged 8 commits into from
Apr 16, 2024

Conversation

hcho112
Copy link
Contributor

@hcho112 hcho112 commented Apr 15, 2024

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this changes code in a published package: I have run pnpm changeset to create a changeset JSON document appropriate for this change.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

This PR contains bug fix that are introduced on #1331. When it was locally tested, it worked as expected. However we have recently received a feedback that it breaks existing flow. (Possibly from recent Chrome browser change perhaps?)

After further investigation, I have confirmed that existing approach would not work because 'Credentials' (A.K.A PublicKeyCredential) is a non-enumerable object where it doesn't allow to apply Object.keys().

Since existing 'sanitization' is tailored for specific purpose (convert Uint8Array to ArrayBuffer), decided to tailor the function for its needs by explicitly checks for expected response properties from navigator.credentials.create and navigator.credentials.get.

One thing to note on current approach is that if we encounter Credentials response that contains Uint8Array, after sanitization, it loses its prototype property and become an Object. However rest of the code doesn't expect it prototype and still returns expectedKeypairs as outcome so it still work as expected.

Test Plan

Since this repo is using pnpm, it is hard to setup link towards to other project. Therefore I used Verdaccio to locally publish the package with version 1.2.1 (locally increment the version) and import the package to fast-auth-signer repo. (Please review how to use Verdaccio from their website, if anyone need further help on getting it working, please leave on comment)

Related issues/PRs

#1330

@hcho112 hcho112 requested a review from vikinatora April 15, 2024 07:54
Copy link

changeset-bot bot commented Apr 15, 2024

🦋 Changeset detected

Latest commit: 845e1a9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@near-js/biometric-ed25519 Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hcho112
Copy link
Contributor Author

hcho112 commented Apr 15, 2024

Hey @vikinatora if this PR gets approved, is it possible to get it released straight after? Its currently blocking groups of people.

@vikinatora
Copy link
Collaborator

@hcho112 Sure, we'll release it asap. Have you tested this code in some way because the module doesn't have any unit tests to verify the behavior?

@hcho112
Copy link
Contributor Author

hcho112 commented Apr 15, 2024

@vikinatora thanks for reminding me, I have committed new test test cases. Since PublicKeyCredential doesn't really exist, I had to mock them in the test.

@hcho112 hcho112 force-pushed the support-bitwarden-webauthn2 branch from f0eb53d to fa6467d Compare April 15, 2024 22:54
@hcho112
Copy link
Contributor Author

hcho112 commented Apr 15, 2024

@vikinatora Also, if you prefer to conduct manual test, you can setup the test environment by following Test Plan. Let me know if you need help on that

@vikinatora vikinatora merged commit 45cfbec into master Apr 16, 2024
1 check passed
@vikinatora vikinatora deleted the support-bitwarden-webauthn2 branch April 16, 2024 05:58
@github-actions github-actions bot mentioned this pull request Apr 16, 2024
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