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
20 changes: 6 additions & 14 deletions app/controllers/users/webauthn_setup_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def confirm
result = form.submit(request.protocol, params)
analytics.track_event(Analytics::WEBAUTHN_SETUP_SUBMITTED, result.to_h)
if result.success?
process_valid_webauthn(form.attestation_response)
process_valid_webauthn
else
process_invalid_webauthn(form)
end
Expand Down Expand Up @@ -46,6 +46,7 @@ def exclude_credentials
end

def handle_successful_delete
create_user_event(:webauthn_key_removed)
WebauthnConfiguration.where(user_id: current_user.id, id: params[:id]).destroy_all
flash[:success] = t('notices.webauthn_deleted')
track_delete(true)
Expand All @@ -57,10 +58,12 @@ def handle_failed_delete
end

def track_delete(success)
counts_hash = MfaContext.new(current_user.reload).enabled_two_factor_configuration_counts_hash

analytics.track_event(
Analytics::WEBAUTHN_DELETED,
success: success,
mfa_options_enabled: MfaContext.new(current_user).enabled_two_factor_configurations_count
mfa_method_counts: counts_hash
)
end

Expand All @@ -73,9 +76,8 @@ def two_factor_enabled?
MfaPolicy.new(current_user).two_factor_enabled?
end

def process_valid_webauthn(attestation_response)
def process_valid_webauthn
mark_user_as_fully_authenticated
create_webauthn_configuration(attestation_response)
redirect_to webauthn_setup_success_url
end

Expand Down Expand Up @@ -103,16 +105,6 @@ def mark_user_as_fully_authenticated
user_session[:authn_at] = Time.zone.now
end

def create_webauthn_configuration(attestation_response)
credential = attestation_response.credential
public_key = Base64.strict_encode64(credential.public_key)
id = Base64.strict_encode64(credential.id)
WebauthnConfiguration.create(user_id: current_user.id,
credential_public_key: public_key,
credential_id: id,
name: params[:name])
end

def user_already_has_a_personal_key?
TwoFactorAuthentication::PersonalKeyPolicy.new(current_user).configured?
end
Expand Down
25 changes: 24 additions & 1 deletion app/forms/webauthn_setup_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ def initialize(user, user_session)
def submit(protocol, params)
consume_parameters(params)
success = valid? && valid_attestation_response?(protocol)
FormResponse.new(success: success, errors: errors.messages)
if success
create_webauthn_configuration
create_user_event
end

FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes)
end

# this gives us a hook to override the domain embedded in the attestation test object
Expand Down Expand Up @@ -61,4 +66,22 @@ def safe_response(original_origin)
errors.add :name, I18n.t('errors.webauthn_setup.attestation_error')
false
end

def create_webauthn_configuration
credential = attestation_response.credential
public_key = Base64.strict_encode64(credential.public_key)
id = Base64.strict_encode64(credential.id)
WebauthnConfiguration.create(user_id: user.id,
credential_public_key: public_key,
credential_id: id,
name: name)
end

def create_user_event
Event.create(user_id: user.id, event_type: :webauthn_key_added)
end

def extra_analytics_attributes
{ mfa_method_counts: MfaContext.new(user).enabled_two_factor_configuration_counts_hash }
end
end
2 changes: 2 additions & 0 deletions app/models/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ class Event < ApplicationRecord
piv_cac_disabled: 11,
new_personal_key: 12,
personal_key_used: 13,
webauthn_key_added: 14,
webauthn_key_removed: 15,
}

validates :event_type, presence: true
Expand Down
2 changes: 2 additions & 0 deletions config/locales/event_types/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ en:
piv_cac_disabled: PIV/CAC card unassociated
piv_cac_enabled: PIV/CAC card associated
usps_mail_sent: Letter sent
webauthn_key_added: Hardware security key added
webauthn_key_removed: Hardware security key removed
2 changes: 2 additions & 0 deletions config/locales/event_types/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ es:
piv_cac_disabled: Tarjeta PIV/CAC no asociada
piv_cac_enabled: Tarjeta PIV/CAC asociada
usps_mail_sent: Carta enviada
webauthn_key_added: Clave de seguridad de hardware añadido
webauthn_key_removed: Clave de seguridad de hardware eliminada
2 changes: 2 additions & 0 deletions config/locales/event_types/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ fr:
piv_cac_disabled: Carte PIV/CAC non associée
piv_cac_enabled: Carte PIV/CAC associée
usps_mail_sent: Lettre envoyée
webauthn_key_added: Clé de sécurité ajoutée
webauthn_key_removed: Clé de sécurité retirée
42 changes: 15 additions & 27 deletions spec/controllers/users/webauthn_setup_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
describe 'when signed in and not account creation' do
before do
stub_analytics
user = build(:user, personal_key: 'ABCD-DEFG-HIJK-LMNO')
user = build(:user, :signed_up, :with_authentication_app)
stub_sign_in(user)
end

Expand Down Expand Up @@ -88,7 +88,7 @@
end

it 'tracks the submission' do
result = { success: true, errors: {} }
result = { success: true, errors: {}, mfa_method_counts: { auth_app: 1, phone: 1 } }
expect(@analytics).to receive(:track_event).
with(Analytics::WEBAUTHN_SETUP_SUBMITTED, result)

Expand All @@ -97,19 +97,11 @@
end

describe 'delete' do
before do
mock_mfa = MfaContext.new(controller.current_user)
allow(mock_mfa).to receive(:enabled_two_factor_configurations_count).and_return(2)
allow(MfaContext).to receive(:new).with(controller.current_user).and_return(mock_mfa)
mock_mfa_policy = MfaPolicy.new(controller.current_user)
allow(mock_mfa_policy).to receive(:multiple_factors_enabled?).and_return(true)
allow(MfaPolicy).to receive(:new).with(
controller.current_user
).and_return(mock_mfa_policy)
end

it 'deletes a webauthn configuration' do
expect(Event).to receive(:create).
with(user_id: controller.current_user.id, event_type: :webauthn_key_removed)
cfg = create_webauthn_configuration(controller.current_user, 'key1', 'id1', 'foo1')

delete :delete, params: { id: cfg.id }

expect(response).to redirect_to(account_url)
Expand All @@ -120,7 +112,7 @@
it 'tracks the delete' do
cfg = create_webauthn_configuration(controller.current_user, 'key1', 'id1', 'foo1')

result = { success: true, mfa_options_enabled: 2 }
result = { success: true, mfa_method_counts: { auth_app: 1, phone: 1 } }
expect(@analytics).to receive(:track_event).with(Analytics::WEBAUTHN_DELETED, result)

delete :delete, params: { id: cfg.id }
Expand All @@ -129,25 +121,21 @@
end

describe 'when signed in and account creation' do
before do
user = build(:user)
stub_sign_in(user)
end

describe 'patch confirm' do
let(:params) do
{
it 'processes a valid webauthn and redirects to success page' do
user = build(:user)
stub_sign_in(user)

allow(Figaro.env).to receive(:domain_name).and_return('localhost:3000')
expect(Event).to receive(:create).with(user_id: user.id, event_type: :webauthn_key_added)
controller.user_session[:webauthn_challenge] = challenge

params = {
attestation_object: attestation_object,
client_data_json: client_data_json,
name: 'mykey',
}
end
before do
allow(Figaro.env).to receive(:domain_name).and_return('localhost:3000')
controller.user_session[:webauthn_challenge] = challenge
end

it 'processes a valid webauthn and redirects to success page' do
patch :confirm, params: params

expect(response).to redirect_to(webauthn_setup_success_url)
Expand Down
5 changes: 5 additions & 0 deletions spec/features/users/webauthn_management_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
mock_press_button_on_hardware_key

expect(current_path).to eq webauthn_setup_success_path

click_button t('forms.buttons.continue')

expect(page).to have_current_path(account_path)
expect(page).to have_content t('event_types.webauthn_key_added')
end

it 'gives an error if the challenge/secret is incorrect' do
Expand Down
9 changes: 6 additions & 3 deletions spec/forms/webauthn_setup_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
client_data_json: client_data_json,
name: 'mykey',
}
extra_attributes = { mfa_method_counts: { webauthn: 1 } }

expect(FormResponse).to receive(:new).
with(success: true, errors: {}).and_return(result)
with(success: true, errors: {}, extra: extra_attributes).and_return(result)
expect(subject.submit(protocol, params)).to eq result
end
end
Expand All @@ -32,9 +33,10 @@
client_data_json: client_data_json,
name: 'mykey',
}
extra_attributes = { mfa_method_counts: {} }

expect(FormResponse).to receive(:new).
with(success: false, errors: {}).and_return(result)
with(success: false, errors: {}, extra: extra_attributes).and_return(result)
expect(subject.submit(protocol, params)).to eq result
end

Expand All @@ -48,9 +50,10 @@
client_data_json: client_data_json,
name: 'mykey',
}
extra_attributes = { mfa_method_counts: {} }

expect(FormResponse).to receive(:new).
with(success: false, errors:
with(success: false, extra: extra_attributes, errors:
{ name: [I18n.t('errors.webauthn_setup.attestation_error')] }).and_return(result)
expect(subject.submit(protocol, params)).to eq result
end
Expand Down