fix(db): map 23505 unique_violation to DomainError::Conflict (→ HTTP 409); close TOCTOU on register save
This commit is contained in:
@@ -6,6 +6,15 @@ pub(crate) trait IntoDbResult<T> {
|
|||||||
|
|
||||||
impl<T> IntoDbResult<T> for Result<T, sqlx::Error> {
|
impl<T> IntoDbResult<T> for Result<T, sqlx::Error> {
|
||||||
fn into_domain(self) -> Result<T, DomainError> {
|
fn into_domain(self) -> Result<T, DomainError> {
|
||||||
self.map_err(|e| DomainError::Internal(e.to_string()))
|
self.map_err(|e| {
|
||||||
|
if let sqlx::Error::Database(ref db) = e {
|
||||||
|
if db.code().as_deref() == Some("23505") {
|
||||||
|
return DomainError::Conflict(
|
||||||
|
db.constraint().unwrap_or("conflict").to_string(),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
DomainError::Internal(e.to_string())
|
||||||
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -34,7 +34,19 @@ pub async fn register(
|
|||||||
}
|
}
|
||||||
let hash = hasher.hash(&input.password).await?;
|
let hash = hasher.hash(&input.password).await?;
|
||||||
let user = User::new_local(UserId::new(), username, email, hash);
|
let user = User::new_local(UserId::new(), username, email, hash);
|
||||||
users.save(&user).await?;
|
users
|
||||||
|
.save(&user)
|
||||||
|
.await
|
||||||
|
.map_err(|e| match e {
|
||||||
|
DomainError::Conflict(ref c) if c.contains("username") => {
|
||||||
|
DomainError::Conflict("username taken".into())
|
||||||
|
}
|
||||||
|
DomainError::Conflict(ref c) if c.contains("email") => {
|
||||||
|
DomainError::Conflict("email taken".into())
|
||||||
|
}
|
||||||
|
DomainError::Conflict(_) => DomainError::Conflict("already exists".into()),
|
||||||
|
other => other,
|
||||||
|
})?;
|
||||||
events
|
events
|
||||||
.publish(&DomainEvent::UserRegistered {
|
.publish(&DomainEvent::UserRegistered {
|
||||||
user_id: user.id.clone(),
|
user_id: user.id.clone(),
|
||||||
@@ -90,11 +102,58 @@ mod tests {
|
|||||||
use domain::{
|
use domain::{
|
||||||
errors::DomainError,
|
errors::DomainError,
|
||||||
events::DomainEvent,
|
events::DomainEvent,
|
||||||
ports::{AuthService, GeneratedToken, PasswordHasher},
|
models::{feed::UserSummary, user::User},
|
||||||
|
ports::{AuthService, GeneratedToken, PasswordHasher, UserReader, UserWriter},
|
||||||
testing::{NoOpEventPublisher, TestStore},
|
testing::{NoOpEventPublisher, TestStore},
|
||||||
value_objects::{PasswordHash, UserId},
|
value_objects::{Email, PasswordHash, UserId, Username},
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/// Simulates a concurrent registration that slips past the pre-checks and
|
||||||
|
/// hits the DB unique constraint — exactly what happens in the TOCTOU window.
|
||||||
|
struct ConflictOnSaveStore(TestStore);
|
||||||
|
|
||||||
|
#[async_trait]
|
||||||
|
impl UserReader for ConflictOnSaveStore {
|
||||||
|
async fn find_by_id(&self, id: &UserId) -> Result<Option<User>, DomainError> {
|
||||||
|
self.0.find_by_id(id).await
|
||||||
|
}
|
||||||
|
async fn find_by_username(
|
||||||
|
&self,
|
||||||
|
username: &Username,
|
||||||
|
) -> Result<Option<User>, DomainError> {
|
||||||
|
self.0.find_by_username(username).await
|
||||||
|
}
|
||||||
|
async fn find_by_email(&self, email: &Email) -> Result<Option<User>, DomainError> {
|
||||||
|
self.0.find_by_email(email).await
|
||||||
|
}
|
||||||
|
async fn list_with_stats(&self) -> Result<Vec<UserSummary>, DomainError> {
|
||||||
|
self.0.list_with_stats().await
|
||||||
|
}
|
||||||
|
async fn count(&self) -> Result<i64, DomainError> {
|
||||||
|
self.0.count().await
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[async_trait]
|
||||||
|
impl UserWriter for ConflictOnSaveStore {
|
||||||
|
async fn save(&self, _user: &User) -> Result<(), DomainError> {
|
||||||
|
Err(DomainError::Conflict("users_username_key".into()))
|
||||||
|
}
|
||||||
|
async fn update_profile(
|
||||||
|
&self,
|
||||||
|
user_id: &UserId,
|
||||||
|
display_name: Option<String>,
|
||||||
|
bio: Option<String>,
|
||||||
|
avatar_url: Option<String>,
|
||||||
|
header_url: Option<String>,
|
||||||
|
custom_css: Option<String>,
|
||||||
|
) -> Result<(), DomainError> {
|
||||||
|
self.0
|
||||||
|
.update_profile(user_id, display_name, bio, avatar_url, header_url, custom_css)
|
||||||
|
.await
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
struct FakeHasher;
|
struct FakeHasher;
|
||||||
#[async_trait]
|
#[async_trait]
|
||||||
impl PasswordHasher for FakeHasher {
|
impl PasswordHasher for FakeHasher {
|
||||||
@@ -240,4 +299,20 @@ mod tests {
|
|||||||
.unwrap_err();
|
.unwrap_err();
|
||||||
assert!(matches!(err, DomainError::Conflict(_)));
|
assert!(matches!(err, DomainError::Conflict(_)));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// TOCTOU: a concurrent registration slips past the pre-checks and the DB
|
||||||
|
/// unique constraint fires on save. The map_err must convert it to a
|
||||||
|
/// human-readable Conflict, not bubble up a raw constraint name.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn register_maps_db_conflict_on_username_to_conflict() {
|
||||||
|
let store = ConflictOnSaveStore(TestStore::default());
|
||||||
|
let err = register(&store, &FakeHasher, &FakeAuth, &NoOpEventPublisher, input())
|
||||||
|
.await
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(
|
||||||
|
matches!(err, DomainError::Conflict(ref m) if m == "username taken"),
|
||||||
|
"expected 'username taken', got: {:?}",
|
||||||
|
err
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user