From bdbb7ae1964c27ce4e035b8b8ea058622a02c39e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 19 Jun 2026 11:13:49 +0000 Subject: [PATCH 1/2] fix(auth): revoke JWT access on deactivation, deletion and role change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JWT access tokens freeze role/identity at login (access 1h, refresh 7d) and validated tokens are cached for 30s. The Bearer and cookie auth paths trusted claims.role and never re-checked the account, so demoting an admin, or disabling/deleting an account, did not revoke access until the token expired. The app-password path already re-read role/active from the DB; only the JWT/cookie path had the gap. Re-validate the caller against the live user record on the token path via the already-cached get_user_flags (role / is_external / active), bounded by USER_FLAGS_CACHE_TTL and invalidated eagerly on set_user_active / change_user_role / delete_user_admin: - middleware/user.rs: new resolve_live_role helper (+ pure decide_live_role core) — returns the *current* role, rejects deleted (NotFound) and deactivated accounts, and fails open on transient lookup errors (mirrors require_internal_user). Login/refresh remain the canonical active gate. - middleware/auth.rs: auth_middleware (Bearer + cookie) now populates CurrentUser with the live role and rejects revoked accounts (Bearer -> 401 AccountInactive; cookie -> fall through to 401/login redirect). require_admin emits an audit line on denial. - middleware/admin.rs: require_admin / require_authenticated re-check the live record, return the live role, and audit admin denials. Downstream admin gates (dedup_handler, subject_group_handler, OCS) inherit the live role automatically via CurrentUser / require_authenticated. Tests: decide_live_role policy (active / demoted / deactivated / deleted / transient fail-open) and AccountInactive -> 401. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01TAzLEQDaLak3dnrEN3YT35 --- src/interfaces/middleware/admin.rs | 63 +++++++++---- src/interfaces/middleware/auth.rs | 96 ++++++++++++++++---- src/interfaces/middleware/user.rs | 137 ++++++++++++++++++++++++++++- 3 files changed, 260 insertions(+), 36 deletions(-) diff --git a/src/interfaces/middleware/admin.rs b/src/interfaces/middleware/admin.rs index 8ddd13c2..54a86c68 100644 --- a/src/interfaces/middleware/admin.rs +++ b/src/interfaces/middleware/admin.rs @@ -14,6 +14,7 @@ use crate::application::ports::auth_ports::TokenServicePort; use crate::common::di::AppState; use crate::interfaces::api::cookie_auth::{ACCESS_COOKIE, extract_cookie_value}; use crate::interfaces::errors::AppError; +use crate::interfaces::middleware::user::{LiveRole, resolve_live_role}; /// Validate the request's JWT (from the `Authorization: Bearer …` header /// or the access-token cookie) and require `claims.role == "admin"`. @@ -43,19 +44,37 @@ pub async fn require_admin( .validate_token(&token) .map_err(|e| AppError::unauthorized(format!("Invalid token: {}", e)))?; - if claims.role != "admin" { - return Err(AppError::new( - StatusCode::FORBIDDEN, - "Admin access required", - "Forbidden", - )); - } + let user_id = Uuid::parse_str(&claims.sub) + .map_err(|_| AppError::internal_error("Invalid user ID in token"))?; - Ok(( - Uuid::parse_str(&claims.sub) - .map_err(|_| AppError::internal_error("Invalid user ID in token"))?, - claims.role.clone(), - )) + // Gate on the *live* role, not the JWT claim: a demotion or deactivation + // must take effect within the flags-cache TTL rather than surviving until + // the token expires. + match resolve_live_role( + auth.auth_application_service.as_ref(), + user_id, + &claims.role, + ) + .await + { + LiveRole::Active(role) if role == "admin" => Ok((user_id, role)), + LiveRole::Active(role) => { + tracing::info!( + target: "audit", + event = "authz.admin_denied", + reason = "not_admin", + caller_id = %user_id, + role = %role, + "👮🏻‍♂️ admin-only endpoint denied for non-admin caller" + ); + Err(AppError::new( + StatusCode::FORBIDDEN, + "Admin access required", + "Forbidden", + )) + } + LiveRole::Revoked => Err(AppError::unauthorized("Account is no longer active")), + } } /// Validate the request's JWT (any role) and return `(user_id, role)`. @@ -84,9 +103,19 @@ pub async fn require_authenticated( .validate_token(&token) .map_err(|e| AppError::unauthorized(format!("Invalid token: {}", e)))?; - Ok(( - Uuid::parse_str(&claims.sub) - .map_err(|_| AppError::internal_error("Invalid user ID in token"))?, - claims.role.clone(), - )) + let user_id = Uuid::parse_str(&claims.sub) + .map_err(|_| AppError::internal_error("Invalid user ID in token"))?; + + // Reject tokens whose account was deactivated/deleted, and return the + // caller's live role rather than the (possibly stale) JWT claim. + match resolve_live_role( + auth.auth_application_service.as_ref(), + user_id, + &claims.role, + ) + .await + { + LiveRole::Active(role) => Ok((user_id, role)), + LiveRole::Revoked => Err(AppError::unauthorized("Account is no longer active")), + } } diff --git a/src/interfaces/middleware/auth.rs b/src/interfaces/middleware/auth.rs index 547523c7..2843fa1b 100644 --- a/src/interfaces/middleware/auth.rs +++ b/src/interfaces/middleware/auth.rs @@ -13,6 +13,7 @@ use crate::common::di::AppState; // Re-export CurrentUser from application layer for use in handlers pub use crate::application::dtos::user_dto::CurrentUser; use crate::application::ports::auth_ports::TokenServicePort; +use crate::interfaces::middleware::user::{LiveRole, resolve_live_role}; /// Marker inserted into request extensions when the user was authenticated /// via the `oxicloud_access` HttpOnly cookie rather than a Bearer/Basic header. @@ -111,6 +112,9 @@ pub enum AuthError { #[error("User not found")] UserNotFound, + #[error("Account is no longer active")] + AccountInactive, + #[error("Access denied: {0}")] AccessDenied(String), @@ -127,6 +131,10 @@ impl IntoResponse for AuthError { AuthError::InvalidToken(msg) => (StatusCode::UNAUTHORIZED, msg), AuthError::TokenExpired => (StatusCode::UNAUTHORIZED, "Token expired".to_string()), AuthError::UserNotFound => (StatusCode::UNAUTHORIZED, "User not found".to_string()), + AuthError::AccountInactive => ( + StatusCode::UNAUTHORIZED, + "Account is no longer active".to_string(), + ), AuthError::AccessDenied(msg) => (StatusCode::FORBIDDEN, msg), AuthError::AuthServiceUnavailable => ( StatusCode::INTERNAL_SERVER_ERROR, @@ -181,11 +189,26 @@ pub async fn auth_middleware( let user_id = Uuid::parse_str(&claims.sub).map_err(|_| { AuthError::InvalidToken("Invalid user ID in token".to_string()) })?; + // A cryptographically valid token must not outlive the + // account: re-check the live record so deactivation, + // deletion and demotion take effect within the flags-cache + // TTL instead of waiting for token expiry. The returned + // role is authoritative — never the frozen JWT claim. + let role = match resolve_live_role( + auth_service.auth_application_service.as_ref(), + user_id, + &claims.role, + ) + .await + { + LiveRole::Active(role) => role, + LiveRole::Revoked => return Err(AuthError::AccountInactive), + }; let current_user = Arc::new(CurrentUser { id: user_id, username: claims.username.clone(), email: claims.email.clone(), - role: claims.role.clone(), + role, }); request.extensions_mut().insert(current_user); tracing::Span::current().record("user_id", user_id.to_string()); @@ -280,16 +303,33 @@ pub async fn auth_middleware( let user_id = Uuid::parse_str(&claims.sub).map_err(|_| { AuthError::InvalidToken("Invalid user ID in token".to_string()) })?; - let current_user = Arc::new(CurrentUser { - id: user_id, - username: claims.username.clone(), - email: claims.email.clone(), - role: claims.role.clone(), - }); - request.extensions_mut().insert(current_user); - request.extensions_mut().insert(CookieAuthenticated); - tracing::Span::current().record("user_id", user_id.to_string()); - return Ok(next.run(request).await); + // Same live-account re-check as the Bearer path. On + // revocation we fall through (rather than erroring) so the + // browser receives the standard 401 and redirects to + // /login, exactly like an invalid or expired cookie. + match resolve_live_role( + auth_service.auth_application_service.as_ref(), + user_id, + &claims.role, + ) + .await + { + LiveRole::Active(role) => { + let current_user = Arc::new(CurrentUser { + id: user_id, + username: claims.username.clone(), + email: claims.email.clone(), + role, + }); + request.extensions_mut().insert(current_user); + request.extensions_mut().insert(CookieAuthenticated); + tracing::Span::current().record("user_id", user_id.to_string()); + return Ok(next.run(request).await); + } + LiveRole::Revoked => { + // Fall through to the unauthenticated 401 / login redirect. + } + } } Err(e) => { tracing::debug!("Cookie token validation failed: {}", e); @@ -344,8 +384,11 @@ fn dav_basic_auth_challenge(message: &'static str) -> Response { /// Middleware to verify that the authenticated user has an admin role. /// -/// Must be applied AFTER auth_middleware, as it depends on -/// `CurrentUser` being present in the request extensions. +/// Must be applied AFTER auth_middleware, as it depends on `CurrentUser` +/// being present in the request extensions. The role carried by +/// `CurrentUser` is the *live* role resolved by `auth_middleware` (see +/// [`resolve_live_role`]), not the JWT claim, so a demotion is honoured +/// here within the flags-cache TTL. pub async fn require_admin(request: Request, next: Next) -> Response { // Get the CurrentUser inserted by auth_middleware if let Some(current_user) = request.extensions().get::>() { @@ -353,13 +396,21 @@ pub async fn require_admin(request: Request, next: Next) -> Response { tracing::debug!("Admin access granted for user: {}", current_user.username); return next.run(request).await; } - tracing::warn!( - "Admin access denied for user: {} (role: {})", - current_user.username, - current_user.role + tracing::info!( + target: "audit", + event = "authz.admin_denied", + reason = "not_admin", + caller_id = %current_user.id, + role = %current_user.role, + "👮🏻‍♂️ admin-only route denied for non-admin caller" ); } else { - tracing::warn!("Admin check failed: no authenticated user in request"); + tracing::info!( + target: "audit", + event = "authz.admin_denied", + reason = "unauthenticated", + "👮🏻‍♂️ admin-only route reached with no authenticated user" + ); } // Access denied @@ -415,4 +466,13 @@ mod tests { Some(r#"Basic realm="OxiCloud""#), ); } + + #[test] + fn account_inactive_maps_to_401() { + // A token that is still cryptographically valid but whose account was + // deactivated/deleted must be rejected with 401 (credentials no longer + // valid), so browsers redirect to /login rather than seeing a 403. + let resp = AuthError::AccountInactive.into_response(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + } } diff --git a/src/interfaces/middleware/user.rs b/src/interfaces/middleware/user.rs index 4aa4f6c7..64bf2f44 100644 --- a/src/interfaces/middleware/user.rs +++ b/src/interfaces/middleware/user.rs @@ -32,7 +32,8 @@ use uuid::Uuid; use crate::application::services::auth_application_service::AuthApplicationService; use crate::common::di::AppState; -use crate::domain::entities::user::UserRole; +use crate::domain::entities::user::{UserFlags, UserRole}; +use crate::domain::errors::{DomainError, ErrorKind}; use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::CurrentUser; @@ -102,6 +103,91 @@ pub async fn require_admin_user( Ok(()) } +/// Outcome of re-checking a token-authenticated caller against the live +/// user record (see [`resolve_live_role`]). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum LiveRole { + /// The account exists and is active. Carries the caller's *current* + /// role string (`"admin"` / `"user"`), which is authoritative and + /// supersedes the — possibly stale — JWT `role` claim. + Active(String), + /// The account is deactivated or deleted: the request must be rejected + /// even though its token is still cryptographically valid. + Revoked, +} + +/// Re-validate a caller carried by a still-valid token against the live +/// user record, so deactivation, deletion and role changes take effect +/// within [`USER_FLAGS_CACHE_TTL`](crate::application::services::auth_application_service) +/// instead of waiting for the token to expire (access 1 h / refresh 7 d by +/// default). +/// +/// JWT claims — `role` included — are frozen at login. Without this check a +/// demoted admin keeps admin power, and a disabled or deleted account keeps +/// full access, until its token expires. Returning the *current* role lets +/// every caller stop trusting `claims.role`. +/// +/// Cost: the short-TTL-cached `get_user_flags` (no `image` column), so ~one +/// tiny indexed query per user per cache-TTL window; admin role/active +/// changes invalidate the entry eagerly for immediate effect. +/// +/// Availability stance mirrors [`require_internal_user`]: a *transient* +/// lookup failure fails OPEN with the claim role (a DB blip must not lock +/// every authenticated user out, and login/refresh already enforce `active` +/// at the canonical layer). A *missing* row (`NotFound`) is a definitive +/// revocation and fails CLOSED. +pub async fn resolve_live_role( + auth: &AuthApplicationService, + user_id: Uuid, + claim_role: &str, +) -> LiveRole { + decide_live_role(auth.get_user_flags(user_id).await, user_id, claim_role) +} + +/// Pure decision core of [`resolve_live_role`], split out so the +/// allow/revoke/fail-open policy is unit-testable without a service or DB. +fn decide_live_role( + flags: Result, + user_id: Uuid, + claim_role: &str, +) -> LiveRole { + match flags { + Ok(flags) if flags.active => LiveRole::Active(flags.role.to_string()), + Ok(_) => { + audit_token_revoked(user_id, "deactivated"); + LiveRole::Revoked + } + // The user row is gone — a definitive revocation; fail closed. + Err(e) if matches!(e.kind, ErrorKind::NotFound) => { + audit_token_revoked(user_id, "deleted"); + LiveRole::Revoked + } + // Transient lookup failure (DB blip): fail open on the claim role so + // a momentary outage doesn't 401 every authenticated user at once. + Err(e) => { + tracing::warn!( + user_id = %user_id, + error = %e, + "live-user re-check failed transiently; allowing request on the JWT claim role (fail-open)" + ); + LiveRole::Active(claim_role.to_string()) + } + } +} + +/// Audit a request rejected because the token outlived the account's access +/// (deactivation or deletion). Anti-enumeration is not a concern — the +/// subject is the caller's own account. +fn audit_token_revoked(user_id: Uuid, reason: &'static str) { + tracing::info!( + target: "audit", + event = "auth.token_revoked", + reason = reason, + caller_id = %user_id, + "👮🏻‍♂️ valid token presented for an account that is no longer active — rejected" + ); +} + /// Axum middleware layer that blocks external users from a whole route /// subtree. Apply via `.layer(from_fn_with_state(state, require_internal_user_layer))` /// on the protocol nests (CalDAV / CardDAV / WebDAV) that have no @@ -153,3 +239,52 @@ pub async fn require_internal_user_layer( next.run(request).await } + +#[cfg(test)] +mod tests { + use super::*; + + fn flags(role: UserRole, active: bool) -> UserFlags { + UserFlags { + role, + is_external: false, + active, + } + } + + #[test] + fn active_admin_yields_current_admin_role() { + let live = decide_live_role(Ok(flags(UserRole::Admin, true)), Uuid::nil(), "user"); + // The live record wins over the (stale) claim — a freshly promoted + // user is admin even though their token still says "user". + assert_eq!(live, LiveRole::Active("admin".to_string())); + } + + #[test] + fn active_user_yields_current_user_role() { + // A demoted admin: token claim still "admin", live record "user". + let live = decide_live_role(Ok(flags(UserRole::User, true)), Uuid::nil(), "admin"); + assert_eq!(live, LiveRole::Active("user".to_string())); + } + + #[test] + fn deactivated_account_is_revoked() { + let live = decide_live_role(Ok(flags(UserRole::Admin, false)), Uuid::nil(), "admin"); + assert_eq!(live, LiveRole::Revoked); + } + + #[test] + fn deleted_account_not_found_is_revoked() { + let err = DomainError::new(ErrorKind::NotFound, "User", "no such user"); + let live = decide_live_role(Err(err), Uuid::nil(), "admin"); + assert_eq!(live, LiveRole::Revoked); + } + + #[test] + fn transient_error_fails_open_on_claim_role() { + // A DB blip must not lock everyone out: allow on the claim role. + let err = DomainError::new(ErrorKind::InternalError, "User", "connection reset"); + let live = decide_live_role(Err(err), Uuid::nil(), "admin"); + assert_eq!(live, LiveRole::Active("admin".to_string())); + } +} From 05d51ededd78728d33653b215a429d807c39a0b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 19 Jun 2026 11:27:11 +0000 Subject: [PATCH 2/2] perf(grants,favorites): batch resource resolution to kill N+1 and pool fan-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three list endpoints resolved each resource with one query per id: - GET /api/grants/incoming and /api/grants/outgoing used join_all(ids.map(get_file)) + join_all(ids.map(get_folder)), so a single page (limit ≤ 200) could demand ~200 concurrent connections from the 20-connection primary pool, causing acquire-timeouts and head-of-line blocking under load. - The NextCloud favorites REPORT (oc:filter-files) fetched get_file/ get_folder once per favorite — up to N serial round-trips per sync. Add by-ids batch reads that mirror the existing get_file/get_folder column mapping and NOT is_trashed filter: - FileBlobReadRepository::get_files_by_ids / FolderDbRepository::get_folders_by_ids (one SELECT ... WHERE id = ANY($1)), exposed as FileRetrievalService:: get_files_by_ids / FolderService::get_folders_by_ids returning DTOs. - Both grant handlers and the favorites REPORT now issue two batch queries total and look results up by id, preserving original order. Missing ids (stale grants whose resource was deleted, or trashed/removed favorites) drop out exactly as before. No auth-semantics change: these paths already resolved ids vetted by the authorization engine / favorites table. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01TAzLEQDaLak3dnrEN3YT35 --- .../services/file_retrieval_service.rs | 11 + src/application/services/folder_service.rs | 11 + .../pg/file_blob_read_repository.rs | 46 ++++ .../repositories/pg/folder_db_repository.rs | 31 +++ src/interfaces/api/handlers/grant_handler.rs | 226 +++++++----------- src/interfaces/nextcloud/report_handler.rs | 42 +++- 6 files changed, 226 insertions(+), 141 deletions(-) diff --git a/src/application/services/file_retrieval_service.rs b/src/application/services/file_retrieval_service.rs index 41f83bd8..27ae48e7 100644 --- a/src/application/services/file_retrieval_service.rs +++ b/src/application/services/file_retrieval_service.rs @@ -250,6 +250,17 @@ impl FileRetrievalService { let stream = self.file_read.get_file_stream(id).await?; Ok((dto, OptimizedFileContent::Stream(Box::into_pin(stream)))) } + + /// Batch counterpart of [`FileRetrievalUseCase::get_file`]: resolve many + /// file ids in ONE query instead of one per id. Like `get_file` it + /// performs no per-file authorization — both current callers (ACL grant + /// listing, NextCloud favorites REPORT) resolve ids already vetted by the + /// authorization engine or the favorites table. Missing or trashed ids are + /// absent from the result; callers re-associate by `id`. + pub async fn get_files_by_ids(&self, ids: &[String]) -> Result, DomainError> { + let files = self.file_read.get_files_by_ids(ids).await?; + Ok(files.into_iter().map(FileDto::from).collect()) + } } impl FileRetrievalUseCase for FileRetrievalService { diff --git a/src/application/services/folder_service.rs b/src/application/services/folder_service.rs index 8fd096e8..66f3893f 100644 --- a/src/application/services/folder_service.rs +++ b/src/application/services/folder_service.rs @@ -29,6 +29,17 @@ impl FolderService { } } + /// Batch counterpart of `get_folder`: resolve many folder ids in ONE + /// query instead of one per id. Like `get_folder` it performs no + /// per-folder authorization — both current callers (ACL grant listing, + /// NextCloud favorites REPORT) resolve ids already vetted by the + /// authorization engine or the favorites table. Missing or trashed ids + /// are absent from the result; callers re-associate by `id`. + pub async fn get_folders_by_ids(&self, ids: &[String]) -> Result, DomainError> { + let folders = self.folder_storage.get_folders_by_ids(ids).await?; + Ok(folders.into_iter().map(FolderDto::from).collect()) + } + /// Helper: parse a folder id string into a `Resource::Folder`. Returns /// `DomainError::not_found` on parse error (anti-enumeration — the same /// error as "folder does not exist"). diff --git a/src/infrastructure/repositories/pg/file_blob_read_repository.rs b/src/infrastructure/repositories/pg/file_blob_read_repository.rs index f2fefdfe..4999f101 100644 --- a/src/infrastructure/repositories/pg/file_blob_read_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_read_repository.rs @@ -255,6 +255,52 @@ impl FileBlobReadRepository { }) } + /// Batch-fetch files by id — the by-ids counterpart of [`get_file`], + /// used to resolve a page of ACL grants or favorites in ONE round-trip + /// instead of one query per id (the previous `join_all(ids.map(get_file))` + /// could fan out to ~200 concurrent pooled connections per page). Applies + /// the same `NOT is_trashed` filter and identical column mapping as + /// `get_file`. Ids that are missing or trashed simply drop out, so callers + /// must re-associate results by id; ordering is not guaranteed. + pub async fn get_files_by_ids(&self, ids: &[String]) -> Result, DomainError> { + let uuid_ids: Vec = ids.iter().filter_map(|id| id.parse().ok()).collect(); + if uuid_ids.is_empty() { + return Ok(Vec::new()); + } + + let rows = sqlx::query_as::<_, FileRow>( + "SELECT fi.id::text, fi.name, fi.folder_id::text, fo.path, \ + fi.size, fi.mime_type, \ + EXTRACT(EPOCH FROM fi.created_at)::bigint, \ + EXTRACT(EPOCH FROM fi.updated_at)::bigint, \ + fi.blob_hash, \ + fi.user_id \ + FROM storage.files fi \ + LEFT JOIN storage.folders fo ON fo.id = fi.folder_id \ + WHERE fi.id = ANY($1) AND NOT fi.is_trashed", + ) + .bind(&uuid_ids) + .fetch_all(self.pool.as_ref()) + .await + .map_err(|e| { + DomainError::internal_error("FileBlobRead", format!("get_files_by_ids: {e}")) + })?; + + rows.into_iter() + .map( + |(id, name, fid, fpath, size, mime, ca, ma, blob_hash, uid)| { + Self::row_to_file(id, name, fid, fpath, size, mime, ca, ma, blob_hash, uid) + }, + ) + .collect::, _>>() + .map_err(|e| { + DomainError::internal_error( + "FileBlobRead", + format!("get_files_by_ids mapping: {e}"), + ) + }) + } + /// Returns the user_id (owner) for a given file ID. /// Mirrors `FolderDbRepository::get_folder_user_id`. /// Used by the AuthorizationEngine for owner short-circuit. diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index 97852fae..5d38ce56 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -109,6 +109,37 @@ impl FolderDbRepository { ) .map_err(|e| DomainError::internal_error("FolderDb", format!("entity: {e}"))) } + + /// Batch-fetch folders by id — the by-ids counterpart of `get_folder`, + /// resolving a page of ACL grants or favorites in ONE query instead of + /// one per id. Same `NOT is_trashed` filter and column mapping as + /// `get_folder`; missing or trashed ids drop out and callers re-associate + /// by id; ordering is not guaranteed. + pub async fn get_folders_by_ids(&self, ids: &[String]) -> Result, DomainError> { + let uuid_ids: Vec = ids.iter().filter_map(|id| id.parse().ok()).collect(); + if uuid_ids.is_empty() { + return Ok(Vec::new()); + } + + let rows = sqlx::query_as::<_, FolderRow>( + r#" + SELECT id::text, name, path, parent_id::text, user_id, + EXTRACT(EPOCH FROM created_at)::bigint, + EXTRACT(EPOCH FROM updated_at)::bigint, + EXTRACT(EPOCH FROM tree_modified_at)::bigint + FROM storage.folders + WHERE id = ANY($1) AND NOT is_trashed + "#, + ) + .bind(&uuid_ids) + .fetch_all(self.pool()) + .await + .map_err(|e| DomainError::internal_error("FolderDb", format!("get_folders_by_ids: {e}")))?; + + rows.into_iter() + .map(|r| Self::row_to_folder(r.0, r.1, r.2, r.3, Some(r.4), r.5, r.6, r.7)) + .collect() + } } impl FolderRepository for FolderDbRepository { diff --git a/src/interfaces/api/handlers/grant_handler.rs b/src/interfaces/api/handlers/grant_handler.rs index 7fc39ab3..63674364 100644 --- a/src/interfaces/api/handlers/grant_handler.rs +++ b/src/interfaces/api/handlers/grant_handler.rs @@ -11,8 +11,8 @@ use axum::{ http::StatusCode, response::IntoResponse, }; -use futures::future::join_all; use serde::Deserialize; +use std::collections::HashMap; use std::sync::Arc; use tracing::{error, warn}; use utoipa::IntoParams; @@ -26,13 +26,10 @@ use crate::application::dtos::grant_dto::{ SubjectInputDto, UpdateRoleDto, role_from_permissions, }; use crate::application::ports::authorization_ports::AuthorizationEngine; -use crate::application::ports::file_ports::FileRetrievalUseCase; -use crate::application::ports::folder_ports::FolderUseCase; use crate::application::services::recipient_notification_service::NotifyTrigger; use crate::common::di::AppState; #[allow(unused_imports)] use crate::common::errors::DomainError; -use crate::domain::errors::ErrorKind; use crate::domain::services::authorization::{ GrantCursor, IncomingGrantSummary, OutgoingResourceSummary, Permission, Resource, ResourceKind, Role, Subject, @@ -664,83 +661,64 @@ pub async fn list_shared_with_me( .map(|s| s.resource_id.to_string()) .collect(); - // Resolve resource details concurrently (files and folders in parallel). - let (file_results, folder_results) = tokio::join!( - join_all(file_ids.iter().map(|id| file_service.get_file(id))), - join_all(folder_ids.iter().map(|id| folder_service.get_folder(id))) + // Resolve resource details in two batch queries (was one per id via + // join_all, which could fan out to ~limit concurrent pooled connections + // and starve the primary pool). Missing ids — stale grants whose resource + // was deleted before the cascade trigger fired — drop out of the maps. + let (file_list, folder_list) = tokio::join!( + file_service.get_files_by_ids(&file_ids), + folder_service.get_folders_by_ids(&folder_ids) ); + let file_map: HashMap = match file_list { + Ok(files) => files.into_iter().map(|f| (f.id.clone(), f)).collect(), + Err(e) => return AppError::from(e).into_response(), + }; + let folder_map: HashMap = match folder_list { + Ok(folders) => folders.into_iter().map(|f| (f.id.clone(), f)).collect(), + Err(e) => return AppError::from(e).into_response(), + }; - // Build the unified item list in original grant order (newest first). - // We iterate summaries in order and pick the resolved result from the - // appropriate typed bucket. - let mut file_idx = 0usize; - let mut folder_idx = 0usize; - + // Build the unified item list in original grant order (newest first), + // looking each resolved resource up by id. let mut items: Vec = Vec::with_capacity(summaries.len()); for summary in &summaries { + let rid = summary.resource_id.to_string(); match summary.resource_type { - ResourceKind::File => { - let result = &file_results[file_idx]; - file_idx += 1; - match result { - Ok(file_dto) => { - items.push(SharedWithMeItemDto { - resource_type: ResourceTypeDto::File, - permissions: summary.permissions.iter().map(|p| (*p).into()).collect(), - granted_at: summary.granted_at, - granted_by: summary.granted_by, - resource: ResourceContentDto::File( - file_dto.clone().without_hierarchy_info(), - ), - }); - } - Err(e) if e.kind == ErrorKind::NotFound => { - // Stale grant (file deleted, trigger not yet fired) — skip silently. - warn!( - "Skipping stale file grant for resource_id={}: not found", - summary.resource_id - ); - } - Err(e) => { - return AppError::internal_error(format!( - "Failed to fetch file {}: {e}", - summary.resource_id - )) - .into_response(); - } + ResourceKind::File => match file_map.get(&rid) { + Some(file_dto) => { + items.push(SharedWithMeItemDto { + resource_type: ResourceTypeDto::File, + permissions: summary.permissions.iter().map(|p| (*p).into()).collect(), + granted_at: summary.granted_at, + granted_by: summary.granted_by, + resource: ResourceContentDto::File( + file_dto.clone().without_hierarchy_info(), + ), + }); } - } - ResourceKind::Folder => { - let result = &folder_results[folder_idx]; - folder_idx += 1; - match result { - Ok(folder_dto) => { - items.push(SharedWithMeItemDto { - resource_type: ResourceTypeDto::Folder, - permissions: summary.permissions.iter().map(|p| (*p).into()).collect(), - granted_at: summary.granted_at, - granted_by: summary.granted_by, - resource: ResourceContentDto::Folder( - folder_dto.clone().without_hierarchy_info(), - ), - }); - } - Err(e) if e.kind == ErrorKind::NotFound => { - warn!( - "Skipping stale folder grant for resource_id={}: not found", - summary.resource_id - ); - } - Err(e) => { - return AppError::internal_error(format!( - "Failed to fetch folder {}: {e}", - summary.resource_id - )) - .into_response(); - } + None => warn!( + "Skipping stale file grant for resource_id={}: not found", + summary.resource_id + ), + }, + ResourceKind::Folder => match folder_map.get(&rid) { + Some(folder_dto) => { + items.push(SharedWithMeItemDto { + resource_type: ResourceTypeDto::Folder, + permissions: summary.permissions.iter().map(|p| (*p).into()).collect(), + granted_at: summary.granted_at, + granted_by: summary.granted_by, + resource: ResourceContentDto::Folder( + folder_dto.clone().without_hierarchy_info(), + ), + }); } - } + None => warn!( + "Skipping stale folder grant for resource_id={}: not found", + summary.resource_id + ), + }, } } @@ -909,13 +887,20 @@ pub async fn list_my_shares( .map(|s| s.resource_id.to_string()) .collect(); - let (file_results, folder_results) = tokio::join!( - join_all(file_ids.iter().map(|id| file_service.get_file(id))), - join_all(folder_ids.iter().map(|id| folder_service.get_folder(id))) + // Two batch queries instead of one get_* per id (see list_shared_with_me). + let (file_list, folder_list) = tokio::join!( + file_service.get_files_by_ids(&file_ids), + folder_service.get_folders_by_ids(&folder_ids) ); + let file_map: HashMap = match file_list { + Ok(files) => files.into_iter().map(|f| (f.id.clone(), f)).collect(), + Err(e) => return AppError::from(e).into_response(), + }; + let folder_map: HashMap = match folder_list { + Ok(folders) => folders.into_iter().map(|f| (f.id.clone(), f)).collect(), + Err(e) => return AppError::from(e).into_response(), + }; - let mut file_idx = 0usize; - let mut folder_idx = 0usize; let mut items: Vec = Vec::with_capacity(summaries.len()); for summary in &summaries { @@ -935,64 +920,39 @@ pub async fn list_my_shares( }) .collect(); + let rid = summary.resource_id.to_string(); match summary.resource_type { - ResourceKind::File => { - let result = &file_results[file_idx]; - file_idx += 1; - match result { - Ok(file_dto) => { - // Caller is the granter — they had share-access to the - // resource, so the containing hierarchy is already known - // to them. Keep `path` (unlike list_shared_with_me). - items.push(OutgoingResourceItemDto { - resource_type: ResourceTypeDto::File, - first_shared_at: summary.first_shared_at, - resource: ResourceContentDto::File(file_dto.clone()), - grants, - }); - } - Err(e) if e.kind == ErrorKind::NotFound => { - warn!( - "Skipping stale outgoing file grant for resource_id={}: not found", - summary.resource_id - ); - } - Err(e) => { - return AppError::internal_error(format!( - "Failed to fetch file {}: {e}", - summary.resource_id - )) - .into_response(); - } + ResourceKind::File => match file_map.get(&rid) { + Some(file_dto) => { + // Caller is the granter — they had share-access to the + // resource, so the containing hierarchy is already known + // to them. Keep `path` (unlike list_shared_with_me). + items.push(OutgoingResourceItemDto { + resource_type: ResourceTypeDto::File, + first_shared_at: summary.first_shared_at, + resource: ResourceContentDto::File(file_dto.clone()), + grants, + }); } - } - ResourceKind::Folder => { - let result = &folder_results[folder_idx]; - folder_idx += 1; - match result { - Ok(folder_dto) => { - items.push(OutgoingResourceItemDto { - resource_type: ResourceTypeDto::Folder, - first_shared_at: summary.first_shared_at, - resource: ResourceContentDto::Folder(folder_dto.clone()), - grants, - }); - } - Err(e) if e.kind == ErrorKind::NotFound => { - warn!( - "Skipping stale outgoing folder grant for resource_id={}: not found", - summary.resource_id - ); - } - Err(e) => { - return AppError::internal_error(format!( - "Failed to fetch folder {}: {e}", - summary.resource_id - )) - .into_response(); - } + None => warn!( + "Skipping stale outgoing file grant for resource_id={}: not found", + summary.resource_id + ), + }, + ResourceKind::Folder => match folder_map.get(&rid) { + Some(folder_dto) => { + items.push(OutgoingResourceItemDto { + resource_type: ResourceTypeDto::Folder, + first_shared_at: summary.first_shared_at, + resource: ResourceContentDto::Folder(folder_dto.clone()), + grants, + }); } - } + None => warn!( + "Skipping stale outgoing folder grant for resource_id={}: not found", + summary.resource_id + ), + }, } } diff --git a/src/interfaces/nextcloud/report_handler.rs b/src/interfaces/nextcloud/report_handler.rs index d5e81b38..9e056d0b 100644 --- a/src/interfaces/nextcloud/report_handler.rs +++ b/src/interfaces/nextcloud/report_handler.rs @@ -7,7 +7,7 @@ use quick_xml::{ Reader, Writer, events::{BytesEnd, BytesStart, Event}, }; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use crate::application::dtos::display_helpers::{ @@ -17,7 +17,6 @@ use crate::application::dtos::file_dto::FileDto; use crate::application::dtos::folder_dto::FolderDto; use crate::application::dtos::search_dto::SearchCriteriaDto; use crate::application::ports::favorites_ports::FavoritesUseCase; -use crate::application::ports::file_ports::FileRetrievalUseCase; use crate::application::ports::folder_ports::FolderUseCase; use crate::application::ports::inbound::SearchUseCase; use crate::common::di::AppState; @@ -86,20 +85,47 @@ async fn handle_filter_files( let home_prefix = format!("My Folder - {}/", user.username); - // Pass 1: fetch the favorited DTOs (the per-item fetch is a separate - // concern from the oc:fileid resolution batched below). + // Pass 1: resolve the favorited DTOs in two batch queries (was one + // get_* per favorite — up to N serial round-trips on a sync client's + // REPORT). Results are looked up by id so the response keeps favorites + // order; missing/trashed favorites simply drop out (as before). + let mut file_ids: Vec = Vec::new(); + let mut folder_ids: Vec = Vec::new(); + for fav in &favorites { + match fav.item_type.as_str() { + "file" => file_ids.push(fav.item_id.clone()), + "folder" => folder_ids.push(fav.item_id.clone()), + _ => {} + } + } + + let file_map: HashMap = file_service + .get_files_by_ids(&file_ids) + .await + .map_err(|e| AppError::internal_error(format!("Failed to resolve favorite files: {e}")))? + .into_iter() + .map(|f| (f.id.clone(), f)) + .collect(); + let folder_map: HashMap = folder_service + .get_folders_by_ids(&folder_ids) + .await + .map_err(|e| AppError::internal_error(format!("Failed to resolve favorite folders: {e}")))? + .into_iter() + .map(|f| (f.id.clone(), f)) + .collect(); + let mut files: Vec = Vec::new(); let mut folders: Vec = Vec::new(); for fav in &favorites { match fav.item_type.as_str() { "file" => { - if let Ok(f) = file_service.get_file(&fav.item_id).await { - files.push(f); + if let Some(f) = file_map.get(&fav.item_id) { + files.push(f.clone()); } } "folder" => { - if let Ok(f) = folder_service.get_folder(&fav.item_id).await { - folders.push(f); + if let Some(f) = folder_map.get(&fav.item_id) { + folders.push(f.clone()); } } _ => {}