-
-
Notifications
You must be signed in to change notification settings - Fork 181
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
Base64Url Id instead of byte[] #586
base: master
Are you sure you want to change the base?
Conversation
74ed708
to
61e2694
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #586 +/- ##
=======================================
Coverage 76.04% 76.04%
=======================================
Files 101 101
Lines 2601 2601
Branches 432 432
=======================================
Hits 1978 1978
Misses 513 513
Partials 110 110 ☔ View full report in Codecov by Sentry. |
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've been reviewing this, as well has reading some of the related WebAuthn discussions. like: w3c/webauthn#2119
Although this will introduce lots of "won't-compile"-errors when people upgrade to 4.0, I agree it's probably the right thing to do.
The current set of changes look good, however I think we also need to change the Id
-prop on AuthenticatorAttestationRawResponse
, so I will request changes.
61e2694
to
826d448
Compare
Changed the This might also be the right time to make the // might be wrong to base64url encode this...
[JsonConverter(typeof(Base64UrlConverter))]
[JsonPropertyName("rawId"), Required]
public byte[] RawId { get; init; } |
RawId is decoded to the raw byte value, while Id is the same value in base64url-encoded form.
826d448
to
74bc4f1
Compare
Looks like for some reason the upload artifats github action is deprecated. Should I just update it here or would your prefer it in its own branch? |
@Regenhardt It's own branch please! |
Is this ready for review again? |
Yes it's ready for review, although the failed pipeline has to wait for the new artifacts action to be updated. |
This needs #590 to fix the pipeline. |
#513
Not sure if any of the places that now use
RawId
should be changed to useId
instead, likeCredentialId
being returned from a successful assertion. This basically just changes the model and makes everything use the RawId, so minimal breaking changes hopefully.