From 878debd2ab2884306a1fe89c44d0b7fa02eabf4a Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Tue, 21 Feb 2023 13:25:03 +0100 Subject: [PATCH] Forbid changing ID Closes: #16881 --- .../file/FileMapKeycloakTransaction.java | 33 +++++++++++++++++-- .../common/delegate/EntityFieldDelegate.java | 7 +++- .../models/map/storage/ModelEntityUtil.java | 5 +++ .../resources/admin/UserResource.java | 5 +++ 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapKeycloakTransaction.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapKeycloakTransaction.java index 7a7ef7e63ed6..9850d89e08c7 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapKeycloakTransaction.java +++ b/model/map-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 extends FileMapStorage.Crud { private FileMapKeycloakTransaction tx; @@ -148,6 +157,26 @@ protected boolean removeIfExists(Path sp) { protected String getTxId() { return tx.txId; } + } + + private class IdProtector extends EntityFieldDelegate.WithEntity { + + public IdProtector(V entity) { + super(entity); + } + @Override + public > & org.keycloak.models.map.common.EntityField> 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 index 546053b19614..df8f89f3b2e5 100644 --- 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 extends UpdatableEntity { public abstract class WithEntity implements EntityFieldDelegate { - private final E entity; + protected final E entity; public WithEntity(E entity) { this.entity = entity; @@ -52,6 +52,11 @@ public > & EntityField> 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 index 6e0ba707843d..615875105418 100755 --- 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); }