From 4a09291a92cf467212962c12942d27585702e986 Mon Sep 17 00:00:00 2001 From: vasileios Date: Mon, 16 Oct 2023 08:40:18 +0200 Subject: [PATCH 1/3] [#45] Automated metadata file retrieval and parsing Refactored base configuration model in order to provide a url and do all the fetching and parsing based on this. The urls are cached for a day and the xml file is updated via a command. --- digid_eherkenning/admin.py | 8 +- .../commands/update_stored_metadata.py | 42 +++++ ...iguration_metadata_file_source_and_more.py | 83 ++++++++++ digid_eherkenning/models/base.py | 100 +++++++++++- digid_eherkenning/settings.py | 1 + .../widgets/clearable_private_file_input.html | 14 ++ docs/cli.rst | 1 + docs/configuration.rst | 6 +- tests/conftest.py | 7 + tests/files/digid/metadata | 113 +++++++++---- tests/files/digid/metadata_with_slo_POST | 85 ++++++++++ tests/files/digid/metadata_with_slo_POST_2 | 85 ++++++++++ tests/test_commands.py | 72 +++++++++ tests/test_models.py | 150 ++++++++++++++++++ 14 files changed, 729 insertions(+), 38 deletions(-) create mode 100644 digid_eherkenning/management/commands/update_stored_metadata.py create mode 100644 digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py create mode 100644 digid_eherkenning/templates/admin/widgets/clearable_private_file_input.html create mode 100644 tests/files/digid/metadata_with_slo_POST create mode 100644 tests/files/digid/metadata_with_slo_POST_2 create mode 100644 tests/test_commands.py create mode 100644 tests/test_models.py diff --git a/digid_eherkenning/admin.py b/digid_eherkenning/admin.py index f8a949c..808e688 100644 --- a/digid_eherkenning/admin.py +++ b/digid_eherkenning/admin.py @@ -9,6 +9,7 @@ @admin.register(DigidConfiguration) class DigidConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin): + readonly_fields = ("idp_service_entity_id",) fieldsets = ( ( _("X.509 Certificate"), @@ -23,8 +24,9 @@ class DigidConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin): _("Identity provider"), { "fields": ( - "idp_metadata_file", + "metadata_file_source", "idp_service_entity_id", + "idp_metadata_file", ), }, ), @@ -72,6 +74,7 @@ class DigidConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin): @admin.register(EherkenningConfiguration) class EherkenningConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin): + readonly_fields = ("idp_service_entity_id",) fieldsets = ( ( _("X.509 Certificate"), @@ -86,8 +89,9 @@ class EherkenningConfigurationAdmin(PrivateMediaMixin, SingletonModelAdmin): _("Identity provider"), { "fields": ( - "idp_metadata_file", + "metadata_file_source", "idp_service_entity_id", + "idp_metadata_file", ), }, ), diff --git a/digid_eherkenning/management/commands/update_stored_metadata.py b/digid_eherkenning/management/commands/update_stored_metadata.py new file mode 100644 index 0000000..2ed816b --- /dev/null +++ b/digid_eherkenning/management/commands/update_stored_metadata.py @@ -0,0 +1,42 @@ +from django.core.cache import cache +from django.core.management import BaseCommand, CommandError + +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." + + 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.", + ) + + def handle(self, **options): + if options["digid"]: + config = DigidConfiguration.get_solo() + elif options["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) + config.save() + + self.stdout.write(self.style.SUCCESS("Update was successful")) + else: + self.stdout.write( + self.style.WARNING("Update failed, no metadata file source found") + ) diff --git a/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py b/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py new file mode 100644 index 0000000..c0c06f2 --- /dev/null +++ b/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py @@ -0,0 +1,83 @@ +# Generated by Django 4.2.6 on 2023-10-12 14:36 + +from django.db import migrations, models +import privates.fields +import privates.storages + + +class Migration(migrations.Migration): + dependencies = [ + ( + "digid_eherkenning", + "0005_alter_eherkenningconfiguration_eh_service_instance_uuid_and_more", + ), + ] + + operations = [ + migrations.AddField( + model_name="digidconfiguration", + name="metadata_file_source", + field=models.URLField( + default="", + help_text="The URL-source where the XML metadata file can be retrieved from.", + max_length=255, + verbose_name="metadata file(XML) URL", + ), + ), + migrations.AddField( + model_name="eherkenningconfiguration", + name="metadata_file_source", + field=models.URLField( + default="", + help_text="The URL-source where the XML metadata file can be retrieved from.", + max_length=255, + verbose_name="metadata file(XML) URL", + ), + ), + migrations.AlterField( + model_name="digidconfiguration", + name="idp_metadata_file", + 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", + ), + ), + migrations.AlterField( + model_name="digidconfiguration", + name="idp_service_entity_id", + field=models.CharField( + 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", + ), + ), + migrations.AlterField( + model_name="eherkenningconfiguration", + name="idp_metadata_file", + 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", + ), + ), + migrations.AlterField( + model_name="eherkenningconfiguration", + name="idp_service_entity_id", + field=models.CharField( + 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", + ), + ), + ] diff --git a/digid_eherkenning/models/base.py b/digid_eherkenning/models/base.py index b24cdaa..3c03f61 100644 --- a/digid_eherkenning/models/base.py +++ b/digid_eherkenning/models/base.py @@ -1,13 +1,18 @@ +from django.core.cache import cache from django.core.exceptions import ValidationError +from django.core.files.base import ContentFile from django.db import models from django.utils.encoding import force_str from django.utils.translation import gettext_lazy as _ +from onelogin.saml2.constants import OneLogin_Saml2_Constants +from onelogin.saml2.idp_metadata_parser import OneLogin_Saml2_IdPMetadataParser from privates.fields import PrivateMediaFileField from simple_certmanager.models import Certificate from solo.models import SingletonModel from ..choices import DigestAlgorithms, SignatureAlgorithms, XMLContentTypes +from ..settings import get_setting class ConfigurationManager(models.Manager): @@ -29,17 +34,31 @@ class BaseConfiguration(SingletonModel): ) idp_metadata_file = PrivateMediaFileField( _("identity provider metadata"), - blank=False, - help_text=_("The metadata file of the identity provider."), + blank=True, + null=True, + help_text=_( + "The metadata file of the identity provider. This is auto populated " + "by the retrieved metadata XML file." + ), ) idp_service_entity_id = models.CharField( _("identity provider service entity ID"), max_length=255, - blank=False, + 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 " - "'md:EntityDescriptor' node found in the Identity Provider's metadata." + "'md:EntityDescriptor' node found in the Identity Provider's metadata. " + "This is auto populated by the retrieved metadata XML file." + ), + ) + metadata_file_source = models.URLField( + _("metadata file(XML) URL"), + max_length=255, + default="", + help_text=_( + "The URL-source where the XML metadata file can be retrieved from." ), ) want_assertions_signed = models.BooleanField( @@ -166,7 +185,80 @@ class Meta: def __str__(self): return force_str(self._meta.verbose_name) + def populate_xml_fields(self, urls, xml): + """ + Populates the idp_metadata_file and idp_service_entity_id fields based on the + fetched xml metadata + """ + self.idp_service_entity_id = urls["entityId"] + 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): + """ + 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() + 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) + + 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"), + ) + if self.base_url.endswith("/"): self.base_url = self.base_url[:-1] super().save(*args, **kwargs) diff --git a/digid_eherkenning/settings.py b/digid_eherkenning/settings.py index 9ec613a..fe944fb 100644 --- a/digid_eherkenning/settings.py +++ b/digid_eherkenning/settings.py @@ -12,6 +12,7 @@ # Public settings class Defaults: DIGID_SESSION_AGE: int = 60 * 15 # 15 minutes, in seconds + METADATA_URLS_CACHE_TIMEOUT = 86400 def get_setting(name: str): diff --git a/digid_eherkenning/templates/admin/widgets/clearable_private_file_input.html b/digid_eherkenning/templates/admin/widgets/clearable_private_file_input.html new file mode 100644 index 0000000..3559aca --- /dev/null +++ b/digid_eherkenning/templates/admin/widgets/clearable_private_file_input.html @@ -0,0 +1,14 @@ + + +{% if widget.is_initial %} + {% if download_allowed %} +

{{ widget.initial_text }}: {{ display_value }} + {% else %} +

{{ widget.initial_text }}: {{ display_value }} + {% endif %} + {% if not widget.required %} + + {% endif %} +
+{% endif %} diff --git a/docs/cli.rst b/docs/cli.rst index 56f2511..6f77f1e 100644 --- a/docs/cli.rst +++ b/docs/cli.rst @@ -9,6 +9,7 @@ django-digid-eherkenning ships with a couple of Django management commands: * ``generate_digid_metadata`` * ``generate_eherkenning_metadata`` * ``generate_eherkenning_dienstcatalogus`` +* ``update_stored_metadata`` For details, call: diff --git a/docs/configuration.rst b/docs/configuration.rst index 4a17790..fd2f82e 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -24,7 +24,7 @@ If this header is not set or empty, we instead get the value from ``REMOTE_ADDR` **Protecting metadata endpoints** -The metdata URLs are open by design to facilitate sharing these URLs with identity +The metadata URLs are open by design to facilitate sharing these URLs with identity providers or other interested parties. Because the metadata is generated on the fly, there is a Denial-of-Service risk. We recommend to protect these URLs at the web-server level by: @@ -57,3 +57,7 @@ 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). diff --git a/tests/conftest.py b/tests/conftest.py index b348246..bce0893 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,6 +12,13 @@ DIGID_TEST_METADATA_FILE = BASE_DIR / "files" / "digid" / "metadata" +DIGID_TEST_METADATA_FILE_SLO_POST = ( + BASE_DIR / "files" / "digid" / "metadata_with_slo_POST" +) +DIGID_TEST_METADATA_FILE_SLO_POST_2 = ( + BASE_DIR / "files" / "digid" / "metadata_with_slo_POST_2" +) + DIGID_TEST_KEY_FILE = BASE_DIR / "files" / "snakeoil-cert" / "ssl-cert-snakeoil.key" DIGID_TEST_CERTIFICATE_FILE = ( BASE_DIR / "files" / "snakeoil-cert" / "ssl-cert-snakeoil.pem" diff --git a/tests/files/digid/metadata b/tests/files/digid/metadata index fb153bc..3b56628 100644 --- a/tests/files/digid/metadata +++ b/tests/files/digid/metadata @@ -1,32 +1,83 @@ -DNT82s99BhXBIvewrpNSnBnuACmZFAKg7Ze+rZmflcQ=gDvJjFd221rI7Y6JT6IFNRr9L1JVQgXiOSx62zfy0Qx7wZTjx1ngs+eOcRloHEdIKBd/BCDQGl10VakEmmzB1OYcuR2V5mq7IdR+lIJb+eoKO1dhc6IK+F2vfWCUxphYUDbfWBE0U06YvSI4di2j0CISMXUbbNiO8DeFWI6/NYuiRimONpRomjwz1X+nR1Aaw58A8hrqiYKMZzMDHL2wJ7haK5ZKv3lWtACpSMYcdNXAzo2le9T0IyZjNUlkGtgHH2UyjDUL1OcZnvMSpd3lHFc6HkUfkbrGmJecNJWZyXT+7BH2HxaYFW0jQEYaTw7vyYatYf0HTNqfcN4VePU7Ww==2e9046aba2e95ed07efb655f6f50880ef686e5312e9046aba2e95ed07efb655f6f50880ef686e531MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV -BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN -BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY -RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ -Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd -8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty -PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP -P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef -aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF -AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm -yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR -B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K -e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 -TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx -QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== -2e9046aba2e95ed07efb655f6f50880ef686e531MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV -BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN -BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY -RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ -Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd -8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty -PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP -P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef -aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF -AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm -yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR -B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K -e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 -TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx -QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== - + + + + + + + + + + + + + + DNT82s99BhXBIvewrpNSnBnuACmZFAKg7Ze+rZmflcQ= + + + + gDvJjFd221rI7Y6JT6IFNRr9L1JVQgXiOSx62zfy0Qx7wZTjx1ngs+eOcRloHEdIKBd/BCDQGl10VakEmmzB1OYcuR2V5mq7IdR+lIJb+eoKO1dhc6IK+F2vfWCUxphYUDbfWBE0U06YvSI4di2j0CISMXUbbNiO8DeFWI6/NYuiRimONpRomjwz1X+nR1Aaw58A8hrqiYKMZzMDHL2wJ7haK5ZKv3lWtACpSMYcdNXAzo2le9T0IyZjNUlkGtgHH2UyjDUL1OcZnvMSpd3lHFc6HkUfkbrGmJecNJWZyXT+7BH2HxaYFW0jQEYaTw7vyYatYf0HTNqfcN4VePU7Ww== + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + + + + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV + BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN + BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY + RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ + Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd + 8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty + PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP + P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef + aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF + AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm + yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR + B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K + e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 + TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx + QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== + + + + + + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV + BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN + BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY + RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ + Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd + 8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty + PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP + P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef + aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF + AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm + yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR + B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K + e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 + TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx + QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== + + + + + + + + + + \ No newline at end of file diff --git a/tests/files/digid/metadata_with_slo_POST b/tests/files/digid/metadata_with_slo_POST new file mode 100644 index 0000000..b72ee90 --- /dev/null +++ b/tests/files/digid/metadata_with_slo_POST @@ -0,0 +1,85 @@ + + + + + + + + + + + + + + + DNT82s99BhXBIvewrpNSnBnuACmZFAKg7Ze+rZmflcQ= + + + + gDvJjFd221rI7Y6JT6IFNRr9L1JVQgXiOSx62zfy0Qx7wZTjx1ngs+eOcRloHEdIKBd/BCDQGl10VakEmmzB1OYcuR2V5mq7IdR+lIJb+eoKO1dhc6IK+F2vfWCUxphYUDbfWBE0U06YvSI4di2j0CISMXUbbNiO8DeFWI6/NYuiRimONpRomjwz1X+nR1Aaw58A8hrqiYKMZzMDHL2wJ7haK5ZKv3lWtACpSMYcdNXAzo2le9T0IyZjNUlkGtgHH2UyjDUL1OcZnvMSpd3lHFc6HkUfkbrGmJecNJWZyXT+7BH2HxaYFW0jQEYaTw7vyYatYf0HTNqfcN4VePU7Ww== + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + + + + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV + BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN + BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY + RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ + Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd + 8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty + PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP + P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef + aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF + AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm + yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR + B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K + e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 + TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx + QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== + + + + + + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV + BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN + BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY + RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ + Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd + 8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty + PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP + P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef + aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF + AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm + yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR + B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K + e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 + TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx + QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== + + + + + + + + + + + \ No newline at end of file diff --git a/tests/files/digid/metadata_with_slo_POST_2 b/tests/files/digid/metadata_with_slo_POST_2 new file mode 100644 index 0000000..b567449 --- /dev/null +++ b/tests/files/digid/metadata_with_slo_POST_2 @@ -0,0 +1,85 @@ + + + + + + + + + + + + + + + DNT82s99BhXBIvewrpNSnBnuACmZFAKg7Ze+rZmflcQ= + + + + gDvJjFd221rI7Y6JT6IFNRr9L1JVQgXiOSx62zfy0Qx7wZTjx1ngs+eOcRloHEdIKBd/BCDQGl10VakEmmzB1OYcuR2V5mq7IdR+lIJb+eoKO1dhc6IK+F2vfWCUxphYUDbfWBE0U06YvSI4di2j0CISMXUbbNiO8DeFWI6/NYuiRimONpRomjwz1X+nR1Aaw58A8hrqiYKMZzMDHL2wJ7haK5ZKv3lWtACpSMYcdNXAzo2le9T0IyZjNUlkGtgHH2UyjDUL1OcZnvMSpd3lHFc6HkUfkbrGmJecNJWZyXT+7BH2HxaYFW0jQEYaTw7vyYatYf0HTNqfcN4VePU7Ww== + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + + + + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV + BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN + BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY + RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ + Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd + 8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty + PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP + P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef + aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF + AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm + yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR + B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K + e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 + TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx + QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== + + + + + + + 2e9046aba2e95ed07efb655f6f50880ef686e531 + + MIICsjCCAZqgAwIBAgIJAJhvbn1Aal/aMA0GCSqGSIb3DQEBCwUAMBExDzANBgNV + BAMMBnVidW50dTAeFw0xNzEyMjkxNjI3NDhaFw0yNzEyMjcxNjI3NDhaMBExDzAN + BgNVBAMMBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAObY + RaZaguRBRowjJraDM8A7VpcWi+aIzYc0003gmz1j+/31pQ7QX12cCFAnyo+6bFlQ + Or8/Qda24yIAsaaj96Z3iA4bWDUARLUD8N6gftI8sZwf1JRW6kgwYn3DiLdhDeKd + 8LsrraUzHK/CXccCt5xM6t7gsgBoJvMNmy2ddpuh6T5xLQpREcI9JVqAihRa33ty + PMDk4QzPYjKsshYfFqlpRz6Zb3SMUnqjhRlWKyF/QgviNdrK8w1xlx6ix/oVruOP + P4qSl5vOmGk49CRqWY7IWciD2VHcH04fVfjhwPsh4Dv7uTaMLHwyGvJ8XFjK6yef + aiSODBsnb+bqKe8jWssCAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQsF + AAOCAQEAWOjsjh9NxJ5Lu5bGsNQecxheCtkv6WdTB4O5+z60qVNxQZrwQkPbnTQm + yAfTxR/VubLGqJ3mYU5YsqKJ2Z9Cqvc3wIPXLkruv5A1RwDv0eb0R/UE5nF/nKQR + B3yn9AYVrzu5kNlpFr/hK6brqfznMKPT6K4n9jMdUg9Bek9b2j8vG7L/Jgona43K + e7Ip6lACOP0q5PQvrKIkkdfh5QbZjrCQnUVRsScDh/qjWS01rYUp9z7rfuEuQNt5 + TPNMhvmH+podlP9zgONDdwYsjdP59+pFIRebf3Qlkd2nNk6AfL4oFpacr8x7EuHx + QJZ5MqEauEhsXVSP+gFGywjSsYHMHg== + + + + + + + + + + + \ No newline at end of file diff --git a/tests/test_commands.py b/tests/test_commands.py new file mode 100644 index 0000000..6f4b235 --- /dev/null +++ b/tests/test_commands.py @@ -0,0 +1,72 @@ +from io import StringIO +from unittest.mock import patch + +from django.core.management import CommandError, call_command +from django.test import TestCase + +import pytest + +from digid_eherkenning.models import DigidConfiguration, EherkenningConfiguration + +from .conftest import DIGID_TEST_METADATA_FILE_SLO_POST, EHERKENNING_TEST_METADATA_FILE + + +@pytest.mark.usefixtures("digid_config_defaults", "temp_private_root") +class UpdateStoredMetadataTests(TestCase): + @patch( + "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" + ) + def test_command_triggers_xml_fetching_when_digid(self, get_matadata): + config = DigidConfiguration.get_solo() + + 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() + + call_command("update_stored_metadata", "--digid") + + self.assertTrue(get_matadata.called_once()) + + @patch( + "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" + ) + def test_command_triggers_xml_fetching_when_eherkenning(self, get_matadata): + output = StringIO() + config = EherkenningConfiguration.get_solo() + + with EHERKENNING_TEST_METADATA_FILE.open("rb") as metadata_file: + metadata_content = metadata_file.read().decode("utf-8") + get_matadata.return_value = metadata_content + config.metadata_file_source = ( + "https://eh01.staging.iwelcome.nl/broker/sso/1.13" + ) + config.save() + + call_command("update_stored_metadata", "--eherkenning", stdout=output) + + self.assertTrue(get_matadata.called_once()) + self.assertEqual(output.getvalue(), "Update was successful\n") + + def test_command_fails_when_no_argument_provided(self): + try: + call_command("update_stored_metadata") + except CommandError as e: + error_message = str(e) + + self.assertEqual( + error_message, + "A required argument is missing. Please provide either digid or eherkenning.", + ) + + def test_command_fails_when_no_metadata_file_source(self): + output = StringIO() + call_command("update_stored_metadata", "--digid", stdout=output) + + self.assertEqual( + output.getvalue(), + "Update failed, no metadata file source found\n", + ) diff --git a/tests/test_models.py b/tests/test_models.py new file mode 100644 index 0000000..57458b6 --- /dev/null +++ b/tests/test_models.py @@ -0,0 +1,150 @@ +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.utils.translation import gettext as _ + +import pytest +from freezegun import freeze_time + +from digid_eherkenning.models import DigidConfiguration, EherkenningConfiguration + +from .conftest import ( + DIGID_TEST_METADATA_FILE_SLO_POST, + DIGID_TEST_METADATA_FILE_SLO_POST_2, + EHERKENNING_TEST_METADATA_FILE, +) + + +@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" + ) + def test_fields_are_populated_on_digid_save(self, get_matadata): + config = DigidConfiguration.get_solo() + + 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/with-slo" + ) + config.save() + + self.assertTrue(get_matadata.called_once()) + self.assertEqual( + config.idp_metadata_file.read().decode("utf-8"), metadata_content + ) + self.assertEqual( + config.idp_service_entity_id, + "https://was-preprod1.digid.nl/saml/idp/metadata/with-slo", + ) + + @patch( + "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" + ) + def test_fields_are_populated_on_eherkennig_save(self, get_matadata): + config = EherkenningConfiguration.get_solo() + + with EHERKENNING_TEST_METADATA_FILE.open("rb") as metadata_file: + metadata_content = metadata_file.read().decode("utf-8") + get_matadata.return_value = metadata_content + config.metadata_file_source = ( + "https://eh01.staging.iwelcome.nl/broker/sso/1.13" + ) + config.save() + + self.assertTrue(get_matadata.called_once()) + self.assertEqual( + config.idp_metadata_file.read().decode("utf-8"), metadata_content + ) + self.assertEqual( + config.idp_service_entity_id, + "https://eh01.staging.iwelcome.nl/broker/sso/1.13", + ) + + @patch( + "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" + ) + def test_no_fetching_xml_when_no_file_source_change(self, get_matadata): + config = DigidConfiguration.get_solo() + + 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() + + config.organization_name = "test" + config.save() + + # Make sure we don't try to fetch and parse again the xml file, since there is no update + self.assertTrue(get_matadata.called_once()) + + @patch( + "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" + ) + def test_wrong_xml_format_raises_validation_error(self, get_matadata): + config = DigidConfiguration.get_solo() + + 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() + + get_matadata.return_value = "wrong xml format" + config.metadata_file_source = "https://example.com" + + with self.assertRaisesMessage( + ValidationError, + _( + "An error has occured while processing the xml file. Make sure the file is valid" + ), + ): + config.save() + + @override_settings(METADATA_URLS_CACHE_TIMEOUT=2) + @patch( + "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" + ) + def test_urls_caching(self, get_matadata): + 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" + ) + + frozen_datetime.tick(delta=datetime.timedelta(seconds=3)) + + self.assertIsNone(cache.get("DigidConfiguration")) From 5c04869cd2d1ab38f4e2c2dd1062ae22430dfb8e Mon Sep 17 00:00:00 2001 From: vasileios Date: Wed, 18 Oct 2023 16:31:09 +0200 Subject: [PATCH 2/3] [#45] PR feedback process --- digid_eherkenning/admin.py | 13 +++- .../commands/update_stored_metadata.py | 30 +++---- ...iguration_metadata_file_source_and_more.py | 6 +- digid_eherkenning/models/base.py | 78 ++++++------------- digid_eherkenning/settings.py | 1 - .../widgets/custom_file_input.html} | 0 docs/configuration.rst | 4 - tests/test_commands.py | 8 +- tests/test_models.py | 49 +++--------- 9 files changed, 62 insertions(+), 127 deletions(-) rename digid_eherkenning/templates/admin/{widgets/clearable_private_file_input.html => digid_eherkenning/widgets/custom_file_input.html} (100%) diff --git a/digid_eherkenning/admin.py b/digid_eherkenning/admin.py index 808e688..0270cf5 100644 --- a/digid_eherkenning/admin.py +++ b/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 = ( ( diff --git a/digid_eherkenning/management/commands/update_stored_metadata.py b/digid_eherkenning/management/commands/update_stored_metadata.py index 2ed816b..5df4643 100644 --- a/digid_eherkenning/management/commands/update_stored_metadata.py +++ b/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( diff --git a/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py b/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py index c0c06f2..d88f540 100644 --- a/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py +++ b/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", ), ), diff --git a/digid_eherkenning/models/base.py b/digid_eherkenning/models/base.py index 3c03f61..494a9c4 100644 --- a/digid_eherkenning/models/base.py +++ b/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] diff --git a/digid_eherkenning/settings.py b/digid_eherkenning/settings.py index fe944fb..9ec613a 100644 --- a/digid_eherkenning/settings.py +++ b/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): diff --git a/digid_eherkenning/templates/admin/widgets/clearable_private_file_input.html b/digid_eherkenning/templates/admin/digid_eherkenning/widgets/custom_file_input.html similarity index 100% rename from digid_eherkenning/templates/admin/widgets/clearable_private_file_input.html rename to digid_eherkenning/templates/admin/digid_eherkenning/widgets/custom_file_input.html diff --git a/docs/configuration.rst b/docs/configuration.rst index fd2f82e..095d73f 100644 --- a/docs/configuration.rst +++ b/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). diff --git a/tests/test_commands.py b/tests/test_commands.py index 6f4b235..d29d2f0 100644 --- a/tests/test_commands.py +++ b/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(), diff --git a/tests/test_models.py b/tests/test_models.py index 57458b6..120a3ce 100644 --- a/tests/test_models.py +++ b/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 (, 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() From bca768a8b4858926e341337aea45f71533b7802b Mon Sep 17 00:00:00 2001 From: vasileios Date: Fri, 20 Oct 2023 13:06:46 +0200 Subject: [PATCH 3/3] [#45] Second PR feedback process --- ...iguration_metadata_file_source_and_more.py | 10 ++++---- digid_eherkenning/models/base.py | 22 ++++++++++------- tests/test_commands.py | 24 +++++++++++++------ tests/test_models.py | 11 +++++++-- 4 files changed, 45 insertions(+), 22 deletions(-) diff --git a/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py b/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py index d88f540..411f548 100644 --- a/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py +++ b/digid_eherkenning/migrations/0006_digidconfiguration_metadata_file_source_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.6 on 2023-10-18 11:37 +# Generated by Django 4.2.6 on 2023-10-20 08:40 from django.db import migrations, models import privates.fields @@ -39,7 +39,7 @@ class Migration(migrations.Migration): name="idp_metadata_file", 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.", + help_text="The metadata file of the identity provider. This is auto populated from the configured source URL.", storage=privates.storages.PrivateMediaFileSystemStorage(), upload_to="", verbose_name="identity provider metadata", @@ -50,7 +50,7 @@ class Migration(migrations.Migration): name="idp_service_entity_id", field=models.CharField( 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.", + 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 from the configured source URL.", max_length=255, verbose_name="identity provider service entity ID", ), @@ -60,7 +60,7 @@ class Migration(migrations.Migration): name="idp_metadata_file", 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.", + help_text="The metadata file of the identity provider. This is auto populated from the configured source URL.", storage=privates.storages.PrivateMediaFileSystemStorage(), upload_to="", verbose_name="identity provider metadata", @@ -71,7 +71,7 @@ class Migration(migrations.Migration): name="idp_service_entity_id", field=models.CharField( 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.", + 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 from the configured source URL.", max_length=255, verbose_name="identity provider service entity ID", ), diff --git a/digid_eherkenning/models/base.py b/digid_eherkenning/models/base.py index 494a9c4..2a31ec1 100644 --- a/digid_eherkenning/models/base.py +++ b/digid_eherkenning/models/base.py @@ -35,7 +35,7 @@ class BaseConfiguration(SingletonModel): blank=True, help_text=_( "The metadata file of the identity provider. This is auto populated " - "by the retrieved metadata XML file." + "from the configured source URL." ), ) idp_service_entity_id = models.CharField( @@ -46,7 +46,7 @@ class BaseConfiguration(SingletonModel): "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." + "This is auto populated from the configured source URL." ), ) metadata_file_source = models.URLField( @@ -205,13 +205,19 @@ def process_metadata_from_xml_source(self) -> tuple[dict[str, str], str]: required_sso_binding=OneLogin_Saml2_Constants.BINDING_HTTP_POST, required_slo_binding=OneLogin_Saml2_Constants.BINDING_HTTP_POST, ) + # python3-saml library does not use proper-namespaced exceptions except Exception as exc: - raise ValidationError(_(str(exc))) - - if not parsed_idp_metadata.get("idp"): - raise ValidationError(_("The provided URL is wrong")) + raise ValidationError( + _("Failed to parse the metadata, got error: {err}").format(err=str(exc)) + ) from exc + + if not (idp := parsed_idp_metadata.get("idp")): + raise ValidationError( + _( + "Could not find any identity provider information in the metadata at the provided URL." + ) + ) - 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 = { @@ -227,7 +233,7 @@ def process_metadata_from_xml_source(self) -> tuple[dict[str, str], str]: return (urls, xml) def save(self, *args, **kwargs): - if self.pk and self.metadata_file_source: + if self.metadata_file_source: urls, xml = self.process_metadata_from_xml_source() self.populate_xml_fields(urls, xml) diff --git a/tests/test_commands.py b/tests/test_commands.py index d29d2f0..144949d 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -16,39 +16,49 @@ class UpdateStoredMetadataTests(TestCase): @patch( "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" ) - def test_command_triggers_xml_fetching_when_digid(self, get_matadata): + def test_command_triggers_xml_fetching_when_digid(self, get_metadata): + output = StringIO() config = DigidConfiguration.get_solo() 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 + get_metadata.return_value = metadata_content config.metadata_file_source = ( "https://was-preprod1.digid.nl/saml/idp/metadata" ) config.save() - call_command("update_stored_metadata", "digid") + self.assertEqual(get_metadata.call_count, 1) + + get_metadata.reset_mock() + + call_command("update_stored_metadata", "digid", stdout=output) - self.assertTrue(get_matadata.called_once()) + self.assertEqual(get_metadata.call_count, 1) + self.assertEqual(output.getvalue(), "Update was successful\n") @patch( "onelogin.saml2.idp_metadata_parser.OneLogin_Saml2_IdPMetadataParser.get_metadata" ) - def test_command_triggers_xml_fetching_when_eherkenning(self, get_matadata): + def test_command_triggers_xml_fetching_when_eherkenning(self, get_metadata): output = StringIO() config = EherkenningConfiguration.get_solo() with EHERKENNING_TEST_METADATA_FILE.open("rb") as metadata_file: metadata_content = metadata_file.read().decode("utf-8") - get_matadata.return_value = metadata_content + get_metadata.return_value = metadata_content config.metadata_file_source = ( "https://eh01.staging.iwelcome.nl/broker/sso/1.13" ) config.save() + self.assertEqual(get_metadata.call_count, 1) + + get_metadata.reset_mock() + call_command("update_stored_metadata", "eherkenning", stdout=output) - self.assertTrue(get_matadata.called_once()) + self.assertEqual(get_metadata.call_count, 1) self.assertEqual(output.getvalue(), "Update was successful\n") def test_command_fails_when_no_argument_provided(self): diff --git a/tests/test_models.py b/tests/test_models.py index 120a3ce..b5c3ed2 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -102,7 +102,9 @@ def test_wrong_xml_format_raises_validation_error(self, get_matadata): with self.assertRaisesMessage( ValidationError, - _("Start tag expected, '<' not found, line 1, column 1 (, line 1)"), + _( + "Failed to parse the metadata, got error: Start tag expected, '<' not found, line 1, column 1" + ), ): config.save() @@ -121,5 +123,10 @@ def test_no_idp_in_xml_raises_validation_error(self, get_matadata, parse): ) parse.return_value = {"test_no_idp": ""} - with self.assertRaisesMessage(ValidationError, _("The provided URL is wrong")): + with self.assertRaisesMessage( + ValidationError, + _( + "Could not find any identity provider information in the metadata at the provided URL." + ), + ): config.save()