Forbid changing ID

Closes: #16881
keycloak · Feb 22, 2023 · 878debd · 878debd
1 parent b9ab942
commit 878debd
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 3 deletions.
diff --git a/...p-file/src/main/java/org/keycloak/models/map/storage/file/FileMapKeycloakTransaction.java b/...p-file/src/main/java/org/keycloak/models/map/storage/file/FileMapKeycloakTransaction.java
@@ -22,11 +22,13 @@
 import org.keycloak.models.map.common.StringKeyConverter;
 import org.keycloak.models.map.common.StringKeyConverter.StringKey;
 import org.keycloak.models.map.common.UpdatableEntity;
+import org.keycloak.models.map.common.delegate.EntityFieldDelegate;
 import org.keycloak.models.map.storage.MapKeycloakTransaction;
 import org.keycloak.models.map.storage.ModelEntityUtil;
 import org.keycloak.models.map.storage.chm.ConcurrentHashMapKeycloakTransaction;
 import org.keycloak.models.map.storage.chm.MapFieldPredicates;
 import org.keycloak.models.map.storage.chm.MapModelCriteriaBuilder.UpdatePredicatesFunc;
+import org.keycloak.storage.ReadOnlyException;
 import org.keycloak.storage.SearchableModelField;
 import java.io.IOException;
 import java.io.UncheckedIOException;
@@ -38,6 +40,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.function.Function;
 
 /**
@@ -82,11 +85,11 @@ public void rollback() {
     @Override
     public void commit() {
         super.commit();
-        this.renameOnCommit.forEach(FileMapKeycloakTransaction::silentMove);
+        this.renameOnCommit.forEach(FileMapKeycloakTransaction::move);
         this.pathsToDelete.forEach(FileMapKeycloakTransaction::silentDelete);
     }
 
-    private static void silentMove(Path from, Path to) {
+    private static void move(Path from, Path to) {
         try {
             Files.move(from, to, StandardCopyOption.REPLACE_EXISTING);
         } catch (IOException ex) {
@@ -121,6 +124,12 @@ void registerRenameOnCommit(Path from, Path to) {
         this.pathsToDelete.add(from);
     }
 
+    @Override
+    public V registerEntityForChanges(V origEntity) {
+        final V watchedValue = super.registerEntityForChanges(origEntity);
+        return DeepCloner.DUMB_CLONER.entityFieldDelegate(watchedValue, new IdProtector(watchedValue));
+    }
+
     private static class Crud<V extends AbstractEntity & UpdatableEntity, M> extends FileMapStorage.Crud<V, M> {
 
         private FileMapKeycloakTransaction tx;
@@ -148,6 +157,26 @@ protected boolean removeIfExists(Path sp) {
         protected String getTxId() {
             return tx.txId;
         }
+    }
+
+    private class IdProtector extends EntityFieldDelegate.WithEntity<V> {
+
+        public IdProtector(V entity) {
+            super(entity);
+        }
 
+        @Override
+        public <T, EF extends java.lang.Enum<? extends org.keycloak.models.map.common.EntityField<V>> & org.keycloak.models.map.common.EntityField<V>> void set(EF field, T value) {
+            String id = entity.getId();
+            super.set(field, value);
+            if (! Objects.equals(id, map.determineKeyFromValue(entity, false))) {
+                throw new ReadOnlyException("Cannot change " + field + " as that would change primary key");
+            }
+        }
+
+        @Override
+        public String toString() {
+            return super.toString() + " [protected ID]";
+        }
     }
 }
diff --git a/model/map/src/main/java/org/keycloak/models/map/common/delegate/EntityFieldDelegate.java b/model/map/src/main/java/org/keycloak/models/map/common/delegate/EntityFieldDelegate.java
@@ -7,7 +7,7 @@
 public interface EntityFieldDelegate<E> extends UpdatableEntity {
 
     public abstract class WithEntity<E extends UpdatableEntity> implements EntityFieldDelegate<E> {
-        private final E entity;
+        protected final E entity;
 
         public WithEntity(E entity) {
             this.entity = entity;
@@ -52,6 +52,11 @@ public <T, EF extends java.lang.Enum<? extends org.keycloak.models.map.common.En
         public boolean isUpdated() {
             return entity.isUpdated();
         }
+
+        @Override
+        public String toString() {
+            return "&" + String.valueOf(entity);
+        }
     }
 
     // Non-collection values

diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/ModelEntityUtil.java b/model/map/src/main/java/org/keycloak/models/map/storage/ModelEntityUtil.java
@@ -224,6 +224,11 @@ public <V, EF extends java.lang.Enum<? extends EntityField<T>> & EntityField<T>>
                     super.set(field, value);
                 }
             }
+
+            @Override
+            public String toString() {
+                return super.toString() + " [fixed " + entityField + "=" + value + "]";
+            }
         });
     }
 

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
@@ -206,15 +206,20 @@ public Response updateUser(final UserRepresentation rep) {
             }
             return Response.noContent().build();
         } catch (ModelDuplicateException e) {
+            session.getTransactionManager().setRollbackOnly();
             return ErrorResponse.exists("User exists with same username or email");
         } catch (ReadOnlyException re) {
+            session.getTransactionManager().setRollbackOnly();
             return ErrorResponse.error("User is read only!", Status.BAD_REQUEST);
         } catch (ModelException me) {
             logger.warn("Could not update user!", me);
+            session.getTransactionManager().setRollbackOnly();
             return ErrorResponse.error("Could not update user!", Status.BAD_REQUEST);
         } catch (ForbiddenException fe) {
+            session.getTransactionManager().setRollbackOnly();
             throw fe;
         } catch (Exception me) { // JPA
+            session.getTransactionManager().setRollbackOnly();
             logger.warn("Could not update user!", me);// may be committed by JTA which can't
             return ErrorResponse.error("Could not update user!", Status.BAD_REQUEST);
         }