From 15a8671f0dbb46bb45030ca4f5e9f21bda38bfb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Jes=C3=BAs=20Pe=C3=B1a=20Rodr=C3=ADguez?= Date: Wed, 9 Jul 2025 09:50:22 +0200 Subject: [PATCH] feat(saml): prevent duplicate SAML entityID configuration (#8224) --- api/src/backend/api/models.py | 33 +++++++++++--- api/src/backend/api/tests/test_models.py | 58 ++++++++++++++++-------- 2 files changed, 65 insertions(+), 26 deletions(-) diff --git a/api/src/backend/api/models.py b/api/src/backend/api/models.py index e22ccbab7f..0d486afed3 100644 --- a/api/src/backend/api/models.py +++ b/api/src/backend/api/models.py @@ -1483,7 +1483,7 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): ), ] - def clean(self, old_email_domain=None): + def clean(self, old_email_domain=None, is_create=False): # Domain must not contain @ if "@" in self.email_domain: raise ValidationError({"email_domain": "Domain must not contain @"}) @@ -1507,6 +1507,25 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): {"tenant": "There is a problem with your email domain."} ) + # The entityID must be unique in the system + idp_settings = self._parsed_metadata + entity_id = idp_settings.get("entity_id") + + if entity_id: + # Find any SocialApp with this entityID + q = SocialApp.objects.filter(provider="saml", provider_id=entity_id) + + # If updating, exclude our own SocialApp from the check + if not is_create: + q = q.exclude(client_id=old_email_domain) + else: + q = q.exclude(client_id=self.email_domain) + + if q.exists(): + raise ValidationError( + {"metadata_xml": "There is a problem with your metadata."} + ) + def save(self, *args, **kwargs): self.email_domain = self.email_domain.strip().lower() is_create = not SAMLConfiguration.objects.filter(pk=self.pk).exists() @@ -1519,7 +1538,8 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): old_email_domain = None old_metadata_xml = None - self.clean(old_email_domain) + self._parsed_metadata = self._parse_metadata() + self.clean(old_email_domain, is_create) super().save(*args, **kwargs) if is_create or ( @@ -1562,6 +1582,8 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): # Entity ID entity_id = root.attrib.get("entityID") + if not entity_id: + raise ValidationError({"metadata_xml": "Missing entityID in metadata."}) # SSO endpoint (must exist) sso = root.find(".//md:IDPSSODescriptor/md:SingleSignOnService", ns) @@ -1600,9 +1622,8 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): Create or update the corresponding SocialApp based on email_domain. If the domain changed, update the matching SocialApp. """ - idp_settings = self._parse_metadata() settings_dict = SOCIALACCOUNT_PROVIDERS["saml"].copy() - settings_dict["idp"] = idp_settings + settings_dict["idp"] = self._parsed_metadata current_site = Site.objects.get(id=settings.SITE_ID) @@ -1618,7 +1639,7 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): social_app.client_id = client_id social_app.name = name social_app.settings = settings_dict - social_app.provider_id = idp_settings["entity_id"] + social_app.provider_id = self._parsed_metadata["entity_id"] social_app.save() social_app.sites.set([current_site]) else: @@ -1627,7 +1648,7 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): client_id=client_id, name=name, settings=settings_dict, - provider_id=idp_settings["entity_id"], + provider_id=self._parsed_metadata["entity_id"], ) social_app.sites.set([current_site]) diff --git a/api/src/backend/api/tests/test_models.py b/api/src/backend/api/tests/test_models.py index 82ad2e880f..79d405d4a7 100644 --- a/api/src/backend/api/tests/test_models.py +++ b/api/src/backend/api/tests/test_models.py @@ -3,7 +3,7 @@ from allauth.socialaccount.models import SocialApp from django.core.exceptions import ValidationError from api.db_router import MainRouter -from api.models import Resource, ResourceTag, SAMLConfiguration, SAMLDomainIndex, Tenant +from api.models import Resource, ResourceTag, SAMLConfiguration, SAMLDomainIndex @pytest.mark.django_db @@ -142,8 +142,8 @@ class TestSAMLConfigurationModel: """ - def test_creates_valid_configuration(self): - tenant = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant A") + def test_creates_valid_configuration(self, tenants_fixture): + tenant = tenants_fixture[0] config = SAMLConfiguration.objects.using(MainRouter.admin_db).create( email_domain="ssoexample.com", metadata_xml=TestSAMLConfigurationModel.VALID_METADATA, @@ -153,8 +153,8 @@ class TestSAMLConfigurationModel: assert config.email_domain == "ssoexample.com" assert SocialApp.objects.filter(client_id="ssoexample.com").exists() - def test_email_domain_with_at_symbol_fails(self): - tenant = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant B") + def test_email_domain_with_at_symbol_fails(self, tenants_fixture): + tenant = tenants_fixture[0] config = SAMLConfiguration( email_domain="invalid@domain.com", metadata_xml=TestSAMLConfigurationModel.VALID_METADATA, @@ -168,9 +168,8 @@ class TestSAMLConfigurationModel: assert "email_domain" in errors assert "Domain must not contain @" in errors["email_domain"][0] - def test_duplicate_email_domain_fails(self): - tenant1 = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant C1") - tenant2 = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant C2") + def test_duplicate_email_domain_fails(self, tenants_fixture): + tenant1, tenant2, *_ = tenants_fixture SAMLConfiguration.objects.using(MainRouter.admin_db).create( email_domain="duplicate.com", @@ -191,8 +190,8 @@ class TestSAMLConfigurationModel: assert "tenant" in errors assert "There is a problem with your email domain." in errors["tenant"][0] - def test_duplicate_tenant_config_fails(self): - tenant = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant D") + def test_duplicate_tenant_config_fails(self, tenants_fixture): + tenant = tenants_fixture[0] SAMLConfiguration.objects.using(MainRouter.admin_db).create( email_domain="unique1.com", @@ -216,8 +215,8 @@ class TestSAMLConfigurationModel: in errors["tenant"][0] ) - def test_invalid_metadata_xml_fails(self): - tenant = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant E") + def test_invalid_metadata_xml_fails(self, tenants_fixture): + tenant = tenants_fixture[0] config = SAMLConfiguration( email_domain="brokenxml.com", metadata_xml="", @@ -232,8 +231,8 @@ class TestSAMLConfigurationModel: assert "Invalid XML" in errors["metadata_xml"][0] assert "not well-formed" in errors["metadata_xml"][0] - def test_metadata_missing_sso_fails(self): - tenant = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant F") + def test_metadata_missing_sso_fails(self, tenants_fixture): + tenant = tenants_fixture[0] xml = """ """ @@ -250,8 +249,8 @@ class TestSAMLConfigurationModel: assert "metadata_xml" in errors assert "Missing SingleSignOnService" in errors["metadata_xml"][0] - def test_metadata_missing_certificate_fails(self): - tenant = Tenant.objects.using(MainRouter.admin_db).create(name="Tenant G") + def test_metadata_missing_certificate_fails(self, tenants_fixture): + tenant = tenants_fixture[0] xml = """ @@ -270,10 +269,8 @@ class TestSAMLConfigurationModel: assert "metadata_xml" in errors assert "X509Certificate" in errors["metadata_xml"][0] - def test_deletes_saml_configuration_and_related_objects(self): - tenant = Tenant.objects.using(MainRouter.admin_db).create( - name="Tenant for Deletion" - ) + def test_deletes_saml_configuration_and_related_objects(self, tenants_fixture): + tenant = tenants_fixture[0] email_domain = "deleteme.com" # Create the configuration @@ -306,3 +303,24 @@ class TestSAMLConfigurationModel: .filter(email_domain=email_domain) .exists() ) + + def test_duplicate_entity_id_fails_on_creation(self, tenants_fixture): + tenant1, tenant2, *_ = tenants_fixture + SAMLConfiguration.objects.using(MainRouter.admin_db).create( + email_domain="first.com", + metadata_xml=self.VALID_METADATA, + tenant=tenant1, + ) + + config = SAMLConfiguration( + email_domain="second.com", + metadata_xml=self.VALID_METADATA, + tenant=tenant2, + ) + + with pytest.raises(ValidationError) as exc_info: + config.save() + + errors = exc_info.value.message_dict + assert "metadata_xml" in errors + assert "There is a problem with your metadata." in errors["metadata_xml"][0]