fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig #36874

ktyagiapphelix2u · 2025-06-09T07:01:32Z

This PR implements and documents the dynamic lookup behavior of SAML configurations in the edX platform. When a SAMLProviderConfig is created, it references a specific SAMLConfiguration object, but the system will automatically retrieve the newest enabled configuration with the same slug when authenticating users, rather than using the directly referenced one. This maintains configuration versioning history while ensuring consistent authentication experiences.

I've added comprehensive tests that demonstrate how updates to SAML configurations create new records rather than updating in-place, how the system finds and uses the newest enabled configuration with matching slugs, and how fallback mechanisms operate when configurations are disabled or missing. This improves reliability by allowing administrators to roll out configuration updates without disrupting authentication flows.

The implementation includes a robust fallback mechanism that handles cases where no enabled configurations are available by using the most recently created configuration with the matching slug, ensuring authentication services remain operational even during configuration transitions or when configurations are temporarily disabled.

This PR is related to issue Have SAMLProviderConfig point to the most current version of SAMLConfiguration
#334 which requested improvements to how SAML configurations are managed across configuration updates.

common/djangoapps/third_party_auth/models.py

UsamaSadiq · 2025-06-11T11:04:40Z

requirements/edx/base.in

@@ -1,2 +1,3 @@
+edx-toggles


edx-toggles is already part of kernel.in so we don't need to explicitly add it here.

@UsamaSadiq: Would you mind resolving your conversations that are no longer needed, or responding if you have more feedback? Thank you.

UsamaSadiq

Robert can add further thoughts on the rollout process with utilizing logs to identify if everything is working as expected before actually switching it on but rest of the code looks okay.

UsamaSadiq · 2025-06-11T11:05:47Z

common/djangoapps/third_party_auth/tests/test_models.py

+        - The system automatically retrieves the newest version with matching slug
+        - Even when all configs with matching slug are disabled, the system still uses the newest one
+        """
+        from django.utils import timezone


The imports should go to the top of the test file along with other imports.

UsamaSadiq · 2025-06-11T11:06:52Z

common/djangoapps/third_party_auth/tests/test_models.py

+        from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory
+        import mock
+
+        # ===== STEP 1: Setup initial configuration =====


We don't need to add such detailed comments. Having verbose code is enough. If code gets ambiguous, then it's fine to add comment to highlight something.

See https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29#comments-rules for more tips around commenting. Try to determine which comments are just restating what the code is already stating, and which are explaining intent or other necessary notes. This is an art, and not a science, so there is no perfect answer. I think we can possibly pair and work through this with others to discuss in more detail. Thank you.

I'm going to wait to review tests further until the other changes have come in.

common/djangoapps/third_party_auth/models.py

robrap · 2025-06-12T00:56:50Z

common/djangoapps/third_party_auth/tests/test_models.py

+        from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory
+        import mock
+
+        # ===== STEP 1: Setup initial configuration =====


See https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29#comments-rules for more tips around commenting. Try to determine which comments are just restating what the code is already stating, and which are explaining intent or other necessary notes. This is an art, and not a science, so there is no perfect answer. I think we can possibly pair and work through this with others to discuss in more detail. Thank you.

I'm going to wait to review tests further until the other changes have come in.

ktyagiapphelix2u · 2025-06-12T07:20:59Z

@UsamaSadiq @robrap I've updated the PR with the refactor based on your suggestions. The changes have been pushed — could you please take a look and let me know if everything looks good now?

Thanks

common/djangoapps/third_party_auth/models.py

UsamaSadiq · 2025-06-12T10:13:25Z

common/djangoapps/third_party_auth/models.py

+                except Exception as e:
+                    log.exception("Error finding latest SAML config for slug %s: %s",
+                                  self.saml_configuration.slug, e)
+                    conf['saml_sp_configuration'] = direct_config


This line also seems redundant now.

Suggested change

conf['saml_sp_configuration'] = direct_config

UsamaSadiq · 2025-06-12T10:13:49Z

common/djangoapps/third_party_auth/models.py

+            else:
+                log.info("SAML using direct config (toggle OFF): provider=%s, slug=%s, direct_id=%s",
+                         self.slug, direct_config.slug, direct_config.id)
+                conf['saml_sp_configuration'] = direct_config


Same here.

Suggested change

conf['saml_sp_configuration'] = direct_config

@UsamaSadiq Removed as pr your suggestion.

UsamaSadiq · 2025-06-12T10:34:01Z

common/djangoapps/third_party_auth/tests/test_models.py

+        """
+        site = SiteFactory()
+
+        # Create initial SAML configuration


Initial setup should be defined inside setUp(self) function. That'll clean up the test case and make it easier to review the changes.

common/djangoapps/third_party_auth/tests/test_models.py

UsamaSadiq · 2025-06-12T10:41:11Z

common/djangoapps/third_party_auth/tests/test_models.py

+        initial_config.save()
+        initial_config.refresh_from_db()
+
+        # Verify we have two configs with the same slug


You can also look for the implementation of subTest to make the different cases more segregated but this is an optimization. If it takes more time, you can skip this.

UsamaSadiq · 2025-06-12T10:42:03Z

common/djangoapps/third_party_auth/tests/test_models.py

+        )
+        provider_idp = provider_config.get_config()
+        self.assertEqual(
+            provider_idp.conf.get('saml_sp_configuration').id,


Wouldn't it be better to compare the objects directly instead of comparing the ids?
Similar to this

self.assertEqual( provider_idp.conf.get('saml_sp_configuration').entity_id, 'https://default-entity-id.example.com' )

Thankyou For the suggestion. Implemented

robrap

I need more time for review. This is complicated, and it took me a bit to get my head into this and what I think we'd want. You are on the right track, but there is more I need to look into.

common/djangoapps/third_party_auth/models.py

robrap

I added more comments. I have not looked at unit tests, but they could be reviewed after these updates. Thank you!

common/djangoapps/third_party_auth/models.py

robrap · 2025-06-19T01:39:26Z

common/djangoapps/third_party_auth/models.py

Please add a comment like the following before saml_configuration, which is set in this code:

# Ideally we would have stored the SAMLConfiguration keys of site_id and slug, rather than # pointing to a specific record which may no longer be current when we try to use it. This # has been compensated for elsewhere by retrieving the site_id and slug from the stored row, # and using it to get the current (latest) row instead.

As coded, does that admin UI still show the older record or the newer one? Can we adjust this somehow, or would you need to update the record itself, and if so, when would be the right time to do so? These are Django questions, as well as questions regarding the best way to compensate for the current problem.

As discussed on the call:

It may be that we respond to the signal change and update all provider configs when the SAMLConfiguration gets updated. However, if we do this, we'd need to not update if the change was disabling the SAMLConfiguration (vs updating it).

I need to check in with the Enterprise team about all of this, and double-checking that this change is wanted at all. I'm having doubts about whether people are used to and dependent on the current manual process for rolling out changes.

If we continue with this, we'll need to follow https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0051-bp-conventional-commits.html for breaking changes.
UPDATE: I guess this wouldn't be a breaking change if we make the toggle permanent and provide an option here.

[inform]

I created [DEPR]: SAMLProviderConfig reference to outdated SAMLConfiguration #36943 to get approval on this proposal.

I think we should switch to updating on signal change as you originally proposed. Apologies that it took me so long to get clear on this.

Note: If you don't want to chance re-working or abandoning, we can first ensure that the proposal gets approved. I'll leave that to you.

Thankyou Robert For This Start Working On This

common/djangoapps/third_party_auth/models.py

robrap · 2025-06-19T01:59:20Z

common/djangoapps/third_party_auth/models.py

+                log.info("SAML using direct config (toggle OFF): provider=%s, slug=%s, direct_id=%s",
+                         self.slug, direct_config.slug, direct_config.id)
+        else:
+            conf['saml_sp_configuration'] = SAMLConfiguration.current(self.site.id, 'default')


Add the following:

set_custom_attribute('saml_config.using', 'default')

Also, if the toggle is enabled and there is no latest_config (if it can be disabled), I think the default should be the fallback, rather than the direct_config.

Implemented the suggested changes

I'm not sure if this is correct yet. See https://github.com/openedx/edx-platform/pull/36874/files#r2162572219.

common/djangoapps/third_party_auth/models.py

robrap · 2025-06-23T21:36:57Z

common/djangoapps/third_party_auth/models.py

+                    log.exception("Error finding latest SAML config for slug %s, site_id %s: %s",
+                                  self.saml_configuration.slug, self.saml_configuration.site_id, e)
+            else:
+                set_custom_attribute('saml_config.using', 'default')


It seems like the configuration itself is not actually updated in this case? Maybe this logic will need to be updated a bit. However, we may first want to see if we update the code based on the signal before reviewing this again.

robrap · 2025-07-03T18:29:15Z

Closing this PR, because the newer implementation was moved to:

fix: SAML provider config references to use current SAML configuration versions #36954

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch 2 times, most recently from 065be28 to 7a67bc2 Compare June 9, 2025 09:11

robrap reviewed Jun 9, 2025

View reviewed changes

common/djangoapps/third_party_auth/models.py Show resolved Hide resolved

robrap mentioned this pull request Jun 9, 2025

Have SAMLProviderConfig point to the most current version of SAMLConfiguration edx/edx-arch-experiments#334

Closed

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch 2 times, most recently from e021c95 to 7c3f0a9 Compare June 10, 2025 12:44

robrap reviewed Jun 10, 2025

View reviewed changes

common/djangoapps/third_party_auth/models.py Outdated Show resolved Hide resolved

UsamaSadiq reviewed Jun 11, 2025

View reviewed changes

robrap reviewed Jun 12, 2025

View reviewed changes

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch from 7c3f0a9 to fb6fa02 Compare June 12, 2025 07:08

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch from fb6fa02 to 4cb9c21 Compare June 12, 2025 08:17

UsamaSadiq reviewed Jun 12, 2025

View reviewed changes

common/djangoapps/third_party_auth/models.py Show resolved Hide resolved

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch 2 times, most recently from 74c93c0 to 2e3e805 Compare June 12, 2025 09:56

UsamaSadiq reviewed Jun 12, 2025

View reviewed changes

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch from 2e3e805 to d762c3a Compare June 12, 2025 10:25

UsamaSadiq reviewed Jun 12, 2025

View reviewed changes

UsamaSadiq requested changes Jun 12, 2025

View reviewed changes

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch 2 times, most recently from 542ffdc to b8534a6 Compare June 12, 2025 11:09

ktyagiapphelix2u requested review from UsamaSadiq and robrap June 12, 2025 11:21

robrap requested changes Jun 18, 2025

View reviewed changes

common/djangoapps/third_party_auth/models.py Outdated Show resolved Hide resolved

robrap reviewed Jun 19, 2025

View reviewed changes

fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig

648f8af

ktyagiapphelix2u force-pushed the ktyagi/enhance_saml_config branch from b8534a6 to 648f8af Compare June 19, 2025 13:38

robrap reviewed Jun 23, 2025

View reviewed changes

Merge branch 'openedx:master' into ktyagi/enhance_saml_config

8421c6f

robrap closed this Jul 3, 2025

fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig #36874

fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig #36874

Uh oh!

Conversation

ktyagiapphelix2u commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UsamaSadiq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktyagiapphelix2u commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktyagiapphelix2u Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ktyagiapphelix2u commented Jun 12, 2025 •

edited

Loading

ktyagiapphelix2u Jun 12, 2025 •

edited

Loading

robrap Jun 20, 2025 •

edited

Loading

robrap Jun 19, 2025 •

edited

Loading