libre.sh/keycloak-scim
2
0
Fork 0
Code Issues 14 Pull requests Releases Packages Activity Actions 6

Introduce last modified time validation

Browse source 
- Also fixes commit/rollback logic to prevent deletion of wrong files on rollback

Closes #17491
Closes #17660
This commit is contained in:
Stefan Guilhen 2023-03-07 15:48:23 -03:00 committed by Hynek Mlnařík
parent e217644ff4
commit ad3b264088
2 changed files with 115 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

108
model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapKeycloakTransaction.java
View file

@ -17,6 +17,7 @@
package org.keycloak.models.map.storage.file;
import org.jboss.logging.Logger;
import org.keycloak.models.map.common.AbstractEntity;
import org.keycloak.models.map.common.DeepCloner;
import org.keycloak.models.map.common.StringKeyConverter;
@ -32,15 +33,18 @@ import org.keycloak.storage.ReadOnlyException;
import org.keycloak.storage.SearchableModelField;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.nio.file.attribute.BasicFileAttributes;
import java.nio.file.attribute.FileTime;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Function;
/**
@ -51,8 +55,12 @@ import java.util.function.Function;
public class FileMapKeycloakTransaction<V extends AbstractEntity & UpdatableEntity, M>
extends ConcurrentHashMapKeycloakTransaction<String, V, M> {
private static final Logger LOG = Logger.getLogger(FileMapKeycloakTransaction.class);
private final List<Path> createdPaths = new LinkedList<>();
private final List<Path> pathsToDelete = new LinkedList<>();
private final Map<Path, Path> renameOnCommit = new HashMap<>();
private final Map<Path, FileTime> lastModified = new HashMap<>();
private final String txId = StringKey.INSTANCE.yieldNewUniqueKey();
@ -77,16 +85,30 @@ public class FileMapKeycloakTransaction<V extends AbstractEntity & UpdatableEnti
@Override
public void rollback() {
// remove all temporary and empty files that were created.
this.renameOnCommit.keySet().forEach(FileMapKeycloakTransaction::silentDelete);
this.pathsToDelete.forEach(FileMapKeycloakTransaction::silentDelete);
this.createdPaths.forEach(FileMapKeycloakTransaction::silentDelete);
super.rollback();
}
@Override
public void commit() {
super.commit();
this.renameOnCommit.forEach(FileMapKeycloakTransaction::move);
this.pathsToDelete.forEach(FileMapKeycloakTransaction::silentDelete);
// check it is still safe to update/delete before moving the temp files into the actual files or deleting them.
Set<Path> allChangedPaths = new HashSet<>();
allChangedPaths.addAll(this.renameOnCommit.values());
allChangedPaths.addAll(this.pathsToDelete);
allChangedPaths.forEach(this::checkIsSafeToModify);
try {
this.renameOnCommit.forEach(FileMapKeycloakTransaction::move);
this.pathsToDelete.forEach(FileMapKeycloakTransaction::silentDelete);
// TODO: catch exception thrown by move and try to restore any previously completed moves.
} finally {
// ensure all temp files are removed.
this.renameOnCommit.keySet().forEach(FileMapKeycloakTransaction::silentDelete);
// remove any created files that may have been left empty.
this.createdPaths.forEach(path -> silenteDelete(path, true));
}
}
private static void move(Path from, Path to) {
@ -98,18 +120,24 @@ public class FileMapKeycloakTransaction<V extends AbstractEntity & UpdatableEnti
}
private static void silentDelete(Path p) {
silenteDelete(p, false);
}
private static void silenteDelete(final Path path, final boolean checkEmpty) {
try {
if (Files.exists(p)) {
Files.delete(p);
if (Files.exists(path)) {
if (!checkEmpty || Files.size(path) == 0) {
Files.delete(path);
}
}
} catch (IOException e) {
// Swallow the exception
} catch(IOException e) {
// swallow the exception.
}
}
public void touch(Path path) throws FileAlreadyExistsException, IOException {
public void touch(Path path) throws IOException {
Files.createFile(path);
pathsToDelete.add(path);
createdPaths.add(path);
}
public boolean removeIfExists(Path path) {
@ -120,10 +148,56 @@ public class FileMapKeycloakTransaction<V extends AbstractEntity & UpdatableEnti
void registerRenameOnCommit(Path from, Path to) {
this.renameOnCommit.put(from, to);
this.pathsToDelete.remove(to);
this.pathsToDelete.add(from);
}
/**
* Obtains and stores the last modified time of the file identified by the supplied {@link Path}. This value is used
* to determine if the file was changed by another transaction after it was read by this transaction.
*
* @param path the {@link Path} to the file.
*/
FileTime getLastModifiedTime(final Path path) {
try {
BasicFileAttributes attr = Files.readAttributes(path, BasicFileAttributes.class);
FileTime lastModifiedTime = attr.lastModifiedTime();
this.lastModified.put(path, lastModifiedTime);
return lastModifiedTime;
} catch (IOException ex) {
throw new IllegalStateException("Could not read file attributes " + path, ex);
}
}
/**
* Checks if it is safe to modify the file identified by the supplied {@link Path}. In particular, this method
* verifies if the file was changed (removed, updated) after it was read by this transaction. Being it the case, this
* transaction should refrain from performing further updates as it must assume its data has become stale.
*
* @param path the {@link Path} to the file that will be updated.
* @throws IllegalStateException if the file was altered by another transaction.
*/
void checkIsSafeToModify(final Path path) {
try {
// path wasn't previously loaded - log a message and return.
if (this.lastModified.get(path) == null) {
LOG.debugf("File %s was not previously loaded, skipping validation prior to writing", path);
return;
}
// check if the original file was deleted by another transaction.
if (!Files.exists(path)) {
throw new IllegalStateException("File " + path + " was removed by another transaction");
}
// check if the original file was modified by another transaction.
BasicFileAttributes attr = Files.readAttributes(path, BasicFileAttributes.class);
long lastModifiedTime = attr.lastModifiedTime().toMillis();
if (this.lastModified.get(path).toMillis() < lastModifiedTime) {
throw new IllegalStateException("File " + path + " was changed by another transaction");
}
} catch (IOException e) {
throw new RuntimeException(e);
}
}
@Override
public V registerEntityForChanges(V origEntity) {
final V watchedValue = super.registerEntityForChanges(origEntity);
@ -157,6 +231,16 @@ public class FileMapKeycloakTransaction<V extends AbstractEntity & UpdatableEnti
protected String getTxId() {
return tx.txId;
}
@Override
protected FileTime getLastModifiedTime(final Path sp) {
return tx.getLastModifiedTime(sp);
}
@Override
protected void checkIsSafeToModify(final Path sp) {
tx.checkIsSafeToModify(sp);
}
}
private class IdProtector extends EntityFieldDelegate.WithEntity<V> {

20
model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorage.java
View file

@ -45,6 +45,7 @@ import java.io.UncheckedIOException;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.FileTime;
import java.util.Arrays;
import java.util.IdentityHashMap;
import java.util.List;
@ -207,6 +208,7 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
}
protected V parse(Path fileName) {
getLastModifiedTime(fileName);
final V parsedObject = YamlParser.parse(fileName, new MapEntityContext<>(entityClass));
if (parsedObject == null) {
LOG.debugf("Could not parse %s%s", fileName, StackUtil.getShortStackTrace());
@ -368,7 +370,7 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
if (sp == null) {
throw new IllegalArgumentException("Invalid path: " + escapedId);
}
checkIsSafeToModify(sp);
// TODO: improve locking
synchronized (FileMapStorageProviderFactory.class) {
writeYamlContents(sp, value);
@ -430,6 +432,22 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
protected abstract String getTxId();
/**
* Hook to obtain the last modified time of the file identified by the supplied {@link Path}.
*
* @param path the {@link Path} to the file whose last modified time it to be obtained.
* @return the {@link FileTime} corresponding to the file's last modified time.
*/
protected abstract FileTime getLastModifiedTime(final Path path);
/**
* Hook to validate that it is safe to modify the file identified by the supplied {@link Path}. The primary
* goal is to identify if other transactions have modified the file after it was read by the current transaction,
* preventing updates to a stale entity.
*
* @param path the {@link Path} to the file that is to be modified.
*/
protected abstract void checkIsSafeToModify(final Path path);
}
}