From 6763f2ca9ea23a650763527adfc59e3df99695cf Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 2 Jun 2026 13:12:29 +0200 Subject: [PATCH] fix(/api/users): external users can only query themself and their granters --- .../services/auth_application_service.rs | 91 ++++++++++--------- tests/api/external_users.hurl | 29 +++++- 2 files changed, 72 insertions(+), 48 deletions(-) diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 33418cc5..d18f5e27 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -904,23 +904,29 @@ impl AuthApplicationService { /// 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, evaluated top-to-bottom: + /// 1. **Self lookup** — `caller_id == target_id` always succeeds. + /// 2. **Shared-grant relationship** — caller and target appear + /// together on at least one row of `storage.access_grants`, + /// either direction (caller-as-granter / target-as-subject, + /// or target-as-granter / caller-as-subject). Applies to both + /// internal and external callers. This is what lets an + /// external user resolve the display name + photo of the + /// internal user who shared a folder with them — the + /// `granted_by` column on the grant Bob received is Alice's + /// user_id, and SharedWithMe needs to render her vignette. + /// 3. **External callers stop here.** Any remaining check would + /// let them enumerate the user directory; they have no + /// legitimate need beyond resolving people they're already in + /// a grant relationship with. + /// 4. *(Internal callers only)* Target is internal AND + /// `expose_system_users` is on → already broadly visible via + /// the system address book; no extra check. + /// 5. *(Internal callers only)* Caller is admin → always visible. + /// 6. Anything else → `NotFound`. /// - /// 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`. + /// Subject-group co-membership is intentionally NOT a visibility + /// path in v1; can be added later if a concrete need surfaces. pub async fn get_user_profile( &self, caller_id: Uuid, @@ -928,22 +934,9 @@ impl AuthApplicationService { 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). + // (1) Self. if caller_id == target_id { return Ok(UserDto::from(caller)); } @@ -963,20 +956,9 @@ impl AuthApplicationService { 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. + // (2) Shared-grant relationship — works for both internal and + // external callers. LIMIT 1 + the (granted_by) and + // (subject_type, subject_id) indexes keep this cheap. let related: Option = sqlx::query_scalar( r#" SELECT 1 @@ -998,7 +980,26 @@ impl AuthApplicationService { return Ok(UserDto::from(target)); } - // No relationship — anti-enumeration NotFound. + // (3) External callers stop here — no directory enumeration. + if caller.is_external() { + return Err(DomainError::new( + ErrorKind::NotFound, + "User", + "User not found", + )); + } + + // (4) Internal target + system-address-book exposed: already public. + if !target.is_external() && expose_system_users { + return Ok(UserDto::from(target)); + } + + // (5) Admin caller: always visible. + if caller.role() == UserRole::Admin { + return Ok(UserDto::from(target)); + } + + // (6) No relationship — anti-enumeration NotFound. Err(DomainError::new( ErrorKind::NotFound, "User", diff --git a/tests/api/external_users.hurl b/tests/api/external_users.hurl index acc87abb..4fec89a7 100644 --- a/tests/api/external_users.hurl +++ b/tests/api/external_users.hurl @@ -23,6 +23,7 @@ Content-Type: application/json HTTP 200 [Captures] alice_token: jsonpath "$.access_token" +alice_user_id: jsonpath "$.user.id" GET {{base_url}}/api/folders Authorization: Bearer {{alice_token}} @@ -209,12 +210,34 @@ 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. +# 11c — /api/users/{id}: bob CAN look up his own profile (self-lookup +# is the first allow rule) so the SharedWithMe view can show +# his own avatar in the user menu. GET {{base_url}}/api/users/{{bob_user_id}} Authorization: Bearer {{bob_access_token}} -HTTP 403 +HTTP 200 +[Asserts] +jsonpath "$.id" == "{{bob_user_id}}" +jsonpath "$.is_external" == true + +# 11d — bob CAN look up Alice (his granter) — shared-grant relationship +# lets the external recipient resolve the sharer's display name + +# photo for the SharedWithMe view's owner column. +GET {{base_url}}/api/users/{{alice_user_id}} +Authorization: Bearer {{bob_access_token}} + +HTTP 200 +[Asserts] +jsonpath "$.id" == "{{alice_user_id}}" +jsonpath "$.is_external" == false + +# 11e — bob CANNOT enumerate unrelated users. A random UUID returns 404 +# (anti-enumeration; same response as "user doesn't exist"). +GET {{base_url}}/api/users/00000000-0000-0000-0000-baadbeef1234 +Authorization: Bearer {{bob_access_token}} + +HTTP 404 # ─────────────────────────────────────────────────────────────