From a378c38ccb0eca62bdddc31b85fb9738346dc500 Mon Sep 17 00:00:00 2001 From: vasileios Date: Thu, 26 Oct 2023 16:19:46 +0200 Subject: [PATCH] [#3477] New PR feedback --- ...026_1133.py => 0058_auto_20231026_1525.py} | 7 ++-- src/openforms/config/models.py | 26 +++++--------- src/openforms/config/tests/test_models.py | 34 ------------------- .../contrib/digid_eherkenning/apps.py | 2 +- .../contrib/digid_eherkenning/constants.py | 12 ++----- .../contrib/digid_eherkenning/signals.py | 11 ++---- .../tests/test_csp_update.py | 21 ++++++------ .../contrib/digid_eherkenning/utils.py | 19 +++++------ 8 files changed, 37 insertions(+), 95 deletions(-) rename src/openforms/config/migrations/{0058_auto_20231026_1133.py => 0058_auto_20231026_1525.py} (85%) delete mode 100644 src/openforms/config/tests/test_models.py diff --git a/src/openforms/config/migrations/0058_auto_20231026_1133.py b/src/openforms/config/migrations/0058_auto_20231026_1525.py similarity index 85% rename from src/openforms/config/migrations/0058_auto_20231026_1133.py rename to src/openforms/config/migrations/0058_auto_20231026_1525.py index 33e36d30db..22fb1191bb 100644 --- a/src/openforms/config/migrations/0058_auto_20231026_1133.py +++ b/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", diff --git a/src/openforms/config/models.py b/src/openforms/config/models.py index a5de33f633..b7d0775230 100644 --- a/src/openforms/config/models.py +++ b/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") diff --git a/src/openforms/config/tests/test_models.py b/src/openforms/config/tests/test_models.py deleted file mode 100644 index 89186131c7..0000000000 --- a/src/openforms/config/tests/test_models.py +++ /dev/null @@ -1,34 +0,0 @@ -from django.test import TestCase - -from openforms.config.constants import CSPDirective -from openforms.config.models import CSPSetting -from openforms.payments.contrib.ogone.tests.factories import OgoneMerchantFactory - - -class CSPSettingManagerTests(TestCase): - def test_csp_setting_is_set(self): - merchant = OgoneMerchantFactory() - - CSPSetting.objects.set_for( - merchant, [(CSPDirective.FORM_ACTION, "http://example.com")] - ) - csp = CSPSetting.objects.get() - - self.assertEqual(csp.content_object, merchant) - self.assertEqual(csp.directive, CSPDirective.FORM_ACTION) - self.assertEqual(csp.value, "http://example.com") - - def test_csp_setting_is_not_set_with_wrong_directive(self): - merchant = OgoneMerchantFactory() - - with self.assertLogs() as logs: - CSPSetting.objects.set_for( - merchant, [("wrong-directive", "http://example.com")] - ) - message = logs.records[0].getMessage() - - self.assertEqual( - message, - f"Could not create csp setting for model '{merchant.__str__()}'. 'wrong-directive' is not a valid directive.", - ) - self.assertTrue(CSPSetting.objects.none) diff --git a/src/openforms/contrib/digid_eherkenning/apps.py b/src/openforms/contrib/digid_eherkenning/apps.py index 60c2b7cb3a..370d446507 100644 --- a/src/openforms/contrib/digid_eherkenning/apps.py +++ b/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 diff --git a/src/openforms/contrib/digid_eherkenning/constants.py b/src/openforms/contrib/digid_eherkenning/constants.py index 9e77ad51f9..d3840d966f 100644 --- a/src/openforms/contrib/digid_eherkenning/constants.py +++ b/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: "", } diff --git a/src/openforms/contrib/digid_eherkenning/signals.py b/src/openforms/contrib/digid_eherkenning/signals.py index 8eae8e86c3..079fd8c447 100644 --- a/src/openforms/contrib/digid_eherkenning/signals.py +++ b/src/openforms/contrib/digid_eherkenning/signals.py @@ -1,14 +1,9 @@ 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) @@ -16,4 +11,4 @@ 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) diff --git a/src/openforms/contrib/digid_eherkenning/tests/test_csp_update.py b/src/openforms/contrib/digid_eherkenning/tests/test_csp_update.py index dad5d87810..c368a707b6 100644 --- a/src/openforms/contrib/digid_eherkenning/tests/test_csp_update.py +++ b/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"], ) diff --git a/src/openforms/contrib/digid_eherkenning/utils.py b/src/openforms/contrib/digid_eherkenning/utils.py index 7507125620..809ffc0c5d 100644 --- a/src/openforms/contrib/digid_eherkenning/utils.py +++ b/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,7 +28,7 @@ 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 @@ -37,11 +36,9 @@ def create_digid_eherkenning_csp_settings( # 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)])