Luis-3M commented on code in PR #6457:
URL: https://github.com/apache/cloudstack/pull/6457#discussion_r909065379
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java:
##########
@@ -60,6 +62,32 @@ public void testBuildAuthnRequestObject() throws Exception {
assertEquals(req.getIssuer().getValue(), spId);
}
+ @Test
+ public void testBuildAuthnRequestUrlWithoutQueryParam() throws Exception {
+ String consumerUrl = "http://someurl.com";;
+ String idpUrl = "http://idp.domain.example";;
+ String spId = "cloudstack";
+ String authnId = SAMLUtils.generateSecureRandomId();
+ DefaultBootstrap.bootstrap();
+ AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId,
idpUrl, consumerUrl);
+ String appendOperator = idpUrl.contains("?") ? "&" : "?";
+ String redirectUrl = idpUrl + appendOperator +
SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" +
SAMLUtils.encodeSAMLRequest(req), null,
SAML2AuthManager.SAMLSignatureAlgorithm.value());
+ assertEquals(redirectUrl, idpUrl + "?" +
SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" +
SAMLUtils.encodeSAMLRequest(req), null,
SAML2AuthManager.SAMLSignatureAlgorithm.value()));
Review Comment:
> I suppose there would be some part that is unpredictable (e.g. a one time
token) so whether and how to test this
True we can not predict the value of the SAMLRequest.
Based on the original issue, we know that the result of the
`buildAuthnRequestUrl` method must have at least one query param:
`SAMLRequest`. The original code was simply appending this same query param
with `?`, therefore if the IDP URL already contains at least one query param,
then `SAMLRequest` would actually be missing as seen here:
<img width="1024" alt="image"
src="https://user-images.githubusercontent.com/26753403/176317972-39721780-8e59-430f-b940-eb7b98d72483.png";>
Think that we should at least test the existence of this and other
parameters already part of the IDP URL, if any, as well as some basic URL
components.
Let me know your thoughts @DaanHoogland .
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
![https://user-images.githubusercontent.com/26753403/176317972-39721780-8e59-430f-b940-eb7b98d72483.png"](https://user-images.githubusercontent.com/26753403/176317972-39721780-8e59-430f-b940-eb7b98d72483.png"){kind=link}