diff --git a/src/application/services/trash_service.rs b/src/application/services/trash_service.rs index 5892f962..aa09dc32 100644 --- a/src/application/services/trash_service.rs +++ b/src/application/services/trash_service.rs @@ -662,22 +662,25 @@ impl TrashUseCase for TrashService { async fn empty_trash_for_drive(&self, user_id: Uuid, drive_id: Uuid) -> Result<()> { // Per-drive trash empty — the Drive group-by on `/trash` exposes // this as a per-row affordance so multi-drive owners can clear - // one drive without touching the others. Refuses with - // `NotFound` (anti-enum) when the caller lacks Delete on the - // named drive — same shape as the user-facing drive listing - // would emit for an unknown id. - let allowed = self.drives_with_delete_for(user_id).await?; - if !allowed.contains(&drive_id) { - tracing::info!( - target: "audit", - event = "trash.empty_drive_rejected", - reason = "no_delete_on_drive", - user_id = %user_id, - drive_id = %drive_id, - "👮🏻‍♂️ refused per-drive empty — caller lacks Delete on this drive", - ); - return Err(DomainError::not_found("Drive", drive_id.to_string())); - } + // one drive without touching the others. + // + // Route through `authz.require(Delete, Drive)` so the denial + // shape stays consistent with every other write verb: 403 when + // the caller has Read on the drive (viewer/editor holding no + // Delete), 404 when they don't (anti-enum). Before 2026-07-16 + // this method rolled its own `drives_with_delete_for` check + + // hardcoded `NotFound` — that predated the graduated-denial + // engine change and returned 404 unconditionally even for a + // Viewer who could see the drive in `/api/drives`. The engine + // now emits `authz.denied` with `visibility="visible"|"hidden"` + // and the standard mapping renders it as 403 or 404. + self.authz + .require( + Subject::User(user_id), + Permission::Delete, + Resource::Drive(drive_id), + ) + .await?; info!("Emptying trash for drive {} (user {})", drive_id, user_id); self.clear_trash_in(&[drive_id], user_id).await } diff --git a/src/interfaces/nextcloud/uploads_handler.rs b/src/interfaces/nextcloud/uploads_handler.rs index 194b9c8a..d4761c47 100644 --- a/src/interfaces/nextcloud/uploads_handler.rs +++ b/src/interfaces/nextcloud/uploads_handler.rs @@ -6,7 +6,7 @@ use axum::{ use std::sync::Arc; use uuid::Uuid; -use crate::application::ports::file_ports::{FileRetrievalUseCase, FileUploadUseCase}; +use crate::application::ports::file_ports::FileUploadUseCase; use crate::application::ports::storage_ports::StorageUsagePort; use crate::common::di::AppState; use crate::common::mime_detect::filename_from_path; @@ -402,8 +402,6 @@ async fn handle_assemble( .map_err(|e| AppError::internal_error(format!("Failed to list chunks: {}", e)))?; let upload_service = &state.applications.file_upload_service; - let file_service = &state.applications.file_retrieval_service; - let folder_service = &state.applications.folder_service; // Path-based lookups below scope by `drive_id`. The NC session's // chroot is always populated for path-scoped handlers (see @@ -433,64 +431,41 @@ async fn handle_assemble( .await?; let content_type = ingested.content_type.clone(); - // Check if file exists (update vs create). - let existing = file_service - .get_file_by_path(&internal_path, drive_id) - .await; - - let etag: Option = if existing.is_ok() { - let dto = upload_service - .update_file_streaming_with_perms( - &internal_path, - drive_id, - ingested.stored(), - &content_type, - oc_mtime, - user.id, - ) - .await - .map_err(|e| AppError::internal_error(format!("Failed to update file: {}", e)))?; - - Some(dto.etag) - } else { - // New-file branch: resolve the parent folder by path and register - // the file row against the already-ingested blob. - let (parent_sub, filename) = match dest_subpath.rsplit_once('/') { - Some((p, n)) => (p, n), - None => ("", dest_subpath.as_str()), - }; - let parent_internal = - crate::interfaces::nextcloud::webdav_handler::nc_to_internal_path(chroot, parent_sub)?; - let parent_internal = parent_internal.trim_end_matches('/'); - - use crate::application::ports::folder_ports::FolderUseCase; - let parent_folder = match folder_service - .get_folder_by_path(parent_internal, drive_id) - .await - { - Ok(folder) => folder, - Err(e) => { - discard_ingested(&state.core.dedup_service, &ingested).await; - return Err(AppError::internal_error(format!( - "Parent folder lookup failed: {}", - e - ))); - } - }; - - let dto = upload_service - .upload_file_streaming( - filename.to_string(), - Some(parent_folder.id), - content_type.to_string(), - ingested.stored(), - user.id, - ) - .await - .map_err(|e| AppError::internal_error(format!("Failed to create file: {}", e)))?; - - Some(dto.etag) + // AuthZ audit #12 (2026-07-12): the previous shape branched on + // file existence — `update_file_streaming_with_perms` on the + // overwrite path (correct), plain `upload_file_streaming` on + // the create path (NO `authz.require`). Viewer/Commenter on a + // shared drive could MKCOL → PUT chunks → MOVE and land a + // brand-new file, skipping the `Create`-on-parent-folder gate. + // + // `update_file_streaming_with_perms` handles both branches + // atomically: `Update` on the existing file OR `Create` on the + // parent folder / drive root (per the service's own internal + // fork). Funneling everything through the one method also + // deletes the duplicated parent-folder lookup that used to + // live here. + // + // AuthZ audit #2 (2026-07-12): route DomainError through + // `AppError::from` so authz denials keep the graduated 403/404 + // shape instead of collapsing into 500. + let dto = match upload_service + .update_file_streaming_with_perms( + &internal_path, + drive_id, + ingested.stored(), + &content_type, + oc_mtime, + user.id, + ) + .await + { + Ok(dto) => dto, + Err(e) => { + discard_ingested(&state.core.dedup_service, &ingested).await; + return Err(AppError::from(e)); + } }; + let etag: Option = Some(dto.etag); // Cleanup session. let _ = nc.chunked_uploads.cleanup(&user.username, upload_id).await; diff --git a/tests/api/trash_per_drive.hurl b/tests/api/trash_per_drive.hurl index f8cd89d0..4b0fbdeb 100644 --- a/tests/api/trash_per_drive.hurl +++ b/tests/api/trash_per_drive.hurl @@ -190,8 +190,12 @@ HTTP 404 # ───────────────────────────────────────────────────────────── # Step 9 — Provision a Viewer of the shared drive (`tpd_viewer`), -# then assert the per-drive empty refuses for Viewer / Editor -# / non-member callers. Each refusal is 404 (anti-enum). +# then assert the per-drive empty refuses for Viewer / +# Editor / non-member callers. Graduated denial (see +# [[project_authz_require_graduated_denial]]): the Viewer +# and Editor tests get 403 because they hold Read on the +# drive; the non-member fallback keeps the 404 anti-enum +# shape (no Read = no existence oracle). # ───────────────────────────────────────────────────────────── POST {{base_url}}/api/admin/users Authorization: Bearer {{admin_token}} @@ -249,17 +253,19 @@ Authorization: Bearer {{owner_token}} HTTP 204 -# Test 4 — Viewer cannot empty the drive's trash. +# Test 4 — Viewer cannot empty the drive's trash. Viewer has Read +# on the drive → graduated denial returns 403. DELETE {{base_url}}/api/trash/drive/{{shared_drive_id}} Authorization: Bearer {{viewer_token}} -HTTP 404 +HTTP 403 # ───────────────────────────────────────────────────────────── # Step 10 — Test 5: Editor cannot either. # Promote tpd_viewer to Editor; same refusal. Confirms -# `Delete` isn't in the Editor bundle. +# `Delete` isn't in the Editor bundle. Editor has Read → +# graduated denial returns 403. # ───────────────────────────────────────────────────────────── PATCH {{base_url}}/api/drives/{{shared_drive_id}}/members/user/{{viewer_user_id}} Authorization: Bearer {{owner_token}} @@ -272,7 +278,7 @@ HTTP 200 DELETE {{base_url}}/api/trash/drive/{{shared_drive_id}} Authorization: Bearer {{viewer_token}} -HTTP 404 +HTTP 403 # ───────────────────────────────────────────────────────────── @@ -315,6 +321,74 @@ HTTP 200 jsonpath "$.items[*].drive_id" contains "{{shared_drive_id}}" +# ───────────────────────────────────────────────────────────── +# Step 11b — Regression pin for AuthZ audit #10 (2026-07-12). +# `POST /api/trash/{id}/restore` and `DELETE /api/trash/{id}` +# once did `err_str.contains("not found")` to decide "already +# gone" vs real failure — an authz denial (which returns a +# `NotFound`-shaped DomainError to preserve anti-enum on the +# listing side) matched the substring and got synthesised +# into a 200 `{"success": true}` response. Response lied; +# no mutation happened. +# +# Post-fix: both handlers route through +# `AppError::from(e).into_response()`, so authz denials +# surface as the graduated 403 / 404 shape and body is +# never a success envelope. +# +# The Editor (from Step 10 promotion) holds Read on the +# canary — graduated denial returns 403 with a +# `AccessDenied`-shape body, NOT a success envelope. If a +# future refactor reintroduces the substring hack this +# assertion trips before it lands in prod. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/trash/resources +Authorization: Bearer {{viewer_token}} + +HTTP 200 +[Captures] +# The shared drive's trash holds exactly one item at this point (the +# canary owner trashed after Step 9), so `$.items[0]` is unambiguous +# — no filter needed. `TrashResourceItemDto` wraps the underlying +# resource in `.resource` (untagged File | Folder | Drive enum) and +# the trash key equals the original resource id (see +# `storage.trash_items` view), so `.resource.id` is exactly what +# `POST /api/trash/{id}/restore` and `DELETE /api/trash/{id}` accept. +# The `[?(...)]` + `nth 0` shape (see the sibling +# feedback_hurl_jsonpath_filter_empty memory) collapses on a single +# match and returns a scalar hurl can't index, so we avoid it here. +canary_trash_id: jsonpath "$.items[0].resource.id" + + +POST {{base_url}}/api/trash/{{canary_trash_id}}/restore +Authorization: Bearer {{viewer_token}} + +HTTP 403 +[Asserts] +body not contains "\"success\":true" + + +DELETE {{base_url}}/api/trash/{{canary_trash_id}} +Authorization: Bearer {{viewer_token}} + +HTTP 403 +[Asserts] +body not contains "\"success\":true" + + +# The canary is still there — the two Editor attempts didn't mutate. +GET {{base_url}}/api/trash/resources +Authorization: Bearer {{owner_token}} + +HTTP 200 +[Asserts] +# Owner sees TWO trash items at this point — the shared drive's +# canary (from Step 9) plus their personal drive's leftover from +# Step 4 (owner emptied only the shared drive's trash at Step 6). +# `contains` avoids depending on the sort order between them. +jsonpath "$.items[*].resource.id" contains "{{canary_trash_id}}" + + # ───────────────────────────────────────────────────────────── # Step 12 — Cleanup: drop the canary, then the shared drive itself # (D3b's delete-drive guard refuses non-empty drives, so