From 3197739539dd58ce8283367c03be3a4a179c8130 Mon Sep 17 00:00:00 2001
From: Leticia Portella <leportella@gmail.com>
Date: Wed, 23 Jan 2019 17:11:20 +0000
Subject: [PATCH] Divide responsabilities while checking blocked users

The previous version to check if a user exceeded a number of failed
logins everything was checked in a single method. This commit splits the
responsabilities between 4 functions with small responsabilites each
---
 nativeauthenticator/nativeauthenticator.py    | 43 +++++++++++++------
 .../tests/test_authenticator.py               | 33 +++++++++-----
 2 files changed, 53 insertions(+), 23 deletions(-)

diff --git a/nativeauthenticator/nativeauthenticator.py b/nativeauthenticator/nativeauthenticator.py
index 64402eb..d454a33 100644
--- a/nativeauthenticator/nativeauthenticator.py
+++ b/nativeauthenticator/nativeauthenticator.py
@@ -33,7 +33,7 @@ class NativeAuthenticator(Authenticator):
         help="""Configures the number of failed attempts a user can have
                 before being blocked."""
     )
-    secs_before_next_try = Integer(
+    seconds_before_next_try = Integer(
         config=True,
         default=600,
         help="""Configures the number of seconds a user has to wait
@@ -52,25 +52,39 @@ def add_new_table(self):
         if 'users_info' not in inspector.get_table_names():
             UserInfo.__table__.create(self.db.bind)
 
-    def exceed_atempts_of_login(self, username):
-        now = datetime.now()
+    def add_login_attempt(self, username):
+        if not self.login_attempts.get(username):
+            self.login_attempts[username] = {'count': 1,
+                                             'time': datetime.now()}
+        else:
+            self.login_attempts[username]['count'] += 1
+            self.login_attempts[username]['time'] = datetime.now()
+
+    def can_try_to_login_again(self, username):
         login_attempts = self.login_attempts.get(username)
         if not login_attempts:
-            self.login_attempts[username] = {'count': 1, 'time': now}
-            return False
+            return True
 
-        time_last_attempt = now - login_attempts['time']
-        if time_last_attempt.seconds > self.secs_before_next_try:
-            self.login_attempts.pop(username)
-            return False
+        time_last_attempt = datetime.now() - login_attempts['time']
+        if time_last_attempt.seconds > self.seconds_before_next_try:
+            return True
 
-        if login_attempts['count'] < self.allowed_failed_logins:
-            self.login_attempts[username]['count'] += 1
-            self.login_attempts[username]['time'] = now
+        return False
+
+    def is_blocked(self, username):
+        logins = self.login_attempts.get(username)
+
+        if not logins or logins['count'] < self.allowed_failed_logins:
             return False
 
+        if self.can_try_to_login_again(username):
+            return False
         return True
 
+    def successful_login(self, username):
+        if self.login_attempts.get(username):
+            self.login_attempts.pop(username)
+
     @gen.coroutine
     def authenticate(self, handler, data):
         username = data['username']
@@ -81,12 +95,15 @@ def authenticate(self, handler, data):
             return
 
         if self.allowed_failed_logins:
-            if self.exceed_atempts_of_login(username):
+            if self.is_blocked(username):
                 return
 
         if user.is_authorized and user.is_valid_password(password):
+            self.successful_login(username)
             return username
 
+        self.add_login_attempt(username)
+
     def is_password_common(self, password):
         common_credentials_file = os.path.join(
             os.path.dirname(os.path.abspath(__file__)),
diff --git a/nativeauthenticator/tests/test_authenticator.py b/nativeauthenticator/tests/test_authenticator.py
index 19b2b65..f3b34e0 100644
--- a/nativeauthenticator/tests/test_authenticator.py
+++ b/nativeauthenticator/tests/test_authenticator.py
@@ -75,20 +75,33 @@ async def test_handlers(app):
     assert handlers[2][0] == '/authorize'
 
 
-async def test_exceed_atempts_of_login(tmpcwd, app):
+async def test_add_new_attempt_of_login(tmpcwd, app):
     auth = NativeAuthenticator(db=app.db)
-    auth.allowed_failed_logins = 3
-    auth.secs_before_next_try = 10
 
-    username = 'John Snow'
-    auth.get_or_create_user(username, 'password')
+    assert not auth.login_attempts
+    auth.add_login_attempt('username')
+    assert auth.login_attempts['username']['count'] == 1
+    auth.add_login_attempt('username')
+    assert auth.login_attempts['username']['count'] == 2
 
-    for i in range(3):
-        assert not auth.exceed_atempts_of_login(username)
 
-    assert auth.exceed_atempts_of_login(username)
-    time.sleep(12)
-    assert not auth.exceed_atempts_of_login(username)
+async def test_authentication_login_count(tmpcwd, app):
+    auth = NativeAuthenticator(db=app.db)
+    infos = {'username': 'johnsnow', 'password': 'password'}
+    wrong_infos = {'username': 'johnsnow', 'password': 'wrong_password'}
+    auth.get_or_create_user(infos['username'], infos['password'])
+    UserInfo.change_authorization(app.db, 'johnsnow')
+
+    assert not auth.login_attempts
+
+    await auth.authenticate(app, wrong_infos)
+    assert auth.login_attempts['johnsnow']['count'] == 1
+
+    await auth.authenticate(app, wrong_infos)
+    assert auth.login_attempts['johnsnow']['count'] == 2
+
+    await auth.authenticate(app, infos)
+    assert not auth.login_attempts.get('johnsnow')
 
 
 async def test_authentication_with_exceed_atempts_of_login(tmpcwd, app):