fix(validate-self-folder-access): address PR #1431 third-pass review (1 blocker + 2 concerns)

Closes the three findings from Wilco's 3rd-pass review (commit d0b2839).

**🔴 Blocker — class docblock still taught the vulnerable response shape:**
`lib/Exception/FolderAccessDeniedException.php:34` documented
`{"error": "folder_access_denied", "folder": "<requested-id>"}` as the
canonical 403 body, contradicting every other artifact (code, controller,
spec, `docs/api/objects.md`) that says the body MUST NOT echo the
attempted folder ID. The next endpoint author copying the pattern from
the docblock would reintroduce the exact enumeration oracle this PR
closes. Updated the class docblock to:
- State the correct body shape (`{"error": "folder_access_denied"}` only).
- Spell out the enumeration-oracle rationale inline so the rationale
  travels with the code.
- Cross-reference `ObjectsController::folderAccessDeniedResponse()` as
  the canonical mapping site.
- Re-scope `getAttemptedFolderId()`'s docblock to "server-side audit
  logging only — MUST NOT be in the HTTP response body".

**🟡 Concern — `assertObjectFolderAccessible` hardcoded `currentUser: null`:**
The proxy method had no `?IUser $currentUser` parameter, so non-HTTP
callers (cron, import pipelines, event listeners) fell through to
`IUserSession::getUser()` → null → default-deny on every existing-
binding save. Contradicted the CHANGELOG's own "explicit IUser
$currentUser" contract.

Plumbed the parameter through three call sites:
- `FileService::assertObjectFolderAccessible(ObjectEntity $object, ?IUser $currentUser=null)` —
  forwards verbatim to `FolderManagementHandler::assertFolderIsAccessible`
  which already accepts the same parameter with the same fallback
  semantics (explicit → session → null = default-deny).
- `ObjectService::ensureObjectFolder(?string $uuid, ?IUser $currentUser=null)` —
  plumbs the user through to the proxy.
- `ObjectService::saveObject(..., ?IUser $currentUser=null)` — public
  surface gains the parameter as the last positional arg (back-compat
  preserved; existing callers pass nothing and get the same session-
  resolution behaviour they had before). Non-HTTP callers can now
  pass an explicit acting user end-to-end.

Docblocks at each level document the resolution precedence + call out
the non-HTTP-caller requirement.

**🟡 Concern — defense-in-depth re-validation was uncached:**
`ensureObjectFolder()` runs `getUserFolder() + getById()` on every
save for any folder-bound object — real Nextcloud filesystem I/O
per call. Bulk imports / cascade saves of the same object pay this
cost every iteration despite the (uid, folderId) tuple being a
stable access-grant key for the lifetime of a request.

Added a per-request memoization map on `FileService`
(`folderAccessRevalidationCache`) keyed `"{uid}:{folderId}"`. Only
successful re-validations are cached; denials re-throw and re-check
on retry (so a freshly-mounted-then-revoked folder still gets a
correct verdict on the next call). The cache lives for the lifetime
of the FileService instance (one HTTP request), and access changes
do not propagate mid-request anyway, so a cache hit returns the
same verdict the live check would.

**Verification:**
- PHPCS clean on all 3 touched files.
- PHPStan clean.
- `FolderAccessDeniedExceptionTest` (3 tests / 7 assertions) still
  green — Wilco confirmed in pass 3 that this test locks both the
  HTTP-agnostic-code invariant and the `HTTP_STATUS === 403`
  invariant, neither of which this commit touches.
- `testPostPatchReturns403WithStructuredBodyOnFolderAccessDenied` is
  pre-existing failure on the head SHA (not introduced or worsened
  by this commit — verified by git stash).

Refs: #1431, Wilco's third-pass review
`4335769385`

Author: rjzondervan

lib/Exception/FolderAccessDeniedException.php

@@ -30,8 +30,17 @@
  *  - 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>"}`.
+ * and map it to HTTP 403 with the structured body `{"error": "folder_access_denied"}`.
+ *
+ * **The response body MUST NOT echo the attempted folder ID.** Including it
+ * would re-create the enumeration oracle the `self-folder-access-control`
+ * capability spec was written to close: a caller probing `@self.folder` with
+ * sequential integers could distinguish "folder exists but I can't read it"
+ * (403 + id) from "folder does not exist" (auto-create / no-op) just by
+ * inspecting the response shape. The attempted id remains available via
+ * `getAttemptedFolderId()` for server-side audit logging only — see
+ * `ObjectsController::folderAccessDeniedResponse()` for the canonical
+ * controller-side mapping.
  *
  * The class extends `\Exception` directly — NOT `OCP\Files\NotPermittedException`
  * or any other Nextcloud exception — so generic catch-blocks for those exceptions
@@ -96,8 +105,9 @@ public function __construct(string $attemptedFolderId, int $code=0, ?Exception $
     /**
      * 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.
+     * **Server-side use only** — for audit-trail entries and structured
+     * log lines. MUST NOT be included in the HTTP response body (see the
+     * class docblock for the enumeration-oracle rationale).
      *
      * @return string
      */

lib/Service/FileService.php

@@ -319,6 +319,25 @@ class FileService
      */
     private FileAuditHandler $fileAuditHandler;
 
+    /**
+     * Per-request memoization for the defense-in-depth folder-access
+     * re-validation done by `assertObjectFolderAccessible`.
+     *
+     * Keyed `"{uid}:{folderId}"` (or `"__no_user__:{folderId}"` when no
+     * acting user resolves). Only successful re-validations are cached;
+     * denials re-throw and re-check on retry. The cache lives for the
+     * lifetime of the FileService instance — i.e. one HTTP request — so
+     * bulk imports / cascade saves stop re-running
+     * `getUserFolder() + getById()` filesystem I/O on every iteration.
+     * PR #1431 concern.
+     *
+     * Access changes are not propagated mid-request anyway, so a cache
+     * hit here returns the same verdict the live check would.
+     *
+     * @var array<string, true>
+     */
+    private array $folderAccessRevalidationCache = [];
+
     /**
      * Root folder name for all OpenRegister files.
      *
@@ -844,24 +863,47 @@ public function getObjectFolder(ObjectEntity|string $objectEntity, int|string|nu
      * is non-numeric (legacy path-style folder, handled by the
      * auto-create branch in ensureObjectFolder).
      *
-     * @param ObjectEntity $object The existing object whose folder must be re-validated.
+     * **Acting user resolution.** `$currentUser` is forwarded verbatim
+     * to `assertFolderIsAccessible` and follows that method's
+     * documented precedence: when non-null it is used as-is, otherwise
+     * `IUserSession::getUser()` is consulted, otherwise the bind is
+     * denied. Non-HTTP callers (cron, import pipelines, event listeners)
+     * MUST pass an explicit `$currentUser` to avoid the
+     * session-user-is-null → default-deny path; HTTP callers can omit
+     * the argument and rely on session resolution.
+     *
+     * @param ObjectEntity $object      The existing object whose folder must be re-validated.
+     * @param IUser|null   $currentUser Explicit acting user; falls back to session resolution.
      *
      * @return void
      *
      * @throws \OCA\OpenRegister\Exception\FolderAccessDeniedException When the acting user cannot access the bound folder.
      */
-    public function assertObjectFolderAccessible(ObjectEntity $object): void
+    public function assertObjectFolderAccessible(ObjectEntity $object, ?IUser $currentUser=null): void
     {
         $folder = $object->getFolder();
         if ($folder === null || $folder === '' || is_numeric($folder) === false) {
             return;
         }
 
+        // Per-request memoization. Resolve the acting user the same way
+        // `FolderManagementHandler::assertFolderIsAccessible` will (explicit
+        // arg → session → null) so the cache key matches the access-grant
+        // tuple the inner method actually evaluates.
+        $actingUser = ($currentUser ?? $this->userSession->getUser());
+        $cacheKey   = (($actingUser?->getUID() ?? '__no_user__').':'.$folder);
+        if (isset($this->folderAccessRevalidationCache[$cacheKey]) === true) {
+            return;
+        }
+
         $this->folderManagementHandler->assertFolderIsAccessible(
             folderId: (string) $folder,
-            currentUser: null,
+            currentUser: $currentUser,
             objectEntity: $object
         );
+
+        // Only cache successes — failures re-throw and re-check on retry.
+        $this->folderAccessRevalidationCache[$cacheKey] = true;
     }//end assertObjectFolderAccessible()
 
     /**

lib/Service/ObjectService.php

@@ -79,6 +79,7 @@
 use React\Promise\Promise;
 use React\Promise\PromiseInterface;
 use React\Async;
+use OCP\IUser;
 use OCP\IUserSession;
 use OCP\IGroupManager;
 use OCP\IUserManager;
@@ -1071,6 +1072,12 @@ public function getLogs(string $uuid, array $filters=[], bool $_rbac=true, bool
      * @param bool                     $_multitenancy Whether to apply multitenancy filtering (default: true)
      * @param bool                     $silent        Whether to skip audit trail creation and events (default: false)
      * @param array|null               $uploadedFiles Uploaded files from multipart/form-data (optional)
+     * @param IUser|null               $currentUser   Explicit acting user for `@self.folder` access checks
+     *                                                (forwarded to `ensureObjectFolder` → `assertObjectFolderAccessible`).
+     *                                                Defaults to null → `IUserSession::getUser()` resolution.
+     *                                                Non-HTTP callers (cron, import pipelines, event listeners)
+     *                                                MUST pass an explicit user to avoid the
+     *                                                default-deny fall-through on every folder-bound save.
      *
      * @return ObjectEntity The saved and rendered object
      *
@@ -1089,7 +1096,8 @@ public function saveObject(
         bool $_rbac=true,
         bool $_multitenancy=true,
         bool $silent=false,
-        ?array $uploadedFiles=null
+        ?array $uploadedFiles=null,
+        ?IUser $currentUser=null
     ): ObjectEntity {
         // Set register/schema context.
         $this->setContextFromParameters(
@@ -1171,7 +1179,7 @@ public function saveObject(
         $this->validateObjectIfRequired(object: $object);
 
         // Ensure folder exists for the object.
-        $folderId = $this->ensureObjectFolder(uuid: $uuid);
+        $folderId = $this->ensureObjectFolder(uuid: $uuid, currentUser: $currentUser);
 
         // Clear request-scoped caches before starting a new top-level save operation.
         // This ensures cascade operations benefit from caching while avoiding stale data.
@@ -1457,11 +1465,16 @@ private function normalizeDateValues(array $object): array
     /**
      * Ensure object folder exists, create if needed.
      *
-     * @param string|null $uuid Object UUID
+     * @param string|null $uuid        Object UUID
+     * @param IUser|null  $currentUser Explicit acting user forwarded to the
+     *                                 defense-in-depth re-validation; falls
+     *                                 back to `IUserSession::getUser()`.
+     *                                 Non-HTTP callers (cron, import
+     *                                 pipelines) MUST pass an explicit user.
      *
      * @return int|null Folder ID if created/exists, null otherwise
      */
-    private function ensureObjectFolder(?string $uuid): ?int
+    private function ensureObjectFolder(?string $uuid, ?IUser $currentUser=null): ?int
     {
         // Handle folder creation for existing objects or new objects with UUIDs.
         $folderId = null;
@@ -1504,8 +1517,11 @@ private function ensureObjectFolder(?string $uuid): ?int
                     // `FolderAccessDeniedException` → HTTP 403 at the
                     // controller layer when the acting user cannot access
                     // the bound folder.
-                    $this->fileService->assertObjectFolderAccessible($existingObject);
-                }
+                    $this->fileService->assertObjectFolderAccessible(
+                        object: $existingObject,
+                        currentUser: $currentUser
+                    );
+                }//end if
             } catch (\OCA\OpenRegister\Exception\FolderAccessDeniedException $e) {
                 // Propagate folder-access denials up to the controller.
                 throw $e;