Fix SAML SSO plugin redirect URL by Luis-3M · Pull Request #6457 · apache/cloudstack

Luis-3M · 2022-06-14T14:49:40Z

Description

This PR fixes the issue #6427 -> SAML request must be appended to an IdP URL as a query param with an ampersand, if the URL already contains a question mark, as opposed to always assume that IdP URLs don't have any query params.
Google's IdP URL for instance looks like this: https://accounts.google.com/o/saml2/idp?idpid=<ID>, therefore the expected redirect URL would be https://accounts.google.com/o/saml2/idp?idpid=<ID>&SAMLRequest=<SAMLRequest>

This code change is backwards compatible with the current behaviour.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

Successful request

Unsuccessful request (due to a broken redirect URL)

How Has This Been Tested?

boring-cyborg · 2022-06-14T14:49:43Z

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

acs-robot · 2022-06-14T14:50:57Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-06-14T14:52:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-06-14T14:58:10Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6457 (SL-JID-1730)

nvazquez · 2022-06-15T18:58:54Z

@Luis-3M @neogismm it seems we have 2 PRs solving the same issue, can you work on unifying them? (#6451)

Luis-3M · 2022-06-15T22:07:28Z

@nvazquez Yep happy to close mine if @neogismm keeps #6451 open 👍

neogismm · 2022-06-16T06:12:51Z

Hi @Luis-3M thanks for the PR. I was stuck on how to write the tests for the same and looking at your code helps me to learn.

Luis-3M · 2022-06-16T09:18:32Z

No worries at all @neogismm - thank you.

acs-robot · 2022-06-21T16:12:21Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-06-21T16:13:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-06-21T16:13:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-06-21T16:19:57Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6457 (SL-JID-1798)

Luis-3M · 2022-06-21T16:19:58Z

@nvazquez we will keep this one open.

blueorangutan · 2022-06-21T16:24:01Z

UI build: ✖️
(SL-JID-2)

plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java

acs-robot · 2022-06-23T11:30:55Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java

…equestUrl

acs-robot · 2022-06-28T23:09:07Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-06-28T23:10:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-06-28T23:16:52Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6457 (SL-JID-1868)

plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java

harikrishna-patnala · 2022-07-05T05:20:56Z

@Luis-3M Code LGTM. If you can please address one code smell as suggested by Daan, that will look pefect.

Luis-3M · 2022-07-05T08:45:06Z

Thanks for your input @DaanHoogland @harikrishna-patnala .
Suggested changes have been addressed.

acs-robot · 2022-07-05T08:45:17Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-07-05T08:46:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-07-05T08:52:23Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6457 (SL-JID-1904)

sonarqubecloud · 2022-07-05T09:51:25Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

66.7% Coverage
0.0% Duplication

DaanHoogland · 2022-07-05T10:01:51Z

@blueorangutan package

blueorangutan · 2022-07-05T10:03:03Z

@DaanHoogland a 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.

blueorangutan · 2022-07-05T10:52:50Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3711

DaanHoogland · 2022-07-05T11:06:21Z

@blueorangutan test keepEnv

blueorangutan · 2022-07-05T11:07:03Z

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

blueorangutan · 2022-07-05T22:49:14Z

Trillian test result (tid-4430)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40694 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6457-t4430-kvm-centos7.zip
Smoke tests completed. 97 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_disable_oobm_ha_state_ineligible	`Error`	1511.69	test_hostha_kvm.py

boring-cyborg · 2022-07-06T03:58:39Z

Awesome work, congrats on your first merged pull request!

* Add EncryptedElementType key resolver to SAML plugin * saml: Fix SAML SSO plugin redirect URL (apache#6457) This PR fixes the issue apache#6427 -> SAML request must be appended to an IdP URL as a query param with an ampersand, if the URL already contains a question mark, as opposed to always assume that IdP URLs don't have any query params. Google's IdP URL for instance looks like this: https://accounts.google.com/o/saml2/idp?idpid=<ID>, therefore the expected redirect URL would be https://accounts.google.com/o/saml2/idp?idpid=<ID>&SAMLRequest=<SAMLRequest> This code change is backwards compatible with the current behaviour. * Apply backport for SAML session cookie path apache#6149 * ui: Logout before login (apache#6193) This PR calls the logout API before login, to cleanup any duplicate sessionkey, as it was done on the legacy UI: apache#4326 Fixes: apache#6127 --------- Co-authored-by: Marcus Sorensen <mls@apple.com> Co-authored-by: Luis Moreira <Luis-3M@users.noreply.github.com> Co-authored-by: Nicolas Vazquez <nicovazquez90@gmail.com>

Fix SAML SSO plugin redirect URL

43477e0

boring-cyborg bot added the component:saml label Jun 14, 2022

Luis-3M mentioned this pull request Jun 14, 2022

fix google idp saml redirect url #6451

Closed

12 tasks

nvazquez added this to the 4.17.1.0 milestone Jun 15, 2022

nvazquez added the type:bug label Jun 15, 2022

utchoang force-pushed the 4.17 branch 4 times, most recently from 4bf2f2b to 1b71696 Compare June 16, 2022 11:01

Fix redirect url test methods naming convention

bf61ad8

DaanHoogland reviewed Jun 23, 2022

View reviewed changes

plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java Outdated Show resolved Hide resolved

Address PR comments

749dca2

Luis-3M requested a review from DaanHoogland June 23, 2022 11:31

DaanHoogland reviewed Jun 23, 2022

View reviewed changes

plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java Outdated Show resolved Hide resolved

DaanHoogland reviewed Jun 23, 2022

View reviewed changes

plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java Outdated Show resolved Hide resolved

Validate the final redirect URL as a result of the method buildAuthnR…

c21bed8

…equestUrl

boring-cyborg bot added the component:build label Jun 28, 2022

yadvr requested review from DaanHoogland, harikrishna-patnala and weizhouapache June 30, 2022 06:59

DaanHoogland approved these changes Jul 4, 2022

View reviewed changes

harikrishna-patnala approved these changes Jul 5, 2022

View reviewed changes

PR comments

2deaefb

nvazquez approved these changes Jul 5, 2022

View reviewed changes

yadvr merged commit c6b6114 into apache:4.17 Jul 6, 2022

yadvr mentioned this pull request Jul 6, 2022

Broken SAML SSO plugin with malformed URL when authenticating against Google IdP #6427

Closed

Conversation

Luis-3M commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

boring-cyborg bot commented Jun 14, 2022

Uh oh!

acs-robot commented Jun 14, 2022

Uh oh!

blueorangutan commented Jun 14, 2022

Uh oh!

blueorangutan commented Jun 14, 2022

Uh oh!

nvazquez commented Jun 15, 2022

Uh oh!

Luis-3M commented Jun 15, 2022

Uh oh!

neogismm commented Jun 16, 2022

Uh oh!

Luis-3M commented Jun 16, 2022

Uh oh!

acs-robot commented Jun 21, 2022

Uh oh!

blueorangutan commented Jun 21, 2022

Uh oh!

blueorangutan commented Jun 21, 2022

Uh oh!

blueorangutan commented Jun 21, 2022

Uh oh!

Luis-3M commented Jun 21, 2022

Uh oh!

blueorangutan commented Jun 21, 2022

Uh oh!

Uh oh!

acs-robot commented Jun 23, 2022

Uh oh!

Uh oh!

Uh oh!

acs-robot commented Jun 28, 2022

Uh oh!

blueorangutan commented Jun 28, 2022

Uh oh!

blueorangutan commented Jun 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harikrishna-patnala commented Jul 5, 2022

Uh oh!

Luis-3M commented Jul 5, 2022

Uh oh!

acs-robot commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

sonarqubecloud bot commented Jul 5, 2022

Uh oh!

DaanHoogland commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

DaanHoogland commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

boring-cyborg bot commented Jul 6, 2022

Uh oh!

Reviewers

Luis-3M commented Jun 14, 2022 •

edited

Loading