diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 086d381e733..5e97cb663fb 100644 --- a/src/controllers/krate/owners.rs +++ b/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 --git a/src/models/krate.rs b/src/models/krate.rs index 6bc6ca6aa06..94dcd28ed10 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -397,14 +397,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 --git a/src/models/owner.rs b/src/models/owner.rs index 3909d521844..3f2798491e6 100644 --- a/src/models/owner.rs +++ b/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 { + 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 --git a/src/tests/owners.rs b/src/tests/owners.rs index 0035c3073e9..7849abdfb51 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -567,9 +567,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 --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index e059bba47e8..575e579b776 100644 --- a/src/tests/routes/crates/list.rs +++ b/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 --git a/src/tests/routes/me/get.rs b/src/tests/routes/me/get.rs index 2bc038d1f68..772bce83d12 100644 --- a/src/tests/routes/me/get.rs +++ b/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 --git a/src/tests/routes/users/stats.rs b/src/tests/routes/users/stats.rs index 371ea42befb..fd98afcab00 100644 --- a/src/tests/routes/users/stats.rs +++ b/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 --git a/src/tests/team.rs b/src/tests/team.rs index 9e18142173d..8723add2d20 100644 --- a/src/tests/team.rs +++ b/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 });