feat(user): add given_name/family_name auth.users
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<UserPgRepository> 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.
This commit is contained in:
@@ -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<Vec<User>, 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<Vec<User>, DomainError>;
|
||||
|
||||
/// Searches users by username or email (SQL ILIKE) with a limit.
|
||||
async fn search_users(&self, query: &str, limit: i64) -> Result<Vec<User>, DomainError>;
|
||||
/// See [`list_users`] for the meaning of `include_external`.
|
||||
async fn search_users(
|
||||
&self,
|
||||
query: &str,
|
||||
limit: i64,
|
||||
include_external: bool,
|
||||
) -> Result<Vec<User>, DomainError>;
|
||||
|
||||
/// Lists users by role (e.g., "admin" or "user")
|
||||
async fn list_users_by_role(&self, role: &str) -> Result<Vec<User>, DomainError>;
|
||||
|
||||
@@ -312,11 +312,11 @@ impl AuthApplicationService {
|
||||
password: String,
|
||||
) -> Result<UserDto, DomainError> {
|
||||
// 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<Vec<UserDto>, 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<Vec<UserDto>, 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<Vec<UserDto>, 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<UserDto, DomainError> {
|
||||
// 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(),
|
||||
));
|
||||
}
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
|
||||
@@ -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<SubjectGroupPgRepository>,
|
||||
pool: Arc<PgPool>,
|
||||
/// 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<dyn UserStoragePort>` 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<UserPgRepository>,
|
||||
}
|
||||
|
||||
impl SubjectGroupService {
|
||||
pub fn new(repo: Arc<SubjectGroupPgRepository>, pool: Arc<PgPool>) -> Self {
|
||||
Self { repo, pool }
|
||||
pub fn new(
|
||||
repo: Arc<SubjectGroupPgRepository>,
|
||||
pool: Arc<PgPool>,
|
||||
user_storage: Arc<UserPgRepository>,
|
||||
) -> 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() {
|
||||
|
||||
Reference in New Issue
Block a user