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

[PM-7382] Add support for non-UUID credential #11993

Merged
merged 4 commits into from
Nov 21, 2024

Conversation

coroiu
Copy link
Contributor

@coroiu coroiu commented Nov 14, 2024

🎟️ Tracking

📔 Objective

Implement support for credentialIds encoded as Base64. This does not change how new credentialIds are created, this is on purpose so that we don't break older clients

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Technically this deviates from the specification, but nobody is going to be using the authenticator directly but us so it shouldn't matter. We're gonna switch to `passkey-rs` anyways so
@coroiu coroiu requested a review from Hinton November 14, 2024 13:10
@coroiu coroiu requested a review from a team as a code owner November 14, 2024 13:10
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 33.49%. Comparing base (b4aea05) to head (8906495).
Report is 62 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...form/services/fido2/fido2-authenticator.service.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11993      +/-   ##
==========================================
+ Coverage   33.48%   33.49%   +0.01%     
==========================================
  Files        2844     2845       +1     
  Lines       89044    89057      +13     
  Branches    16987    16990       +3     
==========================================
+ Hits        29812    29827      +15     
+ Misses      56887    56885       -2     
  Partials     2345     2345              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details4100cf94-2c35-4220-aebf-cdfd9ea4ca5e

Fixed Issues

Severity Issue Source File / Package
LOW Client_JQuery_Deprecated_Symbols /apps/cli/src/service-container/service-container.ts: 876

Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

I tested this after modifying the makeKey logic to produce a b64 credentialId and it seems to work. The passkey also worked on the native iOS app, so I think we're good with this approach.

@@ -64,7 +64,7 @@ export class Fido2AuthenticatorError extends Error {
}

export interface PublicKeyCredentialDescriptor {
id: BufferSource;
id: Uint8Array;
Copy link
Member

Choose a reason for hiding this comment

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

General question, should be interface be prescriptive enough about the ID to define it as Uint8Array, or use ArrayBuffer? I don't see an issue sticking with Uint8Array though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually commented this in the commit. I don't think it matters

refactor: change interface to use Uint8Array for simplification
Technically this deviates from the specification, but nobody is going to be using the authenticator directly but us so it shouldn't matter. We're gonna switch to passkey-rs anyways so


export function parseCredentialId(encodedCredentialId: string): Uint8Array {
try {
if (encodedCredentialId.startsWith("b64.")) {
Copy link
Member

Choose a reason for hiding this comment

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

I hate it, but let's run with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agree

/**
* Compares two credential IDs for equality.
*/
export function compareCredentialIds(a: Uint8Array, b: Uint8Array): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

We may benefit from having a general util function for comparing Uint8Arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but you've always stopped me when adding functions to the general utils in the past so now I'm going by "if we use it twice put it in general" XD

@coroiu coroiu added the needs-qa Marks a PR as requiring QA approval label Nov 15, 2024
@coroiu coroiu removed the needs-qa Marks a PR as requiring QA approval label Nov 21, 2024
@coroiu coroiu merged commit acf5b1e into main Nov 21, 2024
73 of 74 checks passed
@coroiu coroiu deleted the PM-7382-add-support-for-non-uuid-credential-id branch November 21, 2024 14:54
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.

3 participants