♻️ [#4398] Clean up ownership validation in prefill plugins

* Ensure we use the deserialized, strongly typed options in plugins * Dropped unused error cases/flows that are obsoleted * Made serializer options all required, since the prefill cannot possibly work without and must match the type definitions of the options.
open-formulieren · Dec 3, 2024 · de93db9 · de93db9
1 parent f17486d
commit de93db9
Show file tree

Hide file tree

Showing 11 changed files with 264 additions and 197 deletions.
diff --git a/src/openforms/contrib/objects_api/ownership_validation.py b/src/openforms/contrib/objects_api/ownership_validation.py
@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import logging
-from typing import TYPE_CHECKING
 
 from django.core.exceptions import PermissionDenied
 
@@ -10,14 +9,12 @@
 
 from openforms.contrib.objects_api.clients import ObjectsClient
 from openforms.logging import logevent
+from openforms.prefill.base import BasePlugin as BasePrefillPlugin
+from openforms.registrations.base import BasePlugin as BaseRegistrationPlugin
+from openforms.submissions.models import Submission
 
 logger = logging.getLogger(__name__)
 
-if TYPE_CHECKING:
-    from openforms.prefill.base import BasePlugin as BasePrefillPlugin
-    from openforms.registrations.base import BasePlugin as BaseRegistrationPlugin
-    from openforms.submissions.models import Submission
-
 
 def validate_object_ownership(
     submission: Submission,
@@ -63,7 +60,7 @@ def validate_object_ownership(
         )
 
     try:
-        auth_value = glom(object["record"]["data"], Path(*(object_attribute or [])))
+        auth_value = glom(object["record"]["data"], Path(*object_attribute))
     except PathAccessError as e:
         logger.exception(
             "Could not retrieve auth value for path %s, it could be incorrectly configured",

diff --git a/src/openforms/logging/models.py b/src/openforms/logging/models.py
@@ -1,3 +1,4 @@
+from django.contrib.contenttypes.models import ContentType
 from django.db import models
 from django.template.defaultfilters import capfirst
 from django.urls import reverse
@@ -13,6 +14,11 @@
 
 
 class TimelineLogProxyQueryset(models.QuerySet):
+    # vendored from https://github.com/maykinmedia/django-timeline-logger/pull/32/files
+    def for_object(self, obj: models.Model):
+        content_type = ContentType.objects.get_for_model(obj)
+        return self.filter(content_type=content_type, object_id=obj.pk)
+
     def filter_event(self, event: str):
         return self.filter(extra_data__log_event=event)
 

diff --git a/src/openforms/prefill/base.py b/src/openforms/prefill/base.py
@@ -66,7 +66,7 @@ def get_prefill_values(
         raise NotImplementedError("You must implement the 'get_prefill_values' method.")
 
     def verify_initial_data_ownership(
-        self, submission: Submission, prefill_options: dict
+        self, submission: Submission, prefill_options: OptionsT
     ) -> None:
         """
         Hook to check if the authenticated user is the owner of the object
@@ -75,7 +75,8 @@ def verify_initial_data_ownership(
         If any error occurs in this check, it should raise a `PermissionDenied`
 
         :param submission: an active :class:`Submission` instance
-        :param prefill_options: a dictionary containing the configuration options
+        :param prefill_options: the configuration options, after validation and
+          deserialization through the :attr:`options` serializer class.
         """
         raise NotImplementedError(
             "You must implement the 'verify_initial_data_ownership' method."

diff --git a/src/openforms/prefill/contrib/objects_api/api/serializers.py b/src/openforms/prefill/contrib/objects_api/api/serializers.py
@@ -24,12 +24,15 @@ class PrefillTargetPathsSerializer(serializers.Serializer):
 
 
 class ObjecttypeVariableMappingSerializer(serializers.Serializer):
-    """A mapping between a form variable key and the corresponding Objecttype attribute."""
+    """
+    A mapping between a form variable key and the corresponding Objecttype attribute.
+    """
 
     variable_key = FormioVariableKeyField(
         label=_("variable key"),
         help_text=_(
-            "The 'dotted' path to a form variable key. The format should comply to how Formio handles nested component keys."
+            "The 'dotted' path to a form variable key. The format should comply to how "
+            "Formio handles nested component keys."
         ),
     )
     target_path = serializers.ListField(
@@ -47,30 +50,31 @@ class ObjectsAPIOptionsSerializer(JsonSchemaSerializerMixin, serializers.Seriali
             Q(objects_service=None) | Q(objecttypes_service=None)
         ),
         label=("Objects API group"),
-        required=False,
+        required=True,
         help_text=_("Which Objects API group to use."),
     )
     objecttype_uuid = serializers.UUIDField(
         label=_("objecttype"),
-        required=False,
+        required=True,
         help_text=_("UUID of the objecttype in the Objecttypes API. "),
     )
     objecttype_version = serializers.IntegerField(
         label=_("objecttype version"),
-        required=False,
+        required=True,
         help_text=_("Version of the objecttype in the Objecttypes API."),
     )
     auth_attribute_path = serializers.ListField(
         child=serializers.CharField(label=_("Segment of a JSON path")),
         label=_("Path to auth attribute (e.g. BSN/KVK) in objects"),
         help_text=_(
-            "This is used to perform validation to verify that the authenticated user is the owner of the object."
+            "This is used to perform validation to verify that the authenticated "
+            "user is the owner of the object."
         ),
         allow_empty=False,
         required=True,
     )
     variables_mapping = ObjecttypeVariableMappingSerializer(
         label=_("variables mapping"),
         many=True,
-        required=False,
+        required=True,
     )
diff --git a/src/openforms/prefill/contrib/objects_api/plugin.py b/src/openforms/prefill/contrib/objects_api/plugin.py
@@ -32,32 +32,26 @@ class ObjectsAPIPrefill(BasePlugin[ObjectsAPIOptions]):
     options = ObjectsAPIOptionsSerializer
 
     def verify_initial_data_ownership(
-        self, submission: Submission, prefill_options: dict
+        self, submission: Submission, prefill_options: ObjectsAPIOptions
     ) -> None:
         assert submission.initial_data_reference
-        api_group = ObjectsAPIGroupConfig.objects.filter(
-            pk=prefill_options.get("objects_api_group")
-        ).first()
+        api_group = prefill_options["objects_api_group"]
+        assert api_group, "Can't do anything useful without an API group"
 
-        if not api_group:
-            logger.info(
-                "No api group found to perform initial_data_reference ownership check for submission %s with options %s",
-                submission,
-                prefill_options,
-            )
-            return
-
-        auth_attribute_path = prefill_options.get("auth_attribute_path")
+        auth_attribute_path = prefill_options["auth_attribute_path"]
         if not auth_attribute_path:
             logger.info(
-                "Cannot perform initial data ownership check, because `auth_attribute_path` is missing from %s",
+                "Cannot perform initial data ownership check, because "
+                "`auth_attribute_path` is unset in the options %r",
                 prefill_options,
+                extra={"options": prefill_options},
             )
             logevent.object_ownership_check_improperly_configured(
                 submission, plugin=self
             )
             raise PermissionDenied(
-                f"`auth_attribute_path` missing from options {prefill_options}, cannot perform initial data ownership check"
+                "Can't run ownership check because the 'auth_attribute_path' option is "
+                "not set."
             )
 
         with get_objects_client(api_group) as client:

diff --git a/...kTests.test_verify_initial_data_ownership_called_if_initial_data_reference_specified.yaml b/...kTests.test_verify_initial_data_ownership_called_if_initial_data_reference_specified.yaml
@@ -15,10 +15,10 @@ interactions:
       User-Agent:
       - python-requests/2.32.2
     method: GET
-    uri: http://localhost:8002/api/v2/objects/bd6a9316-f721-40e4-9d4d-73ada5590952
+    uri: http://localhost:8002/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea
   response:
     body:
-      string: '{"url":"http://objects-web:8000/api/v2/objects/bd6a9316-f721-40e4-9d4d-73ada5590952","uuid":"bd6a9316-f721-40e4-9d4d-73ada5590952","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","some":{"path":"foo"}},"geometry":null,"startAt":"2024-11-26","endAt":null,"registrationAt":"2024-11-26","correctionFor":null,"correctedBy":null}}'
+      string: '{"url":"http://objects-web:8000/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea","uuid":"c7ec7188-c8f5-4c13-bd74-942468077eea","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","some":{"path":"foo"}},"geometry":null,"startAt":"2024-12-03","endAt":null,"registrationAt":"2024-12-03","correctionFor":null,"correctedBy":null}}'
     headers:
       Allow:
       - GET, PUT, PATCH, DELETE, HEAD, OPTIONS
@@ -33,11 +33,59 @@ interactions:
       Cross-Origin-Opener-Policy:
       - same-origin
       Date:
-      - Tue, 26 Nov 2024 13:21:31 GMT
+      - Tue, 03 Dec 2024 16:13:43 GMT
       Referrer-Policy:
       - same-origin
       Server:
-      - nginx/1.27.0
+      - nginx/1.27.3
+      Vary:
+      - origin
+      X-Content-Type-Options:
+      - nosniff
+      X-Frame-Options:
+      - DENY
+    status:
+      code: 200
+      message: OK
+- request:
+    body: null
+    headers:
+      Accept:
+      - '*/*'
+      Accept-Encoding:
+      - gzip, deflate, br
+      Authorization:
+      - Token 7657474c3d75f56ae0abd0d1bf7994b09964dca9
+      Connection:
+      - keep-alive
+      Content-Crs:
+      - EPSG:4326
+      User-Agent:
+      - python-requests/2.32.2
+    method: GET
+    uri: http://localhost:8002/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea
+  response:
+    body:
+      string: '{"url":"http://objects-web:8000/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea","uuid":"c7ec7188-c8f5-4c13-bd74-942468077eea","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","some":{"path":"foo"}},"geometry":null,"startAt":"2024-12-03","endAt":null,"registrationAt":"2024-12-03","correctionFor":null,"correctedBy":null}}'
+    headers:
+      Allow:
+      - GET, PUT, PATCH, DELETE, HEAD, OPTIONS
+      Connection:
+      - keep-alive
+      Content-Crs:
+      - EPSG:4326
+      Content-Length:
+      - '432'
+      Content-Type:
+      - application/json
+      Cross-Origin-Opener-Policy:
+      - same-origin
+      Date:
+      - Tue, 03 Dec 2024 16:13:43 GMT
+      Referrer-Policy:
+      - same-origin
+      Server:
+      - nginx/1.27.3
       Vary:
       - origin
       X-Content-Type-Options:

diff --git a/...pCheckTests.test_verify_initial_data_ownership_raising_errors_causes_prefill_to_fail.yaml b/...pCheckTests.test_verify_initial_data_ownership_raising_errors_causes_prefill_to_fail.yaml
@@ -0,0 +1,50 @@
+interactions:
+- request:
+    body: null
+    headers:
+      Accept:
+      - '*/*'
+      Accept-Encoding:
+      - gzip, deflate, br
+      Authorization:
+      - Token 7657474c3d75f56ae0abd0d1bf7994b09964dca9
+      Connection:
+      - keep-alive
+      Content-Crs:
+      - EPSG:4326
+      User-Agent:
+      - python-requests/2.32.2
+    method: GET
+    uri: http://localhost:8002/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea
+  response:
+    body:
+      string: '{"url":"http://objects-web:8000/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea","uuid":"c7ec7188-c8f5-4c13-bd74-942468077eea","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","some":{"path":"foo"}},"geometry":null,"startAt":"2024-12-03","endAt":null,"registrationAt":"2024-12-03","correctionFor":null,"correctedBy":null}}'
+    headers:
+      Allow:
+      - GET, PUT, PATCH, DELETE, HEAD, OPTIONS
+      Connection:
+      - keep-alive
+      Content-Crs:
+      - EPSG:4326
+      Content-Length:
+      - '432'
+      Content-Type:
+      - application/json
+      Cross-Origin-Opener-Policy:
+      - same-origin
+      Date:
+      - Tue, 03 Dec 2024 16:33:22 GMT
+      Referrer-Policy:
+      - same-origin
+      Server:
+      - nginx/1.27.3
+      Vary:
+      - origin
+      X-Content-Type-Options:
+      - nosniff
+      X-Frame-Options:
+      - DENY
+    status:
+      code: 200
+      message: OK
+version: 1
diff --git a/...api/tests/files/vcr_cassettes/ObjectsAPIPrefillDataOwnershipCheckTests/setUpTestData.yaml b/...api/tests/files/vcr_cassettes/ObjectsAPIPrefillDataOwnershipCheckTests/setUpTestData.yaml
@@ -2,7 +2,7 @@ interactions:
 - request:
     body: '{"type": "http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e",
       "record": {"typeVersion": 1, "data": {"bsn": "111222333", "some": {"path": "foo"}},
-      "startAt": "2024-11-26"}}'
+      "startAt": "2024-12-03"}}'
     headers:
       Accept:
       - '*/*'
@@ -24,7 +24,7 @@ interactions:
     uri: http://localhost:8002/api/v2/objects
   response:
     body:
-      string: '{"url":"http://objects-web:8000/api/v2/objects/bd6a9316-f721-40e4-9d4d-73ada5590952","uuid":"bd6a9316-f721-40e4-9d4d-73ada5590952","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","some":{"path":"foo"}},"geometry":null,"startAt":"2024-11-26","endAt":null,"registrationAt":"2024-11-26","correctionFor":null,"correctedBy":null}}'
+      string: '{"url":"http://objects-web:8000/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea","uuid":"c7ec7188-c8f5-4c13-bd74-942468077eea","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","some":{"path":"foo"}},"geometry":null,"startAt":"2024-12-03","endAt":null,"registrationAt":"2024-12-03","correctionFor":null,"correctedBy":null}}'
     headers:
       Allow:
       - GET, POST, HEAD, OPTIONS
@@ -39,13 +39,13 @@ interactions:
       Cross-Origin-Opener-Policy:
       - same-origin
       Date:
-      - Tue, 26 Nov 2024 13:21:30 GMT
+      - Tue, 03 Dec 2024 16:13:43 GMT
       Location:
-      - http://localhost:8002/api/v2/objects/bd6a9316-f721-40e4-9d4d-73ada5590952
+      - http://localhost:8002/api/v2/objects/c7ec7188-c8f5-4c13-bd74-942468077eea
       Referrer-Policy:
       - same-origin
       Server:
-      - nginx/1.27.0
+      - nginx/1.27.3
       Vary:
       - origin
       X-Content-Type-Options: