feat(manager-core,picloud): accept email on admin create + patch
The /admins create/patch endpoints now plumb email through to the repo so the dashboard's invite + edit forms aren't silently dropping it on the floor. Discovered during smoke testing — the database column existed and was exposed in the response DTO, but neither the request DTO nor the repo's create() accepted it. CreateAdminRequest gains optional email; PatchAdminRequest gains email with JSON Merge Patch semantics: absent → don't change null → clear (write NULL) "<string>" → set to that value The tri-state needs Option<Option<String>> with a tiny custom deserializer; serde collapses absent and null otherwise. normalize_email() trims, treats blanks as None, and rejects obviously bogus values (no '@', >254 chars) with a 422. Real email verification is a future concern. Repo trait gains an email parameter on create() and a new update_email() method. The unique-violation branch in create now inspects constraint() to distinguish duplicate username from duplicate email. Integration test exercises create-with-email, PATCH null clears, PATCH value sets, PATCH without email key no-ops on email. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -69,12 +69,14 @@ pub trait AdminUserRepository: Send + Sync {
|
|||||||
async fn list(&self) -> Result<Vec<AdminUserRow>, AdminUserRepositoryError>;
|
async fn list(&self) -> Result<Vec<AdminUserRow>, AdminUserRepositoryError>;
|
||||||
/// Create a new admin. `instance_role` defaults to `Owner` for the
|
/// Create a new admin. `instance_role` defaults to `Owner` for the
|
||||||
/// env-var bootstrap path; admin-creates-admin flows pass an
|
/// env-var bootstrap path; admin-creates-admin flows pass an
|
||||||
/// explicit role.
|
/// explicit role. `email` is optional — pass `None` to leave the
|
||||||
|
/// column NULL.
|
||||||
async fn create(
|
async fn create(
|
||||||
&self,
|
&self,
|
||||||
username: &str,
|
username: &str,
|
||||||
password_hash: &str,
|
password_hash: &str,
|
||||||
instance_role: InstanceRole,
|
instance_role: InstanceRole,
|
||||||
|
email: Option<&str>,
|
||||||
) -> Result<AdminUserRow, AdminUserRepositoryError>;
|
) -> Result<AdminUserRow, AdminUserRepositoryError>;
|
||||||
async fn update_username(
|
async fn update_username(
|
||||||
&self,
|
&self,
|
||||||
@@ -86,6 +88,12 @@ pub trait AdminUserRepository: Send + Sync {
|
|||||||
id: AdminUserId,
|
id: AdminUserId,
|
||||||
password_hash: &str,
|
password_hash: &str,
|
||||||
) -> Result<AdminUserRow, AdminUserRepositoryError>;
|
) -> Result<AdminUserRow, AdminUserRepositoryError>;
|
||||||
|
/// Set or clear the email address. `None` writes NULL to the column.
|
||||||
|
async fn update_email(
|
||||||
|
&self,
|
||||||
|
id: AdminUserId,
|
||||||
|
email: Option<&str>,
|
||||||
|
) -> Result<AdminUserRow, AdminUserRepositoryError>;
|
||||||
/// Update the instance_role. Used by `PATCH /api/v1/admin/admins/{id}`;
|
/// Update the instance_role. Used by `PATCH /api/v1/admin/admins/{id}`;
|
||||||
/// callers enforce the last-owner guard (`count_other_active_owners`)
|
/// callers enforce the last-owner guard (`count_other_active_owners`)
|
||||||
/// before invoking when role transitions away from `Owner`.
|
/// before invoking when role transitions away from `Owner`.
|
||||||
@@ -192,24 +200,37 @@ impl AdminUserRepository for PostgresAdminUserRepository {
|
|||||||
username: &str,
|
username: &str,
|
||||||
password_hash: &str,
|
password_hash: &str,
|
||||||
instance_role: InstanceRole,
|
instance_role: InstanceRole,
|
||||||
|
email: Option<&str>,
|
||||||
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
||||||
let res = sqlx::query_as::<_, AdminUserRecord>(
|
let res = sqlx::query_as::<_, AdminUserRecord>(
|
||||||
"INSERT INTO admin_users (username, password_hash, instance_role) \
|
"INSERT INTO admin_users (username, password_hash, instance_role, email) \
|
||||||
VALUES ($1, $2, $3) \
|
VALUES ($1, $2, $3, $4) \
|
||||||
RETURNING id, username, is_active, instance_role, email, \
|
RETURNING id, username, is_active, instance_role, email, \
|
||||||
created_at, updated_at, last_login_at",
|
created_at, updated_at, last_login_at",
|
||||||
)
|
)
|
||||||
.bind(username)
|
.bind(username)
|
||||||
.bind(password_hash)
|
.bind(password_hash)
|
||||||
.bind(instance_role.as_str())
|
.bind(instance_role.as_str())
|
||||||
|
.bind(email)
|
||||||
.fetch_one(&self.pool)
|
.fetch_one(&self.pool)
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
match res {
|
match res {
|
||||||
Ok(row) => row.try_into(),
|
Ok(row) => row.try_into(),
|
||||||
Err(sqlx::Error::Database(e)) if e.is_unique_violation() => Err(
|
Err(sqlx::Error::Database(e)) if e.is_unique_violation() => {
|
||||||
AdminUserRepositoryError::DuplicateUsername(username.to_string()),
|
// username and email both have unique constraints; the
|
||||||
),
|
// create path can collide on either, so peek at the
|
||||||
|
// constraint name to surface the right error.
|
||||||
|
if e.constraint() == Some("admin_users_email_key") {
|
||||||
|
Err(AdminUserRepositoryError::DuplicateEmail(
|
||||||
|
email.unwrap_or("").to_string(),
|
||||||
|
))
|
||||||
|
} else {
|
||||||
|
Err(AdminUserRepositoryError::DuplicateUsername(
|
||||||
|
username.to_string(),
|
||||||
|
))
|
||||||
|
}
|
||||||
|
}
|
||||||
Err(e) => Err(e.into()),
|
Err(e) => Err(e.into()),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -259,6 +280,32 @@ impl AdminUserRepository for PostgresAdminUserRepository {
|
|||||||
.and_then(TryInto::try_into)
|
.and_then(TryInto::try_into)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async fn update_email(
|
||||||
|
&self,
|
||||||
|
id: AdminUserId,
|
||||||
|
email: Option<&str>,
|
||||||
|
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
||||||
|
let res = sqlx::query_as::<_, AdminUserRecord>(
|
||||||
|
"UPDATE admin_users SET email = $2, updated_at = NOW() \
|
||||||
|
WHERE id = $1 \
|
||||||
|
RETURNING id, username, is_active, instance_role, email, \
|
||||||
|
created_at, updated_at, last_login_at",
|
||||||
|
)
|
||||||
|
.bind(id.into_inner())
|
||||||
|
.bind(email)
|
||||||
|
.fetch_optional(&self.pool)
|
||||||
|
.await;
|
||||||
|
|
||||||
|
match res {
|
||||||
|
Ok(Some(row)) => row.try_into(),
|
||||||
|
Ok(None) => Err(AdminUserRepositoryError::NotFound(id)),
|
||||||
|
Err(sqlx::Error::Database(e)) if e.is_unique_violation() => Err(
|
||||||
|
AdminUserRepositoryError::DuplicateEmail(email.unwrap_or("").to_string()),
|
||||||
|
),
|
||||||
|
Err(e) => Err(e.into()),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
async fn update_instance_role(
|
async fn update_instance_role(
|
||||||
&self,
|
&self,
|
||||||
id: AdminUserId,
|
id: AdminUserId,
|
||||||
|
|||||||
@@ -95,6 +95,9 @@ pub struct CreateAdminRequest {
|
|||||||
/// channel that defaults to `Owner`.
|
/// channel that defaults to `Owner`.
|
||||||
#[serde(default = "default_create_role")]
|
#[serde(default = "default_create_role")]
|
||||||
pub instance_role: InstanceRole,
|
pub instance_role: InstanceRole,
|
||||||
|
/// Optional contact email. Blank/whitespace is normalized to None.
|
||||||
|
#[serde(default)]
|
||||||
|
pub email: Option<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
const fn default_create_role() -> InstanceRole {
|
const fn default_create_role() -> InstanceRole {
|
||||||
@@ -107,6 +110,26 @@ pub struct PatchAdminRequest {
|
|||||||
pub password: Option<String>,
|
pub password: Option<String>,
|
||||||
pub is_active: Option<bool>,
|
pub is_active: Option<bool>,
|
||||||
pub instance_role: Option<InstanceRole>,
|
pub instance_role: Option<InstanceRole>,
|
||||||
|
/// JSON Merge Patch (RFC 7396) semantics for email:
|
||||||
|
/// absent → don't change
|
||||||
|
/// null → clear (set DB column to NULL)
|
||||||
|
/// "<string>" → set to that string
|
||||||
|
/// `Option<Option<T>>` is the idiomatic Rust shape for that
|
||||||
|
/// tri-state; the custom deserializer below distinguishes the
|
||||||
|
/// "missing" case from the "present-and-null" case that serde
|
||||||
|
/// would otherwise collapse together.
|
||||||
|
#[allow(clippy::option_option)]
|
||||||
|
#[serde(default, deserialize_with = "deserialize_present_optional")]
|
||||||
|
pub email: Option<Option<String>>,
|
||||||
|
}
|
||||||
|
|
||||||
|
#[allow(clippy::option_option)]
|
||||||
|
fn deserialize_present_optional<'de, T, D>(deserializer: D) -> Result<Option<Option<T>>, D::Error>
|
||||||
|
where
|
||||||
|
T: serde::Deserialize<'de>,
|
||||||
|
D: serde::Deserializer<'de>,
|
||||||
|
{
|
||||||
|
Ok(Some(Option::<T>::deserialize(deserializer)?))
|
||||||
}
|
}
|
||||||
|
|
||||||
// ----------------------------------------------------------------------------
|
// ----------------------------------------------------------------------------
|
||||||
@@ -169,10 +192,11 @@ async fn create_admin(
|
|||||||
let username = input.username.trim();
|
let username = input.username.trim();
|
||||||
validate_username(username)?;
|
validate_username(username)?;
|
||||||
validate_password(&input.password)?;
|
validate_password(&input.password)?;
|
||||||
|
let email = normalize_email(input.email.as_deref())?;
|
||||||
let hash = hash_password(&input.password).map_err(|e| AdminApiError::Hash(e.to_string()))?;
|
let hash = hash_password(&input.password).map_err(|e| AdminApiError::Hash(e.to_string()))?;
|
||||||
let row = state
|
let row = state
|
||||||
.users
|
.users
|
||||||
.create(username, &hash, input.instance_role)
|
.create(username, &hash, input.instance_role, email.as_deref())
|
||||||
.await?;
|
.await?;
|
||||||
Ok((StatusCode::CREATED, Json(row.into())))
|
Ok((StatusCode::CREATED, Json(row.into())))
|
||||||
}
|
}
|
||||||
@@ -216,6 +240,12 @@ async fn patch_admin(
|
|||||||
// for the initial cut.)
|
// for the initial cut.)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if let Some(email_patch) = input.email.as_ref() {
|
||||||
|
// email_patch is Some(None) → clear, Some(Some(s)) → set.
|
||||||
|
let normalized = normalize_email(email_patch.as_deref())?;
|
||||||
|
latest = Some(state.users.update_email(id, normalized.as_deref()).await?);
|
||||||
|
}
|
||||||
|
|
||||||
if let Some(new_role) = input.instance_role {
|
if let Some(new_role) = input.instance_role {
|
||||||
// Self-elevation guard: only an owner can promote anyone TO
|
// Self-elevation guard: only an owner can promote anyone TO
|
||||||
// owner. An admin cannot turn themselves (or anyone else)
|
// owner. An admin cannot turn themselves (or anyone else)
|
||||||
@@ -358,6 +388,26 @@ fn validate_password(s: &str) -> Result<(), AdminApiError> {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Trim and reject empty / pathological emails, returning the
|
||||||
|
/// canonical form (or None when the input was blank). The shape
|
||||||
|
/// check is intentionally loose — we mainly want to reject blanks
|
||||||
|
/// and obvious junk; real verification is a future concern.
|
||||||
|
fn normalize_email(raw: Option<&str>) -> Result<Option<String>, AdminApiError> {
|
||||||
|
let Some(raw) = raw else {
|
||||||
|
return Ok(None);
|
||||||
|
};
|
||||||
|
let trimmed = raw.trim();
|
||||||
|
if trimmed.is_empty() {
|
||||||
|
return Ok(None);
|
||||||
|
}
|
||||||
|
if trimmed.len() > 254 || !trimmed.contains('@') {
|
||||||
|
return Err(AdminApiError::InvalidEmail(
|
||||||
|
"email must contain '@' and be at most 254 characters".to_string(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
Ok(Some(trimmed.to_string()))
|
||||||
|
}
|
||||||
|
|
||||||
// ----------------------------------------------------------------------------
|
// ----------------------------------------------------------------------------
|
||||||
// Errors
|
// Errors
|
||||||
// ----------------------------------------------------------------------------
|
// ----------------------------------------------------------------------------
|
||||||
@@ -373,6 +423,9 @@ pub enum AdminApiError {
|
|||||||
#[error("{0}")]
|
#[error("{0}")]
|
||||||
InvalidPassword(String),
|
InvalidPassword(String),
|
||||||
|
|
||||||
|
#[error("{0}")]
|
||||||
|
InvalidEmail(String),
|
||||||
|
|
||||||
#[error("cannot leave the system with zero active admins")]
|
#[error("cannot leave the system with zero active admins")]
|
||||||
LastActiveAdmin,
|
LastActiveAdmin,
|
||||||
|
|
||||||
@@ -414,6 +467,7 @@ impl IntoResponse for AdminApiError {
|
|||||||
) => (StatusCode::CONFLICT, self.to_string()),
|
) => (StatusCode::CONFLICT, self.to_string()),
|
||||||
Self::InvalidUsername(_)
|
Self::InvalidUsername(_)
|
||||||
| Self::InvalidPassword(_)
|
| Self::InvalidPassword(_)
|
||||||
|
| Self::InvalidEmail(_)
|
||||||
| Self::LastActiveAdmin
|
| Self::LastActiveAdmin
|
||||||
| Self::LastActiveOwner
|
| Self::LastActiveOwner
|
||||||
| Self::CannotEscalate
|
| Self::CannotEscalate
|
||||||
|
|||||||
@@ -123,6 +123,7 @@ pub async fn bootstrap_first_admin_with<R: AdminUserRepository + ?Sized>(
|
|||||||
&username,
|
&username,
|
||||||
&password_hash,
|
&password_hash,
|
||||||
picloud_shared::InstanceRole::Owner,
|
picloud_shared::InstanceRole::Owner,
|
||||||
|
None,
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
info!(username = %username, "bootstrapped initial admin user");
|
info!(username = %username, "bootstrapped initial admin user");
|
||||||
@@ -176,13 +177,14 @@ mod tests {
|
|||||||
username: &str,
|
username: &str,
|
||||||
_password_hash: &str,
|
_password_hash: &str,
|
||||||
instance_role: InstanceRole,
|
instance_role: InstanceRole,
|
||||||
|
email: Option<&str>,
|
||||||
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
||||||
let row = AdminUserRow {
|
let row = AdminUserRow {
|
||||||
id: AdminUserId::new(),
|
id: AdminUserId::new(),
|
||||||
username: username.to_string(),
|
username: username.to_string(),
|
||||||
is_active: true,
|
is_active: true,
|
||||||
instance_role,
|
instance_role,
|
||||||
email: None,
|
email: email.map(str::to_string),
|
||||||
created_at: Utc::now(),
|
created_at: Utc::now(),
|
||||||
updated_at: Utc::now(),
|
updated_at: Utc::now(),
|
||||||
last_login_at: None,
|
last_login_at: None,
|
||||||
@@ -204,6 +206,13 @@ mod tests {
|
|||||||
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
||||||
unimplemented!()
|
unimplemented!()
|
||||||
}
|
}
|
||||||
|
async fn update_email(
|
||||||
|
&self,
|
||||||
|
_i: AdminUserId,
|
||||||
|
_e: Option<&str>,
|
||||||
|
) -> Result<AdminUserRow, AdminUserRepositoryError> {
|
||||||
|
unimplemented!()
|
||||||
|
}
|
||||||
async fn update_instance_role(
|
async fn update_instance_role(
|
||||||
&self,
|
&self,
|
||||||
_i: AdminUserId,
|
_i: AdminUserId,
|
||||||
@@ -272,7 +281,7 @@ mod tests {
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn populated_db_is_noop() {
|
async fn populated_db_is_noop() {
|
||||||
let repo = InMemoryRepo::default();
|
let repo = InMemoryRepo::default();
|
||||||
repo.create("seeded", "x", InstanceRole::Owner)
|
repo.create("seeded", "x", InstanceRole::Owner, None)
|
||||||
.await
|
.await
|
||||||
.unwrap();
|
.unwrap();
|
||||||
let env = BootstrapEnv {
|
let env = BootstrapEnv {
|
||||||
|
|||||||
@@ -36,7 +36,7 @@ async fn server_with_app(pool: PgPool) -> (TestServer, String) {
|
|||||||
let auth = picloud::AuthDeps::from_pool(pool.clone());
|
let auth = picloud::AuthDeps::from_pool(pool.clone());
|
||||||
let hash = hash_password("test-pw").expect("hash");
|
let hash = hash_password("test-pw").expect("hash");
|
||||||
auth.users
|
auth.users
|
||||||
.create("test-admin", &hash, InstanceRole::Owner)
|
.create("test-admin", &hash, InstanceRole::Owner, None)
|
||||||
.await
|
.await
|
||||||
.expect("seed admin");
|
.expect("seed admin");
|
||||||
|
|
||||||
@@ -114,6 +114,47 @@ async fn auth_me_returns_principal_with_role_and_email(pool: PgPool) {
|
|||||||
assert!(body["id"].as_str().is_some());
|
assert!(body["id"].as_str().is_some());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||||
|
#[sqlx::test(migrations = "../manager-core/migrations")]
|
||||||
|
async fn create_admin_accepts_email_and_patch_clears_it(pool: PgPool) {
|
||||||
|
let s = server(pool).await;
|
||||||
|
// Create with email set.
|
||||||
|
let created = s
|
||||||
|
.post("/api/v1/admin/admins")
|
||||||
|
.json(&json!({
|
||||||
|
"username": "alice",
|
||||||
|
"password": "correct-horse-battery",
|
||||||
|
"instance_role": "member",
|
||||||
|
"email": "alice@example.com",
|
||||||
|
}))
|
||||||
|
.await;
|
||||||
|
created.assert_status(axum::http::StatusCode::CREATED);
|
||||||
|
let body: Value = created.json();
|
||||||
|
let alice_id = body["id"].as_str().expect("id").to_string();
|
||||||
|
assert_eq!(body["email"], "alice@example.com");
|
||||||
|
|
||||||
|
// Patch with email present-and-null clears it.
|
||||||
|
let cleared = s
|
||||||
|
.patch(&format!("/api/v1/admin/admins/{alice_id}"))
|
||||||
|
.json(&json!({ "email": null }))
|
||||||
|
.await;
|
||||||
|
cleared.assert_status_ok();
|
||||||
|
assert!(cleared.json::<Value>()["email"].is_null());
|
||||||
|
|
||||||
|
// Patch with email omitted is a no-op (doesn't clobber a re-set).
|
||||||
|
let reset = s
|
||||||
|
.patch(&format!("/api/v1/admin/admins/{alice_id}"))
|
||||||
|
.json(&json!({ "email": "alice2@example.com" }))
|
||||||
|
.await;
|
||||||
|
reset.assert_status_ok();
|
||||||
|
let omit = s
|
||||||
|
.patch(&format!("/api/v1/admin/admins/{alice_id}"))
|
||||||
|
.json(&json!({ "username": "alice" })) // no email key
|
||||||
|
.await;
|
||||||
|
omit.assert_status_ok();
|
||||||
|
assert_eq!(omit.json::<Value>()["email"], "alice2@example.com");
|
||||||
|
}
|
||||||
|
|
||||||
// ============================================================================
|
// ============================================================================
|
||||||
// Script CRUD
|
// Script CRUD
|
||||||
// ============================================================================
|
// ============================================================================
|
||||||
|
|||||||
@@ -53,7 +53,7 @@ async fn boot(pool: PgPool) -> Seeded {
|
|||||||
let hash = hash_password("owner-pw").expect("hash");
|
let hash = hash_password("owner-pw").expect("hash");
|
||||||
let owner = auth
|
let owner = auth
|
||||||
.users
|
.users
|
||||||
.create("owner", &hash, InstanceRole::Owner)
|
.create("owner", &hash, InstanceRole::Owner, None)
|
||||||
.await
|
.await
|
||||||
.expect("seed owner");
|
.expect("seed owner");
|
||||||
|
|
||||||
@@ -119,7 +119,7 @@ async fn seed_user(
|
|||||||
) -> AdminUserId {
|
) -> AdminUserId {
|
||||||
let repo = PostgresAdminUserRepository::new(pool.clone());
|
let repo = PostgresAdminUserRepository::new(pool.clone());
|
||||||
let hash = hash_password(password).expect("hash");
|
let hash = hash_password(password).expect("hash");
|
||||||
repo.create(username, &hash, role)
|
repo.create(username, &hash, role, None)
|
||||||
.await
|
.await
|
||||||
.expect("seed user")
|
.expect("seed user")
|
||||||
.id
|
.id
|
||||||
|
|||||||
Reference in New Issue
Block a user