From 2cda8e7e224f6337e0e25b2fb4b49074a040b68f Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 4 Jul 2026 19:36:17 +0200 Subject: [PATCH] security(favorite,recent): ensure read permission --- src/application/services/favorites_service.rs | 57 ++++++++----- src/application/services/recent_service.rs | 37 ++++++--- src/common/di.rs | 80 +++++++++++-------- src/domain/services/authorization.rs | 28 +++++++ 4 files changed, 138 insertions(+), 64 deletions(-) diff --git a/src/application/services/favorites_service.rs b/src/application/services/favorites_service.rs index e8970b42..ac372436 100644 --- a/src/application/services/favorites_service.rs +++ b/src/application/services/favorites_service.rs @@ -9,10 +9,12 @@ use crate::application::dtos::favorites_dto::{ BatchFavoritesResult, BatchFavoritesStats, FavoriteItemDto, FavoriteResourceRow, FavoritesCursor, }; +use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::favorites_ports::{FavoritesRepositoryPort, FavoritesUseCase}; -use crate::common::errors::{DomainError, ErrorKind, Result}; -use crate::domain::services::authorization::ResourceKind; +use crate::common::errors::Result; +use crate::domain::services::authorization::{Permission, Resource, ResourceKind, Subject}; use crate::infrastructure::repositories::pg::FavoritesPgRepository; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; /// Implementation of the FavoritesUseCase for managing user favorites. /// @@ -20,12 +22,22 @@ use crate::infrastructure::repositories::pg::FavoritesPgRepository; /// accessing the database directly, following hexagonal architecture. pub struct FavoritesService { repo: Arc, + /// ReBAC engine — enforces `Permission::Read` on the referenced + /// file/folder before enrolling it into a user's favorites. + /// Without this gate the write path is an information oracle: + /// listing endpoints JOIN back to `storage.files/folders` and + /// return name/mime/size/drive_id for any UUID the caller was + /// able to enroll. See `docs/plan/authz_audit/rest_storage.md`. + authorization: Arc, } impl FavoritesService { /// Create a new FavoritesService with the given repository port - pub fn new(repo: Arc) -> Self { - Self { repo } + pub fn new(repo: Arc, authorization: Arc) -> Self { + Self { + repo, + authorization, + } } /// Subset of `(item_id, item_type)` pairs the user has favorited — used to @@ -60,13 +72,15 @@ impl FavoritesUseCase for FavoritesService { item_type, item_id, user_id ); - if item_type != "file" && item_type != "folder" { - return Err(DomainError::new( - ErrorKind::InvalidInput, - "Favorites", - "Item type must be 'file' or 'folder'", - )); - } + // AuthZ pre-write: caller must have Read on the referenced + // resource. Denial routes through `require` → NotFound + // (anti-enum, matches the listing shape) + `authz.denied` + // audit line. Without this gate the write path was an + // information oracle over the whole tenant. + let resource = Resource::parse(item_type, item_id)?; + self.authorization + .require(Subject::User(user_id), Permission::Read, resource) + .await?; self.repo.add_favorite(user_id, item_id, item_type).await?; info!( @@ -125,18 +139,17 @@ impl FavoritesUseCase for FavoritesService { user_id ); - // Validate all item types + // AuthZ pre-write: caller must have Read on every referenced + // resource. Fail the whole batch on the first denial so the + // response shape doesn't tell an attacker which items were + // valid (partial success would leak the same oracle we + // closed on the single-item path). See + // `docs/plan/authz_audit/rest_storage.md`. for (item_id, item_type) in items { - if item_type != "file" && item_type != "folder" { - return Err(DomainError::new( - ErrorKind::InvalidInput, - "Favorites", - format!( - "Item type must be 'file' or 'folder' for item '{}'", - item_id - ), - )); - } + let resource = Resource::parse(item_type, item_id)?; + self.authorization + .require(Subject::User(user_id), Permission::Read, resource) + .await?; } let requested = items.len(); diff --git a/src/application/services/recent_service.rs b/src/application/services/recent_service.rs index 17f0bd2f..54482606 100644 --- a/src/application/services/recent_service.rs +++ b/src/application/services/recent_service.rs @@ -1,10 +1,12 @@ use crate::application::dtos::cursor::PageCursor; use crate::application::dtos::recent_dto::{RecentCursor, RecentItemDto, RecentResourceRow}; +use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::recent_ports::{RecentItemsRepositoryPort, RecentItemsUseCase}; use crate::application::ports::resource_access_hook::ResourceAccessHook; -use crate::common::errors::{DomainError, ErrorKind, Result}; -use crate::domain::services::authorization::ResourceKind; +use crate::common::errors::Result; +use crate::domain::services::authorization::{Permission, Resource, ResourceKind, Subject}; use crate::infrastructure::repositories::pg::RecentItemsPgRepository; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; use std::sync::{Arc, OnceLock}; use tracing::info; use uuid::Uuid; @@ -16,6 +18,13 @@ use uuid::Uuid; pub struct RecentService { repo: Arc, max_recent_items: i32, + /// ReBAC engine — enforces `Permission::Read` on the referenced + /// file/folder before enrolling it into a user's Recent list. + /// The listing side JOINs back to `storage.files/folders` and + /// returns name/mime/size/drive_id for any enrolled UUID, so + /// the write path is an information oracle without this gate. + /// See `docs/plan/authz_audit/rest_storage.md`. + authorization: Arc, /// Set after construction via [`Self::set_resource_access_hook`]. /// The hook is built FROM this service (it wraps an `Arc`), so /// we can't take it as a constructor arg without circular ownership; @@ -28,10 +37,15 @@ pub struct RecentService { impl RecentService { /// Create a new recent items service - pub fn new(repo: Arc, max_recent_items: i32) -> Self { + pub fn new( + repo: Arc, + authorization: Arc, + max_recent_items: i32, + ) -> Self { Self { repo, max_recent_items: max_recent_items.clamp(1, 100), + authorization, resource_access_hook: OnceLock::new(), } } @@ -87,13 +101,16 @@ impl RecentItemsUseCase for RecentService { item_type, item_id, user_id ); - if item_type != "file" && item_type != "folder" { - return Err(DomainError::new( - ErrorKind::InvalidInput, - "RecentItems", - "Item type must be 'file' or 'folder'", - )); - } + // AuthZ pre-write: caller must have Read on the referenced + // resource. Denial routes through `require` → NotFound + // (anti-enum) + `authz.denied` audit line. Without this + // gate the write path was an information oracle over the + // whole tenant via the listing endpoint's JOIN back to + // storage.files/folders. + let resource = Resource::parse(item_type, item_id)?; + self.authorization + .require(Subject::User(user_id), Permission::Read, resource) + .await?; self.repo.upsert_access(user_id, item_id, item_type).await?; self.repo.prune(user_id, self.max_recent_items).await?; diff --git a/src/common/di.rs b/src/common/di.rs index a7547134..8ac3a379 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -889,23 +889,37 @@ impl AppServiceFactory { Some(service) } - /// Creates the favorites service (requires database) - pub fn create_favorites_service(&self, db_pool: &Arc) -> Arc { + /// Creates the favorites service (requires database + authz engine + /// for the Read gate on `add_to_favorites` — see the post-Drive + /// AuthZ audit). + pub fn create_favorites_service( + &self, + db_pool: &Arc, + authorization: &Arc, + ) -> Arc { let repo = Arc::new( crate::infrastructure::repositories::pg::FavoritesPgRepository::new(db_pool.clone()), ); - let service = Arc::new(FavoritesService::new(repo)); + let service = Arc::new(FavoritesService::new(repo, authorization.clone())); tracing::info!("Favorites service initialized"); service } - /// Creates the recent items service (requires database) - pub fn create_recent_service(&self, db_pool: &Arc) -> Arc { + /// Creates the recent items service (requires database + authz + /// engine for the Read gate on `record_item_access` — see the + /// post-Drive AuthZ audit). + pub fn create_recent_service( + &self, + db_pool: &Arc, + authorization: &Arc, + ) -> Arc { let repo = Arc::new( crate::infrastructure::repositories::pg::RecentItemsPgRepository::new(db_pool.clone()), ); let service = Arc::new(RecentService::new( - repo, 50, // Maximum recent items per user + repo, + authorization.clone(), + 50, // Maximum recent items per user )); tracing::info!("Recent items service initialized"); service @@ -1161,31 +1175,6 @@ impl AppServiceFactory { let pool = Arc::new(pools.primary); let maintenance_pool = Arc::new(pools.maintenance); - // Recent service + recording hook are built up-front so the - // hook can be threaded into `create_application_services` below. - // The file services hold the hook directly so every authorised - // `_with_perms` read/write fires into `auth.user_recent_files` - // without per-handler wiring. Reordering vs the legacy in-block - // creation (further down) is safe: `create_recent_service` only - // needs `pool`, which is already in scope. - // - // The back-edge `recent_service_eager.set_resource_access_hook` - // closes the loop so the clear/remove handlers can drop the - // hook's in-memory throttle entries — without it a freshly - // cleared Recent list refuses to re-record the same file for a - // full TTL window, surfacing as "I cleared, opened the file, - // and Recent is still empty" (caught by tests/api/recent.hurl - // step 8). - let recent_service_eager = self.create_recent_service(&pool); - let resource_access_hook: Arc< - dyn crate::application::ports::resource_access_hook::ResourceAccessHook, - > = Arc::new( - crate::infrastructure::services::recent_recording_hook::RecentRecordingHook::new( - recent_service_eager.clone(), - ), - ); - recent_service_eager.set_resource_access_hook(resource_access_hook.clone()); - // 1. Core services (PgPool needed for DedupService index) let core = self.create_core_services(&pool, &maintenance_pool).await?; @@ -1196,6 +1185,10 @@ impl AppServiceFactory { // because services hold an Arc for ReBAC checks. // SubjectGroupPgRepository is constructed here too so the engine can // expand a user's transitive group set on cache misses. + // + // Moved above the eager recent-service build so `create_recent_service` + // can receive an `Arc` — the Read gate on + // `record_item_access` (post-Drive AuthZ audit fix) needs it. let subject_group_repo = Arc::new( crate::infrastructure::repositories::pg::SubjectGroupPgRepository::new(pool.clone()), ); @@ -1206,6 +1199,29 @@ impl AppServiceFactory { subject_group_repo.clone(), ); + // Recent service + recording hook are built up-front so the + // hook can be threaded into `create_application_services` below. + // The file services hold the hook directly so every authorised + // `_with_perms` read/write fires into `auth.user_recent_files` + // without per-handler wiring. + // + // The back-edge `recent_service_eager.set_resource_access_hook` + // closes the loop so the clear/remove handlers can drop the + // hook's in-memory throttle entries — without it a freshly + // cleared Recent list refuses to re-record the same file for a + // full TTL window, surfacing as "I cleared, opened the file, + // and Recent is still empty" (caught by tests/api/recent.hurl + // step 8). + let recent_service_eager = self.create_recent_service(&pool, &authorization); + let resource_access_hook: Arc< + dyn crate::application::ports::resource_access_hook::ResourceAccessHook, + > = Arc::new( + crate::infrastructure::services::recent_recording_hook::RecentRecordingHook::new( + recent_service_eager.clone(), + ), + ); + recent_service_eager.set_resource_access_hook(resource_access_hook.clone()); + // Drive repository — needed both by the lifecycle hook (when auth // is enabled) and by `GET /api/drives` on the final `AppState`, // so declared at the outer scope. @@ -1279,7 +1295,7 @@ impl AppServiceFactory { > = None; { - let favs = self.create_favorites_service(&pool); + let favs = self.create_favorites_service(&pool, &authorization); favorites_service = Some(favs.clone()); apps.favorites_service = Some(favs); diff --git a/src/domain/services/authorization.rs b/src/domain/services/authorization.rs index c0502dda..0e9a6a3d 100644 --- a/src/domain/services/authorization.rs +++ b/src/domain/services/authorization.rs @@ -118,6 +118,34 @@ impl Resource { _ => None, } } + + /// Parse `(item_type, item_id)` from an API-facing pair of strings + /// (favorites, recent, batch endpoints all take this shape). + /// Combines UUID parse + type mapping so callers stay one-line and + /// error shapes are identical across surfaces. Returns + /// `DomainError::new(InvalidInput, …)` on malformed input; callers + /// that need the anti-enum 404 shape do that separately by feeding + /// the parsed `Resource` into `authz.require(...)`. + pub fn parse( + item_type: &str, + item_id: &str, + ) -> Result { + use crate::common::errors::{DomainError, ErrorKind}; + let uuid = Uuid::parse_str(item_id).map_err(|_| { + DomainError::new( + ErrorKind::InvalidInput, + "Resource", + format!("Invalid item UUID '{item_id}'"), + ) + })?; + Self::from_parts(item_type, uuid).ok_or_else(|| { + DomainError::new( + ErrorKind::InvalidInput, + "Resource", + format!("Unsupported item type '{item_type}'"), + ) + }) + } } impl fmt::Display for Resource {