Skip to content

Refactor WebauthnVerificationForm to handle error messages #9613

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 1 commit into from
Nov 17, 2023
Merged

Refactor WebauthnVerificationForm to handle error messages #9613

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
22 changes: 6 additions & 16 deletions app/controllers/two_factor_authentication/webauthn_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def handle_webauthn_result(result)
if result.success?
handle_valid_webauthn
else
handle_invalid_webauthn
handle_invalid_webauthn(result)
end
end

Expand All @@ -54,24 +54,12 @@ def handle_valid_webauthn
redirect_to after_sign_in_path_for(current_user)
end

def handle_invalid_webauthn
def handle_invalid_webauthn(result)
flash[:error] = result.first_error_message

if platform_authenticator?
flash[:error] = t(
'two_factor_authentication.webauthn_error.try_again',
link: view_context.link_to(
t('two_factor_authentication.webauthn_error.additional_methods_link'),
login_two_factor_options_path,
),
)
redirect_to login_two_factor_webauthn_url(platform: 'true')
else
flash[:error] = t(
'two_factor_authentication.webauthn_error.connect_html',
link_html: view_context.link_to(
t('two_factor_authentication.webauthn_error.additional_methods_link'),
login_two_factor_options_path,
),
)
redirect_to login_two_factor_webauthn_url
end
end
Expand Down Expand Up @@ -124,6 +112,8 @@ def analytics_properties
def form
@form ||= WebauthnVerificationForm.new(
user: current_user,
platform_authenticator: platform_authenticator?,
url_options:,
challenge: user_session[:webauthn_challenge],
protocol: request.protocol,
authenticator_data: params[:authenticator_data],
Expand Down
52 changes: 42 additions & 10 deletions app/forms/webauthn_verification_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
# The WebauthnVerificationForm class is responsible for validating webauthn verification input
class WebauthnVerificationForm
include ActiveModel::Model
include ActionView::Helpers::UrlHelper
include ActionView::Helpers::TranslationHelper
include Rails.application.routes.url_helpers

validates :user, presence: true
validates :challenge, presence: true
validates :authenticator_data, presence: true
validates :client_data_json, presence: true
validates :signature, presence: true
validates :webauthn_configuration, presence: true
validates :challenge,
:authenticator_data,
:client_data_json,
:signature,
:webauthn_configuration,
presence: { message: proc { |object| object.instance_eval { generic_error_message } } }
validate :validate_assertion_response
validate :validate_webauthn_error

attr_reader :url_options, :platform_authenticator

alias_method :platform_authenticator?, :platform_authenticator

def initialize(
user:,
platform_authenticator:,
url_options:,
protocol:,
user: nil,
challenge: nil,
authenticator_data: nil,
client_data_json: nil,
Expand All @@ -22,6 +31,9 @@ def initialize(
webauthn_error: nil
)
@user = user
@platform_authenticator = platform_authenticator
@url_options = url_options
@protocol = protocol
@challenge = challenge
@protocol = protocol
@authenticator_data = authenticator_data
Expand All @@ -43,7 +55,7 @@ def submit

def webauthn_configuration
return @webauthn_configuration if defined?(@webauthn_configuration)
@webauthn_configuration = user&.webauthn_configurations&.find_by(credential_id: credential_id)
@webauthn_configuration = user.webauthn_configurations.find_by(credential_id: credential_id)
end

# this gives us a hook to override the domain embedded in the attestation test object
Expand All @@ -64,12 +76,12 @@ def self.domain_name

def validate_assertion_response
return if webauthn_error.present? || webauthn_configuration.blank? || valid_assertion_response?
errors.add(:authenticator_data, 'invalid_authenticator_data', type: :invalid_authenticator_data)
errors.add(:authenticator_data, :invalid_authenticator_data, message: generic_error_message)
Copy link
Contributor Author

@aduth aduth Nov 17, 2023

Choose a reason for hiding this comment

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

This inverts the "type" and "message", where in most other code we'd do the following:

Suggested change
errors.add(:authenticator_data, :invalid_authenticator_data, message: generic_error_message)
errors.add(:authenticator_data, generic_error_message, type: :invalid_authenticator_data)

As mentioned in #9569, I think the pattern in the suggested code above is not really expected to be supported by Rails' ActiveModel::Errors#add method, which is documented as receiving type as the second parameter, and optionally a custom message option. The code proposed here follows the documented usage, and I'd be interested to consider porting existing code to this pattern.

end

def validate_webauthn_error
return if webauthn_error.blank?
errors.add(:webauthn_error, webauthn_error, type: :webauthn_error)
errors.add(:webauthn_error, :webauthn_error, message: generic_error_message)
end

def valid_assertion_response?
Expand Down Expand Up @@ -99,6 +111,26 @@ def public_key
webauthn_configuration&.credential_public_key
end

def generic_error_message
if platform_authenticator?
t(
'two_factor_authentication.webauthn_error.try_again',
link: link_to(
t('two_factor_authentication.webauthn_error.additional_methods_link'),
login_two_factor_options_path,
),
)
else
t(
'two_factor_authentication.webauthn_error.connect_html',
link_html: link_to(
t('two_factor_authentication.webauthn_error.additional_methods_link'),
login_two_factor_options_path,
),
)
end
end

def extra_analytics_attributes
auth_method = if webauthn_configuration&.platform_authenticator
'webauthn_platform'
Expand Down
16 changes: 3 additions & 13 deletions spec/forms/webauthn_verification_form_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'rails_helper'

RSpec.describe WebauthnVerificationForm do
include Rails.application.routes.url_helpers
include WebAuthnHelper

let(:user) { create(:user) }
Expand All @@ -22,6 +23,8 @@
subject(:form) do
WebauthnVerificationForm.new(
user: user,
platform_authenticator:,
url_options: {},
challenge: challenge,
protocol: protocol,
authenticator_data: authenticator_data,
Expand Down Expand Up @@ -62,19 +65,6 @@
end

context 'when the input is invalid' do
context 'when user is missing' do
let(:user) { nil }

it 'returns unsuccessful result' do
expect(result.to_h).to eq(
success: false,
error_details: { user: { blank: true }, webauthn_configuration: { blank: true } },
multi_factor_auth_method: 'webauthn',
webauthn_configuration_id: nil,
)
end
end

context 'when challenge is missing' do
let(:challenge) { nil }

Expand Down