Use redis cache in get_approx_account_lockouts_from_login_attempts (#250)

* Use redis cache in `get_approx_account_lockouts_from_login_attempts`

* use django_redis in ci

* Add `django_redis` and `redis` to requirements.txt

* Fix an issue detected by tests: clear redis cache upon block reset

* Remove the unnecessary `if`

defender/ci_settings.py

@@ -7,7 +7,7 @@ from celery import Celery
 DATABASES = {"default": {"ENGINE": "django.db.backends.sqlite3", "NAME": ":memory:",}}
 
 CACHES = {
-    "default": {"BACKEND": "redis_cache.RedisCache", "LOCATION": "localhost:6379",}
+    "default": {"BACKEND": "django_redis.cache.RedisCache", "LOCATION": "redis://localhost:6379",}
 }
 
 SITE_ID = 1

defender/data.py

@@ -1,6 +1,7 @@
 from datetime import timedelta
 
 from defender import config
+from defender.connection import get_redis_connection
 from .models import AccessAttempt
 from django.db.models import Q
 from django.utils import timezone
@@ -19,6 +20,14 @@ def store_login_attempt(
         login_valid=login_valid,
     )
 
+
+def get_approx_lockouts_cache_key(ip_address, username):
+    """get cache key for approximate number of account lockouts"""
+    return "{0}:approx_lockouts:ip:{1}:user:{2}".format(
+        config.CACHE_PREFIX, ip_address or "", username.lower() if username else ""
+    )
+
+
 def get_approx_account_lockouts_from_login_attempts(ip_address=None, username=None):
     """Get the approximate number of account lockouts in a period of ACCESS_ATTEMPT_EXPIRATION hours.
     This is approximate because we do not consider the time between these failed
@@ -31,10 +40,6 @@ def get_approx_account_lockouts_from_login_attempts(ip_address=None, username=No
     Returns:
         int: The minimum of the count of logged failure attempts and the length of the LOCKOUT_COOLOFF_TIMES - 1, or 0 dependant on either configuration or argument parameters (ie. both ip_address and username being None).
     """
-
-    # TODO: Possibly add logic to temporarily store this info in the cache
-    # to help mitigate any potential performance impact this could have.
-
     if not config.STORE_ACCESS_ATTEMPTS or not (ip_address or username):
         # If we're not storing login attempts OR both ip_address and username are
         # None we should return 0.
@@ -57,4 +62,15 @@ def get_approx_account_lockouts_from_login_attempts(ip_address=None, username=No
         # conditions, we're in an inappropriate context.
         raise Exception("Invalid state requested")
 
-    return AccessAttempt.objects.filter(q).count() // failure_limit
+    cache_key = get_approx_lockouts_cache_key(ip_address, username)
+
+    redis_client = get_redis_connection()
+    cached_value = redis_client.get(cache_key)
+    if cached_value is not None:
+        return int(cached_value)
+
+    lockouts = AccessAttempt.objects.filter(q).count() // failure_limit
+
+    redis_client.set(cache_key, int(lockouts), 60)
+
+    return lockouts

defender/tests.py

@@ -13,7 +13,7 @@ from django.urls import reverse
 
 import redis
 
-from defender.data import get_approx_account_lockouts_from_login_attempts
+from defender.data import get_approx_account_lockouts_from_login_attempts, get_approx_lockouts_cache_key
 
 from . import utils
 from . import config
@@ -964,6 +964,19 @@ class AccessAttemptTest(DefenderTestCase):
     def test_approx_account_lockout_count_default_case_invalid_args_pt2(self):
         with self.assertRaises(Exception):
             get_approx_account_lockouts_from_login_attempts(username=VALID_USERNAME)
+    
+    def test_approx_account_lockout_uses_redis_cache(self):
+        get_approx_account_lockouts_from_login_attempts(
+            ip_address="127.0.0.1", username=VALID_USERNAME
+        )
+
+        redis_client = get_redis_connection()
+        cached_value = redis_client.get(
+            get_approx_lockouts_cache_key(
+                ip_address="127.0.0.1", username=VALID_USERNAME
+            )
+        )
+        self.assertIsNotNone(cached_value)
 
 
 class SignalTest(DefenderTestCase):

defender/utils.py

@@ -12,7 +12,11 @@ from django.utils.module_loading import import_string
 
 from .connection import get_redis_connection
 from . import config
-from .data import get_approx_account_lockouts_from_login_attempts, store_login_attempt
+from .data import (
+    get_approx_account_lockouts_from_login_attempts,
+    get_approx_lockouts_cache_key,
+    store_login_attempt,
+)
 from .signals import (
     send_username_block_signal,
     send_ip_block_signal,
@@ -331,6 +335,10 @@ def unblock_ip(ip_address, pipe=None):
             pipe.execute()
     send_ip_unblock_signal(ip_address)
 
+    redis_cache_key = get_approx_lockouts_cache_key(ip_address, None)
+    redis_client = get_redis_connection()
+    redis_client.delete(redis_cache_key)
+
 
 def unblock_username(username, pipe=None):
     """ unblock the given Username """
@@ -345,6 +353,10 @@ def unblock_username(username, pipe=None):
             pipe.execute()
     send_username_unblock_signal(username)
 
+    redis_cache_key = get_approx_lockouts_cache_key(None, username)
+    redis_client = get_redis_connection()
+    redis_client.delete(redis_cache_key)
+
 
 def reset_failed_attempts(ip_address=None, username=None):
     """ reset the failed attempts for these ip's and usernames
@@ -357,6 +369,10 @@ def reset_failed_attempts(ip_address=None, username=None):
         unblock_ip(ip_address, pipe=pipe)
     unblock_username(username, pipe=pipe)
 
+    redis_cache_key = get_approx_lockouts_cache_key(ip_address, username)
+    redis_client = get_redis_connection()
+    redis_client.delete(redis_cache_key)
+
     pipe.execute()
 
 

requirements.txt

@@ -1,7 +1,8 @@
 -e .
 coverage
 mockredispy
-django-redis-cache
+django-redis>=5,<6
+redis>=5,<6
 importlib-metadata<5.0
 celery
 sphinx_rtd_theme==2.0.0

tox.ini

@@ -2,7 +2,8 @@
 envlist =
     # list of supported Django/Python versions:
     py{37,38,39,py3}-dj{32}
-    py{38,39,310,311}-dj{40,41,42,main}
+    py{38,39,310,311}-dj{40,41,42}
+    py{311}-dj{main}
     py38-{lint,docs}
 
 [gh-actions]