From 4eb970777495052f40297f1594659abbf60152b8 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 23 Jun 2026 00:44:24 +0200 Subject: [PATCH] feat(drive): check user grants on drive use drive's grant whant handling a file or folder --- .../services/drive_management_service.rs | 178 ++++++++++++++ .../pg/file_blob_read_repository.rs | 14 ++ .../repositories/pg/folder_db_repository.rs | 13 ++ src/infrastructure/services/pg_acl_engine.rs | 218 +++++++++++++----- 4 files changed, 365 insertions(+), 58 deletions(-) create mode 100644 src/application/services/drive_management_service.rs diff --git a/src/application/services/drive_management_service.rs b/src/application/services/drive_management_service.rs new file mode 100644 index 00000000..48e5a0f4 --- /dev/null +++ b/src/application/services/drive_management_service.rs @@ -0,0 +1,178 @@ +//! D2 — drive membership management service. +//! +//! Translates `POST/PATCH/DELETE /api/drives/{id}/members` into role-grant +//! writes on `resource_type='drive'`, layering D2's business rules on top: +//! +//! - **Personal-drive guard** (§2): drives with `kind='personal'` are +//! single-user single-owner by invariant; any member mutation is refused +//! at the service edge with `403`. Listing a personal drive's members is +//! still allowed (returns exactly the owner row) so the UI can render the +//! same shape across drive kinds without per-kind branching. +//! +//! - **Last-owner protection** (shared drives): removing or demoting the +//! final `Role::Owner` would leave the drive unmanageable. Refused at the +//! service edge — the caller has to promote someone else to Owner first, +//! or delete the drive. +//! +//! - **Authorization**: caller must hold `Permission::Manage` on the drive +//! to mutate; `Permission::Read` to list. `AuthorizationEngine::require` +//! emits the canonical `authz.denied` audit line on rejection. + +use std::sync::Arc; + +use uuid::Uuid; + +use crate::application::ports::authorization_ports::AuthorizationEngine; +use crate::common::errors::DomainError; +use crate::domain::repositories::drive_repository::DriveRepository; +use crate::domain::services::authorization::{Grant, Permission, Resource, Role, Subject}; +use crate::infrastructure::repositories::pg::DrivePgRepository; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; + +pub struct DriveManagementService { + drive_repo: Arc, + authz: Arc, +} + +impl DriveManagementService { + pub fn new(drive_repo: Arc, authz: Arc) -> Self { + Self { drive_repo, authz } + } + + /// `GET /api/drives/{id}/members` — every role grant on the drive. + pub async fn list_members( + &self, + caller_id: Uuid, + drive_id: Uuid, + ) -> Result, DomainError> { + let resource = Resource::Drive(drive_id); + self.authz + .require(Subject::User(caller_id), Permission::Read, resource) + .await?; + self.authz.list_grants_on_resource(resource).await + } + + /// `POST /api/drives/{id}/members` (create) or + /// `PATCH /api/drives/{id}/members/{subject_id}` (role change). + /// + /// `set_role` is idempotent — `(subject, resource)` is unique — so the + /// two HTTP shapes share one service method. Returns the resulting grant. + pub async fn set_member_role( + &self, + caller_id: Uuid, + drive_id: Uuid, + subject: Subject, + role: Role, + expires_at: Option>, + ) -> Result { + let resource = Resource::Drive(drive_id); + self.authz + .require(Subject::User(caller_id), Permission::Manage, resource) + .await?; + + self.refuse_if_personal(drive_id, "set_member_role").await?; + + // Demotion of the last owner = last-owner protection trips. A fresh + // owner-role write or any non-owner subject is fine; only the case + // "this subject is currently the only owner AND the new role is not + // owner" is refused. + if !matches!(role, Role::Owner) { + self.refuse_if_last_owner_change(drive_id, subject, caller_id) + .await?; + } + + self.authz + .set_role(caller_id, subject, role, resource, expires_at) + .await + } + + /// `DELETE /api/drives/{id}/members/{subject_id}`. Idempotent — removing + /// a subject with no current grant succeeds (matches `clear_role`). + pub async fn remove_member( + &self, + caller_id: Uuid, + drive_id: Uuid, + subject: Subject, + ) -> Result<(), DomainError> { + let resource = Resource::Drive(drive_id); + self.authz + .require(Subject::User(caller_id), Permission::Manage, resource) + .await?; + + self.refuse_if_personal(drive_id, "remove_member").await?; + + self.refuse_if_last_owner_change(drive_id, subject, caller_id) + .await?; + + self.authz.clear_role(subject, resource).await + } + + // ── Business rules ────────────────────────────────────────────────────── + + /// Personal drives are single-user single-owner; any member mutation is + /// a category error (§2). Returns `Forbidden` with an audit line. + async fn refuse_if_personal(&self, drive_id: Uuid, op: &str) -> Result<(), DomainError> { + let drive = self.drive_repo.get_by_id(drive_id).await.map_err(|e| { + DomainError::internal_error("Drive", format!("Failed to fetch drive: {e:?}")) + })?; + if drive.drive.is_personal() { + tracing::info!( + target: "audit", + event = "drive_membership.rejected", + reason = "personal_drive_immutable", + operation = %op, + drive_id = %drive_id, + "👮🏻‍♂️ refused {op} on personal drive {drive_id}", + ); + return Err(DomainError::operation_not_supported( + "Drive", + "Personal drives have a fixed single-owner membership and cannot be modified.", + )); + } + Ok(()) + } + + /// Refuse the change if `subject` is currently the sole `Owner` on the + /// drive and the operation would remove or demote them. A shared drive + /// must always have at least one Owner — otherwise it becomes orphaned + /// (no one can ever grant permissions again). + async fn refuse_if_last_owner_change( + &self, + drive_id: Uuid, + subject: Subject, + caller_id: Uuid, + ) -> Result<(), DomainError> { + let resource = Resource::Drive(drive_id); + let grants = self.authz.list_grants_on_resource(resource).await?; + + // `subject` must currently BE an owner — otherwise no demotion risk. + let subject_is_owner = grants + .iter() + .any(|g| g.subject == subject && matches!(g.role, Role::Owner)); + if !subject_is_owner { + return Ok(()); + } + + let owner_count = grants + .iter() + .filter(|g| matches!(g.role, Role::Owner)) + .count(); + if owner_count <= 1 { + tracing::info!( + target: "audit", + event = "drive_membership.rejected", + reason = "last_owner", + drive_id = %drive_id, + caller_id = %caller_id, + subject_type = subject.type_str(), + subject_id = %subject.id(), + "👮🏻‍♂️ refused last-owner removal on drive {drive_id}", + ); + return Err(DomainError::validation_error( + "A shared drive must keep at least one Owner — promote another \ + member to Owner first, or delete the drive.", + )); + } + Ok(()) + } +} diff --git a/src/infrastructure/repositories/pg/file_blob_read_repository.rs b/src/infrastructure/repositories/pg/file_blob_read_repository.rs index cc4a7428..d7e5f94c 100644 --- a/src/infrastructure/repositories/pg/file_blob_read_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_read_repository.rs @@ -330,6 +330,20 @@ impl FileBlobReadRepository { .ok_or_else(|| DomainError::not_found("File", file_id)) } + /// Returns `drive_id` for a given file. Drives the permission-floor + /// short-circuit in `PgAclEngine::check_inner` — drive membership is + /// the baseline floor per `drive.md §5`. + pub async fn get_file_drive_id(&self, file_id: &str) -> Result { + sqlx::query_scalar::<_, uuid::Uuid>( + "SELECT drive_id FROM storage.files WHERE id = $1::uuid", + ) + .bind(file_id) + .fetch_optional(self.pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("FileBlobRead", format!("drive_id lookup: {e}")))? + .ok_or_else(|| DomainError::not_found("File", file_id)) + } + /// Creates a stub instance for testing — never hits PG. #[cfg(test)] pub fn new_stub() -> Self { diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index d968f05b..4131ccef 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -1335,6 +1335,19 @@ impl FolderDbRepository { .ok_or_else(|| DomainError::not_found("Folder", folder_id)) } + /// Returns `drive_id` for a given folder. Drives the new permission-floor + /// short-circuit in `PgAclEngine::check_inner` (a caller with any role + /// on the folder's drive automatically passes the check — drive + /// membership is the baseline floor per `drive.md §5`). + pub async fn get_folder_drive_id(&self, folder_id: &str) -> Result { + sqlx::query_scalar::<_, Uuid>("SELECT drive_id FROM storage.folders WHERE id = $1::uuid") + .bind(folder_id) + .fetch_optional(self.pool()) + .await + .map_err(|e| DomainError::internal_error("FolderDb", format!("drive_id lookup: {e}")))? + .ok_or_else(|| DomainError::not_found("Folder", folder_id)) + } + /// Verifies that `folder_id` is owned by `owner_id`. /// /// Returns `DomainError::not_found(...)` for both "folder missing" and diff --git a/src/infrastructure/services/pg_acl_engine.rs b/src/infrastructure/services/pg_acl_engine.rs index 4d125466..633c30e1 100644 --- a/src/infrastructure/services/pg_acl_engine.rs +++ b/src/infrastructure/services/pg_acl_engine.rs @@ -81,6 +81,16 @@ const OWNER_CACHE_CAPACITY: u64 = 100_000; /// (see `owner_cache` field doc), hence a generous TTL for a high hit rate. const OWNER_CACHE_TTL: Duration = Duration::from_secs(300); +/// `drive_role_cache` bound: entries are `((Subject, Uuid), Option)` — +/// a few tens of bytes each. 100k accommodates ~5–10 drives per active user +/// with comfortable headroom. +const DRIVE_ROLE_CACHE_CAPACITY: u64 = 100_000; +/// `drive_role_cache` TTL. Membership mutations on a drive explicitly invalidate +/// affected entries (see `invalidate_drive_role_cache_for_drive`), so the TTL +/// is mainly a safety net for paths that skip explicit invalidation. Short +/// enough that any oversight self-heals in <1 minute. +const DRIVE_ROLE_CACHE_TTL: Duration = Duration::from_secs(30); + pub struct PgAclEngine { pool: Arc, folder_repo: Arc, @@ -98,7 +108,30 @@ pub struct PgAclEngine { /// access (a different caller's `owner == uid` test fails against the cached /// *real* owner), and a hard-deleted resource that briefly short-circuits as /// owned simply fails later at execution with NotFound. + /// + /// Post-D0 this caches `resource → drive_id` instead (the legacy `owner_*` + /// rename was avoided to minimise field-name churn in the dual-write + /// window). The drive precheck queries this for every File / Folder check. owner_cache: Cache, + + /// Memoise `(subject, drive_id) → Option`, the strongest role the + /// subject holds on a drive (direct + group-mediated, collapsed). Drives + /// the permission-floor precheck in `check_inner` — on a cache hit the + /// entire drive-grant lookup resolves in-memory, returning the steady + /// state to "0 SQL queries per authz check for callers touching their + /// own drive content" (matching the legacy owner short-circuit). + /// + /// **Invalidation**: explicit on every membership mutation + /// (`set_role` / `clear_role` with `Resource::Drive`) — drops every + /// entry whose drive_id matches. Group-membership changes are caught by + /// the short TTL rather than a deep invalidation tree. + /// + /// **Safety**: cache only widens authorization between mutations; the + /// 30 s TTL bounds how long a revoked grant can still appear effective + /// for a non-explicit invalidation path. Explicit paths (D2's + /// `DriveManagementService`, the grant handler's revoke path) hit the + /// invalidator inline. + drive_role_cache: Cache<(Subject, Uuid), Option>, } impl PgAclEngine { @@ -121,6 +154,10 @@ impl PgAclEngine { .max_capacity(OWNER_CACHE_CAPACITY) .time_to_live(OWNER_CACHE_TTL) .build(), + drive_role_cache: Cache::builder() + .max_capacity(DRIVE_ROLE_CACHE_CAPACITY) + .time_to_live(DRIVE_ROLE_CACHE_TTL) + .build(), } } @@ -176,6 +213,10 @@ impl PgAclEngine { .max_capacity(1) .time_to_live(Duration::from_secs(1)) .build(), + drive_role_cache: Cache::builder() + .max_capacity(1) + .time_to_live(Duration::from_secs(1)) + .build(), } } @@ -190,6 +231,23 @@ impl PgAclEngine { self.user_groups_cache.invalidate(&user_id).await; } + /// Drop every `drive_role_cache` entry whose key targets `drive_id`. + /// Called after every membership mutation on the drive (set_role / + /// clear_role / revoke when the resource is a Drive) so the next authz + /// check sees the fresh role rather than a TTL-bounded stale view. + /// + /// Uses moka's predicate-based eviction — entries are marked for + /// removal asynchronously by the maintenance task; subsequent `get` + /// calls observe the eviction immediately. + pub async fn invalidate_drive_role_cache_for_drive(&self, drive_id: Uuid) { + // `invalidate_entries_if` rejects predicates returning errors — + // simple Fn(K, V) -> bool. We capture `drive_id` by value (Copy) + // and match against the second tuple component. + let _ = self + .drive_role_cache + .invalidate_entries_if(move |key, _v| key.1 == drive_id); + } + /// 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 @@ -296,34 +354,39 @@ impl PgAclEngine { self.subject_match_set(subject, &counters).await } - /// Owner lookup with memoisation. Hits the DB only on a cache miss; the - /// result is cached because a resource's owner never changes. `NotFound` - /// (a hard-deleted / nonexistent resource) is propagated, not cached. - async fn owner_of_cached( + /// Drive lookup with memoisation. Hits the DB only on a cache miss; the + /// result is cached because a resource's `drive_id` is immutable in the + /// current model (cross-drive moves arrive in D6 and will need cache + /// invalidation at that point). `NotFound` is propagated, not cached. + /// + /// **Why this replaces the legacy `owner_of_cached`**: the old short- + /// circuit was `caller_id == resource.user_id`. Post-D0 ownership is + /// modelled through `role_grants` on the resource's drive — a caller + /// with any qualifying role on the drive automatically satisfies the + /// check (per `drive.md §5`, drive role is the baseline floor for + /// every resource in the drive). The lookup shape is identical + /// (Resource → Uuid), so we keep the same cache infrastructure. + async fn drive_of_cached( &self, resource: Resource, counters: &QueryCounters, ) -> Result { - if let Some(owner) = self.owner_cache.get(&resource).await { - return Ok(owner); + if let Some(drive_id) = self.owner_cache.get(&resource).await { + return Ok(drive_id); } counters.sql_queries.fetch_add(1, Ordering::Relaxed); - let owner = self.owner_of(resource).await?; - self.owner_cache.insert(resource, owner).await; - Ok(owner) + let drive_id = self.drive_of(resource).await?; + self.owner_cache.insert(resource, drive_id).await; + Ok(drive_id) } - /// Returns the owner UUID for any resource type. - async fn owner_of(&self, resource: Resource) -> Result { + /// Returns the `drive_id` for a File / Folder. Drives don't have a parent + /// drive — this returns `NotFound` for `Resource::Drive` and the caller + /// must not invoke it on Drive resources. + async fn drive_of(&self, resource: Resource) -> Result { match resource { - Resource::Folder(id) => self.folder_repo.get_folder_user_id(&id.to_string()).await, - Resource::File(id) => self.file_repo.get_file_user_id(&id.to_string()).await, - // Drive owner resolution wires up in D0-6 once `DriveRepository` - // lands (D0-5). Drive entity carries `default_for_user` for - // `kind='personal'`; shared drives resolve through role_grants - // (Owner role). Returning NotFound here means a permission - // check that reached owner_of on a Drive falls through to the - // grant-lookup path — safe default during D0-1. + Resource::Folder(id) => self.folder_repo.get_folder_drive_id(&id.to_string()).await, + Resource::File(id) => self.file_repo.get_file_drive_id(&id.to_string()).await, Resource::Drive(_) => Err(DomainError::not_found("Drive", resource.id().to_string())), } } @@ -450,41 +513,51 @@ impl PgAclEngine { Ok(exists.is_some()) } - /// Direct grant lookup for a drive — no ltree cascade (drives have - /// no ancestors). Mirrors the cascade helpers above but with a - /// straight `resource_type='drive' AND resource_id=$4` filter. - async fn drive_grant_exists( + /// Cached resolution of `(subject, drive_id) → Option` — the + /// strongest role the subject holds on the drive (direct + transitive + /// group grants collapsed). `None` means no qualifying grant; cached + /// negatively to avoid re-querying on repeated denials within the TTL. + /// + /// This is the cache-aware backbone of the permission-floor precheck. + /// `Role::expand()` translates the returned role into its permission + /// bundle; callers ask `role.expand().contains(&permission)` to decide. + async fn caller_role_on_drive_cached( &self, - subject_types: &[&str], - subject_ids: &[Uuid], - permission: Permission, + subject: Subject, drive_id: Uuid, counters: &QueryCounters, - ) -> Result { + ) -> Result, DomainError> { + if let Some(cached) = self.drive_role_cache.get(&(subject, drive_id)).await { + return Ok(cached); + } + // Expand subject for the role lookup. `subject_match_set` is itself + // cached (30 s TTL); the steady state on `drive_role_cache` miss is + // one in-memory expansion + one indexed SQL query. + let (subject_types, subject_ids) = self.subject_match_set(subject, counters).await?; counters.sql_queries.fetch_add(1, Ordering::Relaxed); - let roles = Self::roles_implying_strings(permission); - let exists: Option = sqlx::query_scalar( + let role_str: Option = sqlx::query_scalar( r#" - SELECT 1 + SELECT MIN(g.role)::text FROM storage.role_grants g WHERE g.subject_type = ANY($1) AND g.subject_id = ANY($2) - AND g.role = ANY($3::storage.grant_role[]) AND g.resource_type = 'drive' - AND g.resource_id = $4 + AND g.resource_id = $3 AND (g.expires_at IS NULL OR g.expires_at > NOW()) - LIMIT 1 "#, ) .bind(subject_types) .bind(subject_ids) - .bind(&roles) .bind(drive_id) - .fetch_optional(self.pool.as_ref()) + .fetch_one(self.pool.as_ref()) .await - .map_err(|e| DomainError::internal_error("PgAcl", format!("drive grant: {e}")))?; + .map_err(|e| DomainError::internal_error("PgAcl", format!("drive role lookup: {e}")))?; - Ok(exists.is_some()) + let role = role_str.as_deref().and_then(Role::parse); + self.drive_role_cache + .insert((subject, drive_id), role) + .await; + Ok(role) } /// Look up a single role grant by id, returning the actors a revoke / @@ -584,34 +657,42 @@ impl PgAclEngine { resource: Resource, counters: &QueryCounters, ) -> Result { - // Owner short-circuit (only for User subjects — groups/tokens/external - // are never owners of resources). - // Owner short-circuit applies to Folder/File only — they carry a - // single-owner `user_id` column in their respective tables. Drives - // model ownership through the `Owner` role in `role_grants`, so - // there's no analogous fast path: the grant lookup below resolves - // a drive owner via the same query that resolves any drive role. - if let (Subject::User(uid), Resource::Folder(_) | Resource::File(_)) = (subject, resource) { - match self.owner_of_cached(resource, counters).await { - Ok(owner) if owner == uid => return Ok(true), - Ok(_) => { /* not owner — fall through to grants */ } + // Drive-membership precheck for File/Folder. A role on the resource's + // drive is the baseline floor (`drive.md §5`): the caller passes any + // permission check the role bundle covers. Replaces the legacy + // `caller_id == resource.user_id` owner short-circuit — for a user's + // own personal drive the lifecycle hook seeds an Owner row, so the + // common case (touching your own files) is **0 SQL queries** after + // the first hit on `drive_role_cache` (cached `(subject, drive) → + // Role`, 30 s TTL with explicit invalidation on membership writes). + if matches!(resource, Resource::Folder(_) | Resource::File(_)) { + let drive_id = match self.drive_of_cached(resource, counters).await { + Ok(d) => d, Err(e) if e.kind == crate::common::errors::ErrorKind::NotFound => { - // Resource doesn't exist — no permission. Return false - // rather than propagating NotFound; the caller (`require`) - // converts a false back to NotFound on its own. + // Resource doesn't exist — no permission. `require` + // converts the `false` back to NotFound at its layer. return Ok(false); } Err(e) => return Err(e), + }; + if let Some(role) = self + .caller_role_on_drive_cached(subject, drive_id, counters) + .await? + && role.expand().contains(&permission) + { + return Ok(true); } + // Drive precheck didn't match — fall through to per-resource + // grant + folder-ancestor cascade (existing behaviour, untouched). } - // Expand the subject so group-mediated grants apply when the caller - // is a User. See `subject_match_set` for the shared shape used by - // both the cascade check and the "shared with me" listing queries. - let (subject_types, subject_ids) = self.subject_match_set(subject, counters).await?; - match resource { + // File/Folder dispatch falls through to the cascade query — + // expand the subject set lazily here (it's cached) so the + // Drive branch below never pays for an expansion it doesn't need. Resource::Folder(id) => { + let (subject_types, subject_ids) = + self.subject_match_set(subject, counters).await?; self.folder_cascade_grant_exists( &subject_types, &subject_ids, @@ -622,6 +703,8 @@ impl PgAclEngine { .await } Resource::File(id) => { + let (subject_types, subject_ids) = + self.subject_match_set(subject, counters).await?; self.file_cascade_grant_exists( &subject_types, &subject_ids, @@ -632,8 +715,13 @@ impl PgAclEngine { .await } Resource::Drive(id) => { - self.drive_grant_exists(&subject_types, &subject_ids, permission, id, counters) - .await + // Same cache-aware path the precheck uses — keeps the + // single-source-of-truth for drive role resolution and + // benefits identically from `drive_role_cache`. + Ok(self + .caller_role_on_drive_cached(subject, id, counters) + .await? + .is_some_and(|r| r.expand().contains(&permission))) } } } @@ -1873,6 +1961,13 @@ impl AuthorizationEngine for PgAclEngine { .await .map_err(|e| DomainError::internal_error("PgAcl", format!("set_role: {e}")))?; + // Membership write on a drive — drop every cached + // `(subject, drive_id)` entry pointing at this drive so the next + // authz check resolves against the fresh role. + if let Resource::Drive(drive_id) = resource { + self.invalidate_drive_role_cache_for_drive(drive_id).await; + } + Self::row_to_grant(row) } @@ -1890,6 +1985,13 @@ impl AuthorizationEngine for PgAclEngine { .await .map_err(|e| DomainError::internal_error("PgAcl", format!("clear_role: {e}")))?; + // Symmetric with `set_role` — drop cached drive-role entries on + // membership revocation so a viewer who just got removed doesn't + // keep passing the precheck for up to 30 s. + if let Resource::Drive(drive_id) = resource { + self.invalidate_drive_role_cache_for_drive(drive_id).await; + } + Ok(()) } }