Skip to content

Commit

Permalink
Merge pull request #4271 from open-formulieren/issue/4199-session-per…
Browse files Browse the repository at this point in the history
…sistance-ignores-login-choice

🐛 [#4199] Delete authinfo from session after it's stored on submission
  • Loading branch information
stevenbal authored May 24, 2024
2 parents 6821725 + 49783de commit ba91722
Show file tree
Hide file tree
Showing 12 changed files with 128 additions and 9 deletions.
2 changes: 1 addition & 1 deletion docs/configuration/authentication/eherkenning_eidas.rst
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ that describes the authenticated entity (person or company).
EH4 hoog/high urn:etoegang:core:assurance-class:loa4
=========== =============== ===========================================

Source: `Afsprakenstelsel eToegang <https://afsprakenstelsel.etoegang.nl/display/as/Level+of+assurance>`_
Source: `Afsprakenstelsel eToegang <https://afsprakenstelsel.etoegang.nl/Startpagina/v1/level-of-assurance>`_

.. note::

Expand Down
5 changes: 5 additions & 0 deletions src/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9600,6 +9600,11 @@ components:
format: uri
description: URL where the user initialized the submission.
maxLength: 1000
anonymous:
type: boolean
writeOnly: true
default: false
description: Whether the submission was started anonymously or not.
required:
- form
- formUrl
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ def test_raising_loa_requirements_fails_in_flight_authentications(self):
acs_response_2 = self.client.get(acs_url_2.url, follow=True)

# we are logged in
self.assertIn(FORM_AUTH_SESSION_KEY, self.client.session)
self.assertTrue(submission.is_authenticated)
# and we end up starting the form
self.assertEqual(acs_response_2.resolver_match.view_name, "core:form-detail")
self.assertEqual(acs_response_2.resolver_match.kwargs["slug"], "slurm")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ def test_raising_loa_requirements_fails_in_flight_authentications(self):
acs_response_2 = self.client.get(acs_url_2.url, follow=True)

# we are logged in
self.assertIn(FORM_AUTH_SESSION_KEY, self.client.session)
self.assertTrue(submission.is_authenticated)
# and we end up starting the form
self.assertEqual(acs_response_2.resolver_match.view_name, "core:form-detail")
self.assertEqual(acs_response_2.resolver_match.kwargs["slug"], "slurm")
Expand Down
2 changes: 2 additions & 0 deletions src/openforms/authentication/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from .utils import (
is_authenticated_with_an_allowed_plugin,
is_authenticated_with_plugin,
remove_auth_info_from_session,
store_auth_details,
)

Expand All @@ -13,4 +14,5 @@
"store_auth_details",
"is_authenticated_with_plugin",
"is_authenticated_with_an_allowed_plugin",
"remove_auth_info_from_session",
]
18 changes: 16 additions & 2 deletions src/openforms/authentication/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@

from .constants import FORM_AUTH_SESSION_KEY, REGISTRATOR_SUBJECT_SESSION_KEY
from .registry import register
from .utils import store_auth_details, store_registrator_details
from .utils import (
remove_auth_info_from_session,
store_auth_details,
store_registrator_details,
)

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -53,8 +57,11 @@
[submission_start, submission_resumed], dispatch_uid="auth.set_submission_form_auth"
)
def set_auth_attribute_on_session(
sender, instance: Submission, request: Request, **kwargs
sender, instance: Submission, request: Request, anonymous=False, **kwargs
):
if anonymous:
return

# form_auth has information from an authentication backend, so could be a client or employee
form_auth = request.session.get(FORM_AUTH_SESSION_KEY)

Expand Down Expand Up @@ -123,6 +130,11 @@ def set_auth_attribute_on_session(
else:
store_auth_details(instance, form_auth)

# After the authentication details have been attached to the submission, the session
# no longer has to keep track of this information, because the user could start another
# form with different / no authentication
remove_auth_info_from_session(request)


@receiver(
[submission_complete, authentication_logout],
Expand All @@ -145,3 +157,5 @@ def set_cosign_data_on_submission(
instance.co_sign_data = form_auth
instance.co_sign_data["cosign_date"] = timezone.now().isoformat()
instance.save()

remove_auth_info_from_session(request)
29 changes: 28 additions & 1 deletion src/openforms/authentication/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,19 @@
from django.urls import reverse

from furl import furl
from rest_framework.test import APIRequestFactory

from openforms.authentication.contrib.digid.constants import DIGID_DEFAULT_LOA
from openforms.forms.tests.factories import FormStepFactory
from openforms.submissions.tests.factories import SubmissionFactory

from ..utils import get_cosign_login_url, store_auth_details, store_registrator_details
from ..constants import FORM_AUTH_SESSION_KEY, AuthAttribute
from ..utils import (
get_cosign_login_url,
remove_auth_info_from_session,
store_auth_details,
store_registrator_details,
)


class UtilsTests(TestCase):
Expand Down Expand Up @@ -71,3 +79,22 @@ def test_get_cosign_info(self):
kwargs={"form_slug": "form-with-cosign"},
),
)

def test_remove_auth_info_from_session(self):
factory = APIRequestFactory()
request = factory.get("/foo")
request.session = self.client.session

request.session[FORM_AUTH_SESSION_KEY] = {
"plugin": "digid",
"attribute": AuthAttribute.bsn,
"value": "123456782",
"loa": DIGID_DEFAULT_LOA,
}

remove_auth_info_from_session(request)

self.assertNotIn(FORM_AUTH_SESSION_KEY, request.session)

# Should not raise any errors if the key is not present in the session
remove_auth_info_from_session(request)
6 changes: 6 additions & 0 deletions src/openforms/authentication/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from openforms.forms.models import Form
from openforms.submissions.models import Submission
from openforms.typing import AnyRequest

from .constants import FORM_AUTH_SESSION_KEY, AuthAttribute
from .models import AuthInfo, RegistratorInfo
Expand Down Expand Up @@ -81,3 +82,8 @@ def get_cosign_login_url(request: Request, form: Form, plugin_id: str) -> str:
auth_page = furl(auth_url)
auth_page.args.set("next", next_url)
return auth_page.url


def remove_auth_info_from_session(request: AnyRequest) -> None:
if FORM_AUTH_SESSION_KEY in request.session:
del request.session[FORM_AUTH_SESSION_KEY]
13 changes: 13 additions & 0 deletions src/openforms/submissions/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,14 @@ class SubmissionSerializer(serializers.HyperlinkedModelSerializer):
read_only=True,
)

anonymous = serializers.BooleanField(
label=_("Anonymous"),
help_text=_("Whether the submission was started anonymously or not."),
required=False,
write_only=True,
default=False, # type: ignore
)

class Meta:
model = Submission
fields = (
Expand All @@ -159,6 +167,7 @@ class Meta:
"is_authenticated",
"payment",
"form_url",
"anonymous",
)
extra_kwargs = {
"id": {
Expand All @@ -183,6 +192,10 @@ class Meta:
},
}

def create(self, validated_data):
validated_data.pop("anonymous", None)
return super().create(validated_data)

def to_representation(self, instance):
check_submission_logic(instance, unsaved_data=self.context.get("unsaved_data"))
return super().to_representation(instance)
Expand Down
5 changes: 4 additions & 1 deletion src/openforms/submissions/api/viewsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,10 @@ def perform_create(self, serializer):

# dispatch signal for modules to tap into
submission_start.send(
sender=self.__class__, instance=serializer.instance, request=self.request
sender=self.__class__,
instance=serializer.instance,
request=self.request,
anonymous=serializer.validated_data["anonymous"],
)

# store the submission ID in the session, so that only the session owner can
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
If authentication is optional, then this behaviour does not apply.
"""

from unittest.mock import patch

from django.test import override_settings, tag

from rest_framework import status
Expand Down Expand Up @@ -51,16 +53,21 @@ def setUpTestData(cls) -> None:
"api:form-detail", kwargs={"uuid_or_slug": cls.form.uuid}
)

def test_start_submission_is_allowed(self):
@patch("openforms.submissions.api.viewsets.submission_start.send", autospec=True)
def test_start_submission_is_allowed(self, mock_signal):
body = {
"form": f"http://testserver.com{self.form_url}",
"formUrl": "http://testserver.com/my-form",
"anonymous": True,
}

response = self.client.post(self.endpoint, body, HTTP_HOST="testserver.com")

self.assertEqual(response.status_code, status.HTTP_201_CREATED)

mock_signal.assert_called_once()
self.assertEqual(mock_signal.call_args_list[0].kwargs["anonymous"], True)

def test_submitting_step_data_is_allowed_anon_user(self):
submission = SubmissionFactory.create(form=self.form)
assert not submission.is_authenticated, "Submission must be anonymous"
Expand Down
44 changes: 43 additions & 1 deletion src/openforms/submissions/tests/test_start_submission.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

from unittest.mock import patch

from django.test import override_settings
from django.test import override_settings, tag

from rest_framework import status
from rest_framework.reverse import reverse, reverse_lazy
Expand Down Expand Up @@ -125,6 +125,48 @@ def test_start_submission_bsn_in_session(self):
self.assertEqual(submission.auth_info.value, "123456782")
self.assertEqual(submission.auth_info.plugin, "digid")

# Auth info should be removed from session after the submission is started
self.assertNotIn(FORM_AUTH_SESSION_KEY, self.client.session)

@tag("gh-4199")
def test_two_submissions_within_same_session(self):
session = self.client.session
session[FORM_AUTH_SESSION_KEY] = {
"plugin": "digid",
"attribute": AuthAttribute.bsn,
"value": "123456782",
}
session.save()

body = {
"form": f"http://testserver.com{self.form_url}",
"formUrl": "http://testserver.com/my-form",
}

response = self.client.post(self.endpoint, body)

self.assertEqual(response.status_code, status.HTTP_201_CREATED)
submission = Submission.objects.get()
self.assertEqual(submission.auth_info.value, "123456782")
self.assertEqual(submission.auth_info.plugin, "digid")

# Auth info should be removed from session after the submission is started
self.assertNotIn(FORM_AUTH_SESSION_KEY, self.client.session)

body = {
"form": f"http://testserver.com{self.form_url}",
"formUrl": "http://testserver.com/my-form",
"anonymous": True,
}

response = self.client.post(self.endpoint, body)

self.assertEqual(response.status_code, status.HTTP_201_CREATED)

submission = Submission.objects.last()
with self.assertRaises(Submission.auth_info.RelatedObjectDoesNotExist):
submission.auth_info

def test_start_submission_on_deleted_form(self):
form = FormFactory.create(deleted_=True)
FormStepFactory.create(form=form)
Expand Down

0 comments on commit ba91722

Please sign in to comment.