diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 98d576b..c861dee 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -12,7 +12,9 @@ jobs: strategy: max-parallel: 5 matrix: - python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10'] + # 3.7 is gone from the ubuntu-latest runner images (setup-python: + # "Version 3.7 with arch x64 not found"). + python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10'] fail-fast: false steps: diff --git a/experiments/apps.py b/experiments/apps.py index e7515bb..b45a0cc 100644 --- a/experiments/apps.py +++ b/experiments/apps.py @@ -4,6 +4,9 @@ class ExperimentsConfig(AppConfig): name = 'experiments' label = 'experiments' + # Django >= 6 defaults new projects to BigAutoField; pin the historical + # AutoField so existing installations are not asked for an id migration. + default_auto_field = 'django.db.models.AutoField' def ready(self): from django.contrib.auth.signals import user_logged_in, user_logged_out diff --git a/experiments/tests/test_views.py b/experiments/tests/test_views.py new file mode 100644 index 0000000..fa69526 --- /dev/null +++ b/experiments/tests/test_views.py @@ -0,0 +1,104 @@ +from __future__ import absolute_import + +from django.contrib.sessions.backends.db import SessionStore as DatabaseSession +from django.test import TestCase, override_settings +from django.urls import reverse + +from experiments import conf +from experiments.utils import WebUser + +from mock import patch + +OAUTH_STATE_KEY = 'oauth_state' +OAUTH_STATE = 'state-written-by-a-concurrent-request' + + +class ConfirmHumanViewTest(TestCase): + def setUp(self): + """Store a session with a seeded key and point the test client at it.""" + session = DatabaseSession() + session['seeded'] = 'before' + session.save() + self.session_key = session.session_key + self.client.cookies['sessionid'] = self.session_key + self.url = reverse('experiment_confirm_human') + + def stored(self): + """Re-read the session from the store, bypassing the request copy.""" + return DatabaseSession(session_key=self.session_key) + + def test_get_is_rejected(self): + """The endpoint stays POST-only.""" + response = self.client.get(self.url) + self.assertEqual(response.status_code, 405) + + def test_post_marks_the_session_as_human(self): + """The primary effect: the confirmed-human flag lands in the session.""" + response = self.client.post(self.url) + self.assertEqual(response.status_code, 204) + self.assertTrue(self.stored().get(conf.CONFIRM_HUMAN_SESSION_KEY)) + + def test_leaves_untouched_keys_alone(self): + """Keys confirm_human never touched keep their stored values.""" + self.client.post(self.url) + self.assertEqual(self.stored().get('seeded'), 'before') + + def test_does_not_drop_a_concurrent_session_write(self): + """A write landing while confirm_human replays counters must survive. + + confirm_human() can spend seconds replaying enrollments and goals to + the counter store. A concurrent request writing to the same session in + that window (an OAuth login storing its state, for example) used to be + overwritten when this request's stale session snapshot was saved at the + end of the request. The concurrent write is injected inside + confirm_human(), which is exactly where such requests land. + """ + original = WebUser.confirm_human + + def concurrent_write_then_confirm(user): + concurrent = DatabaseSession(session_key=self.session_key) + concurrent[OAUTH_STATE_KEY] = OAUTH_STATE + concurrent.save() + return original(user) + + with patch.object(WebUser, 'confirm_human', concurrent_write_then_confirm): + response = self.client.post(self.url) + + self.assertEqual(response.status_code, 204) + stored = self.stored() + self.assertEqual(stored.get(OAUTH_STATE_KEY), OAUTH_STATE) + self.assertTrue(stored.get(conf.CONFIRM_HUMAN_SESSION_KEY)) + + def test_repeat_ping_makes_no_session_write(self): + """A ping that changes nothing must not write the session at all.""" + self.client.post(self.url) + with patch.object(DatabaseSession, 'save') as save: + self.client.post(self.url) + save.assert_not_called() + + def test_without_a_session_cookie_it_still_answers(self): + """A cookieless request (bot, first hit) is answered, not crashed.""" + del self.client.cookies['sessionid'] + response = self.client.post(self.url) + self.assertEqual(response.status_code, 204) + + def test_concurrent_write_survives_save_every_request(self): + """SESSION_SAVE_EVERY_REQUEST makes the middleware save even an + unmodified session, so the merged state must also be what the request's + own session object holds by the time the middleware runs.""" + original = WebUser.confirm_human + + def concurrent_write_then_confirm(user): + concurrent = DatabaseSession(session_key=self.session_key) + concurrent[OAUTH_STATE_KEY] = OAUTH_STATE + concurrent.save() + return original(user) + + with override_settings(SESSION_SAVE_EVERY_REQUEST=True): + with patch.object(WebUser, 'confirm_human', concurrent_write_then_confirm): + response = self.client.post(self.url) + + self.assertEqual(response.status_code, 204) + stored = self.stored() + self.assertEqual(stored.get(OAUTH_STATE_KEY), OAUTH_STATE) + self.assertTrue(stored.get(conf.CONFIRM_HUMAN_SESSION_KEY)) diff --git a/experiments/utils.py b/experiments/utils.py index 52b9a2d..c3bf567 100644 --- a/experiments/utils.py +++ b/experiments/utils.py @@ -54,7 +54,7 @@ def clear_participant_cache(request): def _get_participant(request, session, user): if request and hasattr(request, 'user') and not user: user = request.user - if request and hasattr(request, 'session') and not session: + if request and hasattr(request, 'session') and session is None: session = request.session if request and conf.BOT_REGEX.search(request.META.get("HTTP_USER_AGENT", "")): @@ -64,7 +64,10 @@ def _get_participant(request, session, user): return WebUser(user=user, request=request) else: return DummyUser() - elif session: + elif session is not None: + # Truthiness is not identity here: since Django 6.1 an *empty* session + # is falsy, and a fresh visitor's empty session must still get a + # WebUser, not a DummyUser. return WebUser(session=session, request=request) else: return DummyUser() @@ -409,7 +412,10 @@ def _is_verified_human(self): @property def _session_key(self): - if not self.session: + # `is None`, not truthiness: since Django 6.1 an *empty* session is + # falsy, and returning None for every fresh visitor would key all of + # their enrollments and counters to the same identifier. + if self.session is None: return None if 'experiments_session_key' not in self.session: if not self.session.session_key: diff --git a/experiments/views.py b/experiments/views.py index c8bdada..5df1646 100644 --- a/experiments/views.py +++ b/experiments/views.py @@ -1,3 +1,4 @@ +from django.contrib.sessions.backends.signed_cookies import SessionStore as SignedCookiesStore from django.http import HttpResponse, HttpResponseBadRequest from django.views.decorators.cache import never_cache from django.shortcuts import get_object_or_404 @@ -19,15 +20,71 @@ "\x00\x49\x45\x4e\x44\xae\x42\x60\x82\x00") +_MISSING = object() + + @never_cache @require_POST def confirm_human(request): - if conf.CONFIRM_HUMAN: - experiment_user = participant(request) - experiment_user.confirm_human() + """Mark the session as belonging to a human, without clobbering the session. + + See _save_only_confirm_human_changes for why the session handling is not + left to the middleware. + """ + if not conf.CONFIRM_HUMAN: + return HttpResponse(status=204) + + session = getattr(request, 'session', None) + before = dict(session.items()) if session is not None else {} + + experiment_user = participant(request) + experiment_user.confirm_human() + + _save_only_confirm_human_changes(session, before) return HttpResponse(status=204) +def _save_only_confirm_human_changes(session, before): + """Persist this request's session changes without dropping concurrent ones. + + confirm_human() replays the participant's enrollments and goals - a counter + round trip each - between writing its session flag and the session being + saved at the end of the request. That can take seconds, and the middleware + then writes back the whole session dict as it looked when this request + loaded it: a concurrent request that wrote to the same session in the + meantime (an OAuth login storing its state, for example) is silently + overwritten by this request's stale snapshot. + + Instead, re-read the stored session, write only the keys confirm_human + changed, and keep the middleware from saving the stale snapshot. + """ + if session is None or not session.session_key: + # No stored session yet, so there is nothing to race with - let the + # middleware create and save the session as it normally would. + return + if isinstance(session, SignedCookiesStore): + # Cookie-backed sessions have no server-side store to race on; + # persistence is the response cookie the middleware writes. + return + + changed = {key: value for key, value in session.items() if before.get(key, _MISSING) != value} + + fresh = type(session)(session_key=session.session_key) + if changed: + for key, value in changed.items(): + fresh[key] = value + fresh.save() + + # The middleware must not write this request's stale snapshot over the + # merge above. With SESSION_SAVE_EVERY_REQUEST=True it saves even an + # unmodified session, so the request's session object is also pointed at + # the merged state - whatever the middleware does, it persists that. + # (_session_cache is the only way to replace the contents without + # marking the session dirty.) + session._session_cache = dict(fresh.items()) + session.modified = False + + @never_cache def record_experiment_goal(request, goal_name, cache_buster=None): participant(request).goal(goal_name) diff --git a/tox.ini b/tox.ini index 5a07e4b..d661ed2 100644 --- a/tox.ini +++ b/tox.ini @@ -17,6 +17,8 @@ envlist = {py,pypy}{38,39,310,311}-django{4.1} {py,pypy}{38,39,310,311,312}-django{4.2} {py,pypy}{310,311,312}-django{5.0} + {py,pypy}{310,311,312,313}-django{5.1,5.2} + py{312,313}-django{6.0,6.1} [gh-actions] python = @@ -27,6 +29,8 @@ python = 3.9: py39 3.10: py310 3.11: py311 + 3.12: py312 + 3.13: py313 pypy-3.8: pypy38 pypy-3.9: pypy39 pypy-3.10: pypy310 @@ -51,3 +55,7 @@ deps = django4.1: Django==4.1.* django4.2: Django==4.2.* django5.0: Django==5.0.* + django5.1: Django==5.1.* + django5.2: Django==5.2.* + django6.0: Django==6.0.* + django6.1: Django==6.1.*