From 911de8f347b269bd22a50d2cc111ec62fbbf4414 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aleksi=20Ha=CC=88kli?= <aleksi.hakli@iki.fi>
Date: Wed, 13 Feb 2019 00:55:53 +0200
Subject: [PATCH] Refactor is_user_lockable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Aleksi Häkli <aleksi.hakli@iki.fi>
---
 axes/attempts.py                  | 38 ++++++++++++++-----------------
 axes/tests/test_access_attempt.py | 25 ++++++++++++++++++--
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/axes/attempts.py b/axes/attempts.py
index e116165..7e1278d 100644
--- a/axes/attempts.py
+++ b/axes/attempts.py
@@ -1,5 +1,6 @@
 from datetime import timedelta
 from hashlib import md5
+from logging import getLogger
 
 from django.contrib.auth import get_user_model
 from django.utils import timezone
@@ -8,6 +9,8 @@ from axes.conf import settings
 from axes.models import AccessAttempt
 from axes.utils import get_axes_cache, get_client_ip, get_client_username
 
+log = getLogger(settings.AXES_LOGGER)
+
 
 def _query_user_attempts(request, credentials=None):
     """
@@ -151,32 +154,25 @@ def ip_in_blacklist(ip):
 
 
 def is_user_lockable(request, credentials=None):
-    """
-    Check if the user has a profile with nolockout attribute set.
-
-    If so, then return the value to see if this user is special and does not get their account locked out.
-    """
-
     if request.method != 'POST':
         return True
 
+    username_field = getattr(get_user_model(), 'USERNAME_FIELD', 'username')
+    username_value = get_client_username(request, credentials)
+    kwargs = {
+        username_field: username_value
+    }
+
+    UserModel = get_user_model()
+
+    # Special users with nolockout attribute set should never be locked out
     try:
-        field = getattr(get_user_model(), 'USERNAME_FIELD', 'username')
-        kwargs = {
-            field: get_client_username(request, credentials)
-        }
-        user = get_user_model().objects.get(**kwargs)
+        user = UserModel.objects.get(**kwargs)
+        return not user.nolockout
+    except (UserModel.DoesNotExist, AttributeError):
+        pass
 
-        if hasattr(user, 'nolockout'):
-            # need to invert since we need to return
-            # false for users that can't be blocked
-            return not user.nolockout
-
-    except get_user_model().DoesNotExist:
-        # not a valid user
-        return True
-
-    # Default behavior for a user to be lockable
+    # Default case is that users can be locked out
     return True
 
 
diff --git a/axes/tests/test_access_attempt.py b/axes/tests/test_access_attempt.py
index 5eadb2e..d50a654 100644
--- a/axes/tests/test_access_attempt.py
+++ b/axes/tests/test_access_attempt.py
@@ -5,14 +5,14 @@ import string
 import time
 from unittest.mock import patch
 
-from django.contrib.auth import authenticate
+from django.contrib.auth import authenticate, get_user_model
 from django.contrib.auth.models import User
 from django.http import HttpRequest
 from django.test import TestCase, override_settings
 from django.test.client import RequestFactory
 from django.urls import reverse
 
-from axes.attempts import get_cache_key
+from axes.attempts import get_cache_key, is_user_lockable
 from axes.conf import settings
 from axes.models import AccessAttempt, AccessLog
 from axes.signals import user_locked_out
@@ -496,3 +496,24 @@ class AccessAttemptTest(TestCase):
         # But we should find one now
         response = self._login()
         self.assertContains(response, self.LOCKED_MESSAGE, status_code=403)
+
+    @patch('axes.attempts.log')
+    def test_is_user_lockable(self, log):
+        request = HttpRequest()
+        request.method = 'POST'
+
+        UserModel = get_user_model()
+        UserModel.objects.create(username='jane.doe')
+
+        with self.subTest('User is marked as nolockout.'):
+            with patch.object(UserModel, 'nolockout', True, create=True):
+                lockable = is_user_lockable(request, {'username': 'jane.doe'})
+                self.assertFalse(lockable)
+
+        with self.subTest('User exists but attemptee can be locked out.'):
+            lockable = is_user_lockable(request, {'username': 'jane.doe'})
+            self.assertTrue(lockable)
+
+        with self.subTest('User does not exist and attemptee can be locked out.'):
+            lockable = is_user_lockable(request, {'username': 'john.doe'})
+            self.assertTrue(lockable)