Implement admin account syncing #7858
Conversation
Turbo87
added
C-enhancement ✨
Turbo87
force-pushed
the
sync-admins
branch
6 times, most recently
from
5ea074d
to
6e5223d
Compare
There was a problem hiding this comment.
👍
I had a pretty good look at the syncing, and it looks good to me.
The one other question I have is: how do we get notified if the sync job fails? I'm conscious of JD's recent issue where it turned out some team sync actions hadn't been happening for a while — how noisy should we make it when the job fails, and similarly, how noisy should the job be when it actually changes something? Team membership changes don't happen very often, so I wonder if we should get notifications every time the job actually updates anything.
Cargo.toml
Outdated
| @@ -49,6 +49,7 @@ slow-tests = [] | |||
| [dependencies] | |||
| anyhow = "=1.0.79" | |||
| async-trait = "=0.1.77" | |||
| mockall = "=0.12.1" | |||
There was a problem hiding this comment.
I know the dependency table isn't completely ordered right now, but this should probably be somewhere with the other ms.
There was a problem hiding this comment.
I honestly wonder why I put it there now... this was supposed to be sorted 😅
Turbo87
force-pushed
the
sync-admins
branch
2 times, most recently
from
72c189b
to
58affde
Compare
|
As discussed in the team meeting on friday, I've added background job deduplication. If a sync-admin job already exists in the queue then I've also added notification emails for all of the existing admins when new admins are added or admin access is revoked. Note that admins whose access is revoked will also get these emails, to avoid us unknowingly getting our access revoked by an attacker. @LawnGnome @walterhpearce could you give this another round of review to see if the concerns are sufficiently addressed? 🙏 |
|
☔ The latest upstream changes (presumably 38d7a17) made this pull request unmergeable. Please resolve the merge conflicts. |
https://team-api.infra.rust-lang.org is hosted on GitHub Pages, which uses Let's Encrypt as CA. The certificates for https://team-api.infra.rust-lang.org itself are short-lived and somewhat out of our control, but the intermediate certificate from Let's Encrypt can at least be pinned to reduce the risk of man in the middle attacks.
This PR is based on #7852 and adds a background job that automatically syncs the
users.is_admincolumn in the database with the team members of thecrates-io-adminsteam on https://github.com/rust-lang/team/ (via https://team-api.infra.rust-lang.org/v1/teams/crates-io-admins.json).