Allow removing deleted organizations from crate owners (#7051)

Author: nic-hartley

Diff for: src/controllers/krate/owners.rs

@@ -141,7 +141,7 @@ fn modify_owners(
             msgs.join(",")
         } else {
             for login in &logins {
-                krate.owner_remove(app, conn, user, login)?;
+                krate.owner_remove(conn, login)?;
             }
             if User::owning(&krate, conn)?.is_empty() {
                 return Err(cargo_err(

Diff for: src/models/krate.rs

@@ -339,14 +339,8 @@ impl Crate {
         }
     }
 
-    pub fn owner_remove(
-        &self,
-        app: &App,
-        conn: &mut PgConnection,
-        req_user: &User,
-        login: &str,
-    ) -> AppResult<()> {
-        let owner = Owner::find_or_create_by_login(app, conn, req_user, login)?;
+    pub fn owner_remove(&self, conn: &mut PgConnection, login: &str) -> AppResult<()> {
+        let owner = Owner::find_by_login(conn, login)?;
 
         let target = crate_owners::table.find((self.id(), owner.id(), owner.kind()));
         diesel::update(target)

Diff for: src/models/owner.rs

@@ -1,8 +1,8 @@
 use diesel::pg::Pg;
 use diesel::prelude::*;
 
-use crate::app::App;
 use crate::util::errors::{cargo_err, AppResult};
+use crate::{app::App, schema::teams};
 
 use crate::models::{Crate, Team, User};
 use crate::schema::{crate_owners, users};
@@ -59,6 +59,7 @@ impl Owner {
     /// up-to-date GitHub ID. Fails out if the user isn't found in the
     /// database, the team isn't found on GitHub, or if the user isn't a member
     /// of the team on GitHub.
+    ///
     /// May be a user's GH login or a full team name. This is case
     /// sensitive.
     pub fn find_or_create_by_login(
@@ -82,6 +83,30 @@ impl Owner {
         }
     }
 
+    /// Finds the owner by name. Never recreates a team, to ensure that
+    /// organizations that were deleted after they were added can still be
+    /// removed.
+    ///
+    /// May be a user's GH login or a full team name. This is case
+    /// sensitive.
+    pub fn find_by_login(conn: &mut PgConnection, name: &str) -> AppResult<Owner> {
+        if name.contains(':') {
+            teams::table
+                .filter(lower(teams::login).eq(&name.to_lowercase()))
+                .first(conn)
+                .map(Owner::Team)
+                .map_err(|_| cargo_err(&format_args!("could not find team with login `{name}`")))
+        } else {
+            users::table
+                .filter(lower(users::gh_login).eq(name.to_lowercase()))
+                .filter(users::gh_id.ne(-1))
+                .order(users::gh_id.desc())
+                .first(conn)
+                .map(Owner::User)
+                .map_err(|_| cargo_err(&format_args!("could not find user with login `{name}`")))
+        }
+    }
+
     pub fn kind(&self) -> i32 {
         match *self {
             Owner::User(_) => OwnerKind::User as i32,

Diff for: src/tests/owners.rs

@@ -566,9 +566,7 @@ fn deleted_ownership_isnt_in_owner_user() {
 
     app.db(|conn| {
         let krate = CrateBuilder::new("foo_my_packages", user.id).expect_build(conn);
-        krate
-            .owner_remove(app.as_inner(), conn, user, &user.gh_login)
-            .unwrap();
+        krate.owner_remove(conn, &user.gh_login).unwrap();
     });
 
     let json: UserResponse = anon.get("/api/v1/crates/foo_my_packages/owner_user").good();

Diff for: src/tests/routes/crates/list.rs

@@ -839,9 +839,7 @@ fn crates_by_user_id_not_including_deleted_owners() {
 
     app.db(|conn| {
         let krate = CrateBuilder::new("foo_my_packages", user.id).expect_build(conn);
-        krate
-            .owner_remove(app.as_inner(), conn, user, "foo")
-            .unwrap();
+        krate.owner_remove(conn, "foo").unwrap();
     });
 
     let response = anon.search_by_user_id(user.id);

Diff for: src/tests/routes/me/get.rs

@@ -42,9 +42,7 @@ fn test_user_owned_crates_doesnt_include_deleted_ownership() {
 
     app.db(|conn| {
         let krate = CrateBuilder::new("foo_my_packages", user_model.id).expect_build(conn);
-        krate
-            .owner_remove(app.as_inner(), conn, user_model, &user_model.gh_login)
-            .unwrap();
+        krate.owner_remove(conn, &user_model.gh_login).unwrap();
     });
 
     let json = user.show_me();

Diff for: src/tests/routes/users/stats.rs

@@ -39,7 +39,7 @@ fn user_total_downloads() {
             .execute(conn)
             .unwrap();
         no_longer_my_krate
-            .owner_remove(app.as_inner(), conn, user, &user.gh_login)
+            .owner_remove(conn, &user.gh_login)
             .unwrap();
     });
 

Diff for: src/tests/team.rs

@@ -3,7 +3,10 @@ use crate::{
     builders::{CrateBuilder, PublishBuilder},
     new_team, OwnerTeamsResponse, RequestHelper, TestApp,
 };
-use crates_io::models::{Crate, NewTeam};
+use crates_io::{
+    models::{Crate, NewTeam},
+    schema::teams,
+};
 
 use diesel::*;
 use http::StatusCode;
@@ -67,14 +70,15 @@ fn one_colon() {
 }
 
 #[test]
-fn nonexistent_team() {
+fn add_nonexistent_team() {
     let (app, _, user, token) = TestApp::init().with_token();
 
     app.db(|conn| {
-        CrateBuilder::new("foo_nonexistent", user.as_model().id).expect_build(conn);
+        CrateBuilder::new("foo_add_nonexistent", user.as_model().id).expect_build(conn);
     });
 
-    let response = token.add_named_owner("foo_nonexistent", "github:test-org:this-does-not-exist");
+    let response =
+        token.add_named_owner("foo_add_nonexistent", "github:test-org:this-does-not-exist");
     assert_eq!(response.status(), StatusCode::OK);
     assert_eq!(
         response.into_json(),
@@ -268,6 +272,29 @@ fn remove_team_as_team_owner() {
     );
 }
 
+#[test]
+fn remove_nonexistent_team() {
+    let (app, _, user, token) = TestApp::init().with_token();
+
+    app.db(|conn| {
+        CrateBuilder::new("foo_remove_nonexistent", user.as_model().id).expect_build(conn);
+        insert_into(teams::table)
+            .values((
+                teams::login.eq("github:test-org:this-does-not-exist"),
+                teams::github_id.eq(5678),
+            ))
+            .execute(conn)
+            .expect("couldn't insert nonexistent team")
+    });
+
+    token
+        .remove_named_owner(
+            "foo_remove_nonexistent",
+            "github:test-org:this-does-not-exist",
+        )
+        .good();
+}
+
 /// Test trying to publish a crate we don't own
 #[test]
 fn publish_not_owned() {
@@ -422,9 +449,7 @@ fn crates_by_team_id_not_including_deleted_owners() {
 
         let krate = CrateBuilder::new("foo", user.id).expect_build(conn);
         add_team_to_crate(&t, &krate, user, conn).unwrap();
-        krate
-            .owner_remove(app.as_inner(), conn, user, &t.login)
-            .unwrap();
+        krate.owner_remove(conn, &t.login).unwrap();
         t
     });