diff --git a/gradle/changelog/write_lock_check.yaml b/gradle/changelog/write_lock_check.yaml new file mode 100644 index 0000000000..9648489d78 --- /dev/null +++ b/gradle/changelog/write_lock_check.yaml @@ -0,0 +1,2 @@ +- type: added + description: Write lock check for queryable stores diff --git a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStore.java b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStore.java index ed55e52548..daeb57be12 100644 --- a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStore.java +++ b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStore.java @@ -55,8 +55,9 @@ class SQLiteQueryableMutableStore extends SQLiteQueryableStore implements Class clazz, QueryableTypeDescriptor queryableTypeDescriptor, String[] parentIds, - ReadWriteLock lock) { - super(objectMapper, connection, clazz, queryableTypeDescriptor, parentIds, lock); + ReadWriteLock lock, + boolean readOnly) { + super(objectMapper, connection, clazz, queryableTypeDescriptor, parentIds, lock, readOnly); this.objectMapper = objectMapper; this.clazz = clazz; this.parentIds = parentIds; diff --git a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStore.java b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStore.java index e8c35eaf87..a2fc8f890f 100644 --- a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStore.java +++ b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStore.java @@ -30,6 +30,7 @@ import sonia.scm.store.LogicalCondition; import sonia.scm.store.QueryableMaintenanceStore; import sonia.scm.store.QueryableStore; import sonia.scm.store.StoreException; +import sonia.scm.store.StoreReadOnlyException; import java.sql.Connection; import java.sql.PreparedStatement; @@ -71,19 +72,22 @@ class SQLiteQueryableStore implements QueryableStore, QueryableMaintenance private final String[] parentIds; private final ReadWriteLock lock; + private final boolean readOnly; public SQLiteQueryableStore(ObjectMapper objectMapper, Connection connection, Class clazz, QueryableTypeDescriptor queryableTypeDescriptor, String[] parentIds, - ReadWriteLock lock) { + ReadWriteLock lock, + boolean readOnly) { this.objectMapper = objectMapper; this.connection = connection; this.clazz = clazz; this.parentIds = parentIds; this.queryableTypeDescriptor = queryableTypeDescriptor; this.lock = lock; + this.readOnly = readOnly; } @Override @@ -243,11 +247,18 @@ class SQLiteQueryableStore implements QueryableStore, QueryableMaintenance } R executeWrite(SQLNodeWithValue sqlStatement, StatementCallback callback) { + assertNotReadOnly(); String sql = sqlStatement.toSQL(); log.debug("executing 'write' SQL: {}", sql); return executeWithLock(sqlStatement, callback, lock.writeLock(), sql); } + private void assertNotReadOnly() { + if (readOnly) { + throw new StoreReadOnlyException(clazz.getName()); + } + } + private R executeWithLock(SQLNodeWithValue sqlStatement, StatementCallback callback, Lock writeLock, String sql) { writeLock.lock(); try (PreparedStatement statement = connection.prepareStatement(sql, RETURN_GENERATED_KEYS)) { diff --git a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java index 859ff18804..f109fb7aba 100644 --- a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java +++ b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java @@ -28,6 +28,8 @@ import sonia.scm.SCMContextProvider; import sonia.scm.config.ConfigValue; import sonia.scm.plugin.PluginLoader; import sonia.scm.plugin.QueryableTypeDescriptor; +import sonia.scm.repository.Repository; +import sonia.scm.repository.RepositoryReadOnlyChecker; import sonia.scm.security.KeyGenerator; import sonia.scm.store.QueryableMaintenanceStore; import sonia.scm.store.QueryableStoreFactory; @@ -59,6 +61,7 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { private final ObjectMapper objectMapper; private final KeyGenerator keyGenerator; private final DataSource dataSource; + private final RepositoryReadOnlyChecker readOnlyChecker; private final Map queryableTypes = new HashMap<>(); @@ -69,6 +72,7 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { PluginLoader pluginLoader, ObjectMapper objectMapper, KeyGenerator keyGenerator, + RepositoryReadOnlyChecker readOnlyChecker, @ConfigValue(key = "queryableStore.maxPoolSize", defaultValue = DEFAULT_MAX_POOL_SIZE) int maxPoolSize, @ConfigValue(key = "queryableStore.connectionTimeout", defaultValue = DEFAULT_CONNECTION_TIMEOUT_IN_SECONDS) int connectionTimeoutInSeconds, @ConfigValue(key = "queryableStore.idleTimeout", defaultValue = DEFAULT_IDLE_TIMEOUT_IN_SECONDS) int idleTimeoutInSeconds, @@ -80,6 +84,7 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { objectMapper, keyGenerator, pluginLoader.getExtensionProcessor().getQueryableTypes(), + readOnlyChecker, maxPoolSize, connectionTimeoutInSeconds, idleTimeoutInSeconds, @@ -92,14 +97,16 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { public SQLiteQueryableStoreFactory(String connectionString, ObjectMapper objectMapper, KeyGenerator keyGenerator, - Iterable queryableTypeIterable) { - this(connectionString, objectMapper, keyGenerator, queryableTypeIterable, 10, 30, 600, 1800, 30); + Iterable queryableTypeIterable, + RepositoryReadOnlyChecker readOnlyChecker) { + this(connectionString, objectMapper, keyGenerator, queryableTypeIterable, readOnlyChecker, 10, 30, 600, 1800, 30); } private SQLiteQueryableStoreFactory(String connectionString, ObjectMapper objectMapper, KeyGenerator keyGenerator, Iterable queryableTypeIterable, + RepositoryReadOnlyChecker readOnlyChecker, int maxPoolSize, int connectionTimeoutInSeconds, int idleTimeoutInSeconds, @@ -112,6 +119,7 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { idleTimeoutInSeconds, maxLifetimeInSeconds, leakDetectionThresholdInSeconds); + this.readOnlyChecker = readOnlyChecker; this.dataSource = new HikariDataSource(config); this.objectMapper = objectMapper @@ -170,17 +178,57 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { @Override public SQLiteQueryableStore getReadOnly(Class clazz, String... parentIds) { - return new SQLiteQueryableStore<>(objectMapper, openDefaultConnection(), clazz, getQueryableTypeDescriptor(clazz), parentIds, lock); + QueryableTypeDescriptor queryableTypeDescriptor = getQueryableTypeDescriptor(clazz); + return new SQLiteQueryableStore<>( + objectMapper, + openDefaultConnection(), + clazz, + queryableTypeDescriptor, + parentIds, + lock, + mustBeReadOnly(queryableTypeDescriptor, parentIds) + ); } @Override public QueryableMaintenanceStore getForMaintenance(Class clazz, String... parentIds) { - return new SQLiteQueryableStore<>(objectMapper, openDefaultConnection(), clazz, getQueryableTypeDescriptor(clazz), parentIds, lock); + QueryableTypeDescriptor queryableTypeDescriptor = getQueryableTypeDescriptor(clazz); + return new SQLiteQueryableStore<>( + objectMapper, + openDefaultConnection(), + clazz, + queryableTypeDescriptor, + parentIds, + lock, + mustBeReadOnly(queryableTypeDescriptor, parentIds) + ); } @Override public SQLiteQueryableMutableStore getMutable(Class clazz, String... parentIds) { - return new SQLiteQueryableMutableStore<>(objectMapper, keyGenerator, openDefaultConnection(), clazz, getQueryableTypeDescriptor(clazz), parentIds, lock); + QueryableTypeDescriptor queryableTypeDescriptor = getQueryableTypeDescriptor(clazz); + return new SQLiteQueryableMutableStore<>( + objectMapper, + keyGenerator, + openDefaultConnection(), + clazz, + queryableTypeDescriptor, + parentIds, + lock, + mustBeReadOnly(queryableTypeDescriptor, parentIds) + ); + } + + private boolean mustBeReadOnly(QueryableTypeDescriptor queryableTypeDescriptor, String... parentIds) { + for (int i = 0; i < queryableTypeDescriptor.getTypes().length; i++) { + if (queryableTypeDescriptor.getTypes()[i].startsWith(Repository.class.getName()) && parentIds.length > i) { + String repositoryId = parentIds[i]; + if (repositoryId != null && readOnlyChecker.isReadOnly(repositoryId)) { + return true; + } + } + } + return false; } private QueryableTypeDescriptor getQueryableTypeDescriptor(Class clazz) { diff --git a/scm-persistence/src/test/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStoreTest.java b/scm-persistence/src/test/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStoreTest.java index 15f57e2c34..22de59c125 100644 --- a/scm-persistence/src/test/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStoreTest.java +++ b/scm-persistence/src/test/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStoreTest.java @@ -21,9 +21,12 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import sonia.scm.group.Group; +import sonia.scm.repository.Repository; import sonia.scm.store.IdGenerator; import sonia.scm.store.QueryableMutableStore; import sonia.scm.store.QueryableStore; +import sonia.scm.store.StoreReadOnlyException; import sonia.scm.user.User; import java.nio.file.Path; @@ -36,6 +39,7 @@ import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertThrows; @SuppressWarnings("resource") class SQLiteQueryableMutableStoreTest { @@ -510,6 +514,75 @@ class SQLiteQueryableMutableStoreTest { assertThat(crewmateStoreForShipTwo.getAll()).hasSize(2); } } + } + @Test + void shouldThrowExceptionWhenReadOnlyStoreIsModified() { + StoreTestBuilder storeTestBuilder = new StoreTestBuilder( + connectionString, + Repository.class.getName() + ".class" + ).readOnly("42"); + + assertThrows(StoreReadOnlyException.class, () -> + storeTestBuilder + .withIds("42") + .put("tricia", new User("trillian")) + ); + } + + @Test + void shouldThrowExceptionWhenStoreForReadOnlyRepositoryAsFirstParentIsModified() { + StoreTestBuilder storeTestBuilder = new StoreTestBuilder( + connectionString, + Repository.class.getName() + ".class", + Group.class.getName() + ".class" + ).readOnly("42"); + + assertThrows(StoreReadOnlyException.class, () -> + storeTestBuilder + .withIds("42", "hitchhikers") + .put("tricia", new User("trillian")) + ); + } + + @Test + void shouldThrowExceptionWhenStoreForReadOnlyRepositoryAsSecondParentIsModified() { + StoreTestBuilder storeTestBuilder = new StoreTestBuilder( + connectionString, + Group.class.getName() + ".class", + Repository.class.getName() + ".class" + ).readOnly("42"); + + assertThrows(StoreReadOnlyException.class, () -> + storeTestBuilder + .withIds("hitchhikers", "42") + .put("tricia", new User("trillian")) + ); + } + + @Test + void shouldWriteToWritableStore() { + StoreTestBuilder storeTestBuilder = new StoreTestBuilder( + connectionString, + Repository.class.getName() + ".class" + ).readOnly("42"); + + SQLiteQueryableMutableStore store = storeTestBuilder.withIds("23"); + store.put("tricia", new User("trillian")); + + assertThat(store.get("tricia")).isNotNull(); + } + + @Test + void shouldWriteToWritableStoreWithDifferentParentClass() { + StoreTestBuilder storeTestBuilder = new StoreTestBuilder( + connectionString, + Group.class.getName() + ".class" + ).readOnly("42"); + + SQLiteQueryableMutableStore store = storeTestBuilder.withIds("42"); + store.put("tricia", new User("trillian")); + + assertThat(store.get("tricia")).isNotNull(); } } diff --git a/scm-persistence/src/test/java/sonia/scm/store/sqlite/StoreTestBuilder.java b/scm-persistence/src/test/java/sonia/scm/store/sqlite/StoreTestBuilder.java index bdc4aa0846..b72b076352 100644 --- a/scm-persistence/src/test/java/sonia/scm/store/sqlite/StoreTestBuilder.java +++ b/scm-persistence/src/test/java/sonia/scm/store/sqlite/StoreTestBuilder.java @@ -18,17 +18,25 @@ package sonia.scm.store.sqlite; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import org.mockito.Mockito; +import sonia.scm.repository.Repository; +import sonia.scm.repository.RepositoryReadOnlyChecker; import sonia.scm.security.UUIDKeyGenerator; import sonia.scm.store.IdGenerator; import sonia.scm.store.QueryableMaintenanceStore; import sonia.scm.store.QueryableStore; import sonia.scm.user.User; +import java.util.Collection; +import java.util.HashSet; import java.util.List; import static com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES; import static com.fasterxml.jackson.databind.SerializationFeature.WRITE_DATES_AS_TIMESTAMPS; import static com.fasterxml.jackson.databind.SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.when; import static sonia.scm.store.sqlite.QueryableTypeDescriptorTestData.createDescriptor; class StoreTestBuilder { @@ -48,6 +56,8 @@ class StoreTestBuilder { private final String[] parentClasses; private final IdGenerator idGenerator; + private Collection readOnlyRepositoryIds = new HashSet<>(); + StoreTestBuilder(String connectionString, String... parentClasses) { this(connectionString, IdGenerator.DEFAULT, parentClasses); } @@ -80,12 +90,27 @@ class StoreTestBuilder { return createStoreFactory(clazz).getMutable(clazz, ids); } + StoreTestBuilder readOnly(String... repositoryIds) { + readOnlyRepositoryIds.addAll(List.of(repositoryIds)); + return this; + } + private SQLiteQueryableStoreFactory createStoreFactory(Class clazz) { + RepositoryReadOnlyChecker readOnlyChecker; + if (readOnlyRepositoryIds.isEmpty()) { + readOnlyChecker = new RepositoryReadOnlyChecker(); + } else { + readOnlyChecker = Mockito.mock(RepositoryReadOnlyChecker.class); + when(readOnlyChecker.isReadOnly(argThat((String repoId) -> readOnlyRepositoryIds.contains(repoId)))) + .thenReturn(true); + when(readOnlyChecker.isReadOnly(any(Repository.class))).thenCallRealMethod(); + } return new SQLiteQueryableStoreFactory( connectionString, mapper, new UUIDKeyGenerator(), - List.of(createDescriptor("", clazz.getName(), parentClasses, idGenerator)) + List.of(createDescriptor("", clazz.getName(), parentClasses, idGenerator)), + readOnlyChecker ); } } diff --git a/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java b/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java index 2991727ef2..961b2e1b68 100644 --- a/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java +++ b/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java @@ -35,6 +35,7 @@ import org.junit.jupiter.api.extension.ParameterContext; import org.junit.jupiter.api.extension.ParameterResolutionException; import org.junit.jupiter.api.extension.ParameterResolver; import sonia.scm.plugin.QueryableTypeDescriptor; +import sonia.scm.repository.RepositoryReadOnlyChecker; import sonia.scm.security.UUIDKeyGenerator; import sonia.scm.store.sqlite.SQLiteQueryableStoreFactory; import sonia.scm.util.IOUtil; @@ -101,7 +102,8 @@ public class QueryableStoreExtension implements ParameterResolver, BeforeEachCal connectionString, mapper, new UUIDKeyGenerator(), - queryableTypeDescriptors + queryableTypeDescriptors, + new RepositoryReadOnlyChecker() ) ); }