From 3e9ba71baa6a766597921281804726159ebd74a4 Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 11 Apr 2016 18:08:10 +0200 Subject: [PATCH] KEYCLOAK-2769 Better error handling of expired code in IdentityBrokerService --- .../keycloak/services/messages/Messages.java | 2 + .../resources/IdentityBrokerService.java | 99 +++++++++++++------ .../OIDCKeyCloakServerBrokerBasicTest.java | 30 +++++- .../keycloak/testsuite/pages/ErrorPage.java | 9 ++ .../main/resources/theme/base/login/error.ftl | 2 +- .../login/messages/messages_en.properties | 1 + 6 files changed, 108 insertions(+), 35 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/messages/Messages.java b/services/src/main/java/org/keycloak/services/messages/Messages.java index d9a2fe3577..c585c62c13 100755 --- a/services/src/main/java/org/keycloak/services/messages/Messages.java +++ b/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"; 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 227ebfa4c2..c0cdb59a5b 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/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); - ClientSessionModel clientSession = clientCode.getClientSession(); + ParsedCodeContext parsedCode = parseClientSessionCode(code); + 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); - ClientSessionModel clientSession = clientCode.getClientSession(); + ParsedCodeContext parsedCode = parseClientSessionCode(code); + 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); - if (clientCode == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { - return redirectToErrorPage(Messages.INVALID_CODE); + ParsedCodeContext parsedCode = parseClientSessionCode(code); + if (parsedCode.response != null) { + 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); - if (clientCode == null || !clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { - return redirectToErrorPage(Messages.INVALID_CODE); + ParsedCodeContext parsedCode = parseClientSessionCode(code); + if (parsedCode.response != null) { + 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) { - ClientModel client = clientSession.getClient(); + if (clientSession.getUserSession() != null) { + this.event.session(clientSession.getUserSession()); + } - if (client == null) { - throw new IdentityBrokerException("Invalid client"); - } + ClientModel client = clientSession.getClient(); + + 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) { - this.event.session(clientSession.getUserSession()); + if (!clientCode.isValid(AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { + 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()) { - logger.debugf("Authorization code is valid."); - } + if (isDebugEnabled()) { + 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; + } + } } 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 ce4b718741..4d72ea8489 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 @@ -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); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/ErrorPage.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/ErrorPage.java index b8213319c6..1cee3062e9 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/ErrorPage.java +++ b/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..."); } diff --git a/themes/src/main/resources/theme/base/login/error.ftl b/themes/src/main/resources/theme/base/login/error.ftl index 95de521af8..c069e26bd7 100755 --- a/themes/src/main/resources/theme/base/login/error.ftl +++ b/themes/src/main/resources/theme/base/login/error.ftl @@ -8,7 +8,7 @@

${message.summary}

<#if client?? && client.baseUrl?has_content> -

${msg("backToApplication")}

+

${msg("backToApplication")}

diff --git a/themes/src/main/resources/theme/base/login/messages/messages_en.properties b/themes/src/main/resources/theme/base/login/messages/messages_en.properties index ee8a767095..ec02b1c7ce 100755 --- a/themes/src/main/resources/theme/base/login/messages/messages_en.properties +++ b/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.