[#45] PR feedback process

digid_eherkenning/admin.py

@@ -2,13 +2,22 @@
 from django.utils.translation import gettext_lazy as _
 
 from privates.admin import PrivateMediaMixin
+from privates.widgets import PrivateFileWidget
 from solo.admin import SingletonModelAdmin
 
 from .models import DigidConfiguration, EherkenningConfiguration
 
 
+class CustomPrivateFileWidget(PrivateFileWidget):
+    template_name = "admin/digid_eherkenning/widgets/custom_file_input.html"
+
+
+class CustomPrivateMediaMixin(PrivateMediaMixin):
+    private_media_file_widget = CustomPrivateFileWidget
+
+
 @admin.register(DigidConfiguration)
-class DigidConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin):
+class DigidConfigurationAdmin(CustomPrivateMediaMixin, SingletonModelAdmin):
     readonly_fields = ("idp_service_entity_id",)
     fieldsets = (
         (
@@ -73,7 +82,7 @@ class DigidConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin):
 
 
 @admin.register(EherkenningConfiguration)
-class EherkenningConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin):
+class EherkenningConfigurationAdmin(CustomPrivateMediaMixin, SingletonModelAdmin):
     readonly_fields = ("idp_service_entity_id",)
     fieldsets = (
         (

digid_eherkenning/management/commands/update_stored_metadata.py

@@ -1,40 +1,28 @@
-from django.core.cache import cache
-from django.core.management import BaseCommand, CommandError
+from django.core.management import BaseCommand
 
 from digid_eherkenning.models.digid import DigidConfiguration
 from digid_eherkenning.models.eherkenning import EherkenningConfiguration
 
 
 class Command(BaseCommand):
-    help = "Updates the stored metadata file and repopulates the db fields."
+    help = "Updates the stored metadata file and prepopulates the db fields."
 
     def add_arguments(self, parser):
         parser.add_argument(
-            "--digid",
-            action="store_true",
-            help="Update the DigiD configuration metadata.",
-        )
-        parser.add_argument(
-            "--eherkenning",
-            action="store_true",
-            help="Update the Eherkenning configuration metadata.",
+            "config_model",
+            type=str,
+            choices=["digid", "eherkenning"],
+            help="Update the DigiD or Eherkenning configuration metadata.",
         )
 
     def handle(self, **options):
-        if options["digid"]:
+        if options["config_model"] == "digid":
             config = DigidConfiguration.get_solo()
-        elif options["eherkenning"]:
+        elif options["config_model"] == "eherkenning":
             config = EherkenningConfiguration.get_solo()
-        else:
-            raise CommandError(
-                "A required argument is missing. Please provide either digid or eherkenning."
-            )
 
-        # delete the cache for the urls in order to trigger fetching and parsing xml again
-        if config.metadata_file_source and cache.get(config._meta.object_name):
-            cache.delete(config._meta.object_name)
+        if config.metadata_file_source:
             config.save()
-
             self.stdout.write(self.style.SUCCESS("Update was successful"))
         else:
             self.stdout.write(

digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py

@@ -1,4 +1,4 @@
-# Generated by Django 4.2.6 on 2023-10-12 14:36
+# Generated by Django 4.2.6 on 2023-10-18 11:37
 
 from django.db import migrations, models
 import privates.fields
@@ -40,7 +40,6 @@ class Migration(migrations.Migration):
             field=privates.fields.PrivateMediaFileField(
                 blank=True,
                 help_text="The metadata file of the identity provider. This is auto populated by the retrieved metadata XML file.",
-                null=True,
                 storage=privates.storages.PrivateMediaFileSystemStorage(),
                 upload_to="",
                 verbose_name="identity provider metadata",
@@ -53,7 +52,6 @@ class Migration(migrations.Migration):
                 blank=True,
                 help_text="Example value: 'https://was-preprod1.digid.nl/saml/idp/metadata'. Note that this must match the 'entityID' attribute on the 'md:EntityDescriptor' node found in the Identity Provider's metadata. This is auto populated by the retrieved metadata XML file.",
                 max_length=255,
-                null=True,
                 verbose_name="identity provider service entity ID",
             ),
         ),
@@ -63,7 +61,6 @@ class Migration(migrations.Migration):
             field=privates.fields.PrivateMediaFileField(
                 blank=True,
                 help_text="The metadata file of the identity provider. This is auto populated by the retrieved metadata XML file.",
-                null=True,
                 storage=privates.storages.PrivateMediaFileSystemStorage(),
                 upload_to="",
                 verbose_name="identity provider metadata",
@@ -76,7 +73,6 @@ class Migration(migrations.Migration):
                 blank=True,
                 help_text="Example value: 'https://was-preprod1.digid.nl/saml/idp/metadata'. Note that this must match the 'entityID' attribute on the 'md:EntityDescriptor' node found in the Identity Provider's metadata. This is auto populated by the retrieved metadata XML file.",
                 max_length=255,
-                null=True,
                 verbose_name="identity provider service entity ID",
             ),
         ),

digid_eherkenning/models/base.py

@@ -1,4 +1,3 @@
-from django.core.cache import cache
 from django.core.exceptions import ValidationError
 from django.core.files.base import ContentFile
 from django.db import models
@@ -12,7 +11,6 @@
 from solo.models import SingletonModel
 
 from ..choices import DigestAlgorithms, SignatureAlgorithms, XMLContentTypes
-from ..settings import get_setting
 
 
 class ConfigurationManager(models.Manager):
@@ -35,7 +33,6 @@ class BaseConfiguration(SingletonModel):
     idp_metadata_file = PrivateMediaFileField(
         _("identity provider metadata"),
         blank=True,
-        null=True,
         help_text=_(
             "The metadata file of the identity provider. This is auto populated "
             "by the retrieved metadata XML file."
@@ -45,7 +42,6 @@ class BaseConfiguration(SingletonModel):
         _("identity provider service entity ID"),
         max_length=255,
         blank=True,
-        null=True,
         help_text=_(
             "Example value: 'https://was-preprod1.digid.nl/saml/idp/metadata'. Note "
             "that this must match the 'entityID' attribute on the "
@@ -185,7 +181,7 @@ class Meta:
     def __str__(self):
         return force_str(self._meta.verbose_name)
 
-    def populate_xml_fields(self, urls, xml):
+    def populate_xml_fields(self, urls: dict[str, str], xml: str) -> None:
         """
         Populates the idp_metadata_file and idp_service_entity_id fields based on the
         fetched xml metadata
@@ -194,70 +190,46 @@ def populate_xml_fields(self, urls, xml):
         content = ContentFile(xml.encode("utf-8"))
         self.idp_metadata_file.save("metadata.xml", content, save=False)
 
-    def fetch_xml_metadata(self):
-        """
-        Retrieves the xml metadata from the provided url (metadata_file_source)
-
-        :return a string of the xml metadata.
-        """
-        return OneLogin_Saml2_IdPMetadataParser.get_metadata(self.metadata_file_source)
-
-    def parse_data_from_xml_source(self):
+    def process_metadata_from_xml_source(self) -> tuple[dict[str, str], str]:
         """
         Parses the xml metadata
 
         :return a tuple of a dictionary with the useful urls and the xml string itself.
         """
-        urls = {}
-        xml = ""
-
         try:
-            xml = self.fetch_xml_metadata()
+            xml = OneLogin_Saml2_IdPMetadataParser.get_metadata(
+                self.metadata_file_source
+            )
             parsed_idp_metadata = OneLogin_Saml2_IdPMetadataParser.parse(
                 xml,
                 required_sso_binding=OneLogin_Saml2_Constants.BINDING_HTTP_POST,
                 required_slo_binding=OneLogin_Saml2_Constants.BINDING_HTTP_POST,
             )
-        except Exception:
-            raise ValidationError(
-                _(
-                    "An error has occured while processing the xml file. Make sure the file is valid"
-                )
-            )
-
-        if idp := parsed_idp_metadata.get("idp"):
-            # sometimes the xml file contains urn instead of a url as an entity ID
-            # use the provided url instead
-            urls = {
-                "entityId": (
-                    idp.get("entityId")
-                    if not idp.get("entityId").startswith("urn")
-                    else self.metadata_file_source
-                ),
-                "sso_url": idp.get("singleSignOnService", {}).get("url"),
-                "slo_url": idp.get("singleLogoutService", {}).get("url"),
-            }
-
-            if cache.get(self._meta.object_name):
-                cache.delete(self._meta.object_name)
+        except Exception as exc:
+            raise ValidationError(_(str(exc)))
+
+        if not parsed_idp_metadata.get("idp"):
+            raise ValidationError(_("The provided URL is wrong"))
+
+        idp = parsed_idp_metadata.get("idp")
+        # sometimes the xml file contains urn instead of a url as an entity ID
+        # use the provided url instead
+        urls = {
+            "entityId": (
+                entity_id
+                if not (entity_id := idp.get("entityId")).startswith("urn:")
+                else self.metadata_file_source
+            ),
+            "sso_url": idp.get("singleSignOnService", {}).get("url"),
+            "slo_url": idp.get("singleLogoutService", {}).get("url"),
+        }
 
         return (urls, xml)
 
     def save(self, *args, **kwargs):
         if self.pk and self.metadata_file_source:
-            if (
-                cache.get(self._meta.object_name, {}).get("entityId")
-                != self.metadata_file_source
-            ):
-                urls, xml = self.parse_data_from_xml_source()
-
-                if urls and xml:
-                    self.populate_xml_fields(urls, xml)
-                    cache.set(
-                        self._meta.object_name,
-                        urls,
-                        get_setting("METADATA_URLS_CACHE_TIMEOUT"),
-                    )
+            urls, xml = self.process_metadata_from_xml_source()
+            self.populate_xml_fields(urls, xml)
 
         if self.base_url.endswith("/"):
             self.base_url = self.base_url[:-1]

digid_eherkenning/settings.py

@@ -12,7 +12,6 @@
 # Public settings
 class Defaults:
     DIGID_SESSION_AGE: int = 60 * 15  # 15 minutes, in seconds
-    METADATA_URLS_CACHE_TIMEOUT = 86400
 
 
 def get_setting(name: str):

docs/configuration.rst

@@ -57,7 +57,3 @@ Django settings
   .. note:: This setting is a last resort and it will expire after 15 minutes even if
      there is user activity. Typically you want to define a middleware in your project
      to extend the session duration while there is still activity.
-
-``METADATA_URLS_CACHE_TIMEOUT``
-  The library uses django cache in order to store some useful urls. This prevents reading an XML file
-  if this has not been updated. Defaults to 86400 (1 day).

tests/test_commands.py

@@ -27,7 +27,7 @@ def test_command_triggers_xml_fetching_when_digid(self, get_matadata):
             )
             config.save()
 
-            call_command("update_stored_metadata", "--digid")
+            call_command("update_stored_metadata", "digid")
 
         self.assertTrue(get_matadata.called_once())
 
@@ -46,7 +46,7 @@ def test_command_triggers_xml_fetching_when_eherkenning(self, get_matadata):
             )
             config.save()
 
-            call_command("update_stored_metadata", "--eherkenning", stdout=output)
+            call_command("update_stored_metadata", "eherkenning", stdout=output)
 
         self.assertTrue(get_matadata.called_once())
         self.assertEqual(output.getvalue(), "Update was successful\n")
@@ -59,12 +59,12 @@ def test_command_fails_when_no_argument_provided(self):
 
         self.assertEqual(
             error_message,
-            "A required argument is missing. Please provide either digid or eherkenning.",
+            "Error: the following arguments are required: config_model",
         )
 
     def test_command_fails_when_no_metadata_file_source(self):
         output = StringIO()
-        call_command("update_stored_metadata", "--digid", stdout=output)
+        call_command("update_stored_metadata", "digid", stdout=output)
 
         self.assertEqual(
             output.getvalue(),

tests/test_models.py

@@ -1,13 +1,10 @@
-import datetime
 from unittest.mock import patch
 
-from django.core.cache import cache
 from django.core.exceptions import ValidationError
-from django.test import TestCase, override_settings
+from django.test import TestCase
 from django.utils.translation import gettext as _
 
 import pytest
-from freezegun import freeze_time
 
 from digid_eherkenning.models import DigidConfiguration, EherkenningConfiguration
 
@@ -20,14 +17,6 @@
 
 @pytest.mark.usefixtures("digid_config_defaults", "temp_private_root")
 class BaseModelTests(TestCase):
-    def setUp(self):
-        cache.clear()
-        return super().setUp()
-
-    def tearDown(self):
-        cache.clear()
-        return super().tearDown()
-
     @patch(
         "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata"
     )
@@ -113,38 +102,24 @@ def test_wrong_xml_format_raises_validation_error(self, get_matadata):
 
         with self.assertRaisesMessage(
             ValidationError,
-            _(
-                "An error has occured while processing the xml file. Make sure the file is valid"
-            ),
+            _("Start tag expected, '<' not found, line 1, column 1 (<string>, line 1)"),
         ):
             config.save()
 
-    @override_settings(METADATA_URLS_CACHE_TIMEOUT=2)
+    @patch("onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.parse")
     @patch(
         "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata"
     )
-    def test_urls_caching(self, get_matadata):
+    def test_no_idp_in_xml_raises_validation_error(self, get_matadata, parse):
         config = DigidConfiguration.get_solo()
 
-        with freeze_time("2023-05-22 12:00:00Z") as frozen_datetime:
-            with DIGID_TEST_METADATA_FILE_SLO_POST.open("rb") as metadata_file:
-                metadata_content = metadata_file.read().decode("utf-8")
-                get_matadata.return_value = metadata_content
-                config.metadata_file_source = (
-                    "https://was-preprod1.digid.nl/saml/idp/metadata"
-                )
-                config.save()
-
-            with DIGID_TEST_METADATA_FILE_SLO_POST_2.open("rb") as metadata_file:
-                metadata_content = metadata_file.read().decode("utf-8")
-                get_matadata.return_value = metadata_content
-                config.metadata_file_source = "https://example.com"
-                config.save()
-
-            self.assertEqual(
-                cache.get("DigidConfiguration")["entityId"], "https://example.com"
+        with DIGID_TEST_METADATA_FILE_SLO_POST_2.open("rb") as metadata_file:
+            metadata_content = metadata_file.read().decode("utf-8")
+            get_matadata.return_value = metadata_content
+            config.metadata_file_source = (
+                "https://was-preprod1.digid.nl/saml/idp/metadata"
             )
+            parse.return_value = {"test_no_idp": ""}
 
-            frozen_datetime.tick(delta=datetime.timedelta(seconds=3))
-
-            self.assertIsNone(cache.get("DigidConfiguration"))
+        with self.assertRaisesMessage(ValidationError, _("The provided URL is wrong")):
+            config.save()