From 21833ee938094d4f877ef75294dd9ce0e0b6b8cf Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Tue, 4 Oct 2022 11:33:07 +0200 Subject: [PATCH 8/8] misc: clean SessionIndex during logout (#69740) SessionIndex are deleted when the linked session does not exist anymore and 5 minutes after the creation of the logout request. --- .../0008_add_timestamp_to_session_index.py | 27 +++++++++ mellon/models.py | 35 ++++++++--- mellon/views.py | 10 +++- tests/test_models.py | 58 +++++++++++++++++++ tests/test_sso_slo.py | 8 ++- 5 files changed, 127 insertions(+), 11 deletions(-) create mode 100644 mellon/migrations/0008_add_timestamp_to_session_index.py create mode 100644 tests/test_models.py diff --git a/mellon/migrations/0008_add_timestamp_to_session_index.py b/mellon/migrations/0008_add_timestamp_to_session_index.py new file mode 100644 index 0000000..89b8c88 --- /dev/null +++ b/mellon/migrations/0008_add_timestamp_to_session_index.py @@ -0,0 +1,27 @@ +# Generated by Django 2.2.26 on 2022-10-04 09:10 + +import django.utils.timezone +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('mellon', '0007_sessionindex_transient_name_id'), + ] + + operations = [ + migrations.AddField( + model_name='sessionindex', + name='created', + field=models.DateTimeField( + auto_now_add=True, default=django.utils.timezone.now, verbose_name='created' + ), + preserve_default=False, + ), + migrations.AddField( + model_name='sessionindex', + name='logout_timestamp', + field=models.DateTimeField(null=True, verbose_name='Timestamp of the last logout'), + ), + ] diff --git a/mellon/models.py b/mellon/models.py index 13bf36d..6655a7f 100644 --- a/mellon/models.py +++ b/mellon/models.py @@ -14,10 +14,12 @@ # along with this program. If not, see . +import datetime from importlib import import_module from django.conf import settings from django.db import models +from django.utils.timezone import now from django.utils.translation import gettext_lazy as _ @@ -50,17 +52,36 @@ class SessionIndex(models.Model): saml_identifier = models.ForeignKey( verbose_name=_('SAML identifier'), to=UserSAMLIdentifier, on_delete=models.CASCADE ) + created = models.DateTimeField(verbose_name=_('created'), auto_now_add=True) + logout_timestamp = models.DateTimeField(verbose_name=_('Timestamp of the last logout'), null=True) - @staticmethod - def cleanup(cls): + @classmethod + def clean_session_indexes_after_logout(cls, delay_in_minutes=5, chunk_size=1000): session_engine = import_module(settings.SESSION_ENGINE) store = session_engine.SessionStore() - ids = [] - for si in cls.objects.all(): - if not store.exists(si.session_key): - ids.append(si.id) - cls.objects.filter(id__in=ids).delete() + try: + Session = store.model + except AttributeError: + Session = None + + candidates = cls.objects.filter( + models.Q(logout_timestamp__lt=now() - datetime.timedelta(minutes=delay_in_minutes)) + | models.Q(created__lt=now() - datetime.timedelta(days=1)) + )[:chunk_size] + candidates_session_keys = candidates.values_list('session_key', flat=True) + if Session is not None: + # fast path + existing_session_keys = Session.objects.filter( + session_key__in=candidates_session_keys + ).values_list('session_key', flat=True) + dead_session_keys = candidates_session_keys.difference(existing_session_keys) + else: + dead_session_keys = [] + for session_key in candidates_session_keys: + if not store.exists(session_key): + dead_session_keys.append(session_key) + cls.objects.filter(session_key__in=dead_session_keys).delete() class Meta: verbose_name = _('SAML SessionIndex') diff --git a/mellon/views.py b/mellon/views.py index 0437ba0..928f4f5 100644 --- a/mellon/views.py +++ b/mellon/views.py @@ -35,6 +35,7 @@ from django.shortcuts import render, resolve_url from django.urls import reverse from django.utils.encoding import force_str from django.utils.http import urlencode +from django.utils.timezone import now from django.utils.translation import gettext as _ from django.views.decorators.csrf import csrf_exempt from django.views.generic import View @@ -753,12 +754,13 @@ class LogoutView(ProfileMixin, LogMixin, View): try: session_indexes = models.SessionIndex.objects.filter( saml_identifier__user=request.user, saml_identifier__issuer__entity_id=issuer - ).order_by('-id')[:1] + ).order_by('-id') if not session_indexes: self.log.error('unable to find lasso session dump') else: - session_dump = utils.make_session_dump(session_indexes) + session_dump = utils.make_session_dump(session_indexes[:1]) logout.setSessionFromDump(session_dump) + session_indexes.update(logout_timestamp=now()) logout.initRequest(issuer, lasso.HTTP_METHOD_REDIRECT) logout.buildRequestMsg() except lasso.Error as e: @@ -801,6 +803,7 @@ class LogoutView(ProfileMixin, LogMixin, View): response = HttpResponseRedirect(next_url) if cookie_name in request.COOKIES: response.delete_cookie(cookie_name) + models.SessionIndex.clean_session_indexes_after_logout() return response TOKEN_SALT = 'mellon-logout-token' @@ -845,7 +848,7 @@ class LogoutView(ProfileMixin, LogMixin, View): return None session_indexes = models.SessionIndex.objects.filter( saml_identifier__user=request.user, saml_identifier__issuer__entity_id=issuer - ).order_by('-id')[:1] + ).order_by('-id') if not session_indexes: return None @@ -854,6 +857,7 @@ class LogoutView(ProfileMixin, LogMixin, View): 'session_index_pk': session_indexes[0].pk, } token = signing.dumps(token_content, salt=cls.TOKEN_SALT) + session_indexes.update(logout_timestamp=now()) return reverse('mellon_logout') + '?' + urlencode({'token': token}) diff --git a/tests/test_models.py b/tests/test_models.py new file mode 100644 index 0000000..ae51d67 --- /dev/null +++ b/tests/test_models.py @@ -0,0 +1,58 @@ +# django-mellon - SAML2 authentication for Django +# Copyright (C) 2014-2022 Entr'ouvert +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. + +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import datetime +from importlib import import_module + +import pytest +from django.utils.timezone import now + +from mellon import models + + +@pytest.mark.parametrize( + 'session_engine_path', + [ + 'mellon.sessions_backends.db', + 'django.contrib.sessions.backends.cache', + ], +) +def test_session_index_cleaning(session_engine_path, db, settings, django_user_model, freezer): + settings.SESSION_ENGINE = session_engine_path + session_engine = import_module(settings.SESSION_ENGINE) + store = session_engine.SessionStore(None) + user = django_user_model.objects.create(username='user') + issuer = models.Issuer.objects.create(entity_id='https://idp.example.com/', slug='idp') + usi = models.UserSAMLIdentifier.objects.create(user=user, issuer=issuer, name_id='1234') + store['x'] = 1 + store.set_expiry(86400 * 31) # expire session after one month + store.save() + models.SessionIndex.objects.create( + session_index='abcd', session_key=store.session_key, saml_identifier=usi + ) + assert models.SessionIndex.objects.count() == 1 + + # check SessionIndex is only cleaned if the session is dead, + # logout_timestamp being only used as a hint + freezer.move_to(datetime.timedelta(days=10)) + models.SessionIndex.clean_session_indexes_after_logout() + assert models.SessionIndex.objects.count() == 1 + models.SessionIndex.objects.update(logout_timestamp=now()) + models.SessionIndex.clean_session_indexes_after_logout() + assert models.SessionIndex.objects.count() == 1 + + store.flush() # delete the session + models.SessionIndex.clean_session_indexes_after_logout() + assert models.SessionIndex.objects.count() == 0 diff --git a/tests/test_sso_slo.py b/tests/test_sso_slo.py index c40b800..3fedc93 100644 --- a/tests/test_sso_slo.py +++ b/tests/test_sso_slo.py @@ -866,7 +866,7 @@ def test_sso_slo_transient_name_identifier(db, app, idp, caplog, sp_settings): assert response.location == '/' -def test_sso_slo_token(db, app, rf, idp, caplog, django_user_model): +def test_sso_slo_token(db, app, rf, idp, caplog, django_user_model, freezer): from mellon.views import LogoutView caplog.set_level(logging.WARNING) @@ -874,10 +874,14 @@ def test_sso_slo_token(db, app, rf, idp, caplog, django_user_model): url, body, relay_state = idp.process_authn_request_redirect(response['Location']) response = app.post('/login/', params={'SAMLResponse': body, 'RelayState': relay_state}) + assert models.SessionIndex.objects.count() == 1 + assert models.SessionIndex.objects.filter(logout_timestamp__isnull=True).count() == 1 request = rf.get('/whatever/') request.session = app.session request.user = django_user_model.objects.get() token_logout_url = LogoutView.make_logout_token_url(request, next_url='/somepath/') + assert models.SessionIndex.objects.count() == 1 + assert models.SessionIndex.objects.filter(logout_timestamp__isnull=False).count() == 1 assert token_logout_url app.session.flush() assert '_auth_user_id' not in app.session @@ -885,6 +889,8 @@ def test_sso_slo_token(db, app, rf, idp, caplog, django_user_model): assert urlparse.urlparse(response['Location']).path == '/singleLogout' url = idp.process_logout_request_redirect(response.location) caplog.clear() + freezer.move_to(datetime.timedelta(minutes=6)) response = app.get(url) assert len(caplog.records) == 0, 'logout failed' assert response.location == '/somepath/' + assert models.SessionIndex.objects.count() == 0 -- 2.37.2