Pull in opr #842 - 2-factor auth #86
Conversation
change language from 'zh_Hans_CN' to 'zh_CN'
* supporting two factor authentication: * Support Mail, SMS, or Google Authenticator second factor authentication. * Ability to change second factor authentication to existing users * Provide rescue mail in case of lost phone * update docs with two factor authentication changes * updating requirements, authors * adding two_factor test * improving tests coverage
request.json->request.get_json update tests to use hash_password pep8 compliance Update dependencies for tests move from pyenchant to msgcheck
Co-Authored-By: malware-watch <malware-watch@users.noreply.github.com>
These changes are mostly docs and cleanup.
check imports if TWO_FACTOR is True
I belive that - I tried and it was not enjoyable.
Is this really necessary? I can't think of any reason to use normal auth and 2FA at the same time. People would just use the normal auth route and bypass the two factor one if they don't have to go through the whole extra step.
I would think it can be switched to use JSON Web Tokens, which shouldn't break anything, but I am not very familiar with JWTs. Makes sense
Done. Easy enough fix. Since we can't necessarily test for the failed imports, I've added the #pragma: no cover tag to those statements Thanks!
I was confused by that as well, I tried using generate_secret, but the onetimepass lib didn't like that, so I reverted to the original method.
That can be changed to anything. I am unaware of any length restriction for the totp_secret. I commited a fix
That somewhat makes sense. The US phone number +1(555)555-5555 is 15 digits, so it could be increased based on the user's locale. It has to be defined in the User's models I committed a fix
This can be changed, but if a User accidentally goes to /login, then they would have to go through the 2FA again as well. Somewhat annoying. Makes sense - lets leave it
Completely doable. |
|
Most of the missing coverage in utils.py seems to be coming from the TwilioSmsSender Class, which probably could be skipped in coverage, as you would need to create a Twilio client and send actual SMS messages. The drop in coverage from views.py appears to be coming from missing tests that made the two-factor views more json friendly, mostly assert flashes and JSON form error checking. I'll write some tests at somepoint when I am able to sit down and write them. |
Increase sizes of examples for 2FA phone and secret.
|
It is getting really close. For additional tests on NEW functionality - I will file an issue and assign to you - that won't hold up getting this in. For generating totp_secret - I will look into this - I am concerned about the current implementation both from a security perspective and that changing it might require a backwards incompatibility. I will file an issue and assign to me (but I want to resolve this PRIOR to releasing). As for 2 endpoints - the way I see most websites written is that 2FA is an option. Some financial sites require it - but most sites simply offer that as more secure login option that is per user. I can see having an option to require 2FA. I was looking at view.py - I am confused by the URLs - looks like they aren't actually configurable like all the other urls - and have strange names like /two_factor_setup_function - that doesn't seem consistent. I think we should change those to config variables and use URL friendly names like: |
|
Sounds good. Not sure how the dual login would best be implemented. Most likely would have to modify the original login to redirect/return 400 if the user has two factor enabled. As for the secret, I am not married to pyqrcode and onetimepass, those were in there from the original author. I think he used a Miguel Grinberg blog post to develop most of the two factor extension. If there are better/more recent packages that can implement it, I say use those. If you want to update the routes in views.py and make them configurable, I say go for it. Shouldn't be too difficult. I am just trying to find a good balance between work, kids and all. |
|
Thanks for your comments and all the work. I am really looking forward to getting this in and released. The original PR is pretty old - so I expect some new things have evolved. I think what I will do is go ahead and merge this and file issues. This PR is already too large to keep reviewing, then followup with view changes and looking into secret/key generation. I just ask that you review the changes. |
|
@baurt I have a question for you - I am going through the views and trying to understand the flow - I am extremely confused by the two_factor_password_confirmation view - which has as a default endpoint /change/two_factor_password_confirmation. It seems like you actually have to call this to get 'password_confirmed' set in the session prior to setting up 2FA. More confusing is that the form is called two_factor_change_method_verify_password_form but from what I can tell - it has nothing to do with changing method - it just has password and verify_password - so it is nothing more that login and/or change password? What am I missing? I suppose lastly - is there a way to change your preferred method? I realize you didn't write this - but presumably you know/understand how to use it..... Thanks! |
|
The only way that I figured out what was going on was by manually testing a built extension in an app that I have. What is happening is that the endpoint /change/two_factor_password_confirmation takes you to a form with a password that needs to be verified. Once you verify the password, it takes you to the setup function to change the preferred method. I think that the 'password_confirmed' session variable is one way of preventing unauthorized access to the setup function without having gone through the whole first time login. It took me a while to understand most of what is going on. So, there are two ways of reaching the setup function legitimately, first time login or via /change/two_factor_password_confirmation. I think the first time login method is fairly clear. The next way is via that endpoint. Say a user is logged in and they want to change how 2FA occurs. They go to /change/two_factor_password_confirmation and to prevent unauthorized changing of the method (say by forgetting to logout/leaving the session open) you have to enter your password again. If that is correct, then it takes you to the setup function where you change your method. So, can this process be cleaned up? Surely. Maybe the solution is to document the living daylights out of these views because they are confusing. Let me know what you think. |
Absolutely. I think everything so far looks good to merge. I'll definitely review changes from the issues. The tests probably will have to be modified slightly to accomodate some of the changes. |
These changes are mostly docs and cleanup.
that was a pain... Some questions before merging/releasing.
Consider having separate endpoints so can have both normal and 2FA - we already made this mistake with passwordless sharing the /login endpoint.
This seems to rely on lots of info in session - so that means this isn't really useful for token based applications?
try not to require onetimepasswd and qrcode (in setup.py install_requires) unless config is set.
generate_totp should be overridable - perhaps move it to userMixin like some other things? I am confused by passlib.totp.generate_secret() as referenced in: https://passlib.readthedocs.io/en/stable/narr/totp-tutorial.html#totp-tutorial
in docs example: DbModel has only 16 chars for totp - that seems low
in docs example: DbModel phone number is just 15 chars
login - not sure @anonymous_user_required is good (I know that is what current /login does). That can be annoying for UIs - I would suggest just logging out current user and allowing a new login.
-- generate_qrcode - probably should be a utility and throw exceptions (minor).