-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: Improve SAML configuration checks and update warning messages #37377
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.
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
Conversation
2ba0539 to
3dd7d88
Compare
7ccd670 to
e00af91
Compare
|
why i change get_config() when use get_config(), it show many error, same error again again for missing metadata. So I have improved it to new logic |
d008425 to
c0f5c34
Compare
0dc6d82 to
e37bf16
Compare
a099247 to
6cf1af3
Compare
|
The Problem with get_config(): get_config() was mixing configuration validation with metadata validation. It was generating spam error messages about missing metadata ("No SAMLProviderData found"). It was checking both config AND metadata, but this command should only check config. Metadata checking belongs in the --pull command, not the config check command |
7de16fd to
92dee4f
Compare
|
Proposed summary output: |
0ab4058 to
93b9c21
Compare
|
@robrap I have updated the test cases and updated the saml config report for missing config one and kept it as warning only and added test case when it will give missing config this will give missing config as |
robrap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't go through tests yet. This is a start. Thank you.
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
616df54 to
07a090f
Compare
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
85e9b5f to
d277d54
Compare
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
robrap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though there are a bunch of comments, I think this is close. Thanks. The tests don't seem overly dense and redundant any longer.
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
c9300d2 to
54fc1d9
Compare
robrap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Minor comments.
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
1520eb2 to
36c8bf5
Compare
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
…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.
…penedx#37377) (#18) - 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.
…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.
Description
Improve SAML command to stop wrong warnings. Tests updated too.
Private Ticket
https://2u-internal.atlassian.net/jira/software/c/projects/BOMS/boards/3017?assignee=712020%3A61c35560-3472-4e12-b833-884e5c4bbff4&selectedIssue=BOMS-64
Related PR
#37330
Saml Configuration Report