From 6e2b770feaf3291c98cce2a98d1797fbe6f5b2a7 Mon Sep 17 00:00:00 2001 From: David Pointcheval Date: Fri, 4 Sep 2026 14:30:30 +0200 Subject: [PATCH 1/2] fix: return 409 Conflict instead of 500 when creating a duplicate admin or realm --- CHANGELOG/fix_admin-realm-create-conflict.md | 3 +++ server/src/database/impls/mysql.rs | 16 +++++++++++-- server/src/database/impls/postgres.rs | 16 +++++++++++-- server/src/database/impls/sqlite.rs | 16 +++++++++++-- server/src/tests/admin_api.rs | 15 ++++++------ server/src/tests/super_admin_api.rs | 25 +++++++++++++++++++- 6 files changed, 76 insertions(+), 15 deletions(-) create mode 100644 CHANGELOG/fix_admin-realm-create-conflict.md diff --git a/CHANGELOG/fix_admin-realm-create-conflict.md b/CHANGELOG/fix_admin-realm-create-conflict.md new file mode 100644 index 0000000..af74150 --- /dev/null +++ b/CHANGELOG/fix_admin-realm-create-conflict.md @@ -0,0 +1,3 @@ +## Bug Fixes + +- `POST /admins` and `POST /admins/realms` used to leak a raw `500` with database internals when creating an admin or realm whose ID already existed; they now return a clean `409 Conflict`, matching the fix already applied to `POST /realms/{realm_id}/userpass`. diff --git a/server/src/database/impls/mysql.rs b/server/src/database/impls/mysql.rs index 5ffe2c0..aae25e2 100644 --- a/server/src/database/impls/mysql.rs +++ b/server/src/database/impls/mysql.rs @@ -252,7 +252,13 @@ impl Database for MySqlDatabase { .bind(realm.session_max_stale_age_seconds) .bind(realm.certificate_max_age_seconds) .execute(&self.pool) - .await?; + .await + .map_err(|e| { + crate::database::AuthDbError::from_insert_error( + e, + format!("realm '{}' already exists", realm.id), + ) + })?; Ok(()) } @@ -693,7 +699,13 @@ impl Database for MySqlDatabase { .bind(&admin.totp_secret) .bind(&admin.totp_auth_url) .execute(&self.pool) - .await?; + .await + .map_err(|e| { + crate::database::AuthDbError::from_insert_error( + e, + format!("admin '{}' already exists", admin.id), + ) + })?; // Insert into user_realms join table for realm_id in &admin.realms { diff --git a/server/src/database/impls/postgres.rs b/server/src/database/impls/postgres.rs index 8e5fbd9..def3ed0 100644 --- a/server/src/database/impls/postgres.rs +++ b/server/src/database/impls/postgres.rs @@ -216,7 +216,13 @@ impl Database for PostgresDatabase { .bind(realm.session_max_stale_age_seconds) .bind(realm.certificate_max_age_seconds) .execute(&self.pool) - .await?; + .await + .map_err(|e| { + crate::database::AuthDbError::from_insert_error( + e, + format!("realm '{}' already exists", realm.id), + ) + })?; Ok(()) } @@ -657,7 +663,13 @@ impl Database for PostgresDatabase { .bind(&admin.totp_secret) .bind(&admin.totp_auth_url) .execute(&self.pool) - .await?; + .await + .map_err(|e| { + crate::database::AuthDbError::from_insert_error( + e, + format!("admin '{}' already exists", admin.id), + ) + })?; // Insert into user_realms join table for realm_id in &admin.realms { diff --git a/server/src/database/impls/sqlite.rs b/server/src/database/impls/sqlite.rs index 29694ea..f58a760 100644 --- a/server/src/database/impls/sqlite.rs +++ b/server/src/database/impls/sqlite.rs @@ -264,7 +264,13 @@ impl Database for SqliteDatabase { .bind(realm.session_max_stale_age_seconds) .bind(realm.certificate_max_age_seconds) .execute(&self.pool) - .await?; + .await + .map_err(|e| { + crate::database::AuthDbError::from_insert_error( + e, + format!("realm '{}' already exists", realm.id), + ) + })?; Ok(()) } @@ -705,7 +711,13 @@ impl Database for SqliteDatabase { .bind(&admin.totp_secret) .bind(&admin.totp_auth_url) .execute(&self.pool) - .await?; + .await + .map_err(|e| { + crate::database::AuthDbError::from_insert_error( + e, + format!("admin '{}' already exists", admin.id), + ) + })?; // Insert into user_realms join table for realm_id in &admin.realms { diff --git a/server/src/tests/admin_api.rs b/server/src/tests/admin_api.rs index d037b39..3b1f337 100644 --- a/server/src/tests/admin_api.rs +++ b/server/src/tests/admin_api.rs @@ -7,7 +7,7 @@ //! 3. Exercises one scenario against the `/users` scope. use crate::{ - AuthResult, AuthenticationNextStep, Realm, RealmAuthParams, + AuthError, AuthResult, AuthenticationNextStep, Realm, RealmAuthParams, client::AuthClientScheme, database::APP_REALM_ADMIN_USERNAME, models::ADMIN_REALM, @@ -75,7 +75,8 @@ async fn test_create_admin() -> AuthResult<()> { ctx.stop_server().await } -/// Creating a user whose ID already exists must return an error. +/// Creating a user whose ID already exists must return a clean `409 Conflict`, +/// not a `500` leaking the underlying database error. #[actix_web::test] async fn test_create_duplicate_user_fails() -> AuthResult<()> { init_test_logging(None); @@ -87,14 +88,12 @@ async fn test_create_duplicate_user_fails() -> AuthResult<()> { .create_admin_as_super_admin(&test_admin(APP_REALM_ADMIN_USERNAME)) .await; + let err = result.expect_err("Expected an error when creating a duplicate user"); assert!( - result.is_err(), - "Expected an error when creating a duplicate user" - ); - info!( - "create_duplicate_user returned expected error: {:?}", - result + matches!(err, AuthError::FailedHttpStatus(ref m) if m.contains("409")), + "Expected a 409 Conflict, got: {err:?}" ); + info!("create_duplicate_user returned expected error: {err:?}"); ctx.stop_server().await } diff --git a/server/src/tests/super_admin_api.rs b/server/src/tests/super_admin_api.rs index b40bcc7..05723ac 100644 --- a/server/src/tests/super_admin_api.rs +++ b/server/src/tests/super_admin_api.rs @@ -8,7 +8,7 @@ //! automatically sent on every call. use crate::{ - AuthResult, + AuthError, AuthResult, models::ADMIN_REALM, tests::{ helpers::{ @@ -180,6 +180,29 @@ async fn test_create_realm() -> AuthResult<()> { ctx.stop_server().await } +/// Creating a realm whose ID already exists must return a clean `409 Conflict`, +/// not a `500` leaking the underlying database error. +#[actix_web::test] +async fn test_create_duplicate_realm_fails() -> AuthResult<()> { + init_test_logging(None); + let ctx = start_default_test_server().await?; + let client = authenticate_as_admin(&ctx).await?; + + // The `_` admin realm is always seeded — trying to create it again must fail. + let result = client + .create_realm_as_super_admin(&test_realm(ADMIN_REALM)) + .await; + + let err = result.expect_err("Expected an error when creating a duplicate realm"); + assert!( + matches!(err, AuthError::FailedHttpStatus(ref m) if m.contains("409")), + "Expected a 409 Conflict, got: {err:?}" + ); + info!("create_duplicate_realm returned expected error: {err:?}"); + + ctx.stop_server().await +} + // ── Authorization enforcement ──────────────────────────────────────────────── /// A realm admin (non-super-admin) must not be able to update any realm (HTTP 403). From 1913c3cabef5767820b8ae19141d6d73ea710141 Mon Sep 17 00:00:00 2001 From: David Pointcheval Date: Fri, 4 Sep 2026 14:51:58 +0200 Subject: [PATCH 2/2] fix: rename misleading duplicate-admin test and add userpass duplicate coverage --- server/src/tests/admin_api.rs | 8 +++---- server/src/tests/super_admin_api.rs | 34 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/server/src/tests/admin_api.rs b/server/src/tests/admin_api.rs index 3b1f337..207651f 100644 --- a/server/src/tests/admin_api.rs +++ b/server/src/tests/admin_api.rs @@ -75,10 +75,10 @@ async fn test_create_admin() -> AuthResult<()> { ctx.stop_server().await } -/// Creating a user whose ID already exists must return a clean `409 Conflict`, +/// Creating an admin whose ID already exists must return a clean `409 Conflict`, /// not a `500` leaking the underlying database error. #[actix_web::test] -async fn test_create_duplicate_user_fails() -> AuthResult<()> { +async fn test_create_duplicate_admin_fails() -> AuthResult<()> { init_test_logging(None); let ctx = start_default_test_server().await?; let client = authenticate_as_admin(&ctx).await?; @@ -88,12 +88,12 @@ async fn test_create_duplicate_user_fails() -> AuthResult<()> { .create_admin_as_super_admin(&test_admin(APP_REALM_ADMIN_USERNAME)) .await; - let err = result.expect_err("Expected an error when creating a duplicate user"); + let err = result.expect_err("Expected an error when creating a duplicate admin"); assert!( matches!(err, AuthError::FailedHttpStatus(ref m) if m.contains("409")), "Expected a 409 Conflict, got: {err:?}" ); - info!("create_duplicate_user returned expected error: {err:?}"); + info!("create_duplicate_admin returned expected error: {err:?}"); ctx.stop_server().await } diff --git a/server/src/tests/super_admin_api.rs b/server/src/tests/super_admin_api.rs index 05723ac..61e7a9d 100644 --- a/server/src/tests/super_admin_api.rs +++ b/server/src/tests/super_admin_api.rs @@ -395,6 +395,40 @@ async fn test_userpass_crud_by_super_admin() -> AuthResult<()> { ctx.stop_server().await } +/// Re-provisioning credentials for an existing `(realm, username)` pair must +/// return a clean `409 Conflict`, not a `500` leaking the underlying database +/// error — and must not double as a password oracle by distinguishing a +/// byte-for-byte resubmission from a genuine conflict. +#[actix_web::test] +async fn test_create_duplicate_userpass_fails() -> AuthResult<()> { + init_test_logging(None); + let ctx = start_default_test_server().await?; + let client = authenticate_as_admin(&ctx).await?; + + let userpass = create_user( + ADMIN_REALM, + "duplicate_userpass_test", + "initial_pass", + false, + )?; + client + .create_admin_credentials_in_realm(ADMIN_REALM, &userpass) + .await?; + + let result = client + .create_admin_credentials_in_realm(ADMIN_REALM, &userpass) + .await; + + let err = result.expect_err("Expected an error when re-provisioning existing credentials"); + assert!( + matches!(err, AuthError::FailedHttpStatus(ref m) if m.contains("409")), + "Expected a 409 Conflict, got: {err:?}" + ); + info!("create_duplicate_userpass returned expected error: {err:?}"); + + ctx.stop_server().await +} + // ── list_all_userpass requires super admin ──────────────────────────────────── /// `GET /admins/userpass` is in the `/admins` scope which hosts super-admin-only