Skip to content
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

saml: purge token after first response and improve setting description #9377

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

rohityadavcloud
Copy link
Member

@rohityadavcloud rohityadavcloud commented Jul 12, 2024

This improves the description of a saml signature checking global setting, and purges the SAML token upon handling the first SAML response.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Testing

The community QA server is applied with these changes to test the SAML SSO with the rick user https://qa.cloudstack.cloud/simulator

This improves the description of a saml signature checking global
setting, and purges the SAML token upon handling the first SAML
response.

Signed-off-by: Rohit Yadav <[email protected]>
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm

@rohityadavcloud
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@@ -228,6 +228,7 @@ public String authenticate(final String command, final Map<String, Object[]> par
"Received SAML response for a SSO request that we may not have made or has expired, please try logging in again",
params, responseType));
}
samlAuthManager.purgeToken(token);
Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't very useful for now; but potentially guards against any replays

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 15.08%. Comparing base (32cc1d4) to head (df2328c).
Report is 1 commits behind head on 4.19.

Files Patch % Lines
...g/apache/cloudstack/saml/SAML2AuthManagerImpl.java 0.00% 4 Missing ⚠️
...ack/api/command/SAML2LoginAPIAuthenticatorCmd.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               4.19    #9377    +/-   ##
==========================================
  Coverage     15.07%   15.08%            
- Complexity    11166    11173     +7     
==========================================
  Files          5405     5405            
  Lines        472672   472677     +5     
  Branches      58189    58295   +106     
==========================================
+ Hits          71257    71283    +26     
+ Misses       393486   393464    -22     
- Partials       7929     7930     +1     
Flag Coverage Δ
uitests 4.27% <ø> (ø)
unittests 15.80% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Rohit Yadav <[email protected]>
@rohityadavcloud
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10334

Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

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

code lgtm

@DaanHoogland
Copy link
Contributor

unit test failure :( , trying locally.

@weizhouapache
Copy link
Member

unit test failure :( , trying locally.

the unit test with ActionEventInterceptorTest ?
it is happening with all prs againt 4.19

@rohityadavcloud
Copy link
Member Author

Yup it’s failure on 4.19 branch, anyone fixing it?

@DaanHoogland
Copy link
Contributor

it compiles without issues locally

@shwstppr shwstppr closed this Jul 12, 2024
@shwstppr shwstppr reopened this Jul 12, 2024
@rohityadavcloud
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@shwstppr
Copy link
Contributor

Seems like I didn't wait enough while reopening the PR to kick GH actions. Trying again

@shwstppr shwstppr closed this Jul 12, 2024
@shwstppr shwstppr reopened this Jul 12, 2024
@shwstppr
Copy link
Contributor

Or maybe it was my mobile app 🤦

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10345

Copy link
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

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

clgtm

@sureshanaparti
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@sureshanaparti a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-10827)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47305 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9377-t10827-kvm-centos7.zip
Smoke tests completed. 132 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@rohityadavcloud rohityadavcloud merged commit 2cfb541 into apache:4.19 Jul 15, 2024
51 of 72 checks passed
@rohityadavcloud rohityadavcloud deleted the gs-improve-msg branch July 15, 2024 04:15
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jul 23, 2024
apache#9377)

* saml: purge token after first response and improve setting description

This improves the description of a saml signature checking global
setting, and purges the SAML token upon handling the first SAML
response.

Signed-off-by: Rohit Yadav <[email protected]>

* fix failing unit test

Signed-off-by: Rohit Yadav <[email protected]>

---------

Signed-off-by: Rohit Yadav <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants