We shouldn't let you add someone as an owner without their consent

[Gankra]

This is a vector for abuse. We should require confirmation from the person being added before adding them, just like Github does for adding someone as an owner to a repo/team.

---

Implementation steps added by carols10cents:

The following tasks assume we'll need a web UI for this in order to support users with arbitrarily old versions of cargo. I don't think crates.io can have a state where this feature is "half on", and even if we did, I think we'd have to stay in that state forever because people are always going to be using older versions of cargo. I'm open to hearing ideas on this issue though!

Crates.io Deployable chunk 1: Listing invitations

• (Backend of deployable chunk 1 of owner invitations #1040) Add a new table, crate_owner_invitations or something, that has columns:
  • id of the user who ran cargo owner --add
  • id of the user who is being added
  • id of the crate involved
  • status of the invitiation the only invitations in this table should be pending invitations; accepted invitations can be removed from this table and records added to crate_owners instead; declined invitations can be removed from this table.
  • when this request was created
• Don't add records to this table on a cargo owner --add in this PR, add records to this table either manually with psql or in tests
• (Backend of deployable chunk 1 of owner invitations #1040) Make an API route that can list all of the currently logged in user's pending invitations
• (Frontend page for listing pending owner invites #1057) Make a frontend page that calls that API route and displays the currently logged in user's pending invitations (don't link to this page from anywhere yet though)

Crates.io Deployable chunk 2: Accepting invitations

Still not creating records in crate_owner_invitations when cargo owner --add is run

• Make an API route for the currently logged in user to accept an invitation for a particular crate
  • In a transaction:
    • Add a record to the crate_owners table
    • Remove the record from the crate_owner_invitations table
• On the frontend page that lists pending invitations, have a button that calls the accept API route (and displays success/error nicely, etc)

Crates.io Deployable chunk 3: Declining invitations

Still not creating records in crate_owner_invitations when cargo owner --add is run

• Make an API route for the currently logged in user to decline an invitation for a particular crate
  • Delete the crate_owner_invitations record
• On the frontend page that lists pending invitations, have a button that calls the decline API route (and displays success/error nicely, etc)

Crates.io Deployable chunk 4: Requiring approval for ownership

Assume no changes to cargo, because we can't change old cargo anyway!

• When someone runs cargo owner --add for a user (not a team):
  • Don't create a record in crate_owners, instead create a record in crate_owner_invitations
  • Hopefully send a response back from crates.io that tells the person running the command that the person being added should visit the list invitations page on crates.io to accept ownership (this might not be possible depending on what cargo does today though)
• To make the approval process discoverable as the person being added to the crate from old cargo, when someone runs cargo publish on a crate where there's a pending invitation to add them as an owner:
  • Fail the publish
  • Return an error message from crates.io that directs the user that they have to accept the ownership invitation first by going to the list invitations page, and then they can try publishing again
• Document this requirement in the cargo owner docs (which currently live in cargo's repo) split into Document that adding someone as an owner now invites them, not adds immediately cargo#4599
• Super nice would be some sort of notification that shows up anywhere on crates.io and lets you know you have outstanding ownership invitations and you should go to the invitation list page to approve or decline them (thank you for this idea @natboehm!!!) split into Add some sort of notification that you have invitations to own a crate #1116

Cargo deployable chunk 5: adding CLI support for accept/decline

split into rust-lang/cargo#4600

Crates.io deployable chunk 6: UI for adding someone as an owner

split into #1117

[Gankra]

Workflow sketch:

• old_owner does cargo owner --add new_owner
• backend creates an owner entry but as a new kind of owner: OwnerKind::Pending (this should ideally force us to audit all the places where we evaluate rights, yay enums)
• backend sends a confirmation email to new_owner
• new_owner clicks link to crates.io; maybe clicks a button on a page (while signed in)
• backend changes Pending to User

[Gankra]

Unclear what the process for teams should be. I think you need to be a member of a team to add it already? I don't really remember.

[sunjay]

Running cargo publish should tell the user that they are still pending verification. For teams, would it be feasible to keep people's verification status on a per team basis? i.e. you have publishing rights as long as you're on that team but you have to verify that you want those rights before you can actually publish.

Keeping that kind of information isn't bullet proof though because people can leave or be removed from teams so I don't know how you'd verify them again after that.

A future feature that would be nice would be something that would allow you to block people from requesting to add you. Or maybe that could be done for the time being by just leaving the invite pending?

We could streamline the consent process by asking them if they consent during cargo publish. That would look like this:

1. Owner gives me publishing rights
2. I go to publish the crate
3. Instead of being redirected to check my email or something, I can just answer yes to being an owner and having publishing rights
4. Publishing continues and I am never asked again

Also to consider when adding consent: how do people remove consent later? This needs to be thought about both from the perspective of the old owner and the new owner since either may want to remove publishing rights from new owner.

Sorry for just dropping ideas here but I wanted to make sure these things get considered.

[sunjay]

Also I just checked and my crates user page doesn't actually list crates published by the teams that I'm on, so maybe teams isn't as high of a priority for this? I can't think of a way right now that someone could abuse GitHub organizations to spam, annoy or abuse someone through crates.io.

[ashleygwilliams]

so, one large blocking issue here is that crates.io does not have guaranteed email information for users as GitHub does not reliably send it.

[sunjay]

@ashleygwilliams Hmm. If we ask for the consent at publish time as well as do the email confirmation, we don't necessarily need people's emails. We just won't list them as owners until they've consented.

Could also add a custom cargo command just to accept the invitation to be an owner.

[sunjay]

Also to address the email thing, when I was working with the passport-github package, I found out that they actually do a separate API call to fetch the email after authentication. Maybe that could work as a longer term solution for getting emails? It's not ideal, but it does seem to work.

[ashleygwilliams]

so i think the first step here is to improve the crates.io ACL to include and require an email address. i'm down for any strategy re: that, so @sunjay your suggestion seems good to me. backfill, however, is gonna be a large task- either requiring a lot of user effort or a kinda tricky migration.

the next thing is that, afaik, since we never reliably had email addresses, we don't have any mailer service... at all. so writing one of those and making sure it works for non critical things, is the next step. probably a welcome email or something.

these are two Very Large changes to make. once they are in place and stable, i think figuring out how to properly implement an access invite service would be the next step.

(full disclosure: i care about this issue a lot. but, even npm does not have this yet. i am also not happy about that, but it is a lot of work and it's work that needs to be done right.)

[carols10cents]

Natalie is working on the email ish #808 so emails are coming.

[sunjay]

By mailer service do you mean something that sends emails? What about using something existing like mailgun? The pricing is really great even at scale.

I care about this issue a lot too and would be happy to donate my time in a few weeks to making it happen. (I've never worked on this codebase so I can't do it alone!) I can help with the Rust backend stuff and probably the migration too. I haven't used Ember in about 3 years so I'm probably not going to be able to do any changes we need there.

Maybe we can even figure out a way to fetch emails lazily (and then store them) as we need to.

To prevent being spammy, it might be prudent to also ask people if we can email them at all.

At minimum I can help in the planning and figuring out to work this in and make sure the experience is right. Like you said, it's a lot of work and it's work that needs to be done right.

Edit: just saw @carols10cents comment. 🎉

[Gankra]

I think I kinda just like @sunjay's suggestion of hooking verification into the publish process? Absolutely no notification if you've been added as an owner (so there's no way to even spam someone with ownership requests), and you can claim ownership the moment it's actually relevant (when you need to publish).

We could do the same thing for owner --remove, in the case of an ownership transfer (and then it's kinda like an atomic transfer op, which is neat).

Presumably, if you're adding someone as an owner, you have another communication channel to inform them that you did this.

[sunjay]

If we go with just implementing this through the cargo commands then this mostly becomes a task of adding some things to the API and database, then modifying the appropriate cargo commands. What do you think @ashleygwilliams?

We could have something like cargo owner --accept crate-name or something as well as something to list all the crates someone is an owner of/has pending requests to be an owner of.

[Gankra]

Reflecting further on teams: I don't think we need to worry about them. You need to be on a team to add it (partly this is because Github is incredibly unreliable in acknowledging a team's existence unless you're on it), so basically the abuse vector is someone on a team is trying to harass the entire team?

Only case I can think of is someone rage quitting a community, and in that case that person already has plenty of gas to do worse things, i would think.

[ashleygwilliams]

i am coming around to this CLI solution. it's really elegant actually. i think people will eventually ask for email integration, but that gives us time to build all that shenanigans out. good call @sunjay !

[sunjay]

Thanks! I'll admit I felt apprehensive about sharing the idea in the first place given that I've never even touched the code in this repo. I'm happy to hear you both liking it. Thanks for being open to that. 😊

I can help with the cargo integration stuff in about a week or so. What needs to be done from the crates.io side? I think much of your initial comments still apply just without the email stuff.

[Gankra]

A small but notable issue with CI-based solutions is that you won't be able to transfer ownership to people with old Cargo installs. This mostly just means we should delay flipping the switch in the backend for a bit. I don't think that's a big deal?

[Gankra]

It might be possible to auto-confirm anyone with an old cargo version who tries to perform an owner action, but idk if that's possible or reasonable

[carols10cents]

What needs to be done from the crates.io side?

I need a bit of time to think about this but I will absolutely get back to you within a day or so :)

[sunjay]

@gankro is it a reasonable solution to return an error from crates.io to anyone who is using an old version? The error could contain a message asking them to upgrade their cargo version and a link to instructions on how to do so. The new version would handle this error and instead ask for their consent.

I think it's reasonable to say that all current owners don't need to be confirmed and that this change only applies for newly added owners going further in the future.

Thanks @carols10cents!

[carols10cents]

Ok here's what I'm thinking for the overall flow:

• user 1 owns crate1, wants to add user2 as an owner
• user1 runs cargo owner --add user2, just as today
• user2 can run cargo owner --list-invitations (or something) to list pending ownership additions. This should list the username of the person who added you and the crate name, possibly when they added you
• user2 runs cargo owner --accept crate1 to accept ownership
• user2 runs cargo owner --decline crate1 to decline ownership

Other thoughts/constraints/things I would expect test cases for if applicable:

• I don't think the new owner should have to publish to accept the crate-- they might want to list the crate on their page, etc but not publish a new version yet.
• If you decline ownership for a crate, the user who added you shouldn't be able to re-add you. If you declined ownership by mistake, hmm... we could either list the crates you've declined when you list invitations, and you could change them to accept, or say contact crates.io admins and i'll go muck in the database.
• user1 should be able to rescind a pending ownership offer that hasn't been accepted. I think we could make cargo owner --remove user2 handle this case as well? Or should we have a separate command?
• user1 should not be able to remove themselves as an owner (leaving 0 owners) even if there are pending invitations; they have to be accepted (makes the transfer dance a little more complicated but I think this is a worthwhile tradeoff)
• All current owners should be assumed accepted

Implementation thoughts:

• I think we should store records of pending ownership in a new table, and the current crate_owners table should be for accepted ownership only. This means we don't have to remember to add a where status='accepted' condition to every query to crate_owners because we'll inevitably forget one and chaos will ensue.
• Rolling this out is going to be tricky.
  • I don't think we can force people to upgrade their cargo. This might mean we need a web UI as well.
  • I'll have to look and see in current versions of cargo what output is displayed when you run cargo owner --add; if there's text displayed that was returned by crates.io, we can hook into that and make crates.io return text that says "have user2 go to https://crates.io/pending_ownership to accept ownership". If all of the text output is hardcoded in cargo though, there's not much we can do for people adding owners from old cargo.
  • For users trying to accept ownership from an old cargo, they can either go to the web UI (which may or may not be discoverable), or we should be able to hook into cargo publish as a fallback: we can check to see if there are outstanding invitations at that point, and cargo publish definitely displays error messages that come from crates.io, so if you're in this case, we can fail the publish with a message that says "you have not yet accepted ownership, go to https://crates.io/pending_ownership to accept and then publish again"

I'm going to submit this comment because it's getting long, then I'm going to edit the issue description with a list of tasks that I think need to be done and how we might break this into smaller deployable pieces.

[carols10cents]

Ok, I've added a list of things to the issue description that I think need to be done in the form of a plan that splits these steps into smaller, independently mergeable and deployable chunks. Am I forgetting anything that anyone can think of? @sunjay do these steps all make sense? Please ask questions if anything is unclear, either now or during implementation!

To address some other things people brought up in the comments:

backend creates an owner entry but as a new kind of owner: OwnerKind::Pending (this should ideally force us to audit all the places where we evaluate rights, yay enums)

I'm not a fan of this because currently this enum consists of OwnerKind::User and OwerKind::Team. Pending/declined ownership feels orthogonal to me, and as was discussed, doesn't really apply to teams at all.

A future feature that would be nice would be something that would allow you to block people from requesting to add you.

Agreed! I think blocking could be done as a separate issue from this; I've filed #928 for this and I can flesh that out a bit more as we figure out how this issue is going to go.

Also to consider when adding consent: how do people remove consent later? This needs to be thought about both from the perspective of the old owner and the new owner since either may want to remove publishing rights from new owner.

It's possible to remove owners today, I think that feature covers this aside from making removal also remove pending/declined invitations.

Regarding teams:

• Gankro is correct that you currently need to be on a team in order to add the team as an owner
• Team membership is checked with github on crate publish, so if a user leaves a team in github, they lose their publish rights as well
• Since you've consented in github to be on a team, I don't think we need to get consent for team ownership in crates.io too

Also, I'm going to have Natalie update #808 with our plans on emails and address questions about that; please check over there later if you have email thoughts :) Once we have email support in general, we can decide if we want to send email notifications that someone has invited you to own a crate or not, but I think for now the CLI/UI solution I've outlined in the description should be enough, and it sounds like that was the conclusion everyone was coming to as well.

[Gankra]

Sounds good, some quick notes:

list the crates you've declined when you list invitations, and you could change them to accept

I would advise against this, as it strikes me as a recipe for a very bad surprise for whoever owns a crate years later.

I think we should store records of pending ownership in a new table, and the current crate_owners table should be for accepted ownership only. This means we don't have to remember to add a where status='accepted' condition to every query to crate_owners because we'll inevitably forget one and chaos will ensue.

I'm 95% sure making Pending an OwnerKind avoids this issue, because:

• we have to check that field everywhere to know how to resolve the ownership
• we only ever check for strict equality (no != or _ =>)

That said I'm sympathetic to this as a broader longterm maintenance concern. It's a reasonable hardening strategy. I also agree it's a bit sketchy in terms of overloading. (PendingUser?)

This might mean we need a web UI as well.

An incredibly bare bones solution might be any GET to crates.io/crates/foo/accept_author does an acceptance with the currently logged in user? This is intended to be a hack, so I don't think it needs to be that good.

[sunjay]

The plan looks great and I have lots of comments and some ideas that I think we should consider. I'll need a few days to get back to you as this week is particularly hectic for me. Expect my reply on Friday or Saturday. Thanks for your patience and thanks for continuing to include me in the discussions. This is fun. :)

[sunjay]

Thanks for giving me a few days! Sorry it took me longer than expected for me to get back to you all. @carols10cents has done a wonderful job designing everything so if it's alright I'll just address a few specific points I had some comments about. :)

I don't think the new owner should have to publish to accept the crate-- they might want to list the crate on their page, etc but not publish a new version yet.

This was more of a convenience feature. The idea was that instead of having to execute a separate command at publish, we would give them the option to accept the invitation right when they need it. This would be something in addition to the --accept/--decline part of the flow. Apon further reflection, I'm not sure if it's entirely necessary.

What will be necessary is to have a useful error message when someone tries to publish without having accepted the invitation first. The tricky part as you've mentioned is to make the error backwards-compatible so that people with older cargo versions can still know to upgrade or whatever.

If you decline ownership for a crate, the user who added you shouldn't be able to re-add you. If you declined ownership by mistake, hmm... we could either list the crates you've declined when you list invitations, and you could change them to accept, or say contact crates.io admins and i'll go muck in the database.

Regarding this, I think it's alright for now to just make it so you can send someone an invitation again after it is declined. We're not sending emails right now so there isn't actually much abuse that can happen. I think a cargo owner --block command would be a more robust solution.

Having said that, I think it would be a good idea to brainstorm a list of ways people can abuse using cargo and then systematically address those things. I already have somethings that I have thought about which I would be happy to share.

user1 should be able to rescind a pending ownership offer that hasn't been accepted. I think we could make cargo owner --remove user2 handle this case as well? Or should we have a separate command?

I think --remove is natural given that they have to use --add to add them. The documentation for this will need to be very clear about what to do in various scenarios. (I am happy to help with that.)

user1 should not be able to remove themselves as an owner (leaving 0 owners) even if there are pending invitations; they have to be accepted (makes the transfer dance a little more complicated but I think this is a worthwhile tradeoff)

This makes sense. To transfer ownership from user1 to user2:

1. user1 adds user2 to the crate
2. user2 accepts the invitation
3. user1 removes themselves from the crate

This seems like a fairly natural flow for this to take.

I don't think we can force people to upgrade their cargo. This might mean we need a web UI as well.

Could we have the URL only option that @gankro suggested? We would only need people to upgrade their cargo if they want to accept/decline/block invitations. Everything else would remain the same. Is it still not sufficient to just produce a message saying that they need to have cargo version x.x or higher? We could make this very clear in the documentation too. I'm asking because I think strict backwards compatibility while desirable is really hard to maintain and the cases where people would need to be able to do this are few.

If we were to stick to command line only for now, we would still add the web UI in the future. It just wouldn't be mandatory for the release of invitations. (I think prioritizing the bare minimum implementation is really important.)

I'll have to look and see in current versions of cargo what output is displayed when you run cargo owner --add

This is definitely important to know. :)

For users trying to accept ownership from an old cargo, they can either go to the web UI (which may or may not be discoverable), or we should be able to hook into cargo publish as a fallback: we can check to see if there are outstanding invitations at that point, and cargo publish definitely displays error messages that come from crates.io, so if you're in this case, we can fail the publish with a message that says "you have not yet accepted ownership, go to https://crates.io/pending_ownership to accept and then publish again"

If we do add something to publish, I don't think it should be automatic. I think we should ask the user to say yes or no to whether they want to accept the invitation. Or at least hit enter to confirm. I think it's important that they know that they're accepting an invitation instead of it happening automatically without their knowledge.

Am I forgetting anything that anyone can think of? @sunjay do these steps all make sense? Please ask questions if anything is unclear, either now or during implementation!

I think the basic flow is really clear. Here's how I see it:

• user1 owns crate1, wants to add user2 as an owner
• user1 runs cargo owner --add user2, just as today
• user1 receives a message saying that user2 has been invited to become an owner and that they must accept the invitation before they are allowed to publish or be listed on crates.io
  • the command that user2 needs to run is listed as part of the message so user1 can communicate what to do to user2
• user1 and user2 can run cargo owner --list-invitations (or something) to list pending invitations
  • This should list the username of the person who added you, the crate name, and when they added you
• user2 runs cargo owner --accept crate1 to accept ownership
• user2 runs cargo owner --decline crate1 to decline ownership
• user2 runs cargo owner --block crate1 to block any further invitations from that crate (blocking to be added later)
• user2 runs cargo owner --unblock crate1 to unblock invitations from that crate (blocking to be added later)

There's some more I want to say but this comment is long and I've rewritten it 4 times over the past few days so I'm going to submit it. Please don't hesitate to give me any feedback about any of this! :)

[sunjay]

One thing I would really like as part of all of this abuse prevention work (but not this issue specifically) is to setup some integration tests that actually simulate abuse on a test user. I am happy to help out with this since I have some ideas already.

Some important points about this:

• The tests should cover different ways to abuse a user or a crate and should demonstrate that the abuse is either thwarted or reduced by the crates.io infrastructure
• The tests should work against a test user and not actually abuse a user
• The tests should run on the live site so we know this stuff actually works
• The tests should only run a maximum of once every 2 to 4 weeks so we don't attack our own servers

I'm not talking about load testing or anything else related to that. I would specifically like to help setup scenarios that demonstrate that abusing a user isn't possible.

Even though we definitely can't cover every case with this, it would be nice to have the infrastructure setup so that if in the future someone finds a way to abuse the system, we can write a regression test which demonstrates that the issue has been fixed and remains fixed.

I have to catch a flight right now but I'll post some more ideas and the things I have in mind later. I can move this to another issue too since this is kind of out of scope for what we're focusing on.

[carols10cents]

Thank you for your comments @sunjay!

This was more of a convenience feature. The idea was that instead of having to execute a separate command at publish, we would give them the option to accept the invitation right when they need it. This would be something in addition to the --accept/--decline part of the flow. Apon further reflection, I'm not sure if it's entirely necessary.

Yes, I don't think integrating acceptance with publish is necessary for an MVP of this feature.

We would only need people to upgrade their cargo if they want to accept/decline/block invitations. Everything else would remain the same. Is it still not sufficient to just produce a message saying that they need to have cargo version x.x or higher? We could make this very clear in the documentation too. I'm asking because I think strict backwards compatibility while desirable is really hard to maintain and the cases where people would need to be able to do this are few.

Enterprises and linux distributions are not able to just upgrade whenever. So no, we can't just make people upgrade in order to be able to accept/decline publishing invitations, unfortunately.

If we were to stick to command line only for now, we would still add the web UI in the future. It just wouldn't be mandatory for the release of invitations. (I think prioritizing the bare minimum implementation is really important.)

Since we can't make people upgrade, the bare minimum implementation can be the web UI, and we can add the command line UI in the future.

One thing I would really like as part of all of this abuse prevention work (but not this issue specifically) is to setup some integration tests that actually simulate abuse on a test user. I am happy to help out with this since I have some ideas already.

Please open a new issue to discuss the more general things you have in mind-- I'd be interested to hear what you're thinking about that couldn't be implemented with the tests we have today that run in CI and why they'd have to run against the live site.

[carols10cents]

Invitations have been turned on!!! Thank you @sunjay and @natboehm!!

I think I'm going to split the remaining tasks into separate issues, since they're mostly enhancements, the main request is "done" at this point.

[vi]

Maybe trying to publish new version of a crate when you are invited should count as implied acceptance?

This way user can just not bother with accepting or declining anything.