Skip to content
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 support for one-time passwords (two-factor authentication) #2655

Merged
merged 30 commits into from Jun 4, 2018
Merged

Conversation

brianhelba
Copy link
Member

@brianhelba brianhelba commented Mar 9, 2018

This now has basic functionality. Still TODO:

Additional work that may be out of scope for this initial PR:

  • Rate limit login requests
  • Switch caching of the last-used token to be stored in MongoDB (see here for possibly-helpful resources)
  • Option to mandate use of 2FA by all users
  • Allow user to recover account access with HOTP-based codes (currently, a site admin must disable a user's OTP to recover access)
  • Use application secrets (once they exist) when generating TOTP keys
  • Set OTP issuer when User._TotpFactory is created (requires more robust server hostname detection)

Technical reference: http://passlib.readthedocs.io/en/stable/narr/totp-tutorial.html


Current login dialog:
image

Before 2FA enrollment:
image

2FA enrollment process:
image

@brianhelba brianhelba changed the title Add support for one-time passwords (two-factor authentication) WIP: Add support for one-time passwords (two-factor authentication) Mar 9, 2018
@mgrauer
Copy link
Contributor

mgrauer commented May 11, 2018

@jtomeck Screenshots please? Whenever you are done with the current round of work.

@jtomeck
Copy link
Contributor

jtomeck commented May 11, 2018

@brianhelba @mgrauer I worked on styling the UI of the one time password widget. Here is a screenshot of my approach.

screencapture-localhost-9080-2018-05-11-16_39_03

@jeffbaumes
Copy link
Member

I like the styling @jtomeck! Peanut gallery item: Should the bottom Cancel/Enable buttons be together (say both on the left) with the same size / font size, not unlike the "Close and comment" and Comment buttons on GitHub comments?

image

@danlamanna
Copy link
Member

danlamanna commented May 12, 2018

More peanut gallery items:

  • Will the bottom brown be affected by the brand color settings, and is that good/bad? (Not sure how that's implemented)
  • Can we grep this whole branch and ensure that "Two-Factor" is labeled as such everywhere? I saw the original PR had both "2-factor" and "two-factor".

@danlamanna
Copy link
Member

danlamanna commented May 12, 2018

Also, most MFA implementations I've used have a modal dialog for username/password and then a separate dialog for the second factor. Is this a workflow we should consider? The benefit of this approach would be that we only prompt the user for a second factor if they actually have it enabled, avoiding confusion for those who don't.

@jtomeck
Copy link
Contributor

jtomeck commented May 14, 2018

@jeffbaumes I think that's a good idea. I had a thought process behind why I did it the way I did, but I don't think the result really does anything to help the user on their path through the UI.

@danlamanna Right now that bottom bar just has the the top bar hex value for its color. I don't think it would be good if that matched the brand color because it could hurt the visibility of the buttons in the bar. I think this raises the question of if we want to change the color of the bottom bar, as well as the other blue elements I've designed into the layout. Maybe there are some stylus color variables somewhere in the CSS that I can play around with.

@@ -127,7 +138,16 @@ def validate(self, doc):

return doc

def authenticate(self, login, password):
def filter(self, doc, user, additionalKeys=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

@girder/developers I'm not sure if I like what's being done here, but it's the only way I see to create 'derived' values when a document is filtered.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zachmullen Let me know if you have any thoughts.

}

def hasOtp(self, user):
return 'otp' in user and user['otp']['enabled']
Copy link
Member

Choose a reason for hiding this comment

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

hasOtpEnabled

danlamanna
danlamanna previously approved these changes Jun 1, 2018
@brianhelba
Copy link
Member Author

@girder/developers Review is complete and this is ready to merge. However, we need to fix #2730 before testing will pass in CI.

danlamanna
danlamanna previously approved these changes Jun 1, 2018
danlamanna
danlamanna previously approved these changes Jun 4, 2018
@brianhelba brianhelba changed the title WIP: Add support for one-time passwords (two-factor authentication) Add support for one-time passwords (two-factor authentication) Jun 4, 2018
@brianhelba
Copy link
Member Author

@danlamanna Fixed a bug causing tests to sometimes fail. PTAL again.

@brianhelba brianhelba merged commit 3e5b8cd into master Jun 4, 2018
@brianhelba brianhelba deleted the otp branch June 4, 2018 23:56
@mgrauer
Copy link
Contributor

mgrauer commented Jun 5, 2018

💕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants