fix(api): make app_members POST and PATCH atomic

The previous handlers did `find()` then `upsert()` in two round-trips:

- POST: two concurrent grants both pass the duplicate check; the
  second `upsert` silently rewrites the role instead of returning
  409, weakening the "409 on duplicate" contract under load.
- PATCH: a concurrent DELETE between `find` and `upsert` makes PATCH
  silently re-create a row instead of returning 404, weakening the
  "404 if no existing membership" contract.

Adds two repo primitives that fold the check into the write:

- `try_insert` — `INSERT ... ON CONFLICT DO NOTHING RETURNING`; None
  return ⇒ already exists ⇒ 409.
- `update_role` — `UPDATE ... WHERE app_id AND user_id RETURNING`;
  None return ⇒ no row ⇒ 404.

Handlers use these directly; existing `upsert` stays for test helpers
that genuinely want upsert semantics.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-05-27 22:00:04 +02:00
parent b7175cc581
commit 2948875a96
2 changed files with 82 additions and 20 deletions

View File

@@ -22,7 +22,7 @@ use axum::response::{IntoResponse, Json, Response};
use axum::routing::{get, patch};
use axum::{Extension, Router};
use chrono::{DateTime, Utc};
use picloud_shared::{AdminUserId, AppId, AppRole, InstanceRole, Principal};
use picloud_shared::{AdminUserId, AppRole, InstanceRole, Principal};
use serde::{Deserialize, Serialize};
use serde_json::json;
use uuid::Uuid;
@@ -143,13 +143,17 @@ async fn grant_member(
.ok_or(AppMembersApiError::UserNotFound(input.user_id))?;
validate_grant_target(&user)?;
if s.members.find(user.id, app.id).await?.is_some() {
return Err(AppMembersApiError::AlreadyMember {
username: user.username,
});
}
let row = s.members.upsert(app.id, user.id, input.role).await?;
// Atomic insert — if a row already exists, returns None and we 409.
// Avoids the find-then-upsert race where two concurrent POSTs would
// both pass the existence check and the second `upsert` would
// silently rewrite the role.
let row = s
.members
.try_insert(app.id, user.id, input.role)
.await?
.ok_or_else(|| AppMembersApiError::AlreadyMember {
username: user.username.clone(),
})?;
Ok((StatusCode::CREATED, Json(compose_dto(user, row))))
}
@@ -169,11 +173,15 @@ async fn patch_member(
.await?
.ok_or(AppMembersApiError::UserNotFound(user_id))?;
if s.members.find(user_id, app.id).await?.is_none() {
return Err(AppMembersApiError::MembershipNotFound);
}
let row = s.members.upsert(app.id, user_id, input.role).await?;
// Atomic update — returns None if no row exists, so 404 is decided
// by the same statement that does the write. Eliminates the
// find-then-upsert race where a concurrent DELETE between the two
// calls would let PATCH silently re-create the row.
let row = s
.members
.update_role(app.id, user_id, input.role)
.await?
.ok_or(AppMembersApiError::MembershipNotFound)?;
Ok(Json(compose_dto(user, row)))
}
@@ -211,13 +219,7 @@ async fn resolve_app(
apps: &dyn AppRepository,
ident: &str,
) -> Result<picloud_shared::App, AppMembersApiError> {
if let Ok(uuid) = ident.parse::<Uuid>() {
if let Some(app) = apps.get_by_id(AppId::from(uuid)).await? {
return Ok(app);
}
return Err(AppMembersApiError::AppNotFound(ident.to_string()));
}
apps.get_by_slug_or_history(ident)
crate::app_repo::resolve_app(apps, ident)
.await?
.map(|l| l.app)
.ok_or_else(|| AppMembersApiError::AppNotFound(ident.to_string()))