-
Notifications
You must be signed in to change notification settings - Fork 69
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.
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
Add 2 factor authentication as optional feature #70
Merged
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
f13707c
Add onetimepass as dependency to native auth
leportella 9e52615
Add onetimepass secret creation on UserInfo orm
leportella d7156ac
Add sn authentication on authenticate method
leportella 1a4efcd
Add two factor auth code on signup template
leportella 0756327
Add 2 factor auth on login template
leportella 215f527
Add two factor authentication documentation
leportella a9f7057
Change name from second_factor to two_factor
leportella ab8b16e
Fix errors on nativeauth default variables
leportella feb5f0e
Add option to allow users to have a 2fa
leportella e8c47ed
Add authenticator input on login website
leportella 458bf45
Makind 2fa not allowed by default
leportella File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,4 @@ pytest >= 3.7 | |
pytest-asyncio | ||
notebook==5.7.2 | ||
bcrypt | ||
onetimepass |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
import base64 | ||
import bcrypt | ||
import os | ||
import re | ||
from jupyterhub.orm import Base | ||
|
||
import onetimepass | ||
from sqlalchemy import Boolean, Column, Integer, String, LargeBinary | ||
from sqlalchemy.orm import validates | ||
|
||
|
@@ -13,6 +16,12 @@ class UserInfo(Base): | |
password = Column(LargeBinary, nullable=False) | ||
is_authorized = Column(Boolean, default=False) | ||
email = Column(String) | ||
otp_secret = Column(String(10)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does the '10' refer to here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the token is created with size 10, so I limited here |
||
|
||
def __init__(self, **kwargs): | ||
super(UserInfo, self).__init__(**kwargs) | ||
if not self.otp_secret: | ||
self.otp_secret = base64.b32encode(os.urandom(10)).decode('utf-8') | ||
|
||
@classmethod | ||
def find(cls, db, username): | ||
|
@@ -40,3 +49,6 @@ def validate_email(self, key, address): | |
assert re.match(r"^[A-Za-z0-9\.\+_-]+@[A-Za-z0-9\._-]+\.[a-zA-Z]*$", | ||
address) | ||
return address | ||
|
||
def is_valid_token(self, token): | ||
return onetimepass.valid_totp(token, self.otp_secret) |
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split this into two parts:
In the former, some users might choose to protect their own accounts with 2FA, but isn't enforced. (2) is more complicated - what happens to people who already had created accounts without 2FA? How can they log in?
So I think one PR should be to allow_two_factor_authentication (or just allow_2fa), and it makes it optional for users to set up and use 2FA. There should probably also be a button in the admin screen that shows admins if this user has 2fa enabled, and optionally allows them to reset it.
After that, we can figure out how to make it required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this makes sense. I'll refactor this PR to make the 2fa an optional character within user and then make another one to make it required and how to deal with people that signed in without 2fa, ok?