From 2948875a966e0aacccc7aa18ac22636d86927cec Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Wed, 27 May 2026 22:00:04 +0200 Subject: [PATCH] fix(api): make app_members POST and PATCH atomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/manager-core/src/app_members_api.rs | 42 ++++++++------- crates/manager-core/src/app_members_repo.rs | 60 +++++++++++++++++++++ 2 files changed, 82 insertions(+), 20 deletions(-) diff --git a/crates/manager-core/src/app_members_api.rs b/crates/manager-core/src/app_members_api.rs index 52ed182..ab15edb 100644 --- a/crates/manager-core/src/app_members_api.rs +++ b/crates/manager-core/src/app_members_api.rs @@ -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 { - if let Ok(uuid) = ident.parse::() { - 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())) diff --git a/crates/manager-core/src/app_members_repo.rs b/crates/manager-core/src/app_members_repo.rs index 9d157d8..6a7fb06 100644 --- a/crates/manager-core/src/app_members_repo.rs +++ b/crates/manager-core/src/app_members_repo.rs @@ -69,6 +69,27 @@ pub trait AppMembersRepository: Send + Sync { role: AppRole, ) -> Result; + /// Atomic insert. Returns `Some(row)` on success, `None` if a + /// membership already exists. Lets the HTTP handler return 409 + /// without a separate `find` round-trip (no TOCTOU between check + /// and insert). + async fn try_insert( + &self, + app_id: AppId, + user_id: AdminUserId, + role: AppRole, + ) -> Result, AppMembersRepositoryError>; + + /// Atomic role update. Returns `Some(row)` on success, `None` if no + /// membership row exists. Lets PATCH return 404 without a separate + /// `find` round-trip (no TOCTOU between check and update). + async fn update_role( + &self, + app_id: AppId, + user_id: AdminUserId, + role: AppRole, + ) -> Result, AppMembersRepositoryError>; + /// Remove a membership. No-op (Ok) when the row doesn't exist — /// the user wasn't a member, which is the desired post-condition. async fn remove( @@ -165,6 +186,45 @@ impl AppMembersRepository for PostgresAppMembersRepository { Ok(()) } + async fn try_insert( + &self, + app_id: AppId, + user_id: AdminUserId, + role: AppRole, + ) -> Result, AppMembersRepositoryError> { + let row = sqlx::query_as::<_, AppMembershipRecord>( + "INSERT INTO app_members (app_id, user_id, role) \ + VALUES ($1, $2, $3) \ + ON CONFLICT (app_id, user_id) DO NOTHING \ + RETURNING app_id, user_id, role, created_at", + ) + .bind(app_id.into_inner()) + .bind(user_id.into_inner()) + .bind(role.as_str()) + .fetch_optional(&self.pool) + .await?; + row.map(TryInto::try_into).transpose() + } + + async fn update_role( + &self, + app_id: AppId, + user_id: AdminUserId, + role: AppRole, + ) -> Result, AppMembersRepositoryError> { + let row = sqlx::query_as::<_, AppMembershipRecord>( + "UPDATE app_members SET role = $1 \ + WHERE app_id = $2 AND user_id = $3 \ + RETURNING app_id, user_id, role, created_at", + ) + .bind(role.as_str()) + .bind(app_id.into_inner()) + .bind(user_id.into_inner()) + .fetch_optional(&self.pool) + .await?; + row.map(TryInto::try_into).transpose() + } + async fn list_for_user( &self, user_id: AdminUserId,