Skip to content

Reset webauthn devices in reset 2FA action #6125

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.

Sign up for GitHub

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

Merged
jenshenny merged 1 commit into from
Dec 9, 2025
+20 −9
Merged

Reset webauthn devices in reset 2FA action #6125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions app/avo/actions/reset_user_2fa.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@ class Avo::Actions::ResetUser2fa < Avo::Actions::ApplicationAction
}

self.message = lambda {
"Are you sure you would like to disable MFA and reset the password for #{record.handle} #{record.email}?"
"Are you sure you would like to disable MFA, WebAuthn devices, and reset the password for #{record.handle} #{record.email}?"
}

self.confirm_button_label = "Reset MFA"

class ActionHandler < Avo::Actions::ActionHandler
def handle_record(user)
user.disable_totp!
user.disable_totp! if user.totp_enabled?
user.webauthn_credentials.destroy_all if user.webauthn_enabled?
Copy link
Member

@colby-swandale colby-swandale Dec 8, 2025

Choose a reason for hiding this comment

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

Using destroy_all is going to trigger the disable_user_mfa & send_deletion_email callback for each webauthn device. Considering this is an admin action, should we switch this to be delete_all to skip the callbacks? (I suspect this will also resolve your comment below)

Copy link
Member

@colby-swandale colby-swandale Dec 8, 2025

Choose a reason for hiding this comment

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

Should we consider moving the idea of resetting MFA (OTP + WebAuthN) into a User#reset_all_mfa method to better capture everything and test for it?

Copy link
Member

@colby-swandale colby-swandale Dec 8, 2025

Choose a reason for hiding this comment

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

Should we also consider wrapping this all in a transaction?

Copy link
Member Author

@jenshenny jenshenny Dec 9, 2025

Choose a reason for hiding this comment

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

The avo action is wrapped in a transaction so I think we're fine on that part.

I tried to move everything into reset_all_mfa , have a delete_all, and only send one email instead of one per device. However, the audits are subscribed to active_record.sql events and won't capture the delete_all webauthn cred records in the audit. It's going to take me a bit longer to implement getting these deleted records in the audit so I'm going to ship this as is to unblock the support request and come back to this after.

user.reset_mfa_attributes

user.password = SecureRandom.hex(20).encode("UTF-8")
user.save!
end
Expand Down
6 changes: 6 additions & 0 deletions app/models/concerns/user_multifactor_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ def mfa_method_added(default_level)
self.mfa_hashed_recovery_codes = new_mfa_recovery_codes.map { |code| BCrypt::Password.create(code) }
end

def reset_mfa_attributes
self.mfa_level = "disabled"
self.new_mfa_recovery_codes = nil
self.mfa_hashed_recovery_codes = []
end

private

def strong_mfa_level?
Expand Down
5 changes: 1 addition & 4 deletions app/models/concerns/user_totp_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ def totp_disabled?
def disable_totp!
self.totp_seed = nil

if no_mfa_devices?
self.mfa_level = "disabled"
self.mfa_hashed_recovery_codes = []
end
reset_mfa_attributes if no_mfa_devices?

save!(validate: false)
Mailer.totp_disabled(id, Time.now.utc).deliver_later
Expand Down
4 changes: 1 addition & 3 deletions app/models/webauthn_credential.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ def enable_user_mfa

def disable_user_mfa
return unless user.no_mfa_devices?
Copy link
Member Author

@jenshenny jenshenny Dec 3, 2025

Choose a reason for hiding this comment

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

For some reason, when calling destroy_all, the webauthn credential still persists in the association so destroying the last credential would not result in disabling mfa.

I called reset_mfa_attributes in the admin action itself, even though it is repetitive, it ensures the action actually correctly removed all MFA methods.

user.mfa_level = :disabled
user.new_mfa_recovery_codes = nil
user.mfa_hashed_recovery_codes = []
user.reset_mfa_attributes
user.save!(validate: false)
end
end
7 changes: 7 additions & 0 deletions test/system/avo/users_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ class Avo::UsersSystemTest < ApplicationSystemTestCase

user = create(:user)
user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
webauthn_credential = create(:webauthn_credential, user:)
user_attributes = user.attributes.with_indifferent_access
webauthn_attributes = webauthn_credential.attributes.with_indifferent_access

visit avo.resources_user_path(user)

Expand Down Expand Up @@ -41,6 +43,7 @@ class Avo::UsersSystemTest < ApplicationSystemTestCase
assert_not_equal user_attributes[:encrypted_password], user.encrypted_password
assert_nil user.totp_seed
assert_empty user.mfa_hashed_recovery_codes
assert_empty user.webauthn_credentials

audit = user.audits.sole
event = user.events.where(tag: Events::UserEvent::PASSWORD_CHANGED).sole
Expand All @@ -64,6 +67,10 @@ class Avo::UsersSystemTest < ApplicationSystemTestCase
.except("mfa_level", "updated_at", "totp_seed", "mfa_hashed_recovery_codes", "encrypted_password")
.transform_values(&:as_json)
},
"gid://gemcutter/WebauthnCredential/#{webauthn_credential.id}" => {
"changes" => webauthn_attributes.transform_values { [it.as_json, nil] },
"unchanged" => {}
},
event.to_gid.as_json => {
"changes" => event.attributes.transform_values { [nil, it.as_json] },
"unchanged" => {}
Expand Down
Loading