Merge pull request #2577 from mposolda/master

KEYCLOAK-2769 Better error handling of expired code in IdentityBroker…

services/src/main/java/org/keycloak/services/messages/Messages.java

@@ -154,6 +154,8 @@ public class Messages {
 
     public static final String IDENTITY_PROVIDER_LINK_SUCCESS = "identityProviderLinkSuccess";
 
+    public static final String STALE_CODE = "staleCodeMessage";
+
     public static final String IDENTITY_PROVIDER_NOT_UNIQUE = "identityProviderNotUniqueMessage";
 
     public static final String REALM_SUPPORTS_NO_CREDENTIALS = "realmSupportsNoCredentialsMessage";

services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java

@@ -142,7 +142,12 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal
         }
 
         try {
-            ClientSessionCode clientSessionCode = parseClientSessionCode(code);
+            ParsedCodeContext parsedCode = parseClientSessionCode(code);
+            if (parsedCode.response != null) {
+                return parsedCode.response;
+            }
+
+            ClientSessionCode clientSessionCode = parsedCode.clientSessionCode;
             IdentityProvider identityProvider = getIdentityProvider(session, realmModel, providerId);
             Response response = identityProvider.performLogin(createAuthenticationRequest(providerId, clientSessionCode));
 
@@ -241,14 +246,14 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal
     }
 
     public Response authenticated(BrokeredIdentityContext context) {
-        ClientSessionCode clientCode = null;
         IdentityProviderModel identityProviderConfig = context.getIdpConfig();
-        try {
-            clientCode = parseClientSessionCode(context.getCode());
-        } catch (Exception e) {
-            return redirectToErrorPage(Messages.IDENTITY_PROVIDER_AUTHENTICATION_FAILED, e, identityProviderConfig.getProviderId());
 
+        ParsedCodeContext parsedCode = parseClientSessionCode(context.getCode());
+        if (parsedCode.response != null) {
+            return parsedCode.response;
         }
+        ClientSessionCode clientCode = parsedCode.clientSessionCode;
+
         String providerId = identityProviderConfig.getAlias();
         if (!identityProviderConfig.isStoreToken()) {
             if (isDebugEnabled()) {
@@ -325,8 +330,11 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal
     @GET
     @Path("/after-first-broker-login")
     public Response afterFirstBrokerLogin(@QueryParam("code") String code) {
-        ClientSessionCode clientCode = parseClientSessionCode(code);
+        ParsedCodeContext parsedCode = parseClientSessionCode(code);
-        ClientSessionModel clientSession = clientCode.getClientSession();
+        if (parsedCode.response != null) {
+            return parsedCode.response;
+        }
+        ClientSessionModel clientSession = parsedCode.clientSessionCode.getClientSession();
 
         try {
             this.event.detail(Details.CODE_ID, clientSession.getId())
@@ -439,8 +447,11 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal
     @GET
     @Path("/after-post-broker-login")
     public Response afterPostBrokerLoginFlow(@QueryParam("code") String code) {
-        ClientSessionCode clientCode = parseClientSessionCode(code);
+        ParsedCodeContext parsedCode = parseClientSessionCode(code);
-        ClientSessionModel clientSession = clientCode.getClientSession();
+        if (parsedCode.response != null) {
+            return parsedCode.response;
+        }
+        ClientSessionModel clientSession = parsedCode.clientSessionCode.getClientSession();
 
         try {
             SerializedBrokeredIdentityContext serializedCtx = SerializedBrokeredIdentityContext.readFromClientSession(clientSession, PostBrokerLoginConstants.PBL_BROKERED_IDENTITY_CONTEXT);
@@ -526,20 +537,23 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal
 
     @Override
     public Response cancelled(String code) {
-        ClientSessionCode clientCode = ClientSessionCode.parse(code, this.session, this.realmModel);
+        ParsedCodeContext parsedCode = parseClientSessionCode(code);
-        if (clientCode == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) {
+        if (parsedCode.response != null) {
-            return redirectToErrorPage(Messages.INVALID_CODE);
+            return parsedCode.response;
         }
+        ClientSessionCode clientCode = parsedCode.clientSessionCode;
 
         return browserAuthentication(clientCode.getClientSession(), null);
     }
 
     @Override
     public Response error(String code, String message) {
-        ClientSessionCode clientCode = ClientSessionCode.parse(code, this.session, this.realmModel);
+        ParsedCodeContext parsedCode = parseClientSessionCode(code);
-        if (clientCode == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) {
+        if (parsedCode.response != null) {
-            return redirectToErrorPage(Messages.INVALID_CODE);
+            return parsedCode.response;
         }
+        ClientSessionCode clientCode = parsedCode.clientSessionCode;
+
         return browserAuthentication(clientCode.getClientSession(), message);
     }
 
@@ -600,36 +614,41 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal
 
     }
 
-    private ClientSessionCode parseClientSessionCode(String code) {
+    private ParsedCodeContext parseClientSessionCode(String code) {
         ClientSessionCode clientCode = ClientSessionCode.parse(code, this.session, this.realmModel);
 
-        if (clientCode != null && clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) {
+        if (clientCode != null) {
             ClientSessionModel clientSession = clientCode.getClientSession();
 
-            if (clientSession != null) {
+            if (clientSession.getUserSession() != null) {
-                ClientModel client = clientSession.getClient();
+                this.event.session(clientSession.getUserSession());
+            }
 
-                if (client == null) {
+            ClientModel client = clientSession.getClient();
-                    throw new IdentityBrokerException("Invalid client");
+
-                }
+            if (client != null) {
 
                 logger.debugf("Got authorization code from client [%s].", client.getClientId());
                 this.event.client(client);
                 this.session.getContext().setClient(client);
 
-                if (clientSession.getUserSession() != null) {
+                if (!clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) {
-                    this.event.session(clientSession.getUserSession());
+                    logger.debugf("Authorization code is not valid. Client session ID: %s, Client session's action: %s", clientSession.getId(), clientSession.getAction());
+                    Response staleCodeError = redirectToErrorPage(Messages.STALE_CODE);
+                    return ParsedCodeContext.response(staleCodeError);
                 }
-            }
 
-            if (isDebugEnabled()) {
+                if (isDebugEnabled()) {
-                logger.debugf("Authorization code is valid.");
+                    logger.debugf("Authorization code is valid.");
-            }
+                }
 
-            return clientCode;
+                return ParsedCodeContext.clientSessionCode(clientCode);
+            }
         }
 
-        throw new IdentityBrokerException("Invalid code, please login again through your client.");
+        logger.debugf("Authorization code is not valid. Code: %s", code);
+        Response staleCodeError = redirectToErrorPage(Messages.STALE_CODE);
+        return ParsedCodeContext.response(staleCodeError);
     }
 
     private AuthenticationRequest createAuthenticationRequest(String providerId, ClientSessionCode clientSessionCode) {
@@ -804,4 +823,22 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal
             this.session.getTransaction().rollback();
         }
     }
+
+
+    private static class ParsedCodeContext {
+        private ClientSessionCode clientSessionCode;
+        private Response response;
+
+        public static ParsedCodeContext clientSessionCode(ClientSessionCode clientSessionCode) {
+            ParsedCodeContext ctx = new ParsedCodeContext();
+            ctx.clientSessionCode = clientSessionCode;
+            return ctx;
+        }
+
+        public static ParsedCodeContext response(Response response) {
+            ParsedCodeContext ctx = new ParsedCodeContext();
+            ctx.response = response;
+            return ctx;
+        }
+    }
 }

testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java

@@ -49,6 +49,7 @@ import java.util.List;
 import javax.ws.rs.core.UriBuilder;
 
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 /**
@@ -147,7 +148,7 @@ public class OIDCKeyCloakServerBrokerBasicTest extends AbstractKeycloakIdentityP
 
 
     @Test
-    public void testAccessDeniedError() throws Exception {
+    public void testConsentDeniedWithExpiredClientSession() throws Exception {
         // Disable update profile
         setUpdateProfileFirstLogin(IdentityProviderRepresentation.UPFLM_OFF);
 
@@ -174,9 +175,26 @@ public class OIDCKeyCloakServerBrokerBasicTest extends AbstractKeycloakIdentityP
         // 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
+        // Set time offset
         Time.setOffset(60);
         try {
+            // User rejected consent
+            grantPage.assertCurrent();
+            grantPage.cancel();
+
+            // Assert error page with backToApplication link displayed
+            errorPage.assertCurrent();
+            errorPage.clickBackToApplication();
+
+            assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8081/auth/realms/realm-with-broker/protocol/openid-connect/auth"));
+
+
+            // Login to broker again
+            loginIDP("test-user");
+
+            // Set time offset and manually remove expiredSessions TODO: Will require custom endpoint when migrate to integration-arquillian
+            Time.setOffset(120);
+
             brokerServerRule.stopSession(this.session, true);
             this.session = brokerServerRule.startSession();
 
@@ -189,8 +207,14 @@ public class OIDCKeyCloakServerBrokerBasicTest extends AbstractKeycloakIdentityP
             grantPage.assertCurrent();
             grantPage.cancel();
 
-            // Assert classic error page ( We're sorry ) displayed. TODO: Should be improved? See KEYCLOAK-2740
+            // Assert error page without backToApplication link (clientSession expired and was removed on the server)
             errorPage.assertCurrent();
+            try {
+                errorPage.clickBackToApplication();
+                fail("Not expected to have link backToApplication available");
+            } catch (NoSuchElementException nsee) {
+                // Expected;
+            }
 
         } finally {
             Time.setOffset(0);

testsuite/integration/src/test/java/org/keycloak/testsuite/pages/ErrorPage.java

@@ -16,8 +16,10 @@
  */
 package org.keycloak.testsuite.pages;
 
+import org.junit.Assert;
 import org.keycloak.testsuite.OAuthClient;
 import org.keycloak.testsuite.rule.WebResource;
+import org.openqa.selenium.By;
 import org.openqa.selenium.WebElement;
 import org.openqa.selenium.support.FindBy;
 
@@ -32,10 +34,17 @@ public class ErrorPage extends AbstractPage {
     @FindBy(className = "instruction")
     private WebElement errorMessage;
 
+    @FindBy(id = "backToApplication")
+    private WebElement backToApplicationLink;
+
     public String getError() {
         return errorMessage.getText();
     }
 
+    public void clickBackToApplication() {
+        backToApplicationLink.click();
+    }
+
     public boolean isCurrent() {
         return driver.getTitle().equals("We're sorry...");
     }

themes/src/main/resources/theme/base/login/error.ftl

@@ -8,7 +8,7 @@
         <div id="kc-error-message">
             <p class="instruction">${message.summary}</p>
             <#if client?? && client.baseUrl?has_content>
-                <p><a href="${client.baseUrl}">${msg("backToApplication")}</a></p>
+                <p><a id="backToApplication" href="${client.baseUrl}">${msg("backToApplication")}</a></p>
             </#if>
         </div>
     </#if>

themes/src/main/resources/theme/base/login/messages/messages_en.properties

@@ -202,6 +202,7 @@ invalidCodeMessage=An error occurred, please login again through your applicatio
 identityProviderUnexpectedErrorMessage=Unexpected error when authenticating with identity provider
 identityProviderNotFoundMessage=Could not find an identity provider with the identifier.
 identityProviderLinkSuccess=Your account was successfully linked with {0} account {1} .
+staleCodeMessage=This page is no longer valid, please go back to your application and login again
 realmSupportsNoCredentialsMessage=Realm does not support any credential type.
 identityProviderNotUniqueMessage=Realm supports multiple identity providers. Could not determine which identity provider should be used to authenticate with.
 emailVerifiedMessage=Your email address has been verified.