From 9b0f8819d40484ad89e98f42a6cdf04db5e87d45 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 27 Nov 2024 15:08:24 +0100 Subject: [PATCH 1/3] :test_tube: [#4862] Add regression test for hashed auth attributes Submissions that require cosigning may not have their auth attributes hashed when the cosigner logs out. It should only remove the submission from the session. --- .../authentication/api/tests/test_logout.py | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/openforms/authentication/api/tests/test_logout.py b/src/openforms/authentication/api/tests/test_logout.py index d01735ac4f..833688f796 100644 --- a/src/openforms/authentication/api/tests/test_logout.py +++ b/src/openforms/authentication/api/tests/test_logout.py @@ -2,12 +2,14 @@ from unittest.mock import Mock, patch from django.contrib.auth import SESSION_KEY +from django.test import tag from django.urls import reverse from rest_framework import status from rest_framework.test import APITestCase from openforms.accounts.tests.factories import StaffUserFactory +from openforms.authentication.contrib.digid.constants import DIGID_DEFAULT_LOA from openforms.submissions.tests.factories import SubmissionFactory from openforms.submissions.tests.mixins import SubmissionsMixin @@ -146,3 +148,49 @@ def test_logout_non_existing_plugin(self): self.assertNotIn(FORM_AUTH_SESSION_KEY, session) self.assertNotIn(REGISTRATOR_SUBJECT_SESSION_KEY, session) + + @tag("gh-4862", "taiga-wdw-57") + def test_logout_as_cosigner(self): + """ + A cosigner logging out may not result in auth attributes being hashed. + + The BSN/KVK is still needed for the full registration phase. + """ + submission = SubmissionFactory.from_components( + form__slug="form-to-cosign", + form__authentication_backends=["digid"], + components_list=[{"type": "cosign", "key": "cosign"}], + submitted_data={"cosign": "test@example.com"}, + auth_info__value="111222333", + completed=True, + cosign_complete=False, + pre_registration_completed=True, + public_registration_reference="OF-9999", + ) + # simulate logging in as cosigner and starting the cosign flow + session = self.client.session + session[FORM_AUTH_SESSION_KEY] = { + "plugin": "digid", + "attribute": "bsn", + "value": "123456782", + "loa": DIGID_DEFAULT_LOA, + } + session.save() + view_url = reverse( + "submissions:find-submission-for-cosign", + kwargs={"form_slug": "form-to-cosign"}, + ) + response = self.client.post( + view_url, data={"code": "OF-9999"}, format="multipart" + ) + assert response.status_code == 302 + + url = reverse("api:submission-logout", kwargs={"uuid": submission.uuid}) + + response = self.client.delete(url) + + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + submission.refresh_from_db() + self.assertEqual(submission.auth_info.value, "111222333") + uuids = self._get_session_submission_uuids() + self.assertNotIn(str(submission.uuid), uuids) From c1bae9b66da96763178256157b84fe23ca42b3f9 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 27 Nov 2024 15:26:06 +0100 Subject: [PATCH 2/3] :bug: [#4862] Prevent submissions identifying attributes hashing if they still need cosigning A submission is only processed/registered when it's cosigned, so we may definitely not hash the identifying attributes when it's still pending cosigning. The logout endpoint can't tell if the cosigner or the original author is logged in to perform the cosigning, so we must look at the state of the submission itself. --- src/openforms/authentication/api/views.py | 13 ++++++---- .../submissions/models/submission.py | 26 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/openforms/authentication/api/views.py b/src/openforms/authentication/api/views.py index 7e21661b2d..2e7fa5ae5b 100644 --- a/src/openforms/authentication/api/views.py +++ b/src/openforms/authentication/api/views.py @@ -68,11 +68,14 @@ def delete(self, request, *args, **kwargs): plugin = register[submission.auth_info.plugin] plugin.logout(request) - if not submission.auth_info.attribute_hashed: - submission.auth_info.hash_identifying_attributes() - - if not submission.registrator.attribute_hashed: - submission.registrator.hash_identifying_attributes() + if submission.is_ready_to_hash_identifying_attributes: + if not submission.auth_info.attribute_hashed: + submission.auth_info.hash_identifying_attributes() + + if ( + registrator := submission.registrator + ) and not registrator.attribute_hashed: + registrator.hash_identifying_attributes() if FORM_AUTH_SESSION_KEY in request.session: del request.session[FORM_AUTH_SESSION_KEY] diff --git a/src/openforms/submissions/models/submission.py b/src/openforms/submissions/models/submission.py index c4e62a43cd..b8612c472c 100644 --- a/src/openforms/submissions/models/submission.py +++ b/src/openforms/submissions/models/submission.py @@ -469,8 +469,34 @@ def has_registrator(self): def is_completed(self): return bool(self.completed_on) + @property + def is_ready_to_hash_identifying_attributes(self) -> bool: + """ + Check if the submission can safely hash the identifying auth attributes. + """ + # no authentication -> there's nothing to hash + if not self.is_authenticated: + return False + # completed, but not cosigned yet -> registration after cosigning requires + # unhashed attributes + if self.is_completed and self.waiting_on_cosign: + return False + + # the submission has not been submitted/completed/finalized, so it will + # definitely not be processed yet -> okay to hash + if not self.is_completed: + return True + + # fully registered, no more processing needed -> safe to hash + if self.is_registered: + return True + + # otherwise assume it's not safe yet + return False + @property def is_registered(self) -> bool: + # success is set if it succeeded or there was no registration backend configured return self.registration_status == RegistrationStatuses.success @transaction.atomic() From fc750278e6a9b01c04cdb133cbd23891efb35763 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 27 Nov 2024 15:34:07 +0100 Subject: [PATCH 3/3] :art: [#4862] Re-use new property in cleanup task This should help centralize the logic for when a submission is ready to have its identifying attributes hashed. --- src/openforms/authentication/api/views.py | 2 +- src/openforms/submissions/models/submission.py | 2 +- src/openforms/submissions/tasks/cleanup.py | 7 +------ 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/openforms/authentication/api/views.py b/src/openforms/authentication/api/views.py index 2e7fa5ae5b..1ebbe8e90d 100644 --- a/src/openforms/authentication/api/views.py +++ b/src/openforms/authentication/api/views.py @@ -39,7 +39,7 @@ def get_objects(self): return list(register.iter_enabled_plugins()) -class SubmissionLogoutView(GenericAPIView): +class SubmissionLogoutView(GenericAPIView[Submission]): authentication_classes = (AnonCSRFSessionAuthentication,) permission_classes = (ActiveSubmissionPermission,) serializer_class = ( diff --git a/src/openforms/submissions/models/submission.py b/src/openforms/submissions/models/submission.py index b8612c472c..f41ec6716f 100644 --- a/src/openforms/submissions/models/submission.py +++ b/src/openforms/submissions/models/submission.py @@ -451,7 +451,7 @@ def form_login_required(self): return self.form.login_required @property - def is_authenticated(self): + def is_authenticated(self) -> bool: return hasattr(self, "auth_info") @property diff --git a/src/openforms/submissions/tasks/cleanup.py b/src/openforms/submissions/tasks/cleanup.py index 1ca0e13c7e..2ed0d12a61 100644 --- a/src/openforms/submissions/tasks/cleanup.py +++ b/src/openforms/submissions/tasks/cleanup.py @@ -8,7 +8,6 @@ from openforms.celery import app -from ..constants import RegistrationStatuses from ..models import Submission from ..status import SubmissionProcessingStatus from ..tokens import submission_status_token_generator @@ -74,11 +73,7 @@ def maybe_hash_identifying_attributes(submission_id: int) -> None: submission = Submission.objects.get(id=submission_id) assert submission.completed_on is not None, "Submission must be completed first" - # success is set if it succeeded or there was no registration backend configured - if submission.registration_status != RegistrationStatuses.success: - return - - if not submission.is_authenticated: + if not submission.is_ready_to_hash_identifying_attributes: return if not submission.auth_info.attribute_hashed: