Skip to content

LG 10021 hide error after successful webauth authentication #8678

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
kevinsmaster5 merged 7 commits into from
Jun 30, 2023
Merged

LG 10021 hide error after successful webauth authentication #8678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
726a717
changelog: User-Facing Improvements, Bug Fixes, Remove display of fai…
kevinsmaster5 Jun 27, 2023
ef81e7f
add test coverage for webauthn signin errors
kevinsmaster5 Jun 27, 2023
21b0b8a
move new test case to the end of the spec file
kevinsmaster5 Jun 28, 2023
7c52b31
fix mis-nested new 'it' block
kevinsmaster5 Jun 28, 2023
3c7fda6
fix lint errors
kevinsmaster5 Jun 28, 2023
b052e75
fix typo in spec and refactor element selection in ts file
kevinsmaster5 Jun 28, 2023
f29e4ee
add Non-null assertion operator to alert container
kevinsmaster5 Jun 29, 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
5 changes: 5 additions & 0 deletions app/javascript/packs/webauthn-authenticate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ function webauthn() {
const webauthnInProgressContainer = document.getElementById('webauthn-auth-in-progress')!;
const webauthnSuccessContainer = document.getElementById('webauthn-auth-successful')!;

const webauthAlertContainer = document.querySelector('.usa-alert--error')!;
const webauthnPlatformRequested =
webauthnInProgressContainer.dataset.platformAuthenticatorRequested === 'true';
const multipleFactorsEnabled =
Expand Down Expand Up @@ -35,6 +36,10 @@ function webauthn() {
(document.getElementById('signature') as HTMLInputElement).value = result.signature;
webauthnInProgressContainer.classList.add('display-none');
webauthnSuccessContainer.classList.remove('display-none');
// Check if alert container is shown and remove when device passes successfully.
if (webauthAlertContainer) {
webauthAlertContainer.remove();
}
Comment on lines 39 to 42
Copy link
Copy Markdown
Contributor

@aduth aduth Jun 29, 2023

Choose a reason for hiding this comment

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

Technically we're not checking if it's shown, since the element will always exist in the markup, and we're removing it whether or not it's visible. To that point, we may be able to even go as far to assume the element exists and assign the variable above using the TypeScript non-null assertion operator, similar to what we're doing with webauthnInProgressContainer and webauthnSuccessContainer variables before it.

Copy link
Copy Markdown
Contributor Author

@kevinsmaster5 kevinsmaster5 Jun 29, 2023

Choose a reason for hiding this comment

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

I had encountered unwanted behavior where the user not clicking cancel initially got an error then had to re-attempt in order to authenticate. I could not discern why but adding the check with that if statement fixed it.
The element with the class .usa-alert--error gets inserted as that view loads and if it needs to display the error. If there's no error that element doesn't show up in the DOM.
Would it be more preferable to use a non-null operator?

})
.catch((error: Error) => {
(document.getElementById('webauthn_error') as HTMLInputElement).value = error.name;
Expand Down
15 changes: 15 additions & 0 deletions spec/features/webauthn/sign_in_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,19 @@
expect(page).to have_content(t('errors.general'))
expect(page).to have_current_path(login_two_factor_webauthn_path)
end

it 'does not show error after successful challenge/secret reattempt' do
mock_webauthn_verification_challenge

sign_in_user(webauthn_configuration.user)
# click the next button or cancel from the browser dialog
click_button t('forms.buttons.continue')

expect(page).to have_content(t('errors.general'))

mock_press_button_on_hardware_key_on_verification
click_button t('forms.buttons.continue')

expect(page).to_not have_content(t('errors.general'))
Comment on lines 58 to 65
Copy link
Copy Markdown
Contributor

@aduth aduth Jun 28, 2023

Choose a reason for hiding this comment

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

One downside of how we stub out the WebAuthn behaviors in the specs is that none of this runs in JavaScript, which is where most users would encounter the errors. Curious how difficult it would be to have a JavaScript-enabled test.

Copy link
Copy Markdown
Contributor

@aduth aduth Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I spent part of my afternoon yesterday exploring if we could have more realistic WebAuthn stubbing for JavaScript-enabled browsers in the spec. I think it's possible, but I didn't quite reach a working solution.

Two avenues I considered:

I'd imagine the logic for these could live in the underlying helpers, e.g. mock_press_button_on_hardware_key_on_verification, using different code paths depending if javascript_enabled? is true.

I like the WebAuthn DevTools approach, but I couldn't quite figure out the right parameters to send, and which methods to call. The closest I got to a working solution was implementing the first approach. I got the tests passing locally, but strangely only when the browser was shown (i.e. using the SHOW_BROWSER=true environment variable). It's also kinda unwieldy because it inlines parts of base64ToArrayBuffer.

See code: https://github.com/18F/identity-idp/compare/aduth-try-webauthn-js-stub

Long story short: I worry this test isn't really giving us any regression assurances, since I suspect it passes on main as well. We could alternatively try a JavaScript spec which tests the JavaScript code. That would also be rather difficult with how packs are tightly integrated with page markup and very side-effecty. Maybe in the future we'd refactor the authentication button to a custom element like app/javascript/packages/webauthn/webauthn-authenticate-button-element.ts, which could be easier to test against.

Given all the challenges, I think it'd be okay to leave it like this for now, though maybe we should create a ticket to further explore one of the above alternatives?

Copy link
Copy Markdown
Contributor Author

@kevinsmaster5 kevinsmaster5 Jun 29, 2023

Choose a reason for hiding this comment

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

I will work on creating that ticket.

I can confirm that test does pass on main so your suspicion is correct.

end
end