Fix checks and consistency in the API endpoints to manage ownership. #1772
Conversation
smarnach
force-pushed
the
fix-owner-checks
branch
2 times, most recently
from
ff88830 to
7976674
Compare
|
☔ The latest upstream changes (presumably #1760) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Can we split the transaction wrapping into its own commit? I don't anticipate any problems with this, but in the event we need to roll something back it's helpful to have each problem solved separately |
|
@sgrif If I split this up, the transaction wrapping will necessarily be the first commit, since everything else depends on it. This means we can't roll back the transaction wrapping while keeping any of the other changes, if this is what you have in mind, but we will still be able to roll back some of the other fixes and keep the transaction wrapping. |
|
Yes, having the transaction wrapping first was what I had in mind |
smarnach
force-pushed
the
fix-owner-checks
branch
from
0d0aed7 to
7ebcfe7
Compare
|
@sgrif I've split this up in three commits (to improve diff readability), and made sure the tests pass for each step (to facilitate partial rollbacks). Please take another look. |
There was a problem hiding this comment.
Thanks @smarnach! This PR looks great overall. I have one small suggestion below, although I'm fine merging as-is if you think it makes more sense to have a separate PR to use the json! macro more consistently in tests. (It looks like there are at least 2 more similar uses of format! in tests/user.rs.)
src/tests/util.rs
Outdated
| let body = format!("{{\"users\":[\"{}\"]}}", owner); | ||
| self.delete_with_body(&url, body.as_bytes()) | ||
| let body = format!("{{\"owners\":[\"{}\"]}}", owners.join("\", \"")); | ||
| method(&self, &url, body.as_bytes()) |
There was a problem hiding this comment.
It's probably worth switching to the json! macro here.
let body = json!({ "owners": owners });
method(&self, &url, body.to_string().as_bytes())
|
@jtgeibel Thanks for the review! Using the I noticed another race condition I haven't fixed yet, though. When adding new owners, the code first reads the list of all current owners from the DB, then for each owner checks whether it occurs in the list of current owners, then creates a new owner invitation. If there already was an owner invitation for an owner, and it gets accepted right after the code has checked for the list of current owners, we will create a new invitation for a user that is already an owner, which would normally be prohibited. This race condition could be fixed by using the "serializable" isolation level for the transaction, but then we would also need to add code to retry the transaction if it fails due to a serialization error. I don't think this would be worth it – the above race condition should be exceedingly rare, and even when it gets triggered, the impact is negligible – there will be a spurious invitation, and accepting or rejecting it has no effect, since the invitee already is an owner. |
|
@bors: r+ |
|
📌 Commit 794cc87 has been approved by |
Fix checks and consistency in the API endpoints to manage ownership. Fixes #1583. The API endpoints to add and remove owners for a crate has multiple issues. * The database interactions are not wrapped in a transaction, making them prone to race conditions. * The lack of a transaction can also result in partially applied operations, e.g. when adding or removing multiple owners at once. * The check whether the last owner was removed only triggers if the _original_ list of owners had a length of exactly one. This is wrong in at least two cases: * Teams do not have permission to manage ownership, so they should not be counted. * If multiple users are removed at once, we can end up with no owners even when there originally was more than one user. This change fixes all mentioned problems, and adds tests for all bullet points except for the first one (which can't be tested with reasonable effort).
|
☀️ Test successful - checks-travis |
Fixes #1583.
The API endpoints to add and remove owners for a crate has multiple issues.
The database interactions are not wrapped in a transaction, making them prone to race conditions.
The lack of a transaction can also result in partially applied operations, e.g. when adding or
removing multiple owners at once.
The check whether the last owner was removed only triggers if the original list of owners had a
length of exactly one. This is wrong in at least two cases:
Teams do not have permission to manage ownership, so they should not be counted.
If multiple users are removed at once, we can end up with no owners even when there originally
was more than one user.
This change fixes all mentioned problems, and adds tests for all bullet points except for the first
one (which can't be tested with reasonable effort).