From 6d8b3878a8f17be10cc34a1c3a663d0af6928f6f Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Mon, 22 Dec 2025 23:16:00 +0530 Subject: [PATCH 1/4] feat: implement bypass actor ruleset --- sync-team/src/github/api/mod.rs | 277 +++++++++++++++++++++++++++++- sync-team/src/github/api/write.rs | 77 +++++++++ sync-team/src/github/mod.rs | 193 +++++++++++++++++++-- 3 files changed, 531 insertions(+), 16 deletions(-) diff --git a/sync-team/src/github/api/mod.rs b/sync-team/src/github/api/mod.rs index debac26ca..7005b6960 100644 --- a/sync-team/src/github/api/mod.rs +++ b/sync-team/src/github/api/mod.rs @@ -521,25 +521,110 @@ pub(crate) enum RulesetEnforcement { #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub(crate) struct RulesetBypassActor { - pub(crate) actor_id: i64, + /// The ID of the actor that can bypass a ruleset. + /// - Required for Integration, RepositoryRole, and Team actor types (use Some(ActorId::Id(n))) + /// - Must be 1 for OrganizationAdmin (use Some(ActorId::Id(1))) + /// - Must be null for DeployKey (use Some(ActorId::Null)) + /// - Omitted for EnterpriseOwner (use None) + #[serde( + skip_serializing_if = "Option::is_none", + default, + deserialize_with = "deserialize_actor_id" + )] + pub(crate) actor_id: Option, pub(crate) actor_type: RulesetActorType, - pub(crate) bypass_mode: RulesetBypassMode, + /// The bypass mode for the actor. Defaults to "always" per GitHub API. + /// - always: Actor can always bypass + /// - pull_request: Actor can only bypass on pull requests (not applicable for DeployKey) + /// - exempt: Rules won't run for actor, no audit entry created + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) bypass_mode: Option, } +/// Custom deserializer for actor_id that distinguishes between null and missing +fn deserialize_actor_id<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + use serde::{Deserialize, de}; + + struct ActorIdVisitor; + + impl<'de> de::Visitor<'de> for ActorIdVisitor { + type Value = Option; + + fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + formatter.write_str("an integer, null, or missing field") + } + + fn visit_none(self) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Null)) + } + + fn visit_some(self, deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let id = i64::deserialize(deserializer)?; + Ok(Some(ActorId::Id(id))) + } + + fn visit_unit(self) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Null)) + } + + fn visit_i64(self, v: i64) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Id(v))) + } + + fn visit_u64(self, v: u64) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Id(v as i64))) + } + } + + deserializer.deserialize_option(ActorIdVisitor) +} + +/// Represents the actor_id field which can be a number or null #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -#[serde(rename_all = "snake_case")] +#[serde(untagged)] +pub(crate) enum ActorId { + /// A specific actor ID (used for Integration, RepositoryRole, Team, OrganizationAdmin) + Id(i64), + /// Explicitly null (required for DeployKey) + Null, +} + +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "PascalCase")] pub(crate) enum RulesetActorType { Integration, OrganizationAdmin, RepositoryRole, Team, + DeployKey, + EnterpriseOwner, } #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "snake_case")] pub(crate) enum RulesetBypassMode { Always, + #[serde(rename = "pull_request")] PullRequest, + Exempt, } #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] @@ -636,3 +721,189 @@ pub(crate) enum RulesetOp { CreateForRepo, UpdateRuleset(i64), } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_bypass_actor_serialization() { + // Test Team actor with ID + let team_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(234)), + actor_type: RulesetActorType::Team, + bypass_mode: Some(RulesetBypassMode::Always), + }; + let json = + serde_json::to_string(&team_actor).expect("Team actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":234,"actor_type":"Team","bypass_mode":"always"}"#, + "Team actor should serialize with numeric actor_id, PascalCase actor_type, and snake_case bypass_mode" + ); + + // Test Integration actor with ID + let integration_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(123456)), + actor_type: RulesetActorType::Integration, + bypass_mode: Some(RulesetBypassMode::Always), + }; + let json = serde_json::to_string(&integration_actor) + .expect("Integration actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":123456,"actor_type":"Integration","bypass_mode":"always"}"#, + "Integration actor should serialize with numeric actor_id" + ); + + // Test OrganizationAdmin with ID 1 (per GitHub API spec) + let org_admin_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(1)), + actor_type: RulesetActorType::OrganizationAdmin, + bypass_mode: None, // Use API default + }; + let json = serde_json::to_string(&org_admin_actor) + .expect("OrganizationAdmin actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":1,"actor_type":"OrganizationAdmin"}"#, + "OrganizationAdmin should use actor_id:1 and omit bypass_mode when None" + ); + + // Test DeployKey with null actor_id (per GitHub API spec) + let deploy_key_actor = RulesetBypassActor { + actor_id: Some(ActorId::Null), + actor_type: RulesetActorType::DeployKey, + bypass_mode: Some(RulesetBypassMode::Always), + }; + let json = serde_json::to_string(&deploy_key_actor) + .expect("DeployKey actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":null,"actor_type":"DeployKey","bypass_mode":"always"}"#, + "DeployKey should serialize actor_id as null" + ); + + // Test EnterpriseOwner with None actor_id (field omitted per GitHub API spec) + let enterprise_actor = RulesetBypassActor { + actor_id: None, + actor_type: RulesetActorType::EnterpriseOwner, + bypass_mode: Some(RulesetBypassMode::Exempt), + }; + let json = serde_json::to_string(&enterprise_actor) + .expect("EnterpriseOwner actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_type":"EnterpriseOwner","bypass_mode":"exempt"}"#, + "EnterpriseOwner should omit actor_id field entirely" + ); + + // Test pull_request bypass mode + let pr_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(789)), + actor_type: RulesetActorType::Team, + bypass_mode: Some(RulesetBypassMode::PullRequest), + }; + let json = serde_json::to_string(&pr_actor) + .expect("PullRequest bypass mode serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":789,"actor_type":"Team","bypass_mode":"pull_request"}"#, + "PullRequest bypass mode should serialize as 'pull_request' with underscore" + ); + } + + #[test] + fn test_bypass_actor_deserialization() { + // Test deserializing from GitHub API response + let json = r#"{"actor_id":234,"actor_type":"Team","bypass_mode":"always"}"#; + let actor: RulesetBypassActor = + serde_json::from_str(json).expect("Should deserialize valid Team actor"); + assert_eq!( + actor.actor_id, + Some(ActorId::Id(234)), + "actor_id should be numeric" + ); + assert_eq!( + actor.actor_type, + RulesetActorType::Team, + "actor_type should be Team" + ); + assert_eq!( + actor.bypass_mode, + Some(RulesetBypassMode::Always), + "bypass_mode should be Always" + ); + + // Test with null actor_id (DeployKey) + // Custom deserializer ensures JSON null becomes Some(ActorId::Null), not None + let json = r#"{"actor_id":null,"actor_type":"DeployKey","bypass_mode":"always"}"#; + let actor: RulesetBypassActor = serde_json::from_str(json) + .expect("Should deserialize DeployKey actor with null actor_id"); + assert_eq!( + actor.actor_id, + Some(ActorId::Null), + "JSON null should deserialize to Some(ActorId::Null) with custom deserializer" + ); + assert_eq!(actor.actor_type, RulesetActorType::DeployKey); + + // Test with missing bypass_mode (should default to None) + let json = r#"{"actor_id":1,"actor_type":"OrganizationAdmin"}"#; + let actor: RulesetBypassActor = serde_json::from_str(json) + .expect("Should deserialize OrganizationAdmin without bypass_mode"); + assert_eq!(actor.actor_id, Some(ActorId::Id(1))); + assert_eq!( + actor.bypass_mode, None, + "bypass_mode should be None when omitted from JSON" + ); + + // Test with missing actor_id (EnterpriseOwner) + let json = r#"{"actor_type":"EnterpriseOwner","bypass_mode":"exempt"}"#; + let actor: RulesetBypassActor = serde_json::from_str(json) + .expect("Should deserialize EnterpriseOwner without actor_id"); + assert_eq!( + actor.actor_id, None, + "actor_id should be None when omitted from JSON" + ); + assert_eq!(actor.actor_type, RulesetActorType::EnterpriseOwner); + assert_eq!(actor.bypass_mode, Some(RulesetBypassMode::Exempt)); + + // Test all actor types can be deserialized + let actor_types = [ + ("Integration", RulesetActorType::Integration), + ("OrganizationAdmin", RulesetActorType::OrganizationAdmin), + ("RepositoryRole", RulesetActorType::RepositoryRole), + ("Team", RulesetActorType::Team), + ("DeployKey", RulesetActorType::DeployKey), + ("EnterpriseOwner", RulesetActorType::EnterpriseOwner), + ]; + for (type_str, expected_type) in actor_types { + let json = format!( + r#"{{"actor_id":1,"actor_type":"{}","bypass_mode":"always"}}"#, + type_str + ); + let actor: RulesetBypassActor = serde_json::from_str(&json) + .unwrap_or_else(|e| panic!("Should deserialize actor type {}: {}", type_str, e)); + assert_eq!( + actor.actor_type, expected_type, + "actor_type {} should deserialize correctly", + type_str + ); + } + + // Test all bypass modes can be deserialized + let bypass_modes = [ + ("always", RulesetBypassMode::Always), + ("pull_request", RulesetBypassMode::PullRequest), + ("exempt", RulesetBypassMode::Exempt), + ]; + for (mode_str, expected_mode) in bypass_modes { + let json = format!( + r#"{{"actor_id":1,"actor_type":"Team","bypass_mode":"{}"}}"#, + mode_str + ); + let actor: RulesetBypassActor = serde_json::from_str(&json) + .unwrap_or_else(|e| panic!("Should deserialize bypass mode {}: {}", mode_str, e)); + assert_eq!( + actor.bypass_mode, + Some(expected_mode), + "bypass_mode {} should deserialize correctly", + mode_str + ); + } + } +} diff --git a/sync-team/src/github/api/write.rs b/sync-team/src/github/api/write.rs index f24f6d741..6c89c58ff 100644 --- a/sync-team/src/github/api/write.rs +++ b/sync-team/src/github/api/write.rs @@ -89,6 +89,83 @@ impl GitHubWrite { Ok(data.organization.team.id) } + /// Resolve a team's database ID for use in rulesets + /// Returns None if the team doesn't exist in the organization + pub(crate) fn resolve_team_database_id( + &self, + org: &str, + name: &str, + ) -> anyhow::Result> { + #[derive(serde::Serialize)] + struct Params<'a> { + org: &'a str, + team: &'a str, + } + let query = " + query($org: String!, $team: String!) { + organization(login: $org) { + team(slug: $team) { + databaseId + } + } + } + "; + #[derive(serde::Deserialize)] + struct Data { + organization: Option, + } + #[derive(serde::Deserialize)] + struct Organization { + team: Option, + } + #[derive(serde::Deserialize)] + struct Team { + #[serde(rename = "databaseId")] + database_id: Option, + } + + let data: Data = self + .client + .graphql(query, Params { org, team: name }, org)?; + + Ok(data + .organization + .and_then(|org| org.team) + .and_then(|team| team.database_id)) + } + + /// Resolve a user's database ID for use in rulesets + /// Returns None if the user doesn't exist + pub(crate) fn resolve_user_database_id( + &self, + login: &str, + org: &str, + ) -> anyhow::Result> { + #[derive(serde::Serialize)] + struct Params<'a> { + login: &'a str, + } + let query = " + query($login: String!) { + user(login: $login) { + databaseId + } + } + "; + #[derive(serde::Deserialize)] + struct Data { + user: Option, + } + #[derive(serde::Deserialize)] + struct User { + #[serde(rename = "databaseId")] + database_id: Option, + } + + let data: Data = self.client.graphql(query, Params { login }, org)?; + Ok(data.user.and_then(|u| u.database_id)) + } + /// Create a team in a org pub(crate) fn create_team( &self, diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 68ae7e287..f1a85c88f 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -364,7 +364,7 @@ impl SyncGitHub { if use_rulesets { for branch_protection in &expected_repo.branch_protections { let ruleset = construct_ruleset(expected_repo, branch_protection); - rulesets.push(ruleset); + rulesets.push((ruleset, branch_protection.clone())); } } @@ -636,12 +636,15 @@ impl SyncGitHub { if let Some(actual_ruleset) = actual_rulesets_map.remove(&ruleset_name) { // Ruleset exists, check if it needs updating - // For simplicity, we compare the entire ruleset - // In production, you might want more sophisticated comparison - if actual_ruleset.rules != expected_ruleset.rules + // We compare rules, conditions, and enforcement + // Note: We don't compare bypass_actors here since they will be populated/updated + // during apply based on the branch_protection configuration + let needs_update = actual_ruleset.rules != expected_ruleset.rules || actual_ruleset.conditions != expected_ruleset.conditions || actual_ruleset.enforcement != expected_ruleset.enforcement - { + || self.bypass_actors_need_update(&actual_ruleset, branch_protection); + + if needs_update { let id = actual_ruleset.id.unwrap_or(0); ruleset_diffs.push(RulesetDiff { name: ruleset_name, @@ -650,6 +653,7 @@ impl SyncGitHub { actual_ruleset, expected_ruleset, ), + branch_protection: branch_protection.clone(), }); } } else { @@ -657,6 +661,7 @@ impl SyncGitHub { ruleset_diffs.push(RulesetDiff { name: ruleset_name, operation: RulesetDiffOperation::Create(expected_ruleset), + branch_protection: branch_protection.clone(), }); } } @@ -664,9 +669,17 @@ impl SyncGitHub { // Any remaining rulesets in actual_rulesets_map should be deleted for (name, ruleset) in actual_rulesets_map { if let Some(id) = ruleset.id { + // For delete operations, we create a dummy branch_protection since it won't be used ruleset_diffs.push(RulesetDiff { name, operation: RulesetDiffOperation::Delete(id), + branch_protection: rust_team_data::v1::BranchProtection { + pattern: String::new(), + dismiss_stale_review: false, + mode: rust_team_data::v1::BranchProtectionMode::PrNotRequired, + allowed_merge_teams: vec![], + merge_bots: vec![], + }, }); } } @@ -685,6 +698,45 @@ impl SyncGitHub { TeamRole::Member } } + + /// Check if bypass actors need to be updated based on branch protection configuration. + /// + /// This performs a simple comparison based on the count of expected actors. + /// A more sophisticated check would require resolving team/user IDs during diff + /// creation, which we avoid for performance. The count-based check will detect + /// most cases where updates are needed (teams added/removed, bots changed). + /// + /// Note: This may produce false positives if team/bot resolution fails during + /// construct_bypass_actors (e.g., team doesn't exist), since the expected count + /// assumes all actors can be resolved. This is acceptable as it errs on the side + /// of attempting updates rather than missing required changes. + fn bypass_actors_need_update( + &self, + actual_ruleset: &api::Ruleset, + branch_protection: &rust_team_data::v1::BranchProtection, + ) -> bool { + let expected_count = + branch_protection.allowed_merge_teams.len() + branch_protection.merge_bots.len(); + + let actual_count = actual_ruleset + .bypass_actors + .as_ref() + .map_or(0, |actors| actors.len()); + + // Update needed if counts don't match or if expected but no actual actors + if expected_count != actual_count { + return true; + } + + // If both are zero, no update needed + if expected_count == 0 { + return false; + } + + // Counts match and both are non-zero - assume update not needed + // A more sophisticated check would compare actual IDs/types + false + } } fn calculate_permission_diffs( @@ -895,6 +947,86 @@ pub(crate) fn convert_branch_pattern_to_ref_pattern(pattern: &str) -> String { format!("refs/heads/{}", pattern) } +/// Construct bypass actors from branch protection settings. +/// +/// This function resolves team names and bot logins to their GitHub database IDs +/// and creates `RulesetBypassActor` entries that allow these actors to bypass +/// branch protection rules. +/// +/// # Arguments +/// * `org` - The organization name +/// * `branch_protection` - Branch protection configuration containing allowed teams and bots +/// * `github_write` - GitHub API client for resolving IDs +/// +/// # Returns +/// A vector of `RulesetBypassActor` entries, or an error if ID resolution fails +pub(crate) fn construct_bypass_actors( + org: &str, + branch_protection: &rust_team_data::v1::BranchProtection, + github_write: &api::GitHubWrite, +) -> anyhow::Result> { + let mut bypass_actors = Vec::new(); + + for team_name in &branch_protection.allowed_merge_teams { + match github_write.resolve_team_database_id(org, team_name) { + Ok(Some(actor_id)) => { + bypass_actors.push(api::RulesetBypassActor { + actor_id: Some(api::ActorId::Id(actor_id)), + actor_type: api::RulesetActorType::Team, + bypass_mode: Some(api::RulesetBypassMode::Always), + }); + } + Ok(None) => { + debug!( + "Warning: Could not resolve database ID for team '{}/{}'. \ + The team may not exist in the organization.", + org, team_name + ); + } + Err(e) => { + anyhow::bail!( + "Failed to resolve database ID for team '{}/{}': {}", + org, + team_name, + e + ); + } + } + } + + for merge_bot in &branch_protection.merge_bots { + let user_login = match merge_bot { + MergeBot::Homu => "bors", + MergeBot::RustTimer => "rust-timer", + }; + match github_write.resolve_user_database_id(user_login, org) { + Ok(Some(actor_id)) => { + bypass_actors.push(api::RulesetBypassActor { + actor_id: Some(api::ActorId::Id(actor_id)), + actor_type: api::RulesetActorType::Integration, + bypass_mode: Some(api::RulesetBypassMode::Always), + }); + } + Ok(None) => { + debug!( + "Warning: Could not resolve database ID for bot '{}'. \ + This bot may not be installed in the organization.", + user_login + ); + } + Err(e) => { + anyhow::bail!( + "Failed to resolve database ID for bot '{}': {}", + user_login, + e + ); + } + } + } + + Ok(bypass_actors) +} + /// Convert a branch protection to a ruleset with merge queue enabled pub fn construct_ruleset( _expected_repo: &rust_team_data::v1::Repo, @@ -961,8 +1093,11 @@ pub fn construct_ruleset( }, }); - // TODO: Add bypass actors based on allowed_merge_teams and merge_bots - // This would require fetching team IDs, which needs more integration + // Note: bypass_actors will be populated later when applying the diff, + // as it requires resolving team/user IDs via GitHubWrite. + // The information needed comes from: + // - branch_protection.allowed_merge_teams (team names in expected_repo.org) + // - branch_protection.merge_bots (bot user logins) api::Ruleset { id: None, @@ -970,7 +1105,7 @@ pub fn construct_ruleset( target: RulesetTarget::Branch, source_type: RulesetSourceType::Repository, enforcement: RulesetEnforcement::Active, - bypass_actors: None, + bypass_actors: None, // Will be populated when applying the diff conditions: Some(RulesetConditions { ref_name: Some(RulesetRefNameCondition { include: vec![convert_branch_pattern_to_ref_pattern( @@ -1109,7 +1244,7 @@ struct CreateRepoDiff { settings: RepoSettings, permissions: Vec, branch_protections: Vec<(String, api::BranchProtection)>, - rulesets: Vec, + rulesets: Vec<(api::Ruleset, rust_team_data::v1::BranchProtection)>, environments: Vec<(String, rust_team_data::v1::Environment)>, } @@ -1131,10 +1266,11 @@ impl CreateRepoDiff { } // Apply rulesets (in addition to branch protections if configured) - for ruleset in &self.rulesets { + for (ruleset, branch_protection) in &self.rulesets { RulesetDiff { name: ruleset.name.clone(), operation: RulesetDiffOperation::Create(ruleset.clone()), + branch_protection: branch_protection.clone(), } .apply(sync, &self.org, &self.name)?; } @@ -1187,7 +1323,7 @@ impl std::fmt::Display for CreateRepoDiff { if !rulesets.is_empty() { writeln!(f, " Rulesets:")?; - for ruleset in rulesets { + for (ruleset, _branch_protection) in rulesets { let id_str = ruleset.id.map_or("new".to_string(), |id| id.to_string()); writeln!(f, " - {} (ID: {})", ruleset.name, id_str)?; if let Some(conditions) = &ruleset.conditions @@ -1823,17 +1959,48 @@ enum BranchProtectionDiffOperation { struct RulesetDiff { name: String, operation: RulesetDiffOperation, + branch_protection: rust_team_data::v1::BranchProtection, } impl RulesetDiff { + /// Populate bypass actors for a ruleset based on branch protection configuration. + fn populate_bypass_actors( + &self, + ruleset: &mut api::Ruleset, + org: &str, + sync: &GitHubWrite, + ) -> anyhow::Result<()> { + let actors = construct_bypass_actors(org, &self.branch_protection, sync)?; + ruleset.bypass_actors = if actors.is_empty() { + None + } else { + Some(actors) + }; + Ok(()) + } + fn apply(&self, sync: &GitHubWrite, org: &str, repo_name: &str) -> anyhow::Result<()> { use api::RulesetOp; match &self.operation { RulesetDiffOperation::Create(ruleset) => { - sync.upsert_ruleset(RulesetOp::CreateForRepo, org, repo_name, ruleset)?; + let mut ruleset_with_bypass = ruleset.clone(); + self.populate_bypass_actors(&mut ruleset_with_bypass, org, sync)?; + sync.upsert_ruleset( + RulesetOp::CreateForRepo, + org, + repo_name, + &ruleset_with_bypass, + )?; } RulesetDiffOperation::Update(id, _, new_ruleset) => { - sync.upsert_ruleset(RulesetOp::UpdateRuleset(*id), org, repo_name, new_ruleset)?; + let mut ruleset_with_bypass = new_ruleset.clone(); + self.populate_bypass_actors(&mut ruleset_with_bypass, org, sync)?; + sync.upsert_ruleset( + RulesetOp::UpdateRuleset(*id), + org, + repo_name, + &ruleset_with_bypass, + )?; } RulesetDiffOperation::Delete(id) => { debug!( From b49faca1ad79c9892b325a24b33b66fea22c9259 Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Mon, 22 Dec 2025 23:43:17 +0530 Subject: [PATCH 2/4] feat: implement allowed-merge-teams allowed-merge-users --- docs/toml-schema.md | 6 +++ rust_team_data/src/v1.rs | 2 + src/schema.rs | 2 + src/static_api.rs | 1 + sync-team/src/github/mod.rs | 44 ++++++++++++++++--- sync-team/src/github/tests/test_utils.rs | 3 ++ tests/static-api/_expected/v1/repos.json | 2 + .../_expected/v1/repos/archived_repo.json | 1 + .../_expected/v1/repos/some_repo.json | 1 + 9 files changed, 56 insertions(+), 6 deletions(-) diff --git a/docs/toml-schema.md b/docs/toml-schema.md index 161afb916..08aee2657 100644 --- a/docs/toml-schema.md +++ b/docs/toml-schema.md @@ -421,6 +421,12 @@ required-approvals = 1 # in [access.teams]. # (optional) allowed-merge-teams = ["awesome-team"] +# Enable bypass for organization administrators. +# Due to GitHub API limitations, individual users cannot be granted bypass +# access directly. Instead, this grants bypass to all organization admins. +# For granular control, add users to a team and use allowed-merge-teams. +# (optional) +allowed-merge-users = ["any-username"] # Enables OrganizationAdmin bypass # Determines the merge queue bot(s) that manage pushes to this branch. # When a bot manages the queue, some other options, like # `required-approvals` and `pr-required` options are not valid. diff --git a/rust_team_data/src/v1.rs b/rust_team_data/src/v1.rs index bc38e5710..56ae1b422 100644 --- a/rust_team_data/src/v1.rs +++ b/rust_team_data/src/v1.rs @@ -258,6 +258,8 @@ pub struct BranchProtection { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, + #[serde(default)] + pub allowed_merge_users: Vec, pub merge_bots: Vec, } diff --git a/src/schema.rs b/src/schema.rs index dfd48af02..06f24ec2e 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -877,6 +877,8 @@ pub(crate) struct BranchProtection { #[serde(default)] pub allowed_merge_teams: Vec, #[serde(default)] + pub allowed_merge_users: Vec, + #[serde(default)] pub merge_bots: Vec, } diff --git a/src/static_api.rs b/src/static_api.rs index 9c107670d..b18f78ea3 100644 --- a/src/static_api.rs +++ b/src/static_api.rs @@ -62,6 +62,7 @@ impl<'a> Generator<'a> { BranchProtectionMode::PrNotRequired }, allowed_merge_teams: b.allowed_merge_teams.clone(), + allowed_merge_users: b.allowed_merge_users.clone(), merge_bots: b .merge_bots .iter() diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index f1a85c88f..9908d1208 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -678,6 +678,7 @@ impl SyncGitHub { dismiss_stale_review: false, mode: rust_team_data::v1::BranchProtectionMode::PrNotRequired, allowed_merge_teams: vec![], + allowed_merge_users: vec![], merge_bots: vec![], }, }); @@ -715,8 +716,13 @@ impl SyncGitHub { actual_ruleset: &api::Ruleset, branch_protection: &rust_team_data::v1::BranchProtection, ) -> bool { - let expected_count = - branch_protection.allowed_merge_teams.len() + branch_protection.merge_bots.len(); + let expected_count = branch_protection.allowed_merge_teams.len() + + if branch_protection.allowed_merge_users.is_empty() { + 0 + } else { + 1 + } // OrganizationAdmin counts as 1 + + branch_protection.merge_bots.len(); let actual_count = actual_ruleset .bypass_actors @@ -949,13 +955,21 @@ pub(crate) fn convert_branch_pattern_to_ref_pattern(pattern: &str) -> String { /// Construct bypass actors from branch protection settings. /// -/// This function resolves team names and bot logins to their GitHub database IDs -/// and creates `RulesetBypassActor` entries that allow these actors to bypass -/// branch protection rules. +/// This function resolves team names, user logins, and bot logins to their GitHub +/// database IDs and creates `RulesetBypassActor` entries that allow these actors +/// to bypass branch protection rules. +/// +/// # Actor Types +/// - Teams: Use `Team` actor type with team database ID +/// - Users: Use `RepositoryRole` actor type with user database ID +/// - Bots: Use `Integration` actor type with bot database ID /// /// # Arguments /// * `org` - The organization name -/// * `branch_protection` - Branch protection configuration containing allowed teams and bots +/// * `branch_protection` - Branch protection configuration containing: +/// - `allowed_merge_teams`: Team names to resolve +/// - `allowed_merge_users`: User logins to resolve +/// - `merge_bots`: Bot types to resolve (Homu/RustTimer) /// * `github_write` - GitHub API client for resolving IDs /// /// # Returns @@ -994,6 +1008,24 @@ pub(crate) fn construct_bypass_actors( } } + // Note: GitHub API has limited support for individual user bypass actors. + // OrganizationAdmin with actor_id=1 grants bypass to all org admins. + // For individual users, they must be added to a team and use allowed_merge_teams. + if !branch_protection.allowed_merge_users.is_empty() { + debug!( + "Warning: Individual user bypass actors (allowed_merge_users) have limited API support. \ + Users: {:?}. Only organization admins can bypass. \ + For granular control, add users to a team and use allowed_merge_teams instead.", + branch_protection.allowed_merge_users + ); + // Add OrganizationAdmin bypass actor (applies to all org admins) + bypass_actors.push(api::RulesetBypassActor { + actor_id: Some(api::ActorId::Id(1)), + actor_type: api::RulesetActorType::OrganizationAdmin, + bypass_mode: Some(api::RulesetBypassMode::Always), + }); + } + for merge_bot in &branch_protection.merge_bots { let user_login = match merge_bot { MergeBot::Homu => "bors", diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index 36cdb83e7..f087c4506 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -448,6 +448,7 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, + allowed_merge_users, merge_bots, } = self; v1::BranchProtection { @@ -455,6 +456,7 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, + allowed_merge_users, merge_bots, } } @@ -465,6 +467,7 @@ impl BranchProtectionBuilder { mode, dismiss_stale_review: false, allowed_merge_teams: vec![], + allowed_merge_users: vec![], merge_bots: vec![], } } diff --git a/tests/static-api/_expected/v1/repos.json b/tests/static-api/_expected/v1/repos.json index ae7d5d782..2e92cd289 100644 --- a/tests/static-api/_expected/v1/repos.json +++ b/tests/static-api/_expected/v1/repos.json @@ -21,6 +21,7 @@ } }, "allowed_merge_teams": [], + "allowed_merge_users": [], "merge_bots": [] } ], @@ -62,6 +63,7 @@ "allowed_merge_teams": [ "foo" ], + "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/archived_repo.json b/tests/static-api/_expected/v1/repos/archived_repo.json index 6c00ae967..2402e2dfc 100644 --- a/tests/static-api/_expected/v1/repos/archived_repo.json +++ b/tests/static-api/_expected/v1/repos/archived_repo.json @@ -19,6 +19,7 @@ } }, "allowed_merge_teams": [], + "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/some_repo.json b/tests/static-api/_expected/v1/repos/some_repo.json index 504838674..d348a5659 100644 --- a/tests/static-api/_expected/v1/repos/some_repo.json +++ b/tests/static-api/_expected/v1/repos/some_repo.json @@ -30,6 +30,7 @@ "allowed_merge_teams": [ "foo" ], + "allowed_merge_users": [], "merge_bots": [] } ], From 023b6b571f29a83a6cbd2707f3431b2eb4fe245f Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Tue, 23 Dec 2025 00:01:38 +0530 Subject: [PATCH 3/4] feat: add logs and fix tests --- sync-team/src/github/mod.rs | 129 ++++++++++++++++------- sync-team/src/github/tests/test_utils.rs | 1 + 2 files changed, 89 insertions(+), 41 deletions(-) diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 9908d1208..a878f6e03 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -5,6 +5,7 @@ mod tests; use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole}; use crate::Config; use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings}; +use anyhow::Context; use log::debug; use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot}; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; @@ -981,6 +982,7 @@ pub(crate) fn construct_bypass_actors( ) -> anyhow::Result> { let mut bypass_actors = Vec::new(); + // Resolve team bypass actors for team_name in &branch_protection.allowed_merge_teams { match github_write.resolve_team_database_id(org, team_name) { Ok(Some(actor_id)) => { @@ -992,33 +994,31 @@ pub(crate) fn construct_bypass_actors( } Ok(None) => { debug!( - "Warning: Could not resolve database ID for team '{}/{}'. \ - The team may not exist in the organization.", + "Team '{}/{}' not found; skipping bypass actor. \ + Verify the team exists in the organization.", org, team_name ); } Err(e) => { - anyhow::bail!( - "Failed to resolve database ID for team '{}/{}': {}", - org, - team_name, - e - ); + return Err(e).context(format!( + "Failed to resolve team '{}/{}' for bypass actor", + org, team_name + )); } } } - // Note: GitHub API has limited support for individual user bypass actors. - // OrganizationAdmin with actor_id=1 grants bypass to all org admins. - // For individual users, they must be added to a team and use allowed_merge_teams. + // Add OrganizationAdmin bypass actor if allowed_merge_users is specified. + // Note: GitHub API limitation - cannot add individual users as bypass actors. + // OrganizationAdmin with actor_id=1 grants bypass to ALL organization admins. + // For granular per-user control, use allowed_merge_teams with a team containing specific users. if !branch_protection.allowed_merge_users.is_empty() { debug!( - "Warning: Individual user bypass actors (allowed_merge_users) have limited API support. \ - Users: {:?}. Only organization admins can bypass. \ - For granular control, add users to a team and use allowed_merge_teams instead.", + "Using OrganizationAdmin bypass actor for allowed_merge_users: {:?}. \ + Note: This grants bypass to ALL org admins, not just specified users. \ + For per-user control, use allowed_merge_teams instead.", branch_protection.allowed_merge_users ); - // Add OrganizationAdmin bypass actor (applies to all org admins) bypass_actors.push(api::RulesetBypassActor { actor_id: Some(api::ActorId::Id(1)), actor_type: api::RulesetActorType::OrganizationAdmin, @@ -1026,6 +1026,7 @@ pub(crate) fn construct_bypass_actors( }); } + // Resolve bot integration bypass actors for merge_bot in &branch_protection.merge_bots { let user_login = match merge_bot { MergeBot::Homu => "bors", @@ -1041,17 +1042,16 @@ pub(crate) fn construct_bypass_actors( } Ok(None) => { debug!( - "Warning: Could not resolve database ID for bot '{}'. \ - This bot may not be installed in the organization.", - user_login + "Bot '{}' not found in organization '{}'; skipping bypass actor. \ + Verify the bot is installed and has access.", + user_login, org ); } Err(e) => { - anyhow::bail!( - "Failed to resolve database ID for bot '{}': {}", - user_login, - e - ); + return Err(e).context(format!( + "Failed to resolve bot '{}' for bypass actor", + user_login + )); } } } @@ -1355,21 +1355,9 @@ impl std::fmt::Display for CreateRepoDiff { if !rulesets.is_empty() { writeln!(f, " Rulesets:")?; - for (ruleset, _branch_protection) in rulesets { - let id_str = ruleset.id.map_or("new".to_string(), |id| id.to_string()); - writeln!(f, " - {} (ID: {})", ruleset.name, id_str)?; - if let Some(conditions) = &ruleset.conditions - && let Some(ref_name) = &conditions.ref_name - { - writeln!(f, " Branches: {}", ref_name.include.join(", "))?; - } - if ruleset - .rules - .iter() - .any(|r| matches!(r, api::RulesetRule::MergeQueue { .. })) - { - writeln!(f, " Merge Queue: Enabled")?; - } + for (ruleset, branch_protection) in rulesets { + writeln!(f, " {}", ruleset.name)?; + log_ruleset(ruleset, None, Some(branch_protection), &mut f)?; } } @@ -1805,6 +1793,7 @@ fn log_branch_protection( fn log_ruleset( current: &api::Ruleset, new: Option<&api::Ruleset>, + branch_protection: Option<&rust_team_data::v1::BranchProtection>, mut result: impl Write, ) -> std::fmt::Result { macro_rules! log { @@ -1860,7 +1849,28 @@ fn log_ruleset( && !bypass_actors.is_empty() { let new_bypass = new.and_then(|n| n.bypass_actors.as_ref()); - log!("Bypass Actors", bypass_actors, new_bypass); + if new_bypass.is_some() { + log!("Bypass Actors", bypass_actors, new_bypass); + } else { + writeln!(result, " Bypass Actors:")?; + for actor in bypass_actors { + let actor_id_str = match &actor.actor_id { + Some(api::ActorId::Id(id)) => id.to_string(), + Some(api::ActorId::Null) => "null".to_string(), + None => "omitted".to_string(), + }; + let mode_str = actor.bypass_mode.as_ref().map_or("default", |m| match m { + api::RulesetBypassMode::Always => "always", + api::RulesetBypassMode::PullRequest => "pull_request", + api::RulesetBypassMode::Exempt => "exempt", + }); + writeln!( + result, + " - Type: {:?}, ID: {}, Mode: {}", + actor.actor_type, actor_id_str, mode_str + )?; + } + } } // Log individual rules @@ -1977,6 +1987,39 @@ fn log_ruleset( } } + // Log expected bypass actors if branch protection config is provided + if let Some(bp) = branch_protection + && (!bp.allowed_merge_teams.is_empty() + || !bp.allowed_merge_users.is_empty() + || !bp.merge_bots.is_empty()) + { + writeln!(result, " Expected Bypass Actors:")?; + + for team in &bp.allowed_merge_teams { + writeln!(result, " - Team: {} (Mode: always)", team)?; + } + + if !bp.allowed_merge_users.is_empty() { + writeln!( + result, + " - OrganizationAdmin: ID=1 (Mode: always) [Note: Grants bypass to all org admins, requested users: {}]", + bp.allowed_merge_users.join(", ") + )?; + } + + for bot in &bp.merge_bots { + let bot_name = match bot { + MergeBot::Homu => "bors", + MergeBot::RustTimer => "rust-timer", + }; + writeln!( + result, + " - Integration: {} (Mode: always)", + bot_name + )?; + } + } + Ok(()) } @@ -2051,8 +2094,12 @@ impl std::fmt::Display for RulesetDiff { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, " {}", self.name)?; match &self.operation { - RulesetDiffOperation::Create(ruleset) => log_ruleset(ruleset, None, f), - RulesetDiffOperation::Update(_, old, new) => log_ruleset(old, Some(new), f), + RulesetDiffOperation::Create(ruleset) => { + log_ruleset(ruleset, None, Some(&self.branch_protection), &mut *f) + } + RulesetDiffOperation::Update(_, old, new) => { + log_ruleset(old, Some(new), Some(&self.branch_protection), &mut *f) + } RulesetDiffOperation::Delete(_) => { writeln!(f, " Deleting ruleset") } diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index f087c4506..e13de38e0 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -424,6 +424,7 @@ pub struct BranchProtectionBuilder { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, + pub allowed_merge_users: Vec, pub merge_bots: Vec, } From e1c1ba6cffb49ed9182722203fcb7b33a1cad651 Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Tue, 23 Dec 2025 10:12:17 +0530 Subject: [PATCH 4/4] revert: allowed merged users --- docs/toml-schema.md | 6 --- rust_team_data/src/v1.rs | 2 - src/schema.rs | 2 - src/static_api.rs | 1 - sync-team/src/github/mod.rs | 40 ++----------------- sync-team/src/github/tests/test_utils.rs | 4 -- tests/static-api/_expected/v1/repos.json | 2 - .../_expected/v1/repos/archived_repo.json | 1 - .../_expected/v1/repos/some_repo.json | 1 - 9 files changed, 3 insertions(+), 56 deletions(-) diff --git a/docs/toml-schema.md b/docs/toml-schema.md index 08aee2657..161afb916 100644 --- a/docs/toml-schema.md +++ b/docs/toml-schema.md @@ -421,12 +421,6 @@ required-approvals = 1 # in [access.teams]. # (optional) allowed-merge-teams = ["awesome-team"] -# Enable bypass for organization administrators. -# Due to GitHub API limitations, individual users cannot be granted bypass -# access directly. Instead, this grants bypass to all organization admins. -# For granular control, add users to a team and use allowed-merge-teams. -# (optional) -allowed-merge-users = ["any-username"] # Enables OrganizationAdmin bypass # Determines the merge queue bot(s) that manage pushes to this branch. # When a bot manages the queue, some other options, like # `required-approvals` and `pr-required` options are not valid. diff --git a/rust_team_data/src/v1.rs b/rust_team_data/src/v1.rs index 56ae1b422..bc38e5710 100644 --- a/rust_team_data/src/v1.rs +++ b/rust_team_data/src/v1.rs @@ -258,8 +258,6 @@ pub struct BranchProtection { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, - #[serde(default)] - pub allowed_merge_users: Vec, pub merge_bots: Vec, } diff --git a/src/schema.rs b/src/schema.rs index 06f24ec2e..dfd48af02 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -877,8 +877,6 @@ pub(crate) struct BranchProtection { #[serde(default)] pub allowed_merge_teams: Vec, #[serde(default)] - pub allowed_merge_users: Vec, - #[serde(default)] pub merge_bots: Vec, } diff --git a/src/static_api.rs b/src/static_api.rs index b18f78ea3..9c107670d 100644 --- a/src/static_api.rs +++ b/src/static_api.rs @@ -62,7 +62,6 @@ impl<'a> Generator<'a> { BranchProtectionMode::PrNotRequired }, allowed_merge_teams: b.allowed_merge_teams.clone(), - allowed_merge_users: b.allowed_merge_users.clone(), merge_bots: b .merge_bots .iter() diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index a878f6e03..4f5249dea 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -679,7 +679,6 @@ impl SyncGitHub { dismiss_stale_review: false, mode: rust_team_data::v1::BranchProtectionMode::PrNotRequired, allowed_merge_teams: vec![], - allowed_merge_users: vec![], merge_bots: vec![], }, }); @@ -717,13 +716,8 @@ impl SyncGitHub { actual_ruleset: &api::Ruleset, branch_protection: &rust_team_data::v1::BranchProtection, ) -> bool { - let expected_count = branch_protection.allowed_merge_teams.len() - + if branch_protection.allowed_merge_users.is_empty() { - 0 - } else { - 1 - } // OrganizationAdmin counts as 1 - + branch_protection.merge_bots.len(); + let expected_count = + branch_protection.allowed_merge_teams.len() + branch_protection.merge_bots.len(); let actual_count = actual_ruleset .bypass_actors @@ -1008,24 +1002,6 @@ pub(crate) fn construct_bypass_actors( } } - // Add OrganizationAdmin bypass actor if allowed_merge_users is specified. - // Note: GitHub API limitation - cannot add individual users as bypass actors. - // OrganizationAdmin with actor_id=1 grants bypass to ALL organization admins. - // For granular per-user control, use allowed_merge_teams with a team containing specific users. - if !branch_protection.allowed_merge_users.is_empty() { - debug!( - "Using OrganizationAdmin bypass actor for allowed_merge_users: {:?}. \ - Note: This grants bypass to ALL org admins, not just specified users. \ - For per-user control, use allowed_merge_teams instead.", - branch_protection.allowed_merge_users - ); - bypass_actors.push(api::RulesetBypassActor { - actor_id: Some(api::ActorId::Id(1)), - actor_type: api::RulesetActorType::OrganizationAdmin, - bypass_mode: Some(api::RulesetBypassMode::Always), - }); - } - // Resolve bot integration bypass actors for merge_bot in &branch_protection.merge_bots { let user_login = match merge_bot { @@ -1989,9 +1965,7 @@ fn log_ruleset( // Log expected bypass actors if branch protection config is provided if let Some(bp) = branch_protection - && (!bp.allowed_merge_teams.is_empty() - || !bp.allowed_merge_users.is_empty() - || !bp.merge_bots.is_empty()) + && (!bp.allowed_merge_teams.is_empty() || !bp.merge_bots.is_empty()) { writeln!(result, " Expected Bypass Actors:")?; @@ -1999,14 +1973,6 @@ fn log_ruleset( writeln!(result, " - Team: {} (Mode: always)", team)?; } - if !bp.allowed_merge_users.is_empty() { - writeln!( - result, - " - OrganizationAdmin: ID=1 (Mode: always) [Note: Grants bypass to all org admins, requested users: {}]", - bp.allowed_merge_users.join(", ") - )?; - } - for bot in &bp.merge_bots { let bot_name = match bot { MergeBot::Homu => "bors", diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index e13de38e0..36cdb83e7 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -424,7 +424,6 @@ pub struct BranchProtectionBuilder { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, - pub allowed_merge_users: Vec, pub merge_bots: Vec, } @@ -449,7 +448,6 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, - allowed_merge_users, merge_bots, } = self; v1::BranchProtection { @@ -457,7 +455,6 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, - allowed_merge_users, merge_bots, } } @@ -468,7 +465,6 @@ impl BranchProtectionBuilder { mode, dismiss_stale_review: false, allowed_merge_teams: vec![], - allowed_merge_users: vec![], merge_bots: vec![], } } diff --git a/tests/static-api/_expected/v1/repos.json b/tests/static-api/_expected/v1/repos.json index 2e92cd289..ae7d5d782 100644 --- a/tests/static-api/_expected/v1/repos.json +++ b/tests/static-api/_expected/v1/repos.json @@ -21,7 +21,6 @@ } }, "allowed_merge_teams": [], - "allowed_merge_users": [], "merge_bots": [] } ], @@ -63,7 +62,6 @@ "allowed_merge_teams": [ "foo" ], - "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/archived_repo.json b/tests/static-api/_expected/v1/repos/archived_repo.json index 2402e2dfc..6c00ae967 100644 --- a/tests/static-api/_expected/v1/repos/archived_repo.json +++ b/tests/static-api/_expected/v1/repos/archived_repo.json @@ -19,7 +19,6 @@ } }, "allowed_merge_teams": [], - "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/some_repo.json b/tests/static-api/_expected/v1/repos/some_repo.json index d348a5659..504838674 100644 --- a/tests/static-api/_expected/v1/repos/some_repo.json +++ b/tests/static-api/_expected/v1/repos/some_repo.json @@ -30,7 +30,6 @@ "allowed_merge_teams": [ "foo" ], - "allowed_merge_users": [], "merge_bots": [] } ],