Skip to content

LG-8553: Don't submit without nickname for security keys #7867

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
mdiarra3 merged 8 commits into from
Mar 2, 2023
Merged

LG-8553: Don't submit without nickname for security keys #7867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c2d9ebd
changelog: Bug Fixes, Authentication, don't allow users to submit wit…
mdiarra3 Feb 22, 2023
bc1c82c
rubocop
mdiarra3 Feb 22, 2023
2855d74
fix spec for webauthn
mdiarra3 Feb 22, 2023
049646c
update javascript to be on form submit
mdiarra3 Feb 22, 2023
c021360
erb lint fix
mdiarra3 Feb 22, 2023
5395925
fix up javascript
mdiarra3 Feb 22, 2023
efadf28
on javascript submit the form without prompting for javascript
mdiarra3 Feb 23, 2023
2e86727
fix indentation
mdiarra3 Mar 2, 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
13 changes: 4 additions & 9 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 @@ -183,17 +183,12 @@ def process_invalid_webauthn(form)
else
flash.now[:error] = t('errors.webauthn_setup.unique_name')
end

render :new
elsif form.platform_authenticator?
flash[:error] = t('errors.webauthn_platform_setup.general_error')
else
if form.platform_authenticator?
flash[:error] = t('errors.webauthn_platform_setup.general_error')
else
flash[:error] = t('errors.webauthn_setup.general_error')
end

redirect_to account_two_factor_authentication_path
flash[:error] = t('errors.webauthn_setup.general_error')
end
render :new
end

def mark_user_as_fully_authenticated
Expand Down
18 changes: 3 additions & 15 deletions app/javascript/packs/webauthn-setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ function webauthn() {
if (!isWebAuthnEnabled()) {
reloadWithError('NotSupportedError');
}
const continueButton = document.getElementById('continue-button')!;
continueButton.addEventListener('click', () => {
const form = document.getElementById('webauthn_form') as HTMLFormElement;
form.addEventListener('submit', (event) => {
event.preventDefault();
document.getElementById('spinner')!.classList.remove('display-none');
document.getElementById('continue-button')!.className = 'display-none';

Expand All @@ -47,19 +48,6 @@ function webauthn() {
})
.catch((err) => reloadWithError(err.name, { force: true }));
});
const input = document.getElementById('nickname') as HTMLInputElement;
input.addEventListener('keypress', function (event) {
if (event.keyCode === 13) {
// prevent form submit
event.preventDefault();
}
});
input.addEventListener('keyup', function (event) {
event.preventDefault();
if (event.keyCode === 13 && input.value) {
continueButton.click();
}
});
Comment thread
mdiarra3 marked this conversation as resolved.
}

if (process.env.NODE_ENV !== 'test') {
Expand Down
11 changes: 5 additions & 6 deletions app/views/users/webauthn_setup/new.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,12 @@
checked: @presenter.remember_device_box_checked?,
},
) %>
<%= submit_tag t('forms.buttons.submit.default'), id: 'submit-button', class: 'display-none' %>
<%= submit_tag(
@presenter.button_text,
id: 'continue-button',
class: 'display-block usa-button usa-button--big usa-button--wide margin-y-5',
) %>
<% end %>
<%= button_tag(
@presenter.button_text,
class: 'display-block usa-button usa-button--big usa-button--wide margin-y-5',
id: 'continue-button',
) %>

<div class="spinner display-none margin-y-4" id='spinner'>
<div class='text-center'>
Expand Down
4 changes: 2 additions & 2 deletions spec/features/webauthn/management_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def expect_webauthn_setup_success

def expect_webauthn_setup_error
expect(page).to have_content t('errors.webauthn_setup.general_error')
expect(current_path).to eq account_two_factor_authentication_path
expect(current_path).to eq webauthn_setup_path
end

def visit_webauthn_platform_setup
Expand All @@ -41,7 +41,7 @@ def expect_webauthn_platform_setup_success

def expect_webauthn_platform_setup_error
expect(page).to have_content t('errors.webauthn_platform_setup.general_error')
expect(current_path).to eq account_two_factor_authentication_path
expect(current_path).to eq webauthn_setup_path
end

context 'with webauthn roaming associations' do
Expand Down
2 changes: 1 addition & 1 deletion spec/features/webauthn/sign_up_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def expect_webauthn_setup_success

def expect_webauthn_setup_error
expect(page).to have_content t('errors.webauthn_setup.general_error')
expect(page).to have_current_path(authentication_methods_setup_path)
expect(page).to have_current_path(webauthn_setup_path)
end

it_behaves_like 'webauthn setup'
Expand Down
6 changes: 3 additions & 3 deletions spec/support/features/webauthn_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def fill_in_nickname_and_click_continue(nickname: 'mykey')
end

def mock_submit_without_pressing_button_on_hardware_key_on_setup
first('#submit-button', visible: false).click
first('#continue-button').click
end

def mock_press_button_on_hardware_key_on_setup
Expand All @@ -37,9 +37,9 @@ def mock_press_button_on_hardware_key_on_setup
set_hidden_field('attestation_object', attestation_object)
set_hidden_field('client_data_json', setup_client_data_json)

button = first('#submit-button', visible: false)
button = first('#continue-button')
if javascript_enabled?
button.execute_script('this.click()')
page.evaluate_script('document.querySelector("form").submit()')
Copy link
Copy Markdown
Contributor Author

@mdiarra3 mdiarra3 Feb 24, 2023

Choose a reason for hiding this comment

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

Because we have to skip over the browser prompt, to properly bypass that we need to just submit the form via javascript instead of through the UI when javascript is enabled.

else
button.click
end
Expand Down