Implement validate-self-folder-access: HTTP 403 + audit on cross-tenant @self.folder writes (#1342)

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+### Breaking Changes
+- **`@self.folder` writes now require read access to the target folder.** Callers that POST/PUT/PATCH an object with a numeric `@self.folder` value (e.g. `"@self": {"folder": "42"}`) now receive HTTP 403 with body `{"error": "folder_access_denied", "folder": "<id>"}` if the acting user cannot read the referenced Nextcloud folder. Previously the binding was unchecked and silently created a cross-tenant link: an authenticated caller could attach an object — and all of its child file writes — to *any* user's folder by guessing or harvesting node IDs. The new check uses the user's user-folder mount and `Folder::isReadable()`, deliberately avoiding the root-folder fallback (which exists only for anonymous public file reads and is preserved on the general-purpose `getNodeById()` helper). Empty and legacy non-numeric folder values continue through the existing auto-create path unchanged. Every denial writes a forensic audit-trail entry (`action: "folder_access_denied"`) before propagating the exception. **Internal callers that legitimately need to bind to a folder outside the session user's tree must now pass an explicit `IUser $currentUser` to `FolderManagementHandler::createObjectFolderById()`.** No internal caller in the OpenRegister codebase regresses (cron jobs and import paths don't set `@self.folder`); downstream apps that already use accessible folder IDs are unaffected. See `docs/api/objects.md#self-folder-access-control-contract` for the full contract. ([#1342](https://github.com/ConductionNL/openregister/issues/1342))
+
 ### Breaking Changes
 - **`@self.files` on rendered objects is now opt-in for full file metadata.** By default, `@self.files` is a lightweight list of integer file IDs (`[123, 456, 789]`). Consumers that need full file metadata (`id`, `path`, `title`, `accessUrl`, `downloadUrl`, `type`, `extension`, `size`, `hash`, `published`, `modified`, `labels`) MUST add `_extend[]=@self.files` (or the equivalent shorthand `_extend[]=_files`) to their request. The change applies to **every** consumer of OpenRegister's render output, including `show` endpoints in dependent apps (e.g. opencatalogi `/publications/{catalogSlug}/{id}`). Migration is a one-line query parameter addition. The previous behavior — full metadata always served on show, no metadata on list — caused asymmetric responses across endpoints and paid the file-lookup cost on every show response regardless of need. The new contract is symmetric across show and list endpoints (both emit `@self.files` as IDs by default; both accept `_extend[]=@self.files` for full metadata) and is documented under the `files-render-extension` capability. **Note:** Using `_extend[]=@self.files` (or `_files`) on **list** endpoints is heavily discouraged because it triggers per-row file/tag lookups (N+1 queries scaling with page size) and will result in degraded performance. Use it only when full file metadata is genuinely required for every row. **SOLR limitation:** on SOLR/index-backed list endpoints, `_extend[]=@self.files` is not yet supported; the lightweight ID list is always returned and the response carries `@self.extend_unsupported: ["@self.files"]` so consumers can detect the mismatch programmatically. Use the database-backed path when full file metadata is required on lists.
 

docs/api/objects.md

@@ -606,6 +606,7 @@ When creating or updating objects, you can explicitly set certain @self metadata
 - **`organisation`**: Organization UUID  
 - **`published`**: Publication timestamp
 - **`depublished`**: Depublication timestamp
+- **`folder`**: Numeric Nextcloud folder ID to bind the object to (see access-control contract below)
 
 Example:
 ```json
@@ -621,6 +622,62 @@ Example:
 
 For detailed information about @self metadata handling, see [Self Metadata Handling](../development/self-metadata-handling.md).
 
+### `@self.folder` access-control contract
+
+The `@self.folder` metadata field binds an object to an existing Nextcloud folder
+by node ID. The bind is governed by an access-control check on every save:
+
+- **Empty / absent** — the system creates a new folder under the register's root
+  folder and stores the new node ID on the object. (Default behaviour, unchanged.)
+- **Legacy non-numeric** (path-style strings from older installs) — auto-create
+  proceeds as before; no access check runs.
+- **Non-empty numeric** (the format produced by current `@self.folder` writes) —
+  the acting user MUST be able to read the folder. The check uses the user's
+  user-folder mount and `Folder::isReadable()`. If either fails — folder doesn't
+  exist in the user's mount, the resolved node is a file, the folder is trashed,
+  or the user has no read permission — the save is rejected.
+
+#### Denial response shape
+
+When `@self.folder` is rejected, the endpoint returns **HTTP 403** with body:
+
+```json
+{
+  "error": "folder_access_denied",
+  "folder": "99"
+}
+```
+
+`folder` echoes the attempted node ID. The check applies uniformly across
+`POST` (create), `PUT` (update), and `PATCH` (partial update) on object endpoints.
+
+#### Acting user resolution ("self")
+
+The check resolves the acting user in this order:
+
+1. The `IUser` explicitly passed to the underlying service helper (DI / cron path).
+2. The session user (`IUserSession::getUser()`).
+3. If neither resolves, the bind is **denied** by default — there is no fail-open
+   path on `@self.folder` writes.
+
+#### Audit trail
+
+Every denial writes a forensic audit-trail entry with `action: "folder_access_denied"`,
+the actor (UID or `"system"`), the attempted folder ID, and a reason code. The
+entry is written **before** the exception is thrown, so even a caller that
+catches the exception has a record. Audit-write failures are logged at warning
+level and do **not** swallow the denial — denial is authoritative.
+
+For cleanup of stale `@self.folder` references on existing objects (folders the
+owner can no longer access), an `occ openregister:folder-audit` command is
+tracked separately as a follow-up.
+
+#### See also
+
+- Capability spec (post-archive): `openspec/specs/self-folder-access-control/spec.md`
+- Architectural context: ADR-007 (Security and Auth), ADR-008 (Backend Layering)
+- Downstream consumer benefiting from this hardening: DocuDesk's `add-dossier-schema` change.
+
 ## Security
 
 - **RBAC**: Respects role-based access control

lib/AppInfo/Application.php

@@ -438,6 +438,7 @@ function (ContainerInterface $container) {
                     userSession: $container->get('OCP\IUserSession'),
                     groupManager: $container->get('OCP\IGroupManager'),
                     logger: $container->get('Psr\Log\LoggerInterface'),
+                    auditTrailMapper: $container->get(\OCA\OpenRegister\Db\AuditTrailMapper::class),
                     fileService: null
                 );
             }

lib/Controller/ObjectsController.php

@@ -32,6 +32,7 @@
 use OCA\OpenRegister\Db\RegisterMapper;
 use OCA\OpenRegister\Db\SchemaMapper;
 use OCA\OpenRegister\Exception\CustomValidationException;
+use OCA\OpenRegister\Exception\FolderAccessDeniedException;
 use OCA\OpenRegister\Exception\ValidationException;
 use OCA\OpenRegister\Exception\RegisterNotFoundException;
 use OCA\OpenRegister\Exception\SchemaNotFoundException;
@@ -1752,6 +1753,10 @@
                 ],
                 statusCode: 422
             );
+        } catch (FolderAccessDeniedException $exception) {
+            // MUST be caught before generic \Exception to avoid being absorbed as a 403 with
+            // a non-structured body. See the `self-folder-access-control` capability spec.
+            return $this->folderAccessDeniedResponse(exception: $exception);
         } catch (\Exception $exception) {
             // Handle all other exceptions (including RBAC permission errors).
             return new JSONResponse(data: ['error' => $exception->getMessage()], statusCode: 403);
@@ -1924,6 +1929,9 @@
                 data: ['error' => $exception->getMessage(), 'errors' => $exception->getErrors()],
                 statusCode: 422
             );
+        } catch (FolderAccessDeniedException $exception) {
+            // MUST be caught before generic \Exception. See `self-folder-access-control` spec.
+            return $this->folderAccessDeniedResponse(exception: $exception);
         } catch (\Exception $exception) {
             // Handle all other exceptions (including RBAC permission errors).
             return new JSONResponse(data: ['error' => $exception->getMessage()], statusCode: 403);
@@ -2097,6 +2105,9 @@
                 data: ['error' => $exception->getMessage(), 'errors' => $exception->getErrors()],
                 statusCode: 422
             );
+        } catch (FolderAccessDeniedException $exception) {
+            // MUST be caught before generic \Exception. See `self-folder-access-control` spec.
+            return $this->folderAccessDeniedResponse(exception: $exception);
         } catch (\Exception $exception) {
             // Handle all other exceptions (including RBAC permission errors).
             $this->logger->error(
@@ -2320,7 +2331,7 @@
             $deleteHandler = $objectService->getDeleteHandler();
             $analysis      = $deleteHandler->canDelete($objectEntity);
 
             return new JSONResponse(data: $analysis->toArray(), statusCode: 200);
         } catch (\OCP\AppFramework\Db\DoesNotExistException $exception) {
             return new JSONResponse(data: ['error' => 'Object not found'], statusCode: 404);
         } catch (\Exception $exception) {
@@ -3476,4 +3487,29 @@
 
         return $result;
     }//end stripEmptyValues()
+
+    /**
+     * Build the structured HTTP 403 response for a folder-access denial.
+     *
+     * Per the `self-folder-access-control` capability spec, every save
+     * endpoint that propagates `FolderAccessDeniedException` MUST return
+     * status 403 with body `{ "error": "folder_access_denied", "folder": "<requested-id>" }`.
+     *
+     * Centralised here so the three save endpoints (create / update / postPatch)
+     * stay in sync without copy-pasting the response shape.
+     *
+     * @param FolderAccessDeniedException $exception The denial exception carrying the attempted folder ID.
+     *
+     * @return JSONResponse HTTP 403 with the structured body.
+     */
+    private function folderAccessDeniedResponse(FolderAccessDeniedException $exception): JSONResponse
+    {
+        return new JSONResponse(
+            data: [
+                'error'  => 'folder_access_denied',
+                'folder' => $exception->getAttemptedFolderId(),
+            ],
+            statusCode: 403
+        );
+    }//end folderAccessDeniedResponse()
 }//end class

lib/Exception/FolderAccessDeniedException.php

@@ -0,0 +1,92 @@
+<?php
+
+/**
+ * Class FolderAccessDeniedException
+ *
+ * Thrown when a `@self.folder` write attempts to bind an object to a folder
+ * that the acting user cannot read.
+ *
+ * @category  Exception
+ * @package   OCA\OpenRegister\Exception
+ * @author    Conduction Development Team <dev@conduction.nl>
+ * @copyright 2024 Conduction B.V.
+ * @license   EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ * @version   GIT: <git-id>
+ * @link      https://OpenRegister.app
+ *
+ * @spec openspec/changes/validate-self-folder-access/specs/self-folder-access-control/spec.md
+ */
+
+namespace OCA\OpenRegister\Exception;
+
+use Exception;
+
+/**
+ * Exception thrown when a `@self.folder` bind is denied.
+ *
+ * Raised by `FolderManagementHandler::createObjectFolderById()` when:
+ *  - the supplied folder ID does not resolve in the acting user's user-folder mount,
+ *  - the resolved node is not a `Folder` (e.g. a file ID was supplied),
+ *  - the resolved folder is not readable by the acting user (`Folder::isReadable() === false`).
+ *
+ * Controllers MUST catch this exception specifically (not generic `\Exception`)
+ * and map it to HTTP 403 with a structured body of the form
+ * `{"error": "folder_access_denied", "folder": "<requested-id>"}`.
+ *
+ * The class extends `\Exception` directly — NOT `OCP\Files\NotPermittedException`
+ * or any other Nextcloud exception — so generic catch-blocks for those exceptions
+ * do not silently absorb a denial.
+ *
+ * @category Exception
+ * @package  OCA\OpenRegister\Exception
+ *
+ * @author    Conduction Development Team <dev@conduction.nl>
+ * @copyright 2024 Conduction B.V.
+ * @license   EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ *
+ * @version GIT: <git-id>
+ *
+ * @link https://OpenRegister.app
+ *
+ * @phpstan-consistent-constructor
+ */
+class FolderAccessDeniedException extends Exception
+{
+
+    /**
+     * The folder ID the caller attempted to bind to.
+     *
+     * @var string
+     */
+    private string $attemptedFolderId;
+
+    /**
+     * FolderAccessDeniedException constructor.
+     *
+     * @param string         $attemptedFolderId The folder ID the caller attempted to bind to.
+     * @param int            $code              HTTP status code (default: 403 Forbidden).
+     * @param Exception|null $previous          The previous exception that caused this one, if any.
+     */
+    public function __construct(string $attemptedFolderId, int $code=403, ?Exception $previous=null)
+    {
+        $this->attemptedFolderId = $attemptedFolderId;
+
+        $message = "Access to folder '".$attemptedFolderId."' is denied for the acting user.";
+        parent::__construct(message: $message, code: $code, previous: $previous);
+
+    }//end __construct()
+
+    /**
+     * Get the folder ID the caller attempted to bind to.
+     *
+     * Used by controller error handlers to populate the `folder` field of
+     * the structured 403 response body.
+     *
+     * @return string
+     */
+    public function getAttemptedFolderId(): string
+    {
+        return $this->attemptedFolderId;
+
+    }//end getAttemptedFolderId()
+}//end class