Fix owner removal check.

Author: smarnach

src/controllers/krate/owners.rs

@@ -111,28 +111,31 @@ fn modify_owners(req: &mut dyn Request, add: bool) -> CargoResult<Response> {
             }
         }
 
-        let mut msgs = Vec::new();
-
-        for login in &logins {
-            if add {
+        let comma_sep_msg = if add {
+            let mut msgs = Vec::new();
+            for login in &logins {
                 let login_test =
                     |owner: &Owner| owner.login().to_lowercase() == *login.to_lowercase();
                 if owners.iter().any(login_test) {
                     return Err(human(&format_args!("`{}` is already an owner", login)));
                 }
-                let msg = krate.owner_add(req.app(), &conn, user, login)?;
+                let msg = krate.owner_add(app, &conn, user, login)?;
                 msgs.push(msg);
-            } else {
-                // Removing the team that gives you rights is prevented because
-                // team members only have Rights::Publish
-                if owners.len() == 1 {
-                    return Err(human("cannot remove the sole owner of a crate"));
-                }
-                krate.owner_remove(req.app(), &conn, user, login)?;
             }
-        }
-
-        let comma_sep_msg = msgs.join(",");
+            msgs.join(",")
+        } else {
+            for login in &logins {
+                krate.owner_remove(app, &conn, user, login)?;
+            }
+            if User::owning(&krate, &conn)?.is_empty() {
+                return Err(human(
+                    "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.",
+                ));
+            }
+            "owners successfully removed".to_owned()
+        };
 
         #[derive(Serialize)]
         struct R {

src/tests/owners.rs

@@ -139,7 +139,7 @@ fn owners_can_remove_self() {
         .bad_with_status(200);
     assert!(json.errors[0]
         .detail
-        .contains("cannot remove the sole owner of a crate"));
+        .contains("cannot remove all individual owners of a crate"));
 
     create_and_add_owner(&app, &token, "secondowner", &krate);
 
@@ -170,6 +170,15 @@ fn modify_multiple_owners() {
     let user2 = create_and_add_owner(&app, &token, "user2", &krate);
     let user3 = create_and_add_owner(&app, &token, "user3", &krate);
 
+    // Deleting all owners is not allowed.
+    let json = token
+        .remove_named_owners("owners_multiple", &[username, "user2", "user3"])
+        .bad_with_status(200);
+    assert!(&json.errors[0]
+        .detail
+        .contains("cannot remove all individual owners of a crate"));
+    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 3);
+
     // Deleting two owners at once is allowed.
     let json = token
         .remove_named_owners("owners_multiple", &["user2", "user3"])

src/tests/team.rs

@@ -184,7 +184,8 @@ fn add_team_as_non_member() {
 #[test]
 fn remove_team_as_named_owner() {
     let (app, _) = TestApp::with_proxy().empty();
-    let user_on_both_teams = app.db_new_user(mock_user_on_both_teams().gh_login);
+    let username = mock_user_on_both_teams().gh_login;
+    let user_on_both_teams = app.db_new_user(username);
     let token_on_both_teams = user_on_both_teams.db_new_token("arbitrary token name");
 
     app.db(|conn| {
@@ -195,6 +196,15 @@ fn remove_team_as_named_owner() {
         .add_named_owner("foo_remove_team", "github:crates-test-org:core")
         .good();
 
+    // Removing the individual owner is not allowed, since team members don't
+    // have permission to manage ownership
+    let json = token_on_both_teams
+        .remove_named_owner("foo_remove_team", username)
+        .bad_with_status(200);
+    assert!(json.errors[0]
+        .detail
+        .contains("cannot remove all individual owners of a crate"));
+
     token_on_both_teams
         .remove_named_owner("foo_remove_team", "github:crates-test-org:core")
         .good();