fix(authz): invalidate role cache on change

This commit is contained in:
Edouard Vanbelle
2026-07-06 23:01:37 +02:00
parent 79f4792709
commit f7deb7aaf4
7 changed files with 134 additions and 10 deletions
@@ -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
@@ -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
@@ -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
+68 -1
View File
@@ -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(())
}
}
+11 -8
View File
@@ -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
+6 -1
View File
@@ -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
+1
View File
@@ -91,6 +91,7 @@ Content-Type: application/json
HTTP 201
[Captures]
shared_drive_id: jsonpath "$.id"
shared_root_id: jsonpath "$.root_folder_id"
# ─────────────────────────────────────────────────────────────