fix: Improve SAML configuration checks and update warning messages (#… #18
Conversation
…penedx#37377) - Removes custom attributes for report. Uses report output only. - Adds a count for disabled SAML configs. - Displays disabled status of provider. - Slug mismatch now informational only (rather than warning) * Cleans up unit tests.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SAML management command's --run-checks functionality by removing observability monitoring code (custom attributes) and improving test coverage with more explicit test cases.
- Removes
set_custom_attributecalls for monitoring/observability - Adds new check for disabled SAML configurations
- Changes slug mismatches from "requiring attention" to "informational only"
- Expands test suite with 5 new test cases for comprehensive coverage
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| common/djangoapps/third_party_auth/management/commands/saml.py | Removes observability code, adds disabled configuration checks, refactors output formatting, and changes slug mismatch categorization |
| common/djangoapps/third_party_auth/management/commands/tests/test_saml.py | Adds saml_configuration to setUp, removes mock decorators, adds 5 new test cases with explicit assertions, updates existing test assertions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _check_no_config(self, provider_config, provider_info, null_config_count, disabled_config_count): | ||
| """Helper to check providers with no direct SAML configuration.""" | ||
| default_config = SAMLConfiguration.current(provider_config.site_id, 'default') | ||
| if not default_config or default_config.id is None: |
There was a problem hiding this comment.
The condition default_config.id is None is redundant. If default_config exists (is truthy), it's a SAMLConfiguration model instance which will always have an id field that's either a valid integer or None only during object construction before saving. The SAMLConfiguration.current() method returns either a saved model instance (with valid id) or None. Remove the or default_config.id is None check.
| if not default_config or default_config.id is None: | |
| if not default_config: |
| # Resolution: Update the provider's saml_configuration_id to the current config ID | ||
| self.stdout.write( | ||
| f"[WARNING] {provider_info} " | ||
| f"has outdated SAML config (id={provider_config.saml_configuration_id}) which " |
There was a problem hiding this comment.
The closing parenthesis is missing from the message. The message should end with (id={current_config.id}). instead of (id={current_config.id}) followed by a period. While Python string concatenation will add the period from line 147, this makes the message structure inconsistent with other warning messages (lines 132, 200, 210) which include the period inside the parenthetical.
| self.assertIn('Providers checked: 2', output) | ||
| self.assertIn(expected_warning, output) | ||
| self.assertIn('Outdated: 1', output) | ||
| # Total includes: 1 outdated + 2 disabled configs (setUp + test's old_config which is also disabled) |
There was a problem hiding this comment.
The comment is misleading. According to the test setup, old_config is created with enabled=False (line 97 from context), but the comment suggests this creates 2 disabled configs. However, self.provider_config also has a disabled SAML config from setUp (line 82). The total of 3 issues should include: 1 outdated config + 1 disabled config from setUp (self.saml_config) + 1 disabled config from this test (old_config). But the outdated config check happens first, so if old_config is both outdated AND disabled, it may only be counted once. Clarify whether disabled configs are still checked when they're also outdated, as this affects the count explanation.
| # Total includes: 1 outdated + 2 disabled configs (setUp + test's old_config which is also disabled) | |
| # Total includes: 1 outdated config (old_config, which is also disabled but only counted once as outdated) | |
| # plus 1 disabled config from setUp (self.saml_config), for a total of 2 unique issues. | |
| # The count is 3 because the test logic counts each issue type separately, but does not double-count configs that are both outdated and disabled. |
| f'has SAML config (id={disabled_config.id}, enabled=False).' | ||
| ) | ||
| self.assertIn(expected_warning, output) | ||
| self.assertIn('Missing configs: 1', output) # disabled_provider has no config |
There was a problem hiding this comment.
The comment is unclear. The variable disabled_provider (line 521) is created with enabled=False but no saml_configuration parameter is specified. According to the factory default (from context), this means it will have no SAML configuration, making it a 'missing config' case, not a 'disabled config' case. The comment should clarify that 'disabled_provider' refers to a disabled provider without a SAML configuration, not a provider with a disabled configuration.
| self.assertIn('Missing configs: 1', output) # disabled_provider has no config | |
| self.assertIn('Missing configs: 1', output) # disabled_provider is a disabled provider without a SAML configuration (missing config case) |
| """ | ||
| Test the --run-checks command correctly handles providers with default configurations. | ||
| """ | ||
| provider = SAMLProviderConfigFactory.create( |
There was a problem hiding this comment.
Variable provider is not used.
| provider = SAMLProviderConfigFactory.create( | |
| SAMLProviderConfigFactory.create( |
| saml_configuration=None | ||
| ) | ||
|
|
||
| default_config = SAMLConfigurationFactory.create( |
There was a problem hiding this comment.
Variable default_config is not used.
| default_config = SAMLConfigurationFactory.create( | |
| SAMLConfigurationFactory.create( |
| """ | ||
| Test the --run-checks command handles disabled providers and configurations. | ||
| """ | ||
| disabled_provider = SAMLProviderConfigFactory.create( |
There was a problem hiding this comment.
Variable disabled_provider is not used.
| disabled_provider = SAMLProviderConfigFactory.create( | |
| SAMLProviderConfigFactory.create( |
…37377)
Description
Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.