From 8a42b07cbef8e208fffea6725373f86ec1e5a984 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 10 Jun 2026 09:27:32 +0000 Subject: [PATCH] perf(auth): cached image-free user-flags lookup for per-request guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every WebDAV / CalDAV / CardDAV request paid one full-row user fetch in require_internal_user_layer just to read `is_external` (and the NC Basic Auth middleware repeated it right after its own cache hit). That SELECT includes the `image` column — a data URI of up to 512 KiB — so a sync client issuing hundreds of PROPFINDs per minute dragged hundreds of MB of avatar bytes out of Postgres to evaluate a boolean. - New `UserFlags { role, is_external, active }` + a repo query selecting only those three columns (inherent method, mirroring `update_image`). - `AuthApplicationService::get_user_flags`: moka cache, 30 s TTL, 10k capacity. `change_user_role` / `set_user_active` invalidate eagerly, so admin changes still apply immediately; anything else is visible within the TTL — preserving the documented "no token rotation needed" semantics at a per-request cost of zero DB round-trips when warm. - `require_internal_user`, `require_admin_user` and the NC Basic Auth external check now go through the flags lookup. https://claude.ai/code/session_01Dp3oWon5GBMVn4j3QXZdgx --- .../services/auth_application_service.rs | 44 +++++++++++++++++-- src/domain/entities/user.rs | 12 +++++ .../repositories/pg/user_pg_repository.rs | 36 ++++++++++++++- src/interfaces/middleware/user.rs | 27 +++++++----- .../nextcloud/basic_auth_middleware.rs | 9 ++-- 5 files changed, 108 insertions(+), 20 deletions(-) diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 007626b9..36584601 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -11,7 +11,7 @@ use crate::common::config::OidcConfig; use crate::common::errors::{DomainError, ErrorKind}; use crate::domain::entities::magic_link_token::{MagicLinkResourceKind, MagicLinkStatus}; use crate::domain::entities::session::Session; -use crate::domain::entities::user::{User, UserRole}; +use crate::domain::entities::user::{User, UserFlags, UserRole}; use crate::domain::repositories::magic_link_token_repository::MagicLinkTokenRepository; use crate::infrastructure::repositories::pg::SessionPgRepository; use crate::infrastructure::repositories::pg::UserPgRepository; @@ -140,8 +140,20 @@ pub struct AuthApplicationService { /// Magic-link token repository — populated when the magic-link feature /// is enabled (PR 8+). `None` means redemption endpoints return 503. magic_link_repo: Option>, + /// Per-user authorization flags (`role` / `is_external` / `active`), + /// consulted by middleware guards on every WebDAV / CalDAV / CardDAV + /// request. The short TTL keeps the "role changes apply without token + /// rotation" property within seconds while removing one DB round-trip + /// per request; the known mutation paths (`change_user_role`, + /// `set_user_active`) also invalidate eagerly. + user_flags_cache: Cache, } +/// TTL for [`AuthApplicationService::user_flags_cache`]. Upper bound on how +/// long a role / external / active change can take to be observed by the +/// per-request guards when it bypasses the eager invalidation paths. +const USER_FLAGS_CACHE_TTL: Duration = Duration::from_secs(30); + impl AuthApplicationService { pub fn new( user_storage: Arc, @@ -170,6 +182,10 @@ impl AuthApplicationService { .time_to_live(Duration::from_secs(60)) .build(), magic_link_repo: None, + user_flags_cache: Cache::builder() + .max_capacity(10_000) + .time_to_live(USER_FLAGS_CACHE_TTL) + .build(), } } @@ -1141,6 +1157,24 @@ impl AuthApplicationService { Ok(UserDto::from(user)) } + /// Cached, image-free lookup of the caller's authorization flags + /// (`role` / `is_external` / `active`). This is the per-request fast + /// path for middleware guards: the full `get_user` row fetch drags the + /// `image` column (a data URI of up to 512 KiB) across the wire, which + /// a sync client issuing hundreds of DAV requests per minute paid on + /// every single one just to read a boolean. + /// + /// Staleness is bounded by [`USER_FLAGS_CACHE_TTL`]; role and active + /// changes made through this service invalidate the entry eagerly. + pub async fn get_user_flags(&self, user_id: Uuid) -> Result { + if let Some(flags) = self.user_flags_cache.get(&user_id) { + return Ok(flags); + } + let flags = self.user_storage.get_user_flags(user_id).await?; + self.user_flags_cache.insert(user_id, flags); + Ok(flags) + } + /// Apply a profile update on behalf of the calling user (PR 24). /// /// Hard rules: @@ -1797,7 +1831,9 @@ impl AuthApplicationService { pub async fn set_user_active(&self, user_id: Uuid, active: bool) -> Result<(), DomainError> { self.user_storage .set_user_active_status(user_id, active) - .await + .await?; + self.user_flags_cache.invalidate(&user_id); + Ok(()) } /// Change user role (admin only) @@ -1809,7 +1845,9 @@ impl AuthApplicationService { format!("Invalid role: {}. Must be 'admin' or 'user'", role), )); } - self.user_storage.change_role(user_id, role).await + self.user_storage.change_role(user_id, role).await?; + self.user_flags_cache.invalidate(&user_id); + Ok(()) } /// Update user's storage quota (admin only) diff --git a/src/domain/entities/user.rs b/src/domain/entities/user.rs index 22417cd8..4431831c 100644 --- a/src/domain/entities/user.rs +++ b/src/domain/entities/user.rs @@ -20,6 +20,18 @@ impl std::fmt::Display for UserRole { } } +/// Authorization-relevant account flags, fetched without the heavyweight +/// profile columns. The full user row drags `image` along — a data URI of +/// up to 512 KiB — which per-request guards (`require_internal_user`, +/// `require_admin_user`, the NC Basic Auth external check) must never pay +/// for just to read a boolean or a role. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct UserFlags { + pub role: UserRole, + pub is_external: bool, + pub active: bool, +} + #[derive(Debug, Clone)] pub struct User { id: Uuid, diff --git a/src/infrastructure/repositories/pg/user_pg_repository.rs b/src/infrastructure/repositories/pg/user_pg_repository.rs index 82132fad..9d896a5d 100644 --- a/src/infrastructure/repositories/pg/user_pg_repository.rs +++ b/src/infrastructure/repositories/pg/user_pg_repository.rs @@ -5,7 +5,7 @@ use uuid::Uuid; use crate::application::ports::auth_ports::UserStoragePort; use crate::common::errors::DomainError; -use crate::domain::entities::user::{User, UserRole}; +use crate::domain::entities::user::{User, UserFlags, UserRole}; use crate::domain::repositories::user_repository::{ StorageStats, UserRepository, UserRepositoryError, UserRepositoryResult, }; @@ -51,6 +51,40 @@ impl UserPgRepository { } } + /// Fetch only the authorization-relevant flags of a user. Not part of + /// the `UserRepository` trait — called directly from + /// `AuthApplicationService::get_user_flags`. + /// + /// Deliberately selects three tiny columns instead of the full row: + /// the full-row SELECT includes `image` (a data URI of up to 512 KiB), + /// which per-request middleware guards were paying on every WebDAV / + /// CalDAV / CardDAV request just to read `is_external` or `role`. + pub async fn get_user_flags(&self, id: Uuid) -> UserRepositoryResult { + let row = sqlx::query( + r#" + SELECT role::text as role_text, is_external, active + FROM auth.users + WHERE id = $1 + "#, + ) + .bind(id) + .fetch_one(&*self.pool) + .await + .map_err(Self::map_sqlx_error)?; + + let role_str: Option = row.try_get("role_text").unwrap_or(None); + let role = match role_str.as_deref() { + Some("admin") => UserRole::Admin, + _ => UserRole::User, + }; + + Ok(UserFlags { + role, + is_external: row.get("is_external"), + active: row.get("active"), + }) + } + /// Updates a user's profile image (URL or data URI). Not part of the /// `UserRepository` trait — called directly from `AuthApplicationService`. pub async fn update_image( diff --git a/src/interfaces/middleware/user.rs b/src/interfaces/middleware/user.rs index ddce6387..4aa4f6c7 100644 --- a/src/interfaces/middleware/user.rs +++ b/src/interfaces/middleware/user.rs @@ -4,9 +4,11 @@ //! so handlers compose them uniformly as one-liners. They assume the //! caller has already been authenticated by the //! [`AuthUser`](super::auth::AuthUser) extractor, and pull the current -//! user state from the database via `AuthApplicationService` so role / -//! external-flag changes take effect on the next request without -//! waiting for token rotation. +//! user flags via `AuthApplicationService::get_user_flags` — a +//! lightweight, short-TTL-cached lookup (no `image` column) — so role / +//! external-flag changes take effect within seconds without waiting +//! for token rotation, while the hot DAV paths stop paying one full-row +//! DB fetch per request. //! //! ```ignore //! let caller_id = auth_user.id; @@ -30,6 +32,7 @@ use uuid::Uuid; use crate::application::services::auth_application_service::AuthApplicationService; use crate::common::di::AppState; +use crate::domain::entities::user::UserRole; use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::CurrentUser; @@ -56,8 +59,8 @@ pub async fn require_internal_user( auth: &AuthApplicationService, caller_id: Uuid, ) -> Result<(), AppError> { - match auth.get_user_by_id(caller_id).await { - Ok(dto) if dto.is_external => Err(AppError::new( + match auth.get_user_flags(caller_id).await { + Ok(flags) if flags.is_external => Err(AppError::new( StatusCode::FORBIDDEN, "External users cannot access this endpoint", "Forbidden", @@ -70,9 +73,11 @@ pub async fn require_internal_user( /// admins, `Err(403)` otherwise. /// /// The check pulls the role from the user record (not from JWT -/// claims) so a role change takes effect on the next request without -/// waiting for token rotation. Mirrors [`require_internal_user`]'s -/// shape so handlers compose either of them as a one-liner via `?`. +/// claims) so a role change takes effect within the flags-cache TTL — +/// or immediately when changed through `change_user_role`, which +/// invalidates the entry — without waiting for token rotation. Mirrors +/// [`require_internal_user`]'s shape so handlers compose either of +/// them as a one-liner via `?`. /// /// Use this in handlers that already have an /// [`AuthUser`](super::auth::AuthUser) extractor (and thus a validated @@ -82,12 +87,12 @@ pub async fn require_admin_user( auth: &AuthApplicationService, caller_id: Uuid, ) -> Result<(), AppError> { - let user = auth - .get_user_by_id(caller_id) + let flags = auth + .get_user_flags(caller_id) .await .map_err(AppError::from)?; - if user.role != "admin" { + if flags.role != UserRole::Admin { return Err(AppError::new( StatusCode::FORBIDDEN, "Admin access required", diff --git a/src/interfaces/nextcloud/basic_auth_middleware.rs b/src/interfaces/nextcloud/basic_auth_middleware.rs index 70b3c7e1..8486a4a1 100644 --- a/src/interfaces/nextcloud/basic_auth_middleware.rs +++ b/src/interfaces/nextcloud/basic_auth_middleware.rs @@ -64,8 +64,7 @@ pub async fn basic_auth_middleware( // Check account lockout before attempting password verification (saves CPU). // The lockout is per (account, IP), see #323 for rationale. - let client_ip = - crate::interfaces::middleware::rate_limit::extract_client_ip(&request); + let client_ip = crate::interfaces::middleware::rate_limit::extract_client_ip(&request); if let Some(auth_svc) = state.auth_service.as_ref() && let Err(secs) = auth_svc.login_lockout.check(&username, &client_ip) { @@ -103,11 +102,11 @@ pub async fn basic_auth_middleware( // this is the belt-and-braces check in case one slipped // through (e.g. user later flipped to is_external). if let Some(auth_svc) = state.auth_service.as_ref() - && let Ok(user) = auth_svc + && let Ok(flags) = auth_svc .auth_application_service - .get_user_by_id(user_id) + .get_user_flags(user_id) .await - && user.is_external + && flags.is_external { tracing::info!( target: "audit",