Repository: jazzband/django-axes (python)

Issue:
BUG: Concurrent session logout are miss-tracked
**Describe the bug**
When there are concurrent sessions held by the same user on differente devices the first which logs-out also marks the time on the latter.

**To Reproduce**
Steps to reproduce the behavior:
1. Configure AXES using `AxesDatabaseHandler` (the default one)
1. Create a user
2. Log in with that user using a browser
3. Log in with that user using a different browser (or incognito mode)
4. <repeat step 3 with other browser/client if needed>
5. Log out from one of the active browsers

Now every `AccessLog` for that user has the same `logout_time`, even those for which there is still an active session. It is not possible to update access-logs for those records.
https://github.com/jazzband/django-axes/blob/fd9d185ad32570434d14de79e45976c6e18fcc54/axes/handlers/database.py#L319-L321

**Expected behavior**
Every session for a single user should be related to a single `AccessLog`, to enable a correct tracking of the user.


**Your environment**
python version: 3.10
django version: 4.2
django-axes version: 
Operating system: Linux

**Possible implementation**
It would be advisable to have an other optional field on `AccessLog` which can be a digest of the current session-id.
This could also be used as a method do detect whenever an access "expires" without log-off.
The reason for not using a FK to session is:
- it is not secure since the PK of session usually is the session-id which must be kept secret and hard to find (if possible)
- already `username` is not a real FK to user but just simple column


Pull request:
Add session hash to access log
# What does this PR do?
This PR adds a new field on `AccessLog` to better track sessions.

There is still an issue for sessions in which a user changes its password: because `django.contrib.auth` creates a new session upon password change and no logout/login signal is fired since this is neither a login nor a logout. I suppose it would be fine to keep this case "untracked" since it is not possible to hook into that method and usually changing password is not a frequent task.

When no session engine is configured it falls back to current behaviour: a logout will set the `logout_time` to every `AccessLog` without it. However this could be tolerated since there is no standard way to track such case and usually when there is no session there is not the concept of login/logout.

Fixes #1190 


## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you make sure to update the documentation with your changes?
- [x] Did you write any new necessary tests?


Patch:
diff --git a/axes/handlers/database.py b/axes/handlers/database.py
index 95678b2..be2ded4 100644
--- a/axes/handlers/database.py
+++ b/axes/handlers/database.py
@@ -14,6 +14,7 @@ from axes.attempts import (
 from axes.conf import settings
 from axes.handlers.base import AxesBaseHandler, AbstractAxesHandler
 from axes.helpers import (
+    get_client_session_hash,
     get_client_str,
     get_client_username,
     get_credentials,
@@ -284,6 +285,9 @@ class AxesDatabaseHandler(AbstractAxesHandler, AxesBaseHandler):
                 http_accept=request.axes_http_accept,
                 path_info=request.axes_path_info,
                 attempt_time=request.axes_attempt_time,
+                # evaluate session hash here to ensure having the correct
+                # value which is stored on the backend
+                session_hash=get_client_session_hash(request),
             )
 
         if settings.AXES_RESET_ON_SUCCESS:
@@ -317,7 +321,10 @@ class AxesDatabaseHandler(AbstractAxesHandler, AxesBaseHandler):
         if username and not settings.AXES_DISABLE_ACCESS_LOG:
             # 2. database query: Update existing attempt logs with logout time
             AccessLog.objects.filter(
-                username=username, logout_time__isnull=True
+                username=username,
+                logout_time__isnull=True,
+                # update only access log for given session
+                session_hash=get_client_session_hash(request),
             ).update(logout_time=request.axes_attempt_time)
 
     def post_save_access_attempt(self, instance, **kwargs):
diff --git a/axes/helpers.py b/axes/helpers.py
index 8570279..78cc668 100644
--- a/axes/helpers.py
+++ b/axes/helpers.py
@@ -8,6 +8,7 @@ from urllib.parse import urlencode
 from django.core.cache import BaseCache, caches
 from django.http import HttpRequest, HttpResponse, JsonResponse, QueryDict
 from django.shortcuts import redirect, render
+from django.utils.encoding import force_bytes
 from django.utils.module_loading import import_string
 
 from axes.conf import settings
@@ -614,3 +615,24 @@ def toggleable(func) -> Callable:
             return func(*args, **kwargs)
 
     return inner
+
+
+def get_client_session_hash(request: HttpRequest) -> str:
+    """
+    Get client session and returns the SHA256 hash of session key, forcing session creation if required.
+
+    If no session is available on request returns an empty string.
+    """
+    try:
+        session = request.session
+    except AttributeError:
+        # when no session is available just return an empty string
+        return ""
+
+    # ensure that a session key exists at this point
+    # because session middleware usually creates the session key at the end
+    # of request cycle
+    if session.session_key is None:
+        session.create()
+
+    return sha256(force_bytes(session.session_key)).hexdigest()
diff --git a/axes/migrations/0009_add_session_hash.py b/axes/migrations/0009_add_session_hash.py
new file mode 100644
index 0000000..bd10ebe
--- /dev/null
+++ b/axes/migrations/0009_add_session_hash.py
@@ -0,0 +1,22 @@
+# Generated by Django 4.2.2 on 2024-04-30 07:57
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("axes", "0008_accessfailurelog"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="accesslog",
+            name="session_hash",
+            field=models.CharField(
+                blank=True,
+                default="",
+                max_length=64,
+                verbose_name="Session key hash (sha256)",
+            ),
+        ),
+    ]
diff --git a/axes/models.py b/axes/models.py
index 1b42141..9c8a7da 100644
--- a/axes/models.py
+++ b/axes/models.py
@@ -53,6 +53,7 @@ class AccessAttempt(AccessBase):
 
 class AccessLog(AccessBase):
     logout_time = models.DateTimeField(_("Logout Time"), null=True, blank=True)
+    session_hash = models.CharField(_("Session key hash (sha256)"), default="", blank=True, max_length=64)
 
     def __str__(self):
         return f"Access Log for {self.username} @ {self.attempt_time}"
diff --git a/docs/4_configuration.rst b/docs/4_configuration.rst
index f64320e..1a60594 100644
--- a/docs/4_configuration.rst
+++ b/docs/4_configuration.rst
@@ -137,6 +137,12 @@ with the ``AXES_HANDLER`` setting in project configuration:
   logs attempts to database and creates AccessAttempt and AccessLog records
   that persist until removed from the database manually or automatically
   after their cool offs expire (checked on each login event).
+
+.. note::
+  To keep track of concurrent sessions AccessLog stores an hash of ``session_key`` if the session engine is configured.
+  When no session engine is configured each access is stored with the same dummy value, then a logout will cause each *not-logged-out yet* logs to set a logout time.
+  Due to how ``django.contrib.auth`` works it is not possible to correctly track the logout of a session in which the user changed its password, since it will create a new session without firing any logout event.
+
 - ``axes.handlers.cache.AxesCacheHandler``
   only uses the cache for monitoring attempts and does not persist data
   other than in the cache backend; this data can be purged automatically


Test patch:
diff --git a/tests/test_logging.py b/tests/test_logging.py
index d7d1331..9394031 100644
--- a/tests/test_logging.py
+++ b/tests/test_logging.py
@@ -1,7 +1,6 @@
 from unittest.mock import patch
 
 from django.test import override_settings
-from django.urls import reverse
 
 from axes import __version__
 from axes.apps import AppConfig
@@ -59,16 +58,21 @@ class AppsTestCase(AxesTestCase):
 class AccessLogTestCase(AxesTestCase):
     def test_access_log_on_logout(self):
         """
-        Test a valid logout and make sure the logout_time is updated.
+        Test a valid logout and make sure the logout_time is updated only for that.
         """
 
         self.login(is_valid_username=True, is_valid_password=True)
-        self.assertIsNone(AccessLog.objects.latest("id").logout_time)
+        latest_log = AccessLog.objects.latest("id")
+        self.assertIsNone(latest_log.logout_time)
+        other_log = self.create_log(session_hash='not-the-session')
+        self.assertIsNone(other_log.logout_time)
 
-        response = self.client.post(reverse("admin:logout"))
+        response = self.logout()
         self.assertContains(response, "Logged out")
-
-        self.assertIsNotNone(AccessLog.objects.latest("id").logout_time)
+        other_log.refresh_from_db()
+        self.assertIsNone(other_log.logout_time)
+        latest_log.refresh_from_db()
+        self.assertIsNotNone(latest_log.logout_time)
 
     @override_settings(DATA_UPLOAD_MAX_NUMBER_FIELDS=1500)
     def test_log_data_truncated(self):
@@ -86,7 +90,7 @@ class AccessLogTestCase(AxesTestCase):
         AccessLog.objects.all().delete()
 
         response = self.login(is_valid_username=True, is_valid_password=True)
-        response = self.client.post(reverse("admin:logout"))
+        response = self.logout()
 
         self.assertEqual(AccessLog.objects.all().count(), 0)
         self.assertContains(response, "Logged out", html=True)
@@ -109,7 +113,7 @@ class AccessLogTestCase(AxesTestCase):
         AccessLog.objects.all().delete()
 
         response = self.login(is_valid_username=True, is_valid_password=True)
-        response = self.client.post(reverse("admin:logout"))
+        response = self.logout()
 
         self.assertEqual(AccessLog.objects.count(), 0)
         self.assertContains(response, "Logged out", html=True)