From 4288260aa679ed51356269ac4f9b637ca6d8fed4 Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 13 Nov 2015 17:37:08 +0100 Subject: [PATCH] KEYCLOAK-1822 Don't redirect to login theme when error during link identity in account mgmt. --- .../account/messages/messages_en.properties | 1 + .../login/messages/messages_en.properties | 1 - .../keycloak/models/utils/FormMessage.java | 3 ++ .../services/resources/AccountService.java | 17 +++++++++++ .../resources/IdentityBrokerService.java | 23 +++++++++++---- .../AbstractKeycloakIdentityProviderTest.java | 28 +++++++++++++++++++ .../pages/AccountFederatedIdentityPage.java | 9 ++++++ 7 files changed, 76 insertions(+), 6 deletions(-) diff --git a/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties b/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties index 3d2eeea5fa..49801041cc 100755 --- a/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties +++ b/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties @@ -133,6 +133,7 @@ federatedIdentityLinkNotActiveMessage=This identity is not active anymore. federatedIdentityRemovingLastProviderMessage=You can''t remove last federated identity as you don''t have password. identityProviderRedirectErrorMessage=Failed to redirect to identity provider. identityProviderRemovedMessage=Identity provider removed successfully. +identityProviderAlreadyLinkedMessage=Federated identity returned by {0} is already linked to another user. accountDisabledMessage=Account is disabled, contact admin. diff --git a/forms/common-themes/src/main/resources/theme/base/login/messages/messages_en.properties b/forms/common-themes/src/main/resources/theme/base/login/messages/messages_en.properties index 803ef2dcc0..68853082ca 100644 --- a/forms/common-themes/src/main/resources/theme/base/login/messages/messages_en.properties +++ b/forms/common-themes/src/main/resources/theme/base/login/messages/messages_en.properties @@ -185,7 +185,6 @@ resetCredentialNotAllowedMessage=Reset Credential not allowed permissionNotApprovedMessage=Permission not approved. noRelayStateInResponseMessage=No relay state in response from identity provider. -identityProviderAlreadyLinkedMessage=The identity returned by the identity provider is already linked to another user. insufficientPermissionMessage=Insufficient permissions to link identities. couldNotProceedWithAuthenticationRequestMessage=Could not proceed with authentication request to identity provider. couldNotObtainTokenMessage=Could not obtain token from identity provider. diff --git a/model/api/src/main/java/org/keycloak/models/utils/FormMessage.java b/model/api/src/main/java/org/keycloak/models/utils/FormMessage.java index b840de6dee..9f5c5deeb6 100755 --- a/model/api/src/main/java/org/keycloak/models/utils/FormMessage.java +++ b/model/api/src/main/java/org/keycloak/models/utils/FormMessage.java @@ -23,6 +23,9 @@ public class FormMessage { private String message; private Object[] parameters; + public FormMessage() { + } + /** * Create message. * diff --git a/services/src/main/java/org/keycloak/services/resources/AccountService.java b/services/src/main/java/org/keycloak/services/resources/AccountService.java index 2bebec3bfb..af24034223 100755 --- a/services/src/main/java/org/keycloak/services/resources/AccountService.java +++ b/services/src/main/java/org/keycloak/services/resources/AccountService.java @@ -61,6 +61,7 @@ import org.keycloak.services.messages.Messages; import org.keycloak.services.util.ResolveRelative; import org.keycloak.services.validation.Validation; import org.keycloak.common.util.UriUtils; +import org.keycloak.util.JsonSerialization; import javax.ws.rs.Consumes; import javax.ws.rs.GET; @@ -74,6 +75,8 @@ import javax.ws.rs.core.Response; import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriInfo; import javax.ws.rs.core.Variant; + +import java.io.IOException; import java.lang.reflect.Method; import java.net.URI; import java.util.HashSet; @@ -116,6 +119,9 @@ public class AccountService extends AbstractSecuredLocalService { public static final String KEYCLOAK_STATE_CHECKER = "KEYCLOAK_STATE_CHECKER"; + // Used when some other context (ie. IdentityBrokerService) wants to forward error to account management and display it here + public static final String ACCOUNT_MGMT_FORWARDED_ERROR_NOTE = "ACCOUNT_MGMT_FORWARDED_ERROR"; + private final AppAuthManager authManager; private EventBuilder event; private AccountProvider account; @@ -217,6 +223,17 @@ public class AccountService extends AbstractSecuredLocalService { setReferrerOnPage(); + String forwardedError = auth.getClientSession().getNote(ACCOUNT_MGMT_FORWARDED_ERROR_NOTE); + if (forwardedError != null) { + try { + FormMessage errorMessage = JsonSerialization.readValue(forwardedError, FormMessage.class); + account.setError(errorMessage.getMessage(), errorMessage.getParameters()); + auth.getClientSession().removeNote(ACCOUNT_MGMT_FORWARDED_ERROR_NOTE); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } + } + return account.createResponse(page); } else { return login(path); 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 c8784bda2f..5912536c11 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java @@ -65,6 +65,7 @@ import org.keycloak.services.Urls; import org.keycloak.services.validation.Validation; import org.keycloak.social.SocialIdentityProvider; import org.keycloak.common.util.ObjectUtil; +import org.keycloak.util.JsonSerialization; import javax.ws.rs.*; import javax.ws.rs.core.Context; @@ -74,6 +75,7 @@ import javax.ws.rs.core.Response.Status; import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriInfo; +import java.io.IOException; import java.net.URI; import java.util.ArrayList; import java.util.HashMap; @@ -419,7 +421,6 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal return finishBrokerAuthentication(context, federatedUser, clientSession, providerId); } } catch (Exception e) { - // TODO? return redirectToErrorPage(Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR, e); } } @@ -465,7 +466,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal this.event.event(EventType.FEDERATED_IDENTITY_LINK); if (federatedUser != null) { - return redirectToErrorPage(Messages.IDENTITY_PROVIDER_ALREADY_LINKED, context.getIdpConfig().getAlias()); + return redirectToAccountErrorPage(clientSession, Messages.IDENTITY_PROVIDER_ALREADY_LINKED, context.getIdpConfig().getAlias()); } UserModel authenticatedUser = clientSession.getUserSession().getUser(); @@ -475,12 +476,10 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal } if (!authenticatedUser.isEnabled()) { - fireErrorEvent(Errors.USER_DISABLED); - return redirectToErrorPage(Messages.ACCOUNT_DISABLED); + return redirectToAccountErrorPage(clientSession, Messages.ACCOUNT_DISABLED); } if (!authenticatedUser.hasRole(this.realmModel.getClientByClientId(ACCOUNT_MANAGEMENT_CLIENT_ID).getRole(MANAGE_ACCOUNT))) { - fireErrorEvent(Errors.NOT_ALLOWED); return redirectToErrorPage(Messages.INSUFFICIENT_PERMISSION); } @@ -581,6 +580,20 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal return ErrorPage.error(this.session, message, parameters); } + private Response redirectToAccountErrorPage(ClientSessionModel clientSession, String message, Object ... parameters) { + fireErrorEvent(message); + + FormMessage errorMessage = new FormMessage(message, parameters); + try { + String serializedError = JsonSerialization.writeValueAsString(errorMessage); + clientSession.setNote(AccountService.ACCOUNT_MGMT_FORWARDED_ERROR_NOTE, serializedError); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } + + return Response.status(302).location(UriBuilder.fromUri(clientSession.getRedirectUri()).build()).build(); + } + private Response redirectToLoginPage(Throwable t, ClientSessionCode clientCode) { String message = t.getMessage(); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java index 904caf6282..1d2e5b68d4 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java @@ -389,6 +389,34 @@ public abstract class AbstractKeycloakIdentityProviderTest extends AbstractIdent this.updateProfilePage.assertCurrent(); } + + // KEYCLOAK-1822 + @Test + public void testAccountManagementLinkedIdentityAlreadyExists() { + // Login as "test-user" through broker + IdentityProviderModel identityProvider = getIdentityProviderModel(); + assertSuccessfulAuthentication(identityProvider, "test-user", "test-user@localhost", false); + + // Login as pedroigor to account management + accountFederatedIdentityPage.realm("realm-with-broker"); + accountFederatedIdentityPage.open(); + assertTrue(driver.getTitle().equals("Log in to realm-with-broker")); + loginPage.login("pedroigor", "password"); + assertTrue(accountFederatedIdentityPage.isCurrent()); + + // Try to link my "pedroigor" identity with "test-user" from brokered Keycloak. + accountFederatedIdentityPage.clickAddProvider(identityProvider.getAlias()); + + assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8082/auth/")); + this.loginPage.login("test-user", "password"); + doAfterProviderAuthentication(); + + // Error is displayed in account management because federated identity"test-user" already linked to local account "test-user" + assertTrue(accountFederatedIdentityPage.isCurrent()); + assertEquals("Federated identity returned by " + getProviderId() + " is already linked to another user.", accountFederatedIdentityPage.getError()); + } + + @Test(expected = NoSuchElementException.class) public void testIdentityProviderNotAllowed() { this.driver.navigate().to("http://localhost:8081/test-app/"); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/AccountFederatedIdentityPage.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/AccountFederatedIdentityPage.java index 4c0279848f..54a5cbb4ba 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/AccountFederatedIdentityPage.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/AccountFederatedIdentityPage.java @@ -5,12 +5,17 @@ import javax.ws.rs.core.UriBuilder; import org.keycloak.services.Urls; import org.keycloak.testsuite.Constants; import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; +import org.openqa.selenium.support.FindBy; /** * @author Marek Posolda */ public class AccountFederatedIdentityPage extends AbstractAccountPage { + @FindBy(className = "alert-error") + private WebElement errorMessage; + public AccountFederatedIdentityPage() {}; private String realmName = "test"; @@ -39,4 +44,8 @@ public class AccountFederatedIdentityPage extends AbstractAccountPage { public void clickRemoveProvider(String providerId) { driver.findElement(By.id("remove-" + providerId)).click(); } + + public String getError() { + return errorMessage.getText(); + } }