Skip to content

Standardize form validation for WebauthnSetupForm #10782

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
aduth merged 2 commits into from
Jun 21, 2024
Merged

Standardize form validation for WebauthnSetupForm #10782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d9d118
Standardize form validation for WebauthnSetupForm
aduth Jun 7, 2024
3a19e1f
Add attestation validation error to attestation_object
aduth Jun 7, 2024
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
4 changes: 2 additions & 2 deletions app/controllers/users/webauthn_setup_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def confirm
user_session: user_session,
device_name: DeviceName.from_user_agent(request.user_agent),
)
result = form.submit(request.protocol, confirm_params)
result = form.submit(confirm_params)
@platform_authenticator = form.platform_authenticator?
@presenter = WebauthnSetupPresenter.new(
current_user: current_user,
Expand Down Expand Up @@ -161,7 +161,7 @@ def confirm_params
:name,
:platform_authenticator,
:transports,
)
).merge(protocol: request.protocol)
end
end
end
50 changes: 27 additions & 23 deletions app/forms/webauthn_setup_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class WebauthnSetupForm
:name,
presence: { message: proc { |object| object.send(:generic_error_message) } }
validate :name_is_unique
validate :validate_attestation_response

attr_reader :attestation_response

Expand All @@ -22,12 +23,13 @@ def initialize(user:, user_session:, device_name:)
@name = nil
@platform_authenticator = false
@authenticator_data_flags = nil
@protocol = nil
@device_name = device_name
end

def submit(protocol, params)
def submit(params)
consume_parameters(params)
success = valid? && valid_attestation_response?(protocol)
success = valid?
if success
create_webauthn_configuration
event = PushNotification::RecoveryInformationChangedEvent.new(user: user)
Expand Down Expand Up @@ -59,7 +61,7 @@ def generic_error_message

private

attr_reader :success, :transports, :invalid_transports
attr_reader :success, :transports, :invalid_transports, :protocol
attr_accessor :user, :challenge, :attestation_object, :client_data_json,
:name, :platform_authenticator, :authenticator_data_flags, :device_name

Expand All @@ -74,6 +76,7 @@ def consume_parameters(params)
@transports, @invalid_transports = params[:transports]&.split(',')&.partition do |transport|
WebauthnConfiguration::VALID_TRANSPORTS.include?(transport)
end
@protocol = params[:protocol]
end

def name_is_unique
Expand All @@ -94,32 +97,22 @@ def name_is_unique
end
end

def validate_attestation_response
return if valid_attestation_response?(protocol)
errors.add(:attestation_object, :invalid, message: general_error_message)
end

def valid_attestation_response?(protocol)
original_origin = "#{protocol}#{self.class.domain_name}"
@attestation_response = ::WebAuthn::AuthenticatorAttestationResponse.new(
attestation_object: Base64.decode64(@attestation_object),
client_data_json: Base64.decode64(@client_data_json),
)
safe_response("#{protocol}#{self.class.domain_name}")
end

def safe_response(original_origin)
response = @attestation_response.valid?(@challenge.pack('c*'), original_origin)
add_attestation_error unless response
response
rescue StandardError
add_attestation_error
false
end

def add_attestation_error
if @platform_authenticator
errors.add :name, I18n.t('errors.webauthn_platform_setup.general_error'),
type: :attestation_error
else
errors.add :name, I18n.t(
'errors.webauthn_setup.general_error_html',
link_html: I18n.t('errors.webauthn_setup.additional_methods_link'),
), type: :attestation_error
begin
attestation_response.valid?(@challenge.pack('c*'), original_origin)
rescue StandardError
false
end
end

Expand Down Expand Up @@ -151,6 +144,17 @@ def create_webauthn_configuration
)
end

def general_error_message
if platform_authenticator
I18n.t('errors.webauthn_platform_setup.general_error')
else
I18n.t(
'errors.webauthn_setup.general_error_html',
link_html: I18n.t('errors.webauthn_setup.additional_methods_link'),
)
end
end

def mfa_user
@mfa_user ||= MfaContext.new(user)
end
Expand Down
6 changes: 4 additions & 2 deletions spec/controllers/users/webauthn_setup_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,10 @@
'Multi-Factor Authentication Setup',
{
enabled_mfa_methods_count: 0,
errors: { name: [I18n.t('errors.webauthn_platform_setup.general_error')] },
error_details: { name: { attestation_error: true } },
errors: {
attestation_object: [I18n.t('errors.webauthn_platform_setup.general_error')],
},
error_details: { attestation_object: { invalid: true } },
in_account_creation_flow: false,
mfa_method_counts: {},
multi_factor_auth_method: 'webauthn_platform',
Expand Down
57 changes: 33 additions & 24 deletions spec/forms/webauthn_setup_form_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
platform_authenticator: false,
transports: 'usb',
authenticator_data_value: '153',
protocol:,
}
end
let(:subject) { WebauthnSetupForm.new(user:, user_session:, device_name:) }
Expand All @@ -41,7 +42,7 @@
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}

expect(subject.submit(protocol, params).to_h).to eq(
expect(subject.submit(params).to_h).to eq(
success: true,
errors: {},
**extra_attributes,
Expand All @@ -57,7 +58,7 @@
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::RecoveryInformationChangedEvent.new(user: user))

subject.submit(protocol, params)
subject.submit(params)
end

context 'with platform authenticator' do
Expand All @@ -66,7 +67,7 @@
end

it 'creates a platform authenticator' do
result = subject.submit(protocol, params)
result = subject.submit(params)
expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn_platform'

user.reload
Expand All @@ -81,7 +82,7 @@
let(:params) { super().merge(authenticator_data_value: '65') }

it 'includes data flags with bs set as false ' do
result = subject.submit(protocol, params)
result = subject.submit(params)

expect(result.to_h[:authenticator_data_flags]).to eq(
up: true,
Expand All @@ -98,7 +99,7 @@
let(:params) { super().merge(authenticator_data_value: 'bad_error') }

it 'should not include authenticator data flag' do
result = subject.submit(protocol, params)
result = subject.submit(params)

expect(result.to_h[:authenticator_data_flags]).to be_nil
end
Expand All @@ -108,7 +109,7 @@
let(:params) { super().merge(authenticator_data_value: nil) }

it 'should not include authenticator data flag' do
result = subject.submit(protocol, params)
result = subject.submit(params)

expect(result.to_h[:authenticator_data_flags]).to be_nil
end
Expand All @@ -119,15 +120,15 @@
let(:params) { super().merge(transports: 'wrong') }

it 'creates a webauthn configuration without transports' do
subject.submit(protocol, params)
subject.submit(params)

user.reload

expect(user.webauthn_configurations.roaming_authenticators.first.transports).to be_nil
end

it 'includes unknown transports in extra analytics' do
result = subject.submit(protocol, params)
result = subject.submit(params)

expect(result.to_h).to eq(
success: true,
Expand Down Expand Up @@ -169,13 +170,17 @@
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}

expect(subject.submit(protocol, params).to_h).to eq(
expect(subject.submit(params).to_h).to eq(
success: false,
errors: { name: [I18n.t(
'errors.webauthn_setup.general_error_html',
link_html: I18n.t('errors.webauthn_setup.additional_methods_link'),
)] },
error_details: { name: { attestation_error: true } },
errors: {
attestation_object: [
I18n.t(
'errors.webauthn_setup.general_error_html',
link_html: I18n.t('errors.webauthn_setup.additional_methods_link'),
),
],
},
error_details: { attestation_object: { invalid: true } },
**extra_attributes,
)
end
Expand All @@ -185,7 +190,7 @@
let(:params) { super().except(:transports) }

it 'creates a webauthn configuration without transports' do
subject.submit(protocol, params)
subject.submit(params)

user.reload

Expand Down Expand Up @@ -214,13 +219,17 @@
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}

expect(subject.submit(protocol, params).to_h).to eq(
expect(subject.submit(params).to_h).to eq(
success: false,
errors: { name: [I18n.t(
'errors.webauthn_setup.general_error_html',
link_html: I18n.t('errors.webauthn_setup.additional_methods_link'),
)] },
error_details: { name: { attestation_error: true } },
errors: {
attestation_object: [
I18n.t(
'errors.webauthn_setup.general_error_html',
link_html: I18n.t('errors.webauthn_setup.additional_methods_link'),
),
],
},
error_details: { attestation_object: { invalid: true } },
**extra_attributes,
)
end
Expand All @@ -234,7 +243,7 @@
user
end
it 'checks for unique device on a webauthn device' do
result = subject.submit(protocol, params)
result = subject.submit(params)
expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn'
expect(result.errors[:name]).to eq(
[I18n.t(
Expand Down Expand Up @@ -263,7 +272,7 @@
end

it 'adds a new platform device with the same existing name and appends a (1)' do
result = subject.submit(protocol, params)
result = subject.submit(params)
expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn_platform'
expect(user.webauthn_configurations.platform_authenticators.count).to eq(2)
expect(
Expand All @@ -289,7 +298,7 @@
end

it 'adds a second new platform device with the same existing name and appends a (2)' do
result = subject.submit(protocol, params)
result = subject.submit(params)

expect(result.success?).to eq(true)
expect(user.webauthn_configurations.platform_authenticators.count).to eq(3)
Expand Down