Merge #1108

1108: Require owner approval r=carols10cents

This PR addresses issue #924, adding owners with their consent, deployable chunk 4. It implements the Crates functionality necessary for Cargo to invite a user to be an owner instead of automatically adding any user to be an owner of any crate. These changes correlate with the Cargo changes made in [this PR](rust-lang/cargo#4551). Function `owner_add` now creates an owner invitation instead of adding a user to be an owner, and returns a `boolean` okay value with a `String` message indicating that the user was invited to be an owner of the crate. This `boolean` okay value is not explicitly used for anything but is necessary to support older versions of Cargo, without which older versions will fail to decode only a `String` response.

Author: bors-voyager[bot]

Diff for: app/templates/application.hbs

@@ -65,6 +65,7 @@
                 {{#rl-dropdown tagName="ul" class="dropdown current-user-links" closeOnChildClick="a:link"}}
                     <li>{{#link-to 'dashboard'}}Dashboard{{/link-to}}</li>
                     <li>{{#link-to 'me'}}Account Settings{{/link-to}}</li>
+                    <li>{{#link-to 'me.pending-invites'}}Owner Invites{{/link-to}}</li>
                     <li class='last'>{{#link-to 'logout'}}Sign Out{{/link-to}}</li>
                 {{/rl-dropdown}}
             {{/rl-dropdown-container}}

Diff for: src/crate_owner_invitation.rs

@@ -121,8 +121,10 @@ fn accept_invite(
     conn: &PgConnection,
     crate_invite: InvitationResponse,
 ) -> CargoResult<Response> {
-    let user_id = req.user()?.id;
     use diesel::{delete, insert};
+    use diesel::pg::upsert::{do_update, OnConflictExtension};
+
+    let user_id = req.user()?.id;
     let pending_crate_owner = crate_owner_invitations::table
         .filter(crate_owner_invitations::crate_id.eq(crate_invite.crate_id))
         .filter(crate_owner_invitations::invited_user_id.eq(user_id))
@@ -136,7 +138,11 @@ fn accept_invite(
     };
 
     conn.transaction(|| {
-        insert(&owner).into(crate_owners::table).execute(conn)?;
+        insert(&owner.on_conflict(
+            crate_owners::table.primary_key(),
+            do_update().set(crate_owners::deleted.eq(false)),
+        )).into(crate_owners::table)
+            .execute(conn)?;
         delete(
             crate_owner_invitations::table
                 .filter(crate_owner_invitations::crate_id.eq(crate_invite.crate_id))

Diff for: src/krate/mod.rs

@@ -27,6 +27,7 @@ use download::{EncodableVersionDownload, VersionDownload};
 use git;
 use keyword::{CrateKeyword, EncodableKeyword};
 use owner::{rights, CrateOwner, EncodableOwner, Owner, OwnerKind, Rights, Team};
+use crate_owner_invitation::NewCrateOwnerInvitation;
 use pagination::Paginate;
 use render;
 use schema::*;
@@ -460,22 +461,52 @@ impl Crate {
         conn: &PgConnection,
         req_user: &User,
         login: &str,
-    ) -> CargoResult<()> {
+    ) -> CargoResult<String> {
+        use diesel::insert;
+
         let owner = Owner::find_or_create_by_login(app, conn, req_user, login)?;
 
-        let crate_owner = CrateOwner {
-            crate_id: self.id,
-            owner_id: owner.id(),
-            created_by: req_user.id,
-            owner_kind: owner.kind() as i32,
-        };
-        diesel::insert(&crate_owner.on_conflict(
-            crate_owners::table.primary_key(),
-            do_update().set(crate_owners::deleted.eq(false)),
-        )).into(crate_owners::table)
-            .execute(conn)?;
+        match owner {
+            // Users are invited and must accept before being added
+            owner @ Owner::User(_) => {
+                let owner_invitation = NewCrateOwnerInvitation {
+                    invited_user_id: owner.id(),
+                    invited_by_user_id: req_user.id,
+                    crate_id: self.id,
+                };
 
-        Ok(())
+                diesel::insert(&owner_invitation.on_conflict_do_nothing())
+                    .into(crate_owner_invitations::table)
+                    .execute(conn)?;
+
+                Ok(format!(
+                    "user {} has been invited to be an owner of crate {}",
+                    owner.login(),
+                    self.name
+                ))
+            }
+            // Teams are added as owners immediately
+            owner @ Owner::Team(_) => {
+                let crate_owner = CrateOwner {
+                    crate_id: self.id,
+                    owner_id: owner.id(),
+                    created_by: req_user.id,
+                    owner_kind: OwnerKind::Team as i32,
+                };
+
+                insert(&crate_owner.on_conflict(
+                    crate_owners::table.primary_key(),
+                    do_update().set(crate_owners::deleted.eq(false)),
+                )).into(crate_owners::table)
+                    .execute(conn)?;
+
+                Ok(format!(
+                    "team {} has been added as an owner of crate {}",
+                    owner.login(),
+                    self.name
+                ))
+            }
+        }
     }
 
     pub fn owner_remove(
@@ -924,8 +955,10 @@ pub fn new(req: &mut Request) -> CargoResult<Response> {
         let owners = krate.owners(&conn)?;
         if rights(req.app(), &owners, &user)? < Rights::Publish {
             return Err(human(
-                "crate name has already been claimed by \
-                 another user",
+                "this crate exists but you don't seem to be an owner. \
+                 If you believe this is a mistake, perhaps you need \
+                 to accept an invitation to be an owner before \
+                 publishing.",
             ));
         }
 
@@ -1373,12 +1406,15 @@ fn modify_owners(req: &mut Request, add: bool) -> CargoResult<Response> {
         .or(request.users)
         .ok_or_else(|| human("invalid json request"))?;
 
+    let mut msgs = Vec::new();
+
     for login in &logins {
         if add {
             if owners.iter().any(|owner| owner.login() == *login) {
                 return Err(human(&format_args!("`{}` is already an owner", login)));
             }
-            krate.owner_add(req.app(), &conn, user, 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
@@ -1389,11 +1425,17 @@ fn modify_owners(req: &mut Request, add: bool) -> CargoResult<Response> {
         }
     }
 
+    let comma_sep_msg = msgs.join(",");
+
     #[derive(Serialize)]
     struct R {
         ok: bool,
+        msg: String,
     }
-    Ok(req.json(&R { ok: true }))
+    Ok(req.json(&R {
+        ok: true,
+        msg: comma_sep_msg,
+    }))
 }
 
 /// Handles the `GET /crates/:crate_id/reverse_dependencies` route.

Diff for: src/tests/all.rs

@@ -100,7 +100,7 @@ mod token;
 mod user;
 mod version;
 
-#[derive(Deserialize)]
+#[derive(Deserialize, Debug)]
 struct GoodCrate {
     #[serde(rename = "crate")] krate: EncodableCrate,
     warnings: Warnings,
@@ -110,7 +110,7 @@ struct CrateList {
     crates: Vec<EncodableCrate>,
     meta: CrateMeta,
 }
-#[derive(Deserialize)]
+#[derive(Deserialize, Debug)]
 struct Warnings {
     invalid_categories: Vec<String>,
     invalid_badges: Vec<String>,

Diff for: src/tests/krate.rs

@@ -699,7 +699,9 @@ fn new_krate_wrong_user() {
 
     let json = bad_resp!(middle.call(&mut req));
     assert!(
-        json.errors[0].detail.contains("another user"),
+        json.errors[0]
+            .detail
+            .contains("this crate exists but you don't seem to be an owner.",),
         "{:?}",
         json.errors
     );
@@ -2003,6 +2005,56 @@ fn block_blacklisted_documentation_url() {
     assert_eq!(json.krate.documentation, None);
 }
 
+// This is testing Cargo functionality! ! !
+// specifically functions modify_owners and add_owners
+// which call the `PUT /crates/:crate_id/owners` route
+#[test]
+fn test_cargo_invite_owners() {
+    let (_b, app, middle) = ::app();
+    let mut req = ::req(app.clone(), Method::Get, "/");
+
+    let new_user = {
+        let conn = app.diesel_database.get().unwrap();
+        let owner = ::new_user("avocado").create_or_update(&conn).unwrap();
+        ::sign_in_as(&mut req, &owner);
+        ::CrateBuilder::new("guacamole", owner.id).expect_build(&conn);
+        ::new_user("cilantro").create_or_update(&conn).unwrap()
+    };
+
+    #[derive(Serialize)]
+    struct OwnerReq {
+        owners: Option<Vec<String>>,
+    }
+    #[derive(Deserialize, Debug)]
+    struct OwnerResp {
+        ok: bool,
+        msg: String,
+    }
+
+    let body = serde_json::to_string(&OwnerReq {
+        owners: Some(vec![new_user.gh_login]),
+    });
+    let mut response = ok_resp!(
+        middle.call(
+            req.with_path("/api/v1/crates/guacamole/owners")
+                .with_method(Method::Put)
+                .with_body(body.unwrap().as_bytes()),
+        )
+    );
+
+    let r = ::json::<OwnerResp>(&mut response);
+    // this ok:true field is what old versions of Cargo
+    // need - do not remove unless you're cool with
+    // dropping support for old versions
+    assert!(r.ok);
+    // msg field is what is sent and used in updated
+    // version of cargo
+    assert_eq!(
+        r.msg,
+        "user cilantro has been invited to be an owner of crate guacamole"
+    )
+}
+
 // #[test]
 // fn new_crate_bad_tarball() {
 //     let (_b, app, middle) = ::app();

Diff for: src/tests/owners.rs

@@ -5,6 +5,7 @@ use cargo_registry::user::EncodablePublicUser;
 use cargo_registry::crate_owner_invitation::{EncodableCrateOwnerInvitation, InvitationResponse,
                                              NewCrateOwnerInvitation};
 use cargo_registry::schema::crate_owner_invitations;
+use cargo_registry::krate::Crate;
 
 use conduit::{Handler, Method};
 use diesel;
@@ -49,14 +50,52 @@ fn new_crate_owner() {
         )
     );
     assert!(::json::<O>(&mut response).ok);
-    bad_resp!(
+
+    let krate_id = {
+        let conn = app.diesel_database.get().unwrap();
+        Crate::by_name("foo_owner")
+            .first::<Crate>(&*conn)
+            .unwrap()
+            .id
+    };
+
+    let body = json!({
+        "crate_owner_invite": {
+            "invited_by_username": "foo",
+            "crate_name": "foo_owner",
+            "crate_id": krate_id,
+            "created_at": "",
+            "accepted": true
+        }
+    });
+
+    ::logout(&mut req);
+    ::sign_in_as(&mut req, &u2);
+
+    // accept invitation for user to be added as owner
+    let mut response = ok_resp!(
         middle.call(
-            req.with_path("/api/v1/crates/foo_owner/owners")
+            req.with_path(&format!("/api/v1/me/crate_owner_invitations/{}", krate_id))
                 .with_method(Method::Put)
-                .with_body(body.as_bytes()),
+                .with_body(body.to_string().as_bytes()),
         )
     );
 
+    #[derive(Deserialize)]
+    struct CrateOwnerInvitation {
+        crate_owner_invitation: InvitationResponse,
+    }
+
+    #[derive(Deserialize)]
+    struct InvitationResponse {
+        crate_id: i32,
+        accepted: bool,
+    }
+
+    let crate_owner_invite = ::json::<CrateOwnerInvitation>(&mut response);
+    assert!(crate_owner_invite.crate_owner_invitation.accepted);
+    assert_eq!(crate_owner_invite.crate_owner_invitation.crate_id, krate_id);
+
     // Make sure this shows up as one of their crates.
     let query = format!("user_id={}", u2.id);
     let mut response = ok_resp!(
@@ -100,13 +139,14 @@ fn owners_can_remove_self() {
         Method::Get,
         "/api/v1/crates/owners_selfremove/owners",
     );
-    {
+    let (first_owner, second_owner) = {
         let conn = app.diesel_database.get().unwrap();
-        ::new_user("secondowner").create_or_update(&conn).unwrap();
         let user = ::new_user("firstowner").create_or_update(&conn).unwrap();
+        let user_two = ::new_user("secondowner").create_or_update(&conn).unwrap();
         ::sign_in_as(&mut req, &user);
         ::CrateBuilder::new("owners_selfremove", user.id).expect_build(&conn);
-    }
+        (user, user_two)
+    };
 
     let mut response = ok_resp!(middle.call(&mut req));
     let r: R = ::json(&mut response);
@@ -128,80 +168,75 @@ fn owners_can_remove_self() {
         ok_resp!(middle.call(req.with_method(Method::Put,).with_body(body.as_bytes(),),));
     assert!(::json::<O>(&mut response).ok);
 
-    // Deleting yourself when there are other owners is allowed.
-    let body = r#"{"users":["firstowner"]}"#;
-    let mut response =
-        ok_resp!(middle.call(req.with_method(Method::Delete,).with_body(body.as_bytes(),),));
-    assert!(::json::<O>(&mut response).ok);
+    // Need to accept owner invitation to add secondowner as owner
+    let krate_id = {
+        let conn = app.diesel_database.get().unwrap();
+        Crate::by_name("owners_selfremove")
+            .first::<Crate>(&*conn)
+            .unwrap()
+            .id
+    };
 
-    // After you delete yourself, you no longer have permisions to manage the crate.
-    let body = r#"{"users":["secondowner"]}"#;
-    let mut response =
-        ok_resp!(middle.call(req.with_method(Method::Delete,).with_body(body.as_bytes(),),));
-    let json = ::json::<::Bad>(&mut response);
-    assert!(
-        json.errors[0]
-            .detail
-            .contains("only owners have permission to modify owners",)
+    let body = json!({
+        "crate_owner_invite": {
+            "invited_by_username": "foo",
+            "crate_name": "foo_owner",
+            "crate_id": krate_id,
+            "created_at": "",
+            "accepted": true
+        }
+    });
+
+    ::logout(&mut req);
+    ::sign_in_as(&mut req, &second_owner);
+
+    let mut response = ok_resp!(
+        middle.call(
+            req.with_path(&format!("/api/v1/me/crate_owner_invitations/{}", krate_id))
+                .with_method(Method::Put)
+                .with_body(body.to_string().as_bytes())
+        )
     );
-}
 
-#[test]
-fn owners() {
     #[derive(Deserialize)]
-    struct R {
-        users: Vec<EncodablePublicUser>,
-    }
-    #[derive(Deserialize)]
-    struct O {
-        ok: bool,
+    struct CrateOwnerInvitation {
+        crate_owner_invitation: InvitationResponse,
     }
 
-    let (_b, app, middle) = ::app();
-    let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates/foo_owners/owners");
-    {
-        let conn = app.diesel_database.get().unwrap();
-        ::new_user("foobar").create_or_update(&conn).unwrap();
-        let user = ::new_user("foo").create_or_update(&conn).unwrap();
-        ::sign_in_as(&mut req, &user);
-        ::CrateBuilder::new("foo_owners", user.id).expect_build(&conn);
+    #[derive(Deserialize)]
+    struct InvitationResponse {
+        crate_id: i32,
+        accepted: bool,
     }
 
-    let mut response = ok_resp!(middle.call(&mut req));
-    let r: R = ::json(&mut response);
-    assert_eq!(r.users.len(), 1);
+    let crate_owner_invite = ::json::<CrateOwnerInvitation>(&mut response);
+    assert!(crate_owner_invite.crate_owner_invitation.accepted);
+    assert_eq!(crate_owner_invite.crate_owner_invitation.crate_id, krate_id);
 
-    let mut response = ok_resp!(middle.call(req.with_method(Method::Get)));
-    let r: R = ::json(&mut response);
-    assert_eq!(r.users.len(), 1);
+    ::logout(&mut req);
+    ::sign_in_as(&mut req, &first_owner);
 
-    let body = r#"{"users":["foobar"]}"#;
-    let mut response =
-        ok_resp!(middle.call(req.with_method(Method::Put,).with_body(body.as_bytes(),),));
-    assert!(::json::<O>(&mut response).ok);
-
-    let mut response = ok_resp!(middle.call(req.with_method(Method::Get)));
-    let r: R = ::json(&mut response);
-    assert_eq!(r.users.len(), 2);
-
-    let body = r#"{"users":["foobar"]}"#;
-    let mut response =
-        ok_resp!(middle.call(req.with_method(Method::Delete,).with_body(body.as_bytes(),),));
+    // Deleting yourself when there are other owners is allowed.
+    let body = r#"{"users":["firstowner"]}"#;
+    let mut response = ok_resp!(
+        middle.call(
+            req.with_path("/api/v1/crates/owners_selfremove/owners")
+                .with_method(Method::Delete)
+                .with_body(body.as_bytes())
+        )
+    );
     assert!(::json::<O>(&mut response).ok);
 
-    let mut response = ok_resp!(middle.call(req.with_method(Method::Get)));
-    let r: R = ::json(&mut response);
-    assert_eq!(r.users.len(), 1);
-
-    let body = r#"{"users":["foo"]}"#;
+    // After you delete yourself, you no longer have permisions to manage the crate.
+    let body = r#"{"users":["secondowner"]}"#;
     let mut response =
         ok_resp!(middle.call(req.with_method(Method::Delete,).with_body(body.as_bytes(),),));
-    ::json::<::Bad>(&mut response);
-
-    let body = r#"{"users":["foobar"]}"#;
-    let mut response =
-        ok_resp!(middle.call(req.with_method(Method::Put,).with_body(body.as_bytes(),),));
-    assert!(::json::<O>(&mut response).ok);
+    let json = ::json::<::Bad>(&mut response);
+    assert!(
+        json.errors[0]
+            .detail
+            .contains("only owners have permission to modify owners",)
+    );
 }
 
 /*  Testing the crate ownership between two crates and one team.

Diff for: src/tests/team.rs

@@ -219,7 +219,9 @@ fn remove_team_as_named_owner() {
         )
     );
     assert!(
-        json.errors[0].detail.contains("another user"),
+        json.errors[0]
+            .detail
+            .contains("this crate exists but you don't seem to be an owner.",),
         "{:?}",
         json.errors
     );
@@ -314,7 +316,9 @@ fn publish_not_owned() {
         )
     );
     assert!(
-        json.errors[0].detail.contains("another user"),
+        json.errors[0]
+            .detail
+            .contains("this crate exists but you don't seem to be an owner.",),
         "{:?}",
         json.errors
     );

Diff for: src/tests/user.rs

@@ -544,8 +544,8 @@ fn test_insert_into_email_table() {
         let conn = app.diesel_database.get().unwrap();
         let user = NewUser {
             gh_id: 1,
-            email: Some("potato@example.com"),
-            ..::new_user("potato")
+            email: Some("apple@example.com"),
+            ..::new_user("apple")
         };
 
         let user = user.create_or_update(&conn).unwrap();
@@ -554,8 +554,8 @@ fn test_insert_into_email_table() {
 
     let mut response = ok_resp!(middle.call(req.with_path("/api/v1/me").with_method(Method::Get),));
     let r = ::json::<R>(&mut response);
-    assert_eq!(r.user.email.unwrap(), "potato@example.com");
-    assert_eq!(r.user.login, "potato");
+    assert_eq!(r.user.email.unwrap(), "apple@example.com");
+    assert_eq!(r.user.login, "apple");
 
     ::logout(&mut req);
 
@@ -565,7 +565,7 @@ fn test_insert_into_email_table() {
         let user = NewUser {
             gh_id: 1,
             email: Some("banana@example.com"),
-            ..::new_user("potato")
+            ..::new_user("apple")
         };
 
         let user = user.create_or_update(&conn).unwrap();
@@ -574,8 +574,8 @@ fn test_insert_into_email_table() {
 
     let mut response = ok_resp!(middle.call(req.with_path("/api/v1/me").with_method(Method::Get),));
     let r = ::json::<R>(&mut response);
-    assert_eq!(r.user.email.unwrap(), "potato@example.com");
-    assert_eq!(r.user.login, "potato");
+    assert_eq!(r.user.email.unwrap(), "apple@example.com");
+    assert_eq!(r.user.login, "apple");
 }
 
 /* Given a new user, check that when an email is added,