From d59a28f1cb52158569ae3188d8e590a706127a2f Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 10 Feb 2015 10:37:45 +0100 Subject: [PATCH 1/2] Added test for import kerberos model --- .../AbstractIdentityProviderModelTest.java | 2 ++ .../broker/ImportIdentityProviderTest.java | 23 +++++++++++++++++-- .../broker-test/test-realm-with-broker.json | 12 ++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderModelTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderModelTest.java index 2dd3164575..45f638cac6 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderModelTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderModelTest.java @@ -18,6 +18,7 @@ package org.keycloak.testsuite.broker; import org.junit.Before; +import org.keycloak.broker.kerberos.KerberosIdentityProviderFactory; import org.keycloak.broker.oidc.OIDCIdentityProviderFactory; import org.keycloak.broker.saml.SAMLIdentityProviderFactory; import org.keycloak.social.facebook.FacebookIdentityProviderFactory; @@ -47,6 +48,7 @@ public abstract class AbstractIdentityProviderModelTest extends AbstractModelTes this.expectedProviders.add(FacebookIdentityProviderFactory.PROVIDER_ID); this.expectedProviders.add(GitHubIdentityProviderFactory.PROVIDER_ID); this.expectedProviders.add(TwitterIdentityProviderFactory.PROVIDER_ID); + this.expectedProviders.add(KerberosIdentityProviderFactory.PROVIDER_ID); this.expectedProviders = Collections.unmodifiableSet(this.expectedProviders); } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/ImportIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/ImportIdentityProviderTest.java index 9a646cb32d..8ac0ee7e15 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/ImportIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/ImportIdentityProviderTest.java @@ -18,6 +18,9 @@ package org.keycloak.testsuite.broker; import org.junit.Test; +import org.keycloak.broker.kerberos.KerberosIdentityProvider; +import org.keycloak.broker.kerberos.KerberosIdentityProviderConfig; +import org.keycloak.broker.kerberos.KerberosIdentityProviderFactory; import org.keycloak.broker.oidc.OAuth2IdentityProviderConfig; import org.keycloak.broker.oidc.OIDCIdentityProvider; import org.keycloak.broker.oidc.OIDCIdentityProviderConfig; @@ -150,6 +153,8 @@ public class ImportIdentityProviderTest extends AbstractIdentityProviderModelTes assertGitHubIdentityProviderConfig(identityProvider); } else if (TwitterIdentityProviderFactory.PROVIDER_ID.equals(providerId)) { assertTwitterIdentityProviderConfig(identityProvider); + } else if (KerberosIdentityProviderFactory.PROVIDER_ID.equals(providerId)) { + assertKerberosIdentityProviderConfig(identityProvider); } else { continue; } @@ -243,8 +248,8 @@ public class ImportIdentityProviderTest extends AbstractIdentityProviderModelTes } private void assertTwitterIdentityProviderConfig(IdentityProviderModel identityProvider) { - TwitterIdentityProvider gitHubIdentityProvider = new TwitterIdentityProviderFactory().create(identityProvider); - OAuth2IdentityProviderConfig config = gitHubIdentityProvider.getConfig(); + TwitterIdentityProvider twitterIdentityProvider = new TwitterIdentityProviderFactory().create(identityProvider); + OAuth2IdentityProviderConfig config = twitterIdentityProvider.getConfig(); assertEquals("model-twitter", config.getId()); assertEquals(TwitterIdentityProviderFactory.PROVIDER_ID, config.getProviderId()); @@ -255,6 +260,20 @@ public class ImportIdentityProviderTest extends AbstractIdentityProviderModelTes assertEquals("clientSecret", config.getClientSecret()); } + private void assertKerberosIdentityProviderConfig(IdentityProviderModel identityProvider) { + KerberosIdentityProvider kerberosIdentityProvider = new KerberosIdentityProviderFactory().create(identityProvider); + KerberosIdentityProviderConfig config = kerberosIdentityProvider.getConfig(); + + assertEquals("model-kerberos", config.getId()); + assertEquals(KerberosIdentityProviderFactory.PROVIDER_ID, config.getProviderId()); + assertEquals("Kerberos", config.getName()); + assertEquals(true, config.isEnabled()); + assertEquals(true, config.isUpdateProfileFirstLogin()); + assertEquals("HTTP/server.domain.org@DOMAIN.ORG", config.getServerPrincipal()); + assertEquals("/etc/http.keytab", config.getKeyTab()); + assertTrue(config.getDebug()); + } + private RealmModel installTestRealm() throws IOException { RealmRepresentation realmRepresentation = loadJson("broker-test/test-realm-with-broker.json"); 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 2c9804866c..a49a6a5026 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 @@ -140,6 +140,18 @@ "userInfoUrl": "http://localhost:8082/auth/realms/realm-with-oidc-identity-provider/protocol/openid-connect/userinfo", "defaultScope": "email profile" } + }, + { + "id" : "model-kerberos", + "providerId" : "kerberos", + "name" : "Kerberos", + "enabled": true, + "updateProfileFirstLogin" : "true", + "config": { + "serverPrincipal": "HTTP/server.domain.org@DOMAIN.ORG", + "keyTab": "/etc/http.keytab", + "debug": "true" + } } ], "users": [ From 90496f62ac5ea76403a20b5456a59833b4707c1c Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 10 Feb 2015 13:54:14 +0100 Subject: [PATCH 2/2] 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": [