Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements passkey authentication (WebAuthn) for the application, allowing users to sign in using biometric authentication, security keys, or platform authenticators instead of traditional passwords or email codes.
Changes:
- Adds a complete WebAuthn library implementation in
lib/action_pack/web_authn/including CBOR decoding, COSE key handling, and authenticator response validation - Implements
Identity::Credentialmodel for storing and managing user credentials with passkey registration and authentication methods - Adds UI for managing passkeys including registration, editing, and deletion of credentials
Reviewed changes
Copilot reviewed 51 out of 73 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/action_pack/web_authn/*.rb | Core WebAuthn library implementing CBOR decoding, COSE key parsing, and authenticator response validation |
| app/models/identity/credential.rb | Model for managing WebAuthn credentials with registration and authentication logic |
| app/models/identity/credential/authenticator.rb | Registry for mapping AAGUIDs to authenticator metadata (icons, names) |
| app/controllers/users/credentials_controller.rb | Controller for credential management (CRUD operations) |
| app/controllers/sessions/passkeys_controller.rb | Controller for passkey-based authentication |
| app/javascript/controllers/*.js | Stimulus controllers for credential registration and conditional mediation |
| app/views/users/credentials/*.html.erb | Views for credential management UI |
| db/migrate/*.rb | Migrations for identity_credentials table |
| config/passkey_aaguids.yml | Configuration mapping AAGUIDs to authenticator details |
| test/lib/action_pack/web_authn/*.rb | Comprehensive test coverage for WebAuthn library |
| test/controllers/sessions/passkeys_controller_test.rb | Integration tests for passkey authentication |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # | ||
| # # Look up the credential by ID | ||
| # credential = user.credentials.find_by!( | ||
| # credentail_id: params[:id] |
There was a problem hiding this comment.
Typo in comment: "credentail_id" should be "credential_id".
| class Identity::Credential < ApplicationRecord | ||
| belongs_to :identity | ||
|
|
||
| serialize :transports, coder: JSON, type: Array, default: [] | ||
|
|
||
| class << self | ||
| def creation_options(identity:, display_name:) | ||
| ActionPack::WebAuthn::PublicKeyCredential::CreationOptions.new( | ||
| id: identity.id, | ||
| name: identity.email_address, | ||
| display_name: display_name, | ||
| resident_key: :required, | ||
| exclude_credentials: identity.credentials.map(&:to_public_key_credential) | ||
| ) | ||
| end | ||
|
|
||
| def request_options(credentials: []) | ||
| ActionPack::WebAuthn::PublicKeyCredential::RequestOptions.new(credentials: credentials.map(&:to_public_key_credential)) | ||
| end | ||
|
|
||
| def register(passkey:, challenge:, origin: ActionPack::WebAuthn::Current.origin, **attributes) | ||
| public_key_credential = ActionPack::WebAuthn::PublicKeyCredential.create( | ||
| client_data_json: passkey[:client_data_json], | ||
| attestation_object: Base64.urlsafe_decode64(passkey[:attestation_object]), | ||
| challenge: challenge, | ||
| origin: origin, | ||
| transports: Array(passkey[:transports]) | ||
| ) | ||
|
|
||
| create!( | ||
| **attributes, | ||
| name: attributes.fetch(:name, Authenticator.find_by_aaguid(public_key_credential.aaguid)&.name), | ||
| credential_id: public_key_credential.id, | ||
| public_key: public_key_credential.public_key.to_der, | ||
| sign_count: public_key_credential.sign_count, | ||
| aaguid: public_key_credential.aaguid, | ||
| backed_up: public_key_credential.backed_up, | ||
| transports: public_key_credential.transports | ||
| ) | ||
| end | ||
|
|
||
| def authenticate(passkey:, challenge:, origin: ActionPack::WebAuthn::Current.origin) | ||
| find_by(credential_id: passkey[:id])&.authenticate(passkey: passkey, challenge: challenge, origin: origin) | ||
| end | ||
| end | ||
|
|
||
| def authenticate(passkey:, challenge:, origin: ActionPack::WebAuthn::Current.origin) | ||
| pkc = to_public_key_credential | ||
| pkc.authenticate( | ||
| client_data_json: passkey[:client_data_json], | ||
| authenticator_data: Base64.urlsafe_decode64(passkey[:authenticator_data]), | ||
| signature: Base64.urlsafe_decode64(passkey[:signature]), | ||
| challenge: challenge, | ||
| origin: origin | ||
| ) | ||
| update!(sign_count: pkc.sign_count, backed_up: pkc.backed_up) | ||
| self | ||
| rescue ActionPack::WebAuthn::Authenticator::Response::InvalidResponseError | ||
| nil | ||
| end | ||
|
|
||
| def authenticator | ||
| Authenticator.find_by_aaguid(aaguid) | ||
| end | ||
|
|
||
| def to_public_key_credential | ||
| ActionPack::WebAuthn::PublicKeyCredential.new( | ||
| id: credential_id, | ||
| public_key: OpenSSL::PKey.read(public_key), | ||
| sign_count: sign_count, | ||
| transports: transports | ||
| ) | ||
| end | ||
| end |
There was a problem hiding this comment.
Missing model tests for Identity::Credential. The codebase has comprehensive test coverage for other models (as seen in test/models/), but there are no tests for the newly added Identity::Credential model. Consider adding tests to cover:
- Credential creation and registration
- Credential authentication
- The authenticator lookup functionality
- Public key credential conversion
- Edge cases and error handling
dhh
left a comment
dhh
left a comment
There was a problem hiding this comment.
Very nice! Love how it's dependency free. But damn, even in Ruby, the complexity required is pretty intense. All the more benefit for us wrapping this neatly for all Rails users to use, though!
| challenge: session.delete(:webauthn_challenge) | ||
| ) | ||
|
|
||
| if credential |
There was a problem hiding this comment.
Small nit, and you could do it either way, but I think these extracted methods are a bit anemic. There's no reuse, and the job of the create action is to do the work and return. So I would inline them.
| Current.referrer = request.referrer | ||
|
|
||
| ActionPack::WebAuthn::Current.host = request.host | ||
| ActionPack::WebAuthn::Current.origin = request.base_url |
There was a problem hiding this comment.
It seems a bit much to have these included everywhere. Maybe do a current_web_authn_request.rb and only include that where they're needed?
| ) | ||
|
|
||
| render json: { location: edit_user_credential_path(Current.user, credential, created: true) } | ||
| rescue ActionPack::WebAuthn::Authenticator::Response::InvalidResponseError => error |
There was a problem hiding this comment.
This nested class is a bit much. I'd probably just go with something like Action::Pack::WebAuthn::InvalidAuthenticationResponseError or the like.
|
|
||
| def update | ||
| @credential.update!(credential_params) | ||
| redirect_to user_credentials_path(Current.user) |
There was a problem hiding this comment.
This should be going under my_credentials_path, as we don't need someone editing another person's credentials.
| REGISTRY = Rails.application.config_for(:passkey_aaguids).each_with_object({}) do |(_key, attrs), hash| | ||
| authenticator = new(name: attrs[:name], icon: attrs[:icon]) | ||
| attrs[:aaguids].each { |aaguid| hash[aaguid] = authenticator } | ||
| end.freeze |
There was a problem hiding this comment.
This is getting close to Ruby golf. Would try not to save so aggressively on lines and explain what's going on a little more.
| <div class="panel panel--centered flex flex-column gap-half" data-controller="passkey" | ||
| data-passkey-public-key-value="<%= @request_options.as_json.to_json %>" | ||
| data-passkey-url-value="<%= session_passkey_path %>" | ||
| data-passkey-csrf-token-value="<%= form_authenticity_token %>"> |
There was a problem hiding this comment.
Double indent attributes of an opening tag.
| <section class="panel panel--wide shadow center" | ||
| data-controller="credential" | ||
| data-credential-public-key-value="<%= @creation_options.as_json.to_json %>" | ||
| data-credential-register-url-value="<%= user_credentials_path(Current.user) %>"> |
| @@ -0,0 +1,17 @@ | |||
| class CreateIdentityCredentials < ActiveRecord::Migration[8.2] | |||
| def change | |||
| create_table :identity_credentials, id: :uuid do |t| | |||
There was a problem hiding this comment.
Feels like this could be generic and then belong to a polymorphic holder stand-in.
|
|
||
| unless sign_count_increased? | ||
| raise InvalidResponseError, "Sign count did not increase" | ||
| end |
There was a problem hiding this comment.
Something a bit awkward here. Would extract these into something like validate_client_data_type, validate_signature, validate_sign_count_increase.
| false | ||
| end | ||
|
|
||
| def validate!(challenge:, origin:, user_verification: :preferred) |
There was a problem hiding this comment.
I think Rails actually has some generic validator setup that you could consider mixing in.
No description provided.