From f8b30e78a6be004d91721098a6eb9fd579cb9336 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 20 May 2026 12:48:06 +0200 Subject: [PATCH] refactor(create_folder): add an ownership check while creating a folder + refactor code --- src/application/ports/inbound.rs | 6 +- src/application/services/batch_operations.rs | 9 +-- .../services/file_management_service.rs | 57 +++++++------------ src/application/services/folder_service.rs | 31 +++++----- src/common/stubs.rs | 6 +- .../repositories/pg/folder_db_repository.rs | 16 ++++++ src/interfaces/api/handlers/folder_handler.rs | 20 +------ src/interfaces/api/handlers/webdav_handler.rs | 17 +++--- src/interfaces/nextcloud/webdav_handler.rs | 3 +- 9 files changed, 76 insertions(+), 89 deletions(-) diff --git a/src/application/ports/inbound.rs b/src/application/ports/inbound.rs index 3a7380ac..86816eeb 100644 --- a/src/application/ports/inbound.rs +++ b/src/application/ports/inbound.rs @@ -13,7 +13,11 @@ use crate::common::errors::DomainError; /// Primary port for folder operations pub trait FolderUseCase: Send + Sync + 'static { /// Creates a new folder - async fn create_folder(&self, dto: CreateFolderDto) -> Result; + async fn create_folder( + &self, + dto: CreateFolderDto, + caller_id: Uuid, + ) -> Result; /// Gets a folder by its ID async fn get_folder(&self, id: &str) -> Result; diff --git a/src/application/services/batch_operations.rs b/src/application/services/batch_operations.rs index 4d161272..7a12c4f2 100644 --- a/src/application/services/batch_operations.rs +++ b/src/application/services/batch_operations.rs @@ -975,18 +975,11 @@ impl BatchOperationService { let folder_service = self.folder_service.clone(); async move { - // If a parent is specified, verify the caller owns it - if let Some(ref pid) = parent_id - && let Err(e) = folder_service.get_folder_owned(pid, user_id).await - { - let id = format!("{}:{}", name, pid); - return (id, Err(e)); - } let dto = crate::application::dtos::folder_dto::CreateFolderDto { name: name.clone(), parent_id: parent_id.clone(), }; - let create_result = folder_service.create_folder(dto).await; + let create_result = folder_service.create_folder(dto, user_id).await; let id = format!("{}:{}", name, parent_id.unwrap_or_default()); (id, create_result) } diff --git a/src/application/services/file_management_service.rs b/src/application/services/file_management_service.rs index 7bec7187..77c53795 100644 --- a/src/application/services/file_management_service.rs +++ b/src/application/services/file_management_service.rs @@ -82,33 +82,26 @@ impl FileManagementService { } /// Verifies that the target folder is owned by the caller. - /// If folder_id is None (root), ownership is implicitly granted. + /// + /// `None` means the target is the user's root namespace + /// (`storage.files.folder_id IS NULL`) — implicitly owned by the caller, so + /// the check is skipped. Fails closed if `folder_repo` was not injected. async fn verify_target_folder_owner( &self, - folder_id: &Option, + folder_id: Option<&str>, caller_id: Uuid, ) -> Result<(), DomainError> { - let folder_id = match folder_id { - Some(id) => id, - None => return Ok(()), // Moving to root is always allowed + let Some(target) = folder_id else { + // TODO: File creation to root is currently allowed, check is this policy is relevant + return Ok(()); }; - - if let Some(folder_repo) = &self.folder_repo { - let folder_owner = folder_repo.get_folder_user_id(folder_id).await?; - if folder_owner != caller_id { - return Err(DomainError::not_found( - "Folder", - "Target folder not found or access denied", - )); - } - Ok(()) - } else { - // Fallback: no folder repo injected — deny by default (fail-closed) - Err(DomainError::internal_error( + let Some(folder_repo) = &self.folder_repo else { + return Err(DomainError::internal_error( "FileManagement", "Folder ownership verification unavailable", - )) - } + )); + }; + folder_repo.verify_owner(target, caller_id).await } } @@ -151,7 +144,7 @@ impl FileManagementUseCase for FileManagementService { // Verify file ownership first self.verify_owner(file_id, caller_id).await?; // Verify target folder ownership (prevents file from "disappearing") - self.verify_target_folder_owner(&folder_id, caller_id) + self.verify_target_folder_owner(folder_id.as_deref(), caller_id) .await?; self.move_file(file_id, folder_id).await } @@ -192,7 +185,7 @@ impl FileManagementUseCase for FileManagementService { target_folder_id: Option, ) -> Result { self.verify_owner(file_id, caller_id).await?; - self.verify_target_folder_owner(&target_folder_id, caller_id) + self.verify_target_folder_owner(target_folder_id.as_deref(), caller_id) .await?; self.copy_file(file_id, target_folder_id).await } @@ -334,21 +327,11 @@ impl FileManagementUseCase for FileManagementService { target_parent_id: Option, dest_name: Option, ) -> Result { - if let Some(folder_repo) = &self.folder_repo { - let owner = folder_repo.get_folder_user_id(source_folder_id).await?; - if owner != caller_id { - return Err(DomainError::not_found( - "Folder", - "Source folder not found or access denied", - )); - } - } else { - return Err(DomainError::internal_error( - "FileManagement", - "Folder ownership verification unavailable", - )); - } - self.verify_target_folder_owner(&target_parent_id, caller_id) + // Source ownership: source_folder_id is required (not optional), but reuse the + // wrapper which also enforces the fail-closed semantics if folder_repo is absent. + self.verify_target_folder_owner(Some(source_folder_id), caller_id) + .await?; + self.verify_target_folder_owner(target_parent_id.as_deref(), caller_id) .await?; self.copy_folder_tree(source_folder_id, target_parent_id, dest_name) .await diff --git a/src/application/services/folder_service.rs b/src/application/services/folder_service.rs index d57f805f..b8e910b0 100644 --- a/src/application/services/folder_service.rs +++ b/src/application/services/folder_service.rs @@ -25,7 +25,11 @@ impl FolderService { struct FolderServiceStub; impl FolderUseCase for FolderServiceStub { - async fn create_folder(&self, _dto: CreateFolderDto) -> Result { + async fn create_folder( + &self, + _dto: CreateFolderDto, + _user_id: Uuid, + ) -> Result { Ok(FolderDto::empty()) } @@ -134,8 +138,11 @@ impl FolderService { impl FolderUseCase for FolderService { /// Creates a new folder - async fn create_folder(&self, dto: CreateFolderDto) -> Result { - // Input validation + async fn create_folder( + &self, + dto: CreateFolderDto, + caller_id: Uuid, + ) -> Result { if let Err(reason) = validate_storage_name(&dto.name) { return Err(DomainError::validation_error(format!( "Invalid folder name '{}': {reason}", @@ -143,21 +150,19 @@ impl FolderUseCase for FolderService { ))); } - // If a parent_id is provided, verify it exists - if let Some(parent_id) = &dto.parent_id { - let parent_exists = self.folder_storage.get_folder(parent_id).await.is_ok(); - if !parent_exists { - return Err(DomainError::not_found("Folder", parent_id)); - } - } + let Some(parent_id) = dto.parent_id.as_deref() else { + return Err(DomainError::validation_error( + "Root folder creation is reserved for registration", + )); + }; + self.folder_storage + .verify_owner(parent_id, caller_id) + .await?; - // Create the folder let folder = self .folder_storage .create_folder(dto.name, dto.parent_id) .await?; - - // Convert to DTO Ok(FolderDto::from(folder)) } diff --git a/src/common/stubs.rs b/src/common/stubs.rs index 01328051..43fd0f5f 100644 --- a/src/common/stubs.rs +++ b/src/common/stubs.rs @@ -353,7 +353,11 @@ impl I18nService for StubI18nService { pub struct StubFolderUseCase; impl FolderUseCase for StubFolderUseCase { - async fn create_folder(&self, _dto: CreateFolderDto) -> Result { + async fn create_folder( + &self, + _dto: CreateFolderDto, + _user_id: Uuid, + ) -> Result { Ok(FolderDto::default()) } diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index ca69564b..8001204b 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -1044,4 +1044,20 @@ impl FolderDbRepository { .map_err(|e| DomainError::internal_error("FolderDb", format!("user_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 + /// "folder owned by someone else" — same error to avoid leaking the + /// existence of resources belonging to other users. + pub async fn verify_owner(&self, folder_id: &str, owner_id: Uuid) -> Result<(), DomainError> { + let actual = self.get_folder_user_id(folder_id).await?; + if actual != owner_id { + return Err(DomainError::not_found( + "Folder", + "Target folder not found or access denied", + )); + } + Ok(()) + } } diff --git a/src/interfaces/api/handlers/folder_handler.rs b/src/interfaces/api/handlers/folder_handler.rs index 8caea7b8..45cc0276 100644 --- a/src/interfaces/api/handlers/folder_handler.rs +++ b/src/interfaces/api/handlers/folder_handler.rs @@ -76,25 +76,7 @@ impl FolderHandler { } } - // ── SECURITY: Verify parent folder ownership (IDOR V-04 fix) ── - if let Some(ref parent_id) = dto.parent_id { - use crate::application::ports::inbound::FolderUseCase; - if service - .get_folder_owned(parent_id, auth_user.id) - .await - .is_err() - { - tracing::warn!( - "create_folder: user '{}' attempted to create folder in parent '{}' owned by another user", - auth_user.username, - parent_id, - ); - return AppError::not_found(format!("Parent folder not found: {}", parent_id)) - .into_response(); - } - } - - match service.create_folder(dto).await { + match service.create_folder(dto, auth_user.id).await { Ok(folder) => (StatusCode::CREATED, Json(folder)).into_response(), Err(err) => AppError::from(err).into_response(), } diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 826c8664..2a2f58b3 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -999,6 +999,7 @@ async fn handle_mkcol( req: Request, path: String, ) -> Result, AppError> { + let user = extract_user(&req)?; let folder_service = &state.applications.folder_service; if path.is_empty() || path == "/" { @@ -1041,15 +1042,13 @@ async fn handle_mkcol( name: segment.to_string(), parent_id: parent_id.clone(), }; + // Propagate DomainError -> AppError so NotFound/Conflict map to + // their proper HTTP status codes (was: blanket 500 swallowed + // ownership-rejection NotFound from verify_owner). let created = folder_service - .create_folder(create_dto) + .create_folder(create_dto, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to create folder '{}': {}", - accumulated_path, e - )) - })?; + .map_err(AppError::from)?; parent_id = Some(created.id); } } @@ -1551,7 +1550,7 @@ async fn handle_copy( parent_id: target_parent_id, }; folder_service - .create_folder(create_dto) + .create_folder(create_dto, user.id) .await .map_err(|e| { AppError::internal_error(format!( @@ -1655,7 +1654,7 @@ async fn handle_copy( parent_id: target_parent_id, }; folder_service - .create_folder(create_dto) + .create_folder(create_dto, user.id) .await .map_err(|e| { AppError::internal_error(format!( diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index a383ad05..dd133112 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -551,6 +551,7 @@ async fn handle_put( .map_err(|e| AppError::internal_error(format!("Failed to update file: {}", e)))?; // Update audio metadata for supported audio files. + // TODO: use notification service or hook if let Some(ref audio_service) = state.applications.audio_metadata_service && let Ok(file_id) = uuid::Uuid::parse_str(&updated.id) { @@ -655,7 +656,7 @@ async fn handle_mkcol( name: segment.to_string(), parent_id: Some(parent_id.clone()), }; - match folder_service.create_folder(dto).await { + match folder_service.create_folder(dto, user.id).await { Ok(created) => { parent_id = created.id.clone(); }