diff --git a/config.toml b/config.toml index a52cd38e5..4d73169de 100644 --- a/config.toml +++ b/config.toml @@ -15,6 +15,12 @@ allowed-github-orgs = [ "rust-analyzer", ] +# GitHub organizations where member management is independent +# and should not be automatically synchronized +independent-github-orgs = [ + "rust-embedded", +] + permissions-bors-repos = [ "bors-kindergarten", "rust", @@ -27,3 +33,25 @@ permissions-bools = [ "dev-desktop", "sync-team-confirmation", ] + +# GitHub accounts that are allowed to stay in the GitHub organizations, +# even if they may not be members of any team. +# Note: Infra admins are automatically included from the infra-admins team. +special-org-members = [ + # Bots. + "bors", + "craterbot", + "rust-embedded-bot", + "rust-highfive", + "rust-lang-core-team-agenda-bot", + "rust-lang-glacier-bot", + "rust-lang-owner", + "rust-log-analyzer", + "rust-timer", + "rustbot", + + # Ferrous systems employees who manage the github sponsors + # for rust-analyzer. + "MissHolmes", + "skade", +] diff --git a/src/data.rs b/src/data.rs index a0f6897fa..b523dfc40 100644 --- a/src/data.rs +++ b/src/data.rs @@ -217,6 +217,26 @@ impl Data { .flatten() .any(|github_team| github_team.name == team_name && github_team.org == org) } + + pub(crate) fn get_sync_team_config(&self) -> anyhow::Result { + let mut special_org_members = self.config.special_org_members().clone(); + + // Add infra admins from the infra-admins team + let infra_admins_team = self + .team("infra-admins") + .context("cannot find infra-admins team")?; + let infra_admins_usernames = infra_admins_team + .raw_people() + .members + .iter() + .map(|m| m.github.clone()); + special_org_members.extend(infra_admins_usernames); + + Ok(sync_team::Config { + special_org_members, + independent_github_orgs: self.config.independent_github_orgs().clone(), + }) + } } fn load_file(path: &Path) -> Result { diff --git a/src/main.rs b/src/main.rs index 3c4f225aa..be879a166 100644 --- a/src/main.rs +++ b/src/main.rs @@ -590,5 +590,11 @@ fn perform_sync(opts: SyncOpts, data: Data) -> anyhow::Result<()> { let subcmd = opts.command.unwrap_or(SyncCommand::DryRun); let only_print_plan = matches!(subcmd, SyncCommand::PrintPlan); let dry_run = only_print_plan || matches!(subcmd, SyncCommand::DryRun); - run_sync_team(team_api, &services, dry_run, only_print_plan) + run_sync_team( + team_api, + &services, + dry_run, + only_print_plan, + data.get_sync_team_config()?, + ) } diff --git a/src/schema.rs b/src/schema.rs index 44eecef01..3c07bed7b 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -3,15 +3,18 @@ pub(crate) use crate::permissions::Permissions; use anyhow::{bail, format_err, Error}; use serde::de::{Deserialize, Deserializer}; use serde_untagged::UntaggedEnumVisitor; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeSet, HashMap, HashSet}; #[derive(serde_derive::Deserialize, Debug)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] pub(crate) struct Config { allowed_mailing_lists_domains: HashSet, allowed_github_orgs: HashSet, + independent_github_orgs: BTreeSet, permissions_bors_repos: HashSet, permissions_bools: HashSet, + // Use a BTreeSet for consistent ordering in tests + special_org_members: BTreeSet, } impl Config { @@ -30,6 +33,14 @@ impl Config { pub(crate) fn permissions_bools(&self) -> &HashSet { &self.permissions_bools } + + pub(crate) fn independent_github_orgs(&self) -> &BTreeSet { + &self.independent_github_orgs + } + + pub(crate) fn special_org_members(&self) -> &BTreeSet { + &self.special_org_members + } } // This is an enum to allow two kinds of values for the email field: diff --git a/sync-team/src/github/api/read.rs b/sync-team/src/github/api/read.rs index e68a191f8..9a4031ec2 100644 --- a/sync-team/src/github/api/read.rs +++ b/sync-team/src/github/api/read.rs @@ -15,6 +15,9 @@ pub(crate) trait GithubRead { /// Get the owners of an org fn org_owners(&self, org: &str) -> anyhow::Result>; + /// Get the members of an org + fn org_members(&self, org: &str) -> anyhow::Result>; + /// Get all teams associated with a org /// /// Returns a list of tuples of team name and slug @@ -119,6 +122,26 @@ impl GithubRead for GitHubApiRead { Ok(owners) } + fn org_members(&self, org: &str) -> anyhow::Result> { + #[derive(serde::Deserialize, Eq, PartialEq, Hash)] + struct User { + id: u64, + login: String, + } + let mut members = HashMap::new(); + self.client.rest_paginated( + &Method::GET, + &GitHubUrl::orgs(org, "members")?, + |resp: Vec| { + for user in resp { + members.insert(user.id, user.login); + } + Ok(()) + }, + )?; + Ok(members) + } + fn org_teams(&self, org: &str) -> anyhow::Result> { let mut teams = Vec::new(); diff --git a/sync-team/src/github/api/write.rs b/sync-team/src/github/api/write.rs index 9b03bf416..439e1eb2b 100644 --- a/sync-team/src/github/api/write.rs +++ b/sync-team/src/github/api/write.rs @@ -344,6 +344,18 @@ impl GitHubWrite { Ok(()) } + /// Remove a member from an org + pub(crate) fn remove_gh_member_from_org(&self, org: &str, user: &str) -> anyhow::Result<()> { + debug!("Removing user {user} from org {org}"); + if !self.dry_run { + let method = Method::DELETE; + let url = GitHubUrl::orgs(org, &format!("members/{user}"))?; + let resp = self.client.req(method.clone(), &url)?.send()?; + allow_not_found(resp, method, url.url())?; + } + Ok(()) + } + /// Remove a collaborator from a repo pub(crate) fn remove_collaborator_from_repo( &self, diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 3f563e621..15d8674b4 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -3,10 +3,11 @@ mod api; mod tests; use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole}; +use crate::Config; use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings}; use log::debug; use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot}; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Write; pub(crate) use self::api::{GitHubApiRead, GitHubWrite, HttpClient}; @@ -18,8 +19,9 @@ pub(crate) fn create_diff( github: Box, teams: Vec, repos: Vec, + config: Config, ) -> anyhow::Result { - let github = SyncGitHub::new(github, teams, repos)?; + let github = SyncGitHub::new(github, teams, repos, config)?; github.diff_all() } @@ -29,8 +31,10 @@ struct SyncGitHub { github: Box, teams: Vec, repos: Vec, + config: Config, usernames_cache: HashMap, org_owners: HashMap>, + org_members: HashMap>, } impl SyncGitHub { @@ -38,6 +42,7 @@ impl SyncGitHub { github: Box, teams: Vec, repos: Vec, + config: Config, ) -> anyhow::Result { debug!("caching mapping between user ids and usernames"); let users = teams @@ -60,30 +65,118 @@ impl SyncGitHub { .collect::>(); let mut org_owners = HashMap::new(); + let mut org_members = HashMap::new(); for org in &orgs { org_owners.insert((*org).to_string(), github.org_owners(org)?); + org_members.insert((*org).to_string(), github.org_members(org)?); } Ok(SyncGitHub { github, teams, repos, + config, usernames_cache, org_owners, + org_members, }) } pub(crate) fn diff_all(&self) -> anyhow::Result { let team_diffs = self.diff_teams()?; let repo_diffs = self.diff_repos()?; + let org_membership_diffs = self.diff_org_memberships()?; Ok(Diff { team_diffs, repo_diffs, + org_membership_diffs, }) } + /// Collect all org members from the respective teams + fn get_org_members_from_teams(&self) -> HashMap> { + let mut org_team_members: HashMap> = HashMap::new(); + + for team in &self.teams { + if let Some(gh) = &team.github { + for toml_gh_team in &gh.teams { + org_team_members + .entry(toml_gh_team.org.clone()) + .or_default() + .extend(toml_gh_team.members.iter().copied()); + } + } + } + org_team_members + } + + /// Diff organization memberships between TOML teams and GitHub + fn diff_org_memberships(&self) -> anyhow::Result> { + let toml_org_team_members = self.get_org_members_from_teams(); + + let mut org_diffs: BTreeMap = BTreeMap::new(); + + for (org, toml_members) in toml_org_team_members { + // Skip independent organizations - they manage their own members + if self.config.independent_github_orgs.contains(&org) { + debug!("Skipping member sync for independent organization: {}", org); + continue; + } + + let Some(gh_org_members) = self.org_members.get(&org) else { + panic!("GitHub organization {org} not found"); + }; + + let members_to_remove = self.members_to_remove(toml_members, gh_org_members); + + // The rest are members that should be removed + if !members_to_remove.is_empty() { + let mut members_to_remove: Vec = members_to_remove.into_values().collect(); + members_to_remove.sort(); + + org_diffs.insert( + org.clone(), + OrgMembershipDiff { + org, + members_to_remove, + }, + ); + } + } + + Ok(org_diffs.into_values().collect()) + } + + /// Return GitHub members that should be removed from the organization. + fn members_to_remove( + &self, + toml_members: HashSet, + gh_org_members: &HashMap, + ) -> HashMap { + // Initialize `members_to_remove` to all GitHub members in the org. + // Next, we'll delete members from `members_to_remove` that don't respect certain criteria. + let mut members_to_remove = gh_org_members.clone(); + + // People who belong to a team should stay in the org. + for member in toml_members { + members_to_remove.remove(&member); + } + + // Members that are explicitly allowed in the `config.toml` file should stay in the org. + for allowed_member in &self.config.special_org_members { + if let Some(member_to_retain) = members_to_remove + .iter() + .find(|(_, username)| username == &allowed_member) + .map(|(id, _)| *id) + { + members_to_remove.remove(&member_to_retain); + } + } + members_to_remove + } + fn diff_teams(&self) -> anyhow::Result> { let mut diffs = Vec::new(); let mut unseen_github_teams = HashMap::new(); @@ -584,6 +677,7 @@ const BOTS_TEAMS: &[&str] = &["bors", "highfive", "rfcbot", "bots"]; pub(crate) struct Diff { team_diffs: Vec, repo_diffs: Vec, + org_membership_diffs: Vec, } impl Diff { @@ -595,12 +689,17 @@ impl Diff { for repo_diff in self.repo_diffs { repo_diff.apply(sync)?; } + for org_diff in self.org_membership_diffs { + org_diff.apply(sync)?; + } Ok(()) } pub(crate) fn is_empty(&self) -> bool { - self.team_diffs.is_empty() && self.repo_diffs.is_empty() + self.team_diffs.is_empty() + && self.repo_diffs.is_empty() + && self.org_membership_diffs.is_empty() } } @@ -620,6 +719,13 @@ impl std::fmt::Display for Diff { } } + if !&self.org_membership_diffs.is_empty() { + writeln!(f, "💻 Org membership Diffs:")?; + for org_diff in &self.org_membership_diffs { + write!(f, "{org_diff}")?; + } + } + Ok(()) } } @@ -655,6 +761,34 @@ impl std::fmt::Display for RepoDiff { } } +#[derive(Debug)] +struct OrgMembershipDiff { + org: OrgName, + members_to_remove: Vec, +} + +impl OrgMembershipDiff { + fn apply(self, sync: &GitHubWrite) -> anyhow::Result<()> { + for member in &self.members_to_remove { + sync.remove_gh_member_from_org(&self.org, member)?; + } + + Ok(()) + } +} + +impl std::fmt::Display for OrgMembershipDiff { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if !self.members_to_remove.is_empty() { + writeln!(f, "❌ Removing the following members from `{}`:", self.org)?; + for member in &self.members_to_remove { + writeln!(f, " - {member}",)?; + } + } + Ok(()) + } +} + #[derive(Debug)] struct CreateRepoDiff { org: String, diff --git a/sync-team/src/github/tests/mod.rs b/sync-team/src/github/tests/mod.rs index f74a8ae70..c22696329 100644 --- a/sync-team/src/github/tests/mod.rs +++ b/sync-team/src/github/tests/mod.rs @@ -97,6 +97,34 @@ fn team_dont_add_member_if_invitation_is_pending() { insta::assert_debug_snapshot!(team_diff, @"[]"); } +#[test] +fn remove_org_members() { + let mut model = DataModel::default(); + let rust_lang_org = "rust-lang"; + let user = model.create_user("sakura"); + model.create_team(TeamData::new("team-1").gh_team(rust_lang_org, "members-gh", &[user])); + let mut gh = model.gh_model(); + gh.add_member(rust_lang_org, "martin"); + + // Add a bot that shouldn't be removed from the org. + let bot = "my-bot"; + gh.add_member(rust_lang_org, bot); + model.add_allowed_org_member(bot); + + let gh_org_diff = model.diff_org_membership(gh); + + insta::assert_debug_snapshot!(gh_org_diff, @r#" + [ + OrgMembershipDiff { + org: "rust-lang", + members_to_remove: [ + "martin", + ], + }, + ] + "#); +} + #[test] fn team_remove_member() { let mut model = DataModel::default(); @@ -866,3 +894,26 @@ fn repo_remove_branch_protection() { ] "#); } + +#[test] +fn independent_orgs_are_not_synced() { + let mut model = DataModel::default(); + let user = model.create_user("sakura"); + + let independent_org = "independent-org"; + + // Create a team, so that membership is synced. + model.create_team(TeamData::new("team").gh_team(independent_org, "team-gh", &[user])); + + let mut gh = model.gh_model(); + + // Add a member who is not part of any team. + gh.add_member(independent_org, "independent-user-1"); + + model.add_independent_github_org(independent_org); + + let gh_org_diff = model.diff_org_membership(gh); + + // No members should be removed for independent organizations + insta::assert_debug_snapshot!(gh_org_diff, @"[]"); +} diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index 8f2f88733..5fc259595 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -1,16 +1,18 @@ use std::collections::{BTreeSet, HashMap, HashSet}; use derive_builder::Builder; -use rust_team_data::v1; use rust_team_data::v1::{ - Bot, BranchProtectionMode, GitHubTeam, MergeBot, Person, RepoPermission, TeamGitHub, TeamKind, + self, Bot, BranchProtectionMode, GitHubTeam, MergeBot, Person, RepoPermission, TeamGitHub, + TeamKind, }; +use crate::Config; use crate::github::api::{ BranchProtection, GithubRead, Repo, RepoTeam, RepoUser, Team, TeamMember, TeamPrivacy, TeamRole, }; use crate::github::{ - RepoDiff, SyncGitHub, TeamDiff, api, construct_branch_protection, convert_permission, + OrgMembershipDiff, RepoDiff, SyncGitHub, TeamDiff, api, construct_branch_protection, + convert_permission, }; pub const DEFAULT_ORG: &str = "rust-lang"; @@ -27,6 +29,7 @@ pub struct DataModel { people: Vec, teams: Vec, repos: Vec, + config: Config, } impl DataModel { @@ -64,6 +67,14 @@ impl DataModel { .expect("Repo not found") } + pub fn add_allowed_org_member(&mut self, member: &str) { + self.config.special_org_members.insert(member.to_string()); + } + + pub fn add_independent_github_org(&mut self, org: &str) { + self.config.independent_github_orgs.insert(org.to_string()); + } + /// Creates a GitHub model from the current team data mock. /// Note that all users should have been created before calling this method, so that /// GitHub knows about the users' existence. @@ -105,7 +116,13 @@ impl DataModel { slug: gh_team.name.clone(), }); - org.members.extend(gh_team.members.iter().copied()); + org.members.extend( + gh_team + .members + .iter() + .copied() + .map(|user_id| (user_id, users[&user_id].clone())), + ); } } @@ -168,6 +185,12 @@ impl DataModel { GithubMock { users, orgs } } + pub fn diff_org_membership(&self, github: GithubMock) -> Vec { + self.create_sync(github) + .diff_org_memberships() + .expect("Cannot diff org membership") + } + pub fn diff_teams(&self, github: GithubMock) -> Vec { self.create_sync(github) .diff_teams() @@ -183,8 +206,9 @@ impl DataModel { fn create_sync(&self, github: GithubMock) -> SyncGitHub { let teams = self.teams.iter().cloned().map(|t| t.into()).collect(); let repos = self.repos.iter().cloned().map(|r| r.into()).collect(); + let config = self.config.clone(); - SyncGitHub::new(Box::new(github), teams, repos).expect("Cannot create SyncGitHub") + SyncGitHub::new(Box::new(github), teams, repos, config).expect("Cannot create SyncGitHub") } } @@ -418,6 +442,16 @@ pub struct GithubMock { } impl GithubMock { + pub fn add_member(&mut self, org: &str, username: &str) { + let user_id = self.users.len() as UserId; + self.users.insert(user_id, username.to_string()); + self.orgs + .entry(org.to_string()) + .or_default() + .members + .insert((user_id, username.to_string())); + } + pub fn add_invitation(&mut self, org: &str, repo: &str, user: &str) { self.get_org_mut(org) .team_invitations @@ -457,6 +491,10 @@ impl GithubRead for GithubMock { Ok(self.get_org(org).owners.iter().copied().collect()) } + fn org_members(&self, org: &str) -> anyhow::Result> { + Ok(self.get_org(org).members.iter().cloned().collect()) + } + fn org_teams(&self, org: &str) -> anyhow::Result> { Ok(self .get_org(org) @@ -549,7 +587,7 @@ impl GithubRead for GithubMock { #[derive(Default)] struct GithubOrg { - members: BTreeSet, + members: BTreeSet<(UserId, String)>, owners: BTreeSet, teams: Vec, // Team name -> list of invited users diff --git a/sync-team/src/lib.rs b/sync-team/src/lib.rs index cd22d4a27..8999597f2 100644 --- a/sync-team/src/lib.rs +++ b/sync-team/src/lib.rs @@ -4,6 +4,8 @@ pub mod team_api; mod utils; mod zulip; +use std::collections::BTreeSet; + use crate::github::{GitHubApiRead, GitHubWrite, HttpClient, create_diff}; use crate::team_api::TeamApi; use crate::zulip::SyncZulip; @@ -13,11 +15,18 @@ use secrecy::SecretString; const USER_AGENT: &str = "rust-lang teams sync (https://github.com/rust-lang/sync-team)"; +#[derive(Debug, Clone, Default)] +pub struct Config { + pub special_org_members: BTreeSet, + pub independent_github_orgs: BTreeSet, +} + pub fn run_sync_team( team_api: TeamApi, services: &[String], dry_run: bool, only_print_plan: bool, + config: Config, ) -> anyhow::Result<()> { if dry_run { warn!("sync-team is running in dry mode, no changes will be applied."); @@ -31,7 +40,7 @@ pub fn run_sync_team( let gh_read = Box::new(GitHubApiRead::from_client(client.clone())?); let teams = team_api.get_teams()?; let repos = team_api.get_repos()?; - let diff = create_diff(gh_read, teams, repos)?; + let diff = create_diff(gh_read, teams, repos, config.clone())?; if !diff.is_empty() { info!("{diff}"); } diff --git a/tests/static-api/_expected/v1/people.json b/tests/static-api/_expected/v1/people.json index 899d2d5ff..3a4ae9a7e 100644 --- a/tests/static-api/_expected/v1/people.json +++ b/tests/static-api/_expected/v1/people.json @@ -1,5 +1,10 @@ { "people": { + "test-admin": { + "name": "Test Admin", + "email": "test-admin@example.com", + "github_id": 7 + }, "user-0": { "name": "Zeroth user", "email": "user0@example.com", diff --git a/tests/static-api/_expected/v1/teams.json b/tests/static-api/_expected/v1/teams.json index 4eb23d49d..2331ff47b 100644 --- a/tests/static-api/_expected/v1/teams.json +++ b/tests/static-api/_expected/v1/teams.json @@ -75,6 +75,24 @@ "roles": [], "discord": [] }, + "infra-admins": { + "name": "infra-admins", + "kind": "marker_team", + "subteam_of": null, + "members": [ + { + "name": "Test Admin", + "github": "test-admin", + "github_id": 7, + "is_lead": false + } + ], + "alumni": [], + "github": null, + "website_data": null, + "roles": [], + "discord": [] + }, "leaderless": { "name": "leaderless", "kind": "team", diff --git a/tests/static-api/_expected/v1/teams/infra-admins.json b/tests/static-api/_expected/v1/teams/infra-admins.json new file mode 100644 index 000000000..6834181f1 --- /dev/null +++ b/tests/static-api/_expected/v1/teams/infra-admins.json @@ -0,0 +1,18 @@ +{ + "name": "infra-admins", + "kind": "marker_team", + "subteam_of": null, + "members": [ + { + "name": "Test Admin", + "github": "test-admin", + "github_id": 7, + "is_lead": false + } + ], + "alumni": [], + "github": null, + "website_data": null, + "roles": [], + "discord": [] +} \ No newline at end of file diff --git a/tests/static-api/config.toml b/tests/static-api/config.toml index 069dafc3d..e93673baa 100644 --- a/tests/static-api/config.toml +++ b/tests/static-api/config.toml @@ -6,6 +6,10 @@ allowed-github-orgs = [ "test-org", ] +independent-github-orgs = [ + "org-independent", +] + permissions-bors-repos = [ "crates-io", "crater", @@ -14,3 +18,7 @@ permissions-bors-repos = [ permissions-bools = [ "crater", ] + +special-org-members = [ + "test-bot", +] diff --git a/tests/static-api/people/test-admin.toml b/tests/static-api/people/test-admin.toml new file mode 100644 index 000000000..68cbbcb63 --- /dev/null +++ b/tests/static-api/people/test-admin.toml @@ -0,0 +1,4 @@ +name = 'Test Admin' +github = 'test-admin' +github-id = 7 +email = 'test-admin@example.com' diff --git a/tests/static-api/teams/infra-admins.toml b/tests/static-api/teams/infra-admins.toml new file mode 100644 index 000000000..9de69c4a1 --- /dev/null +++ b/tests/static-api/teams/infra-admins.toml @@ -0,0 +1,9 @@ +name = "infra-admins" +kind = "marker-team" + +[people] +leads = [] +members = [ + "test-admin", +] +alumni = []