Merge pull request #648 from mposolda/master

Federation & LDAP related fixes

federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java

@@ -126,7 +126,10 @@ public class LDAPFederationProvider implements UserFederationProvider {
 
     @Override
     public boolean removeUser(RealmModel realm, UserModel user) {
-        if (editMode == EditMode.READ_ONLY || editMode == EditMode.UNSYNCED) return false;
+        if (editMode == EditMode.READ_ONLY || editMode == EditMode.UNSYNCED) {
+            logger.warnf("User '%s' can't be deleted in LDAP as editMode is '%s'", user.getUsername(), editMode.toString());
+            return false;
+        }
 
         try {
             return LDAPUtils.removeUser(partitionManager, user.getUsername());
@@ -137,17 +140,41 @@ public class LDAPFederationProvider implements UserFederationProvider {
 
     @Override
     public List<UserModel> searchByAttributes(Map<String, String> attributes, RealmModel realm, int maxResults) {
-        IdentityManager identityManager = getIdentityManager();
         List<UserModel> searchResults =new LinkedList<UserModel>();
         try {
+            Map<String, User> plUsers = searchPicketlink(attributes, maxResults);
+            for (User user : plUsers.values()) {
+                if (session.userStorage().getUserByUsername(user.getLoginName(), realm) == null) {
+                    UserModel imported = importUserFromPicketlink(realm, user);
+                    searchResults.add(imported);
+                }
+            }
+        } catch (IdentityManagementException ie) {
+            throw convertIDMException(ie);
+        }
+        return searchResults;
+    }
+
+    protected Map<String, User> searchPicketlink(Map<String, String> attributes, int maxResults) {
+        IdentityManager identityManager = getIdentityManager();
         Map<String, User> results = new HashMap<String, User>();
         if (attributes.containsKey(USERNAME)) {
             User user = BasicModel.getUser(identityManager, attributes.get(USERNAME));
-                if (user != null) results.put(user.getLoginName(), user);
+            if (user != null) {
-            } else if (attributes.containsKey(EMAIL)) {
+                results.put(user.getLoginName(), user);
+                return results;
+            }
+        }
+
+        if (attributes.containsKey(EMAIL)) {
             User user = queryByEmail(identityManager, attributes.get(EMAIL));
-                if (user != null) results.put(user.getLoginName(), user);
+            if (user != null) {
-            } else if (attributes.containsKey(FIRST_NAME) || attributes.containsKey(LAST_NAME)) {
+                results.put(user.getLoginName(), user);
+                return results;
+            }
+        }
+
+        if (attributes.containsKey(FIRST_NAME) || attributes.containsKey(LAST_NAME)) {
             IdentityQuery<User> query = identityManager.createIdentityQuery(User.class);
             if (attributes.containsKey(FIRST_NAME)) {
                 query.setParameter(User.FIRST_NAME, attributes.get(FIRST_NAME));
@@ -161,16 +188,8 @@ public class LDAPFederationProvider implements UserFederationProvider {
                 results.put(user.getLoginName(), user);
             }
         }
-            for (User user : results.values()) {
+
-                if (session.userStorage().getUserByUsername(user.getLoginName(), realm) == null) {
+        return results;
-                    UserModel imported = importUserFromPicketlink(realm, user);
-                    searchResults.add(imported);
-                }
-            }
-        } catch (IdentityManagementException ie) {
-            throw convertIDMException(ie);
-        }
-        return searchResults;
     }
 
     @Override

forms/common-themes/src/main/resources/theme/admin/base/resources/js/controllers/users.js

@@ -253,6 +253,8 @@ module.controller('UserDetailCtrl', function($scope, realm, user, User, UserFede
             }, function() {
                 $location.url("/realms/" + realm.realm + "/users");
                 Notifications.success("The user has been deleted.");
+            }, function() {
+                Notifications.error("User couldn't be deleted");
             });
         });
     };

forms/common-themes/src/main/resources/theme/admin/base/resources/partials/user-detail.html

@@ -41,7 +41,7 @@
                     <div class="col-sm-5">
                         <!-- Characters >,<,/,\ are forbidden in username -->
                         <input class="form-control" type="text" id="username" name="username" data-ng-model="user.username" autofocus
-                               required ng-pattern="/^[^\<\>\\\/]*$/">
+                               required ng-pattern="/^[^\<\>\\\/]*$/" data-ng-readonly="!create">
                     </div>
                 </div>
 

model/api/src/main/java/org/keycloak/models/UserFederationManager.java

@@ -7,11 +7,16 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import org.jboss.logging.Logger;
+
 /**
  * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
  * @version $Revision: 1 $
  */
 public class UserFederationManager implements UserProvider {
+
+    private static final Logger logger = Logger.getLogger(UserFederationManager.class);
+
     protected KeycloakSession session;
 
     public UserFederationManager(KeycloakSession session) {
@@ -83,6 +88,7 @@ public class UserFederationManager implements UserProvider {
             RealmModel realmModel = tx.realms().getRealm(realm.getId());
             UserModel deletedUser = tx.userStorage().getUserById(user.getId(), realmModel);
             tx.userStorage().removeUser(realmModel, deletedUser);
+            logger.debugf("Removed invalid user '%s'", user.getUsername());
             tx.getTransaction().commit();
         } finally {
             tx.close();

services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java

@@ -305,7 +305,7 @@ public class UsersResource {
     @Path("{username}")
     @DELETE
     @NoCache
-    public void deleteUser(final @PathParam("username") String username) {
+    public Response deleteUser(final @PathParam("username") String username) {
         auth.requireManage();
 
         UserModel user = session.users().getUserByUsername(username, realm);
@@ -313,7 +313,12 @@ public class UsersResource {
             throw new NotFoundException("User not found");
         }
 
-        new UserManager(session).removeUser(realm, user);
+        boolean removed = new UserManager(session).removeUser(realm, user);
+        if (removed) {
+            return Response.noContent().build();
+        } else {
+            return Flows.errors().error("User couldn't be deleted", Response.Status.BAD_REQUEST);
+        }
     }
 
     /**

testsuite/integration/src/test/java/org/keycloak/testsuite/forms/FederationProvidersIntegrationTest.java

@@ -275,6 +275,8 @@ public class FederationProvidersIntegrationTest {
             } catch (ModelReadOnlyException e) {
 
             }
+
+            Assert.assertFalse(session.users().removeUser(appRealm, user));
         } finally {
             keycloakRule.stopSession(session, false);
         }
@@ -288,6 +290,60 @@ public class FederationProvidersIntegrationTest {
         }
     }
 
+    @Test
+    public void testRemoveFederatedUser() {
+        KeycloakSession session = keycloakRule.startSession();
+        try {
+            RealmModel appRealm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("registerUserSuccess2", appRealm);
+            Assert.assertNotNull(user);
+            Assert.assertNotNull(user.getFederationLink());
+            Assert.assertEquals(user.getFederationLink(), ldapModel.getId());
+
+            Assert.assertTrue(session.users().removeUser(appRealm, user));
+            Assert.assertNull(session.users().getUserByUsername("registerUserSuccess2", appRealm));
+        } finally {
+            keycloakRule.stopSession(session, true);
+        }
+    }
+
+    @Test
+    public void testSearch() {
+        KeycloakSession session = keycloakRule.startSession();
+        PartitionManager partitionManager = getPartitionManager(session, ldapModel);
+        try {
+            RealmModel appRealm = session.realms().getRealmByName("test");
+            LDAPUtils.addUser(partitionManager, "username1", "John1", "Doel1", "user1@email.org");
+            LDAPUtils.addUser(partitionManager, "username2", "John2", "Doel2", "user2@email.org");
+            LDAPUtils.addUser(partitionManager, "username3", "John3", "Doel3", "user3@email.org");
+            LDAPUtils.addUser(partitionManager, "username4", "John4", "Doel4", "user4@email.org");
+
+            // Users are not at local store at this moment
+            Assert.assertNull(session.userStorage().getUserByUsername("username1", appRealm));
+            Assert.assertNull(session.userStorage().getUserByUsername("username2", appRealm));
+            Assert.assertNull(session.userStorage().getUserByUsername("username3", appRealm));
+            Assert.assertNull(session.userStorage().getUserByUsername("username4", appRealm));
+
+            // search by username
+            session.users().searchForUser("username1", appRealm);
+            SyncProvidersTest.assertUserImported(session.userStorage(), appRealm, "username1", "John1", "Doel1", "user1@email.org");
+
+            // search by email
+            session.users().searchForUser("user2@email.org", appRealm);
+            SyncProvidersTest.assertUserImported(session.userStorage(), appRealm, "username2", "John2", "Doel2", "user2@email.org");
+
+            // search by lastName
+            session.users().searchForUser("Doel3", appRealm);
+            SyncProvidersTest.assertUserImported(session.userStorage(), appRealm, "username3", "John3", "Doel3", "user3@email.org");
+
+            // search by firstName + lastName
+            session.users().searchForUser("John4 Doel4", appRealm);
+            SyncProvidersTest.assertUserImported(session.userStorage(), appRealm, "username4", "John4", "Doel4", "user4@email.org");
+        } finally {
+            keycloakRule.stopSession(session, true);
+        }
+    }
+
     @Test
     public void testUnsynced() {
         KeycloakSession session = keycloakRule.startSession();
@@ -311,6 +367,9 @@ public class FederationProvidersIntegrationTest {
 
             // LDAP password is still unchanged
             Assert.assertTrue(LDAPUtils.validatePassword(getPartitionManager(session, model), "johnkeycloak", "new-password"));
+
+            // ATM it's not permitted to delete user in unsynced mode. Should be user deleted just locally instead?
+            Assert.assertFalse(session.users().removeUser(appRealm, user));
         } finally {
             keycloakRule.stopSession(session, false);
         }

testsuite/integration/src/test/java/org/keycloak/testsuite/forms/SyncProvidersTest.java

@@ -185,7 +185,7 @@ public class SyncProvidersTest {
         }
     }
 
-    private void assertUserImported(UserProvider userProvider, RealmModel realm, String username, String expectedFirstName, String expectedLastName, String expectedEmail) {
+    public static void assertUserImported(UserProvider userProvider, RealmModel realm, String username, String expectedFirstName, String expectedLastName, String expectedEmail) {
         UserModel user = userProvider.getUserByUsername(username, realm);
         Assert.assertNotNull(user);
         Assert.assertEquals(expectedFirstName, user.getFirstName());