From 4c9a9e7e83ba6f4240aaaef10cb67f8ac7edf96f Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 8 Sep 2026 11:22:02 +0200 Subject: [PATCH 1/2] models/krate: Add batch owner removal by identity `Crate::remove_owners()` soft-deletes selected user and team identities with one Diesel update scoped to the crate. Matching includes the owner kind so users and teams with the same ID remain distinct. Co-authored-by: moskirathe --- crates/crates_io_database/src/models/krate.rs | 31 ++++- .../crates_io_database/tests/crate_owners.rs | 129 ++++++++++++++++++ 2 files changed, 159 insertions(+), 1 deletion(-) create mode 100644 crates/crates_io_database/tests/crate_owners.rs diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index bfd761b6059..577a07a2a6f 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -1,6 +1,6 @@ use crate::fns::canon_crate_name; use crate::models::version::TopVersions; -use crate::models::{CrateOwner, Owner, PublicUser, User, Version}; +use crate::models::{CrateOwner, Owner, OwnerKind, PublicUser, User, Version}; use crate::schema::*; use chrono::{DateTime, Utc}; use diesel::associations::Identifiable; @@ -213,6 +213,35 @@ impl Crate { Ok(users.chain(teams).collect()) } + /// Soft-deletes the selected owner identities and returns the number of active rows removed. + /// Missing or already deleted owners are ignored, and an empty selection removes nothing. + pub async fn remove_owners( + &self, + mut conn: &AsyncPgConnection, + owners: &[(OwnerKind, i32)], + ) -> QueryResult { + let user_ids = owners + .iter() + .filter_map(|&(kind, id)| (kind == OwnerKind::User).then_some(id)); + let team_ids = owners + .iter() + .filter_map(|&(kind, id)| (kind == OwnerKind::Team).then_some(id)); + let selected_users = crate_owners::owner_kind + .eq(OwnerKind::User) + .and(crate_owners::owner_id.eq_any(user_ids)); + let selected_teams = crate_owners::owner_kind + .eq(OwnerKind::Team) + .and(crate_owners::owner_id.eq_any(team_ids)); + + diesel::update(crate_owners::table) + .filter(crate_owners::crate_id.eq(self.id)) + .filter(crate_owners::deleted.eq(false)) + .filter(selected_users.or(selected_teams)) + .set(crate_owners::deleted.eq(true)) + .execute(&mut conn) + .await + } + pub async fn owner_remove( &self, mut conn: &AsyncPgConnection, diff --git a/crates/crates_io_database/tests/crate_owners.rs b/crates/crates_io_database/tests/crate_owners.rs new file mode 100644 index 00000000000..a4edc80e49a --- /dev/null +++ b/crates/crates_io_database/tests/crate_owners.rs @@ -0,0 +1,129 @@ +use crates_io_database::models::{Crate, CrateOwner, NewCrate, NewUser, OwnerKind}; +use crates_io_database::schema::{crate_owners, teams}; +use crates_io_test_db::TestDatabase; +use diesel::prelude::*; +use diesel_async::{AsyncPgConnection, RunQueryDsl}; + +/// Batch removal matches complete owner identities within one crate. +#[tokio::test] +async fn remove_owners_by_identity() { + let test_db = TestDatabase::new(); + let mut conn = test_db.async_connect().await; + let user1 = insert_user(&mut conn, "first", 101).await; + let user2 = insert_user(&mut conn, "second", 102).await; + let team1 = insert_team(&mut conn, "first", 201).await; + let team2 = insert_team(&mut conn, "second", 202).await; + let team3 = insert_team(&mut conn, "non-owner", 203).await; + let crate1 = insert_crate(&mut conn, "foo", user1).await; + let crate2 = insert_crate(&mut conn, "bar", user1).await; + + insert_crate_owner(&conn, crate1.id, OwnerKind::User, user1, user1).await; + insert_crate_owner(&conn, crate1.id, OwnerKind::User, user2, user1).await; + insert_crate_owner(&conn, crate1.id, OwnerKind::Team, team1, user1).await; + insert_crate_owner(&conn, crate1.id, OwnerKind::Team, team2, user1).await; + insert_crate_owner(&conn, crate2.id, OwnerKind::User, user1, user1).await; + insert_crate_owner(&conn, crate2.id, OwnerKind::User, user2, user1).await; + insert_crate_owner(&conn, crate2.id, OwnerKind::Team, team1, user1).await; + insert_crate_owner(&conn, crate2.id, OwnerKind::Team, team2, user1).await; + + assert_eq!(crate1.remove_owners(&conn, &[]).await.unwrap(), 0); + let selected = [ + (OwnerKind::User, user1), + (OwnerKind::Team, team2), + (OwnerKind::User, user1), + (OwnerKind::Team, team3), + ]; + assert_eq!(crate1.remove_owners(&conn, &selected).await.unwrap(), 2); + assert_eq!(crate1.remove_owners(&conn, &selected).await.unwrap(), 0); + + let rows = OwnerRow::query() + .order(( + crate_owners::crate_id, + crate_owners::owner_kind, + crate_owners::owner_id, + )) + .load(&mut conn) + .await + .unwrap(); + + let rows: Vec<_> = rows + .into_iter() + .map(|row| (row.crate_id, row.owner_kind, row.owner_id, row.deleted)) + .collect(); + + assert_eq!( + rows, + [ + (crate1.id, 0, user1, true), + (crate1.id, 0, user2, false), + (crate1.id, 1, team1, false), + (crate1.id, 1, team2, true), + (crate2.id, 0, user1, false), + (crate2.id, 0, user2, false), + (crate2.id, 1, team1, false), + (crate2.id, 1, team2, false), + ] + ); +} + +/// Stored ownership identity and deletion state. +#[derive(Debug, HasQuery)] +#[diesel(table_name = crate_owners)] +struct OwnerRow { + crate_id: i32, + owner_kind: i32, + owner_id: i32, + deleted: bool, +} + +/// Inserts a user and returns its assigned ID. +async fn insert_user(conn: &mut AsyncPgConnection, name: &str, gh_id: i32) -> i32 { + let user = NewUser::builder() + .username(name) + .gh_login(name) + .gh_id(gh_id) + .build(); + user.insert(conn).await.unwrap() +} + +/// Inserts a team and returns its assigned ID. +async fn insert_team(conn: &mut AsyncPgConnection, name: &str, github_id: i32) -> i32 { + let login = format!("github:org:{name}"); + diesel::insert_into(teams::table) + .values(( + teams::login.eq(login), + teams::github_id.eq(github_id), + teams::org_id.eq(1), + )) + .returning(teams::id) + .get_result(conn) + .await + .unwrap() +} + +/// Creates a crate owned by the given user. +async fn insert_crate(conn: &mut AsyncPgConnection, name: &str, user_id: i32) -> Crate { + let krate = NewCrate { + name, + ..Default::default() + }; + krate.create(conn, user_id).await.unwrap() +} + +/// Inserts an ownership record for a user or team. +async fn insert_crate_owner( + conn: &AsyncPgConnection, + crate_id: i32, + owner_kind: OwnerKind, + owner_id: i32, + created_by: i32, +) { + let owner = CrateOwner { + crate_id, + owner_id, + owner_kind, + created_by, + email_notifications: true, + }; + owner.insert(conn).await.unwrap(); +} From 35c4ad0482210b513d5fbddd0811079cf0882cde Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 8 Sep 2026 12:04:21 +0200 Subject: [PATCH 2/2] controllers/owners: Resolve owners before batch removal Owner removals now resolve all supplied names against the initial owner list before updating ownership rows in one query. Repeated names select the same owners, and missing names fail before any updates. The transaction still rejects removing every individual owner. The name-based SQL removal method is no longer needed. Co-authored-by: moskirathe --- crates/crates_io_database/src/models/krate.rs | 64 +------------------ src/controllers/krate/owners.rs | 39 ++++++----- src/tests/owners.rs | 16 +++-- src/tests/routes/crates/list.rs | 5 +- src/tests/routes/me/get.rs | 7 +- src/tests/routes/users/stats.rs | 4 +- src/tests/team.rs | 5 +- 7 files changed, 48 insertions(+), 92 deletions(-) diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index 577a07a2a6f..4c9e58b0027 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -7,11 +7,10 @@ use diesel::associations::Identifiable; use diesel::dsl; use diesel::pg::Pg; use diesel::prelude::*; -use diesel::sql_types::{Bool, Integer, Text}; +use diesel::sql_types::{Bool, Text}; use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl}; use secrecy::SecretString; use serde::Serialize; -use thiserror::Error; use super::Team; @@ -241,52 +240,6 @@ impl Crate { .execute(&mut conn) .await } - - pub async fn owner_remove( - &self, - mut conn: &AsyncPgConnection, - login: &str, - ) -> Result<(), OwnerRemoveError> { - let query = diesel::sql_query( - r#"WITH crate_owners_with_login AS ( - SELECT - crate_owners.*, - CASE WHEN crate_owners.owner_kind = 1 THEN - teams.login - ELSE - users.gh_login - END AS login - FROM crate_owners - LEFT JOIN teams - ON crate_owners.owner_id = teams.id - AND crate_owners.owner_kind = 1 - LEFT JOIN users - ON crate_owners.owner_id = users.id - AND crate_owners.owner_kind = 0 - WHERE crate_owners.crate_id = $1 - AND crate_owners.deleted = false - ) - UPDATE crate_owners - SET deleted = true - FROM crate_owners_with_login - WHERE crate_owners.crate_id = crate_owners_with_login.crate_id - AND crate_owners.owner_id = crate_owners_with_login.owner_id - AND crate_owners.owner_kind = crate_owners_with_login.owner_kind - AND lower(crate_owners_with_login.login) = lower($2);"#, - ); - - let num_updated_rows = query - .bind::(self.id) - .bind::(login) - .execute(&mut conn) - .await?; - - if num_updated_rows == 0 { - return Err(OwnerRemoveError::not_found(login)); - } - - Ok(()) - } } /// Details of a newly created invite. @@ -299,18 +252,3 @@ pub enum NewOwnerInvite { /// The invitee was a [`Team`], and they were immediately added as an owner. Team(Team), } - -#[derive(Debug, Error)] -pub enum OwnerRemoveError { - #[error(transparent)] - Diesel(#[from] diesel::result::Error), - #[error("Could not find owner with login `{login}`")] - NotFound { login: String }, -} - -impl OwnerRemoveError { - pub fn not_found(login: &str) -> Self { - let login = login.to_string(); - Self::NotFound { login } - } -} diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 1d84586340e..4fcea5d1e99 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -2,8 +2,9 @@ use crate::controllers::helpers::authorization::Rights; use crate::controllers::krate::CratePath; -use crate::models::krate::OwnerRemoveError; -use crate::models::{Crate, Email, OauthGithub, Owner, PublicUser, Team, User, users_by_username}; +use crate::models::{ + Crate, Email, OauthGithub, Owner, OwnerKind, PublicUser, Team, User, users_by_username, +}; use crate::models::{ CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, NewTeam, krate::NewOwnerInvite, token::EndpointScope, @@ -290,10 +291,27 @@ pub async fn remove_owners( check_owner_permissions(&app, user, &owners).await?; - conn.transaction(async |conn| { - for login in &body.owners { - krate.owner_remove(conn, login).await?; + let mut selected = Vec::new(); + for login in &body.owners { + let normalized_login = login.to_lowercase(); + let mut matching = owners + .iter() + .filter(|owner| owner.login().to_lowercase() == normalized_login) + .peekable(); + + if matching.peek().is_none() { + let message = format!("could not find owner with login `{login}`"); + return Err(bad_request(message)); } + + selected.extend(matching.map(|owner| match owner { + Owner::User(user) => (OwnerKind::User, user.id), + Owner::Team(team) => (OwnerKind::Team, team.id), + })); + } + + conn.transaction(async |conn| { + krate.remove_owners(conn, &selected).await?; if User::owning(&krate, conn).await?.is_empty() { return Err(bad_request( "cannot remove all individual owners of a crate. \ @@ -637,17 +655,6 @@ impl From for OwnerAddError { } } -impl From for BoxedAppError { - fn from(error: OwnerRemoveError) -> Self { - match error { - OwnerRemoveError::Diesel(error) => error.into(), - OwnerRemoveError::NotFound { login } => { - bad_request(format!("could not find owner with login `{login}`")) - } - } - } -} - #[cfg(test)] mod tests { use super::Login; diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 947fc5b94f8..d4dedfe480c 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -1,7 +1,7 @@ use crate::builders::{CrateBuilder, PublishBuilder}; use crate::util::{MockAnonymousUser, MockCookieUser, MockTokenUser, RequestHelper, Response}; use crate::{TestApp, add_team_to_crate, new_team}; -use crates_io::models::{Crate, CrateOwner}; +use crates_io::models::{Crate, CrateOwner, OwnerKind}; use crates_io::schema::emails; use crates_io::views::{ EncodableCrateOwnerInvitationV1, EncodableOwner, EncodablePublicUser, InvitationResponse, @@ -229,9 +229,9 @@ async fn modify_multiple_owners() -> anyhow::Result<()> { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"cannot remove all individual owners of a crate. Team member don't have permission to modify owners, so at least one individual owner is required."}]}"#); assert_eq!(krate.owners(&conn).await?.len(), 3); - // Deleting two owners at once is allowed. + // Repeated names select the same owner from the initial owner list. let response = token - .remove_named_owners("owners_multiple", &["user2", "user3"]) + .remove_named_owners("owners_multiple", &["user2", "USER2", "user2", "user3"]) .await; assert_snapshot!(response.status(), @"200 OK"); assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); @@ -263,6 +263,13 @@ async fn modify_multiple_owners() -> anyhow::Result<()> { assert_eq!(krate.owners(&conn).await?.len(), 3); + let response = token + .remove_named_owners("owners_multiple", &["user2", "unknown"]) + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `unknown`"}]}"#); + assert_eq!(krate.owners(&conn).await?.len(), 3); + Ok(()) } @@ -405,7 +412,8 @@ async fn deleted_ownership_isnt_in_owner_user() { let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut conn) .await; - krate.owner_remove(&conn, &user.username).await.unwrap(); + let owners = [(OwnerKind::User, user.id)]; + krate.remove_owners(&conn, &owners).await.unwrap(); let json: UserResponse = anon .get("/api/v1/crates/foo_my_packages/owner_user") diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 4cca0245f79..36fa255b823 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -1,7 +1,7 @@ use crate::builders::{CrateBuilder, VersionBuilder}; use crate::util::{RequestHelper, TestApp}; use crate::{new_category, new_user}; -use crates_io::models::Category; +use crates_io::models::{Category, OwnerKind}; use crates_io::schema::{crates, version_downloads, versions}; use crates_io_database::schema::categories; use diesel::dsl::{IntervalDsl, now}; @@ -1372,7 +1372,8 @@ async fn crates_by_user_id_not_including_deleted_owners() -> anyhow::Result<()> let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut conn) .await; - krate.owner_remove(&conn, "foo").await.unwrap(); + let owners = [(OwnerKind::User, user.id)]; + krate.remove_owners(&conn, &owners).await.unwrap(); for response in search_both_by_user_id(&anon, user.id).await { assert_eq!(response.crates.len(), 0); diff --git a/src/tests/routes/me/get.rs b/src/tests/routes/me/get.rs index b1c1e618e37..d3b0b939207 100644 --- a/src/tests/routes/me/get.rs +++ b/src/tests/routes/me/get.rs @@ -1,5 +1,6 @@ use crate::builders::CrateBuilder; use crate::util::{RequestHelper, TestApp}; +use crates_io::models::OwnerKind; use crates_io::schema::users; use crates_io::views::{EncodablePrivateUser, OwnedCrate}; use diesel::prelude::*; @@ -61,10 +62,8 @@ async fn test_user_owned_crates_doesnt_include_deleted_ownership() { let krate = CrateBuilder::new("foo_my_packages", user_model.id) .expect_build(&mut conn) .await; - krate - .owner_remove(&conn, &user_model.username) - .await - .unwrap(); + let owners = [(OwnerKind::User, user_model.id)]; + krate.remove_owners(&conn, &owners).await.unwrap(); let json = user.show_me().await; assert_eq!(json.owned_crates.len(), 0); diff --git a/src/tests/routes/users/stats.rs b/src/tests/routes/users/stats.rs index 31daebd725e..537be15ad06 100644 --- a/src/tests/routes/users/stats.rs +++ b/src/tests/routes/users/stats.rs @@ -1,4 +1,5 @@ use crate::util::{RequestHelper, TestApp}; +use crates_io::models::OwnerKind; use serde::Deserialize; #[derive(Deserialize)] @@ -52,8 +53,9 @@ async fn user_total_downloads() -> anyhow::Result<()> { .set(crate_downloads::downloads.eq(5)) .execute(&mut conn) .await?; + let owners = [(OwnerKind::User, user.id)]; no_longer_my_krate - .owner_remove(&conn, &user.username) + .remove_owners(&conn, &owners) .await .unwrap(); diff --git a/src/tests/team.rs b/src/tests/team.rs index 7026a948890..2bdc8a8baee 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -1,6 +1,6 @@ use crate::builders::{CrateBuilder, PublishBuilder}; use crate::{OwnerTeamsResponse, RequestHelper, TestApp, add_team_to_crate, new_team}; -use crates_io::models::{Crate, CrateOwner, NewTeam}; +use crates_io::models::{Crate, CrateOwner, NewTeam, OwnerKind}; use crates_io::views::EncodableOwner; use diesel::*; @@ -486,7 +486,8 @@ async fn crates_by_team_id_not_including_deleted_owners() -> anyhow::Result<()> .expect_build(&mut conn) .await; add_team_to_crate(&t, &krate, user.id, &mut conn).await?; - krate.owner_remove(&conn, &t.login).await.unwrap(); + let owners = [(OwnerKind::Team, t.id)]; + krate.remove_owners(&conn, &owners).await.unwrap(); let json = anon.search(&format!("team_id={}", t.id)).await; assert_eq!(json.crates.len(), 0);