diff --git a/app/templates/application.hbs b/app/templates/application.hbs index 297729e8673..74b4a1a290f 100644 --- a/app/templates/application.hbs +++ b/app/templates/application.hbs @@ -65,6 +65,7 @@ {{#rl-dropdown tagName="ul" class="dropdown current-user-links" closeOnChildClick="a:link"}}
  • {{#link-to 'dashboard'}}Dashboard{{/link-to}}
  • {{#link-to 'me'}}Account Settings{{/link-to}}
  • +
  • {{#link-to 'me.pending-invites'}}Owner Invites{{/link-to}}
  • {{#link-to 'logout'}}Sign Out{{/link-to}}
  • {{/rl-dropdown}} {{/rl-dropdown-container}} diff --git a/src/crate_owner_invitation.rs b/src/crate_owner_invitation.rs index 1f609f6b91b..51b3499353a 100644 --- a/src/crate_owner_invitation.rs +++ b/src/crate_owner_invitation.rs @@ -121,8 +121,10 @@ fn accept_invite( conn: &PgConnection, crate_invite: InvitationResponse, ) -> CargoResult { - 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 --git a/src/krate/mod.rs b/src/krate/mod.rs index 9c844570f43..b933a25ad83 100644 --- a/src/krate/mod.rs +++ b/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 { + 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 { 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 { .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 { } } + 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 --git a/src/tests/all.rs b/src/tests/all.rs index 7522bea4195..5da065f5033 100644 --- a/src/tests/all.rs +++ b/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, meta: CrateMeta, } -#[derive(Deserialize)] +#[derive(Deserialize, Debug)] struct Warnings { invalid_categories: Vec, invalid_badges: Vec, diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 0cdab179180..ad8e901df49 100644 --- a/src/tests/krate.rs +++ b/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>, + } + #[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::(&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 --git a/src/tests/owners.rs b/src/tests/owners.rs index a0f47fcb360..199848eca87 100644 --- a/src/tests/owners.rs +++ b/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::(&mut response).ok); - bad_resp!( + + let krate_id = { + let conn = app.diesel_database.get().unwrap(); + Crate::by_name("foo_owner") + .first::(&*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::(&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::(&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::(&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::(&*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, - } - #[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::(&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::(&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::(&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::(&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 --git a/src/tests/team.rs b/src/tests/team.rs index 27ecf2a1992..e52345f5759 100644 --- a/src/tests/team.rs +++ b/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 --git a/src/tests/user.rs b/src/tests/user.rs index 297e2d598ae..e1d88d9ed7b 100644 --- a/src/tests/user.rs +++ b/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::(&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::(&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,