[#3477] New PR feedback

src/openforms/config/migrations/0058_auto_20231026_1133.py → src/openforms/config/migrations/0058_auto_20231026_1525.py

@@ -1,10 +1,11 @@
-# Generated by Django 3.2.21 on 2023-10-26 09:33
+# Generated by Django 3.2.21 on 2023-10-26 13:25
 
 from django.db import migrations, models
 import django.db.models.deletion
 
 
 class Migration(migrations.Migration):
+
     dependencies = [
         ("contenttypes", "0002_remove_content_type_name"),
         ("config", "0057_globalconfiguration_recipients_email_digest"),
@@ -25,9 +26,7 @@ class Migration(migrations.Migration):
         migrations.AddField(
             model_name="cspsetting",
             name="object_id",
-            field=models.TextField(
-                blank=True, db_index=True, null=True, verbose_name="object id"
-            ),
+            field=models.TextField(blank=True, db_index=True, verbose_name="object id"),
         ),
         migrations.AlterField(
             model_name="cspsetting",

src/openforms/config/models.py

@@ -12,7 +12,7 @@
     MinValueValidator,
     RegexValidator,
 )
-from django.db import models
+from django.db import models, transaction
 from django.template.loader import render_to_string
 from django.utils.encoding import force_str
 from django.utils.safestring import mark_safe
@@ -656,24 +656,17 @@ def as_dict(self):
 
 
 class CSPSettingManager(models.Manager.from_queryset(CSPSettingQuerySet)):
-    def set_for(self, obj: models.Model, settings: list[tuple[str, str]]) -> None:
+    @transaction.atomic
+    def set_for(
+        self, obj: models.Model, settings: list[tuple[CSPDirective, str]]
+    ) -> None:
         """
         Deletes all the connected csp settings and creates new ones based on the new provided data.
         """
-        instances = []
-        for setting in settings:
-            directive, value = setting
-            if directive not in CSPDirective.values:
-                logger.error(
-                    "Could not create csp setting for model '%s'. '%s' is not a valid directive.",
-                    obj,
-                    directive,
-                )
-                return
-
-            instances.append(
-                CSPSetting(content_object=obj, directive=directive, value=value)
-            )
+        instances = [
+            CSPSetting(content_object=obj, directive=directive, value=value)
+            for directive, value in settings
+        ]
 
         CSPSetting.objects.filter(
             content_type=get_content_type_for_model(obj), object_id=str(obj.id)
@@ -704,7 +697,6 @@ class CSPSetting(models.Model):
     object_id = models.TextField(
         verbose_name=_("object id"),
         blank=True,
-        null=True,
         db_index=True,
     )
     content_object = GenericForeignKey("content_type", "object_id")

src/openforms/contrib/digid_eherkenning/apps.py

@@ -5,7 +5,7 @@
 class DigidEherkenningApp(AppConfig):
     name = "openforms.contrib.digid_eherkenning"
     label = "contrib_digid_eherkenning"
-    verbose_name = _("DigiD Eherkenning Contrib")
+    verbose_name = _("DigiD/Eherkenning utilities")
 
     def ready(self):
         # register the signals

src/openforms/contrib/digid_eherkenning/constants.py

@@ -1,12 +1,6 @@
-from digid_eherkenning.models.digid import DigidConfiguration
-from digid_eherkenning.models.eherkenning import EherkenningConfiguration
+from digid_eherkenning.models import DigidConfiguration, EherkenningConfiguration
 
 ADDITIONAL_CSP_VALUES = {
-    "digid": "https://digid.nl https://*.digid.nl",
-    "eherkenning": "",
-}
-
-CONFIG_TYPES = {
-    DigidConfiguration: "digid",
-    EherkenningConfiguration: "eherkenning",
+    DigidConfiguration: "https://digid.nl https://*.digid.nl",
+    EherkenningConfiguration: "",
 }

src/openforms/contrib/digid_eherkenning/signals.py

@@ -1,19 +1,14 @@
 from django.db.models.signals import post_save
 from django.dispatch import receiver
 
-from digid_eherkenning.models.digid import DigidConfiguration
-from digid_eherkenning.models.eherkenning import EherkenningConfiguration
+from digid_eherkenning.models import DigidConfiguration, EherkenningConfiguration
 
-from openforms.contrib.digid_eherkenning.utils import (
-    create_digid_eherkenning_csp_settings,
-)
-
-from .constants import CONFIG_TYPES
+from .utils import create_digid_eherkenning_csp_settings
 
 
 @receiver(post_save, sender=DigidConfiguration)
 @receiver(post_save, sender=EherkenningConfiguration)
 def trigger_csp_update(
     sender, instance: DigidConfiguration | EherkenningConfiguration, **kwargs
 ):
-    create_digid_eherkenning_csp_settings(instance, CONFIG_TYPES[type(instance)])
+    create_digid_eherkenning_csp_settings(instance)

src/openforms/contrib/digid_eherkenning/tests/test_csp_update.py

@@ -4,8 +4,7 @@
 from django.test import TestCase, override_settings
 from django.urls import reverse
 
-from digid_eherkenning.models.digid import DigidConfiguration
-from digid_eherkenning.models.eherkenning import EherkenningConfiguration
+from digid_eherkenning.models import DigidConfiguration, EherkenningConfiguration
 from privates.test import temp_private_root
 from simple_certmanager.test.factories import CertificateFactory
 
@@ -69,10 +68,10 @@ def test_csp_updates_for_digid(self, get_metadata):
         self.assertEqual(csp_added.directive, CSPDirective.FORM_ACTION)
         self.assertEqual(
             csp_added.value,
-            "https://digid.nl "
-            "https://*.digid.nl "
             "https://test-digid.nl/saml/idp/request_authentication "
-            "https://test-digid.nl/saml/idp/request_logout",
+            "https://test-digid.nl/saml/idp/request_logout "
+            "https://digid.nl "
+            "https://*.digid.nl",
         )
 
         # assert new csp entry is added and the old one is deleted after url update
@@ -87,10 +86,10 @@ def test_csp_updates_for_digid(self, get_metadata):
         self.assertEqual(csp_updated.directive, CSPDirective.FORM_ACTION)
         self.assertEqual(
             csp_updated.value,
-            "https://digid.nl "
-            "https://*.digid.nl "
             "https://test-digid.nl/saml/idp/request_authentication "
-            "https://test-digid.nl/saml/idp/request_logout",
+            "https://test-digid.nl/saml/idp/request_logout "
+            "https://digid.nl "
+            "https://*.digid.nl",
         )
 
     @patch(
@@ -119,10 +118,10 @@ def test_response_headers_contain_form_action_values_in_digid(self, get_metadata
         self.assertIn(
             "form-action "
             "'self' "
-            "https://digid.nl "
-            "https://*.digid.nl "
             "https://test-digid.nl/saml/idp/request_authentication "
-            "https://test-digid.nl/saml/idp/request_logout;",
+            "https://test-digid.nl/saml/idp/request_logout "
+            "https://digid.nl "
+            "https://*.digid.nl;",
             response.headers["Content-Security-Policy"],
         )
 

src/openforms/contrib/digid_eherkenning/utils.py

@@ -2,8 +2,7 @@
 
 from django.templatetags.static import static
 
-from digid_eherkenning.models.digid import DigidConfiguration
-from digid_eherkenning.models.eherkenning import EherkenningConfiguration
+from digid_eherkenning.models import DigidConfiguration, EherkenningConfiguration
 
 from openforms.authentication.constants import LogoAppearance
 from openforms.config.constants import CSPDirective
@@ -29,19 +28,17 @@ def get_eherkenning_logo(request) -> Dict[str, str]:
 
 
 def create_digid_eherkenning_csp_settings(
-    config: DigidConfiguration | EherkenningConfiguration, config_type: str
+    config: DigidConfiguration | EherkenningConfiguration,
 ) -> None:
     if not config.metadata_file_source:
         return
 
     # create the new directives based on the POST bindings of the metadata XML and
     # the additional constants
     urls, _ = config.process_metadata_from_xml_source()
-    if ADDITIONAL_CSP_VALUES[config_type]:
-        urls = (
-            f"{ADDITIONAL_CSP_VALUES[config_type]} {urls['sso_url']} {urls['slo_url']}"
-        )
-    else:
-        urls = f"{urls['sso_url']} {urls['slo_url']}"
-
-    CSPSetting.objects.set_for(config, [(CSPDirective.FORM_ACTION, urls)])
+    csp_values = [urls["sso_url"], urls["slo_url"]]
+    if additional_csp_values := ADDITIONAL_CSP_VALUES.get(type(config), []):
+        csp_values.append(additional_csp_values)
+
+    form_action_urls = " ".join(csp_values)
+    CSPSetting.objects.set_for(config, [(CSPDirective.FORM_ACTION, form_action_urls)])