Add WebAuthn verification URL endpoint #3305
Conversation
Hardcoded hostname to example.com, pending followup to use Rails config. - Add /api/v1/webauthn_verification - Add WebauthnVerificationController - Add WebauthnUserMethods#refresh_webauthn_verification Co-authored-by: Ashley Ellis Pierce <anellis12@gmail.com>
|
|
||
| def refresh_webauthn_verification | ||
| self.webauthn_verification = WebauthnVerification.create( | ||
| path_token: SecureRandom.base58(16), |
There was a problem hiding this comment.
Base 58 chosen because it plays nicely with manually typing in the code and also with direct copy & paste action. 16 characters doesn't impose an undue burden on folks manually typing, but gives a large search space (58^16 ≈ 1.64×10²⁸) that attackers would need to search to find the path token.
| should respond_with :success | ||
|
|
||
| should "return Webauthn verification URL with path token" do | ||
| response = YAML.load(@response.body) |
There was a problem hiding this comment.
Unlike the API key controller, we assume for now that YAML is the only supported format. We can add JSON later if necessary, but at the moment this endpoint is only intended for the use of the CLI.
Codecov Report
@@ Coverage Diff @@
## webauthn-cli #3305 +/- ##
=============================================
Coverage 98.52% 98.53%
=============================================
Files 114 115 +1
Lines 3396 3412 +16
=============================================
+ Hits 3346 3362 +16
Misses 50 50
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
It's unclear why this ordering is meaningful to Rails and also unclear how it evaded the tests. Next order of business is working out to test to ensure this ordering is not broken in future.
test/functional/api/v1/webauthn_verifications_controller_test.rb
Outdated
Show resolved
Hide resolved
This catches unsafe classes if present in output.
|
@aellispierce looking at ruby/rubygems#6179, I'm wondering if it makes sense for the Edit: specifically at L104 in |
@jchestershopify The rubygems_api_request takes the path because you could be sending the request to a different gem host beyond standard rubygems.org (like staging, localhost, your own private gem host, etc), and we want the request to be sent to whatever host you specify. While we could recreate the full URL on the client, since the client knows the host you're interacting with, that seems more error prone to me and I'm not sure I see the benefit? It'd be an extra step of us having to reconstruct the full url in the client to give to the user instead of just plopping in the response directly from the API. And while we want dynamic hosts when we send requests, we couldn't receive some sort of dynamic host webauthn url. Like a webauthn token created on rubygems.org would not work on localhost:3000 or myprivategemhost.org so we don't need the flexibility of the client being able to create a link with whatever host you've specified. |
|
That makes sense, thanks for walking me through it! |
* Add WebAuthn verification URL endpoint Hardcoded hostname to example.com, pending followup to use Rails config. - Add /api/v1/webauthn_verification - Add WebauthnVerificationController - Add WebauthnUserMethods#refresh_webauthn_verification Co-authored-by: Ashley Ellis Pierce <anellis12@gmail.com>
* Add WebAuthn verification URL endpoint Hardcoded hostname to example.com, pending followup to use Rails config. - Add /api/v1/webauthn_verification - Add WebauthnVerificationController - Add WebauthnUserMethods#refresh_webauthn_verification Co-authored-by: Ashley Ellis Pierce <anellis12@gmail.com>
* Add WebAuthn verification URL endpoint Hardcoded hostname to example.com, pending followup to use Rails config. - Add /api/v1/webauthn_verification - Add WebauthnVerificationController - Add WebauthnUserMethods#refresh_webauthn_verification Co-authored-by: Ashley Ellis Pierce <anellis12@gmail.com>
* Add WebAuthn verification URL endpoint Hardcoded hostname to example.com, pending followup to use Rails config. - Add /api/v1/webauthn_verification - Add WebauthnVerificationController - Add WebauthnUserMethods#refresh_webauthn_verification Co-authored-by: Ashley Ellis Pierce <anellis12@gmail.com>
* Add WebAuthn verification URL endpoint Hardcoded hostname to example.com, pending followup to use Rails config. - Add /api/v1/webauthn_verification - Add WebauthnVerificationController - Add WebauthnUserMethods#refresh_webauthn_verification Co-authored-by: Ashley Ellis Pierce <anellis12@gmail.com>
* Add WebAuthn verification URL endpoint Hardcoded hostname to example.com, pending followup to use Rails config. - Add /api/v1/webauthn_verification - Add WebauthnVerificationController - Add WebauthnUserMethods#refresh_webauthn_verification Co-authored-by: Ashley Ellis Pierce <anellis12@gmail.com>
What problem are you solving?
This PR primarily intendeds to add an endpoint to be consumed by the CLI during the WebAuthn verification flow.
Part of #3298, which adds WebAuthn verification support to the CLI. It builds on the
WebauthnVerificationmodel introduced in Shopify#76.Added in this PR:
/api/v1/webauthn_verificationWebauthnVerificationControllerWebauthnUserMethods#refresh_webauthn_verificationNote: The hostname in generated URLs is hardcoded to example.com, pending followup to use Rails config.
What approach did you choose and why?
The approach here is a standard Rails controller, with a singular route. This is because only one verification per
Userwill be valid at a time. Each time a verification path is requested, the relevantpath_tokenis reset.Logic for creating the
WebauthnVerificationis located in theUserWebauthnMethodsmodule. While the underlying data for a verification is properly separated from the User (not all users will have a current verification), the User acts as a natural point at which to request that a newWebauthnVerificationbe created.What should reviewers focus on?
The tests were inspired by the tests in
ApiKeysControllerTest, with scenarios removed that didn't make sense (for example, requiring an OTP code doesn't make sense during the Webauthn verification process). I am keen to get feedback on whether the tests have overlooked key scenarios.