keycloak-memoizeit

Display proper error message if LDAP-linked user couldn't

8/26/2014 2:10:48 PM

Details

diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java
index cb948ba..45bd0df 100755
--- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java
+++ b/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());
diff --git a/forms/common-themes/src/main/resources/theme/admin/base/resources/js/controllers/users.js b/forms/common-themes/src/main/resources/theme/admin/base/resources/js/controllers/users.js
index 8543323..304d66c 100755
--- a/forms/common-themes/src/main/resources/theme/admin/base/resources/js/controllers/users.js
+++ b/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");
             });
         });
     };
diff --git a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java b/model/api/src/main/java/org/keycloak/models/UserFederationManager.java
index 083886d..13faf73 100755
--- a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java
+++ b/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();
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java
index 52e1e65..13de4df 100755
--- a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java
+++ b/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);
+        }
     }
 
     /**
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/FederationProvidersIntegrationTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/FederationProvidersIntegrationTest.java
index 09a07e6..d0b2080 100755
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/FederationProvidersIntegrationTest.java
+++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/FederationProvidersIntegrationTest.java
@@ -237,6 +237,23 @@ 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 testReadonly() {
         KeycloakSession session = keycloakRule.startSession();
         try {
@@ -275,6 +292,8 @@ public class FederationProvidersIntegrationTest {
             } catch (ModelReadOnlyException e) {
 
             }
+
+            Assert.assertFalse(session.users().removeUser(appRealm, user));
         } finally {
             keycloakRule.stopSession(session, false);
         }
@@ -311,6 +330,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);
         }