Auto merge of #1772 - smarnach:fix-owner-checks, r=sgrif

bors · bors · commit 2a036e552c61 · 2019-07-16T17:59:11.000Z
Fix checks and consistency in the API endpoints to manage ownership. Fixes #1583. The API endpoints to add and remove owners for a crate has multiple issues. * The database interactions are not wrapped in a transaction, making them prone to race conditions. * The lack of a transaction can also result in partially applied operations, e.g. when adding or removing multiple owners at once. * The check whether the last owner was removed only triggers if the _original_ list of owners had a length of exactly one. This is wrong in at least two cases: * Teams do not have permission to manage ownership, so they should not be counted. * If multiple users are removed at once, we can end up with no owners even when there originally was more than one user. This change fixes all mentioned problems, and adds tests for all bullet points except for the first one (which can't be tested with reasonable effort).
diff --git a/src/controllers/krate/owners.rs b/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,
+        }))
+    })
 }
diff --git a/src/tests/owners.rs b/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
diff --git a/src/tests/team.rs b/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();
diff --git a/src/tests/user.rs b/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())
     }
 }
 
diff --git a/src/tests/util.rs b/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.
