From 5f2f218eaa07e8cfbe1128915cc25efd549498ee Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Wed, 15 Aug 2018 15:37:04 -0700
Subject: [PATCH 01/10] Lockout rough draft

---
 portal/factories/app.py                     |  6 ++-
 portal/migrations/versions/e503ae1c261a_.py | 41 +++++++++++++++++++++
 portal/models/login.py                      |  4 ++
 portal/models/user.py                       | 33 ++++++++++++++++-
 portal/views/auth.py                        | 11 +++++-
 portal/views/extend_flask_user.py           | 29 +++++++++++++++
 requirements.txt                            |  2 +-
 7 files changed, 122 insertions(+), 4 deletions(-)
 create mode 100644 portal/migrations/versions/e503ae1c261a_.py

diff --git a/portal/factories/app.py b/portal/factories/app.py
index 9e90e6e3e1..89615de4e3 100644
--- a/portal/factories/app.py
+++ b/portal/factories/app.py
@@ -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
@@ -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,
diff --git a/portal/migrations/versions/e503ae1c261a_.py b/portal/migrations/versions/e503ae1c261a_.py
new file mode 100644
index 0000000000..f0d35ce7b0
--- /dev/null
+++ b/portal/migrations/versions/e503ae1c261a_.py
@@ -0,0 +1,41 @@
+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',
+            sa.DateTime(),
+            nullable=True
+        )
+    )
+    op.add_column('users',
+        sa.Column(
+            'password_verification_failures',
+            sa.Integer(),
+            nullable=True
+        )
+    )
+    # ### 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 ###
diff --git a/portal/models/login.py b/portal/models/login.py
index c36a5507b4..7124d3c4eb 100644
--- a/portal/models/login.py
+++ b/portal/models/login.py
@@ -20,6 +20,10 @@ def login_user(user, auth_method):
     if not _call_or_get(flask_login_current_user.is_authenticated):
         flask_user_login(user)
     initiate_encounter(user, auth_method)
+
+    # After a successfull login make sure lockout is reset
+    user.reset_lockout();
+
     # remove session var used to capture locale_code prior to login, now
     # that we have a user.
     if 'locale_code' in session:
diff --git a/portal/models/user.py b/portal/models/user.py
index 3e4d521eef..7f777e3348 100644
--- a/portal/models/user.py
+++ b/portal/models/user.py
@@ -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
 
@@ -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'
@@ -270,6 +274,9 @@ 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)
+    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',
@@ -681,6 +688,30 @@ def locale_name_from_code(locale_code):
 
         return locale_options
 
+    @property
+    def is_locked_out(self):
+        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()
+            return False
+
+        return self.password_verification_failures > PERMITTED_FAILED_LOGIN_ATTEMPTS
+
+    def reset_lockout(self):
+        self.password_verification_failures = 0
+        self.last_password_verification_failure = None
+        db.session.commit()
+
+    def add_password_verification_failure(self):
+        self.last_password_verification_failure = datetime.utcnow()
+        self.password_verification_failures += 1
+        db.session.commit()
+
     def add_organization(self, organization_name):
         """Shortcut to add a clinic/organization by name"""
         org = Organization.query.filter_by(name=organization_name).one()
diff --git a/portal/views/auth.py b/portal/views/auth.py
index 0349d0ec39..480b64c45a 100644
--- a/portal/views/auth.py
+++ b/portal/views/auth.py
@@ -26,6 +26,7 @@
 from flask_user.signals import (
     user_changed_password,
     user_logged_in,
+    user_password_failed,
     user_registered,
     user_reset_password,
 )
@@ -363,6 +364,13 @@ def flask_user_login_event(app, user, **extra):
                     context='login')
     login_user(user, 'password_authenticated')
 
+def flask_user_password_failed_event(app, user, **extra):
+    auditable_event(
+        'local user failed password verification', user_id=user.id,
+        subject_id=user.id, context='account'
+    )
+    user.add_password_verification_failure()
+
 
 def flask_user_registered_event(app, user, **extra):
     auditable_event(
@@ -384,9 +392,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)
 
 
diff --git a/portal/views/extend_flask_user.py b/portal/views/extend_flask_user.py
index 290ccad646..f300401765 100644
--- a/portal/views/extend_flask_user.py
+++ b/portal/views/extend_flask_user.py
@@ -1,5 +1,7 @@
 """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 ..models.role import ROLE
@@ -40,3 +42,30 @@ 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)
+
+
+def login_view():
+    """Logs in users with unlocked accounts
+
+    If the user's account is unlocked call the default login view.
+    Otherwise, return a message to the user.
+    """
+    pass
+
+
+class LockoutLoginForm(LoginForm):
+    def validate(self):
+        # 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 user.is_locked_out:
+            current_app.logger.warn(
+                'User attempted to login but their account is locked'
+            )
+            error_message = 'Your account had been locked out'
+            self.password.errors.append(error_message)
+            return False
+
+        return super(LockoutLoginForm, self).validate()
+
diff --git a/requirements.txt b/requirements.txt
index 0459567e4a..c892dc681e 100644
--- a/requirements.txt
+++ b/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
 flask-webtest==0.0.9
 flask-wtf==0.14.2         # via flask-user
 flask==1.0.2

From 996c545c625daa5440582a1519324601c15fca92 Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Thu, 16 Aug 2018 00:51:04 -0700
Subject: [PATCH 02/10] Clean up lockout. Everything but unit tests

---
 portal/migrations/versions/e503ae1c261a_.py | 10 +++--
 portal/models/user.py                       | 30 ++++++++++++--
 portal/views/auth.py                        | 16 +++++--
 portal/views/extend_flask_user.py           | 46 ++++++++++++++-------
 4 files changed, 79 insertions(+), 23 deletions(-)

diff --git a/portal/migrations/versions/e503ae1c261a_.py b/portal/migrations/versions/e503ae1c261a_.py
index f0d35ce7b0..fe825f6758 100644
--- a/portal/migrations/versions/e503ae1c261a_.py
+++ b/portal/migrations/versions/e503ae1c261a_.py
@@ -17,18 +17,22 @@
 
 def upgrade():
     # ### commands auto generated by Alembic - please adjust! ###
-    op.add_column('users',
+    op.add_column(
+        'users',
         sa.Column(
             'last_password_verification_failure',
             sa.DateTime(),
             nullable=True
         )
     )
-    op.add_column('users',
+
+    op.add_column(
+        'users',
         sa.Column(
             'password_verification_failures',
             sa.Integer(),
-            nullable=True
+            nullable=False,
+            server_default=sa.text('0')
         )
     )
     # ### end Alembic commands ###
diff --git a/portal/models/user.py b/portal/models/user.py
index 7f777e3348..d5fa4091d0 100644
--- a/portal/models/user.py
+++ b/portal/models/user.py
@@ -274,7 +274,8 @@ 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)
+    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',
@@ -690,6 +691,12 @@ def locale_name_from_code(locale_code):
 
     @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
 
@@ -698,20 +705,37 @@ def is_locked_out(self):
             datetime.utcnow() - self.last_password_verification_failure
         if time_since_last_failure >= LOCKOUT_PERIOD:
             self.reset_lockout()
-            return False
 
-        return self.password_verification_failures > PERMITTED_FAILED_LOGIN_ATTEMPTS
+        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()
diff --git a/portal/views/auth.py b/portal/views/auth.py
index 480b64c45a..15d754ca8e 100644
--- a/portal/views/auth.py
+++ b/portal/views/auth.py
@@ -364,12 +364,22 @@ def flask_user_login_event(app, user, **extra):
                     context='login')
     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(
-        'local user failed password verification', user_id=user.id,
-        subject_id=user.id, context='account'
+        'local user failed password verification. Count "{}"'.format(
+            count
+        ),
+        user_id=user.id,
+        subject_id=user.id,
+        context='login'
     )
-    user.add_password_verification_failure()
 
 
 def flask_user_registered_event(app, user, **extra):
diff --git a/portal/views/extend_flask_user.py b/portal/views/extend_flask_user.py
index f300401765..e219d94ed4 100644
--- a/portal/views/extend_flask_user.py
+++ b/portal/views/extend_flask_user.py
@@ -3,7 +3,9 @@
 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
@@ -44,28 +46,44 @@ def reset_password_view_function(token):
     return challenge_identity(user_id=user_id, next_url=next_url)
 
 
-def login_view():
-    """Logs in users with unlocked accounts
-
-    If the user's account is unlocked call the default login view.
-    Otherwise, return a message to the user.
-    """
-    pass
-
-
 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_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:
-            current_app.logger.warn(
-                'User attempted to login but their account is locked'
+            # Make sure validators are run so we
+            # can populate self.password.errors
+            super(LoginForm, self).validate()
+
+            auditable_event(
+                'local user attempted to login after being locked out',
+                user_id=user.id,
+                subject_id=user.id,
+                context='login'
             )
-            error_message = 'Your account had been locked out'
+
+            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. \
+                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()
-

From 1d1cb82bb971328b5a134b5fc5382fd2afa2d895 Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Thu, 16 Aug 2018 01:13:13 -0700
Subject: [PATCH 03/10] Self and pep8 code review

---
 portal/factories/app.py | 2 +-
 portal/models/login.py  | 3 ---
 portal/models/user.py   | 4 ++--
 portal/views/auth.py    | 4 ++++
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/portal/factories/app.py b/portal/factories/app.py
index 89615de4e3..da1c4ddeea 100644
--- a/portal/factories/app.py
+++ b/portal/factories/app.py
@@ -197,7 +197,7 @@ def configure_extensions(app):
     user_manager.init_app(
         app,
         forgot_password_view_function=patch_forgot_password,
-        login_form =LockoutLoginForm,
+        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,
diff --git a/portal/models/login.py b/portal/models/login.py
index 7124d3c4eb..c59b659eb3 100644
--- a/portal/models/login.py
+++ b/portal/models/login.py
@@ -21,9 +21,6 @@ def login_user(user, auth_method):
         flask_user_login(user)
     initiate_encounter(user, auth_method)
 
-    # After a successfull login make sure lockout is reset
-    user.reset_lockout();
-
     # remove session var used to capture locale_code prior to login, now
     # that we have a user.
     if 'locale_code' in session:
diff --git a/portal/models/user.py b/portal/models/user.py
index d5fa4091d0..3001a511a8 100644
--- a/portal/models/user.py
+++ b/portal/models/user.py
@@ -275,7 +275,7 @@ class User(db.Model, UserMixin):
     confirmed_at = db.Column(db.DateTime())
 
     password_verification_failures = \
-            db.Column(db.Integer, default=0, nullable=False)
+        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',
@@ -707,7 +707,7 @@ def is_locked_out(self):
             self.reset_lockout()
 
         failures = self.password_verification_failures
-        return failures > PERMITTED_FAILED_LOGIN_ATTEMPTS
+        return failures >= PERMITTED_FAILED_LOGIN_ATTEMPTS
 
     def reset_lockout(self):
         """resets variables that track lockout
diff --git a/portal/views/auth.py b/portal/views/auth.py
index 15d754ca8e..ce4dadb1c3 100644
--- a/portal/views/auth.py
+++ b/portal/views/auth.py
@@ -362,6 +362,10 @@ 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')
 
 

From 927f9e37f27a1d6ad0104d3f312f37b6cc2d1f96 Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Thu, 16 Aug 2018 01:14:20 -0700
Subject: [PATCH 04/10] Fix pep8 issue

---
 portal/views/auth.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/portal/views/auth.py b/portal/views/auth.py
index ce4dadb1c3..25781a3e45 100644
--- a/portal/views/auth.py
+++ b/portal/views/auth.py
@@ -362,9 +362,9 @@ 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();
+    user.reset_lockout()
 
     login_user(user, 'password_authenticated')
 

From b17de1c0323c450d964176bb1d29e6439e0051bb Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Thu, 16 Aug 2018 01:17:39 -0700
Subject: [PATCH 05/10] Remove newline in login.py

---
 portal/models/login.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/portal/models/login.py b/portal/models/login.py
index c59b659eb3..c36a5507b4 100644
--- a/portal/models/login.py
+++ b/portal/models/login.py
@@ -20,7 +20,6 @@ def login_user(user, auth_method):
     if not _call_or_get(flask_login_current_user.is_authenticated):
         flask_user_login(user)
     initiate_encounter(user, auth_method)
-
     # remove session var used to capture locale_code prior to login, now
     # that we have a user.
     if 'locale_code' in session:

From be24acea97e12bd13f245b83739569ab02ee1a23 Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Fri, 17 Aug 2018 14:21:13 -0700
Subject: [PATCH 06/10] Increment version in fork of flask-user

---
 requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/requirements.txt b/requirements.txt
index c892dc681e..5b945d11d3 100644
--- a/requirements.txt
+++ b/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
-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
+git+https://github.com/uwcirg/Flask-User.git@v0.6.21.1#egg=flask-user==0.6.21.1 # pyup: <0.7 # pin until 1.0 is ready for prod
 flask-webtest==0.0.9
 flask-wtf==0.14.2         # via flask-user
 flask==1.0.2

From 98c36a6a4dcd824d8fb08a671c98a049dff002fe Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Fri, 17 Aug 2018 14:55:07 -0700
Subject: [PATCH 07/10] Address code reviews from Ivan and Paul

---
 portal/views/extend_flask_user.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/portal/views/extend_flask_user.py b/portal/views/extend_flask_user.py
index e219d94ed4..bee1f380dc 100644
--- a/portal/views/extend_flask_user.py
+++ b/portal/views/extend_flask_user.py
@@ -3,7 +3,6 @@
 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
@@ -59,11 +58,11 @@ def validate(self):
         """
         # 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)
+        user = user_manager.find_user_by_email(self.email.data)[0]
 
         # If the user is locked out display a message
         # under the password field
-        if user.is_locked_out:
+        if user is not None and user.is_locked_out:
             # Make sure validators are run so we
             # can populate self.password.errors
             super(LoginForm, self).validate()
@@ -77,9 +76,9 @@ def validate(self):
 
             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. \
+                Please try again in %(time)d minutes. \
                 If you\'re still having issues, please click \
-                "Having trouble logging in?" below.')
+                "Having trouble logging in?" below.', time=30)
             self.password.errors.append(error_message)
 
             return False

From aa48a30a8bbe419f0d64922519bcb4c2bef7263c Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Mon, 20 Aug 2018 00:41:56 -0700
Subject: [PATCH 08/10] changing machines

---
 tests/__init__.py  | 21 +++++++++++++++++----
 tests/test_auth.py | 16 ++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/tests/__init__.py b/tests/__init__.py
index 375e18ef86..6fdf9b19d8 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -11,6 +11,7 @@
 from __future__ import unicode_literals  # isort:skip
 
 from datetime import datetime
+from flask import url_for
 from flask_testing import TestCase as Base
 from flask_webtest import SessionScope
 from sqlalchemy.exc import IntegrityError
@@ -156,11 +157,11 @@ def init_data(self):
 
     def add_user(
             self, username, first_name="", last_name="", image_url=None,
-            pre_registered=False):
+            password='fakePa$$'):
         """Create a user and add to test db, and return it"""
-        # Unless testing a pre_registered case, give the user a fake password
-        # so they appear registered
-        password = None if pre_registered else 'fakePa$$'
+        # Hash the password
+        password = self.app.user_manager.hash_password(password)
+        
         test_user = User(
             username=username, first_name=first_name, last_name=last_name,
             image_url=image_url, password=password)
@@ -212,6 +213,18 @@ def login(
             follow_redirects=follow_redirects
         )
 
+    def local_login(self, username, password):
+        """logs in a local user through user.login view"""
+        url = url_for('user.login')
+        return self.client.post(
+            url,
+            data={
+                'username': username,
+                'password': password,
+            },
+            follow_redirects=True
+        )
+
     def add_client(self):
         """Prep db with a test client for test user"""
         self.promote_user(role_name=ROLE.APPLICATION_DEVELOPER.value)
diff --git a/tests/test_auth.py b/tests/test_auth.py
index de8a8f421a..0f0ed4d0f6 100644
--- a/tests/test_auth.py
+++ b/tests/test_auth.py
@@ -55,6 +55,22 @@ def test_local_user_add(self):
         new_user = User.query.filter_by(username=data['email']).first()
         assert new_user.active is True
 
+    def test_local_login_valid_username_and_password(self):
+        """login through the login form"""
+        username = 'localuser'
+        password = 'Password1'
+        user = self.add_user(username=username, password=password)
+        response = self.local_login(username, password)
+        assert response.status_code is 200
+
+    def test_local_login_valid_username_invalid_password(self):
+        """login through the login form"""
+        username = 'localuser'
+        password = 'Password1'
+        user = self.add_user(username=username, password=password)
+        response = self.local_login(username, 'invalidpassword')
+        assert response.status_code is 302
+
     def test_register_now(self):
         """Initiate process to register exiting account"""
         self.app.config['NO_CHALLENGE_WO_DATA'] = False

From 5ef418f34c06fec7acad2af8344af7c33fb5ee42 Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Mon, 20 Aug 2018 02:38:37 -0700
Subject: [PATCH 09/10] Add tests

---
 portal/models/user.py |   2 +-
 tests/__init__.py     |  10 ++--
 tests/test_auth.py    | 109 ++++++++++++++++++++++++++++++++++++++----
 3 files changed, 108 insertions(+), 13 deletions(-)

diff --git a/portal/models/user.py b/portal/models/user.py
index 3001a511a8..f6baf4e52a 100644
--- a/portal/models/user.py
+++ b/portal/models/user.py
@@ -707,7 +707,7 @@ def is_locked_out(self):
             self.reset_lockout()
 
         failures = self.password_verification_failures
-        return failures >= PERMITTED_FAILED_LOGIN_ATTEMPTS
+        return failures > PERMITTED_FAILED_LOGIN_ATTEMPTS
 
     def reset_lockout(self):
         """resets variables that track lockout
diff --git a/tests/__init__.py b/tests/__init__.py
index 6fdf9b19d8..0e2b50ebd3 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -157,7 +157,7 @@ def init_data(self):
 
     def add_user(
             self, username, first_name="", last_name="", image_url=None,
-            password='fakePa$$'):
+            password='fakePa$$', email=None):
         """Create a user and add to test db, and return it"""
         # Hash the password
         password = self.app.user_manager.hash_password(password)
@@ -165,6 +165,8 @@ def add_user(
         test_user = User(
             username=username, first_name=first_name, last_name=last_name,
             image_url=image_url, password=password)
+        if email is not None:
+            test_user.email = email
         with SessionScope(db):
             db.session.add(test_user)
             db.session.commit()
@@ -213,16 +215,16 @@ def login(
             follow_redirects=follow_redirects
         )
 
-    def local_login(self, username, password):
+    def local_login(self, email, password, follow_redirects=True):
         """logs in a local user through user.login view"""
         url = url_for('user.login')
         return self.client.post(
             url,
             data={
-                'username': username,
+                'email': email,
                 'password': password,
             },
-            follow_redirects=True
+            follow_redirects=follow_redirects
         )
 
     def add_client(self):
diff --git a/tests/test_auth.py b/tests/test_auth.py
index 0f0ed4d0f6..779bab4ec5 100644
--- a/tests/test_auth.py
+++ b/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,
+    PERMITTED_FAILED_LOGIN_ATTEMPTS,
     RoleError,
     User,
     UserRelationship,
@@ -57,19 +59,110 @@ def test_local_user_add(self):
 
     def test_local_login_valid_username_and_password(self):
         """login through the login form"""
-        username = 'localuser'
+        # Create a user
+        email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username=username, password=password)
-        response = self.local_login(username, password)
+        user = self.add_user(username='username', email=email, password=password)
+
+        # Attempt to login with valid creds
+        response = self.local_login(user.email, password)
+
+        # Validate login was successful
+        assert response.status_code is 200
+        assert user.password_verification_failures == 0
+
+    def test_local_login_valid_username_invalid_password_increments_lockout(self):
+        """login through the login form"""
+        # Create a user
+        email = 'localuser@test.com'
+        password = 'Password1'
+        user = self.add_user(username='username', email=email, password=password)
+
+        # Attempt to login with an invalid password
+        response = self.local_login(user.email, 'invalidpassword')
+
+        # Verify there was a password failure
+        db.session.refresh(user)
+        assert user.password_verification_failures == 1
+
+    def test_local_login_valid_username_and_password_resets_lockout(self):
+        """login through the login form"""
+        # Create a user
+        email = 'localuser@test.com'
+        password = 'Password1'
+        user = self.add_user(username='username', email=email, password=password)
+
+        # Mock a failed password attempt
+        user.add_password_verification_failure()
+        assert user.password_verification_failures == 1
+
+        # Atempt to login with valid creds
+        response = self.local_login(user.email, password)
+
+        # Verify lockout was reset
+        db.session.refresh(user)
+        assert user.password_verification_failures == 0
+
+    def test_local_login_lockout_after_unsuccessful_attempts(self):
+        """login through the login form"""
+        email = 'localuser@test.com'
+        password = 'Password1'
+        user = self.add_user(username='username', email=email, password=password)
+
+        # Use up all of the permitted login attempts
+        for failureIndex in range(0, PERMITTED_FAILED_LOGIN_ATTEMPTS):
+            response = self.local_login(user.email, 'invalidpassword')
+            assert response.status_code is 200
+
+            db.session.refresh(user)
+            assert user.password_verification_failures == (failureIndex + 1)
+            assert user.is_locked_out == False
+
+        # Attempt to login with invalid creds
+        response = self.local_login(user.email, 'invalidpassword')
         assert response.status_code is 200
 
-    def test_local_login_valid_username_invalid_password(self):
+        # Validate that after another failed attempt
+        # the user is locked out
+        db.session.refresh(user)
+        assert user.is_locked_out == True
+
+    def test_local_login_verify_lockout_resets_after_lockout_period(self):
+        """login through the login form"""
+        email = 'localuser@test.com'
+        password = 'Password1'
+        user = self.add_user(username='username', email=email, password=password)
+
+        # Lock the user out
+        for failureIndex in range(0, PERMITTED_FAILED_LOGIN_ATTEMPTS + 1):
+            user.add_password_verification_failure()
+
+        # Verify the user is locked out
+        assert user.is_locked_out == True
+
+        # Move time to the end of the lockout period
+        user.last_password_verification_failure = datetime.datetime.utcnow() - LOCKOUT_PERIOD
+
+        # Verify we are no longer locked out
+        assert user.is_locked_out == False
+
+    def test_local_login_verify_cant_login_when_locked_out(self):
         """login through the login form"""
-        username = 'localuser'
+        email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username=username, password=password)
-        response = self.local_login(username, 'invalidpassword')
-        assert response.status_code is 302
+        user = self.add_user(username='username', email=email, password=password)
+
+        # Lock the user out
+        for failureIndex in range(0, PERMITTED_FAILED_LOGIN_ATTEMPTS + 1):
+            user.add_password_verification_failure()
+
+        assert user.is_locked_out == True
+
+        # Atempt to login with valid creds
+        response = self.local_login(user.email, password)
+
+        # Verify the user is still locked out
+        assert user.is_locked_out == True
 
     def test_register_now(self):
         """Initiate process to register exiting account"""

From 8943c2eb273bfdda57410257fdcec2fb81ee4190 Mon Sep 17 00:00:00 2001
From: Ryan James <rcjames1004@gmail.com>
Date: Mon, 20 Aug 2018 02:51:55 -0700
Subject: [PATCH 10/10] Fix style issues

---
 tests/__init__.py  |  2 +-
 tests/test_auth.py | 53 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/tests/__init__.py b/tests/__init__.py
index 0e2b50ebd3..54499f8419 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -161,7 +161,7 @@ def add_user(
         """Create a user and add to test db, and return it"""
         # Hash the password
         password = self.app.user_manager.hash_password(password)
-        
+
         test_user = User(
             username=username, first_name=first_name, last_name=last_name,
             image_url=image_url, password=password)
diff --git a/tests/test_auth.py b/tests/test_auth.py
index 779bab4ec5..1fc3fb2ab4 100644
--- a/tests/test_auth.py
+++ b/tests/test_auth.py
@@ -62,7 +62,11 @@ def test_local_login_valid_username_and_password(self):
         # Create a user
         email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username='username', email=email, password=password)
+        user = self.add_user(
+            username='username',
+            email=email,
+            password=password
+        )
 
         # Attempt to login with valid creds
         response = self.local_login(user.email, password)
@@ -71,12 +75,16 @@ def test_local_login_valid_username_and_password(self):
         assert response.status_code is 200
         assert user.password_verification_failures == 0
 
-    def test_local_login_valid_username_invalid_password_increments_lockout(self):
+    def test_local_login_failure_increments_lockout(self):
         """login through the login form"""
         # Create a user
         email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username='username', email=email, password=password)
+        user = self.add_user(
+            username='username',
+            email=email,
+            password=password
+        )
 
         # Attempt to login with an invalid password
         response = self.local_login(user.email, 'invalidpassword')
@@ -90,7 +98,11 @@ def test_local_login_valid_username_and_password_resets_lockout(self):
         # Create a user
         email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username='username', email=email, password=password)
+        user = self.add_user(
+            username='username',
+            email=email,
+            password=password
+        )
 
         # Mock a failed password attempt
         user.add_password_verification_failure()
@@ -107,7 +119,11 @@ def test_local_login_lockout_after_unsuccessful_attempts(self):
         """login through the login form"""
         email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username='username', email=email, password=password)
+        user = self.add_user(
+            username='username',
+            email=email,
+            password=password
+        )
 
         # Use up all of the permitted login attempts
         for failureIndex in range(0, PERMITTED_FAILED_LOGIN_ATTEMPTS):
@@ -116,7 +132,7 @@ def test_local_login_lockout_after_unsuccessful_attempts(self):
 
             db.session.refresh(user)
             assert user.password_verification_failures == (failureIndex + 1)
-            assert user.is_locked_out == False
+            assert not user.is_locked_out
 
         # Attempt to login with invalid creds
         response = self.local_login(user.email, 'invalidpassword')
@@ -125,44 +141,53 @@ def test_local_login_lockout_after_unsuccessful_attempts(self):
         # Validate that after another failed attempt
         # the user is locked out
         db.session.refresh(user)
-        assert user.is_locked_out == True
+        assert user.is_locked_out
 
     def test_local_login_verify_lockout_resets_after_lockout_period(self):
         """login through the login form"""
         email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username='username', email=email, password=password)
+        user = self.add_user(
+            username='username',
+            email=email,
+            password=password
+        )
 
         # Lock the user out
         for failureIndex in range(0, PERMITTED_FAILED_LOGIN_ATTEMPTS + 1):
             user.add_password_verification_failure()
 
         # Verify the user is locked out
-        assert user.is_locked_out == True
+        assert user.is_locked_out
 
         # Move time to the end of the lockout period
-        user.last_password_verification_failure = datetime.datetime.utcnow() - LOCKOUT_PERIOD
+        user.last_password_verification_failure = \
+            datetime.datetime.utcnow() - LOCKOUT_PERIOD
 
         # Verify we are no longer locked out
-        assert user.is_locked_out == False
+        assert not user.is_locked_out
 
     def test_local_login_verify_cant_login_when_locked_out(self):
         """login through the login form"""
         email = 'localuser@test.com'
         password = 'Password1'
-        user = self.add_user(username='username', email=email, password=password)
+        user = self.add_user(
+            username='username',
+            email=email,
+            password=password
+        )
 
         # Lock the user out
         for failureIndex in range(0, PERMITTED_FAILED_LOGIN_ATTEMPTS + 1):
             user.add_password_verification_failure()
 
-        assert user.is_locked_out == True
+        assert user.is_locked_out
 
         # Atempt to login with valid creds
         response = self.local_login(user.email, password)
 
         # Verify the user is still locked out
-        assert user.is_locked_out == True
+        assert user.is_locked_out
 
     def test_register_now(self):
         """Initiate process to register exiting account"""