Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
1946209
changelog: User-Facing Improvements, Webauthn, Remove device nickname…
kevinsmaster5 Nov 24, 2023
b874bd7
use hidden field for platform authenticator nickname
kevinsmaster5 Nov 27, 2023
51405b1
bring device decorator into spec
kevinsmaster5 Nov 27, 2023
1633969
fix lint
kevinsmaster5 Nov 27, 2023
4180505
refactor how browser agent is captured
kevinsmaster5 Nov 28, 2023
b554950
remove fill in nickname helper from webauthn platform based test
kevinsmaster5 Nov 28, 2023
770db03
remove fill in nickname from test
kevinsmaster5 Nov 28, 2023
f1e22da
remove fill in nickname from test
kevinsmaster5 Nov 28, 2023
6ad2140
correct number of arguments error
kevinsmaster5 Nov 28, 2023
77422a9
rename method
kevinsmaster5 Nov 28, 2023
a932de3
rename method
kevinsmaster5 Nov 28, 2023
1d6fe16
reverse name logic to make it more readable
kevinsmaster5 Nov 28, 2023
d34542b
move device nice name to service class
kevinsmaster5 Nov 29, 2023
df02400
formatting, add device name service to device decorator
kevinsmaster5 Nov 29, 2023
0bb905f
append date string if device name is already set
kevinsmaster5 Nov 29, 2023
4b71df1
lint fix
kevinsmaster5 Nov 29, 2023
9cd6a85
make naming consistent for device
kevinsmaster5 Dec 1, 2023
070368c
remove hyphen from duplicate device naming
kevinsmaster5 Dec 4, 2023
e0cef14
revise form setup spec to match module
kevinsmaster5 Dec 4, 2023
f6eeac9
update device decorator spec to include device name service
kevinsmaster5 Dec 4, 2023
f1ce407
add test coverage for name_is_unique changes and change duplicate nam…
kevinsmaster5 Dec 4, 2023
da16b91
add test to validate form does not appear on webauth platform setup
kevinsmaster5 Dec 4, 2023
e55219a
use an incremental number to label duplicate device name
kevinsmaster5 Dec 6, 2023
8ff92ee
put device name service into its own spec
kevinsmaster5 Dec 6, 2023
dbfc6c9
fix increment syntax
kevinsmaster5 Dec 6, 2023
dce75fe
reorder spec pretext values and add context for non-platform webauthn
kevinsmaster5 Dec 6, 2023
4378967
correct naming increment syntax
kevinsmaster5 Dec 6, 2023
31c0ff8
revise test to account for duplicate webauthn platform names
kevinsmaster5 Dec 7, 2023
ef1a37f
lint fix
kevinsmaster5 Dec 7, 2023
e074e57
remove unneeded before do block
kevinsmaster5 Dec 7, 2023
8de32b1
remove unneeded code
kevinsmaster5 Dec 7, 2023
74b84e8
improve test coverage, remove redundant condition from setup
kevinsmaster5 Dec 7, 2023
eabe265
lint fix
kevinsmaster5 Dec 7, 2023
05ed26f
Update spec/services/device_name_spec.rb
kevinsmaster5 Dec 11, 2023
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
6 changes: 5 additions & 1 deletion app/controllers/users/webauthn_setup_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,11 @@ def new
end

def confirm
form = WebauthnSetupForm.new(current_user, user_session)
form = WebauthnSetupForm.new(
user: current_user,
user_session: user_session,
device_name: DeviceName.from_user_agent(request.user_agent),
)
result = form.submit(request.protocol, confirm_params)
@platform_authenticator = form.platform_authenticator?
@presenter = WebauthnSetupPresenter.new(
Expand Down
11 changes: 1 addition & 10 deletions app/decorators/device_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,6 @@ def happened_at
end

def nice_name
browser = BrowserCache.parse(device.user_agent)
os = browser.platform.name
os_version = browser.platform.version&.split('.')&.first
os = "#{os} #{os_version}" if os_version

I18n.t(
'account.index.device',
browser: "#{browser.name} #{browser.version}",
os: os,
)
DeviceName.from_user_agent(device.user_agent)
end
end
15 changes: 10 additions & 5 deletions app/forms/webauthn_setup_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class WebauthnSetupForm

attr_reader :attestation_response, :name_taken

def initialize(user, user_session)
def initialize(user:, user_session:, device_name:)
@user = user
@challenge = user_session[:webauthn_challenge]
@attestation_object = nil
Expand All @@ -19,6 +19,7 @@ def initialize(user, user_session)
@name = nil
@platform_authenticator = false
@authenticator_data_flags = nil
@device_name = device_name
end

def submit(protocol, params)
Expand Down Expand Up @@ -46,13 +47,13 @@ def platform_authenticator?

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

def consume_parameters(params)
@attestation_object = params[:attestation_object]
@client_data_json = params[:client_data_json]
@name = params[:name]
@platform_authenticator = (params[:platform_authenticator].to_s == 'true')
@name = @platform_authenticator ? @device_name : params[:name]
Comment thread
aduth marked this conversation as resolved.
Outdated
@authenticator_data_flags = process_authenticator_data_value(
params[:authenticator_data_value],
)
Expand All @@ -64,11 +65,15 @@ def consume_parameters(params)
def name_is_unique
return unless WebauthnConfiguration.exists?(user_id: @user.id, name: @name)
if @platform_authenticator
errors.add :name, I18n.t('errors.webauthn_platform_setup.unique_name'), type: :unique_name
num_existing_devices = WebauthnConfiguration.
where(user_id: @user.id).
where('name LIKE ?', "#{@name}%").
count
@name = "#{@name} (#{num_existing_devices})"
else
errors.add :name, I18n.t('errors.webauthn_setup.unique_name'), type: :unique_name
@name_taken = true
end
@name_taken = true
end

def valid_attestation_response?(protocol)
Expand Down
14 changes: 14 additions & 0 deletions app/services/device_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
class DeviceName
def self.from_user_agent(user_agent)
browser = BrowserCache.parse(user_agent)
os = browser.platform.name
os_version = browser.platform.version&.split('.')&.first
os = "#{os} #{os_version}" if os_version

I18n.t(
'account.index.device',
browser: "#{browser.name} #{browser.version}",
os: os,
)
end
end
30 changes: 17 additions & 13 deletions app/views/users/webauthn_setup/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,24 @@
<%= hidden_field_tag :client_data_json, '', id: 'client_data_json' %>
<%= hidden_field_tag :authenticator_data_value, '', id: 'authenticator_data_value' %>
<%= hidden_field_tag :transports, '', id: 'transports' %>

<%= hidden_field_tag :platform_authenticator, @platform_authenticator, id: 'platform_authenticator' %>
<%= render ValidatedFieldComponent.new(
form: f,
name: :name,
required: true,
label: @presenter.nickname_label,
hint: @presenter.device_nickname_hint,
input_html: {
id: 'nickname',
class: 'font-family-mono',
size: 16,
maxlength: 20,
},
) %>
<% if !@platform_authenticator %>
Comment thread
aduth marked this conversation as resolved.
Outdated

<%= render ValidatedFieldComponent.new(
form: f,
name: :name,
required: true,
label: @presenter.nickname_label,
hint: @presenter.device_nickname_hint,
input_html: {
id: 'nickname',
class: 'font-family-mono',
size: 16,
maxlength: 20,
},
) %>
<% end %>
<div class="margin-y-4 text-center" id="spinner" hidden>
<%= image_tag(
asset_url('loading-badge.gif'),
Expand Down
1 change: 0 additions & 1 deletion spec/features/remember_device/webauthn_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ def remember_device_and_sign_out_user

# webauthn option is hidden in browsers that don't support it
select_2fa_option('webauthn_platform', visible: :all)
fill_in_nickname_and_click_continue
check t('forms.messages.remember_device')
mock_press_button_on_hardware_key_on_setup

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@
click_continue
expect(page).to have_current_path webauthn_setup_path(platform: true)

fill_in_nickname_and_click_continue
mock_press_button_on_hardware_key_on_setup
expect(page).to have_current_path(auth_method_confirmation_path)
expect(page).to_not have_button(t('mfa.skip'))
Expand Down Expand Up @@ -287,7 +286,7 @@
select_2fa_option('webauthn_platform', visible: :all)

click_continue
fill_in_nickname_and_click_continue

mock_press_button_on_hardware_key_on_setup

click_link t('mfa.add')
Expand Down
2 changes: 0 additions & 2 deletions spec/features/webauthn/management_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ def expect_webauthn_platform_setup_error

# Regression: LG-9860: Ensure that the platform URL parameter is maintained through reauthn
travel_to (IdentityConfig.store.reauthn_window + 1).seconds.from_now
fill_in_nickname_and_click_continue
mock_press_button_on_hardware_key_on_setup

expect(page).to have_current_path login_two_factor_options_path
Expand All @@ -178,7 +177,6 @@ def expect_webauthn_platform_setup_error
click_submit_default

expect(page).to have_current_path webauthn_setup_path(platform: true)
fill_in_nickname_and_click_continue
mock_press_button_on_hardware_key_on_setup

expect_webauthn_platform_setup_success
Expand Down
88 changes: 87 additions & 1 deletion spec/forms/webauthn_setup_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

let(:user) { create(:user) }
let(:user_session) { { webauthn_challenge: webauthn_challenge } }
let(:device_name) { 'Chrome 119 on macOS 10' }
let(:domain_name) { 'localhost:3000' }
let(:params) do
{
Expand All @@ -16,7 +17,7 @@
authenticator_data_value: '153',
}
end
let(:subject) { WebauthnSetupForm.new(user, user_session) }
let(:subject) { WebauthnSetupForm.new(user:, user_session:, device_name:) }

before do
allow(IdentityConfig.store).to receive(:domain_name).and_return(domain_name)
Expand Down Expand Up @@ -221,4 +222,89 @@
end
end
end
describe '.name_is_unique' do
context 'webauthn' do
let(:user) do
user = create(:user)
user.webauthn_configurations << create(:webauthn_configuration, name: params[:name])
user
end
it 'checks for unique device on a webauthn device' do
result = subject.submit(protocol, params)
expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn'
expect(result.errors[:name]).to eq(
[I18n.t(
'errors.webauthn_setup.unique_name',
type: :unique_name,
)],
)
expect(result.to_h[:success]).to eq(false)
end
end
context 'webauthn_platform' do
context 'with one platform authenticator with the same name' do
let(:user) do
user = create(:user)
user.webauthn_configurations << create(
:webauthn_configuration,
name: device_name,
platform_authenticator: true,
transports: ['internal', 'hybrid'],
)
user
end
let(:params) do
super().merge(
platform_authenticator: true,
transports: 'internal,hybrid',
)
end
it 'adds a new platform device with the same existing name and appends a (1)' do
result = subject.submit(protocol, params)
expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn_platform'
expect(user.webauthn_configurations.platform_authenticators.count).to eq(2)
expect(
user.webauthn_configurations.platform_authenticators[1].name,
).
to eq("#{device_name} (1)")
expect(result.to_h[:success]).to eq(true)
end
end

context 'with two existing platform authenticators one with the same name' do
let(:user) do
user = create(:user)
user.webauthn_configurations << create(
:webauthn_configuration,
name: device_name,
platform_authenticator: true,
transports: ['internal', 'hybrid'],
)
user.webauthn_configurations << create(
:webauthn_configuration,
name: device_name,
platform_authenticator: true,
transports: ['internal', 'hybrid'],
)
user
end
let(:params) do
super().merge(
platform_authenticator: true,
transports: 'internal,hybrid',
)
end
it 'adds a second new platform device with the same existing name and appends a (2)' do
result = subject.submit(protocol, params)
expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn_platform'
expect(user.webauthn_configurations.platform_authenticators.count).to eq(3)
expect(
user.webauthn_configurations.platform_authenticators[2].name,
).
to eq("#{device_name} (2)")
expect(result.to_h[:success]).to eq(true)
end
end
end
end
end
11 changes: 11 additions & 0 deletions spec/services/device_name_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
require 'rails_helper'

RSpec.describe DeviceName do
describe '.device_name' do
let(:device) { create(:device) }
it 'gives a shortened os and browser name' do
name = DeviceName.from_user_agent(device.user_agent)
expect(name).to eq('Chrome 58 on Windows 10')
end
end
end
63 changes: 41 additions & 22 deletions spec/views/users/webauthn_setup/new.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,45 @@
RSpec.describe 'users/webauthn_setup/new.html.erb' do
let(:user) { create(:user, :fully_registered) }

let(:user_session) do
{ webauthn_challenge: 'fake_challenge' }
end
let(:presenter) do
WebauthnSetupPresenter.new(
current_user: user,
user_fully_authenticated: true,
user_opted_remember_device_cookie: true,
remember_device_default: true,
platform_authenticator: platform_authenticator,
url_options: {},
)
end

before do
allow(view).to receive(:current_user).and_return(user)
allow(view).to receive(:user_session).and_return(user_session)
allow(view).to receive(:in_multi_mfa_selection_flow?).and_return(false)
assign(:platform_authenticator, platform_authenticator)
assign(:user_session, user_session)
assign(:presenter, presenter)
end

context 'webauthn platform' do
let(:platform_authenticator) { true }
let(:user_session) do
{ webauthn_challenge: 'fake_challenge' }
end
let(:presenter) do
WebauthnSetupPresenter.new(
current_user: user,
user_fully_authenticated: true,
user_opted_remember_device_cookie: true,
remember_device_default: true,
platform_authenticator: platform_authenticator,
url_options: {},
)
end

before do
allow(view).to receive(:current_user).and_return(user)
allow(view).to receive(:user_session).and_return(user_session)
allow(view).to receive(:in_multi_mfa_selection_flow?).and_return(false)
assign(:platform_authenticator, platform_authenticator)
assign(:user_session, user_session)
assign(:presenter, presenter)
end

it 'has a localized title' do
expect(view).to receive(:title=).with(presenter.page_title)

render
end

it 'does not display the form' do
Comment thread
aduth marked this conversation as resolved.
render
expect(rendered).to_not have_content(
t('two_factor_authentication.two_factor_choice_options.webauthn'),
)
end

context 'when user selects multiple MFA options on account creation' do
before do
assign(:need_to_set_up_additional_mfa, false)
Expand Down Expand Up @@ -69,4 +78,14 @@
end
end
end

context 'non-platform webauthn' do
let(:platform_authenticator) { false }
it 'displays the form' do
render
expect(rendered).to have_content(
t('two_factor_authentication.two_factor_choice_options.webauthn'),
)
end
end
end