controllers/owners: Resolve removal owner names in Rust

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::<Integer, _>(self.id)
-            .bind::<Text, _>(login)
+        owners: &[(OwnerKind, i32)],
+    ) -> QueryResult<usize> {
+        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 }
-    }
-}

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();
+}

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<BoxedAppError> for OwnerAddError {
     }
 }
 
-impl From<OwnerRemoveError> 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;

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")

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);

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);