diff --git a/migrations/20260620000000_username_optional_no_email_shape.sql b/migrations/20260620000000_username_optional_no_email_shape.sql new file mode 100644 index 00000000..336abb8f --- /dev/null +++ b/migrations/20260620000000_username_optional_no_email_shape.sql @@ -0,0 +1,56 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- Auth simplification — username + password_hash become nullable +-- ════════════════════════════════════════════════════════════════════════════ +-- This migration completes the auth-simplification design (PR 16 of the +-- auth-simplification plan): +-- +-- * `username` becomes NULLABLE. External users get NULL; internal users +-- keep their handles. Multiple NULLs coexist under the existing UNIQUE +-- index (Postgres allows this by default). +-- * `password_hash` becomes NULLABLE. The sentinel strings +-- `__EXTERNAL_NO_PASSWORD__` and `__OIDC_NO_PASSWORD__` are NULL'd out; +-- the entity-level checks switch from string comparison to +-- `Option::is_some`. +-- * Username format CHECK tightens: 2-64 chars, no `@` (banning `@` +-- keeps the username and email namespaces provably disjoint and +-- prevents the cross-collision attack class described in the +-- auth-simplification plan). +-- +-- Forward-only — do NOT squash with `20260612000003_users_username_email_login.sql`. +-- That migration has already been applied to dev / CI environments; +-- squashing would invalidate `_sqlx_migrations` checksums and lock down +-- the migration runner. + +-- 1. Drop NOT NULL on the two columns we're loosening. +ALTER TABLE auth.users ALTER COLUMN username DROP NOT NULL; +ALTER TABLE auth.users ALTER COLUMN password_hash DROP NOT NULL; + +-- 2. NULL out the email-shaped usernames that PR 9 stamped onto external +-- users. Their identity is the email column; the username field carried +-- a redundant duplicate that was only ever used to satisfy NOT NULL. +UPDATE auth.users + SET username = NULL + WHERE is_external = TRUE; + +-- 3. NULL out the placeholder password_hash sentinels. After this migration +-- `password_hash IS NULL` means "no password set"; non-NULL means +-- "argon2 hash". No more string-comparison gymnastics in the entity. +UPDATE auth.users + SET password_hash = NULL + WHERE password_hash IN ('__EXTERNAL_NO_PASSWORD__', '__OIDC_NO_PASSWORD__'); + +-- 4. Tighten username format. The CHECK fires only when username IS NOT +-- NULL (existing externals stay NULL; new email-shaped values are +-- rejected at write time). Length 2-64 matches the entity validator's +-- new range. Existing internal usernames are all ≥3 and ≤32 chars, +-- so this is non-breaking for current data. +ALTER TABLE auth.users + ADD CONSTRAINT users_username_shape_v2 + CHECK (username IS NULL + OR (username !~ '@' AND char_length(username) BETWEEN 2 AND 64)); + +COMMENT ON COLUMN auth.users.username IS + 'Optional handle (2-64 chars, no `@`). NULL for external users and for users who haven''t claimed one yet. UNIQUE allows multiple NULLs by default.'; + +COMMENT ON COLUMN auth.users.password_hash IS + 'Argon2 password hash. NULL when the user has no password (externals, OIDC-only users, or post-PR-18 email-only signups awaiting their welcome magic-link).'; diff --git a/src/application/dtos/user_dto.rs b/src/application/dtos/user_dto.rs index 8f7a759c..a79cad37 100644 --- a/src/application/dtos/user_dto.rs +++ b/src/application/dtos/user_dto.rs @@ -7,7 +7,13 @@ use uuid::Uuid; #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] pub struct UserDto { pub id: String, - pub username: String, + /// Optional handle. `None` for users who have not claimed one + /// (externals, fresh email-only signups). Frontend display callers + /// should walk `username → given/family → email` as their fallback + /// chain. Omitted from JSON when None (consistent with the existing + /// given_name / family_name fields). + #[serde(skip_serializing_if = "Option::is_none")] + pub username: Option, pub email: String, pub role: String, pub storage_quota_bytes: i64, @@ -40,7 +46,7 @@ impl From for UserDto { fn from(user: User) -> Self { Self { id: user.id().to_string(), - username: user.username().to_string(), + username: user.username().map(str::to_string), email: user.email().to_string(), role: format!("{}", user.role()), storage_quota_bytes: user.storage_quota_bytes(), diff --git a/src/application/services/app_password_service.rs b/src/application/services/app_password_service.rs index 98459f63..ee02434d 100644 --- a/src/application/services/app_password_service.rs +++ b/src/application/services/app_password_service.rs @@ -132,7 +132,7 @@ impl AppPasswordService { // Fetch user for the username (needed for Basic Auth instructions) let user = self.user_repo.get_user_by_id(user_id).await?; - let username = user.username().to_string(); + let username = user.username().unwrap_or("").to_string(); // Generate the plain-text token let plain_token = Self::generate_token(); @@ -349,7 +349,7 @@ impl AppPasswordService { let result = CachedBasicAuthResult { user_id: user.id(), - username: user.username().to_string(), + username: user.username().unwrap_or("").to_string(), email: user.email().to_string(), role: user.role().to_string(), }; diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 66d8a903..eedb98bc 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -304,15 +304,23 @@ impl AuthApplicationService { let password_hash = self.password_hasher.hash_password(&dto.password).await?; // Create user with the pre-generated hash - let user = User::new(dto.username.clone(), dto.email, password_hash, role, quota).map_err( - |e| { - DomainError::new( - ErrorKind::InvalidInput, - "User", - format!("Error creating user: {}", e), - ) - }, - )?; + let user = User::new( + dto.email, + Some(dto.username.clone()), + Some(password_hash), + None, + None, + role, + quota, + false, + ) + .map_err(|e| { + DomainError::new( + ErrorKind::InvalidInput, + "User", + format!("Error creating user: {}", e), + ) + })?; // Save user let created_user = self.user_storage.create_user(user).await?; @@ -387,7 +395,17 @@ impl AuthApplicationService { let quota = self.capped_quota(&role); let password_hash = self.password_hasher.hash_password(&password).await?; - let user = User::new(username.clone(), email, password_hash, role, quota).map_err(|e| { + let user = User::new( + email, + Some(username.clone()), + Some(password_hash), + None, + None, + role, + quota, + false, + ) + .map_err(|e| { DomainError::new( ErrorKind::InvalidInput, "User", @@ -442,9 +460,9 @@ impl AuthApplicationService { event = "auth.login_rejected", reason = "account_deactivated", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), "🔐 login rejected: account deactivated for '{}'", - user.username(), + user.display_for_audit(), ); return Err(DomainError::new( ErrorKind::AccessDenied, @@ -453,10 +471,29 @@ impl AuthApplicationService { )); } - // Verify password using the injected hasher + // Verify password using the injected hasher. If the user has no + // password configured (externals, OIDC-only), short-circuit to + // "invalid credentials" — the password-login path never accepts + // a NULL hash. + let Some(hash) = user.password_hash() else { + tracing::info!( + target: "audit", + event = "auth.login_rejected", + reason = "no_password", + user_id = %user.id(), + username = %user.display_for_audit(), + "🔐 login rejected: user has no password configured for '{}'", + user.display_for_audit(), + ); + return Err(DomainError::new( + ErrorKind::AccessDenied, + "Auth", + "Invalid credentials", + )); + }; let is_valid = self .password_hasher - .verify_password(&dto.password, user.password_hash()) + .verify_password(&dto.password, hash) .await?; if !is_valid { @@ -465,9 +502,9 @@ impl AuthApplicationService { event = "auth.login_rejected", reason = "bad_password", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), "🔐 login rejected: bad password for '{}'", - user.username(), + user.display_for_audit(), ); return Err(DomainError::new( ErrorKind::AccessDenied, @@ -627,9 +664,9 @@ impl AuthApplicationService { reason = "account_deactivated", token_id = %mlt.id(), user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), "🔗 magic-link rejected: account deactivated for '{}'", - user.username(), + user.display_for_audit(), ); return Err(DomainError::new( ErrorKind::AccessDenied, @@ -662,7 +699,7 @@ impl AuthApplicationService { target: "audit", event = "magic_link.redeemed", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), is_external = user.is_external(), resource_kind = ?mlt.resource_kind(), resource_id = ?mlt.resource_id(), @@ -705,10 +742,14 @@ impl AuthApplicationService { )); } - let is_valid = self - .password_hasher - .verify_password(password, user.password_hash()) - .await?; + let Some(hash) = user.password_hash() else { + return Err(DomainError::new( + ErrorKind::AccessDenied, + "Auth", + "Invalid credentials", + )); + }; + let is_valid = self.password_hasher.verify_password(password, hash).await?; if !is_valid { return Err(DomainError::new( @@ -720,7 +761,7 @@ impl AuthApplicationService { Ok(crate::application::dtos::user_dto::CurrentUser { id: user.id(), - username: user.username().to_string(), + username: user.username().unwrap_or("").to_string(), email: user.email().to_string(), role: user.role().to_string(), }) @@ -874,9 +915,16 @@ impl AuthApplicationService { } // Verify current password using the injected hasher + let Some(hash) = user.password_hash() else { + return Err(DomainError::new( + ErrorKind::AccessDenied, + "Auth", + "Current password is incorrect", + )); + }; let is_valid = self .password_hasher - .verify_password(&dto.current_password, user.password_hash()) + .verify_password(&dto.current_password, hash) .await?; if !is_valid { @@ -901,7 +949,7 @@ impl AuthApplicationService { .password_hasher .hash_password(&dto.new_password) .await?; - user.update_password_hash(new_hash); + user.update_password_hash(Some(new_hash)); // Save updated user self.user_storage.update_user(user.clone()).await?; @@ -1263,7 +1311,7 @@ impl AuthApplicationService { // External users never own storage. The DB `users_external_no_storage` // CHECK constraint enforces this; setting quota=0 here keeps the - // domain consistent and matches `User::new_external`. + // domain consistent and matches `User::new(..., is_external = true)`. let quota = if is_external { 0 } else { @@ -1275,19 +1323,33 @@ impl AuthApplicationService { // magic-link / OIDC, but the DB column is NOT NULL). let password_hash = self.password_hasher.hash_password(&dto.password).await?; - // Create domain entity. External path uses `new_external` so the - // is_external flag is set + the EXTERNAL placeholder password - // marker is applied for clarity in DB inspection. `new_external` - // forces role=User (the admin+external combo was rejected above). + // Create domain entity. External users are created with + // is_external=true and role forced to User (the admin+external + // combo was rejected above). For external users the supplied + // password hash is persisted so the audit trail is preserved, + // even though they authenticate via magic-link / OIDC. let user = if is_external { - User::new_external(dto.username.clone(), email).map(|mut u| { - // The hashed password from the request is unused for auth - // but is persisted so audit-trail integrity is preserved. - u.update_password_hash(password_hash); - u - }) + User::new( + email, + Some(dto.username.clone()), + Some(password_hash), + None, + None, + UserRole::User, + 0, + true, + ) } else { - User::new(dto.username.clone(), email, password_hash, role, quota) + User::new( + email, + Some(dto.username.clone()), + Some(password_hash), + None, + None, + role, + quota, + false, + ) } .map_err(|e| { DomainError::new( @@ -1375,7 +1437,11 @@ impl AuthApplicationService { /// `Err`, the transaction rolls back and the user remains intact. pub async fn delete_user_admin(&self, user_id: Uuid) -> Result<(), DomainError> { let user = self.user_storage.get_user_by_id(user_id).await?; - tracing::info!("Admin deleting user: {} ({})", user.username(), user_id); + tracing::info!( + "Admin deleting user: {} ({})", + user.display_for_audit(), + user_id + ); let mut tx = self .user_storage @@ -1783,13 +1849,15 @@ impl AuthApplicationService { username = format!("{}_{}", &username[..username.len().min(27)], suffix); } - let mut new_user = User::new_oidc( - username.clone(), + let mut new_user = User::new( oidc_email, + Some(username.clone()), + None, + Some(provider_name.clone()), + Some(claims.sub.clone()), role, quota, - provider_name.clone(), - claims.sub.clone(), + false, ) .map_err(|e| { DomainError::new( @@ -1829,13 +1897,13 @@ impl AuthApplicationService { // Nextcloud path: return user info so the handler can mint an // app-password and complete the NC login flow. tracing::info!( - user = %user.username(), + user = %user.display_for_audit(), "OIDC login successful for Nextcloud Login Flow v2" ); return Ok(OidcCallbackResult::NextcloudLogin { nc_flow_token: nc_token, user_id: user.id(), - username: user.username().to_string(), + username: user.username().unwrap_or("").to_string(), }); } diff --git a/src/application/services/external_identity_service.rs b/src/application/services/external_identity_service.rs index 1ae12afe..fbd2bbf4 100644 --- a/src/application/services/external_identity_service.rs +++ b/src/application/services/external_identity_service.rs @@ -85,7 +85,7 @@ impl UserLifecycleHook for ExternalIdentityLifecycleHook { target: "audit", event = "external_user.created", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), email = %user.email(), ); } @@ -98,7 +98,7 @@ impl UserLifecycleHook for ExternalIdentityLifecycleHook { target: "audit", event = "external_user.login", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), first_login = user.last_login_at().is_none(), ); } diff --git a/src/application/services/folder_service.rs b/src/application/services/folder_service.rs index e6150f96..8fd096e8 100644 --- a/src/application/services/folder_service.rs +++ b/src/application/services/folder_service.rs @@ -621,7 +621,7 @@ impl FolderService { pub async fn ensure_home_folder( &self, user_id: Uuid, - username: &str, + username: Option<&str>, ) -> Result { let existing = self .folder_storage @@ -637,7 +637,10 @@ impl FolderService { return Ok(false); } - let folder_name = format!("My Folder - {}", username); + let folder_name = match username { + Some(u) => format!("My Folder - {}", u), + None => format!("My Folder - {}", user_id), + }; self.folder_storage .create_home_folder(user_id, folder_name.clone()) .await diff --git a/src/application/services/magic_link_invite_service.rs b/src/application/services/magic_link_invite_service.rs index eb5d38a1..a842911f 100644 --- a/src/application/services/magic_link_invite_service.rs +++ b/src/application/services/magic_link_invite_service.rs @@ -33,7 +33,7 @@ use crate::application::services::user_lifecycle_service::UserLifecycleService; use crate::common::config::MagicLinkConfig; use crate::common::errors::{DomainError, ErrorKind}; use crate::domain::entities::magic_link_token::{MagicLinkResourceKind, MagicLinkToken}; -use crate::domain::entities::user::User; +use crate::domain::entities::user::{User, UserRole}; use crate::domain::repositories::magic_link_token_repository::MagicLinkTokenRepository; use crate::domain::repositories::user_repository::{UserRepository, UserRepositoryError}; use crate::domain::services::authorization::{Resource, ResourceKind}; @@ -119,11 +119,19 @@ impl MagicLinkInviteService { )); } - // username == normalised email for external users. The user - // entity's `validate_username` was widened to 254 chars + email - // shape in PR 6 specifically to allow this. - let user = User::new_external(normalised_email.to_string(), normalised_email.to_string()) - .map_err(|e| { + // External users are created without a username or password. + // `password_hash IS NULL` is the canonical no-password marker. + let user = User::new( + normalised_email.to_string(), + None, + None, + None, + None, + UserRole::User, + 0, + true, + ) + .map_err(|e| { DomainError::new( ErrorKind::InvalidInput, "MagicLinkInvite", @@ -308,10 +316,10 @@ impl MagicLinkInviteService { event = "auth.magic_link_send", reason = "has_credential", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), email = %normalised, "🔗 login-link suppressed: '{}' has another login credential", - user.username(), + user.display_for_audit(), ); return Ok(()); } @@ -322,10 +330,10 @@ impl MagicLinkInviteService { event = "auth.magic_link_send", reason = "account_deactivated", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), email = %normalised, "🔗 login-link suppressed: account deactivated for '{}'", - user.username(), + user.display_for_audit(), ); return Ok(()); } @@ -372,7 +380,7 @@ impl MagicLinkInviteService { event = "auth.magic_link_send", reason = "sent", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), email = %normalised, smtp_code = outcome.code, smtp_message = %outcome.message, diff --git a/src/application/services/subject_group_service.rs b/src/application/services/subject_group_service.rs index 812b41f4..89576334 100644 --- a/src/application/services/subject_group_service.rs +++ b/src/application/services/subject_group_service.rs @@ -270,7 +270,7 @@ impl SubjectGroupService { // 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`. + // `User::new(..., is_external = true)`. if let GroupMember::User(uid) = member { match UserRepository::get_user_by_id(&*self.user_storage, uid).await { Ok(user) if user.is_external() => { @@ -588,12 +588,11 @@ mod integration_tests { 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, + ) VALUES ($1, NULL, $2, NULL, '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"); diff --git a/src/application/services/user_lifecycle_service.rs b/src/application/services/user_lifecycle_service.rs index d12249ff..0a1793f4 100644 --- a/src/application/services/user_lifecycle_service.rs +++ b/src/application/services/user_lifecycle_service.rs @@ -151,7 +151,7 @@ impl UserLifecycleHook for AuditLifecycleHook { target: "audit", event = "user.created", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), is_external = user.is_external(), ); Ok(()) @@ -162,7 +162,7 @@ impl UserLifecycleHook for AuditLifecycleHook { target: "audit", event = "user.login", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), is_external = user.is_external(), first_login = user.last_login_at().is_none(), ); @@ -174,7 +174,7 @@ impl UserLifecycleHook for AuditLifecycleHook { target: "audit", event = "user.logout", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), is_external = user.is_external(), reason = ?reason, ); @@ -193,7 +193,7 @@ impl UserLifecycleHook for AuditLifecycleHook { target: "audit", event = "user.deleted", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), is_external = user.is_external(), mode = ?mode, ); @@ -272,7 +272,7 @@ impl UserLifecycleHook for SessionRevocationLifecycleHook { target: "audit", event = "user.sessions_revoked_on_delete", user_id = %user.id(), - username = %user.username(), + username = %user.display_for_audit(), mode = ?mode, count = count, ); diff --git a/src/domain/entities/user.rs b/src/domain/entities/user.rs index cbe7ef84..98093953 100644 --- a/src/domain/entities/user.rs +++ b/src/domain/entities/user.rs @@ -23,9 +23,19 @@ impl std::fmt::Display for UserRole { #[derive(Debug, Clone)] pub struct User { id: Uuid, - username: String, + /// Optional handle (2-64 chars, no `@`). NULL for users created via + /// email-invitation (`is_external = true`) and for users who have + /// not yet claimed a handle (PR-18 email-only signups). When set, it + /// must satisfy `validate_username` and must NOT contain `@` — + /// keeping the username and email namespaces provably disjoint. + username: Option, email: String, - password_hash: String, + /// Optional Argon2 password hash. NULL when the user has no password + /// (externals, OIDC-only users, email-only signups awaiting their + /// welcome magic-link). After PR 16 this column carries no sentinel + /// strings — `is_some()` means "real argon2 hash"; `None` means "no + /// password configured". + password_hash: Option, role: UserRole, storage_quota_bytes: i64, storage_used_bytes: i64, @@ -54,37 +64,70 @@ pub struct User { } impl User { - /// Create a new user with a pre-hashed password. + /// Create a new user. /// - /// The password hashing should be done externally using PasswordHasherPort - /// to maintain clean architecture and keep cryptographic dependencies - /// out of the domain layer. + /// One unified constructor for every kind of user (internal, OIDC-linked, + /// external). The credential slots and the `is_external` marker are all + /// caller-controlled — what makes a user "OIDC" is `oidc_subject = + /// Some(_)`, what makes them "external" is `is_external = true`. There + /// are no hidden sentinel values; an absent credential is `None`. /// /// # Arguments - /// * `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 - /// * `storage_quota_bytes` - Storage quota in bytes + /// * `email` — required, must satisfy `validate_email` + /// * `username` — optional handle (2-64 chars, no `@`) + /// * `password_hash` — pre-hashed via PasswordHasherPort, or `None` if + /// the user has no password yet (magic-link or OIDC bootstrap) + /// * `oidc_provider`, `oidc_subject` — both `Some` when the user is + /// linked to an external IdP, both `None` otherwise + /// * `role` — `Admin` is rejected when `is_external = true` (mirrors the + /// `users_external_not_admin` DB CHECK constraint) + /// * `storage_quota_bytes` — caller-set; external callers should pass 0 + /// to satisfy the `users_external_no_storage` invariant + /// * `is_external` — TRUE for grant-only recipients (magic-link, OCM) + #[allow(clippy::too_many_arguments)] pub fn new( - username: String, email: String, - password_hash: String, + username: Option, + password_hash: Option, + oidc_provider: Option, + oidc_subject: Option, role: UserRole, storage_quota_bytes: i64, + is_external: bool, ) -> UserResult { - // Validations - Self::validate_username(&username)?; Self::validate_email(&email)?; - - if password_hash.is_empty() { + if let Some(ref u) = username { + Self::validate_username(u)?; + } + if let Some(ref h) = password_hash + && h.is_empty() + { return Err(UserError::InvalidPassword( "Password hash cannot be empty".to_string(), )); } + // Schema-level CHECKs are mirrored at the entity layer so callers + // get a typed error instead of an opaque DB rejection. + if is_external && matches!(role, UserRole::Admin) { + return Err(UserError::ValidationError( + "External users cannot hold the admin role".to_string(), + )); + } + if is_external && storage_quota_bytes != 0 { + return Err(UserError::ValidationError( + "External users must have storage_quota_bytes = 0".to_string(), + )); + } + // OIDC linkage is all-or-nothing: both provider and subject set, + // or neither. The DB has a UNIQUE index on (provider, subject) + // WHERE both non-NULL; partial state would corrupt that. + if oidc_provider.is_some() != oidc_subject.is_some() { + return Err(UserError::ValidationError( + "oidc_provider and oidc_subject must both be set or both be None".to_string(), + )); + } let now = Utc::now(); - Ok(Self { id: Uuid::new_v4(), username, @@ -97,86 +140,10 @@ impl User { updated_at: now, last_login_at: None, active: true, - oidc_provider: None, - oidc_subject: None, + oidc_provider, + oidc_subject, image: None, - is_external: false, - given_name: None, - family_name: None, - }) - } - - /// Create a new OIDC-authenticated user (no password required). - pub fn new_oidc( - username: String, - email: String, - role: UserRole, - storage_quota_bytes: i64, - oidc_provider: String, - oidc_subject: String, - ) -> UserResult { - Self::validate_username(&username)?; - Self::validate_email(&email)?; - let now = Utc::now(); - Ok(Self { - id: Uuid::new_v4(), - username, - email, - password_hash: "__OIDC_NO_PASSWORD__".to_string(), - role, - storage_quota_bytes, - storage_used_bytes: 0, - created_at: now, - updated_at: now, - last_login_at: None, - active: true, - oidc_provider: Some(oidc_provider), - oidc_subject: Some(oidc_subject), - image: None, - is_external: false, - given_name: None, - family_name: None, - }) - } - - /// Create a new external user — magic-link / OIDC-only / OCM-federated - /// recipient who does NOT own storage. The `CHECK (NOT is_external OR - /// storage_used_bytes = 0)` DB constraint enforces the no-storage rule - /// at the schema level. - /// - /// **External users are always `UserRole::User`** — there is no role - /// parameter because admin + external is an explicitly forbidden - /// combination enforced by the `users_external_not_admin` DB CHECK - /// constraint. Granting admin to a federated principal would let - /// external identity providers indirectly manage the local instance. - /// To make an external user an admin: first convert them to internal - /// (`UPDATE auth.users SET is_external = FALSE`), then update role. - /// The two-step process is intentional friction. - /// - /// Quota is set to 0 because external users can't upload content - /// into any folder they own (they have no folder). They can only - /// act on grants the resource owner provides — which counts against - /// the owner's quota, not theirs. - pub fn new_external(username: String, email: String) -> UserResult { - Self::validate_username(&username)?; - Self::validate_email(&email)?; - let now = Utc::now(); - Ok(Self { - id: Uuid::new_v4(), - username, - email, - password_hash: "__EXTERNAL_NO_PASSWORD__".to_string(), - role: UserRole::User, - storage_quota_bytes: 0, - storage_used_bytes: 0, - created_at: now, - updated_at: now, - last_login_at: None, - active: true, - oidc_provider: None, - oidc_subject: None, - image: None, - is_external: true, + is_external, given_name: None, family_name: None, }) @@ -185,9 +152,9 @@ impl User { #[allow(clippy::too_many_arguments)] pub fn from_data( id: Uuid, - username: String, + username: Option, email: String, - password_hash: String, + password_hash: Option, role: UserRole, storage_quota_bytes: i64, storage_used_bytes: i64, @@ -226,9 +193,9 @@ impl User { #[allow(clippy::too_many_arguments)] pub fn from_data_full( id: Uuid, - username: String, + username: Option, email: String, - password_hash: String, + password_hash: Option, role: UserRole, storage_quota_bytes: i64, storage_used_bytes: i64, @@ -269,8 +236,12 @@ impl User { self.id } - pub fn username(&self) -> &str { - &self.username + /// The user's chosen handle. `None` for users who have not claimed + /// one (externals, fresh email-only signups). Display callers should + /// fall back through `given_name`/`family_name` to `email` when this + /// is `None`. + pub fn username(&self) -> Option<&str> { + self.username.as_deref() } pub fn email(&self) -> &str { @@ -305,8 +276,31 @@ impl User { self.active } - pub fn password_hash(&self) -> &str { - &self.password_hash + /// The Argon2 password hash, or `None` when the user has no password + /// configured (externals, OIDC-only users, post-PR-18 email-only + /// signups). `verify_password` callers must short-circuit to + /// "invalid credentials" when this is `None`. + pub fn password_hash(&self) -> Option<&str> { + self.password_hash.as_deref() + } + + /// Convenience: does the user have a real password configured? + pub fn has_password(&self) -> bool { + self.password_hash.is_some() + } + + /// Best-effort label for audit-log interpolation. Returns the + /// username when set; falls back to the user_id otherwise. Always + /// implements `Display` (returns `String`) so audit lines can stay + /// `username = %user.display_for_audit()` regardless of whether the + /// user has claimed a handle. Reserve this for `target: "audit"` + /// lines — user-facing display callers should walk the + /// `username → given/family → email` fallback chain themselves. + pub fn display_for_audit(&self) -> String { + match &self.username { + Some(u) => u.clone(), + None => self.id.to_string(), + } } pub fn oidc_provider(&self) -> Option<&str> { @@ -352,44 +346,48 @@ impl User { self.updated_at = Utc::now(); } - /// Mutate the username after creation. Runs the same validation as the + /// Claim or change the username. 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`. + /// endpoint so a user who started with `None` can claim a handle + /// later, or change to a different one. 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.username = Some(new_username); self.updated_at = Utc::now(); Ok(()) } + /// Unset the username (return to `None`). Use sparingly — most + /// users keep their handle once claimed. Mainly here so admin + /// tooling can clear a problematic handle without deleting the + /// account. + pub fn clear_username(&mut self) { + self.username = None; + self.updated_at = Utc::now(); + } + /// 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. + /// method available — either a real password hash, or a linked OIDC + /// subject. Magic-link eligibility for "no other credential" mode is + /// the negation of this; the `OXICLOUD_MAGIC_LINK_OPEN_TO_PASSWORD_USERS` + /// flag widens the policy at the service layer (`magic_link_eligibility`). 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() + self.password_hash.is_some() || self.oidc_subject.is_some() } - /// Update the password hash. - /// - /// The new password should be hashed externally using PasswordHasherPort - /// before calling this method. - pub fn update_password_hash(&mut self, new_hash: String) { + /// Set the password hash. The new password must be hashed externally + /// via `PasswordHasherPort` before calling this. Passing `None` + /// clears the password (e.g. when a user opts back into magic-link-only + /// auth). + pub fn update_password_hash(&mut self, new_hash: Option) { self.password_hash = new_hash; self.updated_at = Utc::now(); } @@ -421,39 +419,26 @@ impl User { // ── Shared validation helpers ────────────────────────────────────── - /// 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. + /// Usernames are 2-64 chars of `[A-Za-z0-9._-]`. The `@` character is + /// explicitly forbidden — keeping the username and email namespaces + /// provably disjoint is what closes the cross-collision attack class + /// described in the auth-simplification plan (a user can never claim + /// a handle that shadows another user's email). No leading/trailing + /// dot or hyphen. The character set also prevents XSS payloads from + /// being stored as usernames. fn validate_username(username: &str) -> UserResult<()> { - if username.len() < 3 || username.len() > 254 { + let len = username.chars().count(); + if !(2..=64).contains(&len) { return Err(UserError::InvalidUsername( - "Username must be between 3 and 254 characters".to_string(), + "Username must be between 2 and 64 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, - }); + return Err(UserError::InvalidUsername( + "Username must not contain '@' — use the email field for email addresses" + .to_string(), + )); } - if !username .chars() .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') @@ -463,7 +448,6 @@ impl User { .to_string(), )); } - // Disallow leading/trailing dots or hyphens if username.starts_with('.') || username.starts_with('-') || username.ends_with('.') diff --git a/src/infrastructure/services/jwt_service.rs b/src/infrastructure/services/jwt_service.rs index 7570c70f..2e70f111 100644 --- a/src/infrastructure/services/jwt_service.rs +++ b/src/infrastructure/services/jwt_service.rs @@ -156,7 +156,7 @@ impl TokenServicePort for JwtTokenService { // Log information for debugging tracing::debug!( "Generating token for user: {}, id: {}, role: {}", - user.username(), + user.display_for_audit(), user.id(), user.role() ); @@ -166,7 +166,7 @@ impl TokenServicePort for JwtTokenService { exp: now + self.access_token_expiry, iat: now, jti: Uuid::new_v4().to_string(), - username: user.username().to_string(), + username: user.username().unwrap_or("").to_string(), email: user.email().to_string(), role: format!("{}", user.role()), }; @@ -267,9 +267,9 @@ mod tests { fn create_test_user() -> User { User::from_data( Uuid::parse_str("00000000-0000-0000-0000-000000000001").unwrap(), - "testuser".to_string(), + Some("testuser".to_string()), "test@example.com".to_string(), - "hashed_password".to_string(), + Some("hashed_password".to_string()), UserRole::User, 1024 * 1024 * 1024, // 1GB 0, @@ -297,7 +297,7 @@ mod tests { .validate_token(&token) .expect("Should validate token"); assert_eq!(claims.sub, user.id().to_string()); - assert_eq!(claims.username, user.username()); + assert_eq!(Some(claims.username.as_str()), user.username()); assert_eq!(claims.email, user.email()); } diff --git a/src/infrastructure/services/pg_acl_engine.rs b/src/infrastructure/services/pg_acl_engine.rs index b24c3d57..ca61176c 100644 --- a/src/infrastructure/services/pg_acl_engine.rs +++ b/src/infrastructure/services/pg_acl_engine.rs @@ -959,7 +959,7 @@ impl AuthorizationEngine for PgAclEngine { ) SELECT ag.resource_type, ag.resource_id, rp.first_shared_at, ag.subject_type, ag.subject_id, - COALESCE(u.username, sg.name::text, sh.item_name, fi.name, fld.name, ag.subject_id::text) AS subject_display, + COALESCE(u.username, u.email, sg.name::text, sh.item_name, fi.name, fld.name, ag.subject_id::text) AS subject_display, ag.id AS grant_id, ag.granted_at, ag.expires_at, ag.permission, rp.sort_str, rp.sort_int, (sh.password_hash IS NOT NULL) AS has_password @@ -983,7 +983,7 @@ impl AuthorizationEngine for PgAclEngine { WHEN ag.subject_type = 'token' AND sh.password_hash IS NOT NULL THEN 2 ELSE 3 END ASC, - LOWER(COALESCE(u.username, sg.name::text, sh.item_name, ag.subject_id::text)) ASC, + LOWER(COALESCE(u.username, u.email, sg.name::text, sh.item_name, ag.subject_id::text)) ASC, ag.granted_at"# ) } @@ -1023,7 +1023,7 @@ impl AuthorizationEngine for PgAclEngine { ag.resource_id, ag.subject_type, ag.subject_id, - MAX(COALESCE(u.username, sg.name::text, sh.item_name, ag.subject_id::text)) AS subject_display, + MAX(COALESCE(u.username, u.email, sg.name::text, sh.item_name, ag.subject_id::text)) AS subject_display, BOOL_OR(sh.password_hash IS NOT NULL) AS has_password, MAX(CASE WHEN ag.subject_type = 'group' THEN 0 @@ -1108,7 +1108,7 @@ impl AuthorizationEngine for PgAclEngine { ag.resource_id, ag.subject_type, ag.subject_id, - MAX(COALESCE(u.username, sh.item_name, ag.subject_id::text)) AS subject_display, + MAX(COALESCE(u.username, u.email, sh.item_name, ag.subject_id::text)) AS subject_display, BOOL_OR(sh.password_hash IS NOT NULL) AS has_password, CASE WHEN BOOL_OR(ag.permission = 'delete') @@ -1196,7 +1196,7 @@ impl AuthorizationEngine for PgAclEngine { ) SELECT ag.resource_type, ag.resource_id, rp.first_shared_at, ag.subject_type, ag.subject_id, - COALESCE(u.username, sh.item_name, fi.name, fld.name, ag.subject_id::text) AS subject_display, + COALESCE(u.username, u.email, sh.item_name, fi.name, fld.name, ag.subject_id::text) AS subject_display, ag.id AS grant_id, ag.granted_at, ag.expires_at, ag.permission, NULL::text AS sort_str, NULL::bigint AS sort_int, (sh.password_hash IS NOT NULL) AS has_password diff --git a/src/interfaces/api/handlers/app_password_handler.rs b/src/interfaces/api/handlers/app_password_handler.rs index 012b2144..56ba8c78 100644 --- a/src/interfaces/api/handlers/app_password_handler.rs +++ b/src/interfaces/api/handlers/app_password_handler.rs @@ -60,6 +60,33 @@ async fn create_app_password( return Err(err); } + // Require a claimed username. NextCloud Basic Auth resolves users by + // username; an app password is unusable without one. UserDto carries + // an empty string when the underlying `users.username` is NULL — the + // entity rejects empty strings on construction, so empty here is an + // unambiguous signal that the column is NULL. + if let Some(auth_svc) = state.auth_service.as_ref() { + let user_dto = auth_svc + .auth_application_service + .get_user_by_id(user.id) + .await + .map_err(AppError::from)?; + if user_dto.username.is_none() { + tracing::info!( + target: "audit", + event = "auth.app_password_create_rejected", + reason = "no_username", + caller_id = %user.id, + "App-password creation requires a claimed username" + ); + return Err(AppError::new( + axum::http::StatusCode::CONFLICT, + "Claim a username on your profile before creating an app password.", + "UsernameRequired", + )); + } + } + let service = state .app_password_service .as_ref() diff --git a/src/interfaces/api/handlers/auth_handler.rs b/src/interfaces/api/handlers/auth_handler.rs index 9c6b73de..3cf5a4d4 100644 --- a/src/interfaces/api/handlers/auth_handler.rs +++ b/src/interfaces/api/handlers/auth_handler.rs @@ -878,7 +878,11 @@ pub async fn oidc_exchange( tracing::info!( "OIDC token exchange successful for user: {}", - auth_response.user.username + auth_response + .user + .username + .as_deref() + .unwrap_or(&auth_response.user.email) ); // Set HttpOnly cookies for the browser diff --git a/src/interfaces/api/handlers/contacts_handler.rs b/src/interfaces/api/handlers/contacts_handler.rs index 97c5e5c5..e7f68641 100644 --- a/src/interfaces/api/handlers/contacts_handler.rs +++ b/src/interfaces/api/handlers/contacts_handler.rs @@ -188,11 +188,13 @@ fn if_match_passes(if_match: Option<&str>, stored_etag: &str) -> bool { /// they're present, prefer a "First Last" full name; otherwise fall /// back to the username (which is always present). fn user_to_contact(user: UserDto) -> ContactDto { + // Display fallback chain: given+family name → username → email. + // Username is `Option` post PR 16; externals start with None. let full_name = match (user.given_name.as_deref(), user.family_name.as_deref()) { (Some(g), Some(f)) => format!("{g} {f}"), (Some(g), None) => g.to_string(), (None, Some(f)) => f.to_string(), - (None, None) => user.username.clone(), + (None, None) => user.username.clone().unwrap_or_else(|| user.email.clone()), }; ContactDto { id: user.id.clone(), diff --git a/src/interfaces/nextcloud/ocs_handler.rs b/src/interfaces/nextcloud/ocs_handler.rs index e2b137ef..5c255107 100644 --- a/src/interfaces/nextcloud/ocs_handler.rs +++ b/src/interfaces/nextcloud/ocs_handler.rs @@ -271,19 +271,26 @@ pub async fn handle_sharees_search( .await .unwrap_or_default(); + // Skip users with no claimed username — NC sharees autocomplete relies + // on a username being typeable; users still on the email-only signup + // path can't be addressed here. Also skip self (don't suggest sharing + // with yourself). let matches: Vec = users .into_iter() - .filter(|u| u.username != user.username) // Don't suggest self - .take(25) - .map(|u| { - json!({ - "label": u.username, + .filter_map(|u| { + let handle = u.username.clone()?; + if handle == user.username { + return None; + } + Some(json!({ + "label": handle, "value": { "shareType": 0, - "shareWith": u.username + "shareWith": handle, } - }) + })) }) + .take(25) .collect(); sharees_response(matches).into_response() diff --git a/tests/api/external_users.hurl b/tests/api/external_users.hurl index fd5bb049..61ece2e4 100644 --- a/tests/api/external_users.hurl +++ b/tests/api/external_users.hurl @@ -212,7 +212,9 @@ HTTP 403 # 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. +# his own avatar in the user menu. After PR 16 externals have +# NULL username (the field is omitted from JSON when None) — +# the email field is the identity. GET {{base_url}}/api/users/{{bob_user_id}} Authorization: Bearer {{bob_access_token}} @@ -220,6 +222,8 @@ HTTP 200 [Asserts] jsonpath "$.id" == "{{bob_user_id}}" jsonpath "$.is_external" == true +jsonpath "$.email" == "bob@externalcompany.com" +jsonpath "$.username" not exists # 11d — bob CAN look up Alice (his granter) — shared-grant relationship # lets the external recipient resolve the sharer's display name + @@ -285,7 +289,8 @@ 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). +# Bob's profile (with is_external=true). Bob's username +# is NULL post PR 16 (externals don't carry a handle). # ───────────────────────────────────────────────────────────── GET {{base_url}}/api/users/{{bob_user_id}} Authorization: Bearer {{alice_token}} @@ -294,6 +299,8 @@ HTTP 200 [Asserts] jsonpath "$.id" == "{{bob_user_id}}" jsonpath "$.is_external" == true +jsonpath "$.email" == "bob@externalcompany.com" +jsonpath "$.username" not exists # ─────────────────────────────────────────────────────────────