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!
cargo owner --addcargo owner --add in this PR, add records to this table either manually with psql or in testsStill not creating records in crate_owner_invitations when cargo owner --add is run
Still not creating records in crate_owner_invitations when cargo owner --add is run
Assume no changes to cargo, because we can't change old cargo anyway!
cargo owner --add for a user (not a team):cargo publish on a crate where there's a pending invitation to add them as an owner:split into https://github.com/rust-lang/cargo/issues/4600
split into https://github.com/rust-lang/crates.io/issues/1117
Workflow sketch:
cargo owner --add new_ownerOwnerKind::Pending (this should ideally force us to audit all the places where we evaluate rights, yay enums)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.
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:
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.
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.
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.
@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.
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.
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.)
Natalie is working on the email ish https://github.com/rust-lang/crates.io/issues/808 so emails are coming.
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. 馃帀
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.
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.
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.
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 !
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.
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?
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
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 :)
@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!
Ok here's what I'm thinking for the overall flow:
cargo owner --add user2, just as todaycargo 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 youcargo owner --accept crate1 to accept ownershipcargo owner --decline crate1 to decline ownershipOther thoughts/constraints/things I would expect test cases for if applicable:
cargo owner --remove user2 handle this case as well? Or should we have a separate command?Implementation thoughts:
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.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. 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.
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:
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.
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:
!= 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.
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. :)
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:
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:
cargo owner --add user2, just as todaycargo owner --list-invitations (or something) to list pending invitationscargo owner --accept crate1 to accept ownershipcargo owner --decline crate1 to decline ownershipcargo owner --block crate1 to block any further invitations from that crate (blocking to be added later)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! :)
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:
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.
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.
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)
@carols10cents How many versions back of cargo do we need to support? It may be worthwhile to look at the history of what cargo has done in the --add command to see about this particular point in the implementation.
@carols10cents How many versions back of cargo do we need to support? It may be worthwhile to look at the history of what cargo has done in the --add command to see about this particular point in the implementation.
As many as we can back to what was shipped with Rust 1.0, imo.
@carols10cents I've started work on the first deployable chunk in #959. I have some implementation questions which I wasn't sure if I should ask in the PR or here. I've asked below and we can continue on the PR if that's more appropriate.
status of the invitiation: either "pending" or "declined" (accepted invitations can be removed from this table and records added to crate_owners instead)
Regarding status, I think we should also consider a status for when the invitor revokes/removes their invitation using cargo owner --remove. (more on that in a bit)
Make an API route that can list all of the currently logged in user's pending invitations (and perhaps the declined ones too, if we want to support "undecline"?)
I don't think you should be able to undecline. If you accidentally decline, the other person should have to add you again. This is how people understand invitations to work on other platforms. Doing it this way ensures that someone can't accept an invitation that the invitor doesn't expect them to have anymore (i.e. when the wrong user gets invited accidentally and you remove them once you notice).
I think we should talk about the reasons we're storing invitations in the first place. I fully agree that accepted invitations should just be deleted, so we're good on that. Pending invitations obviously need to be stored. Here's what we should discuss: My understanding is that declined or removed invitations should be stored because then we can eventually add some automated spam detection to the system (if we need to). Storing that information makes it possible for admins to look at the database and verify if someone is being abused.
In service of all of that, I think we should have three statuses:
For the purpose of validation, declined and removed and synonymous, but having both gives us some extra information to use for abuse moderation and detection.
What do you all think? :smile:
In service of all of that, I think we should have three statuses:
pending declined removedFor the purpose of validation, declined and removed and synonymous, but having both gives us some extra information to use for abuse moderation and detection.
I don't understand why removed doesn't delete the invitation?
I don't understand why removed doesn't delete the invitation?
My thought process was that if we keep the removed records, we can use them as a record of what's happened with the invitations.
After further thought, I think I'll leave it as is for now with just pending and declined. If that additional information comes in handy later we can always add it with no issues. No need for the increased complexity now.
I think that sounds like a good plan :)
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.
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.
Most helpful comment
Natalie is working on the email ish https://github.com/rust-lang/crates.io/issues/808 so emails are coming.