From 184520c17a3061b27edbd1d4a780e9d3c629a9bd Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 23 Jun 2026 21:08:55 +0200 Subject: [PATCH] feat(drive): plug trash to drives --- frontend/src/lib/api/types.ts | 8 ++ frontend/src/routes/trash/+page.svelte | 49 ++++++- .../20260804000000_trash_items_drive_id.sql | 51 +++++++ src/application/dtos/trash_dto.rs | 11 ++ src/application/services/trash_service.rs | 126 +++++++++++++++--- .../services/trash_service_test.rs | 35 +++-- src/common/di.rs | 4 +- src/domain/repositories/trash_repository.rs | 34 ++++- .../repositories/pg/trash_db_repository.rs | 66 ++++++--- tests/api/trash_resources.hurl | 9 ++ 10 files changed, 324 insertions(+), 69 deletions(-) create mode 100644 migrations/20260804000000_trash_items_drive_id.sql diff --git a/frontend/src/lib/api/types.ts b/frontend/src/lib/api/types.ts index a774c99c..56356ed5 100644 --- a/frontend/src/lib/api/types.ts +++ b/frontend/src/lib/api/types.ts @@ -120,6 +120,14 @@ export interface TrashResourceItem { resource_type: ItemType; trashed_at: string; deletion_date: string; + /** + * Drive the trashed item belongs to (D2b). Enables client-side + * group-by-drive in the `/trash` UI without resolving the drive from + * `resource.drive_id` per row. The drive's display name resolves + * against `drives.svelte` (the in-memory store already populated by + * the sidebar picker / config pages — no extra round-trip). + */ + drive_id: string; resource: FileItem | FolderItem; } diff --git a/frontend/src/routes/trash/+page.svelte b/frontend/src/routes/trash/+page.svelte index 45761863..8cf58d04 100644 --- a/frontend/src/routes/trash/+page.svelte +++ b/frontend/src/routes/trash/+page.svelte @@ -10,7 +10,7 @@ restoreTrashItem } from '$lib/api/endpoints/trash'; import { dateBucket, sizeBucket, typeLabel } from '$lib/api/endpoints/favorites'; - import type { FileItem, TrashResourceItem } from '$lib/api/types'; + import type { Drive, FileItem, TrashResourceItem } from '$lib/api/types'; import Icon from '$lib/icons/Icon.svelte'; import ResourceList, { type GroupByDef, @@ -18,6 +18,7 @@ } from '$lib/components/ResourceList.svelte'; import { confirmDialog } from '$lib/stores/dialogs.svelte'; import { t } from '$lib/i18n/index.svelte'; + import { drives as drivesStore } from '$lib/stores/drives.svelte'; import { ui } from '$lib/stores/ui.svelte'; let raw = $state([]); @@ -41,13 +42,48 @@ // `date` carries the deletion date — rendered as an expiry chip. date: it.deletion_date, category: isFile ? it.resource.category : 'Folder', - modifiedAt: it.trashed_at + modifiedAt: it.trashed_at, + // D2b: surface drive_id so the Drive group-by can bucket by it. + // Reuses the existing `ownerId` slot on ResourceEntry — both + // represent a UUID the listing pivots on; no new field needed. + ownerId: it.drive_id }; }) ); + // "Drive" group rank: default-personal first, then secondary personal, then + // shared — matches `DrivePicker.svelte::sortedDrives` so the sidebar and + // trash sections agree on ordering. Used as the bucket sort key. + function driveRank(d: Drive | null): number { + if (!d) return 99; + if (d.default_for_user) return 0; + return d.kind === 'personal' ? 1 : 2; + } + function driveLabel(driveId: string): string { + const d = drivesStore.findById(driveId); + return d?.name ?? driveId; + } + function driveBucketKey(driveId: string): string { + // Bucket key has to be a string but we want ordering; prefix with + // rank so the natural lexical sort puts buckets in the picker's order. + const d = drivesStore.findById(driveId); + const rank = driveRank(d).toString().padStart(2, '0'); + return `${rank}:${driveId}`; + } + function driveBucketLabel(key: string): string { + const driveId = key.split(':')[1] ?? key; + return driveLabel(driveId); + } + const groupBys: GroupByDef[] = [ - { key: '', label: t('files.name', 'Name'), orderBy: 'name' }, + { key: '', label: t('files.name', 'Name'), orderBy: 'name', icon: 'arrow-up-a-z' }, + { + key: 'drive', + label: t('trash.groupby.drive', 'Drive'), + orderBy: 'name', + bucketOf: (e) => (e.ownerId ? driveBucketKey(e.ownerId) : null), + labelOf: driveBucketLabel + }, { key: 'remainingDays', label: t('trash.groupby.remaining_days', 'Remaining days'), @@ -148,7 +184,12 @@ } } - onMount(() => load(true)); + onMount(() => { + // Drive names for the "Drive" group-by labels — `drivesStore.load()` is + // idempotent (cached on the singleton) so this is essentially free. + void drivesStore.load(); + void load(true); + }); {t('nav.trash', 'Trash')} · OxiCloud diff --git a/migrations/20260804000000_trash_items_drive_id.sql b/migrations/20260804000000_trash_items_drive_id.sql new file mode 100644 index 00000000..1b41aa89 --- /dev/null +++ b/migrations/20260804000000_trash_items_drive_id.sql @@ -0,0 +1,51 @@ +-- D2b — surface `drive_id` on the unified trash view. +-- +-- `storage.trash_items` is the read-side projection of soft-deleted files and +-- folders. D0 added `drive_id` to both `storage.files` and `storage.folders`, +-- but the view shipped before that and still reflects only `user_id`. D2b's +-- per-drive trash authorisation needs `drive_id` per row so: +-- +-- 1. Listing can filter to drives the caller can read (the storage +-- precheck — `pg_acl_engine.caller_role_on_drive_cached` — replaces +-- the legacy `WHERE user_id = caller_id` scope). +-- 2. The UI can group trash items by drive (per the D2b spec). +-- 3. The trash sweeper / orphan reclamation paths (added later in D2b) +-- can operate per-drive instead of per-user. +-- +-- The view is `CREATE OR REPLACE`, so this is a pure schema-shape change — +-- no data migration needed. The two source columns (`storage.files.drive_id`, +-- `storage.folders.drive_id`) are both `NOT NULL` after D0's +-- `20260802100002_drives_not_null.sql`, so the projection inherits NOT NULL +-- semantics automatically (no `COALESCE` fallback needed). + +-- `CREATE OR REPLACE VIEW` is restrictive: it can ADD columns at the END +-- but never re-order or rename existing ones. `drive_id` goes after every +-- pre-existing column so PG doesn't read this as renaming `trashed_at` → +-- `drive_id`. Column ORDER on the view changes; consumers SELECT by name +-- so they're unaffected. + +CREATE OR REPLACE VIEW storage.trash_items AS + SELECT f.id, f.name, 'file' AS item_type, f.user_id, f.trashed_at, + f.original_folder_id AS original_parent_id, f.created_at, + f.drive_id + FROM storage.files f + WHERE f.is_trashed = TRUE + AND (f.folder_id IS NULL + OR NOT EXISTS ( + SELECT 1 FROM storage.folders p + WHERE p.id = f.folder_id AND p.is_trashed = TRUE)) + UNION ALL + SELECT fo.id, fo.name, 'folder' AS item_type, fo.user_id, fo.trashed_at, + fo.original_parent_id, fo.created_at, + fo.drive_id + FROM storage.folders fo + WHERE fo.is_trashed = TRUE + AND (fo.parent_id IS NULL + OR NOT EXISTS ( + SELECT 1 FROM storage.folders p + WHERE p.id = fo.parent_id AND p.is_trashed = TRUE)); + +COMMENT ON VIEW storage.trash_items IS + 'Unified view of all trashed files and folders. `drive_id` added in D2b ' + 'so callers can scope by accessible drives (the legacy per-user scope ' + 'is being phased out alongside the user_id column in D7).'; diff --git a/src/application/dtos/trash_dto.rs b/src/application/dtos/trash_dto.rs index 2cbe2e60..d177e3e9 100644 --- a/src/application/dtos/trash_dto.rs +++ b/src/application/dtos/trash_dto.rs @@ -61,6 +61,11 @@ pub struct TrashResourceRow { pub resource_created_at: DateTime, pub modified_at: DateTime, pub owner_id: Uuid, + /// Drive the trashed item belongs to. Surfaced verbatim on the wire + /// (`TrashResourceItemDto.drive_id`) so the `/trash` UI can group by + /// drive without an extra lookup per row. D2b: filtering by drive is + /// done in SQL via `WHERE drive_id = ANY($accessible_drive_ids)`. + pub drive_id: Uuid, /// Raw BLAKE3 content hash. `Some(_)` for file rows, `None` for /// folder rows. Feeds `File::compute_etag` so the trash listing's /// `etag` matches what GET/HEAD/PROPFIND would return for the @@ -162,6 +167,12 @@ pub struct TrashResourceItemDto { pub trashed_at: DateTime, /// When the item will be permanently deleted by the retention sweeper. pub deletion_date: DateTime, + /// The drive the trashed item belongs to. Enables client-side + /// group-by-drive in the `/trash` UI (D2b spec — see + /// `project_trash_groupbys_d2b` memory). The drive's display name + /// resolves through the `/api/drives` listing the client already + /// holds in `drives.svelte` — no extra round-trip needed. + pub drive_id: Uuid, /// Full resource details — shape determined by `resource_type`. pub resource: ResourceContentDto, } diff --git a/src/application/services/trash_service.rs b/src/application/services/trash_service.rs index aa6a85d3..aa09b85b 100644 --- a/src/application/services/trash_service.rs +++ b/src/application/services/trash_service.rs @@ -19,6 +19,7 @@ use crate::application::ports::trash_ports::TrashUseCase; use crate::common::errors::{DomainError, ErrorKind, Result}; use crate::domain::entities::file::File; use crate::domain::entities::trashed_item::{TrashedItem, TrashedItemType}; +use crate::domain::repositories::drive_repository::DriveRepository; use crate::domain::repositories::folder_repository::FolderRepository; use crate::domain::repositories::trash_repository::TrashRepository; use crate::domain::services::authorization::ResourceKind; @@ -70,6 +71,11 @@ pub struct TrashService { /// Authz engine authz: Arc, + /// Drive repository — D2b uses it to resolve "drives the caller can read" + /// so trash listings filter by drive membership instead of the legacy + /// per-user scope. + drive_repo: Arc, + /// Number of days items should be kept in trash before automatic cleanup retention_days: u32, } @@ -85,6 +91,7 @@ impl TrashService { dedup_service: Arc, content_cache: Option>, authz: Arc, + drive_repo: Arc, ) -> Self { Self { trash_repository, @@ -95,6 +102,7 @@ impl TrashService { file_deleted_hook: None, content_cache, authz, + drive_repo, retention_days, } } @@ -366,10 +374,7 @@ impl TrashUseCase for TrashService { // Get the trash item info!("Retrieving trash item from repository: ID={}", trash_id); - let item_result = self - .trash_repository - .get_trash_item(&trash_uuid, &user_uuid) - .await; + let item_result = self.trash_repository.get_trash_item(&trash_uuid).await; match item_result { Ok(Some(item)) => { @@ -380,6 +385,21 @@ impl TrashUseCase for TrashService { item.original_id() ); + // D2b stage 3: gate the restore on Delete permission against + // the item's original resource. The drive precheck in + // `pg_acl_engine` resolves Owner-on-drive → Delete-permission, + // so a drive Owner can restore items they didn't originally + // trash (per `drive.md §12`). 404-on-deny via `authz.require` + // matches the lookup-NotFound shape. + let original = item.original_id(); + let resource = match item.item_type() { + TrashedItemType::File => Resource::File(original), + TrashedItemType::Folder => Resource::Folder(original), + }; + self.authz + .require(Subject::User(user_id), Permission::Delete, resource) + .await?; + // Restore based on type match item.item_type() { TrashedItemType::File => { @@ -540,10 +560,7 @@ impl TrashUseCase for TrashService { // Get the trash item info!("Retrieving trash item from repository: ID={}", trash_id); - let item_result = self - .trash_repository - .get_trash_item(&trash_uuid, &user_uuid) - .await; + let item_result = self.trash_repository.get_trash_item(&trash_uuid).await; match item_result { Ok(Some(item)) => { @@ -554,6 +571,19 @@ impl TrashUseCase for TrashService { item.original_id() ); + // D2b stage 3: gate the permanent-delete on Delete permission + // against the item's original resource (mirrors `restore_item` + // above). Drive Owners can hard-delete shared-drive items + // they didn't trash. + let original = item.original_id(); + let resource = match item.item_type() { + TrashedItemType::File => Resource::File(original), + TrashedItemType::Folder => Resource::Folder(original), + }; + self.authz + .require(Subject::User(user_id), Permission::Delete, resource) + .await?; + // Permanently delete based on type match item.item_type() { TrashedItemType::File => { @@ -688,6 +718,41 @@ impl TrashUseCase for TrashService { async fn empty_trash(&self, user_id: Uuid) -> Result<()> { info!("Emptying trash for user {}", user_id); + // D2b stage 3: resolve the set of drives the caller can permanently + // delete content in. "Empty trash" means "drop every trashed item + // I have Delete on" — Owner role's bundle includes Delete; Editor / + // Viewer / Contributor / Commenter do not. So this filter picks out + // the drives where the caller is effectively Owner (direct or via a + // group). Single-drive users: this resolves to just their personal + // drive, identical to the legacy `WHERE user_id = $1` scope. + let (subject_types, subject_ids) = self + .authz + .expand_subject_for_listing(Subject::User(user_id)) + .await?; + let drives = self + .drive_repo + .list_for_subjects(&subject_types, &subject_ids) + .await + .map_err(|e| { + DomainError::internal_error( + "Trash", + format!("Failed to resolve accessible drives: {e:?}"), + ) + })?; + let drive_ids: Vec = drives + .iter() + .filter(|d| { + d.caller_role + .is_some_and(|r| r.expand().contains(&Permission::Delete)) + }) + .map(|d| d.drive.id) + .collect(); + + if drive_ids.is_empty() { + info!("empty_trash: caller has Delete on no drive — nothing to do"); + return Ok(()); + } + // Collect ALL trashed file IDs BEFORE bulk-deleting so hooks (thumbnail // cleanup, etc.) can run afterward. We use get_all_trashed_file_ids (not // get_trash_items) because the trash_items view excludes files inside a @@ -696,7 +761,7 @@ impl TrashUseCase for TrashService { let trashed_file_ids: Vec = if self.file_deleted_hook.is_some() { match self .trash_repository - .get_all_trashed_file_ids(&user_id) + .get_all_trashed_file_ids(&drive_ids) .await { Ok(ids) => ids, @@ -709,16 +774,14 @@ impl TrashUseCase for TrashService { Vec::new() }; - // clear_trash() performs bulk SQL DELETEs in 2 queries: - // 1. DELETE FROM storage.files WHERE user_id = $1 AND is_trashed = TRUE - // 2. DELETE FROM storage.folders WHERE user_id = $1 AND is_trashed = TRUE + // clear_trash() performs bulk SQL DELETEs in 2 queries (post D2b stage 3): + // 1. DELETE FROM storage.files WHERE drive_id = ANY($1) AND is_trashed = TRUE + // 2. DELETE FROM storage.folders WHERE drive_id = ANY($1) AND is_trashed = TRUE // // Folder deletion cascades (FK ON DELETE CASCADE) to child folders and // their files. The PG trigger `trg_files_decrement_blob_ref` automatically // decrements blob ref_counts for every deleted file row. - // - // Finally it clears the trash_items index for the user. - self.trash_repository.clear_trash(&user_id).await?; + self.trash_repository.clear_trash(&drive_ids).await?; // The PG trigger decremented ref_counts but cannot delete disk files or // thumbnails. Run garbage_collect() to remove any blobs whose ref_count @@ -766,11 +829,32 @@ impl TrashService { kinds: Option<&[ResourceKind]>, reverse: bool, ) -> Result<(Vec, Option)> { + // D2b: scope by drives the caller can read (resolved through + // role_grants on resource_type='drive', including group-mediated + // grants). Empty set → empty page without a SQL round-trip. + let (subject_types, subject_ids) = self + .authz + .expand_subject_for_listing(Subject::User(user_id)) + .await?; + let drive_ids: Vec = match self + .drive_repo + .list_for_subjects(&subject_types, &subject_ids) + .await + { + Ok(drives) => drives.into_iter().map(|d| d.drive.id).collect(), + Err(e) => { + return Err(DomainError::internal_error( + "Trash", + format!("Failed to resolve accessible drives: {e:?}"), + )); + } + }; + // Fetch one extra row to detect whether a next page exists. let mut rows = self .trash_repository .list_resources_paged( - user_id, + &drive_ids, limit + 1, cursor.as_ref(), order_by, @@ -823,10 +907,10 @@ fn row_to_item_dto(row: TrashResourceRow) -> TrashResourceItemDto { path, parent_id: row.parent_id.map(|u| u.to_string()), owner_id: Some(row.owner_id.to_string()), - // Trash listing — drive_id is informational and the trash - // row doesn't currently SELECT it. Path-based lookups - // never enter this code path. - drive_id: uuid::Uuid::nil(), + // D2b: the trash listing query now SELECTs `drive_id` (the + // unified view exposes it). Surfaced so per-drive grouping + // in the `/trash` UI doesn't need an extra lookup per row. + drive_id: row.drive_id, created_at: row.resource_created_at.timestamp() as u64, modified_at: row.modified_at.timestamp() as u64, is_root: false, @@ -841,6 +925,7 @@ fn row_to_item_dto(row: TrashResourceRow) -> TrashResourceItemDto { resource_type: ResourceTypeDto::Folder, trashed_at: row.trashed_at, deletion_date: row.deletion_date, + drive_id: row.drive_id, resource: ResourceContentDto::Folder(dto), } } else { @@ -884,6 +969,7 @@ fn row_to_item_dto(row: TrashResourceRow) -> TrashResourceItemDto { resource_type: ResourceTypeDto::File, trashed_at: row.trashed_at, deletion_date: row.deletion_date, + drive_id: row.drive_id, resource: ResourceContentDto::File(dto), } } diff --git a/src/application/services/trash_service_test.rs b/src/application/services/trash_service_test.rs index 74621d9b..f5801076 100644 --- a/src/application/services/trash_service_test.rs +++ b/src/application/services/trash_service_test.rs @@ -203,10 +203,7 @@ where let trash_uuid = Uuid::parse_str(trash_id) .map_err(|e| DomainError::validation_error(format!("Invalid trash ID: {}", e)))?; - let item = self - .trash_repository - .get_trash_item(&trash_uuid, &user_id) - .await?; + let item = self.trash_repository.get_trash_item(&trash_uuid).await?; match item { Some(item) => { match item.item_type() { @@ -265,10 +262,7 @@ where let trash_uuid = Uuid::parse_str(trash_id) .map_err(|e| DomainError::validation_error(format!("Invalid trash ID: {}", e)))?; - let item = self - .trash_repository - .get_trash_item(&trash_uuid, &user_id) - .await?; + let item = self.trash_repository.get_trash_item(&trash_uuid).await?; match item { Some(item) => { match item.item_type() { @@ -319,7 +313,11 @@ where } async fn empty_trash(&self, user_id: Uuid) -> Result<()> { - self.trash_repository.clear_trash(&user_id).await + // Test mock — treats `user_id` as a stand-in for the single accessible + // drive (the mock storage is single-user / single-drive). The + // production `TrashService::empty_trash` resolves the real drive set + // via `drive_repo.list_for_subjects` + role-bundle filter. + self.trash_repository.clear_trash(&[user_id]).await } } @@ -364,13 +362,11 @@ impl TrashRepository for MockTrashRepository { Ok(user_items) } - async fn get_trash_item(&self, id: &Uuid, user_id: &Uuid) -> Result> { + async fn get_trash_item(&self, id: &Uuid) -> Result> { + // Mock mirrors the production repo's no-filter lookup — the + // user-scoped check has moved into the service's `authz.require`. let items = self.trash_items.lock().unwrap(); - let item = items - .get(id) - .filter(|item| item.user_id() == *user_id) - .cloned(); - Ok(item) + Ok(items.get(id).cloned()) } async fn restore_from_trash(&self, id: &Uuid, user_id: &Uuid) -> Result<()> { @@ -393,16 +389,19 @@ impl TrashRepository for MockTrashRepository { Ok(()) } - async fn clear_trash(&self, user_id: &Uuid) -> Result<()> { + async fn clear_trash(&self, drive_ids: &[Uuid]) -> Result<()> { + // Test mock: the single-user/single-drive mock uses `user_id` as + // the drive proxy (see `empty_trash` in the service mock). Filter + // trash items whose owner is in the passed-in set. let mut items = self.trash_items.lock().unwrap(); - items.retain(|_, item| item.user_id() != *user_id); + items.retain(|_, item| !drive_ids.contains(&item.user_id())); // Simulate PG CASCADE: clear trashed file/folder storage too self.trashed_files.lock().unwrap().clear(); self.trashed_folders.lock().unwrap().clear(); Ok(()) } - async fn get_all_trashed_file_ids(&self, _user_id: &Uuid) -> Result> { + async fn get_all_trashed_file_ids(&self, _drive_ids: &[Uuid]) -> Result> { let files = self.trashed_files.lock().unwrap(); Ok(files.keys().cloned().collect()) } diff --git a/src/common/di.rs b/src/common/di.rs index 660ecabb..b078b1f5 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -768,6 +768,7 @@ impl AppServiceFactory { repos: &RepositoryServices, core: &CoreServices, authz: &Arc, + drive_repo: &Arc, ) -> Option> { if !self.config.features.enable_trash { tracing::info!("Trash service is disabled in configuration"); @@ -787,6 +788,7 @@ impl AppServiceFactory { core.dedup_service.clone(), Some(core.file_content_cache.clone()), authz.clone(), + drive_repo.clone(), ) .with_file_deleted_hook(core.file_lifecycle.clone()), ); @@ -1137,7 +1139,7 @@ impl AppServiceFactory { // 3b. Trash service (needed before application services) let trash_service = self - .create_trash_service(&repos, &core, &authorization) + .create_trash_service(&repos, &core, &authorization, &drive_repo) .await; // 3c. Storage usage / quota service (needed by the instant-upload diff --git a/src/domain/repositories/trash_repository.rs b/src/domain/repositories/trash_repository.rs index 3a1cee2c..ee6a233f 100644 --- a/src/domain/repositories/trash_repository.rs +++ b/src/domain/repositories/trash_repository.rs @@ -6,15 +6,37 @@ use crate::domain::entities::trashed_item::TrashedItem; pub trait TrashRepository: Send + Sync { async fn add_to_trash(&self, item: &TrashedItem) -> Result<()>; async fn get_trash_items(&self, user_id: &Uuid) -> Result>; - async fn get_trash_item(&self, id: &Uuid, user_id: &Uuid) -> Result>; + /// Fetch a trashed item by its trash-row id. + /// + /// **Caller contract**: this method does NO authorization check. Callers + /// MUST follow with + /// `authz.require(Permission::Delete, Resource::File|Folder(item.original_id()))` + /// before acting on the result. The trash service's `restore_item` and + /// `delete_permanently` are the canonical examples. + /// + /// **Implementor contract**: do NOT re-introduce a `user_id` (or + /// `drive_id`) scope filter at the SQL layer. Authorization is the + /// service's job, not the repository's — adding a scope here would + /// silently break drive-Owner-restores-another-user's-trashed-item + /// (the canonical D2 use case). A direct-by-id lookup is the + /// intended shape. + async fn get_trash_item(&self, id: &Uuid) -> Result>; async fn restore_from_trash(&self, id: &Uuid, user_id: &Uuid) -> Result<()>; async fn delete_permanently(&self, id: &Uuid, user_id: &Uuid) -> Result<()>; - async fn clear_trash(&self, user_id: &Uuid) -> Result<()>; + /// Bulk-delete all trashed files and folders in the given drives. + /// + /// **Caller contract**: pass only drive UUIDs the caller has + /// `Permission::Delete` on (resolved by the service via + /// `DriveRepository::list_for_subjects` + role-bundle filter). This + /// repository performs no authorization — see + /// `TrashService::empty_trash` for the canonical call site. + async fn clear_trash(&self, drive_ids: &[Uuid]) -> Result<()>; - /// All trashed file IDs for this user, regardless of parent folder trash status. - /// Used by empty_trash for thumbnail cleanup — the view used by get_trash_items - /// excludes files inside trashed folders, which would miss their ext thumbnails. - async fn get_all_trashed_file_ids(&self, user_id: &Uuid) -> Result>; + /// All trashed file IDs across the given drives, regardless of parent + /// folder trash status. Used by `empty_trash` for thumbnail cleanup — + /// the trash_items view excludes files inside trashed folders, which + /// would miss their thumbnails. Same caller contract as `clear_trash`. + async fn get_all_trashed_file_ids(&self, drive_ids: &[Uuid]) -> Result>; /// Bulk-delete all expired trash items (files + folders) in a single /// transaction. Returns `(files_deleted, folders_deleted)`. diff --git a/src/infrastructure/repositories/pg/trash_db_repository.rs b/src/infrastructure/repositories/pg/trash_db_repository.rs index 3f0eadc1..db92f0b0 100644 --- a/src/infrastructure/repositories/pg/trash_db_repository.rs +++ b/src/infrastructure/repositories/pg/trash_db_repository.rs @@ -147,18 +147,22 @@ impl TrashRepository for TrashDbRepository { .collect()) } - async fn get_trash_item(&self, id: &Uuid, user_id: &Uuid) -> Result> { + async fn get_trash_item(&self, id: &Uuid) -> Result> { + // D2b stage 3: lookup by id only — the `user_id` filter that used to + // gate this query is replaced by an explicit `authz.require(Delete, + // …)` in the service callers (`restore_item`, `delete_permanently`). + // The drive precheck in `pg_acl_engine` then resolves Owner-on-drive + // → Delete-permission for items in shared drives. let row = sqlx::query_as::<_, (Uuid, String, String, Uuid, Option>, String)>( r#" SELECT t.id, t.name, t.item_type, t.user_id, t.trashed_at, COALESCE(p.path || '/' || t.name, t.name) AS original_path FROM storage.trash_items t LEFT JOIN storage.folders p ON p.id = t.original_parent_id - WHERE t.id = $1 AND t.user_id = $2 + WHERE t.id = $1 "#, ) .bind(id) - .bind(user_id) .fetch_optional(self.pool.as_ref()) .await .map_err(|e| DomainError::internal_error("TrashDb", format!("get: {e}")))?; @@ -182,17 +186,22 @@ impl TrashRepository for TrashDbRepository { Ok(()) } - async fn clear_trash(&self, user_id: &Uuid) -> Result<()> { - // Delete all trashed files for this user - sqlx::query("DELETE FROM storage.files WHERE user_id = $1 AND is_trashed = TRUE") - .bind(user_id) + async fn clear_trash(&self, drive_ids: &[Uuid]) -> Result<()> { + if drive_ids.is_empty() { + return Ok(()); + } + // Delete all trashed files in the given drives. + sqlx::query("DELETE FROM storage.files WHERE drive_id = ANY($1) AND is_trashed = TRUE") + .bind(drive_ids) .execute(self.pool.as_ref()) .await .map_err(|e| DomainError::internal_error("TrashDb", format!("clear files: {e}")))?; - // Delete all trashed folders for this user - sqlx::query("DELETE FROM storage.folders WHERE user_id = $1 AND is_trashed = TRUE") - .bind(user_id) + // Delete all trashed folders in the given drives. FK ON DELETE CASCADE + // sweeps descendant rows; the `trg_files_decrement_blob_ref` trigger + // handles blob refcount drops automatically. + sqlx::query("DELETE FROM storage.folders WHERE drive_id = ANY($1) AND is_trashed = TRUE") + .bind(drive_ids) .execute(self.pool.as_ref()) .await .map_err(|e| DomainError::internal_error("TrashDb", format!("clear folders: {e}")))?; @@ -200,11 +209,14 @@ impl TrashRepository for TrashDbRepository { Ok(()) } - async fn get_all_trashed_file_ids(&self, user_id: &Uuid) -> Result> { + async fn get_all_trashed_file_ids(&self, drive_ids: &[Uuid]) -> Result> { + if drive_ids.is_empty() { + return Ok(Vec::new()); + } let rows = sqlx::query_scalar::<_, String>( - "SELECT id::text FROM storage.files WHERE user_id = $1 AND is_trashed = TRUE", + "SELECT id::text FROM storage.files WHERE drive_id = ANY($1) AND is_trashed = TRUE", ) - .bind(user_id) + .bind(drive_ids) .fetch_all(self.pool.as_ref()) .await .map_err(|e| DomainError::internal_error("TrashDb", format!("all_trashed_files: {e}")))?; @@ -253,23 +265,34 @@ impl TrashRepository for TrashDbRepository { // Cursor-paginated trash listing (used by GET /api/trash/resources) // ════════════════════════════════════════════════════════════════════════════ impl TrashDbRepository { - /// Cursor-paginated list of the user's trashed resources. + /// Cursor-paginated list of trashed resources the caller can read. + /// + /// D2b: scope is drive-membership-based — pass the set of drive UUIDs + /// the caller can read (resolved upstream by `DriveRepository::list_for_subjects` + /// or equivalent). Items in drives outside this set drop out at the + /// WHERE clause. The legacy `WHERE user_id = $1::uuid` filter is gone; + /// for single-drive users this returns exactly the same items as before + /// because the caller's own personal drive is always in `drive_ids`. /// /// Mirrors the favorites/grants pattern: a UNION-ALL CTE over folder and /// file branches (each pre-computing sort columns), then a per-dimension /// keyset WHERE + ORDER BY. /// /// Returns rows in caller-requested sort order. The caller is expected to - /// fetch `limit + 1` to detect end-of-results. + /// fetch `limit + 1` to detect end-of-results. Empty `drive_ids` returns + /// an empty page without hitting PG. pub async fn list_resources_paged( &self, - user_id: Uuid, + drive_ids: &[Uuid], limit: usize, cursor: Option<&TrashCursor>, order_by: &str, kinds: Option<&[ResourceKind]>, reverse: bool, ) -> Result> { + if drive_ids.is_empty() { + return Ok(Vec::new()); + } let include_folders = kinds.is_none_or(|k| k.iter().any(|r| matches!(r, ResourceKind::Folder))); let include_files = kinds.is_none_or(|k| k.iter().any(|r| matches!(r, ResourceKind::File))); @@ -291,6 +314,7 @@ impl TrashDbRepository { fld.created_at AS resource_created_at, fld.updated_at AS modified_at, fld.user_id AS owner_id, + fld.drive_id AS drive_id, NULL::text AS blob_hash, fld.trashed_at AS trashed_at, (fld.trashed_at + ($7::int * INTERVAL '1 day')) AS deletion_date, @@ -299,7 +323,7 @@ impl TrashDbRepository { 0::bigint AS type_order, 0::int AS folder_first FROM storage.folders fld - WHERE fld.user_id = $1::uuid + WHERE fld.drive_id = ANY($1) AND fld.is_trashed = TRUE AND (fld.parent_id IS NULL OR NOT EXISTS ( @@ -317,6 +341,7 @@ impl TrashDbRepository { f.created_at AS resource_created_at, f.updated_at AS modified_at, f.user_id AS owner_id, + f.drive_id AS drive_id, f.blob_hash, f.trashed_at AS trashed_at, (f.trashed_at + ($7::int * INTERVAL '1 day')) AS deletion_date, @@ -327,7 +352,7 @@ impl TrashDbRepository { FROM storage.files f LEFT JOIN storage.folders pfld ON pfld.id = f.folder_id - WHERE f.user_id = $1::uuid + WHERE f.drive_id = ANY($1) AND f.is_trashed = TRUE AND (f.folder_id IS NULL OR NOT EXISTS ( @@ -447,7 +472,7 @@ impl TrashDbRepository { SELECT r.resource_type, r.resource_id, r.name, r.parent_id, r.mime_type, r.size, r.resource_created_at, r.modified_at, - r.owner_id, r.trashed_at, r.deletion_date, r.resource_path, + r.owner_id, r.drive_id, r.trashed_at, r.deletion_date, r.resource_path, r.sort_str, r.type_order, r.folder_first FROM resources r {keyset} @@ -456,7 +481,7 @@ LIMIT $6" ); let rows = sqlx::query(&sql) - .bind(user_id) // $1 + .bind(drive_ids) // $1 (was user_id pre-D2b) .bind(cur_str) // $2 .bind(cur_int) // $3 .bind(cur_ts) // $4 @@ -505,6 +530,7 @@ LIMIT $6" resource_created_at: row.get("resource_created_at"), modified_at: row.get("modified_at"), owner_id: row.get("owner_id"), + drive_id: row.get("drive_id"), blob_hash: row.try_get("blob_hash").ok(), trashed_at, deletion_date, diff --git a/tests/api/trash_resources.hurl b/tests/api/trash_resources.hurl index a7721895..659f396e 100644 --- a/tests/api/trash_resources.hurl +++ b/tests/api/trash_resources.hurl @@ -153,6 +153,10 @@ HTTP 200 # ───────────────────────────────────────────────────────────── # Step 7 – Default listing: 4 items, all carry trashed_at + deletion_date +# + drive_id (D2b — enables per-drive grouping in the UI). +# The listing is also drive-membership scoped (D2b stage 2): the +# admin sees their own personal drive's trashed items, just as +# before, because a personal drive Owner has all roles needed. # ───────────────────────────────────────────────────────────── GET {{base_url}}/api/trash/resources Authorization: Bearer {{token}} @@ -162,6 +166,11 @@ HTTP 200 jsonpath "$.items" count == 4 jsonpath "$.items[0].trashed_at" isString jsonpath "$.items[0].deletion_date" isString +# D2b: drive_id is non-null on every trashed item. +jsonpath "$.items[0].drive_id" isString +jsonpath "$.items[1].drive_id" isString +jsonpath "$.items[2].drive_id" isString +jsonpath "$.items[3].drive_id" isString jsonpath "$.items[0].resource_type" matches "^(file|folder)$" jsonpath "$.items[*].resource.id" contains {{folder_a_id}} jsonpath "$.items[*].resource.id" contains {{folder_b_id}}