DaanHoogland commented on code in PR #6457:
URL: https://github.com/apache/cloudstack/pull/6457#discussion_r907305082
##########
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:
Not sure @Luis-3M , but now they are not testing anything in the production
code except maybe consistence of the result of
`SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" +
SAMLUtils.encodeSAMLRequest(req), null,
SAML2AuthManager.SAMLSignatureAlgorithm.value())`, as you are calling it twice
and coparing the results of both calls concatenated to something that can be
read from this code to be the same. If this function breaks for some reason
this test is still going to pass.
`idpUrl` is the same and `appendOperator` is predictable from what you make
`idpUrl`.
Depending on what you would test you would hard code the expected result and
let the production code produce the same for comparison. I am not sure what the
exact format of an SML url would look like and I suppose there would be some
part that is unpredictable (e.g. a one time token) so whether and how to test
this. I would remove this test if we cannot hardcode anything, as it is giving
us false security this way.
--
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]
