Fix checks and consistency in the API endpoints to manage ownership.

src/controllers/krate/owners.rs

@@ -68,70 +68,83 @@ pub fn remove_owners(req: &mut dyn Request) -> CargoResult<Response> {
     modify_owners(req, false)
 }
 
-fn modify_owners(req: &mut dyn Request, add: bool) -> CargoResult<Response> {
+/// Parse the JSON request body of requests to modify the owners of a crate.
+/// The format is
+///
+///     {"owners": ["username", "github:org:team", ...]}
+fn parse_owners_request(req: &mut dyn Request) -> CargoResult<Vec<String>> {
     let mut body = String::new();
     req.body().read_to_string(&mut body)?;
-
-    let user = req.user()?;
-    let conn = req.db_conn()?;
-    let krate = Crate::by_name(&req.params()["crate_id"]).first::<Crate>(&*conn)?;
-    let owners = krate.owners(&conn)?;
-
-    match user.rights(req.app(), &owners)? {
-        Rights::Full => {}
-        // Yes!
-        Rights::Publish => {
-            return Err(human("team members don't have permission to modify owners"));
-        }
-        Rights::None => {
-            return Err(human("only owners have permission to modify owners"));
-        }
-    }
-
     #[derive(Deserialize)]
     struct Request {
         // identical, for back-compat (owners preferred)
         users: Option<Vec<String>>,
         owners: Option<Vec<String>>,
     }
-
     let request: Request =
         serde_json::from_str(&body).map_err(|_| human("invalid json request"))?;
-
-    let logins = request
+    request
         .owners
         .or(request.users)
-        .ok_or_else(|| human("invalid json request"))?;
+        .ok_or_else(|| human("invalid json request"))
+}
+
+fn modify_owners(req: &mut dyn Request, add: bool) -> CargoResult<Response> {
+    let logins = parse_owners_request(req)?;
+    let app = req.app();
+    let user = req.user()?;
+    let crate_name = &req.params()["crate_id"];
+    let conn = req.db_conn()?;
 
-    let mut msgs = Vec::new();
+    conn.transaction(|| {
+        let krate = Crate::by_name(crate_name).first::<Crate>(&*conn)?;
+        let owners = krate.owners(&conn)?;
 
-    for login in &logins {
-        if add {
-            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)));
+        match user.rights(app, &owners)? {
+            Rights::Full => {}
+            // Yes!
+            Rights::Publish => {
+                return Err(human("team members don't have permission to modify owners"));
             }
-            let msg = krate.owner_add(req.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"));
+            Rights::None => {
+                return Err(human("only owners have permission to modify owners"));
             }
-            krate.owner_remove(req.app(), &conn, user, login)?;
         }
-    }
 
-    let comma_sep_msg = msgs.join(",");
+        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(app, &conn, user, login)?;
+                msgs.push(msg);
+            }
+            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 {
-        ok: bool,
-        msg: String,
-    }
-    Ok(req.json(&R {
-        ok: true,
-        msg: comma_sep_msg,
-    }))
+        #[derive(Serialize)]
+        struct R {
+            ok: bool,
+            msg: String,
+        }
+        Ok(req.json(&R {
+            ok: true,
+            msg: comma_sep_msg,
+        }))
+    })
 }

src/tests/owners.rs

@@ -2,7 +2,7 @@ use crate::{
     add_team_to_crate,
     builders::{CrateBuilder, PublishBuilder},
     new_team,
-    util::RequestHelper,
+    util::{MockCookieUser, MockTokenUser, RequestHelper},
     TestApp,
 };
 use cargo_registry::{
@@ -26,7 +26,7 @@ struct InvitationListResponse {
 }
 
 // Implementing locally for now, unless these are needed elsewhere
-impl crate::util::MockCookieUser {
+impl MockCookieUser {
     /// As the currently logged in user, accept an invitation to become an owner of the named
     /// crate.
     fn accept_ownership_invitation(&self, krate_name: &str, krate_id: i32) {
@@ -111,43 +111,98 @@ fn new_crate_owner() {
         .good();
 }
 
+fn create_and_add_owner(
+    app: &TestApp,
+    token: &MockTokenUser,
+    username: &str,
+    krate: &Crate,
+) -> MockCookieUser {
+    let user = app.db_new_user(username);
+    token.add_user_owner(&krate.name, user.as_model());
+    user.accept_ownership_invitation(&krate.name, krate.id);
+    user
+}
+
 // Ensures that so long as at least one owner remains associated with the crate,
 // a user can still remove their own login as an owner
 #[test]
 fn owners_can_remove_self() {
     let (app, _, user, token) = TestApp::init().with_token();
+    let username = &user.as_model().gh_login;
 
     let krate = app
         .db(|conn| CrateBuilder::new("owners_selfremove", user.as_model().id).expect_build(conn));
 
     // Deleting yourself when you're the only owner isn't allowed.
     let json = token
-        .remove_named_owner("owners_selfremove", &user.as_model().gh_login)
+        .remove_named_owner("owners_selfremove", username)
         .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"));
 
-    let user2 = app.db_new_user("secondowner");
-    token.add_user_owner("owners_selfremove", user2.as_model());
-    user2.accept_ownership_invitation("owners_selfremove", krate.id);
+    create_and_add_owner(&app, &token, "secondowner", &krate);
 
     // Deleting yourself when there are other owners is allowed.
     let json = token
-        .remove_named_owner("owners_selfremove", &user.as_model().gh_login)
+        .remove_named_owner("owners_selfremove", username)
         .good();
     assert!(json.ok);
 
     // After you delete yourself, you no longer have permisions to manage the crate.
     let json = token
-        .remove_named_owner("owners_selfremove", &user2.as_model().gh_login)
+        .remove_named_owner("owners_selfremove", username)
         .bad_with_status(200);
-
     assert!(json.errors[0]
         .detail
         .contains("only owners have permission to modify owners",));
 }
 
+// Verify consistency when adidng or removing multiple owners in a single request.
+#[test]
+fn modify_multiple_owners() {
+    let (app, _, user, token) = TestApp::init().with_token();
+    let username = &user.as_model().gh_login;
+
+    let krate =
+        app.db(|conn| CrateBuilder::new("owners_multiple", user.as_model().id).expect_build(conn));
+
+    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"])
+        .good();
+    assert!(json.ok);
+    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 1);
+
+    // Adding multiple users fails if one of them already is an owner.
+    let json = token
+        .add_named_owners("owners_multiple", &["user2", username])
+        .bad_with_status(200);
+    assert!(&json.errors[0].detail.contains("is already an owner"));
+    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 1);
+
+    // Adding multiple users at once succeeds.
+    let json = token
+        .add_named_owners("owners_multiple", &["user2", "user3"])
+        .good();
+    assert!(json.ok);
+    user2.accept_ownership_invitation(&krate.name, krate.id);
+    user3.accept_ownership_invitation(&krate.name, krate.id);
+    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 3);
+}
+
 /*  Testing the crate ownership between two crates and one team.
     Given two crates, one crate owned by both a team and a user,
     one only owned by a user, check that the CrateList returned

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

src/tests/user.rs

@@ -46,18 +46,19 @@ impl crate::util::MockCookieUser {
     // TODO: I don't like the name of this method or the one above; this is starting to look like
     // a builder might help? I want to explore alternative abstractions in any case
     fn update_email_more_control(&self, user_id: i32, email: Option<&str>) -> Response<OkBool> {
-        let email_json = match email {
-            Some(val) => format!("\"{}\"", val),
-            None => String::from("null"),
-        };
-
         // When updating your email in crates.io, the request goes to the user route with PUT.
         // Ember sends all the user attributes. We check to make sure the ID in the URL matches
         // the ID of the currently logged in user, then we ignore everything but the email address.
-        let body = format!("{{\"user\":{{\"email\":{},\"name\":\"Arbitrary Name\",\"login\":\"arbitrary_login\",\"avatar\":\"https://arbitrary.com/img.jpg\",\"url\":\"https://arbitrary.com\",\"kind\":null}}}}", email_json);
+        let body = json!({"user": {
+            "email": email,
+            "name": "Arbitrary Name",
+            "login": "arbitrary_login",
+            "avatar": "https://arbitrary.com/img.jpg",
+            "url": "https://arbitrary.com",
+            "kind": null
+        }});
         let url = format!("/api/v1/users/{}", user_id);
-
-        self.put(&url, body.as_bytes())
+        self.put(&url, body.to_string().as_bytes())
     }
 
     fn confirm_email(&self, email_token: &str) -> OkBool {
@@ -70,18 +71,19 @@ impl crate::util::MockAnonymousUser {
     // TODO: Refactor to get rid of this duplication with the same method implemented on
     // MockCookieUser
     fn update_email_more_control(&self, user_id: i32, email: Option<&str>) -> Response<OkBool> {
-        let email_json = match email {
-            Some(val) => format!("\"{}\"", val),
-            None => String::from("null"),
-        };
-
         // When updating your email in crates.io, the request goes to the user route with PUT.
         // Ember sends all the user attributes. We check to make sure the ID in the URL matches
         // the ID of the currently logged in user, then we ignore everything but the email address.
-        let body = format!("{{\"user\":{{\"email\":{},\"name\":\"Arbitrary Name\",\"login\":\"arbitrary_login\",\"avatar\":\"https://arbitrary.com/img.jpg\",\"url\":\"https://arbitrary.com\",\"kind\":null}}}}", email_json);
+        let body = json!({"user": {
+            "email": email,
+            "name": "Arbitrary Name",
+            "login": "arbitrary_login",
+            "avatar": "https://arbitrary.com/img.jpg",
+            "url": "https://arbitrary.com",
+            "kind": null
+        }});
         let url = format!("/api/v1/users/{}", user_id);
-
-        self.put(&url, body.as_bytes())
+        self.put(&url, body.to_string().as_bytes())
     }
 }
 

src/tests/util.rs

@@ -497,18 +497,33 @@ impl MockTokenUser {
         &self.token
     }
 
-    /// Add to the specified crate the specified owner.
+    /// Add to the specified crate the specified owners.
+    pub fn add_named_owners(&self, krate_name: &str, owners: &[&str]) -> Response<OkBool> {
+        self.modify_owners(krate_name, owners, Self::put)
+    }
+
+    /// Add a single owner to the specified crate.
     pub fn add_named_owner(&self, krate_name: &str, owner: &str) -> Response<OkBool> {
-        let url = format!("/api/v1/crates/{}/owners", krate_name);
-        let body = format!("{{\"users\":[\"{}\"]}}", owner);
-        self.put(&url, body.as_bytes())
+        self.add_named_owners(krate_name, &[owner])
+    }
+
+    /// Remove from the specified crate the specified owners.
+    pub fn remove_named_owners(&self, krate_name: &str, owners: &[&str]) -> Response<OkBool> {
+        self.modify_owners(krate_name, owners, Self::delete_with_body)
     }
 
-    /// Remove from the specified crate the specified owner.
+    /// Remove a single owner to the specified crate.
     pub fn remove_named_owner(&self, krate_name: &str, owner: &str) -> Response<OkBool> {
+        self.remove_named_owners(krate_name, &[owner])
+    }
+
+    fn modify_owners<F>(&self, krate_name: &str, owners: &[&str], method: F) -> Response<OkBool>
+    where
+        F: Fn(&MockTokenUser, &str, &[u8]) -> Response<OkBool>,
+    {
         let url = format!("/api/v1/crates/{}/owners", krate_name);
-        let body = format!("{{\"users\":[\"{}\"]}}", owner);
-        self.delete_with_body(&url, body.as_bytes())
+        let body = json!({ "owners": owners }).to_string();
+        method(&self, &url, body.to_string().as_bytes())
     }
 
     /// Add a user as an owner for a crate.