From 9b1e8dc515ebc2836b303a6924fb64598d76cb63 Mon Sep 17 00:00:00 2001 From: Alexander Sheehan Date: Thu, 21 Jul 2022 13:27:26 -0400 Subject: [PATCH] chore: rolling back unique entity ID validation on saml provider configs --- common/djangoapps/third_party_auth/models.py | 7 +++-- .../samlproviderconfig/serializers.py | 8 ++++- .../tests/test_samlproviderconfig.py | 20 ++++++++----- .../third_party_auth/tests/test_models.py | 29 ++++++++++--------- 4 files changed, 39 insertions(+), 25 deletions(-) diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 65109a4928..70679c7feb 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -12,7 +12,7 @@ from config_models.models import ConfigurationModel, cache from django.conf import settings from django.contrib.sites.models import Site from django.core.exceptions import ValidationError -from django.db import models, IntegrityError +from django.db import models from django.utils import timezone from django.utils.translation import gettext_lazy as _ from organizations.models import Organization @@ -746,7 +746,10 @@ class SAMLProviderConfig(ProviderConfig): # If any exist, raise an integrity error if existing_provider_configs: exc_str = f'Entity ID: {self.entity_id} already in use' - raise IntegrityError(exc_str) + # There are cases of preexisting configurations that share entity id's so we can't blow up if we + # encounter this issue. Instead just log for clarity. + # raise IntegrityError(exc_str) + log.warning(exc_str) super().save(*args, **kwargs) def get_url_params(self): diff --git a/common/djangoapps/third_party_auth/samlproviderconfig/serializers.py b/common/djangoapps/third_party_auth/samlproviderconfig/serializers.py index d9d30e37d0..4a01d0679c 100644 --- a/common/djangoapps/third_party_auth/samlproviderconfig/serializers.py +++ b/common/djangoapps/third_party_auth/samlproviderconfig/serializers.py @@ -2,10 +2,13 @@ Serializer for SAMLProviderConfig """ +import logging from rest_framework import serializers from common.djangoapps.third_party_auth.models import SAMLProviderConfig, SAMLConfiguration +log = logging.getLogger(__name__) + class SAMLProviderConfigSerializer(serializers.ModelSerializer): # lint-amnesty, pylint: disable=missing-class-docstring saml_config_id = serializers.IntegerField(required=False) @@ -27,7 +30,10 @@ class SAMLProviderConfigSerializer(serializers.ModelSerializer): # lint-amnesty entity_id=data['entity_id'], archived=False, ).exclude(slug=data['slug']): - raise serializers.ValidationError(f"Entity ID: {data['entity_id']} already taken") + # There are cases of preexisting configurations that share entity id's so we can't blow up if we + # encounter this issue. Instead just log for clarity. + # raise serializers.ValidationError(f"Entity ID: {data['entity_id']} already taken") + log.warning(f"Entity ID: {data['entity_id']} already taken") return data def create(self, validated_data): diff --git a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py index 4fc8947e49..c8e8487b65 100644 --- a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py +++ b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py @@ -265,16 +265,20 @@ class SAMLProviderConfigTests(APITestCase): Test that a config cannot be created with an entity ID if another config already exists with that entity ID and a different slug """ - url = reverse('saml_provider_config-list') - data = copy.copy(SINGLE_PROVIDER_CONFIG) - data['enterprise_customer_uuid'] = ENTERPRISE_ID - data['slug'] = 'some-other-slug' + with self.assertLogs() as ctx: + url = reverse('saml_provider_config-list') + data = copy.copy(SINGLE_PROVIDER_CONFIG) + data['enterprise_customer_uuid'] = ENTERPRISE_ID + data['slug'] = 'some-other-slug' - response = self.client.post(url, data) + response = self.client.post(url, data) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert len(SAMLProviderConfig.objects.all()) == 1 - assert str(response.data.get('non_field_errors')[0]) == f"Entity ID: {data['entity_id']} already taken" + # 7/21/22 : Disabling the exception on duplicate entity ID's because of existing data. + assert ctx.records[-2].msg == f"Entity ID: {data['entity_id']} already taken" + assert response.status_code == status.HTTP_201_CREATED + # assert response.status_code == status.HTTP_400_BAD_REQUEST + # assert len(SAMLProviderConfig.objects.all()) == 1 + # assert str(response.data.get('non_field_errors')[0]) == f"Entity ID: {data['entity_id']} already taken" def test_unique_entity_id_constraint_with_same_slug(self): """ diff --git a/common/djangoapps/third_party_auth/tests/test_models.py b/common/djangoapps/third_party_auth/tests/test_models.py index 4d9c1c946e..b844449699 100644 --- a/common/djangoapps/third_party_auth/tests/test_models.py +++ b/common/djangoapps/third_party_auth/tests/test_models.py @@ -1,15 +1,14 @@ """ Tests for third_party_auth/models.py. """ -import pytest +import unittest from django.test import TestCase -from django.db.utils import IntegrityError from .factories import SAMLProviderConfigFactory from ..models import SAMLProviderConfig -class TestSamlProviderConfigModel(TestCase): +class TestSamlProviderConfigModel(TestCase, unittest.TestCase): """ Test model operations for the saml provider config model. """ @@ -22,19 +21,21 @@ class TestSamlProviderConfigModel(TestCase): """ Test that the unique entity ID enforcement does not apply to noncurrent configs """ - assert len(SAMLProviderConfig.objects.all()) == 1 - old_entity_id = self.saml_provider_config.entity_id - self.saml_provider_config.entity_id = f'{self.saml_provider_config.entity_id}-ayylmao' - self.saml_provider_config.save() + with self.assertLogs() as ctx: + assert len(SAMLProviderConfig.objects.all()) == 1 + old_entity_id = self.saml_provider_config.entity_id + self.saml_provider_config.entity_id = f'{self.saml_provider_config.entity_id}-ayylmao' + self.saml_provider_config.save() - # check that we now have two records, one non-current - assert len(SAMLProviderConfig.objects.all()) == 2 - assert len(SAMLProviderConfig.objects.current_set()) == 1 + # check that we now have two records, one non-current + assert len(SAMLProviderConfig.objects.all()) == 2 + assert len(SAMLProviderConfig.objects.current_set()) == 1 - # Make sure we can use that old entity id - SAMLProviderConfigFactory(entity_id=old_entity_id) + # Make sure we can use that old entity id + SAMLProviderConfigFactory(entity_id=old_entity_id) - # Now if we try and create a new model using a current entity ID then it should throw the integrity error - with pytest.raises(IntegrityError): + # 7/21/22 : Disabling the exception on duplicate entity ID's because of existing data. + # with pytest.raises(IntegrityError): bad_config = SAMLProviderConfig(entity_id=self.saml_provider_config.entity_id) bad_config.save() + assert ctx.records[0].msg == f'Entity ID: {self.saml_provider_config.entity_id} already in use'