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
This commit is contained in:
@@ -218,13 +218,6 @@ impl TrashUseCase for TrashService {
|
|||||||
"file" => {
|
"file" => {
|
||||||
info!("Processing file to move to trash: {}", item_id);
|
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)
|
let file_id = Uuid::parse_str(item_id)
|
||||||
.map_err(|_| DomainError::not_found("File", item_id))?;
|
.map_err(|_| DomainError::not_found("File", item_id))?;
|
||||||
self.authz
|
self.authz
|
||||||
@@ -235,11 +228,11 @@ impl TrashUseCase for TrashService {
|
|||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
|
||||||
let file = match self
|
// Authz already passed — use the non-owner-scoped read so that
|
||||||
.file_read_port
|
// grantees with Delete permission can trash files they don't own.
|
||||||
.get_file_for_owner(item_id, user_id)
|
// The file's user_id in storage.files is unchanged, so the item
|
||||||
.await
|
// will appear in the original owner's trash view.
|
||||||
{
|
let file = match self.file_read_port.get_file(item_id).await {
|
||||||
Ok(file) => {
|
Ok(file) => {
|
||||||
debug!("File found: {} ({})", file.name(), item_id);
|
debug!("File found: {} ({})", file.name(), item_id);
|
||||||
file
|
file
|
||||||
@@ -257,8 +250,6 @@ impl TrashUseCase for TrashService {
|
|||||||
let original_path = file.storage_path().to_string();
|
let original_path = file.storage_path().to_string();
|
||||||
debug!("Original file path: {}", original_path);
|
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");
|
debug!("Creating TrashedItem object for the file");
|
||||||
let trashed_item = TrashedItem::new(
|
let trashed_item = TrashedItem::new(
|
||||||
item_uuid,
|
item_uuid,
|
||||||
@@ -320,9 +311,6 @@ impl TrashUseCase for TrashService {
|
|||||||
)
|
)
|
||||||
.await?;
|
.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
|
let folder = self
|
||||||
.folder_storage_port
|
.folder_storage_port
|
||||||
.get_folder(item_id)
|
.get_folder(item_id)
|
||||||
@@ -337,8 +325,6 @@ impl TrashUseCase for TrashService {
|
|||||||
|
|
||||||
let original_path = folder.storage_path().to_string();
|
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(
|
let trashed_item = TrashedItem::new(
|
||||||
item_uuid,
|
item_uuid,
|
||||||
user_uuid,
|
user_uuid,
|
||||||
|
|||||||
@@ -1179,24 +1179,22 @@ Content-Type: application/json
|
|||||||
|
|
||||||
HTTP 200
|
HTTP 200
|
||||||
|
|
||||||
# Batch trash — CURRENT LIMITATION: even with Admin (Delete grant via
|
# Frank (Admin grant = Delete) trashes batch_file_2 — item goes to
|
||||||
# engine), the trash flow inside trash_service uses get_file_for_owner
|
# Alice's trash because file.user_id is unchanged (Alice is still 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.
|
|
||||||
POST {{base_url}}/api/batch/trash
|
POST {{base_url}}/api/batch/trash
|
||||||
Authorization: Bearer {{frank_token}}
|
Authorization: Bearer {{frank_token}}
|
||||||
Content-Type: application/json
|
Content-Type: application/json
|
||||||
{ "file_ids": ["{{batch_file_2_id}}"], "folder_ids": [] }
|
{ "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
|
POST {{base_url}}/api/batch/trash
|
||||||
Authorization: Bearer {{alice_token}}
|
Authorization: Bearer {{alice_token}}
|
||||||
Content-Type: application/json
|
Content-Type: application/json
|
||||||
{ "file_ids": ["{{batch_file_2_id}}"], "folder_ids": [] }
|
{ "file_ids": ["{{batch_file_1_id}}"], "folder_ids": [] }
|
||||||
|
|
||||||
HTTP 200
|
HTTP 200
|
||||||
[Asserts]
|
[Asserts]
|
||||||
|
|||||||
Reference in New Issue
Block a user