♻️ [#4398] Move ownership check to proper location in prefill code

src/openforms/prefill/sources.py

@@ -1,6 +1,8 @@
 import logging
 from collections import defaultdict
 
+from django.core.exceptions import PermissionDenied
+
 import elasticapm
 from rest_framework.exceptions import ValidationError
 from zgw_consumers.concurrent import parallel
@@ -54,11 +56,6 @@ def invoke_plugin(
         if not plugin.is_enabled:
             raise PluginNotEnabled()
 
-        # If an `initial_data_reference` was passed, we must verify that the
-        # authenticated user is the owner of the referenced object
-        if submission.initial_data_reference:
-            plugin.verify_initial_data_ownership(submission)
-
         attributes = [attribute for field in fields for attribute in field]
         try:
             values = plugin.get_prefill_values(submission, attributes, identifier_role)
@@ -101,6 +98,16 @@ def fetch_prefill_values_from_options(
     values: dict[str, JSONEncodable] = {}
     for variable in variables:
         plugin = register[variable.form_variable.prefill_plugin]
+
+        # If an `initial_data_reference` was passed, we must verify that the
+        # authenticated user is the owner of the referenced object
+        try:
+            if submission.initial_data_reference:
+                plugin.verify_initial_data_ownership(submission)
+        except PermissionDenied as exc:
+            logevent.prefill_retrieve_failure(submission, plugin, exc)
+            continue
+
         options_serializer = plugin.options(data=variable.form_variable.prefill_options)
 
         try:

src/openforms/prefill/tests/test_prefill_variables.py

@@ -1,7 +1,7 @@
 from unittest.mock import patch
 
 from django.core.exceptions import PermissionDenied
-from django.test import RequestFactory, TestCase, TransactionTestCase
+from django.test import RequestFactory, TestCase, TransactionTestCase, tag
 
 import requests_mock
 from zgw_consumers.test.factories import ServiceFactory
@@ -19,7 +19,6 @@
     FormVariableFactory,
 )
 from openforms.logging.models import TimelineLogProxy
-from openforms.prefill.contrib.demo.constants import Attributes
 from openforms.submissions.constants import SubmissionValueVariableSources
 from openforms.submissions.tests.factories import (
     SubmissionFactory,
@@ -321,6 +320,7 @@ def test_no_success_message_on_failure(self, m, m_solo):
         for log in logs:
             self.assertNotEqual(log.event, "prefill_retrieve_success")
 
+    @tag("gh-4398")
     def test_verify_initial_data_ownership(self):
         form_step = FormStepFactory.create(
             form_definition__configuration={
@@ -329,15 +329,34 @@ def test_verify_initial_data_ownership(self):
                         "type": "postcode",
                         "key": "postcode",
                         "inputMask": "9999 AA",
-                        "prefill": {
-                            "plugin": "demo",
-                            "attribute": Attributes.random_string,
-                        },
-                        "defaultValue": "",
                     }
                 ]
             }
         )
+        # FormVariableFactory.create(
+        #     key="voornamen",
+        #     form=form_step.form,
+        #     prefill_plugin="objects_api",
+        #     prefill_attribute="",
+        #     prefill_options={
+        #         "version": 2,
+        #         "objecttype": "3edfdaf7-f469-470b-a391-bb7ea015bd6f",
+        #         "objects_api_group": 1,
+        #         "objecttype_version": 1,
+        #     },
+        # )
+        FormVariableFactory.create(
+            key="voornamen",
+            form=form_step.form,
+            prefill_plugin="demo",
+            prefill_attribute="",
+            prefill_options={
+                "version": 2,
+                "objecttype": "3edfdaf7-f469-470b-a391-bb7ea015bd6f",
+                "objects_api_group": 1,
+                "objecttype_version": 1,
+            },
+        )
 
         with self.subTest(
             "verify_initial_data_ownership is not called if initial_data_reference is not specified"
@@ -352,9 +371,13 @@ def test_verify_initial_data_ownership(self):
             with patch(
                 "openforms.prefill.contrib.demo.plugin.DemoPrefill.verify_initial_data_ownership"
             ) as mock_verify_ownership:
-                prefill_variables(submission=submission_step.submission)
+                with patch(
+                    "openforms.prefill.contrib.demo.plugin.DemoPrefill.get_prefill_values_from_options",
+                    return_value={"postcode": "1234AB"},
+                ):
+                    prefill_variables(submission=submission_step.submission)
 
-                mock_verify_ownership.assert_not_called()
+                    mock_verify_ownership.assert_not_called()
 
             logs = TimelineLogProxy.objects.filter(
                 object_id=submission_step.submission.id
@@ -377,11 +400,15 @@ def test_verify_initial_data_ownership(self):
             with patch(
                 "openforms.prefill.contrib.demo.plugin.DemoPrefill.verify_initial_data_ownership"
             ) as mock_verify_ownership:
-                prefill_variables(submission=submission_step.submission)
+                with patch(
+                    "openforms.prefill.contrib.demo.plugin.DemoPrefill.get_prefill_values_from_options",
+                    return_value={"postcode": "1234AB"},
+                ):
+                    prefill_variables(submission=submission_step.submission)
 
-                mock_verify_ownership.assert_called_once_with(
-                    submission_step.submission
-                )
+                    mock_verify_ownership.assert_called_once_with(
+                        submission_step.submission
+                    )
 
             logs = TimelineLogProxy.objects.filter(
                 object_id=submission_step.submission.id
@@ -405,8 +432,7 @@ def test_verify_initial_data_ownership(self):
                 "openforms.prefill.contrib.demo.plugin.DemoPrefill.verify_initial_data_ownership",
                 side_effect=PermissionDenied,
             ) as mock_verify_ownership:
-                with self.assertRaises(PermissionDenied):
-                    prefill_variables(submission=submission_step.submission)
+                prefill_variables(submission=submission_step.submission)
 
                 mock_verify_ownership.assert_called_once_with(
                     submission_step.submission
@@ -418,3 +444,6 @@ def test_verify_initial_data_ownership(self):
             self.assertEqual(
                 logs.filter(extra_data__log_event="prefill_retrieve_success").count(), 0
             )
+            self.assertEqual(
+                logs.filter(extra_data__log_event="prefill_retrieve_failure").count(), 1
+            )