Skip to content

Groups for ownership *or* GitHub-based permissions #137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
SimonSapin opened this issue Feb 24, 2015 · 30 comments
Closed

Groups for ownership *or* GitHub-based permissions #137

SimonSapin opened this issue Feb 24, 2015 · 30 comments

Comments

@SimonSapin
Copy link
Contributor

We (the Servo team) want to publish more of our libraries on crates.io. One thing this implies is dealing with crates ownership: who can publish new versions. Currently, AFAIU, we have to add or remove permissions for individual people for individual crates. Compare with GitHub where we have "teams" where people participate, and teams has a whole have push access to repositories. This is much easier to manage when the number of people and/or repositories/crates grows.

It would help us if cartes.io had "groups" or "teams" or something like it, to manage crates ownership.

Alternatively, I was going to suggest deferring permission management to GitHub: if a crate name is associated with a repository, anyone with permission to push to that repo can also publish a new version. Unfortunately, that part of the GitHub API seems not to be public:

$ curl -si https://api.github.com/repos/rust-lang/crates.io/collaborators/alexcrichton | grep Status
Status: 401 Unauthorized
@Manishearth
Copy link
Member

Crates is anyway associated with a GH account, perhaps it can ask for those additional permissions?

@alexcrichton
Copy link
Member

Yes I'd definitely love to integrate the teams/organizations aspect of Github a bit more into crates.io for smoother management of these aspects.

@Gankra
Copy link
Contributor

Gankra commented Mar 6, 2015

+1

I'm splitting a monolithic crate into a bunch of little ones and this is a headache.

@Gankra
Copy link
Contributor

Gankra commented Mar 6, 2015

discussed this more on IRC. Current plans is to tie this 100% to github orgs/teams as a starter, since that's what most of us just want anyway.

Proposed syntax:
cargo owner --add github:ORG:TEAM

Backend Implementation:

  • Add a new column to the owners table specifying owner_kind, with two possible choices: user, and github team.
  • Change owner auth from "is in owners list" to "is in owners list, or is in a team in owners list". Critically this "is in team" lookup should be deferred totally to Github, so that we don't need to synchronize who's in what team (or make shadow users or anything).

I'm willing to implement but am honestly a bit swamped with school stuff and am not familiar with any of the related codebases or APIs. So basically if I manage to implement it won't be for a while.

@SadieCat
Copy link

Tight integration with a proprietary code hosting service which may not be around in 10 years seems a bit concerning to me.

@Manishearth
Copy link
Member

Ownership is entirely a crates.io-side thing -- this will only affect those uploading crates (not using them). If github goes down, we'd just need to introduce a new model of shared ownership. No biggie.

@SimonSapin
Copy link
Contributor Author

@SaberUK, this is a fair point, but crates.io already has that integration for logging in. You can not (as far as I understand) upload a crate without a GitHub account. And as Manish says, that can always change later.

@mkroman
Copy link

mkroman commented Apr 11, 2015

@SimonSapin while that is true, it sounds more like that is an unnecessary limitation on crates.io's part. And what about people using Gitorious, Bitbucket or GitLab for managing their code?

@Manishearth
Copy link
Member

That's a separate discussion. Nobody's saying that we can't add more authentication methods or grouping methods in the future.

And IIRC you don't need your code to be on GitHub to upload it to Crates. You only need a GitHub account for authentication; and we're proposing that Crates can import groups from GitHub, but that's it. This is additional functionality, not a limitation.

@mkroman
Copy link

mkroman commented Apr 11, 2015

Aha, I see. To me, it sounded like it'd be a tight integration with GitHub right off the bat. Then indeed my issue is unrelated to this.

@littleyang
Copy link

good job

@larsbergstrom
Copy link

@alexcrichton Is there a timeline for this feature? As we stabilize and move more Servo submodules onto crates.io we run into this more and more.

cc @Ms2ger

@alexcrichton
Copy link
Member

@larsbergstrom unfortunately this currently isn't slated to be implemented any time soon

@jdm
Copy link

jdm commented Jul 14, 2015

FWIW, the github integration is as easy as fetching https://api.github.com/repos/[org]/[repo]/collaborators with the appropriate Authorization header (Basic, with a base64 encoded username/github API token): https://github.com/jdm/highfive/blob/master/newpr.py#L18

@Gankra
Copy link
Contributor

Gankra commented Jul 15, 2015

I have been thinking about this as we get ready to ramp up "official rust-lang crates" as a thing. I think it might make ownership teams part of a crate's public interface. In particular, this could be used to partially resolve the desire for "official" and "namespaced" crates.

For instance, say rand is owned by github:rust-lang:some_team. Then when you search for rand you see the rust-lang organization's logo appear next to it in search results. Instantly this stands out as an official package:

screen shot 2015-07-15 at 3 45 45 pm

We could also potentially support searching for team:rust-lang or something.

@azerupi
Copy link

azerupi commented Jul 16, 2015

@gankro That is a really sweet idea, I like it!

Here is my mock-up for your idea :)

crates-mockup

@Gankra
Copy link
Contributor

Gankra commented Aug 5, 2015

I've picked up the implementation yet again -- sorry the Nomicon and std stuff took priority for me, but I'm taking a bit of a break from that stuff now that it's landed.

Working out the details it's a bit more complicated than my initial description made it seem for implementation and policy reasons. In particular I think a meaningful distinction should be made between named and team owners.

Some thoughts:

  • You should only be allowed to add a team as an owner if you're part of the team -- this is a bit of a DDOS-ish guard against forcing the DB to add every team on Github. Also a safety against adding the wrong team.
  • Team owners shouldn't be allowed to modify ownership. This is to prevent a team owner from making themselves a named owner, then deleting all other owners. Named owners are therefore a "trusted" owner, while a team owner is just a "delegate". A minor concern is entering a state where the only active owners are team owners, but this is no worse than all owners disappearing today. As always crates.io admins can intervene in crisis. This also incidentally guards against a team owner removing their own team (as we do for a named owner today).
  • Github teams seem to have a name and an id. Some APIs expect a name. Some an id. The id can be obtained from the name. It's not clear if the id can be persisted, or if it's unstable. My gut says it must be stable. Anything else would be busted, right?

@Ms2ger
Copy link

Ms2ger commented Aug 5, 2015

My gut says it must be stable. Anything else would be busted, right?

Murphy's law says that it's unstable :)

@alexcrichton
Copy link
Member

I would personally hope that the id of a team is stable throughout possible renamings of the team, that... seems like it's the purpose of the id!

@Gankra
Copy link
Contributor

Gankra commented Aug 5, 2015

Another concern: teams should probably have a normalized name (casing) to avoid bizarre behaviour where Github normalizes and we don't. e.g. rust-lang/Owners and rust-lang/owners are different strings but Github normalizes them just fine.

Unfortunately normalization also seems unspecified (looks like ASCII case insensitivity). I wonder if I should just try to contact Github for details (this is impossible to google, you just get random projects on Github).

@SimonSapin
Copy link
Contributor Author

This already happens with user names: @SimonSapin and @SimonSapin both refer to me, but cargo owner --remove only works with the same casing as in cargo owner --list (I tried). It’s a minor usability issue, but it’s not as much of a problem as if it were the other way around: if I could create a GitHub account with a name that GitHub considers different from yours but crates.io normalizes to the same, I could use it to impersonate you.

This kind of security issue happened to Spotify: https://labs.spotify.com/2013/06/18/creative-usernames/

Therefore I am in favor of not trying to do any normalization on crates.io, even if GitHub does. (All identifiers seem to be ASCII-only on GitHub, so the issue is not as subtle as with Spotify.)

@Gankra Gankra mentioned this issue Aug 6, 2015
@Gankra
Copy link
Contributor

Gankra commented Aug 7, 2015

#177 is my initial implementation of this feature, but there's one major snag (I think). crates.io currently requests no perms from Github (it doesn't actually talk to Github at all except to log you in to the site), but you need the read:org permission to check their team memberships. As such people will have to re-auth on crates.io to enable this feature. We can detect if it's an auth failure and just ignore the team queries though for a smooth back-compat.

@Gankra
Copy link
Contributor

Gankra commented Aug 7, 2015

As Gankro:

test2::cargo owner --add github:contain-rs:owners
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
       Owner adding `[
    "github:contain-rs:owners"
]` to `test2`

As FlashCat:

test2::cargo publish
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
   Uploading test2 v0.3.0 (file:///Users/ABeingessner/dev/test2)
api errors: It looks like you don't have permission to query an organization that owns this 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

does that

test2::cargo publish
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
   Uploading test2 v0.3.0 (file:///Users/ABeingessner/dev/test2)
api errors: crate version `0.3.0` is already uploaded

updates toml

test2::cargo publish
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
   Packaging test2 v0.4.0 (file:///Users/ABeingessner/dev/test2)
   Verifying test2 v0.4.0 (file:///Users/ABeingessner/dev/test2)
   Compiling test2 v0.4.0 (file:///Users/ABeingessner/dev/test2/target/package/test2-0.4.0)
   Uploading test2 v0.4.0 (file:///Users/ABeingessner/dev/test2)
test2::cargo owner --list
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
Gankro (Alexis Beingessner <[email protected]>)
github:contain-rs:owners
test2::cargo owner --add FlashCat
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
       Owner adding `[
    "FlashCat"
]` to `test2`
failed to add owners: api errors: team members don't have permission to modify owners
test2::cargo owner --remove FlashCat
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
       Owner removing `["FlashCat"]` from `test2`
failed to remove owners: api errors: team members don't have permission to modify owners

changes to Gankro

test2::cargo owner --add github:rust-lang:owners
    Updating registry `file:///Users/ABeingessner/dev/crates.io/tmp/index-bare`
       Owner adding `[
    "github:rust-lang:owners"
]` to `test2`
failed to add owners: api errors: only members of a team can add it as an owner

😎

@alexcrichton
Copy link
Member

Add in #177, I'll probably deploy tomorrow

@SimonSapin
Copy link
Contributor Author

Is there some docs on how #177 works?

@alexcrichton
Copy link
Member

I'll try to write something up soon, gotta deploy it first...

@SimonSapin
Copy link
Contributor Author

Alright, thank you and @gankro!

@Gankra
Copy link
Contributor

Gankra commented Aug 18, 2015

@alexcrichton I can write it up if you point me at where you want it.

@SimonSapin
Copy link
Contributor Author

@SimonSapin
Copy link
Contributor Author

And cargo help owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests