diff --git a/src/application/ports/mod.rs b/src/application/ports/mod.rs index ba8fd2b3..79eaa9ea 100644 --- a/src/application/ports/mod.rs +++ b/src/application/ports/mod.rs @@ -20,6 +20,7 @@ pub mod music_ports; pub mod outbound; pub mod plugin_ports; pub mod recent_ports; +pub mod resource_access_hook; pub mod share_ports; pub mod storage_ports; pub mod thumbnail_ports; diff --git a/src/application/ports/resource_access_hook.rs b/src/application/ports/resource_access_hook.rs new file mode 100644 index 00000000..e8fc8fca --- /dev/null +++ b/src/application/ports/resource_access_hook.rs @@ -0,0 +1,50 @@ +//! Observer notified when a caller successfully reads or mutates a file. +//! +//! Read-event sibling of [`crate::application::ports::file_lifecycle`]. The +//! lifecycle hook fires on content changes (created/copied/updated/deleted); +//! this one fires on access — every authorised file read, every successful +//! upload, every PUT/COPY — and lets cross-cutting observers (Recent list, +//! audit trail, future "last seen by" UX) react without each +//! protocol-surface handler having to remember to call them. +//! +//! Folders are deliberately out of scope: a listing fires on every UI +//! navigation, every PROPFIND, every NC sync poll, and would dominate +//! `auth.user_recent_files` with noise that no user actually opened. +//! Only file-level interactions count. +//! +//! Implementors run **after** the service layer's authZ check has passed and +//! the read/write has succeeded; a denied or 404'd request never fires the +//! hook. The method is synchronous — implementors that need to do real work +//! spawn it themselves so the user-facing request is never blocked on the +//! side-effect. The recording impl lives in +//! `infrastructure/services/recent_recording_hook.rs`. + +use uuid::Uuid; + +/// Fired by the application services on a successful, authorised access to a +/// file owned (or shared with) the caller. +/// +/// `caller_id` is mandatory because the recording side needs to know **who** +/// touched the file — the same file accessed by two different users records +/// two separate Recent rows. Anonymous surfaces (public share downloads via +/// `/api/s/{token}`) deliberately do not call this hook: a "viewer" without +/// an authenticated identity has no Recent list to land in. +pub trait ResourceAccessHook: Send + Sync { + /// Called after a file read or successful write touched `file_id` on + /// behalf of `caller_id`. The caller has already been authorised — the + /// hook is fire-and-forget; failures are the implementor's problem and + /// must never propagate. + fn on_file_accessed(&self, caller_id: Uuid, file_id: &str); + + /// Called after `caller_id` has emptied their Recent list (either by + /// clearing the whole table or removing a single row). Implementors + /// hold in-memory throttle / dedup state keyed by `(caller, item)`; + /// without this signal a freshly-cleared list would refuse to record + /// the next access until the throttle TTL expires, leaving the user + /// staring at an empty Recent and wondering why their open-then-close + /// did nothing. + /// + /// Default no-op: implementations without any in-memory state — most + /// audit-trail-style observers — needn't react. + fn on_recents_cleared(&self, _caller_id: Uuid) {} +} diff --git a/src/application/services/file_management_service.rs b/src/application/services/file_management_service.rs index 9292833b..84d3780e 100644 --- a/src/application/services/file_management_service.rs +++ b/src/application/services/file_management_service.rs @@ -4,6 +4,7 @@ use crate::application::dtos::file_dto::FileDto; use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::file_lifecycle::FileLifecycleHook; use crate::application::ports::file_ports::FileManagementUseCase; +use crate::application::ports::resource_access_hook::ResourceAccessHook; use crate::application::ports::storage_ports::{CopyFolderTreeResult, FileWritePort}; use crate::application::ports::trash_ports::TrashUseCase; use crate::application::services::trash_service::TrashService; @@ -31,6 +32,11 @@ pub struct FileManagementService { authz: Arc, /// Lifecycle hook dispatcher — fired on file created (copy) and deleted. file_lifecycle_hook: Option>, + /// Read/write access hook — fired so Recent reflects "this is the file + /// I just copied / renamed / moved", same way the read paths surface + /// downloads. Distinct from the lifecycle hook because lifecycle hooks + /// don't carry the `caller_id` the recording side needs. + resource_access_hook: Option>, } impl FileManagementService { @@ -53,6 +59,7 @@ impl FileManagementService { content_cache, authz, file_lifecycle_hook: None, + resource_access_hook: None, } } @@ -62,6 +69,19 @@ impl FileManagementService { self } + /// Registers the read/write access hook (Recent list recorder). + pub fn with_resource_access_hook(mut self, hook: Arc) -> Self { + self.resource_access_hook = Some(hook); + self + } + + /// Internal helper: fire the access hook if registered. + fn notify_file_accessed(&self, caller_id: Uuid, file_id: &str) { + if let Some(hook) = &self.resource_access_hook { + hook.on_file_accessed(caller_id, file_id); + } + } + /// Engine check for a file resource. Parses the id into a `Uuid` and /// requires the specified permission. async fn require_file_perm( @@ -156,6 +176,9 @@ impl FileManagementService { if let Some(hook) = &self.file_lifecycle_hook { hook.on_file_copied(&dto.id, &dto.content_hash, &dto.mime_type, file_id); } + // The caller just spawned a fresh file — show it in their Recent + // list. The source file isn't recorded; only the visible target. + self.notify_file_accessed(caller_id, &dto.id); Ok(dto) } diff --git a/src/application/services/file_retrieval_service.rs b/src/application/services/file_retrieval_service.rs index 490a423e..ec416eb5 100644 --- a/src/application/services/file_retrieval_service.rs +++ b/src/application/services/file_retrieval_service.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use crate::application::dtos::file_dto::FileDto; use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::file_ports::{FileRetrievalUseCase, OptimizedFileContent}; +use crate::application::ports::resource_access_hook::ResourceAccessHook; use crate::application::ports::storage_ports::FileReadPort; use crate::common::errors::DomainError; use crate::domain::services::authorization::{Permission, Resource, Subject}; @@ -33,6 +34,11 @@ pub struct FileRetrievalService { content_cache: Option>, transcode: Option>, authz: Option>, + /// Optional read-event observer. Currently fans out to the Recent-list + /// recorder; future observers (audit trail, "last seen by", …) attach + /// to the same hook so service code only knows the trait, not the impl. + /// `None` for the test/stub path that constructs via [`Self::new`]. + resource_access_hook: Option>, } impl FileRetrievalService { @@ -45,6 +51,7 @@ impl FileRetrievalService { content_cache: None, transcode: None, authz: None, + resource_access_hook: None, } } @@ -61,6 +68,32 @@ impl FileRetrievalService { content_cache: Some(content_cache), transcode: Some(transcode), authz: Some(authz), + resource_access_hook: None, + } + } + + /// Builder: attach a [`ResourceAccessHook`] that fires after every + /// authorised `_with_perms` read. Without it the service is silent — + /// existing behaviour for stub / test paths. + pub fn with_resource_access_hook(mut self, hook: Arc) -> Self { + self.resource_access_hook = Some(hook); + self + } + + /// Fire the access hook if registered. Called from every `_with_perms` + /// read after the authZ + lookup has succeeded (never on failure + /// paths — denied reads must not surface in Recent). + /// + /// `pub` because the WebDAV / NextCloud DAV handlers resolve files + /// by path and authorise via that resolver, not via the + /// `*_with_perms` service methods — they then serve content through + /// the no-perms `get_file_stream` / `get_file_range_stream`. Those + /// handlers must call this directly after their own authZ has + /// passed so cross-protocol downloads (NC desktop, davx5, native + /// `/webdav/`) also surface in Recent. + pub fn notify_file_accessed(&self, caller_id: Uuid, file_id: &str) { + if let Some(hook) = &self.resource_access_hook { + hook.on_file_accessed(caller_id, file_id); } } @@ -262,6 +295,11 @@ impl FileRetrievalUseCase for FileRetrievalService { async fn get_file_with_perms(&self, id: &str, caller_id: Uuid) -> Result { self.require_file(id, Permission::Read, caller_id).await?; let file = self.file_read.get_file(id).await?; + // After authZ + lookup succeed: this caller has just inspected the + // file. Recent listing observes via the hook. The throttle in the + // recording impl coalesces repeat metadata fetches against the same + // file (file viewer poll, browse-then-download pattern). + self.notify_file_accessed(caller_id, id); Ok(FileDto::from(file)) } @@ -333,6 +371,7 @@ impl FileRetrievalUseCase for FileRetrievalService { caller_id: Uuid, ) -> Result> + Send>, DomainError> { self.require_file(id, Permission::Read, caller_id).await?; + self.notify_file_accessed(caller_id, id); self.file_read.get_file_stream(id).await } @@ -359,6 +398,7 @@ impl FileRetrievalUseCase for FileRetrievalService { self.require_file(id, Permission::Read, caller_id).await?; let file = self.file_read.get_file(id).await?; let dto = FileDto::from(file); + self.notify_file_accessed(caller_id, id); self.optimized_inner(id, dto, accept_webp, prefer_original) .await } @@ -393,6 +433,10 @@ impl FileRetrievalUseCase for FileRetrievalService { end: Option, ) -> Result> + Send>, DomainError> { self.require_file(id, Permission::Read, caller_id).await?; + // Range requests are bursty (video seeks, NC chunked downloads) — + // the recording hook's per-(caller, file) throttle absorbs the + // storm so one watched video lands as one Recent row, not 1000. + self.notify_file_accessed(caller_id, id); self.file_read.get_file_range_stream(id, start, end).await } diff --git a/src/application/services/file_upload_service.rs b/src/application/services/file_upload_service.rs index 16fc8554..6f054f55 100644 --- a/src/application/services/file_upload_service.rs +++ b/src/application/services/file_upload_service.rs @@ -5,6 +5,7 @@ use crate::application::dtos::file_dto::FileDto; use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::file_lifecycle::FileLifecycleHook; use crate::application::ports::file_ports::{FileUploadUseCase, StoredBlob}; +use crate::application::ports::resource_access_hook::ResourceAccessHook; use crate::application::ports::storage_ports::{FileReadPort, FileWritePort, StorageUsagePort}; use crate::application::services::storage_usage_service::StorageUsageService; use crate::common::errors::DomainError; @@ -35,6 +36,12 @@ pub struct FileUploadService { content_cache: Option>, /// Single lifecycle dispatcher — fires on_file_created / on_file_updated. file_lifecycle_hook: Option>, + /// Read-event hook — fires "caller just touched this file" so Recent + /// records uploads / overwrites alongside reads. Distinct from + /// `file_lifecycle_hook` because the lifecycle dispatcher only knows + /// `(file_id, blob_hash, content_type)`; the recording side needs the + /// `caller_id` the service already has in hand. + resource_access_hook: Option>, /// Dependencies of the instant-upload path /// (`create_file_from_owned_blob_with_perms`); `None` in minimal test /// wiring. @@ -58,6 +65,7 @@ impl FileUploadService { storage_usage_service: None, content_cache: None, file_lifecycle_hook: None, + resource_access_hook: None, instant_upload: None, } } @@ -73,6 +81,7 @@ impl FileUploadService { storage_usage_service: None, content_cache: None, file_lifecycle_hook: None, + resource_access_hook: None, instant_upload: None, } } @@ -105,6 +114,19 @@ impl FileUploadService { self } + /// Registers the read/write access hook (Recent list recorder). + pub fn with_resource_access_hook(mut self, hook: Arc) -> Self { + self.resource_access_hook = Some(hook); + self + } + + /// Internal helper: fire the access hook if registered. + fn notify_file_accessed(&self, caller_id: Uuid, file_id: &str) { + if let Some(hook) = &self.resource_access_hook { + hook.on_file_accessed(caller_id, file_id); + } + } + /// Configures the storage usage service pub fn with_storage_usage_service( mut self, @@ -282,6 +304,9 @@ impl FileUploadService { if let Some(hook) = &self.file_lifecycle_hook { hook.on_file_updated(file_id, &dto.content_hash, &dto.mime_type); } + // Delta-upload commit path — record the swap so Recent reflects + // "this is the file I just delta-updated". + self.notify_file_accessed(caller_id, file_id); Ok(dto) } @@ -372,6 +397,9 @@ impl FileUploadUseCase for FileUploadService { if let Some(hook) = &self.file_lifecycle_hook { hook.on_file_created(&dto.id, &dto.content_hash, &dto.mime_type, blob.is_new_blob); } + // The caller just created this file — surface it in Recent so the + // "I just uploaded X" UX matches the pre-SvelteKit behaviour. + self.notify_file_accessed(caller_id, &dto.id); Ok(dto) } @@ -429,6 +457,7 @@ impl FileUploadUseCase for FileUploadService { if let Some(hook) = &self.file_lifecycle_hook { hook.on_file_updated(&file_id, &dto.content_hash, content_type); } + self.notify_file_accessed(caller_id, &file_id); return Ok(dto); } @@ -474,6 +503,7 @@ impl FileUploadUseCase for FileUploadService { if let Some(hook) = &self.file_lifecycle_hook { hook.on_file_created(&dto.id, &dto.content_hash, content_type, is_new_blob); } + self.notify_file_accessed(caller_id, &dto.id); Ok(dto) } } diff --git a/src/application/services/recent_service.rs b/src/application/services/recent_service.rs index b54e318b..17f0bd2f 100644 --- a/src/application/services/recent_service.rs +++ b/src/application/services/recent_service.rs @@ -1,10 +1,11 @@ use crate::application::dtos::cursor::PageCursor; use crate::application::dtos::recent_dto::{RecentCursor, RecentItemDto, RecentResourceRow}; use crate::application::ports::recent_ports::{RecentItemsRepositoryPort, RecentItemsUseCase}; +use crate::application::ports::resource_access_hook::ResourceAccessHook; use crate::common::errors::{DomainError, ErrorKind, Result}; use crate::domain::services::authorization::ResourceKind; use crate::infrastructure::repositories::pg::RecentItemsPgRepository; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; use tracing::info; use uuid::Uuid; @@ -15,6 +16,14 @@ use uuid::Uuid; pub struct RecentService { repo: Arc, max_recent_items: i32, + /// Set after construction via [`Self::set_resource_access_hook`]. + /// The hook is built FROM this service (it wraps an `Arc`), so + /// we can't take it as a constructor arg without circular ownership; + /// the OnceLock holds the back-edge so this service can notify the + /// hook when the user clears or removes Recent rows. The notification + /// lets the hook drop its in-memory throttle entries — otherwise a + /// freshly-cleared Recent refuses to re-record until the TTL expires. + resource_access_hook: OnceLock>, } impl RecentService { @@ -23,6 +32,25 @@ impl RecentService { Self { repo, max_recent_items: max_recent_items.clamp(1, 100), + resource_access_hook: OnceLock::new(), + } + } + + /// Wire the access hook in after construction. Idempotent: a second + /// `set` is a no-op (returns the existing value as `Err`). Called + /// from DI once `RecentRecordingHook::new(Arc)` has produced + /// the back-edge that closes the loop. + pub fn set_resource_access_hook(&self, hook: Arc) { + let _ = self.resource_access_hook.set(hook); + } + + /// Internal helper: notify the hook (if registered) that `user_id` + /// has emptied their Recent list — wholly or by removing a single + /// row. The hook drops its in-memory throttle entries so the very + /// next access re-records into the freshly-empty table. + fn notify_recents_cleared(&self, user_id: Uuid) { + if let Some(hook) = self.resource_access_hook.get() { + hook.on_recents_cleared(user_id); } } } @@ -100,6 +128,12 @@ impl RecentItemsUseCase for RecentService { item_id, user_id ); + // Drop the throttle entries so the next access re-records. We + // notify on every call (even when `removed == false`) so the + // semantics are "the user expressed intent to forget this" — + // the hook owns the per-(user, item) cache anyway, dropping a + // miss is a no-op. + self.notify_recents_cleared(user_id); Ok(removed) } @@ -108,6 +142,7 @@ impl RecentItemsUseCase for RecentService { info!("Clearing all recent items for user {}", user_id); self.repo.clear_all(user_id).await?; info!("Cleared all recent items for user {}", user_id); + self.notify_recents_cleared(user_id); Ok(()) } } diff --git a/src/common/di.rs b/src/common/di.rs index b8360dbc..68f2d1ca 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -518,6 +518,9 @@ impl AppServiceFactory { plugin_dispatch: Option< Arc, >, + resource_access_hook: Option< + Arc, + >, ) -> ApplicationServices { // Main services let folder_service = Arc::new(FolderService::new( @@ -529,20 +532,26 @@ impl AppServiceFactory { // bridge (which looks file metadata up by id) can be wired into the // dispatcher they receive. It depends only on repos + core, never on // the upload service, so the reorder is safe. - let file_retrieval_service = Arc::new(FileRetrievalService::new_with_cache( - repos.file_read_repository.clone(), - core.file_content_cache.clone(), - core.image_transcode_service.clone(), - authz.clone(), - )); + let file_retrieval_service = { + let mut svc = FileRetrievalService::new_with_cache( + repos.file_read_repository.clone(), + core.file_content_cache.clone(), + core.image_transcode_service.clone(), + authz.clone(), + ); + if let Some(hook) = resource_access_hook.clone() { + svc = svc.with_resource_access_hook(hook); + } + Arc::new(svc) + }; // Effective lifecycle dispatcher: the core hooks (thumbnails, metadata) // plus, when the plugins feature is enabled, the WASM plugin bridge. let file_lifecycle = self.effective_file_lifecycle(core, &file_retrieval_service, plugin_dispatch); - let file_upload_service = Arc::new( - FileUploadService::new_with_read( + let file_upload_service = Arc::new({ + let mut svc = FileUploadService::new_with_read( repos.file_write_repository.clone(), repos.file_read_repository.clone(), ) @@ -562,8 +571,12 @@ impl AppServiceFactory { authz.clone(), core.dedup_service.clone(), storage_usage.clone(), - ), - ); + ); + if let Some(hook) = resource_access_hook.clone() { + svc = svc.with_resource_access_hook(hook); + } + svc + }); // Delta-upload protocol — chunk negotiation over the same dedup // store. Bounded by the same whole-file ceiling as byte uploads. @@ -580,8 +593,8 @@ impl AppServiceFactory { ); // FileManagementService — ref_count handled by PG trigger, no dedup port needed - let file_management_service = Arc::new( - FileManagementService::with_trash( + let file_management_service = Arc::new({ + let mut svc = FileManagementService::with_trash( repos.file_write_repository.clone(), trash_service.clone(), Some(repos.file_read_repository.clone()), @@ -589,8 +602,12 @@ impl AppServiceFactory { Some(core.file_content_cache.clone()), authz.clone(), ) - .with_file_lifecycle_hook(file_lifecycle.clone()), - ); + .with_file_lifecycle_hook(file_lifecycle.clone()); + if let Some(hook) = resource_access_hook.clone() { + svc = svc.with_resource_access_hook(hook); + } + svc + }); let file_use_case_factory = Arc::new(AppFileUseCaseFactory::new( repos.file_read_repository.clone(), @@ -1121,6 +1138,31 @@ impl AppServiceFactory { let pool = Arc::new(pools.primary); let maintenance_pool = Arc::new(pools.maintenance); + // Recent service + recording hook are built up-front so the + // hook can be threaded into `create_application_services` below. + // The file services hold the hook directly so every authorised + // `_with_perms` read/write fires into `auth.user_recent_files` + // without per-handler wiring. Reordering vs the legacy in-block + // creation (further down) is safe: `create_recent_service` only + // needs `pool`, which is already in scope. + // + // The back-edge `recent_service_eager.set_resource_access_hook` + // closes the loop so the clear/remove handlers can drop the + // hook's in-memory throttle entries — without it a freshly + // cleared Recent list refuses to re-record the same file for a + // full TTL window, surfacing as "I cleared, opened the file, + // and Recent is still empty" (caught by tests/api/recent.hurl + // step 8). + let recent_service_eager = self.create_recent_service(&pool); + let resource_access_hook: Arc< + dyn crate::application::ports::resource_access_hook::ResourceAccessHook, + > = Arc::new( + crate::infrastructure::services::recent_recording_hook::RecentRecordingHook::new( + recent_service_eager.clone(), + ), + ); + recent_service_eager.set_resource_access_hook(resource_access_hook.clone()); + // 1. Core services (PgPool needed for DedupService index) let core = self.create_core_services(&pool, &maintenance_pool).await?; @@ -1179,6 +1221,7 @@ impl AppServiceFactory { &storage_usage, content_index.as_ref().map(|(idx, _)| idx.clone()), plugin_dispatch.clone(), + Some(resource_access_hook.clone()), ); // 5. Share service @@ -1217,9 +1260,11 @@ impl AppServiceFactory { favorites_service = Some(favs.clone()); apps.favorites_service = Some(favs); - let recent = self.create_recent_service(&pool); - recent_service = Some(recent.clone()); - apps.recent_service = Some(recent); + // Already built up-front so the file services could hold the + // RecentRecordingHook — reuse the same Arc here so AppState and + // the recording hook share one service instance. + recent_service = Some(recent_service_eager.clone()); + 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)) diff --git a/src/infrastructure/services/mod.rs b/src/infrastructure/services/mod.rs index cbdf0a9c..77ea5115 100644 --- a/src/infrastructure/services/mod.rs +++ b/src/infrastructure/services/mod.rs @@ -31,6 +31,7 @@ pub mod path_service; pub mod pg_acl_engine; #[cfg(feature = "plugins")] pub mod plugins; +pub mod recent_recording_hook; pub mod retry_blob_backend; pub mod s3_blob_backend; pub mod search_index; diff --git a/src/infrastructure/services/recent_recording_hook.rs b/src/infrastructure/services/recent_recording_hook.rs new file mode 100644 index 00000000..f6ee9cb3 --- /dev/null +++ b/src/infrastructure/services/recent_recording_hook.rs @@ -0,0 +1,112 @@ +//! Recording side of [`ResourceAccessHook`] — turns a successful file access +//! into a row in `auth.user_recent_files` via [`RecentService`]. +//! +//! Wiring lives in `common/di.rs`: this hook is registered once, every +//! `_with_perms` file method on `FileRetrievalService` / `FileManagementService` +//! fans through it, and any future read-path or write-path service can opt in +//! by holding an `Option>` and calling +//! `on_file_accessed` after authZ. +//! +//! Two non-obvious behaviours, with rationale: +//! +//! * **Per-(caller, file) 60-second throttle.** Range-stream downloads send +//! one GET per chunk (NC desktop, video seek, resumable transfers); without +//! throttling each chunk would trigger an upsert against the same row. +//! Moka's `time_to_live` gives us bounded memory and lock-free reads. The +//! underlying `INSERT … ON CONFLICT DO UPDATE accessed_at = now()` is +//! idempotent, so the rare TOCTOU window between `contains_key` and `insert` +//! is harmless — at worst we record twice for the same instant. +//! +//! * **Fire-and-forget via `tokio::spawn`.** The `ResourceAccessHook` method +//! is synchronous by contract (every `with_perms` caller would otherwise +//! have to `await` the side-effect). The spawn lets the user-facing +//! response return immediately; a DB hiccup in Recent recording never +//! bubbles up to the GET / PUT that triggered it. Failures log at warn. + +use std::sync::Arc; +use std::time::Duration; + +use moka::sync::Cache; +use uuid::Uuid; + +use crate::application::ports::recent_ports::RecentItemsUseCase; +use crate::application::ports::resource_access_hook::ResourceAccessHook; +use crate::application::services::recent_service::RecentService; + +/// How long a successful recording suppresses repeat upserts for the same +/// `(caller, file)`. Sized to span a typical streamed range-GET burst while +/// still updating `accessed_at` often enough that the Recent list reflects +/// "this is the file I was just looking at". +const THROTTLE_TTL_SECONDS: u64 = 60; + +/// Bound on simultaneous in-flight throttle entries. Each entry is a tuple +/// `(Uuid, String) -> ()` ≈ 80 B; 16 384 entries ≈ 1.3 MB worst case. LRU +/// eviction keeps memory bounded even if a pathological client touches a +/// million files in a minute. +const THROTTLE_MAX_ENTRIES: u64 = 16_384; + +/// `ResourceAccessHook` implementation that records file accesses into +/// `auth.user_recent_files`, throttled per (caller, file). +pub struct RecentRecordingHook { + recent: Arc, + throttle: Cache<(Uuid, String), ()>, +} + +impl RecentRecordingHook { + pub fn new(recent: Arc) -> Self { + // `support_invalidation_closures` is the moka opt-in needed by + // `invalidate_entries_if` (the per-user throttle reset on + // `on_recents_cleared`). Without it, the predicate-based + // invalidate call silently no-ops and a freshly-cleared Recent + // list refuses to re-record the same file until the TTL + // expires — exactly the bug surfaced by tests/api/recent.hurl + // step 8. + let throttle = Cache::builder() + .max_capacity(THROTTLE_MAX_ENTRIES) + .time_to_live(Duration::from_secs(THROTTLE_TTL_SECONDS)) + .support_invalidation_closures() + .build(); + Self { recent, throttle } + } +} + +impl ResourceAccessHook for RecentRecordingHook { + fn on_file_accessed(&self, caller_id: Uuid, file_id: &str) { + let key = (caller_id, file_id.to_string()); + if self.throttle.contains_key(&key) { + return; + } + // Insert before spawning: even if the spawned task races with another + // call for the same key, the cache entry suppresses the duplicate + // before it reaches the DB. The ON CONFLICT clause covers the + // sub-microsecond TOCTOU window between contains_key and insert. + self.throttle.insert(key.clone(), ()); + + let recent = Arc::clone(&self.recent); + let (caller_id, file_id) = key; + tokio::spawn(async move { + if let Err(e) = recent.record_item_access(caller_id, &file_id, "file").await { + tracing::warn!( + target: "oxicloud::recent", + caller_id = %caller_id, + file_id = %file_id, + "recent recording failed: {e}", + ); + } + }); + } + + fn on_recents_cleared(&self, caller_id: Uuid) { + // Drop every throttle entry that would otherwise suppress the + // next recording for this user. moka schedules the predicate to + // run during the next maintenance pass — it's not synchronous. + // The DB clear has already happened by the time we get here, so + // any racing access between the clear and the next maintenance + // pass just re-records via ON CONFLICT — the worst case is a row + // that surfaces in Recent a few ms after the clear, which is + // exactly what the user asked for. + let _ = self + .throttle + .invalidate_entries_if(move |(k_caller, _), _| *k_caller == caller_id); + } +} diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 74b1bde0..02543404 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -833,6 +833,14 @@ async fn handle_get( return Ok(resp); } + // Recent recording deliberately does NOT fire here: native WebDAV + // is overwhelmingly a sync-engine surface (rclone, davfs2, Finder + // mounts) and a first descent would push every synced file into + // Recent, drowning out the SPA's "what I actually opened" signal. + // See memory note `project_recent_session_intent.md` — the planned + // session-intent gate (interactive JWT vs app-password) will turn + // this back on for the rare human-driven DAV access. + // Range Requests — mount-style clients (rclone, davfs2, Finder) read // by ranges; serve 206/416 instead of re-sending the whole file on // every seek or resume. diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index 05e3f060..410f0ed1 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -355,6 +355,15 @@ async fn handle_get( return Ok(resp); } + // Recent recording deliberately does NOT fire here: NC's primary + // client (Nextcloud desktop, davx5, mobile NC apps) is a sync + // engine, and a first-time descent of a large library would push + // every file into Recent, drowning out the SPA's "what I actually + // opened" signal. See memory note + // `project_recent_session_intent.md` — the planned session-intent + // gate (interactive JWT vs app-password) will turn this back on + // for human-driven NC web access in the same browser session. + // Range Requests — serve 206/416 instead of the whole file on seeks. if let Some(resp) = range_response(headers, &file, &etag, file_service).await { return Ok(resp); diff --git a/tests/api/recent.hurl b/tests/api/recent.hurl index b253d9d2..4f423908 100644 --- a/tests/api/recent.hurl +++ b/tests/api/recent.hurl @@ -59,6 +59,18 @@ file_id: jsonpath "$[0].id" jsonpath "$[0].name" == "hello-renamed.txt" +# Defensive clear before the explicit-POST assertions: earlier +# scenarios in the runner (files-folders.hurl) auto-record every +# file they upload / GET through the service-layer +# `ResourceAccessHook`, so Recent already has rows by the time we +# arrive here. Clearing first lets step 4 assert `count == 1` +# against a known-empty baseline. +DELETE {{base_url}}/api/recent/clear +Authorization: Bearer {{token}} + +HTTP 200 + + # ───────────────────────────────────────────────────────────── # Step 3 – Record access to hello-renamed.txt # ───────────────────────────────────────────────────────────── @@ -100,3 +112,78 @@ HTTP 200 [Asserts] jsonpath "$.items" isCollection jsonpath "$.items" count == 0 + + +# ───────────────────────────────────────────────────────────── +# Step 7 – Auto-recording on upload +# The backend `ResourceAccessHook` fires on a successful +# authorised upload, so the new file lands in Recent +# without the client POSTing /api/recent/file/{id}. +# This is the SvelteKit-era contract: the legacy +# vanilla-JS frontend did the POST itself; the new shell +# relies on the service-layer hook instead. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{token}} +[MultipartFormData] +folder_id: {{home_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +auto_uploaded_id: jsonpath "$.id" + + +GET {{base_url}}/api/recent/resources +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" count == 1 +jsonpath "$.items[0].resource.id" == "{{auto_uploaded_id}}" + + +# ───────────────────────────────────────────────────────────── +# Step 8 – Auto-recording on GET (file download) +# Clear first, then download the file content and +# assert it reappears in Recent. The per-(user, file) +# 60 s throttle inside the recording hook means the +# cleared row may re-record on the very next GET only +# because we just emptied the table — moka stores the +# throttle entry independently of the DB row, but the +# upsert is idempotent and harmless either way. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/recent/clear +Authorization: Bearer {{token}} + +HTTP 200 + + +GET {{base_url}}/api/files/{{auto_uploaded_id}} +Authorization: Bearer {{token}} + +HTTP 200 + + +GET {{base_url}}/api/recent/resources +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" count == 1 +jsonpath "$.items[0].resource.id" == "{{auto_uploaded_id}}" + + +# ───────────────────────────────────────────────────────────── +# Step 9 – Cleanup so the test is idempotent across runs. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/files/{{auto_uploaded_id}} +Authorization: Bearer {{token}} + +HTTP 204 + + +DELETE {{base_url}}/api/recent/clear +Authorization: Bearer {{token}} + +HTTP 200