-
-
Notifications
You must be signed in to change notification settings - Fork 420
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
Optional 'remember MFA' for browser #203
Conversation
Also a thing from the checklist is rate limiting the authentication endpoints. |
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.
Thanks for the PR! I tested it locally (on localhost
) but somehow the mfa
cookie doesn' seem to be saved and the feature doesn't work correctly: it always ask me to enter the OTP code even if I have ticked "Remember this browser for 30 days".
It's also good to support FIDO as well. There's a pending PR on FIDO that would be merged soon, maybe you can base this PR on this one to avoid conflicts.
I left some comments on the PR.
Are you sure you're testing it right? If you use the logout button, the 'mfa' cookie is also deleted. (A user action logout should in remove all user cookies) |
Here's how I tested the feature locally:
|
Yeah so in logout.py:L13 the mfa cookie is deleted. So if you use the 'logout' button to exit the session, it won't work. Only if the session expires (i.e. the cookie slapp is no longer valid) and the user then goes to login, it does work. So in short:
Or we can just remove logout.py:L13 and it will always work. |
@SibrenVasse I see! I also understand now why you need to use the cookie directly. The PR is good for me, I will merge it when we merge the multiple FIDO keys PR. |
I'll also add the feature to the FIDO path. |
Great, thanks! I think the rate-limiting can be in a separate PR. |
Yeah I'll do that. |
1. Use MFA cookie to whitelist browser for MFA for 30 days.
In this PR only for TOTP. Maybe also add this for FIDO?
2. Prevent OTP replay attacks by invalidating last token
See security checklist on https://pyotp.readthedocs.io/en/latest/
3. Add autofocus to email field on login screen
4. Remove cookies from browser on logout
5. Fix flash() calls in mfa.py