fix: SAML provider config references to use current SAML configuration versions #36954
Conversation
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
from
4347225 to
4f7fd1b
Compare
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
I did not really get a chance to review. Just one comment you can adjust before I get to this. Thank you.
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
from
4f7fd1b to
76e98a2
Compare
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
I did not review tests, but you are free to get them working. As noted, I don't think you manually tested yet, right?
|
|
||
| # Fall back to default configuration | ||
| try: | ||
| default_config = SAMLConfiguration.current(self.site_id, 'default') |
There was a problem hiding this comment.
Can this fail, or does it just return None? I think SAMLConfiguration.current(self.site_id, 'default') was used in the legacy implementation outside of a try block, correct?
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
from
5f96401 to
cad5278
Compare
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
Apologies, but I did not review tests yet. I made some proposals for some big simplifications, and we can discuss as needed. I also tried to provide some help learning how to avoid unnecessary comments, and how to better handle custom attributes.
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
- I just provided some high-level comments for unit tests, because other adjustments need to happen first.
- I'm guessing this didn't go through a thorough local re-review before getting to me. I don't know what the right balance is. On the one hand, you are responding to my comments, so I make sense as the re-reviewer. On the other hand, we are trying to get your team up-to-speed enough where US reviews are either not needed, or just light confirmations at the end. If your team doesn't get a chance to do these re-reviews before getting to me, it will make this process take longer overall, even though a single PR might go faster. FYI: @vgulati-apphelix
Thanks.
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
from
92eb914 to
0cca2ec
Compare
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
from
d53c193 to
0b7cd8b
Compare
There was a problem hiding this comment.
It was noted in Slack that I should re-review this, but I don't see anyone on this PR from AppHelix that gave a thumbs up from an internal review. I went ahead and reviewed.
Also, I appreciate when you respond to each of my comments like you mostly did. Some did not have a comment though. I have mostly marked old comments as Resolved.
| self.provider_config.refresh_from_db() | ||
| self.assertEqual(self.provider_config.saml_configuration_id, old_config.id) | ||
|
|
||
| def setUp(self): |
There was a problem hiding this comment.
- If you add a setUp function, it should be first in the class to help the reader.
- Compare this to the other setUp method. Why are factory methods used elsewhere? See https://github.com/openedx/edx-platform/blob/0b7cd8bc0923161a620bb648339ecbb35980eb95/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py#L71-L72.
|
|
||
|
|
||
| @ddt.ddt | ||
| class TestSAMLConfigurationManagementCommand(TestCase): |
There was a problem hiding this comment.
Isn't this what TestSAMLCommand is for? Aren't we just adding additional tests for the same command? Is it possible to reuse its setup?
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
2 times, most recently
from
7550a83 to
a4324ca
Compare
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
from
c519c44 to
9b085b2
Compare
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
I still need to get to through this review, but posting what little I added.
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
Note: All my comments are nits at this point. Feel free to update what you will and we should try to go over and merge in the morning. Thanks.
| error_calls = [ | ||
| call for call in mock_set_custom_attribute.mock_calls | ||
| if call[1][0] == 'saml_config_signal.error_message' | ||
| ] | ||
| assert error_calls, ( | ||
| f"Expected 'saml_config_signal.error_message' call for {description}, " | ||
| f"got: {mock_set_custom_attribute.mock_calls}" | ||
| ) |
There was a problem hiding this comment.
Seems like assert_has_call (or calls?) would be more clean, but maybe I'm missing something? Maybe this is all setup for the following assertion, which may be unnecessary?
ktyagiapphelix2u
force-pushed
the
ktyagi/enahnce_saml_config_2
branch
from
6dffd6b to
d003083
Compare
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
3 participants
Description
Issue
When a new version of a SAMLConfiguration was created, the related SAMLProviderConfig (which depends on it) still pointed to the old version. This caused problems because the provider was using outdated SAML settings. Updating it required manual work, which sometimes led to mistakes in production.
Solution
This update changes the logic so that whenever a new SAMLConfiguration is created, a new SAMLProviderConfig is also created. The new provider config automatically points to the latest SAML configuration. This avoids manual updates and makes sure provider configs always use the most recent SAML settings.
Testing
You can test the changes in SQL Workbench using these queries:
View all SAML configurations:
SELECT * FROM edxapp.third_party_auth_samlconfiguration;
View all SAML provider configs:
SELECT * FROM edxapp.third_party_auth_samlproviderconfig;
(Optional) Delete all provider configs:
SET sql_safe_updates = 0;
DELETE FROM edxapp.third_party_auth_samlproviderconfig;
Private Ticket Link
BOMS-64