feat: add possibility of enabling WebAuthn as a second factor authentication #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
santiagorodriguez96
force-pushed
the
add-optional-2fa-authentication
branch
2 times, most recently
from
89b4533
to
d4ed665
Compare
grzuy
reviewed
May 7, 2020
There was a problem hiding this comment.
Excellent work so far!
Left a couple of comments on my first review pass.
I know a few comments I made are recommending changes in code that is consistent with what we have in the webauthn-rails-demo-app, but still commented what I think might be better and then we can go back and refactor in the other app.
santiagorodriguez96
force-pushed
the
add-optional-2fa-authentication
branch
3 times, most recently
from
81f96ae
to
398f6fe
Compare
grzuy
reviewed
May 12, 2020
grzuy
changed the title
grzuy
reviewed
May 13, 2020
| end | ||
|
|
||
| test 'verify_key should require initiated login and 2FA enabled' do | ||
| post verify_key_second_factor_authentication_path |
There was a problem hiding this comment.
Not sure we are POSTing anything in the request, sounds more like a GET?
There was a problem hiding this comment.
All is doing is asking the server for the options for credentails.get call. Just "asking for information", "getting information".
There was a problem hiding this comment.
Yeah but how we get to execute the stimultus controllers' actions if we don't post a request with json format? I got a little lost here 🤔
grzuy
reviewed
May 13, 2020
test/controllers/second_factor_authentication_controller_test.rb
Outdated
Show resolved
Hide resolved
santiagorodriguez96
force-pushed
the
add-optional-2fa-authentication
branch
from
398f6fe
to
3885a0a
Compare
santiagorodriguez96
force-pushed
the
add-optional-2fa-authentication
branch
from
3885a0a
to
8ac62b7
Compare
grzuy
reviewed
Jun 4, 2020
santiagorodriguez96
force-pushed
the
add-optional-2fa-authentication
branch
from
8ac62b7
to
fb9742b
Compare
grzuy
approved these changes
Jun 20, 2020
This adds a basic UI for enabling WebAuthn 2FA, with a basic button that takes users to a new credential page that asks them for their credential's nickname. After they hit the `Add Security Key` button, they'll be prompted to enter their security key by their browser. Once that's done, the process will be completed and you users will added their security key. * Used stimulus to comunicate between our server and the user client * Used WebAuthnJson gem for calling the browser to create the credentials * Created Credential model that belongs to User. * Add `current_challenge` to Users
santiagorodriguez96
force-pushed
the
add-optional-2fa-authentication
branch
from
fb9742b
to
a899df2
Compare
This commit adds verification of second factor when attempting to sign in, if enabled. It adds a few conditions when checking if a user is authenticated, making that a user that has second factor enabled is authenticated only after the second factor is authenticated. So now, we'll still be storing the `user_id` in the session, after users sign in, but we'll also store the boolean `user_authenticated`, which will be true if user's second factor its authenticated. The flow for authenticating a credential is pretty similar to the one that we did for registering them, with the difference that we want to fetch an existing one instead of creating it, therefore we use methods like `WebAuthn::Credential#options_for_get` and `WebAuthn::Credential#from_get`.
santiagorodriguez96
force-pushed
the
add-optional-2fa-authentication
branch
from
a899df2
to
a85a3e4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
New Feature: Allow users to enable Webauthn as a Second-Factor Authentication
This adds a basic UI for enabling WebAuthn 2FA, with a basic button that takes users to a page were users can add their security keys. After they hit the
Add Security Keybutton, they'll be prompted to enter their security key by their browser. Once that's done, the process will be completed and you users will added their security key.It also adds a second page after signing in where users are asked to authenticate using their second factor – if enabled.
How
It adds a few conditions when checking if a user is authenticated, making that a user that has second factor enabled is authenticated only after the second factor is authenticated.
So now, we'll still be storing the
user_idin the session, after users sign in, but we'll also store the booleanuser_authenticated, which will be true if user's second factor its authenticated.Steps
Screenshots
Credential Registration
Credential Authentication
How to Review
References
https://www.pivotaltracker.com/story/show/172354931