From 00af0e8a89295a84b5d46415262983593b38a502 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 2 Jun 2026 22:50:13 +0200 Subject: [PATCH] feat(registraton): add anti enumeration (cannot know if an account already exists) Important: anti-enumeration is active only if SMTP is defined, welcome email can be used otherwise it is a classic registration with ok or conflic if account alrady exists --- .../services/auth_application_service.rs | 75 +++++++-- src/interfaces/api/handlers/auth_handler.rs | 152 ++++++++++++------ tests/api/registration.hurl | 93 +++++++++-- tests/common/server.env | 1 + 4 files changed, 248 insertions(+), 73 deletions(-) diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index e3c4ea68..f5c46cd8 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -45,6 +45,24 @@ pub enum OidcCallbackResult { /// the same shape as a password login; the optional resource fields tell /// the handler whether to deep-link to the invited resource or fall back /// to the generic `/shared-with-me` landing. +/// Outcome of a `register` call. The handler maps this to either an +/// anti-enumerated uniform 200 (when SMTP is available — there's a +/// "check your email" cover story for the user) or the classic +/// 201/409 split (when SMTP is unavailable — without the cover story, +/// uniform responses would just be misleading UX with no security +/// benefit). Either way the service emits the same audit-log entries. +#[derive(Debug, Clone)] +pub enum RegisterResult { + /// Boxed to avoid the `large_enum_variant` clippy warning — + /// `UserDto` is ~250 bytes, the other variants are zero-sized, + /// so a heap-pointer indirection keeps the enum's stack size + /// small. `register` is called once per request; the + /// allocation cost is negligible. + Created(Box), + UsernameTaken, + EmailTaken, +} + #[derive(Debug, Clone)] pub struct MagicLinkRedemption { pub auth: AuthResponseDto, @@ -254,7 +272,18 @@ impl AuthApplicationService { state.service.clone() } - pub async fn register(&self, dto: RegisterDto) -> Result { + /// Public registration. Returns one of three outcomes: + /// - `Created(user)` — a user was actually created + /// - `UsernameTaken` / `EmailTaken` — collision; no DB write + /// + /// The handler decides the HTTP shape based on whether SMTP is + /// available (anti-enumeration uniform 200 vs classic 201/409). + /// The service emits the same audit-log entries either way — the + /// audit channel is the source of truth for the actual outcome. + /// + /// Real failures (DB error, password too short, etc.) surface as + /// `Err`. + pub async fn register(&self, dto: RegisterDto) -> Result { // Username uniqueness (only when a username was supplied — None // is the "claim later" path, multiple NULLs are allowed by the // UNIQUE index per Postgres semantics). @@ -265,11 +294,16 @@ impl AuthApplicationService { .await .is_ok() { - return Err(DomainError::new( - ErrorKind::AlreadyExists, - "User", - format!("User '{}' already exists", username), - )); + tracing::info!( + target: "audit", + event = "auth.register", + reason = "username_taken", + attempted_username = %username, + attempted_email = %dto.email, + "🛂 register collision: username '{}' already exists", + username, + ); + return Ok(RegisterResult::UsernameTaken); } if self @@ -278,11 +312,15 @@ impl AuthApplicationService { .await .is_ok() { - return Err(DomainError::new( - ErrorKind::AlreadyExists, - "User", - format!("Email '{}' is already registered", dto.email), - )); + tracing::info!( + target: "audit", + event = "auth.register", + reason = "email_taken", + attempted_email = %dto.email, + "🛂 register collision: email '{}' is already registered", + dto.email, + ); + return Ok(RegisterResult::EmailTaken); } // SECURITY: Public registration ALWAYS creates regular users. @@ -308,7 +346,6 @@ impl AuthApplicationService { } None => None, }; - let was_passwordless = password_hash.is_none(); let user = User::new( dto.email.clone(), @@ -330,7 +367,6 @@ impl AuthApplicationService { // Save user let created_user = self.user_storage.create_user(user).await?; - let _ = was_passwordless; // handler dispatches the welcome mail on this path // Lifecycle: HomeFolderLifecycleHook handles personal-folder // creation (was inlined here pre-PR 3); audit log + future @@ -339,8 +375,17 @@ impl AuthApplicationService { lc.dispatch_created(&created_user).await; } - tracing::info!("User registered: {}", created_user.id()); - Ok(UserDto::from(created_user)) + tracing::info!( + target: "audit", + event = "auth.register", + reason = "created", + user_id = %created_user.id(), + username = %created_user.display_for_audit(), + email = %created_user.email(), + is_external = false, + "🛂 user registered", + ); + Ok(RegisterResult::Created(Box::new(UserDto::from(created_user)))) } /// Create the first admin user during initial system setup. diff --git a/src/interfaces/api/handlers/auth_handler.rs b/src/interfaces/api/handlers/auth_handler.rs index 8ad0b4c9..de206f1c 100644 --- a/src/interfaces/api/handlers/auth_handler.rs +++ b/src/interfaces/api/handlers/auth_handler.rs @@ -13,7 +13,7 @@ use crate::application::dtos::user_dto::{ AuthResponseDto, ChangePasswordDto, LoginDto, OidcCallbackQueryDto, OidcExchangeDto, OidcProviderInfoDto, RefreshTokenDto, RegisterDto, SetupAdminDto, UserDto, }; -use crate::application::services::auth_application_service::OidcCallbackResult; +use crate::application::services::auth_application_service::{OidcCallbackResult, RegisterResult}; use crate::common::di::AppState; use crate::interfaces::api::cookie_auth; use crate::interfaces::errors::AppError; @@ -64,15 +64,39 @@ pub fn setup_route() -> Router> { } /// Register a new user account. +/// +/// **Response shape depends on SMTP availability**: +/// +/// - **SMTP configured** (`magic_link_invite_service` is wired): the +/// endpoint returns a **uniform 200** for both success and collision +/// (anti-enumeration). The "Registration request received" message +/// covers both branches honestly because successful email-only +/// signups receive a welcome magic-link. Real outcome recorded in +/// the `audit` channel as `auth.register` with `reason` one of +/// `created`, `email_taken`, `username_taken`. +/// - **SMTP not configured**: there is no welcome-mail cover story, so +/// the classic `201 + UserDto` on success and `409` on collision +/// apply. Anti-enumeration would just be misleading UX (telling the +/// user to check an email that will never arrive). Email-only +/// signup is **503** in this mode because the user would otherwise +/// be stranded with an account they can't log into. +/// +/// **Instance-wide policy stays visible** in both modes: when +/// registration is disabled by the admin or password registration is +/// disabled in OIDC-only mode, the endpoint returns **403** with a +/// clear message. These are instance-wide settings, not per-user +/// oracles — legitimate users deserve an actionable error. #[utoipa::path( post, path = "/api/auth/register", request_body = RegisterDto, responses( - (status = 201, description = "User registered successfully", body = UserDto), - (status = 400, description = "Validation error"), - (status = 403, description = "Registration disabled"), - (status = 409, description = "Username or email already taken"), + (status = 200, description = "Uniform registration response (SMTP configured, anti-enumeration mode)"), + (status = 201, description = "User registered successfully (SMTP not configured)", body = UserDto), + (status = 400, description = "Validation error (malformed request body)"), + (status = 403, description = "Registration disabled (admin setting or OIDC-only mode)"), + (status = 409, description = "Username or email already taken (SMTP not configured)"), + (status = 503, description = "Email-only signup requires SMTP to be configured"), ), tag = "auth" )] @@ -80,15 +104,13 @@ pub async fn register( State(state): State>, Json(dto): Json, ) -> Result { - // Display the supplied identifier in operational logs without - // panicking on the None branch — the user may have registered - // email-only with no username yet. - let log_identifier = dto - .username - .as_deref() - .unwrap_or(dto.email.as_str()) - .to_string(); - tracing::info!("Registration attempt for: {}", log_identifier); + // Uniform 200 response used in anti-enumeration mode (SMTP wired). + let uniform_ok = || { + let payload = serde_json::json!({ + "message": "Registration request received.", + }); + (StatusCode::OK, Json(payload)).into_response() + }; // Verify auth service exists let auth_service = match state.auth_service.as_ref() { @@ -101,9 +123,9 @@ pub async fn register( } }; - // Block password registration when OIDC-only mode is active. The - // email-only signup path is allowed because it doesn't store a - // password — the user later authenticates via magic-link. + // Block password registration when OIDC-only mode is active. + // Email-only signup still works in OIDC-only mode (no password + // stored; the user authenticates via magic-link). if dto.password.is_some() && auth_service .auth_application_service @@ -116,7 +138,7 @@ pub async fn register( )); } - // Check if public registration has been disabled by the admin + // Admin disabled public registration globally — surface 403. if let Some(admin_svc) = state.admin_settings_service.as_ref() && !admin_svc.get_registration_enabled().await { @@ -127,45 +149,81 @@ pub async fn register( )); } + // Email-only signup requires SMTP. Without it the welcome mail + // can't be dispatched and the user is stranded with no way to log + // in. 503 is the right response: instance-wide policy, no per-user + // oracle leaked. + let smtp_enabled = state.magic_link_invite_service.is_some(); + if dto.password.is_none() && !smtp_enabled { + return Err(AppError::new( + StatusCode::SERVICE_UNAVAILABLE, + "Email-only registration requires SMTP to be configured on this server.", + "SmtpRequired", + )); + } + let was_passwordless = dto.password.is_none(); let email = dto.email.clone(); - // Registration logic (duplicate checks, hashing, user creation) is - // all inside the service layer. - let user = match auth_service.auth_application_service.register(dto).await { - Ok(u) => u, + let result = match auth_service.auth_application_service.register(dto).await { + Ok(r) => r, Err(err) => { - tracing::error!("Registration failed for {}: {}", log_identifier, err); + tracing::error!("Registration failed: {}", err); return Err(err.into()); } }; - tracing::info!("Registration successful for: {}", log_identifier); - // Email-only signup: dispatch a welcome magic-link so the user can - // land their first session without a password. Best-effort — SMTP - // failures don't fail the registration. Response shape is uniform - // (200 + anti-enumeration message) so the user is told to check - // their email regardless of whether SMTP was actually wired. - if was_passwordless { - if let Some(invite) = state.magic_link_invite_service.as_ref() - && let Err(e) = invite.send_login_link(&email).await - { - tracing::warn!( - target: "audit", - event = "auth.register_welcome_mail_failed", - user_id = %user.id, - email = %email, - error = %e, - "register: welcome magic-link send failed (user created)", - ); + match result { + RegisterResult::Created(user) => { + // Email-only signup: dispatch the welcome magic-link. + // Best-effort — SMTP failures don't roll back the user. + if was_passwordless + && let Some(invite) = state.magic_link_invite_service.as_ref() + && let Err(e) = invite.send_login_link(&email).await + { + tracing::warn!( + target: "audit", + event = "auth.register_welcome_mail_failed", + user_id = %user.id, + email = %email, + error = %e, + "register: welcome magic-link send failed (user created)", + ); + } + if smtp_enabled { + // Anti-enumeration mode: hide success-vs-collision behind + // the uniform "check your email" cover story. + Ok(uniform_ok()) + } else { + // Classic mode: clear 201 + UserDto so the frontend can + // log the user in directly with the password they just + // submitted. Unbox the DTO for the JSON serialisation. + Ok((StatusCode::CREATED, Json(*user)).into_response()) + } + } + RegisterResult::UsernameTaken => { + if smtp_enabled { + Ok(uniform_ok()) + } else { + Err(AppError::new( + StatusCode::CONFLICT, + "Username is already taken", + "UsernameTaken", + )) + } + } + RegisterResult::EmailTaken => { + if smtp_enabled { + Ok(uniform_ok()) + } else { + Err(AppError::new( + StatusCode::CONFLICT, + "Email is already registered", + "EmailTaken", + )) + } } - let payload = serde_json::json!({ - "message": "Check your email for a sign-in link to complete registration.", - }); - return Ok((StatusCode::OK, Json(payload)).into_response()); } - - Ok((StatusCode::CREATED, Json(user)).into_response()) } /// Authenticate with username and password. diff --git a/tests/api/registration.hurl b/tests/api/registration.hurl index e560c5e0..6ed108d6 100644 --- a/tests/api/registration.hurl +++ b/tests/api/registration.hurl @@ -25,8 +25,11 @@ alice_token: jsonpath "$.access_token" # ───────────────────────────────────────────────────────────── -# Step 2 — Classic registration (with password) still works. -# Returns 201 + UserDto (existing behaviour, unchanged). +# Step 2 — Classic registration (with password). PR 20 anti- +# enumeration mode (SMTP wired) returns a uniform 200 +# regardless of success or collision. No UserDto in +# the response — the frontend logs the user in +# separately to get a session. # ───────────────────────────────────────────────────────────── POST {{base_url}}/api/auth/register Content-Type: application/json @@ -36,13 +39,23 @@ Content-Type: application/json "password": "TestPassword1!" } -HTTP 201 +HTTP 200 [Asserts] -jsonpath "$.username" == "charlie" -jsonpath "$.email" == "charlie@example.com" -jsonpath "$.is_external" == false +jsonpath "$.message" contains "request received" + + +# ───────────────────────────────────────────────────────────── +# Step 2b — Log in as charlie to confirm registration succeeded +# AND to capture her user_id for cleanup. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "charlie", "password": "TestPassword1!" } + +HTTP 200 [Captures] -charlie_user_id: jsonpath "$.id" +charlie_token: jsonpath "$.access_token" +charlie_user_id: jsonpath "$.user.id" # ───────────────────────────────────────────────────────────── @@ -58,7 +71,7 @@ Content-Type: application/json HTTP 200 [Asserts] -jsonpath "$.message" contains "sign-in link" +jsonpath "$.message" contains "request received" # ───────────────────────────────────────────────────────────── @@ -109,7 +122,7 @@ pr18_user_id: jsonpath "$.id" # ───────────────────────────────────────────────────────────── -# Step 7 — Dave can request another magic-link (he has no +# Step 7 — The new user can request another magic-link (no # password configured → eligible). Anti-enumeration # 200 either way. # ───────────────────────────────────────────────────────────── @@ -123,8 +136,66 @@ jsonpath "$.message" contains "sign-in link" # ───────────────────────────────────────────────────────────── -# Cleanup — admin deletes charlie + dave so the DB-clean sweep -# at run.sh end sees no stragglers. +# Step 8 — PR 20 anti-enumeration: register with charlie's +# email AGAIN (different password). Response is the +# same uniform 200 — attacker can't tell from the +# HTTP shape whether the email was already taken. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/register +Content-Type: application/json +{ + "username": "charlie-imposter", + "email": "charlie@example.com", + "password": "AttackerPassword99!" +} + +HTTP 200 +[Asserts] +jsonpath "$.message" contains "request received" + + +# ───────────────────────────────────────────────────────────── +# Step 9 — Verify the collision was silently suppressed: the +# attacker's password does NOT work (the original +# row is intact, no rewrite happened). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "charlie@example.com", "password": "AttackerPassword99!" } + +HTTP 403 + + +# ───────────────────────────────────────────────────────────── +# Step 10 — Charlie's original password still works — the +# collision didn't touch her account. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "charlie", "password": "TestPassword1!" } + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 11 — Username collision (different email): same uniform +# 200, no new user, audit `username_taken`. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/register +Content-Type: application/json +{ + "username": "charlie", + "email": "charlie-other@example.com", + "password": "AttackerPassword99!" +} + +HTTP 200 +[Asserts] +jsonpath "$.message" contains "request received" + + +# ───────────────────────────────────────────────────────────── +# Cleanup — admin deletes both test users. # ───────────────────────────────────────────────────────────── DELETE {{base_url}}/api/admin/users/{{charlie_user_id}} Authorization: Bearer {{alice_token}} diff --git a/tests/common/server.env b/tests/common/server.env index 2123c57d..cbe9838f 100644 --- a/tests/common/server.env +++ b/tests/common/server.env @@ -22,6 +22,7 @@ RUST_LOG="warn,audit=info" # grow up limits for tests OXICLOUD_RATE_LIMIT_REFRESH_MAX=360 OXICLOUD_RATE_LIMIT_LOGIN_MAX=360 +OXICLOUD_RATE_LIMIT_REGISTER_MAX=360 # Magic-link / external-users flow (PR 9). The mock SMTP captures every # outbound message in-process so external_users.hurl can retrieve the