diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index bfd761b6059..4c9e58b0027 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -1,17 +1,16 @@ 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; 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; @@ -213,50 +212,33 @@ impl Crate { Ok(users.chain(teams).collect()) } - pub async fn owner_remove( + /// 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, - 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) + 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?; - - if num_updated_rows == 0 { - return Err(OwnerRemoveError::not_found(login)); - } - - Ok(()) + .await } } @@ -270,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/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(); +} 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);