-
-
Notifications
You must be signed in to change notification settings - Fork 34
Add support for optional AuthenticatorSelection #120
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
Add support for optional AuthenticatorSelection #120
Conversation
- Introduced `AuthenticatorSelection` struct to define Relying Party's requirements for authenticator attributes. - Updated `PublicKeyCredentialCreationOptions` to include `authenticatorSelection` parameter. - Enhanced `WebAuthnManager`'s `beginRegistration` method to accept `authenticatorSelection` as an argument. - Added `ResidentKeyRequirement` struct to specify requirements for client-side-resident public key credentials.
- Enhanced `WebAuthnManager` to better manage authenticator selection during registration. - Updated related structs and methods to improve clarity and maintainability. - Added tests to ensure correct functionality of the new authenticator selection features.
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.
Pull request overview
This PR adds optional AuthenticatorSelection support to PublicKeyCredentialCreationOptions, enabling proper configuration of authenticator requirements during WebAuthn credential creation. This change allows Chrome on Android 16 to create discoverable passkeys in Google Password Manager by specifying that the server requires a client-side-resident credential.
Key changes:
- Introduced
ResidentKeyRequirementenum-like type to specify whether authenticators should create client-side-resident credentials - Added
AuthenticatorSelectionstruct to describe authenticator attribute requirements - Extended
PublicKeyCredentialCreationOptionsandWebAuthnManager.beginRegistration()to accept optional authenticator selection criteria
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Sources/WebAuthn/Ceremonies/Registration/ResidentKeyRequirement.swift |
New file defining the resident key requirement enumeration (required, preferred, discouraged) per WebAuthn spec |
Sources/WebAuthn/Ceremonies/Registration/AuthenticatorSelection.swift |
New file defining the authenticator selection criteria dictionary with optional authenticator attachment, resident key, and user verification properties |
Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift |
Added optional authenticatorSelection property with proper encoding/decoding support |
Sources/WebAuthn/WebAuthnManager.swift |
Added optional authenticatorSelection parameter to beginRegistration() method with default nil value for backward compatibility |
Tests/WebAuthnTests/AuthenticatorSelectionTests.swift |
Comprehensive test suite covering encoding/decoding, initialization, integration with creation options, and WebAuthnManager usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Regarding the failed checks: Would it be better to introduce a separate PublicKeyCredentialCreationOptions constructor rather than making AuthenticatorSelection optional? |
dimitribouniol
left a comment
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.
Looks good for the most part! I wouldn't worry about the breaking change here, it would be limited to key path users, and the library is still in beta.
Other than that, please see my suggestions for the doc comments so we have something nicer to link folks out to, and add support for the requireResidentKey encoded property that is required by the spec so we don't forget to add it later.
Thanks!
Sources/WebAuthn/Ceremonies/Registration/AuthenticatorSelection.swift
Outdated
Show resolved
Hide resolved
Sources/WebAuthn/Ceremonies/Registration/AuthenticatorSelection.swift
Outdated
Show resolved
Hide resolved
Sources/WebAuthn/Ceremonies/Registration/ResidentKeyRequirement.swift
Outdated
Show resolved
Hide resolved
- Updated `AuthenticatorSelection` to include custom encoding and decoding logic for `requireResidentKey`. - Improved documentation with references to the WebAuthn Level 3 Working Draft. - Added tests to validate the behavior of `requireResidentKey` during encoding and decoding processes.
|
91e1acf incorporates your suggested changes.
|
dimitribouniol
left a comment
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.
Just minor nits left, but otherwise looks good to me!
Sources/WebAuthn/Ceremonies/Registration/AuthenticatorSelection.swift
Outdated
Show resolved
Hide resolved
- Removed Codable conformance from the main struct and implemented it in an extension for better separation of concerns. - Updated tests to ensure JSON encoding produces sorted keys and matches expected output format.
|
Resolved in 12a09aa |
|
Thanks! |
First, I want to be up-front that this PR was created with AI assistance. My understanding of WebAuthn is limited.
The purpose of this PR is to add optional
AuthenticatorSelectionsupport toPublicKeyCredentialCreationOptions. Without authenticator selection, Chrome on Android 16 devices always creates device-locked passkeys that are not discoverable. By specifying that the server requires a client-side-resident credential, you can get Android to create the passkey in Google Password Manager, which makes it discoverable.Using this, I was able to get Android to support a passkey workflow that matches Safari.