From 5fab0532dc044978027fc33b7cc63c66bd0c168d Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Mon, 1 Jun 2026 20:37:36 +0200 Subject: [PATCH] feat(user): add given_name/family_name auth.users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reflect OIDC schema migration, User entity additions, and three defense-in-depth gaps closed, with all 280 unit tests and 13 Hurl files green infrastructure (lettre + EmailSender port). - Migration migrations/20260612000003_users_username_email_login.sql — adds nullable given_name/family_name columns to auth.users. - User entity (src/domain/entities/user.rs) — has_login_credential() placeholder-check encapsulation, set_username revalidating setter, given/family-name fields + getters/setters, validate_username widened 32→254 and now accepts email shape. from_data_full extended with two new params; all 7 callsites in user_pg_repository.rs updated. - Schema-side legacy guards (src/application/services/auth_application_service.rs) — bumped the duplicated 32-char check in setup_create_admin and admin_create_user to 254 to match. - Gap #1 (subject_group_service.rs) — add_member now rejects external candidates with an audit-logged AccessDenied. Service gained an Arc field, wired through DI. New integration test test_external_user_cannot_be_added_as_member. - Gap #2 (user_repository.rs + auth_ports.rs + user_pg_repository.rs) — list_users/search_users gained an include_external: bool param defaulting effectively to false everywhere internal-user-facing. auth_application_service exposes a new list_users_including_external for the admin surface. - Gap #3 (pg_acl_engine.rs) — expand_user now SELECTs is_external and skips INTERNAL_GROUP_ID for externals; defaults to is_external=true on missing user to fail closed. --- ...60612000003_users_username_email_login.sql | 25 ++++ src/application/ports/auth_ports.rs | 20 ++- .../services/auth_application_service.rs | 29 ++++- .../services/storage_usage_service.rs | 5 +- .../services/subject_group_service.rs | 115 +++++++++++++++++- src/common/di.rs | 5 + src/domain/entities/user.rs | 102 +++++++++++++++- src/domain/repositories/user_repository.rs | 23 +++- .../repositories/pg/user_pg_repository.rs | 86 ++++++++++--- src/infrastructure/services/pg_acl_engine.rs | 34 +++++- 10 files changed, 399 insertions(+), 45 deletions(-) create mode 100644 migrations/20260612000003_users_username_email_login.sql diff --git a/migrations/20260612000003_users_username_email_login.sql b/migrations/20260612000003_users_username_email_login.sql new file mode 100644 index 00000000..dbb7db6e --- /dev/null +++ b/migrations/20260612000003_users_username_email_login.sql @@ -0,0 +1,25 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- Prelude for magic-link external authentication +-- ════════════════════════════════════════════════════════════════════════════ +-- This migration is purely additive — it lands the schema bits needed by the +-- subsequent magic-link work without altering existing rows or behaviour: +-- +-- * `given_name` / `family_name` — optional human-readable identity fields. +-- Populated from OIDC standard claims (given_name, family_name) at JIT +-- provisioning. External users start with both NULL; either side can be +-- filled in later via a profile-edit endpoint. +-- +-- Note on username length: `auth.users.username` is already `TEXT` with no +-- DB-level length constraint, so it can already hold the 254-char RFC 5321 +-- maximum required for email-as-username. The widening happens at the +-- entity-level validator (`User::validate_username`), not the schema. + +ALTER TABLE auth.users + ADD COLUMN IF NOT EXISTS given_name TEXT NULL, + ADD COLUMN IF NOT EXISTS family_name TEXT NULL; + +COMMENT ON COLUMN auth.users.given_name IS + 'Optional first/given name. Populated from OIDC standard claim `given_name` at JIT provisioning; settable via profile-edit endpoint. NULL until explicitly set.'; + +COMMENT ON COLUMN auth.users.family_name IS + 'Optional last/family name. Populated from OIDC standard claim `family_name` at JIT provisioning; settable via profile-edit endpoint. NULL until explicitly set.'; diff --git a/src/application/ports/auth_ports.rs b/src/application/ports/auth_ports.rs index 1415380e..36845a10 100644 --- a/src/application/ports/auth_ports.rs +++ b/src/application/ports/auth_ports.rs @@ -91,11 +91,25 @@ pub trait UserStoragePort: Send + Sync + 'static { usage_bytes: i64, ) -> Result<(), DomainError>; - /// Lists users with pagination - async fn list_users(&self, limit: i64, offset: i64) -> Result, DomainError>; + /// Lists users with pagination. `include_external` defaults to `false` + /// at every call site that surfaces users to other internal users + /// (autocomplete, sharee search, etc.); only the admin management UI + /// passes `true`. See [`UserRepository::list_users`] for the rationale. + async fn list_users( + &self, + limit: i64, + offset: i64, + include_external: bool, + ) -> Result, DomainError>; /// Searches users by username or email (SQL ILIKE) with a limit. - async fn search_users(&self, query: &str, limit: i64) -> Result, DomainError>; + /// See [`list_users`] for the meaning of `include_external`. + async fn search_users( + &self, + query: &str, + limit: i64, + include_external: bool, + ) -> Result, DomainError>; /// Lists users by role (e.g., "admin" or "user") async fn list_users_by_role(&self, role: &str) -> Result, DomainError>; diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index f008a499..7038b2ec 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -312,11 +312,11 @@ impl AuthApplicationService { password: String, ) -> Result { // Validate username - if username.len() < 3 || username.len() > 32 { + if username.len() < 3 || username.len() > 254 { return Err(DomainError::new( ErrorKind::InvalidInput, "User", - "Username must be between 3 and 32 characters".to_string(), + "Username must be between 3 and 254 characters".to_string(), )); } @@ -773,13 +773,30 @@ impl AuthApplicationService { Ok(admin_users.len() as i64) } + /// Lists internal users only. External (grant-only) users are filtered + /// out so that internal-user surfaces — system address book, OCS + /// sharee search, etc. — never expose external identities. Admin + /// surfaces that need the full list should call + /// [`list_users_including_external`] instead. pub async fn list_users(&self, limit: i64, offset: i64) -> Result, DomainError> { - let users = self.user_storage.list_users(limit, offset).await?; + let users = self.user_storage.list_users(limit, offset, false).await?; Ok(users.into_iter().map(UserDto::from).collect()) } + /// Admin-only: lists users including external (grant-only) recipients. + /// Used by the admin user-management UI. + pub async fn list_users_including_external( + &self, + limit: i64, + offset: i64, + ) -> Result, DomainError> { + let users = self.user_storage.list_users(limit, offset, true).await?; + Ok(users.into_iter().map(UserDto::from).collect()) + } + + /// Searches internal users only. See [`list_users`] for the rationale. pub async fn search_users(&self, query: &str, limit: i64) -> Result, DomainError> { - let users = self.user_storage.search_users(query, limit).await?; + let users = self.user_storage.search_users(query, limit, false).await?; Ok(users.into_iter().map(UserDto::from).collect()) } @@ -793,11 +810,11 @@ impl AuthApplicationService { dto: crate::application::dtos::settings_dto::AdminCreateUserDto, ) -> Result { // Validate username length - if dto.username.len() < 3 || dto.username.len() > 32 { + if dto.username.len() < 3 || dto.username.len() > 254 { return Err(DomainError::new( ErrorKind::InvalidInput, "User", - "Username must be between 3 and 32 characters".to_string(), + "Username must be between 3 and 254 characters".to_string(), )); } diff --git a/src/application/services/storage_usage_service.rs b/src/application/services/storage_usage_service.rs index 0451f251..3aed2399 100644 --- a/src/application/services/storage_usage_service.rs +++ b/src/application/services/storage_usage_service.rs @@ -127,7 +127,10 @@ impl StorageUsagePort for StorageUsageService { info!("Starting batch update of all users' storage usage"); // Get the list of all users - let users = self.user_repository.list_users(1000, 0).await?; + // include_external=false — external users carry no storage by + // construction (DB CHECK `users_external_no_storage`), so there's + // nothing to compute for them. + let users = self.user_repository.list_users(1000, 0, false).await?; let mut update_tasks = Vec::new(); diff --git a/src/application/services/subject_group_service.rs b/src/application/services/subject_group_service.rs index 12684b0f..812b41f4 100644 --- a/src/application/services/subject_group_service.rs +++ b/src/application/services/subject_group_service.rs @@ -23,16 +23,33 @@ use crate::domain::entities::subject_group::{ use crate::domain::repositories::subject_group_repository::{ SubjectGroupRepository, SubjectGroupRepositoryError, }; -use crate::infrastructure::repositories::pg::SubjectGroupPgRepository; +use crate::domain::repositories::user_repository::{UserRepository, UserRepositoryError}; +use crate::infrastructure::repositories::pg::{SubjectGroupPgRepository, UserPgRepository}; pub struct SubjectGroupService { repo: Arc, pool: Arc, + /// Looked up by `add_member` to refuse external-user candidates. + /// External users are grant-only recipients; placing them in a + /// subject group would let any later group-grant on internal + /// resources silently leak access to them. `UserPgRepository` rather + /// than `Arc` because the port's `async fn`s + /// make it not dyn-compatible (matches the convention used by other + /// services in this layer). + user_storage: Arc, } impl SubjectGroupService { - pub fn new(repo: Arc, pool: Arc) -> Self { - Self { repo, pool } + pub fn new( + repo: Arc, + pool: Arc, + user_storage: Arc, + ) -> Self { + Self { + repo, + pool, + user_storage, + } } /// Create a new group. Validates the name (RFC 5321 local-part shape) @@ -249,6 +266,39 @@ impl SubjectGroupService { )); } + // Refuse external-user candidates. External users are grant-only + // recipients; placing one in a subject group would let any later + // group-grant on an internal resource silently leak access. + // Mirrors the no-external-admins enforcement style in + // `User::new_external`. + if let GroupMember::User(uid) = member { + match UserRepository::get_user_by_id(&*self.user_storage, uid).await { + Ok(user) if user.is_external() => { + tracing::info!( + target: "audit", + event = "group.external_member_rejected", + group_id = %group_id, + user_id = %uid, + by = %caller_id, + ); + return Err(DomainError::new( + ErrorKind::AccessDenied, + "SubjectGroup", + "External users cannot be members of subject groups; share resources with them directly".to_string(), + )); + } + Ok(_) => { /* internal user — proceed */ } + Err(UserRepositoryError::NotFound(_)) => { + return Err(DomainError::new( + ErrorKind::NotFound, + "SubjectGroup", + format!("user {} not found", uid), + )); + } + Err(e) => return Err(DomainError::from(e)), + } + } + self.repo .add_member(group_id, member, caller_id) .await @@ -409,7 +459,8 @@ mod integration_tests { ensure_clean_test_db(&pool).await; let pool = Arc::new(pool); let repo = Arc::new(SubjectGroupPgRepository::new(pool.clone())); - SubjectGroupService::new(repo, pool) + let user_storage = Arc::new(UserPgRepository::new(pool.clone())); + SubjectGroupService::new(repo, pool, user_storage) } async fn first_admin(pool: &sqlx::PgPool) -> Uuid { @@ -520,6 +571,62 @@ mod integration_tests { assert_eq!(post, 0, "grants must be revoked atomically with the group"); } + // ── External users cannot be added as subject group members ───────────── + // + // Defense-in-depth gap #1 closed in PR 6: external users (grant-only + // recipients) must never appear inside a subject group, because the + // group could later be granted access to internal resources. + #[tokio::test] + async fn test_external_user_cannot_be_added_as_member() { + let svc = make_service().await; + let admin = first_admin(&svc.pool).await; + + // Insert an external user directly (no public test helper for this yet). + let external_id = Uuid::new_v4(); + sqlx::query( + "INSERT INTO auth.users ( + id, username, email, password_hash, role, + storage_quota_bytes, storage_used_bytes, + created_at, updated_at, active, is_external + ) VALUES ($1, $2, $3, '__EXTERNAL_NO_PASSWORD__', 'user'::auth.userrole, + 0, 0, NOW(), NOW(), TRUE, TRUE)", + ) + .bind(external_id) + .bind(format!("ext-{}@example.com", &external_id.to_string()[..8])) + .bind(format!("ext-{}@example.com", &external_id.to_string()[..8])) + .execute(svc.pool.as_ref()) + .await + .expect("seed external user"); + + let group = svc + .create(&rand_name("ext-reject"), None, admin) + .await + .unwrap(); + + let err = svc + .add_member(group.id, GroupMember::User(external_id), admin) + .await + .expect_err("external user must be rejected as a group member"); + assert_eq!(err.kind, ErrorKind::AccessDenied); + assert!( + err.message.contains("External users"), + "error message should explain the rejection; got: {}", + err.message + ); + + // Verify the membership did NOT land in the table. + let count: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM auth.subject_group_members + WHERE group_id = $1 AND member_user_id = $2", + ) + .bind(group.id) + .bind(external_id) + .fetch_one(svc.pool.as_ref()) + .await + .unwrap(); + assert_eq!(count, 0, "external user must not appear in members table"); + } + // Bonus: service-layer name validation runs before the DB round-trip. #[tokio::test] async fn test_service_rejects_invalid_name_locally() { diff --git a/src/common/di.rs b/src/common/di.rs index 7978d166..bcc17c1f 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -891,6 +891,11 @@ impl AppServiceFactory { crate::application::services::subject_group_service::SubjectGroupService::new( subject_group_repo.clone(), pool.clone(), + Arc::new( + crate::infrastructure::repositories::pg::UserPgRepository::new( + pool.clone(), + ), + ), ), )), }; diff --git a/src/domain/entities/user.rs b/src/domain/entities/user.rs index ad1b795c..cbe7ef84 100644 --- a/src/domain/entities/user.rs +++ b/src/domain/entities/user.rs @@ -43,6 +43,14 @@ pub struct User { /// `application/ports/user_lifecycle.rs`. The DB CHECK constraint /// `users_external_no_storage` is the schema-level safety net. is_external: bool, + /// Optional human-readable first/given name. Populated from OIDC + /// standard claim `given_name` at JIT provisioning, or via the + /// profile-edit endpoint. External users start with `None`. + given_name: Option, + /// Optional human-readable last/family name. Populated from OIDC + /// standard claim `family_name` at JIT provisioning, or via the + /// profile-edit endpoint. External users start with `None`. + family_name: Option, } impl User { @@ -53,7 +61,7 @@ impl User { /// out of the domain layer. /// /// # Arguments - /// * `username` - User's username (3-32 characters) + /// * `username` - User's username (3-254 characters; may be an email) /// * `email` - User's email address /// * `password_hash` - Pre-hashed password (from PasswordHasherPort) /// * `role` - User's role @@ -93,6 +101,8 @@ impl User { oidc_subject: None, image: None, is_external: false, + given_name: None, + family_name: None, }) } @@ -124,6 +134,8 @@ impl User { oidc_subject: Some(oidc_subject), image: None, is_external: false, + given_name: None, + family_name: None, }) } @@ -165,6 +177,8 @@ impl User { oidc_subject: None, image: None, is_external: true, + given_name: None, + family_name: None, }) } @@ -204,6 +218,8 @@ impl User { // sessions take a different path that hydrates from DB via // `from_data_full`. is_external: false, + given_name: None, + family_name: None, } } @@ -224,6 +240,8 @@ impl User { oidc_subject: Option, image: Option, is_external: bool, + given_name: Option, + family_name: Option, ) -> Self { Self { id, @@ -241,6 +259,8 @@ impl User { oidc_subject, image, is_external, + given_name, + family_name, } } @@ -309,16 +329,62 @@ impl User { self.is_external } + pub fn given_name(&self) -> Option<&str> { + self.given_name.as_deref() + } + + pub fn family_name(&self) -> Option<&str> { + self.family_name.as_deref() + } + pub fn set_image(&mut self, image: Option) { self.image = image; self.updated_at = Utc::now(); } + pub fn set_given_name(&mut self, given_name: Option) { + self.given_name = given_name; + self.updated_at = Utc::now(); + } + + pub fn set_family_name(&mut self, family_name: Option) { + self.family_name = family_name; + self.updated_at = Utc::now(); + } + + /// Mutate the username after creation. Runs the same validation as the + /// constructor — callers must still ensure uniqueness at the repo + /// level. Bumps `updated_at`. Used by the post-create profile-edit + /// endpoint so a user invited with `username = email` can switch to a + /// shorter handle later. The home folder name is NOT renamed: it was + /// display text at creation; the folder is owned by `user_id`. + pub fn set_username(&mut self, new_username: String) -> UserResult<()> { + Self::validate_username(&new_username)?; + self.username = new_username; + self.updated_at = Utc::now(); + Ok(()) + } + /// Returns true if this is an OIDC-only user (no password) pub fn is_oidc_user(&self) -> bool { self.oidc_provider.is_some() } + /// Returns true iff this user has any non-magic-link authentication + /// method available — either a real (non-placeholder) password hash, + /// or a linked OIDC subject. Magic-link auto-authentication is only + /// offered for accounts without any of these. + /// + /// The placeholder-string approach is a known smell; a future + /// `auth.user_auth_methods` side-table will replace it. Migrating that + /// refactor touches only this method's body — every magic-link + /// eligibility check goes through here. + pub fn has_login_credential(&self) -> bool { + let has_password = self.password_hash != "__EXTERNAL_NO_PASSWORD__" + && self.password_hash != "__OIDC_NO_PASSWORD__"; + has_password || self.oidc_subject.is_some() + } + /// Update the password hash. /// /// The new password should be hashed externally using PasswordHasherPort @@ -355,15 +421,39 @@ impl User { // ── Shared validation helpers ────────────────────────────────────── - /// Usernames must be 3-32 chars and contain only ASCII alphanumerics, - /// hyphens, underscores, and dots. This prevents XSS payloads like - /// `` from being stored as usernames. + /// Usernames must be 3-254 chars. Two accepted shapes: + /// + /// - **Traditional**: ASCII alphanumerics, hyphens, underscores, and + /// dots. No leading/trailing dot or hyphen. Capped at 254 chars + /// (well above the historical 32-char limit, but still safe — the + /// real upper bound is RFC 5321's email cap for the email shape). + /// - **Email-as-username**: must contain `@` and pass `validate_email`. + /// External users created from invite-by-email get their normalized + /// email as username; internal users may opt into this if they + /// prefer their email as their handle. + /// + /// Both shapes prevent XSS payloads like `` from being + /// stored as usernames — the traditional shape via the explicit + /// character set, the email shape via `validate_email`'s rejection of + /// `<`, `>`, quotes, whitespace, etc. fn validate_username(username: &str) -> UserResult<()> { - if username.len() < 3 || username.len() > 32 { + if username.len() < 3 || username.len() > 254 { return Err(UserError::InvalidUsername( - "Username must be between 3 and 32 characters".to_string(), + "Username must be between 3 and 254 characters".to_string(), )); } + + if username.contains('@') { + // Email shape — defer to the email validator (which checks the + // forbidden-character set and the local-part / domain structure). + return Self::validate_email(username).map_err(|e| match e { + UserError::ValidationError(m) => { + UserError::InvalidUsername(format!("Invalid email-as-username: {}", m)) + } + other => other, + }); + } + if !username .chars() .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') diff --git a/src/domain/repositories/user_repository.rs b/src/domain/repositories/user_repository.rs index eaae091c..81816371 100644 --- a/src/domain/repositories/user_repository.rs +++ b/src/domain/repositories/user_repository.rs @@ -67,11 +67,28 @@ pub trait UserRepository: Send + Sync + 'static { /// Updates the last login date async fn update_last_login(&self, user_id: Uuid) -> UserRepositoryResult<()>; - /// Lists users with pagination - async fn list_users(&self, limit: i64, offset: i64) -> UserRepositoryResult>; + /// Lists users with pagination. + /// + /// `include_external` controls whether external (grant-only) users + /// appear in the result. Default callers should pass `false` so + /// external users stay invisible to internal-user surfaces (system + /// address book autocomplete, sharee search, etc.). Only the admin + /// management UI should request `true`. + async fn list_users( + &self, + limit: i64, + offset: i64, + include_external: bool, + ) -> UserRepositoryResult>; /// Searches users by username or email (SQL ILIKE) with a limit. - async fn search_users(&self, query: &str, limit: i64) -> UserRepositoryResult>; + /// See [`list_users`] for the meaning of `include_external`. + async fn search_users( + &self, + query: &str, + limit: i64, + include_external: bool, + ) -> UserRepositoryResult>; /// Activates or deactivates a user async fn set_user_active_status(&self, user_id: Uuid, active: bool) diff --git a/src/infrastructure/repositories/pg/user_pg_repository.rs b/src/infrastructure/repositories/pg/user_pg_repository.rs index d1c31e18..56daffd8 100644 --- a/src/infrastructure/repositories/pg/user_pg_repository.rs +++ b/src/infrastructure/repositories/pg/user_pg_repository.rs @@ -95,10 +95,11 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, is_external + oidc_provider, oidc_subject, is_external, + given_name, family_name ) VALUES ( $1, $2, $3, $4, $5::auth.userrole, $6, $7, $8, $9, $10, $11, - $12, $13, $14 + $12, $13, $14, $15, $16 ) RETURNING * "#, @@ -117,6 +118,8 @@ impl UserRepository for UserPgRepository { .bind(user_clone.oidc_provider()) .bind(user_clone.oidc_subject()) .bind(user_clone.is_external()) + .bind(user_clone.given_name()) + .bind(user_clone.family_name()) .execute(&mut **tx) .await .map_err(Self::map_sqlx_error)?; @@ -140,7 +143,8 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external + oidc_provider, oidc_subject, image, is_external, + given_name, family_name FROM auth.users WHERE id = $1 "#, @@ -173,6 +177,8 @@ impl UserRepository for UserPgRepository { row.get("oidc_subject"), row.get("image"), row.get("is_external"), + row.get("given_name"), + row.get("family_name"), )) } @@ -184,7 +190,8 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external + oidc_provider, oidc_subject, image, is_external, + given_name, family_name FROM auth.users WHERE username = $1 "#, @@ -217,6 +224,8 @@ impl UserRepository for UserPgRepository { row.get("oidc_subject"), row.get("image"), row.get("is_external"), + row.get("given_name"), + row.get("family_name"), )) } @@ -228,7 +237,8 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external + oidc_provider, oidc_subject, image, is_external, + given_name, family_name FROM auth.users WHERE email = $1 "#, @@ -261,6 +271,8 @@ impl UserRepository for UserPgRepository { row.get("oidc_subject"), row.get("image"), row.get("is_external"), + row.get("given_name"), + row.get("family_name"), )) } @@ -285,7 +297,9 @@ impl UserRepository for UserPgRepository { updated_at = $8, last_login_at = $9, active = $10, - image = $11 + image = $11, + given_name = $12, + family_name = $13 WHERE id = $1 "#, ) @@ -300,6 +314,8 @@ impl UserRepository for UserPgRepository { .bind(user_clone.last_login_at()) .bind(user_clone.is_active()) .bind(user_clone.image()) + .bind(user_clone.given_name()) + .bind(user_clone.family_name()) .execute(&mut **tx) .await .map_err(Self::map_sqlx_error)?; @@ -359,21 +375,29 @@ impl UserRepository for UserPgRepository { } /// Lists users with pagination - async fn list_users(&self, limit: i64, offset: i64) -> UserRepositoryResult> { + async fn list_users( + &self, + limit: i64, + offset: i64, + include_external: bool, + ) -> UserRepositoryResult> { let rows = sqlx::query( r#" SELECT id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external + oidc_provider, oidc_subject, image, is_external, + given_name, family_name FROM auth.users + WHERE ($3 OR is_external = FALSE) ORDER BY created_at DESC LIMIT $1 OFFSET $2 "#, ) .bind(limit) .bind(offset) + .bind(include_external) .fetch_all(&*self.pool) .await .map_err(Self::map_sqlx_error)?; @@ -404,6 +428,8 @@ impl UserRepository for UserPgRepository { row.get("oidc_subject"), row.get("image"), row.get("is_external"), + row.get("given_name"), + row.get("family_name"), ) }) .collect(); @@ -411,7 +437,12 @@ impl UserRepository for UserPgRepository { Ok(users) } - async fn search_users(&self, query: &str, limit: i64) -> UserRepositoryResult> { + async fn search_users( + &self, + query: &str, + limit: i64, + include_external: bool, + ) -> UserRepositoryResult> { let pattern = format!("%{}%", query); let rows = sqlx::query( r#" @@ -419,15 +450,18 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external + oidc_provider, oidc_subject, image, is_external, + given_name, family_name FROM auth.users - WHERE username ILIKE $1 OR email ILIKE $1 + WHERE (username ILIKE $1 OR email ILIKE $1) + AND ($3 OR is_external = FALSE) ORDER BY username LIMIT $2 "#, ) .bind(&pattern) .bind(limit) + .bind(include_external) .fetch_all(&*self.pool) .await .map_err(Self::map_sqlx_error)?; @@ -457,6 +491,8 @@ impl UserRepository for UserPgRepository { row.get("oidc_subject"), row.get("image"), row.get("is_external"), + row.get("given_name"), + row.get("family_name"), ) }) .collect(); @@ -543,7 +579,8 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external + oidc_provider, oidc_subject, image, is_external, + given_name, family_name FROM auth.users WHERE role::text = $1 ORDER BY created_at DESC @@ -580,6 +617,8 @@ impl UserRepository for UserPgRepository { row.get("oidc_subject"), row.get("image"), row.get("is_external"), + row.get("given_name"), + row.get("family_name"), ) }) .collect(); @@ -615,7 +654,8 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external + oidc_provider, oidc_subject, image, is_external, + given_name, family_name FROM auth.users WHERE oidc_provider = $1 AND oidc_subject = $2 "#, @@ -648,6 +688,8 @@ impl UserRepository for UserPgRepository { row.get("oidc_subject"), row.get("image"), row.get("is_external"), + row.get("given_name"), + row.get("family_name"), )) } @@ -757,14 +799,24 @@ impl UserStoragePort for UserPgRepository { .map_err(DomainError::from) } - async fn list_users(&self, limit: i64, offset: i64) -> Result, DomainError> { - UserRepository::list_users(self, limit, offset) + async fn list_users( + &self, + limit: i64, + offset: i64, + include_external: bool, + ) -> Result, DomainError> { + UserRepository::list_users(self, limit, offset, include_external) .await .map_err(DomainError::from) } - async fn search_users(&self, query: &str, limit: i64) -> Result, DomainError> { - UserRepository::search_users(self, query, limit) + async fn search_users( + &self, + query: &str, + limit: i64, + include_external: bool, + ) -> Result, DomainError> { + UserRepository::search_users(self, query, limit, include_external) .await .map_err(DomainError::from) } diff --git a/src/infrastructure/services/pg_acl_engine.rs b/src/infrastructure/services/pg_acl_engine.rs index 5386514c..b24c3d57 100644 --- a/src/infrastructure/services/pg_acl_engine.rs +++ b/src/infrastructure/services/pg_acl_engine.rs @@ -127,7 +127,16 @@ impl PgAclEngine { /// Expand a user subject into the set of subject UUIDs that should match /// in `access_grants`: the user's own UUID, every group the user is - /// transitively a member of, and the implicit `INTERNAL_GROUP_ID`. + /// transitively a member of, and (for internal users only) the implicit + /// `INTERNAL_GROUP_ID`. + /// + /// External users (`auth.users.is_external = TRUE`) do NOT belong to + /// the Internal virtual group — they are grant-only recipients whose + /// access is determined exclusively by explicit grants on their + /// `user_id` or on subject groups they were explicitly added to. + /// `SubjectGroupService::add_member` rejects externals, so the only + /// path by which an external user reaches a resource is via a + /// `subject_type='user'` grant. /// /// This is the **only** place transitive membership is walked. A future /// closure-table swap-in (Option 3 in the design doc) replaces just the @@ -147,10 +156,25 @@ impl PgAclEngine { let mut set: HashSet = HashSet::new(); set.insert(user_id); - // The Internal virtual group: implicit membership for every - // authenticated user. Once the external-users work lands this will - // narrow to `if !user.is_external { ... }`. - set.insert(INTERNAL_GROUP_ID); + + // Look up `is_external` for the caller — external users do not + // belong to the Internal virtual group. Unknown user (no row) is + // treated as external to fail closed: a deleted or bogus user_id + // must not gain implicit Internal membership. + counters.sql_queries.fetch_add(1, Ordering::Relaxed); + let is_external: bool = + sqlx::query_scalar("SELECT is_external FROM auth.users WHERE id = $1") + .bind(user_id) + .fetch_optional(self.pool.as_ref()) + .await + .map_err(|e| { + DomainError::internal_error("PgAcl", format!("lookup is_external: {e}")) + })? + .unwrap_or(true); + + if !is_external { + set.insert(INTERNAL_GROUP_ID); + } if let Some(repo) = &self.group_repo { counters.sql_queries.fetch_add(1, Ordering::Relaxed);