Merge pull request #959 from mposolda/master

KEYCLOAK-1035 Brokered identity linked by account management may not be recognized during login

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() {

forms/common-themes/src/main/resources/theme/account/base/federatedIdentity.ftl

@@ -19,10 +19,10 @@
                 <div class="col-sm-5 col-md-5">
                     <#if identity.connected>
                         <#if federatedIdentity.removeLinkPossible>
-                            <a href="${identity.actionUrl}" type="submit" class="btn btn-primary btn-lg">Remove ${identity.providerName!}</a>
+                            <a href="${identity.actionUrl}" type="submit" id="remove-${identity.providerId!}" class="btn btn-primary btn-lg">Remove ${identity.providerName!}</a>
                         </#if>
                     <#else>
-                        <a href="${identity.actionUrl}" type="submit" class="btn btn-primary btn-lg">Add ${identity.providerName!}</a>
+                        <a href="${identity.actionUrl}" type="submit" id="add-${identity.providerId!}" class="btn btn-primary btn-lg">Add ${identity.providerName!}</a>
                     </#if>
                 </div>
             </div>

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,15 +338,18 @@ public class AuthenticationBrokerResource {
             return Response.status(302).location(UriBuilder.fromUri(clientSession.getRedirectUri()).build()).build();
         }
 
-        UserModel user = session.users().getUserByEmail(updatedIdentity.getEmail(), realm);
+        if (federatedUser == null) {
+
+            UserModel existingUser = session.users().getUserByEmail(updatedIdentity.getEmail(), realm);
             String errorMessage = "federatedIdentityEmailExists";
 
-        if (user == null) {
+            if (existingUser == null) {
-            user = session.users().getUserByUsername(updatedIdentity.getUsername(), realm);
+                existingUser = session.users().getUserByUsername(updatedIdentity.getUsername(), realm);
                 errorMessage = "federatedIdentityUsernameExists";
             }
 
-        if (user == 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());
@@ -360,7 +368,6 @@ public class AuthenticationBrokerResource {
                     federatedUser.addRequiredAction(UPDATE_PROFILE);
                 }
             } else {
-            if (federatedUser == null) {
                 return Flows.forms(session, realm, clientSession.getClient(), uriInfo)
                         .setClientSessionCode(clientCode.getCode())
                         .setError(errorMessage)

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);
     }

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/");

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);
+        TwitterIdentityProvider twitterIdentityProvider = new TwitterIdentityProviderFactory().create(identityProvider);
-        OAuth2IdentityProviderConfig config = gitHubIdentityProvider.getConfig();
+        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");
 

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 <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
+ */
+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();
+    }
+}

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": [
@@ -151,7 +163,10 @@
                 { "type" : "password",
                     "value" : "password" }
             ],
-            "realmRoles": ["manager"]
+            "realmRoles": ["manager"],
+            "applicationRoles": {
+                "account": [ "manage-account" ]
+            }
         }
     ],
     "applications": [