diff --git a/config.toml b/config.toml index a52cd38e5..2974122e4 100644 --- a/config.toml +++ b/config.toml @@ -27,3 +27,29 @@ permissions-bools = [ "dev-desktop", "sync-team-confirmation", ] + +# GitHub accounts that are allowed to stay in the orgs, +# even if they may not be members of any team. +allowed-org-members = [ + # Bots. + "bors", + "craterbot", + "rust-embedded-bot", + "rust-highfive", + "rust-lang-core-team-agenda-bot", + "rust-lang-glacier-bot", + "rust-lang-owner", + "rust-timer", + "rustbot", + + # Infra admins. + "jdno", + "marcoieni", + "Mark-Simulacrum", + "pietroalbini", + + # Ferrous systems employees who manage the github sponsors + # for rust-analyzer. + "missholmes", + "skade", +] diff --git a/rust_team_data/src/v1.rs b/rust_team_data/src/v1.rs index 0ee7b3b91..82119b9a8 100644 --- a/rust_team_data/src/v1.rs +++ b/rust_team_data/src/v1.rs @@ -1,8 +1,15 @@ +use std::collections::BTreeSet; + use indexmap::IndexMap; use serde::{Deserialize, Serialize}; pub static BASE_URL: &str = "https://team-api.infra.rust-lang.org/v1"; +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Default)] +pub struct Config { + pub allowed_org_members: BTreeSet, +} + #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)] #[serde(rename_all = "snake_case")] pub enum TeamKind { diff --git a/src/schema.rs b/src/schema.rs index 44eecef01..33b212a40 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -3,7 +3,7 @@ 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")] @@ -12,6 +12,8 @@ pub(crate) struct Config { allowed_github_orgs: HashSet, permissions_bors_repos: HashSet, permissions_bools: HashSet, + // Use a BTreeSet for consistent ordering in tests + allowed_org_members: BTreeSet, } impl Config { @@ -30,6 +32,10 @@ impl Config { pub(crate) fn permissions_bools(&self) -> &HashSet { &self.permissions_bools } + + pub(crate) fn allowed_org_members(&self) -> &BTreeSet { + &self.allowed_org_members + } } // This is an enum to allow two kinds of values for the email field: diff --git a/src/static_api.rs b/src/static_api.rs index fbd98ff0a..a6008cae3 100644 --- a/src/static_api.rs +++ b/src/static_api.rs @@ -33,6 +33,7 @@ impl<'a> Generator<'a> { self.generate_rfcbot()?; self.generate_zulip_map()?; self.generate_people()?; + self.generate_config()?; self.generate_index_html()?; Ok(()) } @@ -458,6 +459,15 @@ impl<'a> Generator<'a> { Ok(()) } + fn generate_config(&self) -> Result<(), Error> { + let config = v1::Config { + allowed_org_members: self.data.config().allowed_org_members().clone(), + }; + + self.add("v1/config.json", &config)?; + Ok(()) + } + fn generate_index_html(&self) -> Result<(), Error> { const CONTENT: &[u8] = b"\ \n\ 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..a33649c8e 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -6,7 +6,7 @@ use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole}; 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 +18,9 @@ pub(crate) fn create_diff( github: Box, teams: Vec, repos: Vec, + config: rust_team_data::v1::Config, ) -> anyhow::Result { - let github = SyncGitHub::new(github, teams, repos)?; + let github = SyncGitHub::new(github, teams, repos, config)?; github.diff_all() } @@ -29,8 +30,10 @@ struct SyncGitHub { github: Box, teams: Vec, repos: Vec, + config: rust_team_data::v1::Config, usernames_cache: HashMap, org_owners: HashMap>, + org_members: HashMap>, } impl SyncGitHub { @@ -38,6 +41,7 @@ impl SyncGitHub { github: Box, teams: Vec, repos: Vec, + config: rust_team_data::v1::Config, ) -> anyhow::Result { debug!("caching mapping between user ids and usernames"); let users = teams @@ -60,30 +64,112 @@ 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 { + 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.allowed_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 +670,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 +682,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 +712,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 +754,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..84e9b0d1d 100644 --- a/sync-team/src/github/tests/mod.rs +++ b/sync-team/src/github/tests/mod.rs @@ -97,6 +97,33 @@ 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 user = model.create_user("sakura"); + model.create_team(TeamData::new("team-1").gh_team("rust-lang", "members-gh", &[user])); + let mut gh = model.gh_model(); + gh.add_member("rust-lang", "martin"); + + // Add a bot that shouldn't be removed from the org. + let bot = "my-bot"; + gh.add_member("rust-lang", 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(); diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index 8f2f88733..11b3505ec 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -1,16 +1,17 @@ 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, Config, GitHubTeam, MergeBot, Person, RepoPermission, + TeamGitHub, TeamKind, }; 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 +28,7 @@ pub struct DataModel { people: Vec, teams: Vec, repos: Vec, + config: Config, } impl DataModel { @@ -64,6 +66,10 @@ impl DataModel { .expect("Repo not found") } + pub fn add_allowed_org_member(&mut self, member: &str) { + self.config.allowed_org_members.insert(member.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 +111,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 +180,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 +201,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 +437,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 + .get_mut(org) + .unwrap() + .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 +486,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 +582,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..909141d5e 100644 --- a/sync-team/src/lib.rs +++ b/sync-team/src/lib.rs @@ -31,7 +31,8 @@ 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 config = team_api.get_config()?; + let diff = create_diff(gh_read, teams, repos, config)?; if !diff.is_empty() { info!("{diff}"); } diff --git a/sync-team/src/team_api.rs b/sync-team/src/team_api.rs index d5e2218a0..3a1f925ae 100644 --- a/sync-team/src/team_api.rs +++ b/sync-team/src/team_api.rs @@ -47,6 +47,11 @@ impl TeamApi { self.req::("zulip-streams.json") } + pub(crate) fn get_config(&self) -> anyhow::Result { + debug!("loading config from the Team API"); + self.req::("config.json") + } + fn req(&self, url: &str) -> anyhow::Result { match self { TeamApi::Production => { diff --git a/tests/static-api/_expected/v1/config.json b/tests/static-api/_expected/v1/config.json new file mode 100644 index 000000000..b849970b2 --- /dev/null +++ b/tests/static-api/_expected/v1/config.json @@ -0,0 +1,6 @@ +{ + "allowed_org_members": [ + "test-admin", + "test-bot" + ] +} \ No newline at end of file diff --git a/tests/static-api/config.toml b/tests/static-api/config.toml index 069dafc3d..6ef6da654 100644 --- a/tests/static-api/config.toml +++ b/tests/static-api/config.toml @@ -14,3 +14,8 @@ permissions-bors-repos = [ permissions-bools = [ "crater", ] + +allowed-org-members = [ + "test-bot", + "test-admin", +]