refactor(create_folder): add an ownership check while creating a folder + refactor code

This commit is contained in:
Edouard Vanbelle
2026-05-20 12:48:06 +02:00
parent 91ff3df35f
commit f8b30e78a6
9 changed files with 76 additions and 89 deletions
+5 -1
View File
@@ -13,7 +13,11 @@ use crate::common::errors::DomainError;
/// Primary port for folder operations /// Primary port for folder operations
pub trait FolderUseCase: Send + Sync + 'static { pub trait FolderUseCase: Send + Sync + 'static {
/// Creates a new folder /// Creates a new folder
async fn create_folder(&self, dto: CreateFolderDto) -> Result<FolderDto, DomainError>; async fn create_folder(
&self,
dto: CreateFolderDto,
caller_id: Uuid,
) -> Result<FolderDto, DomainError>;
/// Gets a folder by its ID /// Gets a folder by its ID
async fn get_folder(&self, id: &str) -> Result<FolderDto, DomainError>; async fn get_folder(&self, id: &str) -> Result<FolderDto, DomainError>;
+1 -8
View File
@@ -975,18 +975,11 @@ impl BatchOperationService {
let folder_service = self.folder_service.clone(); let folder_service = self.folder_service.clone();
async move { 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 { let dto = crate::application::dtos::folder_dto::CreateFolderDto {
name: name.clone(), name: name.clone(),
parent_id: parent_id.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()); let id = format!("{}:{}", name, parent_id.unwrap_or_default());
(id, create_result) (id, create_result)
} }
@@ -82,33 +82,26 @@ impl FileManagementService {
} }
/// Verifies that the target folder is owned by the caller. /// 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( async fn verify_target_folder_owner(
&self, &self,
folder_id: &Option<String>, folder_id: Option<&str>,
caller_id: Uuid, caller_id: Uuid,
) -> Result<(), DomainError> { ) -> Result<(), DomainError> {
let folder_id = match folder_id { let Some(target) = folder_id else {
Some(id) => id, // TODO: File creation to root is currently allowed, check is this policy is relevant
None => return Ok(()), // Moving to root is always allowed return Ok(());
}; };
let Some(folder_repo) = &self.folder_repo else {
if let Some(folder_repo) = &self.folder_repo { return Err(DomainError::internal_error(
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(
"FileManagement", "FileManagement",
"Folder ownership verification unavailable", "Folder ownership verification unavailable",
)) ));
} };
folder_repo.verify_owner(target, caller_id).await
} }
} }
@@ -151,7 +144,7 @@ impl FileManagementUseCase for FileManagementService {
// Verify file ownership first // Verify file ownership first
self.verify_owner(file_id, caller_id).await?; self.verify_owner(file_id, caller_id).await?;
// Verify target folder ownership (prevents file from "disappearing") // 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?; .await?;
self.move_file(file_id, folder_id).await self.move_file(file_id, folder_id).await
} }
@@ -192,7 +185,7 @@ impl FileManagementUseCase for FileManagementService {
target_folder_id: Option<String>, target_folder_id: Option<String>,
) -> Result<FileDto, DomainError> { ) -> Result<FileDto, DomainError> {
self.verify_owner(file_id, caller_id).await?; 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?; .await?;
self.copy_file(file_id, target_folder_id).await self.copy_file(file_id, target_folder_id).await
} }
@@ -334,21 +327,11 @@ impl FileManagementUseCase for FileManagementService {
target_parent_id: Option<String>, target_parent_id: Option<String>,
dest_name: Option<String>, dest_name: Option<String>,
) -> Result<CopyFolderTreeResult, DomainError> { ) -> Result<CopyFolderTreeResult, DomainError> {
if let Some(folder_repo) = &self.folder_repo { // Source ownership: source_folder_id is required (not optional), but reuse the
let owner = folder_repo.get_folder_user_id(source_folder_id).await?; // wrapper which also enforces the fail-closed semantics if folder_repo is absent.
if owner != caller_id { self.verify_target_folder_owner(Some(source_folder_id), caller_id)
return Err(DomainError::not_found( .await?;
"Folder", self.verify_target_folder_owner(target_parent_id.as_deref(), caller_id)
"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)
.await?; .await?;
self.copy_folder_tree(source_folder_id, target_parent_id, dest_name) self.copy_folder_tree(source_folder_id, target_parent_id, dest_name)
.await .await
+18 -13
View File
@@ -25,7 +25,11 @@ impl FolderService {
struct FolderServiceStub; struct FolderServiceStub;
impl FolderUseCase for FolderServiceStub { impl FolderUseCase for FolderServiceStub {
async fn create_folder(&self, _dto: CreateFolderDto) -> Result<FolderDto, DomainError> { async fn create_folder(
&self,
_dto: CreateFolderDto,
_user_id: Uuid,
) -> Result<FolderDto, DomainError> {
Ok(FolderDto::empty()) Ok(FolderDto::empty())
} }
@@ -134,8 +138,11 @@ impl FolderService {
impl FolderUseCase for FolderService { impl FolderUseCase for FolderService {
/// Creates a new folder /// Creates a new folder
async fn create_folder(&self, dto: CreateFolderDto) -> Result<FolderDto, DomainError> { async fn create_folder(
// Input validation &self,
dto: CreateFolderDto,
caller_id: Uuid,
) -> Result<FolderDto, DomainError> {
if let Err(reason) = validate_storage_name(&dto.name) { if let Err(reason) = validate_storage_name(&dto.name) {
return Err(DomainError::validation_error(format!( return Err(DomainError::validation_error(format!(
"Invalid folder name '{}': {reason}", "Invalid folder name '{}': {reason}",
@@ -143,21 +150,19 @@ impl FolderUseCase for FolderService {
))); )));
} }
// If a parent_id is provided, verify it exists let Some(parent_id) = dto.parent_id.as_deref() else {
if let Some(parent_id) = &dto.parent_id { return Err(DomainError::validation_error(
let parent_exists = self.folder_storage.get_folder(parent_id).await.is_ok(); "Root folder creation is reserved for registration",
if !parent_exists { ));
return Err(DomainError::not_found("Folder", parent_id)); };
} self.folder_storage
} .verify_owner(parent_id, caller_id)
.await?;
// Create the folder
let folder = self let folder = self
.folder_storage .folder_storage
.create_folder(dto.name, dto.parent_id) .create_folder(dto.name, dto.parent_id)
.await?; .await?;
// Convert to DTO
Ok(FolderDto::from(folder)) Ok(FolderDto::from(folder))
} }
+5 -1
View File
@@ -353,7 +353,11 @@ impl I18nService for StubI18nService {
pub struct StubFolderUseCase; pub struct StubFolderUseCase;
impl FolderUseCase for StubFolderUseCase { impl FolderUseCase for StubFolderUseCase {
async fn create_folder(&self, _dto: CreateFolderDto) -> Result<FolderDto, DomainError> { async fn create_folder(
&self,
_dto: CreateFolderDto,
_user_id: Uuid,
) -> Result<FolderDto, DomainError> {
Ok(FolderDto::default()) Ok(FolderDto::default())
} }
@@ -1044,4 +1044,20 @@ impl FolderDbRepository {
.map_err(|e| DomainError::internal_error("FolderDb", format!("user_id lookup: {e}")))? .map_err(|e| DomainError::internal_error("FolderDb", format!("user_id lookup: {e}")))?
.ok_or_else(|| DomainError::not_found("Folder", folder_id)) .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(())
}
} }
+1 -19
View File
@@ -76,25 +76,7 @@ impl FolderHandler {
} }
} }
// ── SECURITY: Verify parent folder ownership (IDOR V-04 fix) ── match service.create_folder(dto, auth_user.id).await {
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 {
Ok(folder) => (StatusCode::CREATED, Json(folder)).into_response(), Ok(folder) => (StatusCode::CREATED, Json(folder)).into_response(),
Err(err) => AppError::from(err).into_response(), Err(err) => AppError::from(err).into_response(),
} }
@@ -999,6 +999,7 @@ async fn handle_mkcol(
req: Request<Body>, req: Request<Body>,
path: String, path: String,
) -> Result<Response<Body>, AppError> { ) -> Result<Response<Body>, AppError> {
let user = extract_user(&req)?;
let folder_service = &state.applications.folder_service; let folder_service = &state.applications.folder_service;
if path.is_empty() || path == "/" { if path.is_empty() || path == "/" {
@@ -1041,15 +1042,13 @@ async fn handle_mkcol(
name: segment.to_string(), name: segment.to_string(),
parent_id: parent_id.clone(), 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 let created = folder_service
.create_folder(create_dto) .create_folder(create_dto, user.id)
.await .await
.map_err(|e| { .map_err(AppError::from)?;
AppError::internal_error(format!(
"Failed to create folder '{}': {}",
accumulated_path, e
))
})?;
parent_id = Some(created.id); parent_id = Some(created.id);
} }
} }
@@ -1551,7 +1550,7 @@ async fn handle_copy(
parent_id: target_parent_id, parent_id: target_parent_id,
}; };
folder_service folder_service
.create_folder(create_dto) .create_folder(create_dto, user.id)
.await .await
.map_err(|e| { .map_err(|e| {
AppError::internal_error(format!( AppError::internal_error(format!(
@@ -1655,7 +1654,7 @@ async fn handle_copy(
parent_id: target_parent_id, parent_id: target_parent_id,
}; };
folder_service folder_service
.create_folder(create_dto) .create_folder(create_dto, user.id)
.await .await
.map_err(|e| { .map_err(|e| {
AppError::internal_error(format!( AppError::internal_error(format!(
+2 -1
View File
@@ -551,6 +551,7 @@ async fn handle_put(
.map_err(|e| AppError::internal_error(format!("Failed to update file: {}", e)))?; .map_err(|e| AppError::internal_error(format!("Failed to update file: {}", e)))?;
// Update audio metadata for supported audio files. // Update audio metadata for supported audio files.
// TODO: use notification service or hook
if let Some(ref audio_service) = state.applications.audio_metadata_service if let Some(ref audio_service) = state.applications.audio_metadata_service
&& let Ok(file_id) = uuid::Uuid::parse_str(&updated.id) && let Ok(file_id) = uuid::Uuid::parse_str(&updated.id)
{ {
@@ -655,7 +656,7 @@ async fn handle_mkcol(
name: segment.to_string(), name: segment.to_string(),
parent_id: Some(parent_id.clone()), parent_id: Some(parent_id.clone()),
}; };
match folder_service.create_folder(dto).await { match folder_service.create_folder(dto, user.id).await {
Ok(created) => { Ok(created) => {
parent_id = created.id.clone(); parent_id = created.id.clone();
} }