diff --git a/frontend/src/lib/api/types.ts b/frontend/src/lib/api/types.ts index 1eed62c0..83f7b481 100644 --- a/frontend/src/lib/api/types.ts +++ b/frontend/src/lib/api/types.ts @@ -24,7 +24,12 @@ export interface FolderItem { is_root: boolean; modified_at: number; name: string; - owner_id: string; + // `null` on share-recipient responses — the backend's + // `FolderDto::without_hierarchy_info` (folder_dto.rs:154) clears + // hierarchy fields (including owner_id) for non-owner callers. + // Backend serialises `Option` (folder_dto.rs:53); this + // type just tells the truth about the wire. + owner_id: string | null; parent_id: string | null; path: string; etag: string; @@ -39,7 +44,9 @@ export interface FileItem { mime_type: string; modified_at: number; name: string; - owner_id: string; + // `null` on share-recipient responses (same as FolderItem above). + // Backend serialises `Option` at file_dto.rs:59. + owner_id: string | null; folder_id: string; path: string; size: number; @@ -249,12 +256,16 @@ export interface Drive { } /** - * Typed mirror of the five known D5 policy keys. Every field defaults to - * `false` (= allowed). The wire shape returned by - * `PATCH /api/drives/{id}/policies` carries all five keys; the request - * body uses [`DrivePoliciesPartial`] so unsupplied keys aren't disturbed. + * Typed mirror of the known drive policy keys. Every field defaults to + * `false` (= "opted out" for the `include_in_*` keys, "allowed" for the + * `forbid_*` keys). The wire shape returned by + * `PATCH /api/drives/{id}/policies` carries every known key; the request + * body uses [`DrivePoliciesPartial`] so unsupplied keys aren't disturbed + * (the backend uses a JSONB `||` merge — see + * `drive_pg_repository.rs::update_policies`). * - * See `docs/plan/drive.md` §8 for what each key gates. + * See `docs/plan/drive.md` §8 for the `forbid_*` gates and §15 for the + * `include_in_*_index` scope flags. */ export interface DrivePolicies { forbid_sharing: boolean; @@ -262,6 +273,18 @@ export interface DrivePolicies { forbid_public_links: boolean; forbid_cross_drive_move: boolean; forbid_owner_role_change: boolean; + /** + * §15 opt-in for `/api/photos` timeline scope. Default personal drives + * are created with `true`; non-default drives (secondary personals, + * shared) start `false` and opt in via the admin policy modal. + */ + include_in_photo_index: boolean; + /** + * §15 opt-in for the Music library surface (currently playlists; + * future `/api/music/tracks` library view will read this too). + * Symmetric shape to `include_in_photo_index`. + */ + include_in_music_index: boolean; } /** diff --git a/frontend/src/routes/admin/+page.svelte b/frontend/src/routes/admin/+page.svelte index 510a9f04..62ef4263 100644 --- a/frontend/src/routes/admin/+page.svelte +++ b/frontend/src/routes/admin/+page.svelte @@ -1080,7 +1080,13 @@ forbid_external_sharing: false, forbid_public_links: false, forbid_cross_drive_move: false, - forbid_owner_role_change: false + forbid_owner_role_change: false, + // §15 opt-in scope flags. Default personal drives ship with `true` + // on the wire (materialised by the DB-side create path + backfill + // migration), so `readPolicyBool` will surface the correct current + // state on modal open. + include_in_photo_index: false, + include_in_music_index: false }); let managePoliciesError = $state(null); let managePoliciesBusy = $state(false); @@ -1102,7 +1108,9 @@ forbid_external_sharing: readPolicyBool(p, 'forbid_external_sharing'), forbid_public_links: readPolicyBool(p, 'forbid_public_links'), forbid_cross_drive_move: readPolicyBool(p, 'forbid_cross_drive_move'), - forbid_owner_role_change: readPolicyBool(p, 'forbid_owner_role_change') + forbid_owner_role_change: readPolicyBool(p, 'forbid_owner_role_change'), + include_in_photo_index: readPolicyBool(p, 'include_in_photo_index'), + include_in_music_index: readPolicyBool(p, 'include_in_music_index') }; } @@ -1207,6 +1215,24 @@ 'admin.drive_policy.forbid_owner_role_change_help', 'Only admin can add, remove, or demote drive Owners while this is on.' ) + }, + { + key: 'include_in_photo_index', + label: () => t('admin.drive_policy.include_in_photo_index', 'Include in Photos'), + help: () => + t( + 'admin.drive_policy.include_in_photo_index_help', + 'Show image and video files from this drive in the Photos timeline and on the Places map. Default personal drives are opted in automatically; turn on for shared drives that genuinely hold photos (e.g. "Family Photos").' + ) + }, + { + key: 'include_in_music_index', + label: () => t('admin.drive_policy.include_in_music_index', 'Include in Music'), + help: () => + t( + 'admin.drive_policy.include_in_music_index_help', + 'Include audio files from this drive in the Music library. Default personal drives are opted in automatically; turn on for shared drives that genuinely hold a music collection (e.g. "Family Music", "Band Collaboration").' + ) } ]; diff --git a/migrations/20260901000000_default_personal_photo_music_flags.sql b/migrations/20260901000000_default_personal_photo_music_flags.sql new file mode 100644 index 00000000..b417f0d9 --- /dev/null +++ b/migrations/20260901000000_default_personal_photo_music_flags.sql @@ -0,0 +1,32 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- PR-A / §15 — default personal drives get include_in_photo_index + +-- include_in_music_index materialised on the JSONB `policies` bag +-- ════════════════════════════════════════════════════════════════════════════ +-- `docs/plan/drive.md` §15 locks the two policies as symmetric per-drive +-- opt-in flags. The default personal drive is always in scope for Photos + +-- Music, so we materialise both flags = `true` on every default personal +-- drive rather than carving out a `default_for_user IS NOT NULL` OR-branch +-- in the query predicate. Net effect: the SQL predicate is a single positive +-- rule keyed off the JSONB flag alone (see `list_media_files` after the +-- companion Rust rewrite). +-- +-- New default personal drives get these flags at creation time via +-- `DriveRepository::create_personal_drive_atomic` (the INSERT literal on +-- that path was updated alongside this migration). This migration handles +-- the existing rows, seeded by the D0 backfill. +-- +-- Non-default drives (secondary personals, shared drives) are NOT touched — +-- they stay opted-out until the owner flips the flag via the admin +-- "Manage policies" modal. +-- +-- Idempotent: `policies || {…}` is a no-op if the keys are already set to +-- the same values, and JSONB `||` is right-precedence so the migration +-- never overwrites an owner's explicit opt-out that was already recorded. +-- (If someone had `include_in_photo_index=false` set on their default +-- personal via a manual PATCH, this UPDATE would still overwrite to true; +-- that's acceptable — the D5 policy UI didn't exist for these flags +-- before this PR, so no such manual opt-out can be in the wild yet.) + +UPDATE storage.drives + SET policies = policies || '{"include_in_photo_index": true, "include_in_music_index": true}'::jsonb + WHERE default_for_user IS NOT NULL; diff --git a/migrations/20260901000001_files_media_timeline_by_drive_index.sql b/migrations/20260901000001_files_media_timeline_by_drive_index.sql new file mode 100644 index 00000000..9ca103d9 --- /dev/null +++ b/migrations/20260901000001_files_media_timeline_by_drive_index.sql @@ -0,0 +1,25 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- PR-A / §15 — partial covering index for the drive-scoped Photos timeline +-- ════════════════════════════════════════════════════════════════════════════ +-- Sibling of `idx_files_media_timeline` (initial_schema.sql:581), keyed on +-- `drive_id` instead of `user_id`. The Photos handler predicate is being +-- rewritten to `fi.drive_id IN (drives with include_in_photo_index = true +-- AND caller has Read)` — that subquery produces a small drive-id set, +-- and this index gives Postgres one IndexScan per drive_id already +-- ordered by `media_sort_date DESC`, so LIMIT stops the scan early. +-- Same O(LIMIT) shape as the pre-D7 user_id-keyed hot path. +-- +-- The old `idx_files_media_timeline (user_id, media_sort_date DESC)` index +-- is intentionally kept for now — it still backs the dedup / storage sweep +-- paths that D7 will migrate separately. Once D7 drops the `user_id` +-- column those paths lose their backing index at the same moment; that PR +-- can drop the old index in the same migration. +-- +-- Partial WHERE clause is identical to the existing sibling so the index +-- stays as compact as its predecessor: only image/video rows that aren't +-- trashed. + +CREATE INDEX IF NOT EXISTS idx_files_media_timeline_by_drive + ON storage.files (drive_id, media_sort_date DESC) + WHERE NOT is_trashed + AND (mime_type LIKE 'image/%' OR mime_type LIKE 'video/%'); diff --git a/src/application/services/drive_management_service.rs b/src/application/services/drive_management_service.rs index b1cad79c..5d472d04 100644 --- a/src/application/services/drive_management_service.rs +++ b/src/application/services/drive_management_service.rs @@ -448,7 +448,7 @@ impl DriveManagementService { &self, caller_id: Uuid, drive_id: Uuid, - partial: crate::domain::entities::drive::DrivePolicies, + partial: serde_json::Value, ) -> Result { let merged = self .drive_repo @@ -474,6 +474,8 @@ impl DriveManagementService { forbid_public_links = merged.forbid_public_links, forbid_cross_drive_move = merged.forbid_cross_drive_move, forbid_owner_role_change = merged.forbid_owner_role_change, + include_in_photo_index = merged.include_in_photo_index, + include_in_music_index = merged.include_in_music_index, "📜 drive policies updated", ); Ok(merged) diff --git a/src/application/services/places_service.rs b/src/application/services/places_service.rs index 6f0f42ad..79314a0b 100644 --- a/src/application/services/places_service.rs +++ b/src/application/services/places_service.rs @@ -4,22 +4,29 @@ use uuid::Uuid; use crate::application::dtos::geo_dto::{GeoBounds, GeoCluster}; use crate::common::errors::DomainError; +use crate::domain::services::authorization::Subject; use crate::infrastructure::repositories::pg::FileBlobReadRepository; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; /// "Places" use case: the caller's geotagged photos aggregated into map /// clusters. /// -/// Strictly user-scoped — the repository filters `WHERE fi.user_id = $1`, so, -/// like [`RecentService`](super::recent_service::RecentService) and the photos -/// timeline, it needs no `AuthorizationEngine` check: the `caller_id` -/// parameter *is* the access scope. +/// Post-§15 the surface follows the Photos scope: default personal drive +/// + drives where `policies.include_in_photo_index = true` AND caller +/// has Read. The repository query joins `role_grants` on the drive +/// resource type; group-mediated grants are honoured via the caller +/// expansion done here. pub struct PlacesService { file_read: Arc, + authorization: Arc, } impl PlacesService { - pub fn new(file_read: Arc) -> Self { - Self { file_read } + pub fn new(file_read: Arc, authorization: Arc) -> Self { + Self { + file_read, + authorization, + } } /// Aggregation cell side, in degrees, for a slippy-map zoom level. The @@ -30,7 +37,8 @@ impl PlacesService { 360.0 / (2_f64.powi(z) * 4.0) } - /// Clustered geotagged photos for `caller_id` within `bounds`. + /// Clustered geotagged photos in the caller's Photos-scope drive set, + /// within `bounds`. pub async fn clusters( &self, caller_id: Uuid, @@ -38,8 +46,12 @@ impl PlacesService { zoom: u8, ) -> Result, DomainError> { let cell = Self::cell_for_zoom(zoom); + let (subject_types, subject_ids) = self + .authorization + .expand_subject_for_listing(Subject::User(caller_id)) + .await?; self.file_read - .list_geo_clusters(caller_id, bounds, cell) + .list_geo_clusters(&subject_types, &subject_ids, bounds, cell) .await } } diff --git a/src/common/di.rs b/src/common/di.rs index 5600aa41..ce6ddf65 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -912,12 +912,20 @@ impl AppServiceFactory { } /// Creates the Places (photo map) service. Reuses the existing file-read - /// repository — the data is the caller's own geotagged photos. + /// repository — the data is the caller's Photos-scope geotagged photos + /// (§15: default personal drive + drives with + /// `include_in_photo_index = true` AND caller has Read). + /// `authorization` is used for the same subject expansion that + /// `photos_handler::list_photos` runs. pub fn create_places_service( &self, file_read: &Arc, + authorization: &Arc, ) -> Arc { - let service = Arc::new(PlacesService::new(file_read.clone())); + let service = Arc::new(PlacesService::new( + file_read.clone(), + authorization.clone(), + )); tracing::info!("Places service initialized"); service } @@ -1285,7 +1293,7 @@ impl AppServiceFactory { apps.recent_service = Some(recent_service_eager.clone()); places_service = if core.config.features.enable_places { - Some(self.create_places_service(&repos.file_read_repository)) + Some(self.create_places_service(&repos.file_read_repository, &authorization)) } else { None }; diff --git a/src/domain/entities/drive.rs b/src/domain/entities/drive.rs index 685804c0..7d1f562b 100644 --- a/src/domain/entities/drive.rs +++ b/src/domain/entities/drive.rs @@ -181,6 +181,26 @@ pub struct DrivePolicies { /// writes) and `::remove_member` (refuses Owner removals) when the /// caller is non-admin. pub forbid_owner_role_change: bool, + /// Opts this drive into the `/api/photos` timeline (§15). Non-default + /// drives are omitted by default so a random shared folder full of + /// screenshots doesn't bleed into the personal timeline; owners flip + /// this on when the drive genuinely is a photo library (e.g. "Family + /// Photos"). Default personal drives get `true` on creation via the + /// `PersonalDriveLifecycleHook` + a one-shot backfill for existing + /// rows, so the SQL predicate is a single positive rule with no + /// per-kind carve-out. Read at `file_blob_read_repository:: + /// list_media_files` + `list_geo_clusters`. See §15 for the query + /// shape and rationale. + pub include_in_photo_index: bool, + /// Same shape as `include_in_photo_index`, applied to the Music + /// library surface (playlists today; a `/api/music/tracks` library + /// view later). Symmetric opt-in — Music was originally cross-drive + /// via a `forbid_music_index` opt-out, but that mixed-form naming + /// created "one include-in, one forbid" confusion and the + /// "shared audio is always intentional" claim didn't hold under + /// scrutiny (voicemail MP3s in a work drive shouldn't bleed into + /// the personal library). See §15. + pub include_in_music_index: bool, } impl DrivePolicies { diff --git a/src/domain/repositories/drive_repository.rs b/src/domain/repositories/drive_repository.rs index 532925ee..67c449cb 100644 --- a/src/domain/repositories/drive_repository.rs +++ b/src/domain/repositories/drive_repository.rs @@ -256,10 +256,18 @@ pub trait DriveRepository: Send + Sync + 'static { /// /// Caller is responsible for the `Manage` permission check; this /// method does not re-verify. + /// + /// `partial` is a raw JSON object carrying **only** the keys the + /// caller wants to change — the repo passes it verbatim to the + /// `policies || $partial` JSONB merge. Using the typed + /// `DrivePolicies` here would serialise every field (including + /// unset ones as `false`) and clobber other flags on the row; + /// keeping the merge on the raw `Value` preserves the + /// partial-update semantic the handler documents. async fn update_policies( &self, drive_id: Uuid, - partial: &crate::domain::entities::drive::DrivePolicies, + partial: &serde_json::Value, ) -> Result; } diff --git a/src/infrastructure/repositories/pg/drive_pg_repository.rs b/src/infrastructure/repositories/pg/drive_pg_repository.rs index 855df271..dffdbd6d 100644 --- a/src/infrastructure/repositories/pg/drive_pg_repository.rs +++ b/src/infrastructure/repositories/pg/drive_pg_repository.rs @@ -116,11 +116,22 @@ impl DriveRepository for DrivePgRepository { .map_err(|e| Self::map_sqlx_err("create_personal_drive_atomic.begin", e))?; // 1. Drive row (root_folder_id NULL — populated in step 3). + // + // Default personal drives are seeded with `include_in_photo_index` + // + `include_in_music_index` = true so the Photos / Music + // predicates (§15) can be a single positive rule keyed off the + // JSONB flag — no per-kind carve-out needed at query time. Any + // future admin PATCH toggling either flag off shows a confirm + // dialog in the UI (unusual action; empties the user's Photos + // timeline / Music library). let drive_id: Uuid = sqlx::query_scalar( r#" INSERT INTO storage.drives (kind, default_for_user, quota_bytes, policies) - VALUES ('personal', $1, $2, '{}'::jsonb) + VALUES ( + 'personal', $1, $2, + '{"include_in_photo_index": true, "include_in_music_index": true}'::jsonb + ) RETURNING id "#, ) @@ -636,16 +647,17 @@ impl DriveRepository for DrivePgRepository { async fn update_policies( &self, drive_id: Uuid, - partial: &crate::domain::entities::drive::DrivePolicies, + partial: &serde_json::Value, ) -> Result { // JSONB-level merge (`||`) keeps unknown keys already on disk — // the column remains the canonical bag (see // `DrivePolicies::from_value` — typed read is lenient, untyped - // write is preserving). RETURNING surfaces the post-merge bag so - // the audit log shows what the row actually carries afterwards. - let partial_json = serde_json::to_value(partial).map_err(|e| { - DriveRepositoryError::StorageError(format!("serialise partial policies: {e}")) - })?; + // write is preserving). The caller passes a raw `Value` with + // ONLY the keys it wants to change (never a full `DrivePolicies` + // round-trip, which would serialise all-false defaults into the + // merge and clobber other flags). RETURNING surfaces the + // post-merge bag so the audit log shows what the row actually + // carries afterwards. let row: Option<(serde_json::Value,)> = sqlx::query_as( "UPDATE storage.drives \ SET policies = policies || $2, \ @@ -654,7 +666,7 @@ impl DriveRepository for DrivePgRepository { RETURNING policies", ) .bind(drive_id) - .bind(&partial_json) + .bind(partial) .fetch_optional(self.pool.as_ref()) .await .map_err(|e| Self::map_sqlx_err("update_policies", e))?; diff --git a/src/infrastructure/repositories/pg/file_blob_read_repository.rs b/src/infrastructure/repositories/pg/file_blob_read_repository.rs index af31116f..89a4757e 100644 --- a/src/infrastructure/repositories/pg/file_blob_read_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_read_repository.rs @@ -445,15 +445,36 @@ impl FileBlobReadRepository { /// /// Uses the denormalised `media_sort_date` column (synced from /// `file_metadata.captured_at` by trigger) so no JOIN with - /// `file_metadata` is needed. The partial index - /// `idx_files_media_timeline` covers the full query: filter + ORDER BY - /// in a single Index Scan — O(LIMIT) not O(N). + /// `file_metadata` is needed. The partial covering index + /// `idx_files_media_timeline_by_drive` (migration 20260901000001) + /// keys on `(drive_id, media_sort_date DESC)` filtered on non-trashed + /// image/video rows — Postgres does one IndexScan per in-scope + /// drive_id already ordered by capture date, so LIMIT stops the scan + /// early. Same O(LIMIT) shape as the pre-D7 `user_id`-keyed hot path. + /// + /// Scope (`docs/plan/drive.md` §15): the caller's *effective subjects* + /// × drives with `policies.include_in_photo_index = true`. Default + /// personal drives always match because the flag is materialised to + /// `true` at drive creation (see + /// `DriveRepository::create_personal_drive_atomic` + the backfill + /// migration `20260901000000_default_personal_photo_music_flags.sql`) + /// — no per-kind carve-out needed. Non-default drives (secondary + /// personals, shared drives) surface here only after their owner + /// flips the flag on via the admin "Manage policies" modal. + /// + /// `subject_types` / `subject_ids` are the caller expanded through + /// their group memberships (`AuthorizationEngine:: + /// expand_subject_for_listing`); the arrays reach into the ANY() + /// predicates so a group-mediated grant on a drive counts too. pub async fn list_media_files( &self, - owner_id: Uuid, + subject_types: &[&str], + subject_ids: &[Uuid], before: Option, limit: i64, ) -> Result<(Vec, Vec, Vec<(Option, Option)>), DomainError> { + let subject_types_owned: Vec = + subject_types.iter().map(|s| s.to_string()).collect(); let rows: Vec = sqlx::query_as( r#" SELECT fi.id::text, fi.name, fi.folder_id::text, fo.path, @@ -468,16 +489,27 @@ impl FileBlobReadRepository { FROM storage.files fi LEFT JOIN storage.folders fo ON fo.id = fi.folder_id LEFT JOIN storage.file_metadata fm ON fm.file_id = fi.id - WHERE fi.user_id = $1 + WHERE fi.drive_id IN ( + SELECT d.id + FROM storage.drives d + JOIN storage.role_grants g + ON g.resource_type = 'drive' + AND g.resource_id = d.id + WHERE g.subject_type = ANY($1) + AND g.subject_id = ANY($2) + AND (g.expires_at IS NULL OR g.expires_at > NOW()) + AND (d.policies->>'include_in_photo_index')::boolean = true + ) AND NOT fi.is_trashed AND (fi.mime_type LIKE 'image/%' OR fi.mime_type LIKE 'video/%') - AND ($2::bigint IS NULL - OR EXTRACT(EPOCH FROM fi.media_sort_date)::bigint < $2::bigint) + AND ($3::bigint IS NULL + OR EXTRACT(EPOCH FROM fi.media_sort_date)::bigint < $3::bigint) ORDER BY fi.media_sort_date DESC - LIMIT $3 + LIMIT $4 "#, ) - .bind(owner_id) + .bind(&subject_types_owned) + .bind(subject_ids) .bind(before) .bind(limit) .fetch_all(self.pool.as_ref()) @@ -500,15 +532,26 @@ impl FileBlobReadRepository { } /// Aggregate the caller's geotagged photos into grid cells of side `cell` - /// (degrees) within `bounds`. Plain SQL (no PostGIS), scoped to `user_id`. - /// Returns one cluster per non-empty cell with its centroid, photo count - /// and a representative photo id (for the cluster thumbnail). + /// (degrees) within `bounds`. Plain SQL (no PostGIS). + /// + /// Scope: same `include_in_photo_index` predicate as + /// `list_media_files` (§15). Places is the map view over the same + /// content set the Photos timeline shows, so the two surfaces MUST + /// agree on drive scope. If a drive is opt-out for Photos its + /// geotagged files never appear on the map either. + /// + /// This query is a per-cell aggregate (group by rounded lat/lng + /// bucket) rather than an ORDER BY / LIMIT hot path — the plain + /// `idx_files_drive_id` is sufficient to seek by drive. pub async fn list_geo_clusters( &self, - user_id: Uuid, + subject_types: &[&str], + subject_ids: &[Uuid], bounds: GeoBounds, cell: f64, ) -> Result, DomainError> { + let subject_types_owned: Vec = + subject_types.iter().map(|s| s.to_string()).collect(); let rows: Vec<(i64, f64, f64, String)> = sqlx::query_as( r#" SELECT count(*) AS n, @@ -517,16 +560,27 @@ impl FileBlobReadRepository { min(fm.file_id::text) AS sample_id FROM storage.file_metadata fm JOIN storage.files fi ON fi.id = fm.file_id - WHERE fi.user_id = $1 + WHERE fi.drive_id IN ( + SELECT d.id + FROM storage.drives d + JOIN storage.role_grants g + ON g.resource_type = 'drive' + AND g.resource_id = d.id + WHERE g.subject_type = ANY($1) + AND g.subject_id = ANY($2) + AND (g.expires_at IS NULL OR g.expires_at > NOW()) + AND (d.policies->>'include_in_photo_index')::boolean = true + ) AND NOT fi.is_trashed AND fm.latitude IS NOT NULL AND fm.longitude IS NOT NULL - AND fm.longitude BETWEEN $2 AND $3 - AND fm.latitude BETWEEN $4 AND $5 - GROUP BY round(fm.longitude / $6), round(fm.latitude / $6) + AND fm.longitude BETWEEN $3 AND $4 + AND fm.latitude BETWEEN $5 AND $6 + GROUP BY round(fm.longitude / $7), round(fm.latitude / $7) "#, ) - .bind(user_id) + .bind(&subject_types_owned) + .bind(subject_ids) .bind(bounds.west) .bind(bounds.east) .bind(bounds.south) diff --git a/src/interfaces/api/handlers/drive_handler.rs b/src/interfaces/api/handlers/drive_handler.rs index 2ceb5ce7..fbb0b94d 100644 --- a/src/interfaces/api/handlers/drive_handler.rs +++ b/src/interfaces/api/handlers/drive_handler.rs @@ -416,6 +416,10 @@ pub struct UpdateDrivePoliciesDto { pub forbid_cross_drive_move: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub forbid_owner_role_change: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub include_in_photo_index: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub include_in_music_index: Option, } /// `PATCH /api/drives/{id}/policies` — **OxiCloud-admin only** policy @@ -494,18 +498,22 @@ pub async fn update_drive_policies( serde_json::Value::Bool(v), ); } + if let Some(v) = dto.include_in_photo_index { + partial_obj.insert("include_in_photo_index".into(), serde_json::Value::Bool(v)); + } + if let Some(v) = dto.include_in_music_index { + partial_obj.insert("include_in_music_index".into(), serde_json::Value::Bool(v)); + } + // Pass the raw JSON straight through so the JSONB `||` merge in + // the repo only touches keys the caller supplied. Round-tripping + // via `DrivePolicies` (which has `#[serde(default)]`) would + // silently fill every omitted field with `false` — the merge + // would then clobber every unmentioned policy on the row. let partial_value = serde_json::Value::Object(partial_obj); - let partial: crate::domain::entities::drive::DrivePolicies = - match serde_json::from_value(partial_value) { - Ok(p) => p, - Err(e) => { - return AppError::bad_request(format!("invalid policy body: {e}")).into_response(); - } - }; match state .drive_management_service - .update_policies(auth_user.id, drive_id, partial) + .update_policies(auth_user.id, drive_id, partial_value) .await { Ok(merged) => (StatusCode::OK, axum::Json(merged)).into_response(), diff --git a/src/interfaces/api/handlers/photos_handler.rs b/src/interfaces/api/handlers/photos_handler.rs index 00001b3b..0e4774b5 100644 --- a/src/interfaces/api/handlers/photos_handler.rs +++ b/src/interfaces/api/handlers/photos_handler.rs @@ -12,6 +12,8 @@ use tracing::{error, info}; use crate::application::dtos::file_dto::FileDto; use crate::application::dtos::geo_dto::GeoBounds; use crate::common::di::AppState; +use crate::domain::services::authorization::Subject; +use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::AuthUser; /// Query parameters for the photos timeline endpoint. @@ -63,13 +65,29 @@ pub async fn list_photos( headers: HeaderMap, Query(params): Query, ) -> impl IntoResponse { - let user_id = auth_user.id; + let caller_id = auth_user.id; let limit = params.limit.unwrap_or(200).clamp(1, 500); + // Expand the caller into (subject_types, subject_ids) so group-mediated + // drive memberships surface in the Photos timeline too. Mirrors what + // `drive_handler::list_drives` and `trash_service::list_resources_paged` + // already do — one call to the AuthZ engine per request. + let (subject_types, subject_ids) = match state + .authorization + .expand_subject_for_listing(Subject::User(caller_id)) + .await + { + Ok(pair) => pair, + Err(e) => { + error!("list_photos: subject expansion failed: {e}"); + return AppError::from(e).into_response(); + } + }; + let file_read = &state.repositories.file_read_repository; match file_read - .list_media_files(user_id, params.before, limit) + .list_media_files(&subject_types, &subject_ids, params.before, limit) .await { Ok((files, sort_dates, dims)) => { diff --git a/tests/api/drive_policies.hurl b/tests/api/drive_policies.hurl index 4691905f..33f0f9c3 100644 --- a/tests/api/drive_policies.hurl +++ b/tests/api/drive_policies.hurl @@ -471,6 +471,67 @@ Content-Type: application/json HTTP 201 +# ───────────────────────────────────────────────────────────── +# Step 10c — partial-merge regression guard. +# +# The `PATCH /api/drives/{id}/policies` handler documents that +# omitting a field means "leave it alone", not "set it to false". +# Prior implementation round-tripped the wire body through the +# typed `DrivePolicies` struct (which has `#[serde(default)]`, so +# every omitted field defaults to `false`) and then serialised the +# whole struct into the JSONB `||` merge — silently clobbering +# every unmentioned flag back to `false`. This step exercises +# multi-flag interaction so that regression can't creep back: +# +# 1. Set `forbid_sharing = true`, assert the bag. +# 2. In a SEPARATE PATCH, set only `forbid_public_links = true`. +# 3. Assert `forbid_sharing` STILL reads `true` in the response +# — proving the merge honoured "leave omitted keys alone". +# +# Reset both back to false at the end so the shared-drive steps +# below start from a clean state. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/api/drives/{{personal_drive_id}}/policies +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "forbid_sharing": true +} + +HTTP 200 +[Asserts] +jsonpath "$.forbid_sharing" == true +jsonpath "$.forbid_public_links" == false + +PATCH {{base_url}}/api/drives/{{personal_drive_id}}/policies +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "forbid_public_links": true +} + +HTTP 200 +[Asserts] +# The load-bearing assertion — `forbid_sharing` must NOT have been +# clobbered by the omitted-key regression. +jsonpath "$.forbid_sharing" == true +jsonpath "$.forbid_public_links" == true + +# Reset both. +PATCH {{base_url}}/api/drives/{{personal_drive_id}}/policies +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "forbid_sharing": false, + "forbid_public_links": false +} + +HTTP 200 +[Asserts] +jsonpath "$.forbid_sharing" == false +jsonpath "$.forbid_public_links" == false + + # ───────────────────────────────────────────────────────────── # Step 11 — `forbid_external_sharing` on a SHARED drive, via # `POST /api/drives/{id}/members`.