Skip to content

LG-10225: Add JavaScript-enabled WebAuthn feature spec capability#8761

Merged
aduth merged 4 commits intomainfrom
aduth-try-webauthn-js-stub
Jul 13, 2023
Merged

LG-10225: Add JavaScript-enabled WebAuthn feature spec capability#8761
aduth merged 4 commits intomainfrom
aduth-try-webauthn-js-stub

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 12, 2023

🎫 Ticket

LG-10225

🛠 Summary of changes

Adds support for simulating results of WebAuthn in JavaScript-enabled browsers in specs, allowing for more realistic test scenarios running in a real browser.

For example, this may have helped avoid the scenario fixed in #8738.

📜 Testing Plan

rspec spec/features/webauthn/sign_in_spec.rb

@aduth aduth changed the title Add JavaScript-enabled WebAuthn stubbing capability Add JavaScript-enabled WebAuthn feature spec capability Jul 12, 2023
@aduth
Copy link
Contributor Author

aduth commented Jul 12, 2023

Addressing original "draft" comments:

Add spec for what was fixed in #8738

Added in 7435b62.

Figure out what to do about the inline base64ToArrayBuffer

Decided to duplicate in the spec helper in 5918ffc.

Opt-in more specs?

Will do this as-needed in future pull requests. Changes here to reorganize, rename, and add new helper methods should simplify this work.

@aduth aduth marked this pull request as ready for review July 12, 2023 14:37
Comment on lines 190 to 189
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea with the rename is that, as of LG-8622 / #8716, we no longer differentiate the error behavior based on whether the user has multiple MFA methods enabled.

@aduth aduth requested a review from a team July 12, 2023 15:33
@aduth aduth force-pushed the aduth-try-webauthn-js-stub branch from 7435b62 to 7ed0aa6 Compare July 12, 2023 15:48
@aduth aduth changed the title Add JavaScript-enabled WebAuthn feature spec capability LG-10225: Add JavaScript-enabled WebAuthn feature spec capability Jul 12, 2023
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

would it make the tests clearer if we passed in platform: true to these helpers instead of inferring from the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have a soft preference for the current approach, for convenience and since the helpers already make many other assumptions of "just working" for both platform authenticators and security keys. Open to revisiting though.

Copy link
Contributor

Choose a reason for hiding this comment

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

when skimming this diff I had trouble telling the difference between _and_cancel and _and_verify because they're the same number of characters, what if we dropped _and_verify since that's kind of default behavior and left _and_cancel as the differentiated option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that seems like a simple way to differentiate. I'll update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't quite as easy as expected, since I already had a method click_webauthn_authenticate_button for just the action of clicking the button itself.

I took a different direction in b75d2ff63, since this feels like two things of simulating the result + clicking the button, where the simulated result has setup and teardown. Yielding to a block seemed appropriate enough? 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not super in love with the block approach but can't think of anything better 🤷

aduth and others added 4 commits July 13, 2023 10:08
changelog: Internal, Automated Testing, Improve realism of WebAuthn tests
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Two separate concerns, where mocking involves setup & teardown.

Avoids some readability issues with similar named methods

See: https://github.com/18F/identity-idp/pull/8761/files#r1261446915
@aduth aduth force-pushed the aduth-try-webauthn-js-stub branch from b75d2ff to 200b0bb Compare July 13, 2023 14:09
@aduth aduth merged commit de13c35 into main Jul 13, 2023
@aduth aduth deleted the aduth-try-webauthn-js-stub branch July 13, 2023 15:27
aduth added a commit that referenced this pull request Aug 14, 2024
aduth added a commit that referenced this pull request Aug 14, 2024
…11077)

* Use faster default driver for feature tests not requiring JavaScript

changelog: Internal, Automated Testing, Use faster default driver for feature tests not requiring JavaScript

* Enable JavaScript for at least one WebAuthn spec

See rationale in #8761

* Remove duplicate spec

Same test case already exists in no-JavaScript context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants