Skip to content

fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig #36874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ktyagiapphelix2u wants to merge 2 commits into from
Closed

fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig #36874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
648f8af
fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig
ktyagiapphelix2u Jun 19, 2025
8421c6f
Merge branch 'openedx:master' into ktyagi/enhance_saml_config
ktyagiapphelix2u Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 47 additions & 5 deletions common/djangoapps/third_party_auth/models.py
View file
Open in desktop

@robrap robrap Jun 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  1. 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.

@robrap robrap Jun 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed on the call:

  1. 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).
  2. 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.
  3. 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.

@robrap robrap Jun 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[inform]

  1. I created [DEPR]: SAMLProviderConfig reference to outdated SAMLConfiguration #36943 to get approval on this proposal.
  2. 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.

@ktyagiapphelix2u ktyagiapphelix2u Jun 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankyou Robert For This Start Working On This

Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from social_core.backends.saml import SAMLAuth
from social_core.exceptions import SocialAuthBaseException
from social_core.utils import module_member
from edx_django_utils.monitoring import set_custom_attribute

from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.djangoapps.theming.helpers import get_current_request
Expand All @@ -29,6 +30,21 @@

from .lti import LTI_PARAMS_KEY, LTIAuthBackend
from .saml import STANDARD_SAML_PROVIDER_KEY, get_saml_idp_choices, get_saml_idp_class
from edx_toggles.toggles import SettingToggle

# .. toggle_name: FEATURES['USE_LATEST_SAML_CONFIG']
# .. toggle_implementation: SettingToggle
# .. toggle_default: False
# .. toggle_description: When enabled, the system uses the latest enabled SAML configuration with
# the same slug when authenticating users. When disabled, the system uses the directly
# referenced configuration. This is a temporary rollout toggle to get us to the
# enabled state.
# .. toggle_warning: Disabling this may affect authentication for users if providers have
# been updated with newer configurations.
# .. toggle_use_cases: temporary
# .. toggle_creation_date: 2025-06-10
# .. toggle_target_removal_date: 2025-09-01
USE_LATEST_SAML_CONFIG = SettingToggle("USE_LATEST_SAML_CONFIG", default=False, module_name=__name__)

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -759,6 +775,10 @@ class SAMLProviderConfig(ProviderConfig):
)
)
archived = models.BooleanField(default=False)
# 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.
saml_configuration = models.ForeignKey(
SAMLConfiguration,
on_delete=models.SET_NULL,
Expand Down Expand Up @@ -880,12 +900,34 @@ def get_config(self):
conf['x509certMulti'] = {'signing': public_keys}
conf['x509cert'] = ''
conf['url'] = sso_url
# This block determines which SAML configuration should be used during authentication
if self.saml_configuration:
Comment thread
robrap marked this conversation as resolved.
direct_config = self.saml_configuration
conf['saml_sp_configuration'] = direct_config
# .. custom_attribute_name: saml_config.use_latest_toggle_enabled
# .. custom_attribute_description: True if USE_LATEST_SAML_CONFIG is enabled; false otherwise.
set_custom_attribute('saml_config.use_latest_toggle_enabled', USE_LATEST_SAML_CONFIG.is_enabled())
if USE_LATEST_SAML_CONFIG.is_enabled():
Comment thread
UsamaSadiq marked this conversation as resolved.
Comment thread
robrap marked this conversation as resolved.
try:
slug = self.saml_configuration.slug
site_id = self.saml_configuration.site_id
latest_config = SAMLConfiguration.current(self.saml_configuration.site_id, self.saml_configuration.slug)
if latest_config:
# .. custom_attribute_name: saml_config.using
# .. custom_attribute_description: Describes which config is being used: direct (from db), latest,
# or default.
set_custom_attribute('saml_config.using', 'latest')
conf['saml_sp_configuration'] = latest_config
except Exception as e:
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')

@robrap robrap Jun 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
conf['saml_sp_configuration'] = SAMLConfiguration.current(self.site.id, 'default')

@robrap robrap Jun 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the following:

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

@robrap robrap Jun 19, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ktyagiapphelix2u ktyagiapphelix2u Jun 19, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the suggested changes

@robrap robrap Jun 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

# Add SAMLConfiguration appropriate for this IdP
conf['saml_sp_configuration'] = (
self.saml_configuration or
SAMLConfiguration.current(self.site.id, 'default')
)
# Create and return the appropriate IdP class with the configuration
idp_class = get_saml_idp_class(self.identity_provider_type)
return idp_class(self.slug, **conf)

Expand Down
107 changes: 105 additions & 2 deletions common/djangoapps/third_party_auth/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@
Tests for third_party_auth/models.py.
"""
import unittest
from datetime import timedelta

from django.test import TestCase, override_settings
from django.utils import timezone
import mock

from .factories import SAMLProviderConfigFactory
from ..models import SAMLProviderConfig, clean_username
from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory
from .factories import SAMLProviderConfigFactory, SAMLConfigurationFactory
from ..models import SAMLProviderConfig, SAMLConfiguration, SAMLProviderData, USE_LATEST_SAML_CONFIG, clean_username


class TestSamlProviderConfigModel(TestCase, unittest.TestCase):
Expand All @@ -17,6 +22,35 @@ def setUp(self):
super().setUp()
self.saml_provider_config = SAMLProviderConfigFactory()

# Setup for SAML configuration dynamic lookup test
self.site = SiteFactory()

# Create initial SAML configuration using the factory
self.initial_config = SAMLConfigurationFactory(
site=self.site,
slug='test-config',
entity_id='https://initial-entity-id.example.com',
private_key='initial_private_key',
public_key='initial_public_key',
)
self.initial_config_id = self.initial_config.id

# Create provider that points to the initial config
self.provider_config = SAMLProviderConfigFactory(
site=self.site,
entity_id='https://test-idp.example.com',
metadata_source='https://test-idp.example.com/metadata.xml',
saml_configuration=self.initial_config
)

# Create provider data to avoid AuthNotConfigured error
SAMLProviderData.objects.create(
entity_id=self.provider_config.entity_id,
sso_url='https://test-idp.example.com/SSO',
public_key='test_public_key',
fetched_at=timezone.now()
)

def test_unique_entity_id_enforcement_for_non_current_configs(self):
"""
Test that the unique entity ID enforcement does not apply to noncurrent configs
Expand Down Expand Up @@ -53,3 +87,72 @@ def test_clean_username_unicode_enabled(self):
Test the username cleaner function with unicode enabled
"""
assert clean_username('ItJüstWòrks™') == 'ItJüstWòrks'

def test_saml_configuration_dynamic_lookup(self):
"""
Test SAML configuration lookup functionality, including the toggle behavior and error handling.
"""

# Update configuration creates a new record with the same slug
self.initial_config.entity_id = 'https://updated-entity-id.example.com'
self.initial_config.change_date = timezone.now() + timedelta(minutes=5)
self.initial_config.save()
self.initial_config.refresh_from_db()

# Verify we have two configs with the same slug

@UsamaSadiq UsamaSadiq Jun 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

config_count = SAMLConfiguration.objects.filter(slug='test-config').count()
self.assertEqual(config_count, 2)

# Test with toggle ENABLED - should use latest config
with mock.patch.object(USE_LATEST_SAML_CONFIG, 'is_enabled', return_value=True):
provider_idp = self.provider_config.get_config()
latest_config = SAMLConfiguration.objects.filter(
slug='test-config', enabled=True
).order_by('-change_date').first()

# Compare the actual objects rather than just IDs
self.assertEqual(
provider_idp.conf.get('saml_sp_configuration'),
latest_config
)
self.assertEqual(
provider_idp.conf.get('saml_sp_configuration').entity_id,
'https://updated-entity-id.example.com'
)

# Test with toggle DISABLED - should use direct config
with mock.patch.object(USE_LATEST_SAML_CONFIG, 'is_enabled', return_value=False):
provider_idp = self.provider_config.get_config()
self.assertEqual(
provider_idp.conf.get('saml_sp_configuration'),
self.provider_config.saml_configuration
)

# Test error handling
with mock.patch('common.djangoapps.third_party_auth.models.SAMLConfiguration.objects.filter') as mock_filter:
mock_filter.side_effect = Exception("Test error")
with mock.patch.object(USE_LATEST_SAML_CONFIG, 'is_enabled', return_value=True):
provider_idp = self.provider_config.get_config()
self.assertEqual(
provider_idp.conf.get('saml_sp_configuration'),
self.provider_config.saml_configuration
)

# Test default fallback
self.provider_config.saml_configuration = None
self.provider_config.save()
default_config = SAMLConfigurationFactory(
site=self.site,
slug='default',
enabled=True,
entity_id='https://default-entity-id.example.com',
)
provider_idp = self.provider_config.get_config()
self.assertEqual(
provider_idp.conf.get('saml_sp_configuration'),
default_config
)
self.assertEqual(
provider_idp.conf.get('saml_sp_configuration').entity_id,
'https://default-entity-id.example.com'
)
Loading