Handle team update scenario where GitHub team has been recreated #11653
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
This seems like a good incremental improvement, thanks! 👍
For a bit more context: the way teams currently work is a bit problematic in various way. One is that they are somewhat tightly coupled to GitHub, which makes it hard(er) for us to support other identity providers. IMHO in the long term we should move team management into crates.io itself to avoid all of these issues, potentially with some functionality to sync GitHub team memberships with crates.io teams.
| let original_team = NewTeam::builder() | ||
| .login("github:test-org:core") | ||
| .org_id(1000) | ||
| .github_id(2001) | ||
| .build() | ||
| .create_or_update(&mut conn) | ||
| .await?; | ||
|
|
||
| let new_team = NewTeam::builder() | ||
| // same team name | ||
| .login(&original_team.login) | ||
| // same org ID | ||
| .org_id(original_team.org_id) | ||
| // different team ID | ||
| .github_id(original_team.github_id + 1) | ||
| .build() | ||
| .create_or_update(&mut conn) | ||
| .await?; |
There was a problem hiding this comment.
I'm a bit worried about these manual database mutations in the test, since they don't quite guarantee that the same logic is applied as it would through the API. I think the issue might be that this is mixing unit testing of the create_or_update() fn with integration testing of the API endpoint behavior.
I would suggest putting the unit test(s) into the database crate via mod tests and then adjusting the test here to only use the API endpoints. It might also make sense to use a custom GitHub API mock for this test so that we can better control the returned IDs.
We should probably also explicitly test the case where the org IDs do not match to avoid accidentally enabling resurrection attacks.
If a team owns a crate, that team is deleted from GitHub, then recreated with the same name, a conflict would occur when adding the team to another crate because a team with the same
loginalready exists but thegithub_idis different so it tries to insert a new team record instead.This PR modifies the
NewTeam::create_or_updatefunction to attempt an update of the team record matching the (org_id,login) pair, allowing existing team records to be updated with new GitHub team IDs.It also adds a test for the above described scenario.
Resolves #6949.