-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
Map authenticator transports on server side #453
Map authenticator transports on server side #453
Conversation
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 #453 +/- ##
==========================================
+ Coverage 73.83% 73.88% +0.04%
==========================================
Files 99 99
Lines 2710 2711 +1
Branches 445 445
==========================================
+ Hits 2001 2003 +2
+ Misses 604 603 -1
Partials 105 105 ☔ View full report in Codecov by Sentry. |
@iamcarbon @aseigler While this could be merged, we're currently not showcasing transports anywhere. In fact, we have this line: https://github.com/passwordless-lib/fido2-net-lib/blob/master/Src/Fido2/AuthenticatorAttestationResponse.cs#L193 which specifically mentions that you would need to get this yourself somehow. Should we remove Transports from |
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.
Actually - when reading the spec, My opinion is that we should add transports to our server side raw class and keep sending it from JS, just like we do. We should additionally make sure it ends up in the RegisteredPublicKeyCredential
class
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.
@joegoldman2 Thank you, I think this looks solid.
Would welcome any second opinions from @iamcarbon or @aseigler, but I'm approving this.
Code looks reasonable to me, not sure what is going on with those failed tests. Is that new or an environment issue? |
I'm trying to re-run those checks to see if can get this in! |
Map the authenticator transports on server side. The demo app already stores the value:
fido2-net-lib/Demo/Controller.cs
Line 130 in ebfd75d