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

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

Merged
merged 10 commits into from
Aug 21, 2018
6 changes: 5 additions & 1 deletion portal/factories/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@
from ..views.clinical import clinical_api
from ..views.coredata import coredata_api
from ..views.demographics import demographics_api
from ..views.extend_flask_user import reset_password_view_function
from ..views.extend_flask_user import (
LockoutLoginForm,
reset_password_view_function,
)
from ..views.fhir import fhir_api
from ..views.filters import filters_blueprint
from ..views.group import group_api
Expand Down Expand Up @@ -194,6 +197,7 @@ def configure_extensions(app):
user_manager.init_app(
app,
forgot_password_view_function=patch_forgot_password,
login_form=LockoutLoginForm,
send_email_function=patch_send_email,
make_safe_url_function=patch_make_safe_url,
reset_password_view_function=reset_password_view_function,
Expand Down
45 changes: 45 additions & 0 deletions portal/migrations/versions/e503ae1c261a_.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
from alembic import op
import sqlalchemy as sa


"""empty message

Revision ID: e503ae1c261a
Revises: a031e26dc1bd
Create Date: 2018-08-14 16:05:28.104848

"""

# revision identifiers, used by Alembic.
revision = 'e503ae1c261a'
down_revision = 'a031e26dc1bd'


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.add_column(
'users',
sa.Column(
'last_password_verification_failure',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

sa.DateTime(),
nullable=True
)
)

op.add_column(
'users',
sa.Column(
'password_verification_failures',
sa.Integer(),
nullable=False,
server_default=sa.text('0')
)
)
# ### end Alembic commands ###


def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_column('users', 'password_verification_failures')
op.drop_column('users', 'last_password_verification_failure')
# ### end Alembic commands ###
57 changes: 56 additions & 1 deletion portal/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
standard_library.install_aliases() # noqa: E402

from cgi import escape
from datetime import datetime
from datetime import datetime, timedelta
from io import StringIO
import time

Expand Down Expand Up @@ -231,6 +231,10 @@ def validate_email(email):
raise BadRequest("requires a valid email address")


LOCKOUT_PERIOD = timedelta(minutes=30)
PERMITTED_FAILED_LOGIN_ATTEMPTS = 5


class User(db.Model, UserMixin):
# PLEASE maintain merge_with() as user model changes #
__tablename__ = 'users' # Override default 'user'
Expand Down Expand Up @@ -270,6 +274,10 @@ class User(db.Model, UserMixin):
reset_password_token = db.Column(db.String(100))
confirmed_at = db.Column(db.DateTime())

password_verification_failures = \
db.Column(db.Integer, default=0, nullable=False)
last_password_verification_failure = db.Column(db.DateTime, nullable=True)

user_audits = db.relationship('Audit', cascade='delete',
foreign_keys=[Audit.user_id])
subject_audits = db.relationship('Audit', cascade='delete',
Expand Down Expand Up @@ -681,6 +689,53 @@ def locale_name_from_code(locale_code):

return locale_options

@property
def is_locked_out(self):
"""tells if user is temporarily locked out

To slow down brute force password attacks we temporarily
lock users out of the system for a short period of time.
This property tells whether or not the user is locked out.
"""
if self.password_verification_failures == 0:
return False

# If we're not in the lockout window reset everything
time_since_last_failure = \
datetime.utcnow() - self.last_password_verification_failure
if time_since_last_failure >= LOCKOUT_PERIOD:
self.reset_lockout()

failures = self.password_verification_failures
return failures >= PERMITTED_FAILED_LOGIN_ATTEMPTS

def reset_lockout(self):
"""resets variables that track lockout

We track when the user fails password verification
to lockout users when they fail too many times.
This function resets those variables
"""
self.password_verification_failures = 0
self.last_password_verification_failure = None
db.session.commit()

def add_password_verification_failure(self):
"""remembers when a user fails password verification

Each time a user fails password verification
this function is called. Use user.is_locked_out
to tell whether this has been called enough times
to lock the user out of the system

:returns: total failures since last reset
"""
self.last_password_verification_failure = datetime.utcnow()
self.password_verification_failures += 1
db.session.commit()

return self.password_verification_failures

def add_organization(self, organization_name):
"""Shortcut to add a clinic/organization by name"""
org = Organization.query.filter_by(name=organization_name).one()
Expand Down
25 changes: 24 additions & 1 deletion portal/views/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from flask_user.signals import (
user_changed_password,
user_logged_in,
user_password_failed,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

user_registered,
user_reset_password,
)
Expand Down Expand Up @@ -361,9 +362,30 @@ def base64_url_decode(s):
def flask_user_login_event(app, user, **extra):
auditable_event("local user login", user_id=user.id, subject_id=user.id,
context='login')

# After a successfull login make sure lockout is reset
user.reset_lockout()

login_user(user, 'password_authenticated')


def flask_user_password_failed_event(app, user, **extra):
"""tracks when a user fails password verification

If this happens too often, for security reasons,
the user will be locked out of the system.
"""
count = user.add_password_verification_failure()
auditable_event(
Copy link
Contributor Author

@CodeRhymesLife CodeRhymesLife Aug 16, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

'local user failed password verification. Count "{}"'.format(
count
),
user_id=user.id,
subject_id=user.id,
context='login'
)


def flask_user_registered_event(app, user, **extra):
auditable_event(
"local user registered", user_id=user.id, subject_id=user.id,
Expand All @@ -384,9 +406,10 @@ def flask_user_changed_password(app, user, **extra):


# Register functions to receive signals from flask_user
user_changed_password.connect(flask_user_changed_password)
user_logged_in.connect(flask_user_login_event)
user_password_failed.connect(flask_user_password_failed_event)
user_registered.connect(flask_user_registered_event)
user_changed_password.connect(flask_user_changed_password)
user_reset_password.connect(flask_user_changed_password)


Expand Down
47 changes: 47 additions & 0 deletions portal/views/extend_flask_user.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
"""Module to extend or specialize flask user views for our needs"""
from flask import abort, current_app, session, url_for
from flask_user.forms import LoginForm
from flask_user.translations import lazy_gettext as _
from flask_user.views import reset_password
from flask_wtf import FlaskForm

from ..audit import auditable_event
from ..models.role import ROLE
from ..models.user import get_user_or_abort
from .portal import challenge_identity
Expand Down Expand Up @@ -40,3 +44,46 @@ def reset_password_view_function(token):

next_url = url_for('user.reset_password', token=token)
return challenge_identity(user_id=user_id, next_url=next_url)


class LockoutLoginForm(LoginForm):
"""adds lockout functionality to the login process"""

def validate(self):
"""prevent locked out users from logging in

Before verifying the user's credentials
check to see if the user is locked out.
If the user is locked out display an error
message below the password field.
"""
# Find user by email address (email field)
user_manager = current_app.user_manager
user, user_email = user_manager.find_user_by_email(self.email.data)

# If the user is locked out display a message
# under the password field
if user.is_locked_out:
# Make sure validators are run so we
# can populate self.password.errors
super(LoginForm, self).validate()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignoring this


auditable_event(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

'local user attempted to login after being locked out',
user_id=user.id,
subject_id=user.id,
context='login'
)

error_message = _('We see you\'re having trouble - let us help. \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish this message was longer

Your account will now be locked while we give it a refresh. \
Please try again in 30 minutes. \
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooo, good call

If you\'re still having issues, please click \
"Having trouble logging in?" below.')
self.password.errors.append(error_message)

return False

# If the user is not locked out proceed with
# validating credentials
return super(LockoutLoginForm, self).validate()
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like how easy this was.

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch BTW. This would have broken the build 😮

Copy link
Member

Choose a reason for hiding this comment

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

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

flask-webtest==0.0.9
flask-wtf==0.14.2 # via flask-user
flask==1.0.2
Expand Down