-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat(protocol): credential mediation #361
Conversation
This adds support for credential mediation.
WalkthroughThis pull request introduces a comprehensive enhancement to the WebAuthn library's credential management by adding a Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebAuthn
participant AuthenticatorService
Client->>WebAuthn: BeginMediatedLogin(user, mediation)
WebAuthn->>AuthenticatorService: Request Credential Assertion
AuthenticatorService-->>WebAuthn: Return Credential Options
WebAuthn-->>Client: Return Credential Assertion with Mediation
Possibly related issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
webauthn/login.go (3)
34-35
: Consider providing a default mediation type.Currently, passing an empty string (
""
) for the mediation requirement relies on the zero value. Using a meaningful default, such asprotocol.MediationOptional
, may improve clarity and prevent confusion.func (webauthn *WebAuthn) BeginLogin(user User, opts ...LoginOption) (*protocol.CredentialAssertion, *SessionData, error) { - return webauthn.BeginMediatedLogin(user, "", opts...) + return webauthn.BeginMediatedLogin(user, protocol.MediationOptional, opts...) }
37-39
: Likewise, consider specifying an explicit mediation requirement for discoverable login.Similar to the comment above, passing an empty string as the mediation requirement leaves it ambiguous. Specifying a default like
protocol.MediationOptional
may be clearer.
76-76
: Mediation field assignment is correct.Ensure the calling code or documentation clarifies expectations when mediation is empty or a non-standard value.
webauthn/registration.go (1)
24-25
: Suggest clarifying the default mediation type in BeginRegistration.Similarly to the login flow, passing an empty string may cause confusion about which mediation policy is in effect.
func (webauthn *WebAuthn) BeginRegistration(user User, opts ...RegistrationOption) (creation *protocol.CredentialCreation, session *SessionData, err error) { - return webauthn.BeginMediatedRegistration(user, "", opts...) + return webauthn.BeginMediatedRegistration(user, protocol.MediationOptional, opts...) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
protocol/authenticator.go
(1 hunks)protocol/options.go
(1 hunks)webauthn/login.go
(3 hunks)webauthn/registration.go
(2 hunks)
🔇 Additional comments (13)
webauthn/login.go (4)
42-43
: New function introducing mediation parameter looks good.
The docstring aligns with the new mediation feature, and the function signature is well-structured.
56-56
: Straightforward propagation of the mediation parameter.
This call neatly forwards all relevant parameters to beginLogin
.
59-62
: Consistent approach for discoverable mediated login.
The additional method is consistent with the other login flows, providing a mediation parameter for discoverable credentials.
65-65
: Updated function signature for beginLogin.
Including the mediation
parameter maintains consistency and extends configurability.
webauthn/registration.go (2)
27-28
: New mediated registration function is well-integrated.
It mirrors the approach in the login flow and appears functionally sound.
72-72
: Mediation field addition is consistent.
The usage aligns with the broader mediation pattern introduced in the codebase.
protocol/options.go (2)
8-9
: Added Mediation field for CredentialCreation.
This field adheres to the new design for specifying mediation requirements.
13-14
: Added Mediation field for CredentialAssertion.
Similarly extends mediation functionality to assertions. The approach is consistent with the creation flow.
protocol/authenticator.go (5)
60-66
: Definition of CredentialMediationRequirement is well-documented.
The references to the W3C Credential Management spec provide clarity on usage.
67-72
: MediationSilent constant implementation looks good.
The descriptive comment helps clarify expected behavior.
73-76
: MediationOptional constant is properly introduced.
Documentation is succinct and accurate to the spec.
77-84
: MediationConditional constant is well-defined.
The thorough explanation of non-modal dialog behavior is beneficial for implementers.
85-88
: MediationRequired constant is clearly described.
Nicely captures the requirement to always involve user mediation.
This adds support for credential mediation.
Closes #347