From dd68d783e0748e9af68428ee2d1fe93598a3bd55 Mon Sep 17 00:00:00 2001
From: Edouard Vanbelle <opensource@edouard.vanbelle.fr>
Date: Thu, 21 May 2026 22:45:49 +0200
Subject: [PATCH] fix(authz): permit policiy: a user with Delete permission can
 delete a file/folder. Only the owner can permanently delete or restore a
 trashed item

---
 src/application/services/trash_service.rs | 24 +++++------------------
 tests/api/grants.hurl                     | 16 +++++++--------
 2 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/src/application/services/trash_service.rs b/src/application/services/trash_service.rs
index 0980da6e..c002c945 100644
--- a/src/application/services/trash_service.rs
+++ b/src/application/services/trash_service.rs
@@ -218,13 +218,6 @@ impl TrashUseCase for TrashService {
             "file" => {
                 info!("Processing file to move to trash: {}", item_id);
 
-                // XXX: right now only owner can move to trash, need to improve
-
-                // Get the file — ownership-verified at SQL level.
-                // Returns NotFound if the file does not exist OR belongs to
-                // another user, preventing cross-user trash operations.
-                debug!("Getting file data (owner-scoped): {}", item_id);
-
                 let file_id = Uuid::parse_str(item_id)
                     .map_err(|_| DomainError::not_found("File", item_id))?;
                 self.authz
@@ -235,11 +228,11 @@ impl TrashUseCase for TrashService {
                     )
                     .await?;
 
-                let file = match self
-                    .file_read_port
-                    .get_file_for_owner(item_id, user_id)
-                    .await
-                {
+                // Authz already passed — use the non-owner-scoped read so that
+                // grantees with Delete permission can trash files they don't own.
+                // The file's user_id in storage.files is unchanged, so the item
+                // will appear in the original owner's trash view.
+                let file = match self.file_read_port.get_file(item_id).await {
                     Ok(file) => {
                         debug!("File found: {} ({})", file.name(), item_id);
                         file
@@ -257,8 +250,6 @@ impl TrashUseCase for TrashService {
                 let original_path = file.storage_path().to_string();
                 debug!("Original file path: {}", original_path);
 
-                // Create the trash item
-                // FIXME: item will be created with user_id that mat not be the owner_id
                 debug!("Creating TrashedItem object for the file");
                 let trashed_item = TrashedItem::new(
                     item_uuid,
@@ -320,9 +311,6 @@ impl TrashUseCase for TrashService {
                     )
                     .await?;
 
-                // Get the folder and verify ownership.
-                // Returns NotFound if the folder does not exist or belongs
-                // to another user — prevents cross-user trash operations.
                 let folder = self
                     .folder_storage_port
                     .get_folder(item_id)
@@ -337,8 +325,6 @@ impl TrashUseCase for TrashService {
 
                 let original_path = folder.storage_path().to_string();
 
-                // Create the trash item
-                // FIXME: item will be created with user_id that mat not be the owner_id
                 let trashed_item = TrashedItem::new(
                     item_uuid,
                     user_uuid,
diff --git a/tests/api/grants.hurl b/tests/api/grants.hurl
index 059efd08..766d397d 100644
--- a/tests/api/grants.hurl
+++ b/tests/api/grants.hurl
@@ -1179,24 +1179,22 @@ Content-Type: application/json
 
 HTTP 200
 
-# Batch trash — CURRENT LIMITATION: even with Admin (Delete grant via
-# engine), the trash flow inside trash_service uses get_file_for_owner
-# at the data layer, which is owner-scoped. So a non-owner with Delete
-# grant gets engine-OK but the SQL filter blocks the fetch → 400.
-# This is documented inconsistency; a follow-up should make trash use
-# the engine for its lookup too. For now: only the owner can trash.
+# Frank (Admin grant = Delete) trashes batch_file_2 — item goes to
+# Alice's trash because file.user_id is unchanged (Alice is still owner).
 POST {{base_url}}/api/batch/trash
 Authorization: Bearer {{frank_token}}
 Content-Type: application/json
 { "file_ids": ["{{batch_file_2_id}}"], "folder_ids": [] }
 
-HTTP 400
+HTTP 200
+[Asserts]
+jsonpath "$.stats.successful" == 1
 
-# Alice (owner) CAN batch-trash — keeps coverage of the success path.
+# Alice trashes batch_file_1 (which frank moved into batch_sub_a in Phase 3C).
 POST {{base_url}}/api/batch/trash
 Authorization: Bearer {{alice_token}}
 Content-Type: application/json
-{ "file_ids": ["{{batch_file_2_id}}"], "folder_ids": [] }
+{ "file_ids": ["{{batch_file_1_id}}"], "folder_ids": [] }
 
 HTTP 200
 [Asserts]