TN-977 Lockout users if they fail password verification 5 times #2567

CodeRhymesLife · 2018-08-16T08:05:19Z

If someone or some thing tries to login to a user's account with an incorrect password more than 5 times, each time within 30 mins of the last, these changes lock the user out of the system.

story

These changes rely on a fork of flask_user that adds an event we use to track password verification failures. Here's the commit that adds the event. Here's the PR that will hopefully get accepted so we can once again rely on the flask_user release.

pep8speaks · 2018-08-16T08:05:26Z

Hello @drryanjames! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 20, 2018 at 09:52 Hours UTC

CodeRhymesLife · 2018-08-16T08:16:53Z

I still need to add unit tests. Since I'm out tomorrow and @pbugni is out Fri, wanted to get this out for review today to try and get it in this sprint.

CodeRhymesLife

Just a few comments and questions

CodeRhymesLife · 2018-08-16T08:18:33Z

portal/views/auth.py

@@ -26,6 +26,7 @@
 from flask_user.signals import (
    user_changed_password,
    user_logged_in,
+    user_password_failed,


Here's the new signal I added to flask-user

CodeRhymesLife · 2018-08-16T08:19:52Z

portal/views/auth.py

+    the user will be locked out of the system.
+    """
+    count = user.add_password_verification_failure()
+    auditable_event(


Should this be a normal log instead of an auditable event because the user is not authenticated?

If I should be a normal log, is it safe to use the user's id in the log?

Should this be a normal log instead of an auditable event because the user is not authenticated?

Good point, that might make more sense

If I should be a normal log, is it safe to use the user's id in the log?

Yup yup, those aren't publicly visible anywhere

CodeRhymesLife · 2018-08-16T08:21:41Z

portal/views/extend_flask_user.py

+        if user.is_locked_out:
+            # Make sure validators are run so we
+            # can populate self.password.errors
+            super(LoginForm, self).validate()


This is a bit odd. I'm calling the base.base class method to get things setup. I don't want to call the base method because it would add some stuff I don't want. I didn't find a better and reliable way to do this.

CodeRhymesLife · 2018-08-16T08:22:10Z

portal/views/extend_flask_user.py

+            # can populate self.password.errors
+            super(LoginForm, self).validate()
+
+            auditable_event(


Again, the user isn't authenticated so not sure if this should be an auditable event.

Yes, keep this. When we get support calls about users not being able to log in - it'll be helpful to have the available records.

CodeRhymesLife · 2018-08-16T08:22:38Z

portal/views/extend_flask_user.py

+                context='login'
+            )
+
+            error_message = _('We see you\'re having trouble - let us help. \


I wish this message was longer

CodeRhymesLife · 2018-08-16T08:23:17Z

requirements.txt

@@ -32,7 +32,7 @@ flask-session==0.3.1
 flask-sqlalchemy==2.3.2
 flask-swagger==0.2.13
 flask-testing==0.7.1
-flask-user==0.6.21 # pyup: <0.7 # pin until 1.0 is ready for prod
+git+https://github.com/uwcirg/Flask-User.git@v0.6.21#egg=flask-user # pyup: <0.7 # pin until 1.0 is ready for prod


I like how easy this was.

Almost :)

Paul helped me repro this, but you'll need to increment the version in your fork if you want it to be installed over (ie replace) the vanilla un-forked flask-user. If you create a fresh virtual environment this will work, but won't work for a venv that already has flask-user installed since pip thinks it's already installed at the same version.

You'll want to pick a version that's higher than v0.6.21, but that won't conflict with the author's next release. Judging from their previous releases, I would suggest v0.6.21.1.

Also, I'm not sure how picky pip is when comparing versions, or installing from git sources using a git tag. If you have any issues, I would suggest your git tag exactly match the version in requirements.txt, ie don't include _with_password_failed_signal :)

Took a while to figure out, but you can force pip to use the version specified in the egg param. I ended up doing this:

git+https://github.com/uwcirg/[email protected]#egg=flask-user==0.6.21.1 # pyup: <0.7 # pin until 1.0 is ready for prod

where v0.6.21.1 is the branch and flask-user==0.6.21.1 tells pip that it's installing version 0.6.21.1.

Nice catch BTW. This would have broken the build 😮

Ah nice! I've only figured out what works for my narrow use-case, will have to try that in the future :)

pbugni

Looks great Ryan.

pbugni · 2018-08-16T17:05:51Z

portal/views/extend_flask_user.py

+            # can populate self.password.errors
+            super(LoginForm, self).validate()
+
+            auditable_event(


Yes, keep this. When we get support calls about users not being able to log in - it'll be helpful to have the available records.

ivan-c · 2018-08-16T18:28:00Z

portal/migrations/versions/e503ae1c261a_.py

+    op.add_column(
+        'users',
+        sa.Column(
+            'last_password_verification_failure',


In the future I would suggest using redis for storing data that could be considered ephemeral (flask-user similarly uses redis to store server-side session data).

Redis also has the ability to automatically expire keys which could slightly simplify the timeout logic

No need for any change- wouldn't be worth the effort to refactor

Thanks for calling this out. When I started thinking about this work I didn't want to store it in a DB because it doesn't make perfect sense there, but didn't know where else to put it. Wish I had spoken with you last week! Will keep redis in mind for the future. I've used it in the past for other stuff. Would have been a great place to put it.

ivan-c · 2018-08-16T18:35:30Z

portal/views/extend_flask_user.py

+
+            error_message = _('We see you\'re having trouble - let us help. \
+                Your account will now be locked while we give it a refresh. \
+                Please try again in 30 minutes. \


Can you replace the 30 with a variable (see below)? In we ever need to change the timeout value, it will save us the effort of re-translating it.

error_message = _('...Please try again in %d minutes...', 30)

Oooo, good call

ivan-c · 2018-08-16T18:50:23Z

requirements.txt

@@ -32,7 +32,7 @@ flask-session==0.3.1
 flask-sqlalchemy==2.3.2
 flask-swagger==0.2.13
 flask-testing==0.7.1
-flask-user==0.6.21 # pyup: <0.7 # pin until 1.0 is ready for prod
+git+https://github.com/uwcirg/Flask-User.git@v0.6.21#egg=flask-user # pyup: <0.7 # pin until 1.0 is ready for prod


Almost :)

Paul helped me repro this, but you'll need to increment the version in your fork if you want it to be installed over (ie replace) the vanilla un-forked flask-user. If you create a fresh virtual environment this will work, but won't work for a venv that already has flask-user installed since pip thinks it's already installed at the same version.

You'll want to pick a version that's higher than v0.6.21, but that won't conflict with the author's next release. Judging from their previous releases, I would suggest v0.6.21.1.

Also, I'm not sure how picky pip is when comparing versions, or installing from git sources using a git tag. If you have any issues, I would suggest your git tag exactly match the version in requirements.txt, ie don't include _with_password_failed_signal :)

CodeRhymesLife · 2018-08-17T21:59:23Z

working on UTs now. Next time I'll try and do these first.

ivan-c

Looks great, thanks!

ivan-c · 2018-08-21T19:33:34Z

tests/test_auth.py

@@ -18,6 +18,8 @@
 from portal.models.intervention import INTERVENTION
 from portal.models.role import ROLE
 from portal.models.user import (
+    LOCKOUT_PERIOD,


Can you move these default/constant values to config.py in a subsequent PR to keep them consistent?

Created #2578 to track this.

CodeRhymesLife added 2 commits August 15, 2018 15:37

Lockout rough draft

5f2f218

Clean up lockout. Everything but unit tests

996c545

CodeRhymesLife requested review from pbugni and ivan-c August 16, 2018 08:05

CodeRhymesLife added 2 commits August 16, 2018 01:13

Self and pep8 code review

1d1cb82

Fix pep8 issue

927f9e3

Remove newline in login.py

b17de1c

CodeRhymesLife commented Aug 16, 2018

View reviewed changes

pbugni approved these changes Aug 16, 2018

View reviewed changes

ivan-c requested changes Aug 16, 2018

View reviewed changes

CodeRhymesLife added 2 commits August 17, 2018 14:21

Increment version in fork of flask-user

be24ace

Address code reviews from Ivan and Paul

98c36a6

uwcirg deleted a comment Aug 17, 2018

uwcirg deleted a comment from CodeRhymesLife Aug 17, 2018

CodeRhymesLife added 2 commits August 20, 2018 00:41

changing machines

aa48a30

Add tests

5ef418f

uwcirg deleted a comment Aug 20, 2018

uwcirg deleted a comment from CodeRhymesLife Aug 20, 2018

Fix style issues

8943c2e

uwcirg deleted a comment Aug 20, 2018

ivan-c approved these changes Aug 21, 2018

View reviewed changes

ivan-c merged commit a9c1b0f into uwcirg:develop Aug 21, 2018

CodeRhymesLife mentioned this pull request Aug 21, 2018

Move Lockout Values to config.py #2578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TN-977 Lockout users if they fail password verification 5 times #2567

TN-977 Lockout users if they fail password verification 5 times #2567

CodeRhymesLife commented Aug 16, 2018 •

edited

Loading

pep8speaks commented Aug 16, 2018 •

edited

Loading

CodeRhymesLife commented Aug 16, 2018

CodeRhymesLife left a comment

CodeRhymesLife Aug 16, 2018

CodeRhymesLife Aug 16, 2018 •

edited

Loading

ivan-c Aug 17, 2018

CodeRhymesLife Aug 16, 2018

CodeRhymesLife Aug 16, 2018

pbugni Aug 16, 2018

CodeRhymesLife Aug 16, 2018

CodeRhymesLife Aug 16, 2018

ivan-c Aug 16, 2018

CodeRhymesLife Aug 17, 2018

CodeRhymesLife Aug 17, 2018

ivan-c Aug 17, 2018

pbugni left a comment

pbugni Aug 16, 2018

ivan-c Aug 16, 2018

CodeRhymesLife Aug 17, 2018

ivan-c Aug 16, 2018

CodeRhymesLife Aug 17, 2018

ivan-c Aug 16, 2018

CodeRhymesLife commented Aug 17, 2018

ivan-c left a comment

ivan-c Aug 21, 2018

CodeRhymesLife Aug 21, 2018

CodeRhymesLife Aug 21, 2018

TN-977 Lockout users if they fail password verification 5 times #2567

TN-977 Lockout users if they fail password verification 5 times #2567

Conversation

CodeRhymesLife commented Aug 16, 2018 • edited Loading

pep8speaks commented Aug 16, 2018 • edited Loading

Comment last updated on August 20, 2018 at 09:52 Hours UTC

CodeRhymesLife commented Aug 16, 2018

CodeRhymesLife left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeRhymesLife Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbugni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeRhymesLife commented Aug 17, 2018

ivan-c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeRhymesLife commented Aug 16, 2018 •

edited

Loading

pep8speaks commented Aug 16, 2018 •

edited

Loading

CodeRhymesLife Aug 16, 2018 •

edited

Loading