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.

Author: Luis-3M

plugins/user-authenticators/saml2/pom.xml

@@ -53,5 +53,11 @@
             <version>${cs.xercesImpl.version}</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.assertj</groupId>
+            <artifactId>assertj-core</artifactId>
+            <version>${cs.assertj.version}</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 </project>

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

@@ -150,7 +150,8 @@ public static String buildAuthnRequestUrl(final String authnId, final SAMLProvid
             if (spMetadata.getKeyPair() != null) {
                 privateKey = spMetadata.getKeyPair().getPrivate();
             }
-            redirectUrl = idpMetadata.getSsoUrl() + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm);
+            String appendOperator = idpMetadata.getSsoUrl().contains("?") ? "&" : "?";
+            redirectUrl = idpMetadata.getSsoUrl() + appendOperator + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm);
         } catch (ConfigurationException | FactoryConfigurationError | MarshallingException | IOException | NoSuchAlgorithmException | InvalidKeyException | java.security.SignatureException e) {
             s_logger.error("SAML AuthnRequest message building error: " + e.getMessage());
         }

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

@@ -19,18 +19,22 @@
 
 package org.apache.cloudstack;
 
-import java.security.KeyPair;
-import java.security.PrivateKey;
-import java.security.PublicKey;
-import java.util.regex.Pattern;
-
+import junit.framework.TestCase;
+import org.apache.cloudstack.saml.SAML2AuthManager;
+import org.apache.cloudstack.saml.SAMLProviderMetadata;
 import org.apache.cloudstack.saml.SAMLUtils;
 import org.apache.cloudstack.utils.security.CertUtils;
 import org.junit.Test;
 import org.opensaml.saml2.core.AuthnRequest;
 import org.opensaml.saml2.core.LogoutRequest;
 
-import junit.framework.TestCase;
+import java.net.URI;
+import java.security.KeyPair;
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.util.regex.Pattern;
+
+import static org.assertj.core.api.Assertions.assertThat;
 
 public class SAMLUtilsTest extends TestCase {
 
@@ -60,6 +64,63 @@ public void testBuildAuthnRequestObject() throws Exception {
         assertEquals(req.getIssuer().getValue(), spId);
     }
 
+    @Test
+    public void testBuildAuthnRequestUrlWithoutQueryParam() throws Exception {
+        String urlScheme = "http";
+
+        String spDomain = "sp.domain.example";
+        String spUrl = urlScheme + "://" + spDomain;
+        String spId = "serviceProviderId";
+
+        String idpDomain = "idp.domain.example";
+        String idpUrl = urlScheme + "://" + idpDomain;
+        String idpId = "identityProviderId";
+
+        String authnId = SAMLUtils.generateSecureRandomId();
+
+        SAMLProviderMetadata spMetadata = new SAMLProviderMetadata();
+        spMetadata.setEntityId(spId);
+        spMetadata.setSsoUrl(spUrl);
+
+        SAMLProviderMetadata idpMetadata = new SAMLProviderMetadata();
+        idpMetadata.setSsoUrl(idpUrl);
+        idpMetadata.setEntityId(idpId);
+
+        URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value()));
+        assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("SAMLRequest");
+        assertEquals(urlScheme, redirectUrl.getScheme());
+        assertEquals(idpDomain, redirectUrl.getHost());
+    }
+
+    @Test
+    public void testBuildAuthnRequestUrlWithQueryParam() throws Exception {
+        String urlScheme = "http";
+
+        String spDomain = "sp.domain.example";
+        String spUrl = urlScheme + "://" + spDomain;
+        String spId = "cloudstack";
+
+        String idpDomain = "idp.domain.example";
+        String idpQueryParam = "idpid=CX1298373";
+        String idpUrl = urlScheme + "://" + idpDomain + "?" + idpQueryParam;
+        String idpId = "identityProviderId";
+
+        String authnId = SAMLUtils.generateSecureRandomId();
+
+        SAMLProviderMetadata spMetadata = new SAMLProviderMetadata();
+        spMetadata.setEntityId(spId);
+        spMetadata.setSsoUrl(spUrl);
+
+        SAMLProviderMetadata idpMetadata = new SAMLProviderMetadata();
+        idpMetadata.setSsoUrl(idpUrl);
+        idpMetadata.setEntityId(idpId);
+
+        URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value()));
+        assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("idpid").hasParameter("SAMLRequest");
+        assertEquals(urlScheme, redirectUrl.getScheme());
+        assertEquals(idpDomain, redirectUrl.getHost());
+    }
+
     @Test
     public void testBuildLogoutRequest() throws Exception {
         String logoutUrl = "http://logoutUrl";

pom.xml

@@ -118,6 +118,7 @@
         <cs.testng.version>7.1.0</cs.testng.version>
         <cs.wiremock.version>2.27.2</cs.wiremock.version>
         <cs.xercesImpl.version>2.12.2</cs.xercesImpl.version>
+        <cs.assertj.version>3.23.1</cs.assertj.version>
 
         <!-- Dependencies versions -->
         <cs.amqp-client.version>5.10.0</cs.amqp-client.version>