From ee9c87877fc1211dc2fbd476866610920388555c Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 8 Apr 2016 19:06:52 +0200 Subject: [PATCH] KEYCLOAK-2769 Fix NPE during 'Identity Broker cancelled' and instead show keycloak 'we are sorry' page --- .../resources/IdentityBrokerService.java | 5 +- .../broker/AbstractIdentityProviderTest.java | 4 ++ .../OIDCKeyCloakServerBrokerBasicTest.java | 68 ++++++++++++++++++- 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java index 09c2218657..227ebfa4c2 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java @@ -16,7 +16,6 @@ */ package org.keycloak.services.resources; -import org.jboss.resteasy.annotations.cache.NoCache; import org.jboss.resteasy.spi.HttpRequest; import org.jboss.resteasy.spi.ResteasyProviderFactory; import org.keycloak.OAuth2Constants; @@ -528,7 +527,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal @Override public Response cancelled(String code) { ClientSessionCode clientCode = ClientSessionCode.parse(code, this.session, this.realmModel); - if (clientCode.getClientSession() == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { + if (clientCode == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { return redirectToErrorPage(Messages.INVALID_CODE); } @@ -538,7 +537,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal @Override public Response error(String code, String message) { ClientSessionCode clientCode = ClientSessionCode.parse(code, this.session, this.realmModel); - if (clientCode.getClientSession() == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { + if (clientCode == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { return redirectToErrorPage(Messages.INVALID_CODE); } return browserAuthentication(clientCode.getClientSession(), message); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java index 52245d8775..28ad98ed68 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java @@ -40,6 +40,7 @@ import org.keycloak.testsuite.broker.util.UserSessionStatusServlet.UserSessionSt import org.keycloak.testsuite.pages.AccountFederatedIdentityPage; import org.keycloak.testsuite.pages.AccountPasswordPage; import org.keycloak.testsuite.pages.AccountUpdateProfilePage; +import org.keycloak.testsuite.pages.ErrorPage; import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.LoginUpdateProfilePage; import org.keycloak.testsuite.pages.OAuthGrantPage; @@ -114,6 +115,9 @@ public abstract class AbstractIdentityProviderTest { @WebResource protected AccountFederatedIdentityPage accountFederatedIdentityPage; + @WebResource + protected ErrorPage errorPage; + protected KeycloakSession session; protected int logoutTimeOffset = 0; diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java index 3349e8ccd6..ce4b718741 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java @@ -17,13 +17,18 @@ package org.keycloak.testsuite.broker; +import org.junit.Assert; import org.junit.ClassRule; import org.junit.Test; import org.keycloak.admin.client.Keycloak; +import org.keycloak.admin.client.resource.RealmResource; +import org.keycloak.common.util.Time; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; +import org.keycloak.models.UserModel; import org.keycloak.representations.AccessTokenResponse; +import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.services.Urls; @@ -32,6 +37,7 @@ import org.keycloak.testsuite.Constants; import org.keycloak.testsuite.pages.AccountApplicationsPage; import org.keycloak.testsuite.pages.OAuthGrantPage; import org.keycloak.testsuite.rule.AbstractKeycloakRule; +import org.keycloak.testsuite.rule.KeycloakRule; import org.keycloak.testsuite.rule.WebResource; import org.keycloak.testsuite.KeycloakServer; import org.keycloak.util.JsonSerialization; @@ -71,9 +77,6 @@ public class OIDCKeyCloakServerBrokerBasicTest extends AbstractKeycloakIdentityP } }; - @WebResource - private OAuthGrantPage grantPage; - @WebResource protected AccountApplicationsPage accountApplicationsPage; @@ -142,6 +145,65 @@ public class OIDCKeyCloakServerBrokerBasicTest extends AbstractKeycloakIdentityP keycloak.realm("realm-with-broker").identityProviders().get("kc-oidc-idp").update(idp); } + + @Test + public void testAccessDeniedError() throws Exception { + // Disable update profile + setUpdateProfileFirstLogin(IdentityProviderRepresentation.UPFLM_OFF); + + Keycloak keycloak1 = Keycloak.getInstance("http://localhost:8081/auth", "master", "admin", "admin", org.keycloak.models.Constants.ADMIN_CLI_CLIENT_ID); + Keycloak keycloak2 = Keycloak.getInstance("http://localhost:8082/auth", "master", "admin", "admin", org.keycloak.models.Constants.ADMIN_CLI_CLIENT_ID); + + // Require broker to show consent screen + RealmResource brokeredRealm = keycloak2.realm("realm-with-oidc-identity-provider"); + List clients = brokeredRealm.clients().findByClientId("broker-app"); + Assert.assertEquals(1, clients.size()); + ClientRepresentation brokerApp = clients.get(0); + brokerApp.setConsentRequired(true); + brokeredRealm.clients().get(brokerApp.getId()).update(brokerApp); + + // Change timeouts on realm-with-broker to lower values + RealmResource realmWithBroker = keycloak1.realm("realm-with-broker"); + RealmRepresentation realmBackup = realmWithBroker.toRepresentation(); + RealmRepresentation realm = realmWithBroker.toRepresentation(); + realm.setAccessCodeLifespanLogin(30);; + realm.setAccessCodeLifespan(30); + realm.setAccessCodeLifespanUserAction(30); + realmWithBroker.update(realm); + + // Login to broker + loginIDP("test-user"); + + // Set time offset and manually enforce removeExpired sessions TODO: Will need custom REST endpoints for that on integration-arquillian + Time.setOffset(60); + try { + brokerServerRule.stopSession(this.session, true); + this.session = brokerServerRule.startSession(); + + session.sessions().removeExpired(getRealm()); + + brokerServerRule.stopSession(this.session, true); + this.session = brokerServerRule.startSession(); + + // User rejected consent + grantPage.assertCurrent(); + grantPage.cancel(); + + // Assert classic error page ( We're sorry ) displayed. TODO: Should be improved? See KEYCLOAK-2740 + errorPage.assertCurrent(); + + } finally { + Time.setOffset(0); + } + + // Revert consent + brokerApp.setConsentRequired(false); + brokeredRealm.clients().get(brokerApp.getId()).update(brokerApp); + + // Revert timeouts + realmWithBroker.update(realmBackup); + } + @Test public void testSuccessfulAuthenticationWithoutUpdateProfile() { super.testSuccessfulAuthenticationWithoutUpdateProfile();