From a1ff8b6369f746cbb5ef3267bfa95f846cc9e81b Mon Sep 17 00:00:00 2001 From: James Maes Date: Thu, 12 Nov 2026 10:55:52 -0600 Subject: [PATCH 1/2] fix(permissions): let the service write its own tables when core actions enforce permissions In a host that sets QInstance.enforcePermissionsInCoreActions, core checks table permissions inside InsertAction, UpdateAction and DeleteAction for every outermost call. Db.asSystem starts a new context, so each write it makes is outermost, and core asks ObjectStoragePermissionChecker about it. The checker refused every write of a table that is not admin-editable ("This storage table changes only through storage actions.") to every session, the system user the service itself writes as included. Seen in a host application: every StorageBucket.put stored the object and logged "Could not write a storage operation log row" with QPermissionDeniedException, so object_storage_operation had no row for any stored file. The same refusal applies to every other Db.asSystem write of those tables. The checker now lets a QSystemUserSession through for those tables. Such a session is built by server code only; no authentication module gives one to a request. A person is refused as before, the storage administrator included (DbSystemSessionTest .testServiceWritesItsOwnTablesWhenCoreActionsEnforcePermissions, which failed before with no operation row). Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 3 ++ .../ObjectStoragePermissionChecker.java | 16 +++++++- .../internal/DbSystemSessionTest.java | 38 +++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c157cc0..ce966ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ All notable changes to this project are documented here. The format follows Keep - Foreign keys on every relation between the qbit tables (`fk__`), generated by `ObjectStorageSchema`: owned observed state (capabilities, folders, objects, versions, usage) cascades from its bucket or object, `object.folder_id` and `object_reference.object_id` are set null, and a bucket's connection, header and protection rules and operation log restrict. Each key is added by an idempotent block that first records rows whose parent is gone in `qqq_fk_quarantine` and clears (nullable column) or deletes (required column) them, so it also upgrades an existing schema. New indexes `ix_object_storage_bucket_1` (connection_id), `ix_object_storage_header_rule_1` and `ix_object_storage_protection_rule_1` (bucket_id) back the keys. Upgrade: run the generated statements again (or the host's copy of them). - Conformance suite against MinIO and Garage (Testcontainers), service, index and action tests, and the index and actions on PostgreSQL. +### Fixed +- The service's own writes are no longer refused in a host that enforces permissions inside the core actions (`QInstance.enforcePermissionsInCoreActions`). `Db.asSystem` starts a new context, so each of its writes is an outermost action there and core asks `ObjectStoragePermissionChecker`, which refused every write of a table nobody may write directly, to every session. The `object_storage_operation` row of every stored object was lost (logged as "Could not write a storage operation log row", the object itself was stored), and so was each other system write of those tables. The checker now lets a `QSystemUserSession` write them; a person is refused as before, `object_storage.admin` included. + ### Removed - The QRun-IO/qqq#1165 stand-ins: `SessionCredentialsClient` (session tokens now go through core `S3BackendMetaData.sessionToken` / `S3ClientFactory`, including for the presigner) and the file reading in `SecretRefs` (core `QMetaDataVariableInterpreter` resolves `${file.PATH}`). **Behavior change:** core reads `${file.PATH}` only inside its allowed roots, which are `/var/run/secrets` and `/run/secrets` by default. A host that mounts storage secrets elsewhere must set `qqq.fileReference.roots` or `QQQ_FILE_REFERENCE_ROOTS`. The qbit's default allow-list (`file./var/run/secrets/storage/*`) is already inside the default roots. - None for the health migration; both Test connection record actions, including on-demand write tests, are retained. diff --git a/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java b/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java index c93b933..f49e16a 100644 --- a/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java +++ b/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java @@ -38,6 +38,7 @@ import com.kingsrook.qqq.backend.core.model.metadata.permissions.MetaDataWithPer import com.kingsrook.qqq.backend.core.model.metadata.processes.QProcessMetaData; import com.kingsrook.qqq.backend.core.model.metadata.tables.QTableMetaData; import com.kingsrook.qqq.backend.core.model.session.QSession; +import com.kingsrook.qqq.backend.core.model.session.QSystemUserSession; /******************************************************************************* @@ -46,7 +47,8 @@ import com.kingsrook.qqq.backend.core.model.session.QSession; ** - writes to the editable config tables (connection operational fields, header ** rules, protection rules, bucket thresholds) need object_storage.admin; ** observed and history tables are never written through the UI or API (they - ** change only through actions and the service); + ** change only through actions and the service, whose own writes run as the + ** core system user and are let through); ** - each process needs the permission registered for it (default admin). *******************************************************************************/ public class ObjectStoragePermissionChecker implements CustomPermissionChecker @@ -65,6 +67,18 @@ public class ObjectStoragePermissionChecker implements CustomPermissionChecker String needed = required(actionInput, metaData); if(needed == null) { + ////////////////////////////////////////////////////////////////////////////////////// + // nobody writes these tables directly - but the service itself does, as the core // + // system user (Db.asSystem). a host that enforces permissions inside the core // + // actions asks this checker about that write too (asSystem starts a new context, // + // so the write is an outermost action), and refusing it lost the operation log row // + // of every stored object. a system user session is built by server code only: no // + // authentication module gives one to a request // + ////////////////////////////////////////////////////////////////////////////////////// + if(QContext.getQSession() instanceof QSystemUserSession) + { + return; + } throw (new QPermissionDeniedException("This storage table changes only through storage actions.")); } if(!has(needed)) diff --git a/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java b/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java index 29af9c7..02d310e 100644 --- a/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java +++ b/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java @@ -21,15 +21,26 @@ package com.kingsrook.qbits.objectstorage.internal; +import java.util.List; import com.kingsrook.qbits.objectstorage.ObjectStorageQBitConfig; +import com.kingsrook.qbits.objectstorage.api.StorageCaller; +import com.kingsrook.qbits.objectstorage.model.ObjectStorageTables; +import com.kingsrook.qbits.objectstorage.permissions.ObjectStoragePermissions; import com.kingsrook.qbits.objectstorage.testsupport.StorageTestSupport; +import com.kingsrook.qqq.backend.core.actions.tables.InsertAction; +import com.kingsrook.qqq.backend.core.actions.tables.QueryAction; import com.kingsrook.qqq.backend.core.context.QContext; +import com.kingsrook.qqq.backend.core.exceptions.QPermissionDeniedException; +import com.kingsrook.qqq.backend.core.model.actions.tables.insert.InsertInput; +import com.kingsrook.qqq.backend.core.model.actions.tables.query.QueryInput; +import com.kingsrook.qqq.backend.core.model.data.QRecord; import com.kingsrook.qqq.backend.core.model.metadata.security.QSecurityKeyType; import com.kingsrook.qqq.backend.core.model.session.QSession; import com.kingsrook.qqq.backend.core.model.session.QSystemUserSession; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; /******************************************************************************* @@ -69,4 +80,31 @@ class DbSystemSessionTest assertThat(inside.hasSecurityKeyValue("tenantAllAccess", true)).isTrue(); assertThat(QContext.getQSession()).isSameAs(caller); } + + + + /******************************************************************************* + ** A host that enforces permissions inside the core actions + ** (QInstance.enforcePermissionsInCoreActions) still gets its operation log + ** row: the service's own write of a table nobody may write directly, made as + ** the system user, is not refused by the qbit's own permission checker. A + ** person is still refused that write, the storage administrator included. + *******************************************************************************/ + @Test + void testServiceWritesItsOwnTablesWhenCoreActionsEnforcePermissions() throws Exception + { + ObjectStorageQBitConfig config = new ObjectStorageQBitConfig().withBackendName(StorageTestSupport.MEMORY).withBucketScoping(false); + StorageTestSupport.setUp(config, instance -> instance.setEnforcePermissionsInCoreActions(true)); + + QContext.setQSession(new QSession().withPermissions(ObjectStoragePermissions.ADMIN)); + new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("stored/by/the/service").write(); + + List logged = new QueryAction().execute(new QueryInput(ObjectStorageTables.OPERATION)).getRecords(); + assertThat(logged).as("the operation log row of a stored object").extracting(row -> row.getValueString("objectKey")).containsExactly("stored/by/the/service"); + + assertThatThrownBy(() -> new InsertAction().execute(new InsertInput(ObjectStorageTables.OPERATION).withRecords(List.of(new QRecord().withValue("action", "PUT"))))) + .as("a person writing the operation log directly").isInstanceOf(QPermissionDeniedException.class) + .hasMessage("This storage table changes only through storage actions."); + assertThat(new QueryAction().execute(new QueryInput(ObjectStorageTables.OPERATION)).getRecords()).hasSize(1); + } } -- 2.54.0 From 59b0ea2ed64d7cd7139818a4537cdd4865ce7b47 Mon Sep 17 00:00:00 2001 From: James Maes Date: Thu, 12 Nov 2026 11:28:59 -0600 Subject: [PATCH 2/2] fix(permissions): mark the service's own writes with the core bypass; report a failed audit row Review of the first fix found the checker's pass too broad: it let any QSystemUserSession insert, update and delete every table nobody may write directly, so server code running as the system user could edit or delete operation log rows. - ObjectStoragePermissionChecker no longer looks at the session class. A write of those tables that reaches it is refused to every session, the core system user included. - Db.insert, Db.update and Db.delete mark their input with core's per-call permission bypass (withPermissionBypass, Db.OWN_WRITE_REASON), so core does not ask about the service's own writes when a host enforces permissions inside the core actions. - A failed operation log row is logged at ERROR, raises the storage.operation_log.failed alert on the StorageAlertSink (resolved by the next row written for the bucket) and is counted in OperationLog.failedWriteCount(). A row the insert action hands back with a record error is now detected too. The operation still succeeds. Tests: the service's own log, index insert, update and delete under core enforcement, with the acting person on the row; a system session outside Db refused insert, update and delete of an operation row; update and delete of an operation row refused to a person, the administrator and the system user; the failure signal. Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 3 +- README.md | 1 + .../qbits/objectstorage/internal/Db.java | 24 ++- .../internal/ObjectStorageRuntime.java | 1 + .../objectstorage/internal/OperationLog.java | 104 +++++++++++- .../ObjectStoragePermissionChecker.java | 24 ++- .../internal/DbSystemSessionTest.java | 153 ++++++++++++++++-- .../internal/OperationLogTest.java | 125 ++++++++++++++ 8 files changed, 398 insertions(+), 37 deletions(-) create mode 100644 src/test/java/com/kingsrook/qbits/objectstorage/internal/OperationLogTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index ce966ef..06c3f53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,8 @@ All notable changes to this project are documented here. The format follows Keep - Conformance suite against MinIO and Garage (Testcontainers), service, index and action tests, and the index and actions on PostgreSQL. ### Fixed -- The service's own writes are no longer refused in a host that enforces permissions inside the core actions (`QInstance.enforcePermissionsInCoreActions`). `Db.asSystem` starts a new context, so each of its writes is an outermost action there and core asks `ObjectStoragePermissionChecker`, which refused every write of a table nobody may write directly, to every session. The `object_storage_operation` row of every stored object was lost (logged as "Could not write a storage operation log row", the object itself was stored), and so was each other system write of those tables. The checker now lets a `QSystemUserSession` write them; a person is refused as before, `object_storage.admin` included. +- The service's own writes are no longer refused in a host that enforces permissions inside the core actions (`QInstance.enforcePermissionsInCoreActions`). `Db.asSystem` starts a new context, so each of its writes is an outermost action there and core asks `ObjectStoragePermissionChecker`, which refuses every write of a table nobody may write directly, to every session. The `object_storage_operation` row of every stored object was lost (logged as "Could not write a storage operation log row", the object itself was stored), and so was each other system write of those tables. `Db.insert`, `Db.update` and `Db.delete` now mark their input with core's per-call permission bypass (`withPermissionBypass`, reason `Db.OWN_WRITE_REASON`), so core does not ask about the service's own writes. The checker is unchanged in what it refuses: a write of those tables that does not carry a bypass is refused to a person, to `object_storage.admin` and to the core system user alike, so server code running as the system user cannot edit or delete operation log rows through an enforced action. +- An operation log row that could not be written is no longer only a warning. It is logged at ERROR, raises the `storage.operation_log.failed` alert (`StorageAlertSink`, severity `error`, dedupe key `bucket:`, resolved by the next row written for that bucket) and is counted in `OperationLog.failedWriteCount()` (per node, since start). A row the insert action hands back with a record error (a refused value, a record lock) is now detected as well; before, it was dropped without any log line from the qbit. The operation the row describes still succeeds. ### Removed - The QRun-IO/qqq#1165 stand-ins: `SessionCredentialsClient` (session tokens now go through core `S3BackendMetaData.sessionToken` / `S3ClientFactory`, including for the presigner) and the file reading in `SecretRefs` (core `QMetaDataVariableInterpreter` resolves `${file.PATH}`). **Behavior change:** core reads `${file.PATH}` only inside its allowed roots, which are `/var/run/secrets` and `/run/secrets` by default. A host that mounts storage secrets elsewhere must set `qqq.fileReference.roots` or `QQQ_FILE_REFERENCE_ROOTS`. The qbit's default allow-list (`file./var/run/secrets/storage/*`) is already inside the default roots. diff --git a/README.md b/README.md index a897d98..4426803 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,7 @@ It is insert-only and inserts a **disabled** generic header rule, `default-mutab - **Previews** are short-lived (5 minute) presigned GETs, so the Next dashboard's `img-src` must allow the storage host (it allows `https:` by default). Video, audio and PDF previews need same-origin media (the dashboard's `media-src` and `frame-src` are `'self'`); a same-origin download route is the planned follow-up. Until then those files show the file card with download and open links. - **Scheduler.** Set `withSchedulerName(...)` to run the nightly scan, HEAD backlog, reference scan, usage snapshot and trash purge. Without it, run Rescan bucket by hand. - **Alerts.** With the Alerts qbit installed, bind a `StorageAlertSink` adapter (`ObjectStorageRuntime.setAlertSink`); without it every condition is still visible on the home and in the tables. +- **A missing audit row.** A storage operation succeeds even when its `object_storage_operation` row cannot be written, and that row is then the one condition no table shows. It is logged at ERROR ("Could not write a storage operation log row"), raised as the `storage.operation_log.failed` alert (severity `error`, dedupe key `bucket:`, resolved by the next row written for that bucket) and counted in `OperationLog.failedWriteCount()`, per node since it started. A host without an alert sink should export that count (a gauge, or a health check of its own) and alert when it is above zero. ## Conformance diff --git a/src/main/java/com/kingsrook/qbits/objectstorage/internal/Db.java b/src/main/java/com/kingsrook/qbits/objectstorage/internal/Db.java index 24d0db2..a27cc98 100644 --- a/src/main/java/com/kingsrook/qbits/objectstorage/internal/Db.java +++ b/src/main/java/com/kingsrook/qbits/objectstorage/internal/Db.java @@ -48,9 +48,21 @@ import com.kingsrook.qqq.backend.core.model.session.QSystemUserSession; ** Small helpers for reading and writing the qbit's own tables as the system ** user (the service writes its audit log and index regardless of the calling ** user's per-bucket scope; the calling user's permissions were checked first). + ** + ** Every write made here carries core's per-call permission bypass + ** (AbstractActionInput.withPermissionBypass). That mark, not the session, is + ** what lets the service write tables nobody may write directly: a host that + ** enforces permissions inside the core actions asks ObjectStoragePermissionChecker + ** about every outermost write that does not carry a bypass, and the checker + ** refuses those tables to every session, the system user included. *******************************************************************************/ public final class Db { + /** The reason recorded on (and logged by core for) each write the service makes of its own tables. */ + public static final String OWN_WRITE_REASON = "The object storage service writes its own tables (operation log, index, capabilities, usage); the caller's storage permission was checked before the storage call."; + + + /******************************************************************************* ** Something that may throw. *******************************************************************************/ @@ -237,7 +249,9 @@ public final class Db *******************************************************************************/ public static List insert(String table, List records) throws QException { - return (new InsertAction().execute(new InsertInput(table).withRecords(records)).getRecords()); + InsertInput input = new InsertInput(table).withRecords(records); + input.withPermissionBypass(OWN_WRITE_REASON); + return (new InsertAction().execute(input).getRecords()); } @@ -259,7 +273,9 @@ public final class Db *******************************************************************************/ public static List update(String table, List records) throws QException { - return (new UpdateAction().execute(new UpdateInput(table).withRecords(records)).getRecords()); + UpdateInput input = new UpdateInput(table).withRecords(records); + input.withPermissionBypass(OWN_WRITE_REASON); + return (new UpdateAction().execute(input).getRecords()); } @@ -271,7 +287,9 @@ public final class Db { if(!ids.isEmpty()) { - new DeleteAction().execute(new DeleteInput(table).withPrimaryKeys(ids)); + DeleteInput input = new DeleteInput(table).withPrimaryKeys(ids); + input.withPermissionBypass(OWN_WRITE_REASON); + new DeleteAction().execute(input); } } diff --git a/src/main/java/com/kingsrook/qbits/objectstorage/internal/ObjectStorageRuntime.java b/src/main/java/com/kingsrook/qbits/objectstorage/internal/ObjectStorageRuntime.java index afb3354..f0de411 100644 --- a/src/main/java/com/kingsrook/qbits/objectstorage/internal/ObjectStorageRuntime.java +++ b/src/main/java/com/kingsrook/qbits/objectstorage/internal/ObjectStorageRuntime.java @@ -99,6 +99,7 @@ public final class ObjectStorageRuntime clock = Clock.systemUTC(); indexHook = null; CapabilityStore.clearCache(); + OperationLog.reset(); } diff --git a/src/main/java/com/kingsrook/qbits/objectstorage/internal/OperationLog.java b/src/main/java/com/kingsrook/qbits/objectstorage/internal/OperationLog.java index 8825093..0593402 100644 --- a/src/main/java/com/kingsrook/qbits/objectstorage/internal/OperationLog.java +++ b/src/main/java/com/kingsrook/qbits/objectstorage/internal/OperationLog.java @@ -21,10 +21,17 @@ package com.kingsrook.qbits.objectstorage.internal; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; import com.kingsrook.qbits.objectstorage.api.StorageCaller; import com.kingsrook.qbits.objectstorage.model.ObjectStorageTables; +import com.kingsrook.qqq.backend.core.cluster.ClusterCaches; +import com.kingsrook.qqq.backend.core.exceptions.QException; import com.kingsrook.qqq.backend.core.logging.QLogger; import com.kingsrook.qqq.backend.core.model.data.QRecord; +import com.kingsrook.qqq.backend.core.utils.CollectionUtils; import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; @@ -33,12 +40,22 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** every caller (UI action, app code, scheduled job) lands here. Presigned URLs ** are logged without the signature (key, method, expiry and issuer only), ** because the URL itself is a bearer token. A logging failure never fails the - ** operation it describes. + ** operation it describes, so it is reported three ways instead: an ERROR log + ** line, the storage.operation_log.failed alert (StorageAlertSink, resolved by + ** the next row written for that bucket), and failedWriteCount(). *******************************************************************************/ public final class OperationLog { private static final QLogger LOG = QLogger.getLogger(OperationLog.class); + /** The alert raised (severity error, dedupe key "bucket:" + alias) when a row could not be written. */ + public static final String ALERT_TYPE = "storage.operation_log.failed"; + + private static final AtomicLong FAILED_WRITES = new AtomicLong(); + + private static final Set UNRESOLVED = ClusterCaches.nodeLocal(ConcurrentHashMap.newKeySet(), + "the buckets whose operation log alert this node raised and has not resolved (alert state, not cached data)"); + /******************************************************************************* @@ -183,6 +200,31 @@ public final class OperationLog + /******************************************************************************* + ** How many operation log rows this node could not write since it started (or + ** since the runtime was reset). The operations themselves were carried out, so + ** anything above zero means storage changes that the audit log does not show. + ** A host that has no alert sink reads this (a metrics gauge, a health check of + ** its own) to alert on a missing audit row. + *******************************************************************************/ + public static long failedWriteCount() + { + return (FAILED_WRITES.get()); + } + + + + /******************************************************************************* + ** Forget the failures counted and the alerts raised (ObjectStorageRuntime.reset). + *******************************************************************************/ + static void reset() + { + FAILED_WRITES.set(0); + UNRESOLVED.clear(); + } + + + /******************************************************************************* ** Insert the row (as the system user), never throwing. *******************************************************************************/ @@ -196,7 +238,7 @@ public final class OperationLog try { Integer bucketId = ConfigSync.bucketId(e.bucketAlias); - Db.asSystem(() -> Db.insert(ObjectStorageTables.OPERATION, new QRecord() + QRecord written = Db.asSystem(() -> Db.insert(ObjectStorageTables.OPERATION, new QRecord() .withValue("occurredAt", ObjectStorageRuntime.clock().instant()) .withValue("actor", e.caller.describe()) .withValue("action", e.action) @@ -210,10 +252,66 @@ public final class OperationLog .withValue("providerRequestId", e.requestId) .withValue("processRunId", e.processRunId) .withValue("detail", e.detail))); + + //////////////////////////////////////////////////////////////////////////////// + // the insert action does not throw for a record it refused (a missing value, // + // a value too long, a record lock): it hands the record back with its errors // + //////////////////////////////////////////////////////////////////////////////// + if(CollectionUtils.nullSafeHasContents(written.getErrors())) + { + throw (new QException("The operation log row was refused: " + written.getErrors())); + } + written(e); } catch(Exception ex) { - LOG.warn("Could not write a storage operation log row", ex, logPair("action", e.action), logPair("key", e.key)); + failed(e, ex); + } + } + + + + /******************************************************************************* + ** A row was written: resolve the alert of an earlier failure for its bucket. + *******************************************************************************/ + private static void written(Entry e) + { + String dedupeKey = "bucket:" + e.bucketAlias; + if(UNRESOLVED.remove(dedupeKey)) + { + try + { + ObjectStorageRuntime.alertSink().resolve(ALERT_TYPE, dedupeKey); + } + catch(RuntimeException sinkFailure) + { + LOG.warn("Could not resolve the storage operation log alert", sinkFailure, logPair("bucket", e.bucketAlias)); + } + } + } + + + + /******************************************************************************* + ** A row could not be written: the operation happened and the audit log does + ** not show it. Say so where an operator sees it, without failing the operation. + *******************************************************************************/ + private static void failed(Entry e, Exception cause) + { + long count = FAILED_WRITES.incrementAndGet(); + LOG.error("Could not write a storage operation log row: the operation is missing from the audit log", cause, + logPair("action", e.action), logPair("bucket", e.bucketAlias), logPair("key", e.key), logPair("result", e.result), logPair("failedWriteCount", count)); + + String dedupeKey = "bucket:" + e.bucketAlias; + UNRESOLVED.add(dedupeKey); + try + { + ObjectStorageRuntime.alertSink().raise(ALERT_TYPE, dedupeKey, "error", "A storage operation on " + e.bucketAlias + " was not recorded in the operation log", + Map.of("bucket", String.valueOf(e.bucketAlias), "action", String.valueOf(e.action), "count", count)); + } + catch(RuntimeException sinkFailure) + { + LOG.warn("Could not raise the storage operation log alert", sinkFailure, logPair("bucket", e.bucketAlias)); } } } diff --git a/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java b/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java index f49e16a..f759267 100644 --- a/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java +++ b/src/main/java/com/kingsrook/qbits/objectstorage/permissions/ObjectStoragePermissionChecker.java @@ -38,7 +38,6 @@ import com.kingsrook.qqq.backend.core.model.metadata.permissions.MetaDataWithPer import com.kingsrook.qqq.backend.core.model.metadata.processes.QProcessMetaData; import com.kingsrook.qqq.backend.core.model.metadata.tables.QTableMetaData; import com.kingsrook.qqq.backend.core.model.session.QSession; -import com.kingsrook.qqq.backend.core.model.session.QSystemUserSession; /******************************************************************************* @@ -47,8 +46,9 @@ import com.kingsrook.qqq.backend.core.model.session.QSystemUserSession; ** - writes to the editable config tables (connection operational fields, header ** rules, protection rules, bucket thresholds) need object_storage.admin; ** observed and history tables are never written through the UI or API (they - ** change only through actions and the service, whose own writes run as the - ** core system user and are let through); + ** change only through actions and the service, whose own writes carry + ** core's per-call permission bypass, see internal.Db; no session is let + ** through, the core system user included); ** - each process needs the permission registered for it (default admin). *******************************************************************************/ public class ObjectStoragePermissionChecker implements CustomPermissionChecker @@ -67,18 +67,12 @@ public class ObjectStoragePermissionChecker implements CustomPermissionChecker String needed = required(actionInput, metaData); if(needed == null) { - ////////////////////////////////////////////////////////////////////////////////////// - // nobody writes these tables directly - but the service itself does, as the core // - // system user (Db.asSystem). a host that enforces permissions inside the core // - // actions asks this checker about that write too (asSystem starts a new context, // - // so the write is an outermost action), and refusing it lost the operation log row // - // of every stored object. a system user session is built by server code only: no // - // authentication module gives one to a request // - ////////////////////////////////////////////////////////////////////////////////////// - if(QContext.getQSession() instanceof QSystemUserSession) - { - return; - } + //////////////////////////////////////////////////////////////////////////////////////// + // nobody writes these tables directly, whatever the session: a person, the storage // + // administrator and the core system user are all refused. the service's own writes // + // (internal.Db) carry core's per-call permission bypass, so core does not ask about // + // them; a write that reaches this point did not come from the service // + //////////////////////////////////////////////////////////////////////////////////////// throw (new QPermissionDeniedException("This storage table changes only through storage actions.")); } if(!has(needed)) diff --git a/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java b/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java index 02d310e..85b06f0 100644 --- a/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java +++ b/src/test/java/com/kingsrook/qbits/objectstorage/internal/DbSystemSessionTest.java @@ -21,22 +21,30 @@ package com.kingsrook.qbits.objectstorage.internal; +import java.io.Serializable; import java.util.List; import com.kingsrook.qbits.objectstorage.ObjectStorageQBitConfig; import com.kingsrook.qbits.objectstorage.api.StorageCaller; import com.kingsrook.qbits.objectstorage.model.ObjectStorageTables; import com.kingsrook.qbits.objectstorage.permissions.ObjectStoragePermissions; import com.kingsrook.qbits.objectstorage.testsupport.StorageTestSupport; +import com.kingsrook.qqq.backend.core.actions.tables.DeleteAction; import com.kingsrook.qqq.backend.core.actions.tables.InsertAction; -import com.kingsrook.qqq.backend.core.actions.tables.QueryAction; +import com.kingsrook.qqq.backend.core.actions.tables.UpdateAction; import com.kingsrook.qqq.backend.core.context.QContext; +import com.kingsrook.qqq.backend.core.exceptions.QException; import com.kingsrook.qqq.backend.core.exceptions.QPermissionDeniedException; +import com.kingsrook.qqq.backend.core.model.actions.tables.delete.DeleteInput; import com.kingsrook.qqq.backend.core.model.actions.tables.insert.InsertInput; -import com.kingsrook.qqq.backend.core.model.actions.tables.query.QueryInput; +import com.kingsrook.qqq.backend.core.model.actions.tables.query.QQueryFilter; +import com.kingsrook.qqq.backend.core.model.actions.tables.update.UpdateInput; import com.kingsrook.qqq.backend.core.model.data.QRecord; +import com.kingsrook.qqq.backend.core.model.metadata.QInstance; import com.kingsrook.qqq.backend.core.model.metadata.security.QSecurityKeyType; import com.kingsrook.qqq.backend.core.model.session.QSession; import com.kingsrook.qqq.backend.core.model.session.QSystemUserSession; +import com.kingsrook.qqq.backend.core.model.session.QUser; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; @@ -47,10 +55,16 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; ** Db.asSystem runs work as the core system user. Token-based authentication ** modules (OAuth2, Auth0) accept a QSystemUserSession without a token and ** reject any other tokenless session, so the class matters, not just the - ** permissions. + ** permissions. The system session is not what lets the service write its own + ** tables, though: each Db write carries core's per-call permission bypass, and + ** a write that does not is refused to every session. *******************************************************************************/ class DbSystemSessionTest { + private static final String REFUSAL = "This storage table changes only through storage actions."; + + + /******************************************************************************* ** Reset the runtime and context after each test. *******************************************************************************/ @@ -86,25 +100,134 @@ class DbSystemSessionTest /******************************************************************************* ** A host that enforces permissions inside the core actions ** (QInstance.enforcePermissionsInCoreActions) still gets its operation log - ** row: the service's own write of a table nobody may write directly, made as - ** the system user, is not refused by the qbit's own permission checker. A - ** person is still refused that write, the storage administrator included. + ** row, and the row names the person who acted: the caller is captured before + ** the write moves to the system user. The other tables the service keeps + ** (here the object index) are written, changed and removed the same way. *******************************************************************************/ @Test void testServiceWritesItsOwnTablesWhenCoreActionsEnforcePermissions() throws Exception { - ObjectStorageQBitConfig config = new ObjectStorageQBitConfig().withBackendName(StorageTestSupport.MEMORY).withBucketScoping(false); - StorageTestSupport.setUp(config, instance -> instance.setEnforcePermissionsInCoreActions(true)); + enforcingInstance(); - QContext.setQSession(new QSession().withPermissions(ObjectStoragePermissions.ADMIN)); + QContext.setQSession(new QSession().withUser(new QUser().withIdReference("pat@example.com").withFullName("Pat Example")).withPermissions(ObjectStoragePermissions.WRITE)); new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("stored/by/the/service").write(); - List logged = new QueryAction().execute(new QueryInput(ObjectStorageTables.OPERATION)).getRecords(); - assertThat(logged).as("the operation log row of a stored object").extracting(row -> row.getValueString("objectKey")).containsExactly("stored/by/the/service"); + assertThat(operations()).as("the operation log row of a stored object").singleElement().satisfies(row -> + { + assertThat(row.getValueString("objectKey")).isEqualTo("stored/by/the/service"); + assertThat(row.getValueString("actor")).as("the person who acted, not the system user that wrote the row").isEqualTo("pat@example.com"); + }); + assertThat(OperationLog.failedWriteCount()).isZero(); - assertThatThrownBy(() -> new InsertAction().execute(new InsertInput(ObjectStorageTables.OPERATION).withRecords(List.of(new QRecord().withValue("action", "PUT"))))) - .as("a person writing the operation log directly").isInstanceOf(QPermissionDeniedException.class) - .hasMessage("This storage table changes only through storage actions."); - assertThat(new QueryAction().execute(new QueryInput(ObjectStorageTables.OPERATION)).getRecords()).hasSize(1); + Serializable id = Db.asSystem(() -> Db.insert(ObjectStorageTables.OBJECT, new QRecord().withValue("objectKey", "a/b.txt").withValue("size", 1L))).getValue("id"); + Db.asSystem(() -> Db.update(ObjectStorageTables.OBJECT, List.of(new QRecord().withValue("id", id).withValue("size", 2L)))); + assertThat(Db.asSystem(() -> Db.first(ObjectStorageTables.OBJECT, "id", id)).getValueLong("size")).isEqualTo(2L); + Db.asSystem(() -> + { + Db.delete(ObjectStorageTables.OBJECT, List.of(id)); + return (null); + }); + assertThat(Db.asSystem(() -> Db.query(ObjectStorageTables.OBJECT))).isEmpty(); + } + + + + /******************************************************************************* + ** Being the system user is not enough to write the operation log. Server code + ** outside the service's Db (a host, another qbit) that runs as the core system + ** user is refused the insert, the update and the delete, also when it borrows + ** Db.asSystem for its session. + *******************************************************************************/ + @Test + void testSystemSessionOutsideTheServiceCannotWriteTheOperationLog() throws Exception + { + QInstance instance = enforcingInstance(); + new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("the/audited/key").write(); + Serializable id = operations().get(0).getValue("id"); + + QContext.setQSession(new QSystemUserSession(instance)); + assertRefused("insert as the system user", () -> new InsertAction().execute(new InsertInput(ObjectStorageTables.OPERATION).withRecords(List.of(new QRecord().withValue("action", "PUT"))))); + assertRefused("update as the system user", () -> new UpdateAction().execute(new UpdateInput(ObjectStorageTables.OPERATION).withRecords(List.of(new QRecord().withValue("id", id).withValue("actor", "nobody"))))); + assertRefused("delete as the system user", () -> new DeleteAction().execute(new DeleteInput(ObjectStorageTables.OPERATION).withPrimaryKey(id))); + + QContext.setQSession(new QSession()); + assertRefused("insert inside Db.asSystem, not through Db", () -> Db.asSystem(() -> new InsertAction().execute(new InsertInput(ObjectStorageTables.OPERATION).withRecords(List.of(new QRecord().withValue("action", "PUT")))))); + assertRefused("update inside Db.asSystem, not through Db", () -> Db.asSystem(() -> new UpdateAction().execute(new UpdateInput(ObjectStorageTables.OPERATION).withRecords(List.of(new QRecord().withValue("id", id).withValue("actor", "nobody")))))); + assertRefused("delete inside Db.asSystem, not through Db", () -> Db.asSystem(() -> new DeleteAction().execute(new DeleteInput(ObjectStorageTables.OPERATION).withPrimaryKey(id)))); + + assertUnchanged(); + } + + + + /******************************************************************************* + ** An operation row is not updated or deleted by an update or delete action + ** that does not carry the bypass, whoever asks: a person, the storage + ** administrator or the system user, by key or by filter. + *******************************************************************************/ + @Test + void testOperationRowIsNeverUpdatedOrDeletedWithoutTheBypass() throws Exception + { + QInstance instance = enforcingInstance(); + new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("the/audited/key").write(); + Serializable id = operations().get(0).getValue("id"); + + List sessions = List.of(new QSession(), new QSession().withPermissions(ObjectStoragePermissions.ADMIN), new QSystemUserSession(instance)); + for(QSession session : sessions) + { + QContext.setQSession(session); + String who = session.getClass().getSimpleName() + (session.hasPermission(ObjectStoragePermissions.ADMIN) ? " (admin)" : ""); + assertRefused("update by " + who, () -> new UpdateAction().execute(new UpdateInput(ObjectStorageTables.OPERATION).withRecords(List.of(new QRecord().withValue("id", id).withValue("result", "FAILED"))))); + assertRefused("delete by key by " + who, () -> new DeleteAction().execute(new DeleteInput(ObjectStorageTables.OPERATION).withPrimaryKey(id))); + assertRefused("delete by filter by " + who, () -> new DeleteAction().execute(new DeleteInput(ObjectStorageTables.OPERATION).withQueryFilter(new QQueryFilter(Db.eq("action", "PUT"))))); + } + + assertUnchanged(); + } + + + + /******************************************************************************* + ** An instance that enforces permissions inside the core actions. + *******************************************************************************/ + private static QInstance enforcingInstance() throws QException + { + ObjectStorageQBitConfig config = new ObjectStorageQBitConfig().withBackendName(StorageTestSupport.MEMORY).withBucketScoping(false); + return (StorageTestSupport.setUp(config, instance -> instance.setEnforcePermissionsInCoreActions(true))); + } + + + + /******************************************************************************* + ** The operation log, read by the service. + *******************************************************************************/ + private static List operations() throws QException + { + return (Db.asSystem(() -> Db.query(ObjectStorageTables.OPERATION))); + } + + + + /******************************************************************************* + ** The qbit's checker refused the write. + *******************************************************************************/ + private static void assertRefused(String what, ThrowingCallable write) + { + assertThatThrownBy(write).as(what).isInstanceOf(QPermissionDeniedException.class).hasMessage(REFUSAL); + } + + + + /******************************************************************************* + ** The one row the service wrote is still there, as it was written. + *******************************************************************************/ + private static void assertUnchanged() throws QException + { + assertThat(operations()).singleElement().satisfies(row -> + { + assertThat(row.getValueString("objectKey")).isEqualTo("the/audited/key"); + assertThat(row.getValueString("result")).isEqualTo("OK"); + assertThat(row.getValueString("actor")).isEqualTo("system (no user)"); + }); } } diff --git a/src/test/java/com/kingsrook/qbits/objectstorage/internal/OperationLogTest.java b/src/test/java/com/kingsrook/qbits/objectstorage/internal/OperationLogTest.java new file mode 100644 index 0000000..c716dc9 --- /dev/null +++ b/src/test/java/com/kingsrook/qbits/objectstorage/internal/OperationLogTest.java @@ -0,0 +1,125 @@ +/* + * QQQ - Low-code Application Framework for Engineers. + * Copyright (C) 2021-2025. Kingsrook, LLC + * 651 N Broad St Ste 205 # 6917 | Middletown DE 19709 | United States + * contact@kingsrook.com + * https://github.com/Kingsrook/ + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.kingsrook.qbits.objectstorage.internal; + + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import com.kingsrook.qbits.objectstorage.ObjectStorageQBitConfig; +import com.kingsrook.qbits.objectstorage.api.StorageAlertSink; +import com.kingsrook.qbits.objectstorage.api.StorageCaller; +import com.kingsrook.qbits.objectstorage.model.ObjectStorageTables; +import com.kingsrook.qbits.objectstorage.testsupport.StorageTestSupport; +import com.kingsrook.qqq.backend.core.logging.CollectedLogMessage; +import com.kingsrook.qqq.backend.core.logging.QCollectingLogger; +import com.kingsrook.qqq.backend.core.logging.QLogger; +import com.kingsrook.qqq.backend.core.model.metadata.QInstance; +import org.apache.logging.log4j.Level; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.assertThat; + + +/******************************************************************************* + ** An operation log row that cannot be written does not fail the operation it + ** describes, so an operator has to be able to see it: an ERROR log line, the + ** storage.operation_log.failed alert and OperationLog.failedWriteCount(). + *******************************************************************************/ +class OperationLogTest +{ + private final List alerts = new ArrayList<>(); + + + + /******************************************************************************* + ** Reset the runtime, the context and the logger after each test. + *******************************************************************************/ + @AfterEach + void tearDown() + { + QLogger.deactivateCollectingLoggerForClass(OperationLog.class); + StorageTestSupport.tearDown(); + } + + + + /******************************************************************************* + ** A row the insert action refuses (it hands the record back with an error, it + ** does not throw) and a row whose insert throws are both counted, logged at + ** ERROR and raised as an alert; the caller is not failed. The next row written + ** for the bucket resolves the alert and the count stays. + *******************************************************************************/ + @Test + void testFailedRowIsAnErrorAnAlertAndACount() throws Exception + { + QInstance instance = StorageTestSupport.setUp(new ObjectStorageQBitConfig().withBackendName(StorageTestSupport.MEMORY).withBucketScoping(false)); + ObjectStorageRuntime.setAlertSink(new StorageAlertSink() + { + /******************************************************************************* + ** Record the alert. + *******************************************************************************/ + @Override + public void raise(String alertType, String dedupeKey, String severity, String message, Map details) + { + alerts.add("raise " + alertType + " " + dedupeKey + " " + severity + " count=" + details.get("count") + " action=" + details.get("action")); + } + + + + /******************************************************************************* + ** Record the resolution. + *******************************************************************************/ + @Override + public void resolve(String alertType, String dedupeKey) + { + alerts.add("resolve " + alertType + " " + dedupeKey); + } + }); + QCollectingLogger logger = QLogger.activateCollectingLoggerForClass(OperationLog.class); + + new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("a").write(); + assertThat(OperationLog.failedWriteCount()).isZero(); + assertThat(alerts).isEmpty(); + + instance.getTable(ObjectStorageTables.OPERATION).getField("detail").setIsRequired(true); + new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("refused").write(); + assertThat(OperationLog.failedWriteCount()).isEqualTo(1); + assertThat(alerts).containsExactly("raise storage.operation_log.failed bucket:files error count=1 action=PUT"); + + instance.getTable(ObjectStorageTables.OPERATION).getField("detail").setIsRequired(false); + String backendName = instance.getTable(ObjectStorageTables.OPERATION).getBackendName(); + instance.getTable(ObjectStorageTables.OPERATION).setBackendName("no-such-backend"); + new OperationLog.Entry(StorageCaller.user(), "DELETE", "files").key("thrown").write(); + assertThat(OperationLog.failedWriteCount()).isEqualTo(2); + assertThat(alerts).last().isEqualTo("raise storage.operation_log.failed bucket:files error count=2 action=DELETE"); + + List errors = logger.getCollectedMessages().stream().filter(message -> Level.ERROR.equals(message.getLevel())).toList(); + assertThat(errors).hasSize(2).allSatisfy(message -> assertThat(message.getMessage()).contains("Could not write a storage operation log row")); + + instance.getTable(ObjectStorageTables.OPERATION).setBackendName(backendName); + new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("b").write(); + new OperationLog.Entry(StorageCaller.user(), "PUT", "files").key("c").write(); + assertThat(alerts).hasSize(3).last().isEqualTo("resolve storage.operation_log.failed bucket:files"); + assertThat(OperationLog.failedWriteCount()).as("the count is of this node since it started, a later row does not lower it").isEqualTo(2); + assertThat(Db.asSystem(() -> Db.query(ObjectStorageTables.OPERATION))).extracting(row -> row.getValueString("objectKey")).containsExactly("a", "b", "c"); + } +} -- 2.54.0