You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/06/11 10:37:25 UTC
[GitHub] [cloudstack] neogismm opened a new pull request, #6451: fix for google idp saml redirect url
neogismm opened a new pull request, #6451:
URL: https://github.com/apache/cloudstack/pull/6451
Fixes #6427
This PR fixes the redirect URL for SAML authentication in case of using Google as IdP.
Note: I could not test my fix because choosing google as idp for a user requires the SSO url which also contains the unique IDP id created by google.
Example:
SSO URL: https://accounts.google.com/o/saml2/idp?dpid={IDP-ID}
Entity ID: https://accounts.google.com/o/saml2?idpid={IDP-ID}
To set up a custom google saml app requires a google workspace account which was not possible for me to create at the moment.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] 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
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on code in PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#discussion_r895596015
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java:
##########
@@ -150,6 +150,9 @@ public static String buildAuthnRequestUrl(final String authnId, final SAMLProvid
if (spMetadata.getKeyPair() != null) {
privateKey = spMetadata.getKeyPair().getPrivate();
}
+ if (idpMetadata.getEntityId().startsWith("https://accounts.google.com/o/saml2?idpid=")) {
Review Comment:
or search for `?` in the idp entity url and based on that use correct concatenation strategy? (+ plus add unit tests)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] neogismm commented on pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
neogismm commented on PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#issuecomment-1157268055
Closing this PR as @Luis-3M's PR has a better solution and also includes the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] neogismm closed pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
neogismm closed pull request #6451: fix google idp saml redirect url
URL: https://github.com/apache/cloudstack/pull/6451
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on code in PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#discussion_r895595359
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java:
##########
@@ -150,6 +150,9 @@ public static String buildAuthnRequestUrl(final String authnId, final SAMLProvid
if (spMetadata.getKeyPair() != null) {
privateKey = spMetadata.getKeyPair().getPrivate();
}
+ if (idpMetadata.getEntityId().startsWith("https://accounts.google.com/o/saml2?idpid=")) {
Review Comment:
Can we use URIBuilder instead, https://hc.apache.org/httpcomponents-core-5.1.x/5.1.1/httpcore5/apidocs/org/apache/hc/core5/net/URIBuilder.html
... or similar that helps generate the url based on a base URL and query params.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Luis-3M commented on a diff in pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
Luis-3M commented on code in PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#discussion_r897015022
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java:
##########
@@ -150,6 +150,9 @@ public static String buildAuthnRequestUrl(final String authnId, final SAMLProvid
if (spMetadata.getKeyPair() != null) {
privateKey = spMetadata.getKeyPair().getPrivate();
}
+ if (idpMetadata.getEntityId().startsWith("https://accounts.google.com/o/saml2?idpid=")) {
Review Comment:
searching for `?` in the IdP url should be sufficient, since we only care about appending the SAML request query param to the URL.
https://github.com/apache/cloudstack/pull/6457
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on a diff in pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
nvazquez commented on code in PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#discussion_r895575182
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java:
##########
@@ -150,6 +150,9 @@ public static String buildAuthnRequestUrl(final String authnId, final SAMLProvid
if (spMetadata.getKeyPair() != null) {
privateKey = spMetadata.getKeyPair().getPrivate();
}
+ if (idpMetadata.getEntityId().startsWith("https://accounts.google.com/o/saml2?idpid=")) {
+ redirectUrl = idpMetadata.getSsoUrl() + SAMLUtils.generateSAMLRequestSignature(SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm);
+ }
Review Comment:
Do we need an else block here? Otherwise the `redirectUrl` will be overridden on the next line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#issuecomment-1152903144
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6451)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6451&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6451&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6451&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6451&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6451&metric=new_coverage&view=list) [33.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6451&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6451&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6451&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
nvazquez commented on PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#issuecomment-1156820086
@Luis-3M @neogismm it seems we have 2 PRs solving the same issue, can you work on unifying them? (#6457)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #6451: fix google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
nvazquez commented on PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#issuecomment-1153758112
Thanks @neogismm can you please rebase and target the PR to the 4.17 branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] boring-cyborg[bot] commented on pull request #6451: fix for google idp saml redirect url
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #6451:
URL: https://github.com/apache/cloudstack/pull/6451#issuecomment-1152896518
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](https://github.com/apache/.github/blob/main/.github/CODE_OF_CONDUCT.md) 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](https://cwiki.apache.org/confluence/display/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/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org