From 5c04869cd2d1ab38f4e2c2dd1062ae22430dfb8e Mon Sep 17 00:00:00 2001 From: vasileios Date: Wed, 18 Oct 2023 16:31:09 +0200 Subject: [PATCH] [#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()