WIP: Teams #177

Gankro · Aug 6, 2015

This is completely untested

I just got it to compile, and wanted to get this up for the bikesheddy part of review while I start setting up testing and stuff.

Some of the JSON schemas still probably need to be updated to yield duplicate fields for bridging legacy and clarity. I don't think Cargo needs to be modified at all (teams are Just Another Name as far as it should be concerned), but I might be forgetting something.

Gankro · Aug 6, 2015

Oh, closes #137

Gankro · Aug 6, 2015

r? @alexcrichton

Gankro · Aug 6, 2015

All old tests pass locally now. Moving on to making new tests.

Gankro · Aug 6, 2015

Gankro reviewed Aug 6, 2015

View changes

Gankro · Aug 6, 2015

src/krate.rs

@@ -174,8 +175,9 @@ impl Crate {
        })));

        try!(conn.execute("INSERT INTO crate_owners
-                           (crate_id, user_id, created_at, updated_at, deleted)
-                           VALUES ($1, $2, $3, $3, FALSE)",
+                           (crate_id, owner_id, created_by, created_at,


Note: I added the created_by field here while debugging a test failure cause by something else. Not sure why it wasn't being set and whether it should be set.

Gankro · Aug 6, 2015

Gankro reviewed Aug 6, 2015

View changes

Gankro · Aug 6, 2015

src/krate.rs


    #[derive(RustcEncodable)]
-    struct R { users: Vec<EncodableUser> }
+    struct R { users: Vec<EncodableOwner> }


Note: this is the one place that I'm not sure how to handle API-wise -- this is something that cargo sends us. I suppose we could have two duplicate fields, both of which are Options? Try to decode both, pick whichever has stuff first?

Yeah for now let's have two duplicate fields, both options. We can slowly phase out the older field later on down the road (could add a comment explaining what's up)

Gankro · Aug 7, 2015

Status Update: I have tested this out locally by hand and everything seems to be working!!!

I just need to write some unit tests. (oh nooo mocking)

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/krate.rs

-                           VALUES ($1, $2, $3, $3, FALSE)",
+                           (crate_id, owner_id, created_by, created_at,
+                             updated_at, deleted, owner_kind)
+                           VALUES ($1, $2, $2, $3, $3, FALSE, 0)",


Could you refactor this literal 0 into something like OwnerKind::User as i32?

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/krate.rs

-    pub fn owners(&self, conn: &Connection) -> CargoResult<Vec<User>> {
+    pub fn owners(&self, conn: &Connection) -> CargoResult<Vec<Owner>> {
+        // Users and Teams are differentiated by the owner_kind field:
+        // User = 0, Team = 1


Could this be indicated through an enum instead?

You can probably delete this comment now

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+/// Unifies the notion of a User or a Team.
+pub enum Owner {
+    User(User),
+    Team(Team)


trailing comma

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+    /// ask github for it. *shrug*
+    github_id: i32,
+    /// Unique table id
+    cargo_id: i32,


Currently the model-like structs all have public fields, and the crates.io ID is just called id

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+    pub id: i32,
+    // Login is deprecated in favour of name, but needs to be printed for back-compat
+    pub login: String,
+    pub kind: String,


This'll need to be Option<String> to work with older Cargos

I think this is only ever encoded today. Cargo only ever sends us a list of names.

It is? It looks like cargo currently requires a login field

Yes, cargo will decode this -- but we won't. So we say we have to provide the login, and downstream Cargo can say it's optional if it wants. But we have to for old Cargos.

Wait I'm confused, are we talking about login or kind?

Oh sorry I meant the kind field. Cargo doesn't currently give us a kind field, right? That means this'll have to be an Option if we want to continue to receive responses from Cargo?

Yeah so due to the github: prefix owners are uniquely identified by their string name -- which is the only way Cargo talks to us about them. They just send a list of names (which isn't parsed with this struct at all).

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/bin/migrate.rs

+        ),
+        Migration::add_table(20150804170128, "teams", "
+            id            SERIAL PRIMARY KEY,
+            name          VARCHAR NOT NULL UNIQUE,


Can you make sure there's an index on this field? You're doing a query on it sometimes so it'll need to be indexed, although I forget if the UNIQUE constraint adds an index or not...

Per http://dba.stackexchange.com/questions/144/when-should-i-use-a-unique-constraint-instead-of-a-unique-index it appears that a unique constraint implies the creation of an index. The top answer argues you should favour an index if you actually intend to index off that value as a matter of clarity (the index won't go away if the uniqueness constraint goes away), but that doesn't seem like a concern here.

Ok, seems fine to me!

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+        match chunks.next().unwrap() {
+            // github:rust-lang:owners
+            "github" => {
+                // Ok to unwrap since we know one ":" is contained


Odd comment? (no unwraps here)

This is actually going to revert back -- the try! on org is otherwise dead code.

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+                'a'...'z' | 'A'...'Z' | '0'...'9' | '-' | '_' => false,
+                _ => true
+            }
+        }


Could this reuse Crate::valid_name?

That doesn't seem very semantically clear. We're trying to filter out to only the valid set of GH team slugs. (actually I think A...Z could be omitted)

Could we just leave this up to github?

Definitely want to sanitize to avoid /'s and what not -- easiest way I could think of was to just whitelist the stuff that's in slugs.

Hm ok, I'll probably do one final auditing pass but this is fine for now

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+                         .header("Accept", "application/vnd.github.v3+json")
+                         .header("User-Agent", "hello!")
+                         .header("Authorization", &format!("token {}", &req_user.gh_access_token))
+                         .exec());


Could you refactor this between src/user/mod.rs to avoid duplication?

Unfortunately this may also not be quite right as it looks like the teams listing for an organization has pagination, so we may have to make multiple requests to look up the teams in an organization.

What's the duplication?

Setting up the http::handle, and various other headers are duplicated, and it'd be useful to just do something like passing in the URL and then getting the response.

So one thing is that Teams and Users auth in different ways. Users are using the oauth2 lib to inject the header, but Teams can't do that (as far as I can tell) because there isn't a way to create an oauth2 object from the string key we store. As such teams just write the appropriate header manually (which is trivial and will never change).

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+                           VALUES ($1, $2)",
+                          &[&name, &github_id]));
+
+        // read it right back out:


If you search for "RETURNING" you can see a method of inserting while also returning the row that was just inserted

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+                                  crate. You may need to re-authenticate on \
+                                  crates.io to grant permission to read \
+                                  github org memberships. Just go to \
+                                  https://crates.io/login"));


It'd be good to refactor this 403-checking logic with the above block to reduce duplication

They have different messages, fwiw.

True, but it's probably not that important in this case

I think it's kinda important from a UX perspective -- people are going to try this feature and get walled on out-of-date authentication. There's two places where this can happen, and they're sufficiently different that they warrant different messages IMO.

Hm ok, can there perhaps be a switch of some form? It'd just be nice to consolidate all this HTTP logic as much as possible.

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+        }));
+
+        // mock Team (only need ID to check team status)
+        let team = Team { github_id: github_id, cargo_id: 0, name: String::new() };


Instead of making a dummy team could this just be a static method which takes only a github id?

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+    /// to this method, we may still succeed if Github returns us the same ID
+    /// as the One True Name. However in this case, the One True Name will
+    /// still be selected.
+    pub fn find_by_name_for_add(conn: &Connection, name: &str, req_user: &User)


This is only called in one location, so perhaps the logic for checking whether the user is a member of a team can be inlined at that location? Either that or otherwise add a method to Owner querying whether a user is a member of that owner.

What's the purpose of the ownership query? (Old drafts had such a method, but it ended up being useless).

Just in the sense that the caller of this method would do something like:

let owner = try!(Owner::find_by_name(name)); if !owner.contains(&user) { return Err(...) }

Is this not the moral equivalent of User::find_or_insert (but generalized)?

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+                    email: None,
+                    avatar: None,
+                    name: None,
+                    kind: String::from("owner"),


s/owner/team/ ?

alexcrichton · Aug 11, 2015

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton · Aug 11, 2015

src/owner.rs

+                    login: name,
+                    email: None,
+                    avatar: None,
+                    name: None,


Some(name) ?

ok I have this back in cache now. name is the pretty-printed-name. e.g. "Alexis Beingessner". So it doesn't super make sense to produce that here? (it will make cargo produce a silly output when you list owners).

We do want a duplicate of login, it's unfortunate that name is taken!

Ah ok, this is fine in that case.

alexcrichton · Aug 14, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton · Aug 14, 2015

src/http.rs

+
+pub fn github(app: &App, url: &str, auth: &Token)
+    -> Result<curl::http::Response, curl::ErrCode> {
+    println!("HTTP: {}", url);


s/println/info/

alexcrichton · Aug 14, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton · Aug 14, 2015

src/http.rs

+     .exec()
+}
+
+pub fn token(token: String) -> Token {


Could you add a comment here indicating that this is intended to pass to the function above? (e.g. the scopes/token_type are blank)

alexcrichton · Aug 14, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton · Aug 14, 2015

src/krate.rs

            human(format!("could not find user with login `{}`", name))
        }));
        try!(conn.execute("UPDATE crate_owners
                              SET deleted = TRUE, updated_at = $1
-                            WHERE crate_id = $2 AND user_id = $3",
-                          &[&::now(), &self.id, &user.id]));
+                            WHERE crate_id = $2 AND owner_id = $3 AND owner_kind = $4",


You shouldn't need the owner_kind field here, owner_id should be unique and should directly imply what owner_kind should be

(discussed on irc -- this is false)

alexcrichton · Aug 14, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton · Aug 14, 2015

src/krate.rs

@@ -476,13 +505,15 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
            (format!("SELECT crates.* FROM crates
                       INNER JOIN crate_owners
                          ON crate_owners.crate_id = crates.id
-                       WHERE crate_owners.user_id = $1 {} \
+                       WHERE crate_owners.owner_id = $1
+                       AND crate_owners.owner_kind = 0 {}


this 0 should be replaced with a symbolic constant

(same below)

alexcrichton · Aug 14, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton · Aug 14, 2015

src/owner.rs

+
+    let resp = try!(http::github(app,
+        &format!("http://api.github.com/teams/{}/memberships/{}", &github_id, &user.gh_login),
+        &http::token(user.gh_access_token.clone())));


Same comment about style here as above

alexcrichton · Aug 14, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton · Aug 14, 2015

src/owner.rs

+    }
+}
+
+fn team_with_gh_id_contains_user(app: &App, github_id: i32, user: &User) -> CargoResult<bool> {


Can you be sure this wraps to 80 chars?

alexcrichton · Aug 14, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton · Aug 14, 2015

src/tests/team.rs

+    body.extend(json.as_bytes().iter().cloned());
+    body.extend([0, 0, 0, 0].iter().cloned());
+    body
+}


These look duplicated with krate.rs, could they be consolidated?

alexcrichton · Aug 15, 2015

alexcrichton reviewed Aug 15, 2015

View changes

alexcrichton · Aug 15, 2015

src/tests/team.rs

+    assert!(json.errors[0].detail.contains("don't have permission"),
+            "{:?}", json.errors);
+}
+


Can you also add tests for:

Owners can remove a team from a crate

Team members cannot remove a team from a crate

alexcrichton · Aug 15, 2015

alexcrichton reviewed Aug 15, 2015

View changes

alexcrichton · Aug 15, 2015

src/version.rs

@@ -343,7 +344,7 @@ fn modify_yank(req: &mut Request, yanked: bool) -> CargoResult<Response> {
    let user = try!(req.user());
    let tx = try!(req.tx());
    let owners = try!(krate.owners(tx));
-    if !owners.iter().any(|u| u.id == user.id) {
+    if try!(rights(req.app(), &owners, &user)) < Rights::Publish {


This means that team members can't yank? I'd be fine allowing this

Team members have Rights::Publish, so they should pass this check.

alexcrichton · Aug 15, 2015

alexcrichton reviewed Aug 15, 2015

View changes

alexcrichton · Aug 15, 2015

src/owner.rs

+                    id: id,
+                    login: name,
+                    email: None,
+                    avatar: None,


Can you make sure the frontend handles this nicely? I'm curious what the page looks like when it lists owners and one of the owners is a team.

You get an output that looks like:

steveklabnik (Steve Klabnik <steve@steveklabnik.com>) github:rust-lang:owners

Ohhh you mean the actual crates.io website

The frontend really drops the ball on this, I'm not gonna lie. Broken image and a link to github.com/github:rust-lang:owners

alexcrichton · Aug 15, 2015

alexcrichton reviewed Aug 15, 2015

View changes

alexcrichton · Aug 15, 2015

src/http.rs

+use oauth2::*;
+use app::App;
+
+pub fn github(app: &App, url: &str, auth: &Token)


This function might be a lot nicer to call if it was something along the lines of:

pub fn github<T: Decodable>(app: &App, url: &str, auth: &Token) -> Result<T, curl::ErrCode> { // ... }

Basically something that does the decoding for you automatically and fills in an error for bad JSON and bad utf-8-ness

The response-code handling isn't uniform, though. In particular when querying team membership, 404 is how Github reports "false". Do you want this to take a closure for (maybe) processing the error code?

I made two fns, one for querying, one for parsing, so that you can intermediate evaluation of the request as desired.

alexcrichton · Aug 17, 2015

OK, I just hooked up coveralls to this repo so in theory when you rebase you'll start seeing coverage information published, and it's a good check to see if at least most of the branches and such were taken during the tests perhaps?

Gankro · Aug 18, 2015

I didn't mean to revert the per_page thing -- you may want to fix this

alexcrichton · Aug 18, 2015

Awesome, thanks so much @Gankro! I've added some extra things here and there and pushed to master.

Gankro force-pushed the Gankro:teams branch 2 times, most recently from 739d162 to 6ee2bef Aug 6, 2015

Gankro referenced this pull request Aug 7, 2015
Closed
Groups for ownership *or* GitHub-based permissions #137

alexcrichton referenced this pull request Aug 11, 2015
Merged
Second try making the graph legend fit without wrap #183

Gankro force-pushed the Gankro:teams branch from 57b53e7 to 2fc417f Aug 11, 2015

Gankro force-pushed the Gankro:teams branch from da16e83 to cfb23d5 Aug 18, 2015

more tests and cleanup

Loading status checks…

e5ace68

alexcrichton merged commit e5ace68 into rust-lang:master Aug 18, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

coverage/coveralls Coverage increased (+0.4%) to 86.265%
Details

steveklabnik referenced this pull request Sep 2, 2015
Closed
GitHub authorization requested permissions #191

rust-lang/crates.io

Join GitHub today

WIP: Teams #177

Conversation

Gankro commented Aug 6, 2015

This comment has been minimized.

Gankro commented Aug 6, 2015

This comment has been minimized.

Gankro commented Aug 6, 2015

Gankro force-pushed the Gankro:teams branch 2 times, most recently from 739d162 to 6ee2bef Aug 6, 2015

This comment has been minimized.

Gankro commented Aug 6, 2015

Gankro reviewed Aug 6, 2015 View changes

This comment has been minimized.

Gankro reviewed Aug 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

Gankro referenced this pull request Aug 7, 2015

This comment has been minimized.

Gankro commented Aug 7, 2015

alexcrichton referenced this pull request Aug 11, 2015

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 11, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Gankro force-pushed the Gankro:teams branch from 57b53e7 to 2fc417f Aug 11, 2015

Gankro force-pushed the Gankro:teams branch 2 times, most recently from `739d162` to `6ee2bef` Aug 6, 2015

Gankro reviewed Aug 6, 2015

View changes

Gankro reviewed Aug 6, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

alexcrichton reviewed Aug 11, 2015

View changes

Gankro force-pushed the Gankro:teams branch from `57b53e7` to `2fc417f` Aug 11, 2015

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton reviewed Aug 14, 2015

View changes

alexcrichton reviewed Aug 14, 2015

View changes