Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/devise/models/webauthn_credential_authenticatable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module WebauthnCredentialAuthenticatable

validates :webauthn_id, uniqueness: true, allow_blank: true

after_initialize do
before_validation do
self.webauthn_id ||= WebAuthn.generate_user_id
end
end
Expand Down
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have the generated migration on the internal app?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have the migrations in our internal app 😕

Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# frozen_string_literal: true

class AddWebauthnIdTo<%= user_table_name.camelize %> < ActiveRecord::Migration[<%= Rails.version.to_f %>]
def up
add_column :<%= user_table_name %>, :webauthn_id, :string
add_index :<%= user_table_name %>, :webauthn_id, unique: true

# WARNING: The code below backfills webauthn_id for all existing records
# one row at a time. For larger tables, consider removing it and running
# the backfill separately (e.g., in a background job or maintenance task).
#
# Worth noting: PostgreSQL and MySQL support single-query backfills:
#
# PostgreSQL:
# UPDATE <%= user_table_name %> SET webauthn_id = encode(gen_random_bytes(64), 'base64') WHERE webauthn_id IS NULL
#
# MySQL:
# UPDATE <%= user_table_name %> SET webauthn_id = TO_BASE64(RANDOM_BYTES(64)) WHERE webauthn_id IS NULL
#
Comment on lines +8 to +19
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RenzoMinelli I'm hesitant over mentioning in the code the option of setting the default in the database – as the value that we are trying to set is not static, I think it will trigger a full rewrite of the database which would lock the table on most database engines 😕

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah looking into this it would be basically the same as what's presented here, needs to update each row individually. Okay sounds good

execute("SELECT id FROM <%= user_table_name %> WHERE webauthn_id IS NULL").each do |row|
webauthn_id = WebAuthn.generate_user_id
execute(ActiveRecord::Base.sanitize_sql_array(
["UPDATE <%= user_table_name %> SET webauthn_id = ? WHERE id = ?", webauthn_id, row["id"]]
))
end
# Note: if your application creates records using methods that skip
# callbacks (e.g., insert_all), consider adding a database default
# to ensure webauthn_id is always set. For example, in PostgreSQL:
#
# change_column_default :<%= user_table_name %>, :webauthn_id, from: nil, to: -> { "encode(gen_random_bytes(64), 'base64')" }
#
# For the same reason, you may want to add a NOT NULL constraint in a
# separate migration after the backfill is complete:
#
# change_column_null :<%= user_table_name %>, :webauthn_id, false
end

def down
remove_column :<%= user_table_name %>, :webauthn_id
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,22 @@
module Devise
module Webauthn
class WebauthnIdGenerator < Rails::Generators::Base
include Rails::Generators::Migration

hide!
namespace "devise:webauthn:webauthn_id"

source_root File.expand_path("templates", __dir__)

desc "Add webauthn_id field to User model"
class_option :resource_name, type: :string, default: "user", desc: "The resource name for Devise (default: user)"

def self.next_migration_number(dirname)
ActiveRecord::Generators::Base.next_migration_number(dirname)
end

def generate_migration
invoke "active_record:migration", [
"add_webauthn_id_to_#{user_table_name}",
"webauthn_id:string:uniq"
]
migration_template "add_webauthn_id.rb.erb", "db/migrate/add_webauthn_id_to_#{user_table_name}.rb"
end

def show_instructions
Expand Down
24 changes: 19 additions & 5 deletions spec/devise/models/passkey_authenticatable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,30 @@

RSpec.describe Devise::Models::PasskeyAuthenticatable, type: :model do
describe "webauthn_id initialization" do
it "generates a webauthn_id on initialize" do
user = Account.new(email: "user@example.com", password: "password", password_confirmation: "password")
it "generates a webauthn_id on create" do
user = Account.create!(email: "user@example.com", password: "password", password_confirmation: "password")
expect(user.webauthn_id).to be_present
end

it "keeps existing webauthn_id" do
user = Account.new(email: "user@example.com", password: "password", password_confirmation: "password",
webauthn_id: "custom")
it "does not generate a webauthn_id on initialize" do
user = Account.new(email: "user@example.com", password: "password", password_confirmation: "password")
expect(user.webauthn_id).to be_nil
end

it "keeps webauthn_id if created with one" do
user = Account.create!(email: "user@example.com", password: "password", password_confirmation: "password",
webauthn_id: "custom")
expect(user.webauthn_id).to eq("custom")
end

it "generates a webauthn_id on update if missing" do
user = Account.create!(email: "user@example.com", password: "password", password_confirmation: "password")
user.update_column(:webauthn_id, nil) # rubocop:disable Rails/SkipsModelValidations
user.reload

user.update!(email: "updated@example.com")
expect(user.webauthn_id).to be_present
end
end

describe "associations" do
Expand Down
24 changes: 19 additions & 5 deletions spec/devise/models/webauthn_two_factor_authenticatable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,30 @@

RSpec.describe Devise::Models::WebauthnTwoFactorAuthenticatable, type: :model do
describe "webauthn_id initialization" do
it "generates a webauthn_id on initialize" do
user = Account.new(email: "user@example.com", password: "password", password_confirmation: "password")
it "generates a webauthn_id on create" do
user = Account.create!(email: "user@example.com", password: "password", password_confirmation: "password")
expect(user.webauthn_id).to be_present
end

it "keeps existing webauthn_id" do
user = Account.new(email: "user@example.com", password: "password", password_confirmation: "password",
webauthn_id: "custom")
it "does not generate a webauthn_id on initialize" do
user = Account.new(email: "user@example.com", password: "password", password_confirmation: "password")
expect(user.webauthn_id).to be_nil
end

it "keeps webauthn_id if created with one" do
user = Account.create!(email: "user@example.com", password: "password", password_confirmation: "password",
webauthn_id: "custom")
expect(user.webauthn_id).to eq("custom")
end

it "generates a webauthn_id on update if missing" do
user = Account.create!(email: "user@example.com", password: "password", password_confirmation: "password")
user.update_column(:webauthn_id, nil) # rubocop:disable Rails/SkipsModelValidations
user.reload

user.update!(email: "updated@example.com")
expect(user.webauthn_id).to be_present
end
end

describe "associations" do
Expand Down
24 changes: 17 additions & 7 deletions spec/generators/devise/webauthn/webauthn_id_generator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,35 @@

before do
prepare_destination
allow(generator_instance).to receive(:invoke)
invoke generator_instance
end

context "when using default resource name" do
let(:generator_instance) { generator }

it "invokes the active_record:migration generator with correct arguments" do
expect(generator).to have_received(:invoke).with("active_record:migration",
["add_webauthn_id_to_users", "webauthn_id:string:uniq"])
it "creates a migration that adds webauthn_id to users" do
assert_migration "db/migrate/add_webauthn_id_to_users.rb" do |migration|
assert_match(/add_column :users, :webauthn_id, :string/, migration)
assert_match(/add_index :users, :webauthn_id, unique: true/, migration)
end
end

it "creates a migration that backfills existing records" do
assert_migration "db/migrate/add_webauthn_id_to_users.rb" do |migration|
assert_match(/SELECT id FROM users WHERE webauthn_id IS NULL/, migration)
assert_match(/WebAuthn\.generate_user_id/, migration)
end
end
end

context "when using a custom resource name" do
let(:generator_instance) { generator([destination_root], ["--resource_name=admin"]) }

it "invokes the active_record:migration generator with correct arguments" do
expect(generator).to have_received(:invoke).with("active_record:migration",
["add_webauthn_id_to_admins", "webauthn_id:string:uniq"])
it "creates a migration that adds webauthn_id to admins" do
assert_migration "db/migrate/add_webauthn_id_to_admins.rb" do |migration|
assert_match(/add_column :admins, :webauthn_id, :string/, migration)
assert_match(/add_index :admins, :webauthn_id, unique: true/, migration)
end
end
end
end