Wrap owner modification endpoints in database transaction.

Author: smarnach

src/controllers/krate/owners.rs

@@ -91,51 +91,57 @@ fn parse_owners_request(req: &mut dyn Request) -> CargoResult<Vec<String>> {
 
 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 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"));
-        }
-    }
 
-    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"));
+            }
+        }
+
+        let mut msgs = Vec::new();
+
+        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)));
+                }
+                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"));
+                }
+                krate.owner_remove(req.app(), &conn, user, login)?;
             }
-            krate.owner_remove(req.app(), &conn, user, login)?;
         }
-    }
 
-    let comma_sep_msg = msgs.join(",");
+        let comma_sep_msg = msgs.join(",");
 
-    #[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,89 @@ 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"));
 
-    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 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/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 = format!("{{\"owners\":[\"{}\"]}}", owners.join("\", \""));
+        method(&self, &url, body.as_bytes())
     }
 
     /// Add a user as an owner for a crate.