Repository: jazzband/django-axes
Pull request #1048 (2023-04-28 15:14:50)

Use cache.incr for setting the cached authentication failure numbers
Relates to #784

Notes on implementation:

- All counters are not in the same value; if we track login attempts by multiple attributes such as username and/or IP address one counter can be higher, one can be lower, if someone is failing to authenticate from multiple IPs with different usernames or with multiple usernames from the same IP. Simplifying solution to only support exclusive username XOR IP address based tracking (instead of include OR) would enable just tracking the user via single attribute or a composite attribute.
- The current cache key(s) calculation approach is not exactly pretty, but it is compatible with the database implementation?

hirotasoshu: 
> * All counters are not in the same value; if we track users by multiple attributes such as username and/or IP address one tracker can be higher, one can be lower, if someone is attempting to authenticate from multiple IPs with different usernames or with multiple usernames from the same IP. Simplifying solution to only support exclusive username XOR IP address based tracking (instead of include OR) would enable just tracking the user via single attribute or a composite attribute.

As I can see in current implementation, if we use username AND ip address, only one cache key is created so we have only one tracker already.
For situation, when we use username OR ip address, I think current implementation is correct, because with that implementation we can lockout users that tries to login with different ips and same username AND users that tries to login with different usernames and same ip AND users that tries to login with same username and same ip. If we use only one attribute or composite attribute we can't cover all that cases. The only problem is that this approach can lead to the creation of a large number of keys, which in turn can lead to high RAM consumption in the case of using IN-MEMORY databases. But it seems to me that in this case you just need to configure it correctly.



Patch:
diff --git a/axes/handlers/cache.py b/axes/handlers/cache.py
index b85f3f6..f5de9b6 100644
--- a/axes/handlers/cache.py
+++ b/axes/handlers/cache.py
@@ -6,7 +6,7 @@ from axes.handlers.base import AxesBaseHandler, AbstractAxesHandler
 from axes.helpers import (
     get_cache,
     get_cache_timeout,
-    get_client_cache_key,
+    get_client_cache_keys,
     get_client_str,
     get_client_username,
     get_credentials,
@@ -44,7 +44,7 @@ class AxesCacheHandler(AbstractAxesHandler, AxesBaseHandler):
             )
 
         cache_keys.extend(
-            get_client_cache_key(
+            get_client_cache_keys(
                 AccessAttempt(username=username, ip_address=ip_address)
             )
         )
@@ -58,7 +58,7 @@ class AxesCacheHandler(AbstractAxesHandler, AxesBaseHandler):
         return count
 
     def get_failures(self, request, credentials: Optional[dict] = None) -> int:
-        cache_keys = get_client_cache_key(request, credentials)
+        cache_keys = get_client_cache_keys(request, credentials)
         failure_count = max(
             self.cache.get(cache_key, default=0) for cache_key in cache_keys
         )
@@ -110,7 +110,18 @@ class AxesCacheHandler(AbstractAxesHandler, AxesBaseHandler):
             log.info("AXES: Login failed from whitelisted client %s.", client_str)
             return
 
-        failures_since_start = 1 + self.get_failures(request, credentials)
+        cache_keys = get_client_cache_keys(request, credentials)
+        cache_timeout = get_cache_timeout()
+        failures = []
+        for cache_key in cache_keys:
+            added = self.cache.add(key=cache_key, value=1, timeout=cache_timeout)
+            if added:
+                failures.append(1)
+            else:
+                failures.append(self.cache.incr(key=cache_key, delta=1))
+                self.cache.touch(key=cache_key, timeout=cache_timeout)
+
+        failures_since_start = max(failures)
         request.axes_failures_since_start = failures_since_start
 
         if failures_since_start > 1:
@@ -126,11 +137,6 @@ class AxesCacheHandler(AbstractAxesHandler, AxesBaseHandler):
                 client_str,
             )
 
-        cache_keys = get_client_cache_key(request, credentials)
-        for cache_key in cache_keys:
-            failures = self.cache.get(cache_key, default=0)
-            self.cache.set(cache_key, failures + 1, get_cache_timeout())
-
         if (
             settings.AXES_LOCK_OUT_AT_FAILURE
             and failures_since_start >= get_failure_limit(request, credentials)
@@ -166,7 +172,7 @@ class AxesCacheHandler(AbstractAxesHandler, AxesBaseHandler):
         log.info("AXES: Successful login by %s.", client_str)
 
         if settings.AXES_RESET_ON_SUCCESS:
-            cache_keys = get_client_cache_key(request, credentials)
+            cache_keys = get_client_cache_keys(request, credentials)
             for cache_key in cache_keys:
                 failures_since_start = self.cache.get(cache_key, default=0)
                 self.cache.delete(cache_key)
diff --git a/axes/helpers.py b/axes/helpers.py
index 331d1a6..d218337 100644
--- a/axes/helpers.py
+++ b/axes/helpers.py
@@ -2,7 +2,7 @@ from datetime import timedelta
 from hashlib import sha256
 from logging import getLogger
 from string import Template
-from typing import Callable, Optional, Type, Union
+from typing import Callable, Optional, Type, Union, List
 from urllib.parse import urlencode
 
 from django.core.cache import BaseCache, caches
@@ -248,7 +248,7 @@ def get_client_parameters(username: str, ip_address: str, user_agent: str) -> li
     return filter_query
 
 
-def make_cache_key_list(filter_kwargs_list):
+def make_cache_key_list(filter_kwargs_list: List[dict]) -> List[str]:
     cache_keys = []
     for filter_kwargs in filter_kwargs_list:
         cache_key_components = "".join(
@@ -259,10 +259,10 @@ def make_cache_key_list(filter_kwargs_list):
     return cache_keys
 
 
-def get_client_cache_key(
+def get_client_cache_keys(
     request_or_attempt: Union[HttpRequest, AccessBase],
     credentials: Optional[dict] = None,
-) -> str:
+) -> List[str]:
     """
     Build cache key name from request or AccessAttempt object.
 


Test patch:
diff --git a/tests/test_handlers.py b/tests/test_handlers.py
index 5ae36b0..d32af60 100644
--- a/tests/test_handlers.py
+++ b/tests/test_handlers.py
@@ -496,12 +496,12 @@ class AxesCacheHandlerTestCase(AxesHandlerBaseTestCase):
             "AXES: Username is None and AXES_ONLY_USER_FAILURES is enabled, new record will NOT be created."
         )
 
-    @patch.object(cache, "set")
-    def test_user_login_failed_with_none_username(self, cache_set):
+    @patch.object(cache, "add")
+    def test_user_login_failed_with_none_username(self, cache_add):
         credentials = {"username": None, "password": "test"}
         sender = MagicMock()
         AxesProxyHandler.user_login_failed(sender, credentials, self.request)
-        self.assertTrue(cache_set.called)
+        self.assertTrue(cache_add.called)
 
 
 @override_settings(AXES_HANDLER="axes.handlers.dummy.AxesDummyHandler")
diff --git a/tests/test_helpers.py b/tests/test_helpers.py
index 2677a17..a0a5c64 100644
--- a/tests/test_helpers.py
+++ b/tests/test_helpers.py
@@ -10,7 +10,7 @@ from axes.apps import AppConfig
 from axes.helpers import (
     cleanse_parameters,
     get_cache_timeout,
-    get_client_cache_key,
+    get_client_cache_keys,
     get_client_ip_address,
     get_client_parameters,
     get_client_str,
@@ -372,7 +372,7 @@ class ClientParametersTestCase(AxesTestCase):
 
 
 class ClientCacheKeyTestCase(AxesTestCase):
-    def test_get_cache_key(self):
+    def test_get_cache_keys(self):
         """
         Test the cache key format.
         """
@@ -386,7 +386,7 @@ class ClientCacheKeyTestCase(AxesTestCase):
             "/admin/login/", data={"username": self.username, "password": "test"}
         )
 
-        self.assertEqual([cache_hash_key], get_client_cache_key(request))
+        self.assertEqual([cache_hash_key], get_client_cache_keys(request))
 
         # Getting cache key from AccessAttempt Object
         attempt = AccessAttempt(
@@ -400,7 +400,7 @@ class ClientCacheKeyTestCase(AxesTestCase):
             failures_since_start=0,
         )
 
-        self.assertEqual([cache_hash_key], get_client_cache_key(attempt))
+        self.assertEqual([cache_hash_key], get_client_cache_keys(attempt))
 
     def test_get_cache_key_empty_ip_address(self):
         """
@@ -420,7 +420,7 @@ class ClientCacheKeyTestCase(AxesTestCase):
             REMOTE_ADDR=empty_ip_address,
         )
 
-        self.assertEqual([cache_hash_key], get_client_cache_key(request))
+        self.assertEqual([cache_hash_key], get_client_cache_keys(request))
 
         # Getting cache key from AccessAttempt Object
         attempt = AccessAttempt(
@@ -434,7 +434,7 @@ class ClientCacheKeyTestCase(AxesTestCase):
             failures_since_start=0,
         )
 
-        self.assertEqual([cache_hash_key], get_client_cache_key(attempt))
+        self.assertEqual([cache_hash_key], get_client_cache_keys(attempt))
 
     def test_get_cache_key_credentials(self):
         """
@@ -454,7 +454,7 @@ class ClientCacheKeyTestCase(AxesTestCase):
         # Difference between the upper test: new call signature with credentials
         credentials = {"username": self.username}
 
-        self.assertEqual([cache_hash_key], get_client_cache_key(request, credentials))
+        self.assertEqual([cache_hash_key], get_client_cache_keys(request, credentials))
 
         # Getting cache key from AccessAttempt Object
         attempt = AccessAttempt(
@@ -467,7 +467,7 @@ class ClientCacheKeyTestCase(AxesTestCase):
             path_info=request.META.get("PATH_INFO", "<unknown>"),
             failures_since_start=0,
         )
-        self.assertEqual([cache_hash_key], get_client_cache_key(attempt))
+        self.assertEqual([cache_hash_key], get_client_cache_keys(attempt))
 
 
 class UsernameTestCase(AxesTestCase):