From ec72374651f2f09f8deb895a56d8b4b5afb8d36c Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 2 Jun 2026 11:20:44 +0200 Subject: [PATCH] feat(api): can grant external user (via email) - add possibility to grant an external user. - route /api/users/{id} added (rate limited for security) - security: start route limitation for external users ex: they must not browse /api/users/{id} nor addressbook --- src/application/dtos/user_dto.rs | 12 ++ .../services/auth_application_service.rs | 109 ++++++++++++++++++ src/common/di.rs | 14 +++ .../api/handlers/contacts_handler.rs | 31 ++++- src/interfaces/api/handlers/mod.rs | 1 + src/interfaces/api/handlers/users_handler.rs | 91 +++++++++++++++ src/interfaces/api/routes.rs | 7 ++ src/interfaces/middleware/mod.rs | 1 + src/interfaces/middleware/user.rs | 92 +++++++++++++++ tests/api/external_users.hurl | 59 +++++++++- 10 files changed, 414 insertions(+), 3 deletions(-) create mode 100644 src/interfaces/api/handlers/users_handler.rs create mode 100644 src/interfaces/middleware/user.rs diff --git a/src/application/dtos/user_dto.rs b/src/application/dtos/user_dto.rs index 54d47f3b..8f7a759c 100644 --- a/src/application/dtos/user_dto.rs +++ b/src/application/dtos/user_dto.rs @@ -24,6 +24,16 @@ pub struct UserDto { /// can't own storage; their quota is always 0. Internal users /// default to `false`. pub is_external: bool, + /// Optional first/given name. Populated from the OIDC `given_name` + /// claim at JIT provisioning, or via a profile-edit endpoint. + /// `None` until explicitly set — `skip_serializing_if = "Option::is_none"` + /// keeps the wire format compact for the common case. + #[serde(skip_serializing_if = "Option::is_none")] + pub given_name: Option, + /// Optional last/family name. Same provenance + serde rules as + /// `given_name`. + #[serde(skip_serializing_if = "Option::is_none")] + pub family_name: Option, } impl From for UserDto { @@ -43,6 +53,8 @@ impl From for UserDto { image: user.image().map(|s| s.to_string()), can_edit_image: !user.is_oidc_user(), is_external: user.is_external(), + given_name: user.given_name().map(str::to_string), + family_name: user.family_name().map(str::to_string), } } } diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 9ec6ebe8..33418cc5 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -897,6 +897,115 @@ impl AuthApplicationService { self.get_user(user_id).await } + /// Visibility-checked profile lookup for `GET /api/users/{id}`. + /// + /// Returns `NotFound` (not `AccessDenied`) when the caller has no + /// legitimate relationship with the target — anti-enumeration: an + /// attacker probing random UUIDs cannot distinguish "user doesn't + /// exist" from "exists but you can't see them". + /// + /// External callers (`is_external = TRUE`) are locked out of the + /// endpoint entirely. They have no legitimate need to enumerate + /// users — their session exists only to interact with resources + /// they were explicitly granted. Returns `AccessDenied`, which the + /// handler surfaces as 403; the external caller's own role is not + /// a secret to themselves, so the honest status is appropriate. + /// + /// Visibility rule for internal callers: + /// 1. caller_id == target_id → always visible (self). + /// 2. caller is admin → always visible (admin needs every user). + /// 3. target is internal AND `expose_system_users` is on → already + /// broadly visible via the system address book; no extra check. + /// 4. caller and target share at least one grant — either + /// direction, either as subject or granter. Subject-group + /// co-membership is intentionally NOT included in v1; can be + /// added later if a concrete need surfaces. + /// 5. Anything else → `NotFound`. + pub async fn get_user_profile( + &self, + caller_id: Uuid, + target_id: Uuid, + expose_system_users: bool, + pool: &sqlx::PgPool, + ) -> Result { + // External-caller lockout. Load the caller eagerly so the + // is_external check covers every branch (including self-lookup + // — an external user reading their own profile via this route + // is still off-limits; the frontend should rely on the existing + // /api/auth/me endpoint for that). + let caller = self.user_storage.get_user_by_id(caller_id).await?; + if caller.is_external() { + return Err(DomainError::new( + ErrorKind::AccessDenied, + "User", + "External users cannot query /api/users/{id}", + )); + } + + // Self: always (now that the external lockout already filtered + // external self-lookups above). + if caller_id == target_id { + return Ok(UserDto::from(caller)); + } + + // Anti-enumeration: NotFound for everything that doesn't pass. + // Convert a real NotFound on `target` to the same anonymous 404, + // so existence isn't leaked through differential responses. + let target = match self.user_storage.get_user_by_id(target_id).await { + Ok(u) => u, + Err(e) if e.kind == ErrorKind::NotFound => { + return Err(DomainError::new( + ErrorKind::NotFound, + "User", + "User not found", + )); + } + Err(e) => return Err(e), + }; + + // Internal target + system-address-book exposed: already public. + if !target.is_external() && expose_system_users { + return Ok(UserDto::from(target)); + } + + // Admin caller: always visible. + if caller.role() == UserRole::Admin { + return Ok(UserDto::from(target)); + } + + // Shared grant: caller and target appear together in at least one + // access_grants row (either as the granted-by + user-subject pair, + // or symmetrically). LIMIT 1 + the (granted_by) + (subject_type, + // subject_id) indexes keep this cheap. + let related: Option = sqlx::query_scalar( + r#" + SELECT 1 + FROM storage.access_grants + WHERE (granted_by = $1 AND subject_type = 'user' AND subject_id = $2) + OR (granted_by = $2 AND subject_type = 'user' AND subject_id = $1) + LIMIT 1 + "#, + ) + .bind(caller_id) + .bind(target_id) + .fetch_optional(pool) + .await + .map_err(|e| { + DomainError::internal_error("UserProfile", format!("visibility query: {}", e)) + })?; + + if related.is_some() { + return Ok(UserDto::from(target)); + } + + // No relationship — anti-enumeration NotFound. + Err(DomainError::new( + ErrorKind::NotFound, + "User", + "User not found", + )) + } + // New method to get user by username - needed for admin user handling pub async fn get_user_by_username(&self, username: &str) -> Result { let user = self.user_storage.get_user_by_username(username).await?; diff --git a/src/common/di.rs b/src/common/di.rs index a08bb152..eb7a0e92 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -925,6 +925,13 @@ impl AppServiceFactory { email_sender: None, // populated below mock_email_sender: None, // populated below magic_link_invite_service: None, // populated below + // 60 lookups / minute / caller; cap at 50 000 tracked + // callers to bound memory. The same limiter instance is + // shared by every clone of AppState since it lives in an + // Arc. + user_profile_rate_limiter: Arc::new( + crate::interfaces::middleware::rate_limit::RateLimiter::new(60, 60, 50_000), + ), }; let email_bundle = build_email_sender(&self.config.smtp); app_state.email_sender = email_bundle.sender; @@ -1302,6 +1309,13 @@ pub struct AppState { pub magic_link_invite_service: Option< Arc, >, + /// Per-caller sliding-window limiter for `GET /api/users/{id}`. The + /// endpoint's primary defense is the visibility check, but a stale + /// JWT could in theory iterate UUIDs against the related-by-grant + /// branch of that check. 60 lookups per minute keyed on the + /// authenticated caller covers any legitimate UI rendering while + /// throttling enumeration. + pub user_profile_rate_limiter: Arc, } // All AppState construction is done via struct literal in build_app_state(). diff --git a/src/interfaces/api/handlers/contacts_handler.rs b/src/interfaces/api/handlers/contacts_handler.rs index 3e55092a..1ee98be2 100644 --- a/src/interfaces/api/handlers/contacts_handler.rs +++ b/src/interfaces/api/handlers/contacts_handler.rs @@ -254,7 +254,22 @@ pub async fn list_address_books( }) .collect(); - if state.expose_system_users && state.auth_service.is_some() { + // Skip the system address book for external callers so they + // don't see an internal-user directory entry (let alone its + // contents). The system book is only useful to internal + // users picking sharees out of the directory. + let hide_system_for_external = match state.auth_service.as_ref() { + Some(svc) => { + crate::interfaces::middleware::user::require_internal_user(svc, auth_user.id) + .await + .is_err() + } + None => false, + }; + if state.expose_system_users + && state.auth_service.is_some() + && !hide_system_for_external + { let now = Utc::now(); response.push(AddressBookResponse { id: SYSTEM_BOOK_ID.to_string(), @@ -451,6 +466,14 @@ pub async fn list_contacts( let Some(auth_service) = &state.auth_service else { return system_book_unavailable(); }; + // External callers must not enumerate the internal-user + // directory through the system address book. + if let Err(e) = + crate::interfaces::middleware::user::require_internal_user(auth_service, auth_user.id) + .await + { + return e.into_response(); + } let caller_id = auth_user.id.to_string(); match auth_service.list_users(params.limit, params.offset).await { Ok(users) => { @@ -562,6 +585,12 @@ pub async fn get_contact( let Some(auth_service) = &state.auth_service else { return system_book_unavailable(); }; + if let Err(e) = + crate::interfaces::middleware::user::require_internal_user(auth_service, auth_user.id) + .await + { + return e.into_response(); + } let Ok(uuid) = Uuid::parse_str(&contact_id) else { return ( StatusCode::BAD_REQUEST, diff --git a/src/interfaces/api/handlers/mod.rs b/src/interfaces/api/handlers/mod.rs index 382808a7..2033ed2c 100644 --- a/src/interfaces/api/handlers/mod.rs +++ b/src/interfaces/api/handlers/mod.rs @@ -21,6 +21,7 @@ pub mod search_handler; pub mod share_handler; pub mod subject_group_handler; pub mod trash_handler; +pub mod users_handler; pub mod webdav_handler; pub mod wopi_handler; diff --git a/src/interfaces/api/handlers/users_handler.rs b/src/interfaces/api/handlers/users_handler.rs new file mode 100644 index 00000000..46425640 --- /dev/null +++ b/src/interfaces/api/handlers/users_handler.rs @@ -0,0 +1,91 @@ +//! User-profile lookup for the frontend. +//! +//! `GET /api/users/{id}` returns a [`UserDto`] for the target user iff +//! the authenticated caller has a legitimate relationship with them. +//! The visibility rule lives in +//! [`AuthApplicationService::get_user_profile`] — handlers never embed +//! their own authz check (CLAUDE.md § Authorization). +//! +//! In addition to the per-request visibility check, every call is +//! throttled by a per-caller sliding-window limiter (60/min) so that a +//! stale JWT can't iterate UUIDs against the related-by-grant branch +//! of the visibility rule. The limiter shares the same `RateLimiter` +//! type as the login / register / refresh middlewares; this handler +//! invokes it inline rather than through a layer because the key is +//! the authenticated caller_id (not the client IP). + +use axum::{ + Json, Router, + extract::{Path, State}, + http::StatusCode, + response::IntoResponse, + routing::get, +}; +use std::sync::Arc; +use uuid::Uuid; + +use crate::common::di::AppState; +use crate::interfaces::errors::AppError; +use crate::interfaces::middleware::auth::AuthUser; + +/// Build the `/users` router — mounted at the `/api/users` prefix by +/// `main.rs`. Auth + CSRF middlewares are applied by the caller. +pub fn user_routes() -> Router> { + Router::new().route("/{id}", get(get_user_profile)) +} + +#[utoipa::path( + get, + path = "/api/users/{id}", + params(("id" = String, Path, description = "User UUID")), + responses( + (status = 200, description = "Profile of a user the caller can see"), + (status = 404, description = "User does not exist OR caller has no visibility (anti-enumeration: indistinguishable)"), + (status = 429, description = "Per-caller rate limit exceeded"), + ), + security(("bearerAuth" = [])), + tag = "users", +)] +async fn get_user_profile( + State(state): State>, + auth_user: AuthUser, + Path(target_id): Path, +) -> Result { + let caller_id = auth_user.id; + + // Rate limit FIRST so an attacker can't exhaust the visibility + // query (which touches `access_grants`) by hammering with random + // UUIDs. + if let Err(()) = state + .user_profile_rate_limiter + .check_and_increment(&caller_id.to_string()) + { + return Err(AppError::new( + StatusCode::TOO_MANY_REQUESTS, + "Too many user lookups; please retry shortly", + "RateLimited", + )); + } + + let auth_svc = state + .auth_service + .as_ref() + .ok_or_else(|| AppError::internal_error("Auth service not configured"))?; + let pool = state + .db_pool + .as_ref() + .ok_or_else(|| AppError::internal_error("Database pool not available"))?; + + let dto = auth_svc + .auth_application_service + .get_user_profile( + caller_id, + target_id, + state.core.config.features.expose_system_users, + pool, + ) + .await + .map_err(AppError::from)?; + + Ok(Json(dto)) +} diff --git a/src/interfaces/api/routes.rs b/src/interfaces/api/routes.rs index c79592b9..8469e43f 100644 --- a/src/interfaces/api/routes.rs +++ b/src/interfaces/api/routes.rs @@ -571,6 +571,13 @@ pub fn create_api_routes(app_state: &Arc) -> Router> { .with_state(app_state.clone()); router = router.nest("/groups", group_router); + // Per-user profile lookup `/api/users/{id}` — authenticated only, + // throttled by a per-caller limiter inside the handler. External + // callers are 403'd in the service layer. + let users_router = crate::interfaces::api::handlers::users_handler::user_routes() + .with_state(app_state.clone()); + router = router.nest("/users", users_router); + // Transparent compression (gzip + brotli) for all API responses. // tower-http negotiates via Accept-Encoding and skips already-compressed // content types automatically. No manual compression in handlers. diff --git a/src/interfaces/middleware/mod.rs b/src/interfaces/middleware/mod.rs index 82a81905..de11d844 100644 --- a/src/interfaces/middleware/mod.rs +++ b/src/interfaces/middleware/mod.rs @@ -4,3 +4,4 @@ pub mod csrf; pub mod rate_limit; pub mod trace_span; pub mod trusted_proxy; +pub mod user; diff --git a/src/interfaces/middleware/user.rs b/src/interfaces/middleware/user.rs new file mode 100644 index 00000000..bd4083bc --- /dev/null +++ b/src/interfaces/middleware/user.rs @@ -0,0 +1,92 @@ +//! Caller-id-based user guards. +//! +//! All guards in this module take `(auth, caller_id) → Result<(), AppError>` +//! 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. +//! +//! ```ignore +//! let caller_id = auth_user.id; +//! require_internal_user(&auth, caller_id).await?; +//! require_admin_user(&auth, caller_id).await?; +//! ``` +//! +//! Future role-based guards (e.g. `require_active_user`) should follow +//! the same shape so they slot in next to these without ceremony. +//! +//! For the legacy header-based admin guard (`require_admin`), see +//! [`super::admin`] — that variant exists because some handlers take +//! `headers: HeaderMap` directly instead of `AuthUser`. + +use axum::http::StatusCode; +use uuid::Uuid; + +use crate::application::services::auth_application_service::AuthApplicationService; +use crate::interfaces::errors::AppError; + +/// Require the caller to be an internal user. Returns `Ok(())` for +/// internal callers, `Err(403)` for externals. +/// +/// External users authenticate via magic-link / OIDC-only / OCM and +/// exist solely to interact with resources they were explicitly +/// granted. They have no business enumerating the user directory, the +/// address book, subject groups, or any other instance-wide listing — +/// this guard locks them out of those surfaces. +/// +/// DB lookup errors fall back to `Ok(())` so a transient outage doesn't +/// lock everyone out — this guard is defense in depth. The canonical +/// filter is at the service / repository layer (`include_external = +/// false` on `list_users`, the visibility rule in `get_user_profile`, +/// etc.); this helper just opts a surface in to "internal only" with +/// one extra line. +/// +/// The 403 status is honest (not 404 stealth) because the caller's own +/// `is_external` flag is not a secret to themselves — the UI already +/// surfaces "you came in through a magic link". +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( + StatusCode::FORBIDDEN, + "External users cannot access this endpoint", + "Forbidden", + )), + _ => Ok(()), + } +} + +/// Require the caller to hold the admin role. Returns `Ok(())` for +/// 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 `?`. +/// +/// Use this in handlers that already have an +/// [`AuthUser`](super::auth::AuthUser) extractor (and thus a validated +/// `caller_id`); use the legacy [`super::admin::require_admin`] variant +/// when the handler signature is `headers: HeaderMap` instead. +pub async fn require_admin_user( + auth: &AuthApplicationService, + caller_id: Uuid, +) -> Result<(), AppError> { + let user = auth + .get_user_by_id(caller_id) + .await + .map_err(AppError::from)?; + + if user.role != "admin" { + return Err(AppError::new( + StatusCode::FORBIDDEN, + "Admin access required", + "Forbidden", + )); + } + Ok(()) +} diff --git a/tests/api/external_users.hurl b/tests/api/external_users.hurl index 059d72a4..acc87abb 100644 --- a/tests/api/external_users.hurl +++ b/tests/api/external_users.hurl @@ -187,8 +187,63 @@ body contains "{{ext_folder_id}}" # ───────────────────────────────────────────────────────────── -# Step 11 — Second redemption of the same token is rejected. -# single-use is enforced by the SQL UPDATE in +# Step 11 — External-user lockouts (PR 11.1 + ContactsHandler). +# Bob (external) must NOT reach the system address book +# or the per-user profile endpoint. Defense-in-depth on +# top of the PR 6 service-level filter. +# ───────────────────────────────────────────────────────────── + +# 11a — system address book: visible at the catalog level +# (`GET /api/address-books`) for bob? It must NOT list the system entry. +GET {{base_url}}/api/address-books +Authorization: Bearer {{bob_access_token}} + +HTTP 200 +[Asserts] +body not contains "OxiCloud Users" +body not contains "\"id\":\"system\"" + +# 11b — system contacts listing: 403 for bob. +GET {{base_url}}/api/address-books/system/contacts +Authorization: Bearer {{bob_access_token}} + +HTTP 403 + +# 11c — /api/users/{id}: bob cannot query anyone's profile, not even +# Alice's. Service-level external lockout in get_user_profile. +GET {{base_url}}/api/users/{{bob_user_id}} +Authorization: Bearer {{bob_access_token}} + +HTTP 403 + + +# ───────────────────────────────────────────────────────────── +# Step 12 — /api/users/{id} happy path (Alice → Bob). +# Visibility rule: they share a grant, so Alice sees +# Bob's profile (with is_external=true). +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/users/{{bob_user_id}} +Authorization: Bearer {{alice_token}} + +HTTP 200 +[Asserts] +jsonpath "$.id" == "{{bob_user_id}}" +jsonpath "$.is_external" == true + + +# ───────────────────────────────────────────────────────────── +# Step 13 — /api/users/{id} 404 anti-enumeration for an +# unrelated UUID (random Uuid that doesn't exist). +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/users/00000000-0000-0000-0000-deadbeefcafe +Authorization: Bearer {{alice_token}} + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# Step 14 — Second redemption of the same magic-link token is +# rejected — single-use is enforced by the SQL UPDATE in # magic_link_token_pg_repository::mark_used. # ───────────────────────────────────────────────────────────── GET {{magic_url}}