Better message when updating users when import is disabled

Closes keycloak#31456 Signed-off-by: Pedro Igor <[email protected]>
pedroigor · Nov 8, 2024 · ee8e949 · ee8e949
1 parent 5aa874e
commit ee8e949
Show file tree

Hide file tree

Showing 7 changed files with 42 additions and 28 deletions.
diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java
@@ -230,7 +230,7 @@ protected UserModel proxy(RealmModel realm, UserModel local, LDAPObject ldapObje
                 // Any attempt to write data, which are not supported by the LDAP schema, should fail
                 // This check is skipped when register new user as there are many "generic" attributes always written (EG. enabled, emailVerified) and those are usually unsupported by LDAP schema
                 if (!model.isImportEnabled() && !newUser) {
-                    UserModel readOnlyDelegate = new ReadOnlyUserModelDelegate(local, ModelException::new);
+                    UserModel readOnlyDelegate = new ReadOnlyUserModelDelegate(local, ReadOnlyException::new);
                     proxied = new LDAPWritesOnlyUserModelDelegate(readOnlyDelegate, this);
                 }
                 break;

diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java
@@ -340,12 +340,7 @@ protected UserModel cacheUser(RealmModel realm, UserModel delegate, Long revisio
         int notBefore = getDelegate().getNotBeforeOfUser(realm, delegate);
 
         if (isReadOnlyOrganizationMember(delegate)) {
-            return new ReadOnlyUserModelDelegate(delegate) {
-                @Override
-                public boolean isEnabled() {
-                    return false;
-                }
-            };
+            return new ReadOnlyUserModelDelegate(delegate, false);
         }
 
         CachedUser cached;
@@ -355,12 +350,7 @@ public boolean isEnabled() {
             ComponentModel component = realm.getComponent(delegate.getFederationLink());
             UserStorageProviderModel model = new UserStorageProviderModel(component);
             if (!model.isEnabled()) {
-                return new ReadOnlyUserModelDelegate(delegate) {
-                    @Override
-                    public boolean isEnabled() {
-                        return false;
-                    }
-                };
+                return new ReadOnlyUserModelDelegate(delegate, false);
             }
             UserStorageProviderModel.CachePolicy policy = model.getCachePolicy();
             if (policy != null && policy == UserStorageProviderModel.CachePolicy.NO_CACHE) {

diff --git a/model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java b/model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java
@@ -115,12 +115,7 @@ private UserFederatedStorageProvider getFederatedStorage() {
     protected UserModel importValidation(RealmModel realm, UserModel user) {
 
         if (isReadOnlyOrganizationMember(user)) {
-            return new ReadOnlyUserModelDelegate(user) {
-                @Override
-                public boolean isEnabled() {
-                    return false;
-                }
-            };
+            return new ReadOnlyUserModelDelegate(user, false);
         }
 
         if (user == null || user.getFederationLink() == null) return user;
@@ -134,12 +129,7 @@ public boolean isEnabled() {
         }
 
         if (!model.isEnabled()) {
-            return new ReadOnlyUserModelDelegate(user) {
-                @Override
-                public boolean isEnabled() {
-                    return false;
-                }
-            };
+            return new ReadOnlyUserModelDelegate(user, false);
         }
 
         ImportedUserValidation importedUserValidation = getStorageProviderInstance(model, ImportedUserValidation.class, true);

diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/ReadOnlyUserModelDelegate.java b/server-spi-private/src/main/java/org/keycloak/models/utils/ReadOnlyUserModelDelegate.java
@@ -31,11 +31,17 @@
 public class ReadOnlyUserModelDelegate extends UserModelDelegate {
 
     private final Function<String, RuntimeException> exceptionCreator;
+    private Boolean enabled;
 
     public ReadOnlyUserModelDelegate(UserModel delegate) {
         this(delegate, ReadOnlyException::new);
     }
 
+    public ReadOnlyUserModelDelegate(UserModel delegate, boolean enabled) {
+        this(delegate, ReadOnlyException::new);
+        this.enabled = enabled;
+    }
+
     public ReadOnlyUserModelDelegate(UserModel delegate, Function<String, RuntimeException> exceptionCreator) {
         super(delegate);
         this.exceptionCreator = exceptionCreator;
@@ -51,6 +57,14 @@ public void setEnabled(boolean enabled) {
         throw readOnlyException("enabled");
     }
 
+    @Override
+    public boolean isEnabled() {
+        if (enabled == null) {
+            return super.isEnabled();
+        }
+        return enabled;
+    }
+
     @Override
     public void setSingleAttribute(String name, String value) {
         throw readOnlyException("attribute(" + name + ")");
@@ -142,7 +156,7 @@ public void grantRole(RoleModel role) {
     }
 
     private RuntimeException readOnlyException(String detail) {
-        String message = String.format("Not possible to write '%s' when updating user '%s'", detail, getUsername());
+        String message = String.format("The user is read-only. Not possible to write '%s' when updating user '%s'.", detail, getUsername());
         return exceptionCreator.apply(message);
     }
 }
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java
@@ -233,7 +233,7 @@ public Response updateUser(final UserRepresentation rep) {
             throw ErrorResponse.exists("User exists with same username or email");
         } catch (ReadOnlyException re) {
             session.getTransactionManager().setRollbackOnly();
-            throw ErrorResponse.error("User is read only!", Status.BAD_REQUEST);
+            throw ErrorResponse.error(re.getMessage() == null ? "User is read only!" : re.getMessage(), Status.BAD_REQUEST);
         } catch (PasswordPolicyNotMetException e) {
             logger.warn("Password policy not met for user " + e.getUsername(), e);
             session.getTransactionManager().setRollbackOnly();

diff --git a/.../tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java b/.../tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java
@@ -30,6 +30,7 @@
 import org.keycloak.models.UserModel;
 import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.representations.idm.ComponentRepresentation;
+import org.keycloak.storage.ReadOnlyException;
 import org.keycloak.storage.UserStoragePrivateUtil;
 import org.keycloak.storage.ldap.LDAPConfig;
 import org.keycloak.storage.ldap.LDAPStorageProvider;
@@ -275,7 +276,7 @@ protected void test02_readOnlyGroupMappings(boolean importEnabled) {
                 try {
                     mary.joinGroup(group12);
                     Assert.fail("Not expected to successfully add group12 in no-import mode and READ_ONLY mode of the group mapper");
-                } catch (ModelException me) {
+                } catch (ReadOnlyException me) {
                     // Ignore
                 }
             });

diff --git a/...org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java b/...org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java
@@ -17,6 +17,8 @@
 
 package org.keycloak.testsuite.federation.ldap.noimport;
 
+import static org.junit.Assert.fail;
+
 import java.util.Collections;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -38,9 +40,11 @@
 import org.keycloak.models.LDAPConstants;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.UserModel;
+import org.keycloak.models.UserModel.RequiredAction;
 import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.models.utils.ModelToRepresentation;
 import org.keycloak.representations.idm.ComponentRepresentation;
+import org.keycloak.representations.idm.ErrorRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
 import org.keycloak.storage.StorageId;
 import org.keycloak.storage.UserStorageProvider;
@@ -350,6 +354,7 @@ public void testImpossibleToChangeNonLDAPMappedAttributes() {
             Assert.assertEquals("654321", johnRep.getAttributes().get("postal_code").get(0));
         } finally {
             // Revert
+            johnRep = john.toRepresentation();
             johnRep.setFirstName(firstNameOrig);
             johnRep.setLastName(lastNameOrig);
             johnRep.singleAttribute("postal_code", postalCodeOrig);
@@ -361,6 +366,20 @@ public void testImpossibleToChangeNonLDAPMappedAttributes() {
         }
     }
 
+    @Test
+    public void testCannotUpdateReadOnlyUserImportDisabled() {
+        UserResource user = ApiUtil.findUserByUsernameId(testRealm(), "johnkeycloak");
+
+        try {
+            UserRepresentation rep = user.toRepresentation();
+            rep.setRequiredActions(List.of(RequiredAction.VERIFY_EMAIL.name()));
+            user.update(rep);
+            fail("Should fail as the user is read-only");
+        } catch (BadRequestException bde) {
+            Assert.assertEquals("The user is read-only. Not possible to write 'required action VERIFY_EMAIL' when updating user 'johnkeycloak'.", bde.getResponse().readEntity(ErrorRepresentation.class).getErrorMessage());
+        }
+    }
+
     // No need to test this in no-import mode. There won't be any users in localStorage.
     @Test
     @Ignore