Allow team leaders to approve changes to their `team.toml`

[marcoieni]

Should we do this?

@rust-lang/team-repo-admins and @rust-lang/leadership-council, do we want to give team leaders (proper team here) the ability to edit their team.toml files? E.g. Kobzol and apraino are leads for the community-survey team. Do we want them to be able to approve and merge changes to the community-survey file?

• Pro: teams are more autonomous and they don't need to wait for a member of @rust-lang/team-repo-admins or @rust-lang/mods to make changes.
• Con: security concern — if a team leader GitHub account is compromised, the attacker can change the team file however they want, including kicking everyone out of the team.

We discussed this topic in the latest infra team meeting and the general consensus is that it is fine to accept the security risk, especially because "cloud permissions" and GitHub teams are not synchronized. I.e. permissions are given to individuals. E.g. if someone is added to the bors team, they don't gain access to the AWS bors DB automatically.

If yes, how?

Note

Leadership council members can skip this section if they are not interested.

If we want to allow team leaders to approve changes to their teams, we need to discuss how to do this.
I have a PR which implements the ownership mechanism with GitHub CODEOWNERS. Unfortunately the con of this approach is that everyone in the @rust-lang/team-repo-admins and @rust-lang/mods teams will be notified about all the PRs in the /people/, /repos/ and /teams/ directories.

I need to know from everyone in the @rust-lang/team-repo-admins and @rust-lang/mods teams whether they want to receive a notification or not on PRs touching the directories listed above. 🙏

I'll try to adjust your team's code review settings based on your preferences. See this screenshot for example (these settings are not saved):

There are many people complaining about CODEOWNERS notifications. Hopefully we'll find a way to configure these settings that makes everyone happy.

[ehuss]

GitHub teams are not synchronized

I don't understand this part. Isn't the main purpose of sync-team to update the GitHub team memberships?

It sounds like we wouldn't be update the people list, so I assume that means we wouldn't be able to add new people unless they are already part of the org, is that correct?

How common is it for PRs to match these criteria (editing just the team)? My impression is that is not super common, so I'm a little uncertain how this particularly improves the review bottleneck compared to the increased security risk.

Can leads self-approve their own PRs?

[Kobzol]

Can leads self-approve their own PRs?

No, but that doesn't really matter, because once you have merge rights, you can trivially bypass that by creating a PR from another account and then approving.

[marcoieni]

I don't understand this part. Isn't the main purpose of sync-team to update the GitHub team memberships?

I'll try to explain myself better: if you add someone to the members list of a team, sync-team adds them to the GitHub team automatically. However, this doesn't grant them access to AWS resources or similar. So from a security point of view it's not a huge concern.

It sounds like we wouldn't be update the people list, so I assume that means we wouldn't be able to add new people unless they are already part of the org, is that correct?

yes: team leads won't be able to edit the /people/ directory 👍

Can leads self-approve their own PRs?

nobody can on GitHub in general.

[ehuss]

No, but that doesn't really matter, because once you have merge rights, you can trivially bypass that by creating a PR from another account and then approving.

My concern was less about the security aspect, and more about the common workflow. Usually it is the team lead making changes to the team, and due to that it sounds like this wouldn't help for that?

I'm just a little unclear about what this buys us, since it seems rare that a non-team lead would want to make a change to just the team file. What kind of use cases are you thinking of, and how often has that come up?

[Kobzol]

I don't have stats, but I saw it quite often that either the person removes themselves from a team, or someone else (either from or outside the team, but not a lead) nominates someone for the team. It doesn't necessarily be done by the team lead. We could in theory allow self-approvals for these situations, although it would be technically complicated.

But this is debating specifics, I think. The over-arching goal is: "give more people merge rights on team", to unblock the review bottleneck. That's essentially what we're trying to achieve here. Giving team leads permissions to merge changes in their team is just an incremental step towards that, that we're trying to start with, since it's technically easier than some other changes.

[marcoieni]

Out of curiosity I looked at the infra team: the last 10 commits are not from a leader

Other use cases not mentioned by Kobzol might be:

• people changing username
• rotating team leaders

Also, if team leaders know that they can merge the PR, they might ask someone else to open it to speed up things 👍

[jackh726]

Something to consider (though technically it may be challeging): A nice workflow would actually to need either 1) a team lead to open the PR, and any other "approver" to approve or 2) a team lead approval for any person.

I'm not really worried about the security concerns here. I'm sure there is some extra risk to potentially allowing a compromised lead account to edit the team members, but given that the users must already be in the org, that risk is pretty minimal.

I would rather not be pinged for every PR. I think better would be to have some bors-like approach than relying on CODEOWNERS (and would make the workflow I mentioned at the top actually feasible, I think).

[marcoieni]

1. a team lead to open the PR, and any other "approver" to approve or 2) a team lead approval for any person.

this is what we achieve with codeowners in my draft PR already, where the "approver" can be someone from team-repo-admins or mods (and infra-admins too technically).
Unfortunately the "approver" cannot be anyone from the all team because in codeowners you can't express "if the author of the PR belongs to this team". codeowners only check who approved the PR.

I think better would be to have some bors-like approach

My preference would be to try to re-use what GitHub offers as much as possible, test it and see if it works out well enough before jumping in and replacing their tools with ours, so that we reduce the maintenance burden of the infra team.

[jackh726]

this is what we achieve with codeowners in my draft PR already, where the "approver" can be someone from team-repo-admins or mods (and infra-admins too technically). Unfortunately the "approver" cannot be anyone from the all team because in codeowners you can't express "if the author of the PR belongs to this team". codeowners only check who approved the PR.

Right, in my mind the "approver" list here is team-repo-admins, mods, and leads.

My preference would be to try to re-use what GitHub offers as much as possible, test it and see if it works out well enough before jumping in and replacing their tools with ours, so that we reduce the maintenance burden of the infra team.

Yeah, I think this is fine. If there's a way to do the CODEOWNERS approach without the ping for now, that would be my preference.

[traviscross]

As a policy matter, the council accepted this via FCP in rust-lang/leadership-council#171, and that FCP has now completed.

[marcoieni]

Since the FCP was approved, let's try to implement this with CODEOWNERS. If it doesn't work, we can always revert 👍

Only Jack expressed the notification preference, so I will assume that the other team members don't want to be notified as well. Feel free to let me know if this doesn't work for you.

For both the rust-lang/mods and rust-lang/team-repo-admins teams, I will disable notifications on review requests.
I'm doing this because according to the GitHub searches for mods and team-repo-admins, the two teams were never requested for a review. If you are curious, the infra team was requested a review once.

I'll get the PR ready for review in ~two weeks.