From 90496f62ac5ea76403a20b5456a59833b4707c1c Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 10 Feb 2015 13:54:14 +0100 Subject: [PATCH] KEYCLOAK-1035 Brokered identity linked by account management may not be recognized during login --- .../model/AccountFederatedIdentityBean.java | 8 ++- .../theme/account/base/federatedIdentity.ftl | 4 +- .../AuthenticationBrokerResource.java | 57 +++++++++++-------- .../broker/AbstractIdentityProviderTest.java | 37 ++++++++++++ .../pages/AccountFederatedIdentityPage.java | 42 ++++++++++++++ .../broker-test/test-realm-with-broker.json | 5 +- 6 files changed, 122 insertions(+), 31 deletions(-) create mode 100644 testsuite/integration/src/test/java/org/keycloak/testsuite/pages/AccountFederatedIdentityPage.java diff --git a/forms/account-freemarker/src/main/java/org/keycloak/account/freemarker/model/AccountFederatedIdentityBean.java b/forms/account-freemarker/src/main/java/org/keycloak/account/freemarker/model/AccountFederatedIdentityBean.java index 66f2d8d22a..f8d10becd2 100755 --- a/forms/account-freemarker/src/main/java/org/keycloak/account/freemarker/model/AccountFederatedIdentityBean.java +++ b/forms/account-freemarker/src/main/java/org/keycloak/account/freemarker/model/AccountFederatedIdentityBean.java @@ -49,7 +49,7 @@ public class AccountFederatedIdentityBean { .queryParam("stateChecker", stateChecker) .build().toString(); - FederatedIdentityEntry entry = new FederatedIdentityEntry(identity, provider.getName(), actionUrl); + FederatedIdentityEntry entry = new FederatedIdentityEntry(identity, provider.getId(), provider.getName(), actionUrl); this.identities.add(entry); } } @@ -78,17 +78,19 @@ public class AccountFederatedIdentityBean { public class FederatedIdentityEntry { private FederatedIdentityModel federatedIdentityModel; + private final String providerId; private final String providerName; private final String actionUrl; - public FederatedIdentityEntry(FederatedIdentityModel federatedIdentityModel, String providerName, String actionUrl) { + public FederatedIdentityEntry(FederatedIdentityModel federatedIdentityModel, String providerId, String providerName, String actionUrl) { this.federatedIdentityModel = federatedIdentityModel; + this.providerId = providerId; this.providerName = providerName; this.actionUrl = actionUrl; } public String getProviderId() { - return federatedIdentityModel != null ? federatedIdentityModel.getIdentityProvider() : null; + return providerId; } public String getProviderName() { diff --git a/forms/common-themes/src/main/resources/theme/account/base/federatedIdentity.ftl b/forms/common-themes/src/main/resources/theme/account/base/federatedIdentity.ftl index a4812a1810..d15ba106ff 100755 --- a/forms/common-themes/src/main/resources/theme/account/base/federatedIdentity.ftl +++ b/forms/common-themes/src/main/resources/theme/account/base/federatedIdentity.ftl @@ -19,10 +19,10 @@
<#if identity.connected> <#if federatedIdentity.removeLinkPossible> - Remove ${identity.providerName!} + Remove ${identity.providerName!} <#else> - Add ${identity.providerName!} + Add ${identity.providerName!}
diff --git a/services/src/main/java/org/keycloak/services/resources/AuthenticationBrokerResource.java b/services/src/main/java/org/keycloak/services/resources/AuthenticationBrokerResource.java index 5320eeeb6e..01af03cbce 100644 --- a/services/src/main/java/org/keycloak/services/resources/AuthenticationBrokerResource.java +++ b/services/src/main/java/org/keycloak/services/resources/AuthenticationBrokerResource.java @@ -17,6 +17,7 @@ */ package org.keycloak.services.resources; +import org.jboss.logging.Logger; import org.jboss.resteasy.spi.HttpRequest; import org.keycloak.ClientConnection; import org.keycloak.broker.provider.AuthenticationRequest; @@ -77,6 +78,8 @@ import static org.keycloak.models.UserModel.RequiredAction.UPDATE_PROFILE; @Path("/broker") public class AuthenticationBrokerResource { + private static final Logger logger = Logger.getLogger(AuthenticationBrokerResource.class); + @Context private UriInfo uriInfo; @@ -132,6 +135,7 @@ public class AuthenticationBrokerResource { return response; } } catch (Exception e) { + logger.error("Could not send authentication request to identity provider " + providerId, e); String message = "Could not send authentication request to identity provider"; event.error(message); return redirectToErrorPage(realm, message); @@ -278,6 +282,7 @@ public class AuthenticationBrokerResource { return performLocalAuthentication(realm, providerId, identity, clientCode); } catch (Exception e) { + logger.error("Could not authenticate user against provider " + providerId, e); if (session.getTransaction().isActive()) { session.getTransaction().rollback(); } @@ -333,34 +338,36 @@ public class AuthenticationBrokerResource { return Response.status(302).location(UriBuilder.fromUri(clientSession.getRedirectUri()).build()).build(); } - UserModel user = session.users().getUserByEmail(updatedIdentity.getEmail(), realm); - String errorMessage = "federatedIdentityEmailExists"; + if (federatedUser == null) { - if (user == null) { - user = session.users().getUserByUsername(updatedIdentity.getUsername(), realm); - errorMessage = "federatedIdentityUsernameExists"; - } + UserModel existingUser = session.users().getUserByEmail(updatedIdentity.getEmail(), realm); + String errorMessage = "federatedIdentityEmailExists"; - if (user == null) { - federatedUser = session.users().addUser(realm, updatedIdentity.getUsername()); - federatedUser.setEnabled(true); - federatedUser.setFirstName(updatedIdentity.getFirstName()); - federatedUser.setLastName(updatedIdentity.getLastName()); - federatedUser.setEmail(updatedIdentity.getEmail()); - - session.users().addFederatedIdentity(realm, federatedUser, federatedIdentityModel); - - event.clone().user(federatedUser).event(EventType.REGISTER) - .detail(Details.REGISTER_METHOD, authMethod) - .detail(Details.EMAIL, federatedUser.getEmail()) - .removeDetail("auth_method") - .success(); - - if (identityProviderConfig.isUpdateProfileFirstLogin()) { - federatedUser.addRequiredAction(UPDATE_PROFILE); + if (existingUser == null) { + existingUser = session.users().getUserByUsername(updatedIdentity.getUsername(), realm); + errorMessage = "federatedIdentityUsernameExists"; } - } else { - if (federatedUser == null) { + + if (existingUser == null) { + logger.debug("Creating user " + updatedIdentity.getUsername() + " and linking to federation provider " + providerId); + federatedUser = session.users().addUser(realm, updatedIdentity.getUsername()); + federatedUser.setEnabled(true); + federatedUser.setFirstName(updatedIdentity.getFirstName()); + federatedUser.setLastName(updatedIdentity.getLastName()); + federatedUser.setEmail(updatedIdentity.getEmail()); + + session.users().addFederatedIdentity(realm, federatedUser, federatedIdentityModel); + + event.clone().user(federatedUser).event(EventType.REGISTER) + .detail(Details.REGISTER_METHOD, authMethod) + .detail(Details.EMAIL, federatedUser.getEmail()) + .removeDetail("auth_method") + .success(); + + if (identityProviderConfig.isUpdateProfileFirstLogin()) { + federatedUser.addRequiredAction(UPDATE_PROFILE); + } + } else { return Flows.forms(session, realm, clientSession.getClient(), uriInfo) .setClientSessionCode(clientCode.getCode()) .setError(errorMessage) 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 4d3c4f4e72..15eacda854 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java @@ -33,6 +33,7 @@ import org.keycloak.representations.IDToken; import org.keycloak.testsuite.OAuthClient; import org.keycloak.testsuite.OAuthClient.AccessTokenResponse; import org.keycloak.testsuite.broker.util.UserSessionStatusServlet.UserSessionStatus; +import org.keycloak.testsuite.pages.AccountFederatedIdentityPage; import org.keycloak.testsuite.pages.AccountPasswordPage; import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.LoginUpdateProfilePage; @@ -92,6 +93,9 @@ public abstract class AbstractIdentityProviderTest { @WebResource protected AccountPasswordPage changePasswordPage; + @WebResource + protected AccountFederatedIdentityPage accountFederatedIdentityPage; + private KeycloakSession session; @Before @@ -118,6 +122,7 @@ public abstract class AbstractIdentityProviderTest { @Test public void testSuccessfulAuthenticationWithoutUpdateProfile() { IdentityProviderModel identityProviderModel = getIdentityProviderModel(); + identityProviderModel.setUpdateProfileFirstLogin(false); assertSuccessfulAuthentication(identityProviderModel); } @@ -202,6 +207,38 @@ public abstract class AbstractIdentityProviderTest { assertEquals("User with email already exists. Please login to account management to link the account.", element.getText()); } + @Test + public void testAccountManagementLinkIdentity() { + // 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()); + + // Link my "pedroigor" identity with "test-user" from brokered Keycloak + IdentityProviderModel identityProviderModel = getIdentityProviderModel(); + accountFederatedIdentityPage.clickAddProvider(identityProviderModel.getId()); + + assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8082/auth/")); + this.loginPage.login("test-user", "password"); + doAfterProviderAuthentication(); + + // Assert identity linked in account management + assertTrue(accountFederatedIdentityPage.isCurrent()); + assertTrue(driver.getPageSource().contains("id=\"remove-" + identityProviderModel.getId() + "\"")); + + // Logout from account management + accountFederatedIdentityPage.logout(); + assertTrue(driver.getTitle().equals("Log in to realm-with-broker")); + + // Assert I am logged immediately to account management + loginPage.clickSocial(identityProviderModel.getId()); + doAfterProviderAuthentication(); + assertTrue(accountFederatedIdentityPage.isCurrent()); + assertTrue(driver.getPageSource().contains("id=\"remove-" + identityProviderModel.getId() + "\"")); + } + @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 new file mode 100644 index 0000000000..2e3556efd3 --- /dev/null +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/AccountFederatedIdentityPage.java @@ -0,0 +1,42 @@ +package org.keycloak.testsuite.pages; + +import javax.ws.rs.core.UriBuilder; + +import org.keycloak.services.resources.flows.Urls; +import org.keycloak.testsuite.Constants; +import org.openqa.selenium.By; + +/** + * @author Marek Posolda + */ +public class AccountFederatedIdentityPage extends AbstractAccountPage { + + public AccountFederatedIdentityPage() {}; + + private String realmName = "test"; + + public void open() { + driver.navigate().to(getPath()); + } + + public void realm(String realmName) { + this.realmName = realmName; + } + + public String getPath() { + return Urls.accountFederatedIdentityPage(UriBuilder.fromUri(Constants.AUTH_SERVER_ROOT).build(), realmName).toString(); + } + + @Override + public boolean isCurrent() { + return driver.getTitle().contains("Account Management") && driver.getPageSource().contains("Federated Identities"); + } + + public void clickAddProvider(String providerId) { + driver.findElement(By.id("add-" + providerId)).click(); + } + + public void clickRemoveProvider(String providerId) { + driver.findElement(By.id("remove-" + providerId)).click(); + } +} diff --git a/testsuite/integration/src/test/resources/broker-test/test-realm-with-broker.json b/testsuite/integration/src/test/resources/broker-test/test-realm-with-broker.json index a49a6a5026..8b372aeb7d 100755 --- a/testsuite/integration/src/test/resources/broker-test/test-realm-with-broker.json +++ b/testsuite/integration/src/test/resources/broker-test/test-realm-with-broker.json @@ -163,7 +163,10 @@ { "type" : "password", "value" : "password" } ], - "realmRoles": ["manager"] + "realmRoles": ["manager"], + "applicationRoles": { + "account": [ "manage-account" ] + } } ], "applications": [