keycloak-aplcache

KEYCLOAK-4446 Do not encrypt SAML status messages SAML status

7/25/2017 6:00:07 PM

Details

diff --git a/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java b/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java
index 29bbbfa..f44534d 100755
--- a/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java
+++ b/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java
@@ -18,7 +18,10 @@
 package org.keycloak.adapters.saml;
 
 import org.keycloak.adapters.spi.AuthenticationError;
+import org.keycloak.dom.saml.v2.protocol.StatusCodeType;
 import org.keycloak.dom.saml.v2.protocol.StatusResponseType;
+import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
+import java.util.Objects;
 
 /**
  * Object that describes the SAML error that happened.
@@ -27,6 +30,7 @@ import org.keycloak.dom.saml.v2.protocol.StatusResponseType;
  * @version $Revision: 1 $
  */
 public class SamlAuthenticationError implements AuthenticationError {
+
     public static enum Reason {
         EXTRACTION_FAILURE,
         INVALID_SIGNATURE,
@@ -59,7 +63,18 @@ public class SamlAuthenticationError implements AuthenticationError {
 
     @Override
     public String toString() {
-        return "SamlAuthenticationError [reason=" + reason + ", status=" + status + "]";
+        return "SamlAuthenticationError [reason=" + reason + ", status=" 
+          + ((status == null || status.getStatus() == null) ? "UNKNOWN" : extractStatusCode(status.getStatus().getStatusCode()))
+          + "]";
     }
     
+    private String extractStatusCode(StatusCodeType statusCode) {
+        if (statusCode == null || statusCode.getValue() == null) {
+            return "UNKNOWN";
+        }
+        if (Objects.equals(JBossSAMLURIConstants.STATUS_RESPONDER.get(), statusCode.getValue().toString())) {
+            return extractStatusCode(statusCode.getStatusCode());
+        }
+        return statusCode.getValue().toString();
+    }
 }
diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java
index caa9709..66a7609 100755
--- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java
+++ b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java
@@ -190,16 +190,9 @@ public class SamlProtocol implements LoginProtocol {
                         }
                         binding.signatureAlgorithm(samlClient.getSignatureAlgorithm()).signWith(keyName, keys.getPrivateKey(), keys.getPublicKey(), keys.getCertificate()).signDocument();
                     }
-                    if (samlClient.requiresEncryption()) {
-                        PublicKey publicKey;
-                        try {
-                            publicKey = SamlProtocolUtils.getEncryptionKey(client);
-                        } catch (Exception e) {
-                            logger.error("failed", e);
-                            return ErrorPage.error(session, Messages.FAILED_TO_PROCESS_RESPONSE);
-                        }
-                        binding.encrypt(publicKey);
-                    }
+                    // There is no support for encrypting status messages in SAML.
+                    // Only assertions, attributes, base ID and name ID can be encrypted
+                    // See Chapter 6 of saml-core-2.0-os.pdf
                     Document document = builder.buildDocument();
                     return buildErrorResponse(authSession, binding, document);
                 } catch (Exception e) {
diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java
index cd9ea11..01b8b08 100644
--- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java
+++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java
@@ -27,6 +27,7 @@ import java.net.URL;
  */
 public class SalesPostServlet extends SAMLServlet {
     public static final String DEPLOYMENT_NAME = "sales-post";
+    public static final String CLIENT_NAME = "http://localhost:8081/sales-post/";
 
     @ArquillianResource
     @OperateOnDeployment(DEPLOYMENT_NAME)
diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java
index e967a33..7b82f9b 100644
--- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java
+++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java
@@ -35,6 +35,11 @@ public class ClientAttributeUpdater {
         return this;
     }
 
+    public ClientAttributeUpdater setConsentRequired(Boolean consentRequired) {
+        rep.setConsentRequired(consentRequired);
+        return this;
+    }
+
     public ClientAttributeUpdater setFrontchannelLogout(Boolean frontchannelLogout) {
         rep.setFrontchannelLogout(frontchannelLogout);
         return this;
diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java
index 77f6281..649fa7a 100644
--- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java
+++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java
@@ -656,6 +656,50 @@ public abstract class AbstractSAMLServletsAdapterTest extends AbstractServletsAd
     }
 
     @Test
+    public void salesPostEncRejectConsent() throws Exception {
+        ClientRepresentation salesPostEncClient = testRealmResource().clients().findByClientId(SalesPostEncServlet.CLIENT_NAME).get(0);
+        try (Closeable client = new ClientAttributeUpdater(testRealmResource().clients().get(salesPostEncClient.getId()))
+          .setConsentRequired(true)
+          .update()) {
+            new SamlClientBuilder()
+              .navigateTo(salesPostEncServletPage.toString())
+              .processSamlResponse(Binding.POST).build()
+              .login().user(bburkeUser).build()
+              .consentRequired().approveConsent(false).build()
+              .processSamlResponse(Binding.POST).build()
+
+              .execute(r -> {
+                  assertThat(r, statusCodeIsHC(Response.Status.OK));
+                  assertThat(r, bodyHC(containsString("urn:oasis:names:tc:SAML:2.0:status:RequestDenied")));  // TODO: revisit - should the HTTP status be 403 too?
+              });
+        } finally {
+            salesPostEncServletPage.logout();
+        }
+    }
+
+    @Test
+    public void salesPostRejectConsent() throws Exception {
+        ClientRepresentation salesPostClient = testRealmResource().clients().findByClientId(SalesPostServlet.CLIENT_NAME).get(0);
+        try (Closeable client = new ClientAttributeUpdater(testRealmResource().clients().get(salesPostClient.getId()))
+          .setConsentRequired(true)
+          .update()) {
+            new SamlClientBuilder()
+              .navigateTo(salesPostServletPage.toString())
+              .processSamlResponse(Binding.POST).build()
+              .login().user(bburkeUser).build()
+              .consentRequired().approveConsent(false).build()
+              .processSamlResponse(Binding.POST).build()
+
+              .execute(r -> {
+                  assertThat(r, statusCodeIsHC(Response.Status.OK));
+                  assertThat(r, bodyHC(containsString("urn:oasis:names:tc:SAML:2.0:status:RequestDenied")));  // TODO: revisit - should the HTTP status be 403 too?
+              });
+        } finally {
+            salesPostServletPage.logout();
+        }
+    }
+
+    @Test
     public void salesPostPassiveTest() {
         salesPostPassiveServletPage.navigateTo();