Merge pull request #2564 from mposolda/master

KEYCLOAK-2769 Fix NPE during 'Identity Broker cancelled' and instead …

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);

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;

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<ClientRepresentation> 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();