diff --git a/src/application/services/drive_management_service.rs b/src/application/services/drive_management_service.rs index 5d472d04..b2b34537 100644 --- a/src/application/services/drive_management_service.rs +++ b/src/application/services/drive_management_service.rs @@ -249,6 +249,20 @@ impl DriveManagementService { .set_role(caller_id, subject, role, resource, expires_at) .await?; + // Drop the entire drive-role cache for this drive so the new + // grant is visible on the very next `check` — without this, a + // caller that gets Owner via `POST /api/drives/{id}/members` + // then immediately acts on drive content (WebDAV cross-drive + // MOVE, admin-driven cleanup, drive management) hits the + // stale "no role for this subject on this drive" entry + // seeded at some earlier `check`. TTL rescues eventually, + // but the storage_cleanup_check.sh drain pattern hits this + // race within a single test-second and fails on `authz.denied` + // for admin's cascade to files inside. + self.authz + .invalidate_drive_role_cache_for_drive(drive_id) + .await; + // D6 §11: canonical `drive.member_added` audit event covers // every successful membership write (add + role-refresh, since // the underlying `set_role` is UPSERT — distinguishing the two @@ -312,6 +326,15 @@ impl DriveManagementService { self.authz.clear_role(subject, resource).await?; + // Mirror of `set_member_role`'s cache invalidation: after + // clearing a role we MUST drop the `drive_role_cache` entries + // targeting this drive, otherwise the just-removed subject's + // former role stays visible until TTL expires. Same anti-drift + // reason as the sibling add path above. + self.authz + .invalidate_drive_role_cache_for_drive(drive_id) + .await; + // D6 §11: canonical `drive.member_removed` audit event covers // every successful removal (owner-driven or admin bypass). // `via_admin` replaces the separate diff --git a/src/application/services/file_management_service.rs b/src/application/services/file_management_service.rs index d617357f..1807852c 100644 --- a/src/application/services/file_management_service.rs +++ b/src/application/services/file_management_service.rs @@ -380,6 +380,20 @@ impl FileManagementUseCase for FileManagementService { let dto = self.move_file(file_id, folder_id, caller_id).await?; + // Cross-drive move invalidates the file's `owner_cache` entry + // in the authz engine — the cache assumed drive_id stability + // that no longer holds. Without this call the drive-role + // precheck at `check_inner` steers to the (stale) source + // drive and legitimate Delete/Update by a destination-drive + // role-holder returns 404 for up to the cache TTL. + if cross_drive.is_some() + && let Ok(file_uuid) = Uuid::parse_str(file_id) + { + self.authz + .invalidate_owner_cache_for_resource(Resource::File(file_uuid)) + .await; + } + // D6 §11 audit: emit only when the move actually crossed a // drive boundary. Same-drive moves are too noisy to audit at // info — operators care about the cross-drive case for diff --git a/src/application/services/folder_service.rs b/src/application/services/folder_service.rs index 4503cf8f..a1add88f 100644 --- a/src/application/services/folder_service.rs +++ b/src/application/services/folder_service.rs @@ -641,6 +641,17 @@ impl FolderUseCase for FolderService { ) })?; + // Cross-drive move flushes the authz engine's `owner_cache` + // — every descendant's cached `Resource → drive_id` mapping + // just got stale via the cascade trigger, and we don't (yet) + // walk the subtree to invalidate individually. Small perf + // cost (single JOIN per resource touched over the next + // minute) versus a stale-authz bug where destination-drive + // Owner cascades don't apply to moved content. + if cross_drive.is_some() { + self.authz.invalidate_owner_cache_all().await; + } + // D6 audit: only emit when the move crossed a drive boundary. // The cascade trigger has already propagated drive_id to the // subtree at this point (see migration diff --git a/src/infrastructure/services/pg_acl_engine.rs b/src/infrastructure/services/pg_acl_engine.rs index ebee39bf..6d346ba4 100644 --- a/src/infrastructure/services/pg_acl_engine.rs +++ b/src/infrastructure/services/pg_acl_engine.rs @@ -283,6 +283,62 @@ impl PgAclEngine { } } + /// Drop the `owner_cache` entry for `resource`. Called after any + /// operation that changes which drive a file/folder belongs to — + /// the pre-D6 comment on `owner_cache` ("a resource's owner is + /// immutable") stopped being true when cross-drive MOVE landed. + /// + /// Without this call, admin (or any other role holder) on the + /// destination drive gets `authz.denied` when acting on the moved + /// resource: the cached (stale) `Resource → src_drive_id` lookup + /// steers the drive-role precheck at `check_inner` toward the + /// SOURCE drive where the caller has no role, and the fallback + /// per-resource cascade doesn't cover drive-level grants. TTL + /// backstops eventually (5 min), but every write path that MOVEs + /// content across drives MUST invalidate here so authz observes + /// the new drive on the next check. + pub async fn invalidate_owner_cache_for_resource(&self, resource: Resource) { + self.owner_cache.invalidate(&resource).await; + } + + /// Bulk cousin of [`Self::invalidate_owner_cache_for_resource`] — + /// clears the entire `owner_cache`. Called by folder cross-drive + /// MOVE where the moved subtree's descendants each carry their + /// own stale entry, and we don't (yet) walk the subtree to + /// invalidate them individually. The cache repopulates lazily on + /// next access; the overhead is a single JOIN per file/folder + /// touched in the following minute or two, versus a stale-authz + /// bug that returned `NotFound` for legitimate Delete. + pub async fn invalidate_owner_cache_all(&self) { + self.owner_cache.invalidate_all(); + } + + /// Sibling of [`Self::invalidate_drive_role_cache_for_drive`] keyed by + /// subject rather than drive. Used by the user-deleted lifecycle hook + /// to reap every cached "user X → drive Y = role R" entry after the + /// user row (and its DB-cascade-cleared role_grants) is gone. Without + /// this call the entry lingers until TTL; in practice auth rejection + /// on the deleted user's tokens fires first, but leaving stale + /// authorisation rows in the cache is poor hygiene and would surface + /// as an issue if a session survived (e.g. long-lived Basic Auth via + /// app password) or if a same-uuid user were ever recreated. + pub async fn invalidate_drive_role_cache_for_subject(&self, subject: Subject) { + if let Err(err) = self + .drive_role_cache + .invalidate_entries_if(move |key, _v| key.0 == subject) + { + tracing::error!( + target: "oxicloud::authz", + event = "authz.cache_invalidation_failed", + cache = "drive_role_cache", + subject = ?subject, + error = %err, + "drive_role_cache cannot be bulk-invalidated by subject — \ + cache builder is missing support_invalidation_closures()", + ); + } + } + /// Expand a user subject into the set of subject UUIDs that should match /// in `access_grants`: the user's own UUID, every group the user is /// transitively a member of, and (for internal users only) the implicit @@ -2099,8 +2155,19 @@ impl UserLifecycleHook for AuthzCacheLifecycleHook { _tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, ) -> Result<(), DomainError> { // No DB writes here — just memory invalidation. `_tx` is - // intentionally ignored. + // intentionally ignored. The DB cascade + // (`trg_cleanup_role_grants_user`) already dropped every + // role_grants row for this subject; we mirror that cleanup on + // both authz caches: + // 1. `user_groups_cache` — recomputed group expansion. + // 2. `drive_role_cache` — cached "user X → drive Y = role R" + // entries seeded by prior authz checks. Without this + // the deleted user's role stays visible in-process for + // up to the cache TTL (~30 s). self.engine.invalidate_user_groups_cache(user.id()).await; + self.engine + .invalidate_drive_role_cache_for_subject(Subject::User(user.id())) + .await; Ok(()) } } diff --git a/tests/api/drive_quota.hurl b/tests/api/drive_quota.hurl index 018ad6ce..b624d748 100644 --- a/tests/api/drive_quota.hurl +++ b/tests/api/drive_quota.hurl @@ -348,8 +348,9 @@ HTTP 507 # 11b — COPY same file into tight drive. Same refusal shape as MOVE # — COPY creates a NEW file row that counts against # `drives.used_bytes` even when blob dedup means no new bytes -# hit the store. -POST {{base_url}}/api/files/copy +# hit the store. Batch endpoint lives under `/api/batch/…`, +# not `/api/files/…`. +POST {{base_url}}/api/batch/files/copy Authorization: Bearer {{owner_token}} Content-Type: application/json { @@ -357,13 +358,15 @@ Content-Type: application/json "target_folder_id": "{{tight_root_id}}" } -# Batch endpoint returns 206 Partial when at least one item fails -# with a per-item error. Per-item quota rejection is the wire -# shape here — assert the 507 landed in the per-item results, -# not on the envelope. -HTTP 206 +# Batch envelope: 200 all-ok, 206 partial, 400 all-failed. Our +# single-item batch has one quota-refused item → 400 with the +# failure in the `.failed[]` array (per `BatchOperationResponse`). +HTTP 400 [Asserts] -jsonpath "$.results[?(@.file_id=='{{big_file_id}}')].error" exists +jsonpath "$.stats.failed" == 1 +jsonpath "$.stats.successful" == 0 +jsonpath "$.failed[0].id" == "{{big_file_id}}" +jsonpath "$.failed[0].error" exists # 11c — Sanity: the file MOVE isn't universally broken. Targeting diff --git a/tests/api/storage_cleanup_check.sh b/tests/api/storage_cleanup_check.sh index 1943b6aa..5bed925f 100755 --- a/tests/api/storage_cleanup_check.sh +++ b/tests/api/storage_cleanup_check.sh @@ -150,9 +150,14 @@ while IFS= read -r drive_id; do while IFS= read -r file_id; do [[ -z "$file_id" ]] && continue - curl -sf -X DELETE -H "$AUTH" "$base_url/api/files/$file_id" >/dev/null + HTTP_STATUS=$(curl -s -H "$AUTH" -o /tmp/del.json -w '%{http_code}' \ + -X DELETE "$base_url/api/files/$file_id") + if [[ "$HTTP_STATUS" != "204" ]]; then + log "FILE DELETE FAILED: file=$file_id drive=$drive_id ($DRIVE_NAME) status=$HTTP_STATUS body=$(cat /tmp/del.json)" + fi done < <(echo "$CONTENTS" | jq -r '.items[] | select(.resource_type == "file") | .resource.id') + # Empty the drive's per-drive trash so D3b's "drive must be empty" # guard passes on the delete. `/api/trash/drive/{id}` is the # Owner-only per-drive empty (admin is Owner now via the grant diff --git a/tests/api/webdav_permissions.hurl b/tests/api/webdav_permissions.hurl index 8b930239..48707af3 100644 --- a/tests/api/webdav_permissions.hurl +++ b/tests/api/webdav_permissions.hurl @@ -91,6 +91,7 @@ Content-Type: application/json HTTP 201 [Captures] shared_drive_id: jsonpath "$.id" +shared_root_id: jsonpath "$.root_folder_id" # ─────────────────────────────────────────────────────────────