diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java index 39a9d2efba..38e57cb256 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -19,6 +19,7 @@ package org.keycloak.broker.saml; import org.jboss.logging.Logger; import org.jboss.resteasy.annotations.cache.NoCache; + import org.keycloak.broker.provider.BrokeredIdentityContext; import org.keycloak.broker.provider.IdentityBrokerException; import org.keycloak.broker.provider.IdentityProvider; @@ -78,10 +79,13 @@ import java.security.Key; import java.security.cert.X509Certificate; import java.util.LinkedList; import java.util.List; + import org.keycloak.rotation.HardcodedKeyLocator; import org.keycloak.rotation.KeyLocator; import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; +import java.util.*; + /** * @author Bill Burke * @version $Revision: 1 $ @@ -333,6 +337,13 @@ public class SAMLEndpoint { try { KeyManager.ActiveRsaKey keys = session.keys().getActiveRsaKey(realm); + if (! isSuccessfulSamlResponse(responseType)) { + String statusMessage = responseType.getStatus() == null ? Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR : responseType.getStatus().getStatusMessage(); + return callback.error(relayState, statusMessage); + } + if (responseType.getAssertions() == null || responseType.getAssertions().isEmpty()) { + return callback.error(relayState, Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR); + } AssertionType assertion = AssertionUtil.getAssertion(responseType, keys.getPrivateKey()); SubjectType subject = assertion.getSubject(); SubjectType.STSubType subType = subject.getSubType(); @@ -395,6 +406,13 @@ public class SAMLEndpoint { } + private boolean isSuccessfulSamlResponse(ResponseType responseType) { + return responseType != null + && responseType.getStatus() != null + && responseType.getStatus().getStatusCode() != null + && responseType.getStatus().getStatusCode().getValue() != null + && Objects.equals(responseType.getStatus().getStatusCode().getValue().toString(), JBossSAMLURIConstants.STATUS_SUCCESS.get()); + } public Response handleSamlResponse(String samlResponse, String relayState, String clientId) { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java index b8b244d545..5110b328cc 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java @@ -28,10 +28,17 @@ public class ConsentPage extends AbstractPage { @FindBy(id = "kc-login") private WebElement submitButton; + @FindBy(id = "kc-cancel") + private WebElement cancelButton; + public void confirm() { submitButton.click(); } + public void cancel() { + cancelButton.click(); + } + @Override public boolean isCurrent() { return driver.getTitle().equalsIgnoreCase("grant access"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java index 217a4e7550..c2c628d942 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java @@ -20,10 +20,7 @@ package org.keycloak.testsuite.broker; import java.util.List; import org.jboss.arquillian.graphene.page.Page; -import org.junit.Before; -import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.representations.idm.RealmRepresentation; -import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.Retry; @@ -35,8 +32,6 @@ import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.UpdateAccountInformationPage; import org.openqa.selenium.TimeoutException; -import static org.keycloak.testsuite.admin.ApiUtil.createUserWithAdminClient; -import static org.keycloak.testsuite.admin.ApiUtil.resetUserPassword; import static org.keycloak.testsuite.broker.BrokerTestTools.encodeUrl; import static org.keycloak.testsuite.broker.BrokerTestTools.waitForPage; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java index 8950d1b087..6f3314fb29 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java @@ -2,28 +2,31 @@ package org.keycloak.testsuite.broker; import org.junit.Before; import org.junit.Test; + import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.admin.client.resource.UsersResource; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.Assert; -import org.keycloak.testsuite.util.RealmBuilder; +import org.keycloak.testsuite.pages.ConsentPage; +import org.keycloak.testsuite.util.*; import org.openqa.selenium.TimeoutException; import java.util.List; +import java.util.concurrent.TimeUnit; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.keycloak.testsuite.admin.ApiUtil.createUserWithAdminClient; import static org.keycloak.testsuite.admin.ApiUtil.resetUserPassword; import static org.keycloak.testsuite.broker.BrokerTestConstants.USER_EMAIL; -import static org.keycloak.testsuite.broker.BrokerTestTools.*; import static org.keycloak.testsuite.util.MailAssert.assertEmailAndGetUrl; -import org.keycloak.testsuite.util.MailServer; -import org.keycloak.testsuite.util.MailServerConfiguration; -import org.keycloak.testsuite.util.UserBuilder; + +import org.jboss.arquillian.graphene.page.Page; + +import static org.keycloak.testsuite.broker.BrokerTestTools.*; public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { @@ -256,6 +259,44 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { assertEquals("Account is disabled, contact admin.", errorPage.getError()); } + @Page + ConsentPage consentPage; + + // KEYCLOAK-4181 + @Test + public void loginWithExistingUserWithErrorFromProviderIdP() { + ClientRepresentation client = adminClient.realm(bc.providerRealmName()) + .clients() + .findByClientId(bc.getIDPClientIdInProviderRealm(suiteContext)) + .get(0); + + adminClient.realm(bc.providerRealmName()) + .clients() + .get(client.getId()) + .update(ClientBuilder.edit(client).consentRequired(true).build()); + + driver.navigate().to(getAccountUrl(bc.consumerRealmName())); + + log.debug("Clicking social " + bc.getIDPAlias()); + accountLoginPage.clickSocial(bc.getIDPAlias()); + + waitForPage(driver, "log in to"); + + Assert.assertTrue("Driver should be on the provider realm page right now", + driver.getCurrentUrl().contains("/auth/realms/" + bc.providerRealmName() + "/")); + + log.debug("Logging in"); + accountLoginPage.login(bc.getUserLogin(), bc.getUserPassword()); + + driver.manage().timeouts().pageLoadTimeout(30, TimeUnit.MINUTES); + + waitForPage(driver, "grant access"); + consentPage.cancel(); + + waitForPage(driver, "log in to"); + } + + protected void testSingleLogout() { log.debug("Testing single log out"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java index 0e00a7001e..5605cf6238 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java @@ -42,6 +42,11 @@ public interface BrokerConfiguration { */ String consumerRealmName(); + /** + * @return Client ID of the identity provider as set in provider realm. + */ + String getIDPClientIdInProviderRealm(SuiteContext suiteContext); + /** * @return User login name of the brokered user */ diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java index aa3e56efe3..61664a9fc4 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java @@ -50,6 +50,7 @@ public class KcOidcBrokerConfiguration implements BrokerConfiguration { public List createProviderClients(SuiteContext suiteContext) { ClientRepresentation client = new ClientRepresentation(); client.setId(CLIENT_ID); + client.setClientId(getIDPClientIdInProviderRealm(suiteContext)); client.setName(CLIENT_ID); client.setSecret(CLIENT_SECRET); client.setEnabled(true); @@ -123,6 +124,11 @@ public class KcOidcBrokerConfiguration implements BrokerConfiguration { return USER_LOGIN; } + @Override + public String getIDPClientIdInProviderRealm(SuiteContext suiteContext) { + return CLIENT_ID; + } + @Override public String getUserPassword() { return USER_PASSWORD; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java index 7da71ad233..e5f5b8df81 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java @@ -54,7 +54,7 @@ public class KcSamlBrokerConfiguration implements BrokerConfiguration { public List createProviderClients(SuiteContext suiteContext) { ClientRepresentation client = new ClientRepresentation(); - client.setClientId(getAuthRoot(suiteContext) + "/auth/realms/" + REALM_CONS_NAME); + client.setClientId(getIDPClientIdInProviderRealm(suiteContext)); client.setEnabled(true); client.setProtocol(IDP_SAML_PROVIDER_ID); client.setRedirectUris(Collections.singletonList( @@ -156,6 +156,11 @@ public class KcSamlBrokerConfiguration implements BrokerConfiguration { return REALM_CONS_NAME; } + @Override + public String getIDPClientIdInProviderRealm(SuiteContext suiteContext) { + return getAuthRoot(suiteContext) + "/auth/realms/" + consumerRealmName(); + } + @Override public String getUserLogin() { return USER_LOGIN; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java index cd315f3e09..b4825cdb38 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java @@ -15,8 +15,6 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { public static class KcSamlSignedBrokerConfiguration extends KcSamlBrokerConfiguration { - public static final KcSamlSignedBrokerConfiguration INSTANCE = new KcSamlSignedBrokerConfiguration(); - @Override public RealmRepresentation createProviderRealm() { RealmRepresentation realm = super.createProviderRealm(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java index 9de5ae2bda..12fe4a1189 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java @@ -61,6 +61,11 @@ public class ClientBuilder { return this; } + public ClientBuilder consentRequired(boolean consentRequired) { + rep.setConsentRequired(consentRequired); + return this; + } + public ClientBuilder publicClient() { rep.setPublicClient(true); return this;