Add Webauthn Verification authenticate endpoint #3331

jenshenny · 2023-01-10T15:09:11Z

What problem are you solving?

Part of #3298

Creates the endpoint for the authentication of the webauthn credential. If successful, a json payload of {"message": "success"} will be returned. Otherwise, {"message": error_message} would be returned.

What approach did you choose and why?

The logic is inspired off of the logic in the sessions controller. I memoized the different variables used to reduce the size of the original method.
Removing user as a part of session[:webauthn_authentication], we should be using the webauthn path token to find the user.

What should reviewers focus on?

Any missing test cases
Is authenticate the right action name? Was thinking between authenticate or verify, but chose authenticate since verify might be confused with the webauthn verification object itself.

Testing

Follow the tophat instructions from #3324 to get to the prompt page. You are able to authenticate your webauthn credential when clicking the button.

jchestershopify

Some nits and questions, but otherwise LGTM.

app/controllers/webauthn_verifications_controller.rb

jchestershopify · 2023-01-10T15:48:32Z

app/controllers/webauthn_verifications_controller.rb

+  def credential_params
+    @parameters ||= params.require(:credentials).permit(:id,
+      :type,
+      :rawId,


What is :rawId and how does it differ from :id?

In the docs, id is a base 64 encoded string of the id, and the rawID is the id in binary form. Both are required to verify the credential.

jchestershopify · 2023-01-10T15:50:24Z

config/routes.rb

@@ -183,7 +183,10 @@

    resources :ownership_calls, only: :index
    resources :webauthn_credentials, only: :destroy
-    get 'webauthn_verification/:webauthn_token', to: 'webauthn_verifications#prompt', as: 'webauthn_verification'
+    resource :webauthn_verification, only: [] do


Dumb question: what does only: [] achieve?

Not dumb at all! To my knowledge, you define an only clause to keep the specified actions for a resource (eg. update, show, edit). Since this controller doesn't implement any of those default actions, I put it as []. I put the "custom" routes in the resource clause to help with grouping and remove some repetition in the code.

This might be an occasion for a EOL comment. Something like "# 'only' is empty because this controller has custom actions rather than 'show', 'create' etc."

I'm leaning towards refraining from adding a comment. There's 4-5 occasions of this in the routes file already so this pattern is not going outside any unexpected code. If you feel strongly, I'll add it in!

Gotcha. In that case we should leave it uncommented.

test/functional/webauthn_verifications_controller_test.rb

codecov · 2023-01-10T18:42:44Z

Codecov Report

Merging #3331 (15d33b0) into webauthn-cli (7cf09a7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##           webauthn-cli    #3331   +/-   ##
=============================================
  Coverage         98.54%   98.54%           
=============================================
  Files               116      116           
  Lines              3426     3441   +15     
=============================================
+ Hits               3376     3391   +15     
  Misses               50       50

Impacted Files	Coverage Δ
...p/controllers/webauthn_verifications_controller.rb	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jenshenny · 2023-01-10T18:59:02Z

test/functional/webauthn_verifications_controller_test.rb

+
+      should respond_with :unauthorized
+      should "return error message" do
+        assert_equal "WebAuthn::ChallengeVerificationError", JSON.parse(response.body)["message"]


WebAuthn::ChallengeVerificationError isn't the best error message, I don't think it would be triggered much but could be worth punting it as an future enhancement

jchestershopify

Looking even better! Some small suggestions.

jchestershopify · 2023-01-10T19:04:46Z

.rubocop.yml

@@ -59,6 +59,7 @@ Metrics/AbcSize:
 Metrics/BlockLength:
  Exclude:
    - test/**/*.rb
+    - 'config/routes.rb'


It was probably time we did this. Might be worth updating Metrics/BlockLength in .rubocop_todo.yml to a lower value, since we've just been incrementing it each time we updated routes.rb.

since we've just been incrementing it each time we updated routes.rb.

Oh haha TIL, will do 🫡

Updated the rubocop files accordingly. Also added tasks and concerns to the mix of exclusions because the content of a task or concern is supposed to be surrounded by a block.

jchestershopify · 2023-01-10T19:07:07Z

config/routes.rb

@@ -183,7 +183,10 @@

    resources :ownership_calls, only: :index
    resources :webauthn_credentials, only: :destroy
-    get 'webauthn_verification/:webauthn_token', to: 'webauthn_verifications#prompt', as: 'webauthn_verification'
+    resource :webauthn_verification, only: [] do


This might be an occasion for a EOL comment. Something like "# 'only' is empty because this controller has custom actions rather than 'show', 'create' etc."

jchestershopify · 2023-01-10T19:07:28Z

config/routes.rb

-    get 'webauthn_verification/:webauthn_token', to: 'webauthn_verifications#prompt', as: 'webauthn_verification'
+    resource :webauthn_verification, only: [] do
+      get ':webauthn_token', to: 'webauthn_verifications#prompt', as: ''
+      post ':webauthn_token', to: 'webauthn_verifications#authenticate', as: :authenticate, constraints: { format: /json/ }


Why a regex for json?

I did some copy and pasta with the regex from another route. I'm intending to keep it as is since we're planning to support html as a format in a followup -> format: /html|json/.

jchestershopify

No notes - LGTM.

jchestershopify reviewed Jan 10, 2023

View reviewed changes

jenshenny force-pushed the add-webauthn-verification-endpoint branch from 035e976 to c444a2f Compare January 10, 2023 18:34

jenshenny marked this pull request as ready for review January 10, 2023 18:57

jenshenny commented Jan 10, 2023

View reviewed changes

jenshenny requested a review from jchestershopify January 10, 2023 18:59

jchestershopify reviewed Jan 10, 2023

View reviewed changes

jenshenny added 2 commits January 10, 2023 14:22

Add WebAuthn Verification authenticate path

9bcb6de

remove user from webauthn authentication session

9fa233e

jenshenny force-pushed the add-webauthn-verification-endpoint branch from c444a2f to 6e5978b Compare January 10, 2023 19:23

Add exclusions to block length rubocop

15d33b0

jenshenny force-pushed the add-webauthn-verification-endpoint branch from 6e5978b to 15d33b0 Compare January 10, 2023 19:25

jenshenny requested a review from jchestershopify January 10, 2023 19:36

jchestershopify reviewed Jan 10, 2023

View reviewed changes

jenshenny requested a review from simi January 10, 2023 19:51

jenshenny merged commit 768ae44 into rubygems:webauthn-cli Jan 10, 2023

jenshenny deleted the add-webauthn-verification-endpoint branch January 10, 2023 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Webauthn Verification authenticate endpoint #3331

Add Webauthn Verification authenticate endpoint #3331

jenshenny commented Jan 10, 2023 •

edited

Loading

jchestershopify left a comment

jchestershopify Jan 10, 2023

jenshenny Jan 10, 2023

jchestershopify Jan 10, 2023

jenshenny Jan 10, 2023

jchestershopify Jan 10, 2023

jenshenny Jan 10, 2023

jchestershopify Jan 10, 2023

codecov bot commented Jan 10, 2023 •

edited

Loading

jenshenny Jan 10, 2023

jchestershopify left a comment

jchestershopify Jan 10, 2023

jenshenny Jan 10, 2023

jenshenny Jan 10, 2023 •

edited

Loading

jchestershopify Jan 10, 2023

jchestershopify Jan 10, 2023

jenshenny Jan 10, 2023 •

edited

Loading

jchestershopify left a comment

Add Webauthn Verification authenticate endpoint #3331

Add Webauthn Verification authenticate endpoint #3331

Conversation

jenshenny commented Jan 10, 2023 • edited Loading

What problem are you solving?

What approach did you choose and why?

What should reviewers focus on?

Testing

jchestershopify left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jchestershopify left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshenny Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshenny Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

jchestershopify left a comment

Choose a reason for hiding this comment

jenshenny commented Jan 10, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

jenshenny Jan 10, 2023 •

edited

Loading

jenshenny Jan 10, 2023 •

edited

Loading