From 9b3d96c92d8cc0a28496e94933fc00021be0620b Mon Sep 17 00:00:00 2001 From: RoseSecurity Date: Sun, 26 Jan 2025 00:21:31 -0500 Subject: [PATCH 1/4] fix: sort responses for push actor IDs --- github/util_v4_branch_protection.go | 39 +++++++++++++++++------------ 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/github/util_v4_branch_protection.go b/github/util_v4_branch_protection.go index eae3467cda..a5245c65b3 100644 --- a/github/util_v4_branch_protection.go +++ b/github/util_v4_branch_protection.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "sort" "strings" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" @@ -258,6 +259,7 @@ func branchProtectionResourceData(d *schema.ResourceData, meta interface{}) (Bra pushActorIDs = append(pushActorIDs, v.(string)) } if len(pushActorIDs) > 0 { + sort.Strings(pushActorIDs) data.PushActorIDs = pushActorIDs } } @@ -460,32 +462,37 @@ func setBypassPullRequestActorIDs(actors []BypassPullRequestActorTypes, data Bra func setPushActorIDs(actors []PushActorTypes, data BranchProtectionResourceData, meta interface{}) []string { pushActors := make([]string, 0, len(actors)) - orgName := meta.(*Owner).name + idMap := make(map[string]bool) + for _, v := range data.PushActorIDs { + idMap[v] = true + } + for _, a := range actors { - IsID := false - for _, v := range data.PushActorIDs { - if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) || (a.Actor.App.ID != nil && a.Actor.App.ID.(string) == v) { - pushActors = append(pushActors, v) - IsID = true - break - } - } - if !IsID { + // Check for raw IDs first + if a.Actor.Team.ID != nil && idMap[a.Actor.Team.ID.(string)] { + pushActors = append(pushActors, a.Actor.Team.ID.(string)) + } else if a.Actor.User.ID != nil && idMap[a.Actor.User.ID.(string)] { + pushActors = append(pushActors, a.Actor.User.ID.(string)) + } else if a.Actor.App.ID != nil && idMap[a.Actor.App.ID.(string)] { + pushActors = append(pushActors, a.Actor.App.ID.(string)) + } else { + // Fall back to formatted strings only if no ID match if a.Actor.Team.Slug != "" { pushActors = append(pushActors, orgName+"/"+string(a.Actor.Team.Slug)) - continue - } - if a.Actor.User.Login != "" { + } else if a.Actor.User.Login != "" { pushActors = append(pushActors, "/"+string(a.Actor.User.Login)) - continue - } - if a.Actor.App != (Actor{}) { + } else if a.Actor.App != (Actor{}) { pushActors = append(pushActors, a.Actor.App.ID.(string)) } } } + + // Sort for consistent ordering + // This is important for preventing unnecessary drift in the Terraform state + sort.Strings(pushActors) + log.Printf("[DEBUG] Final sorted pushActors: %v", pushActors) return pushActors } From d2b28577d4778a28b84cc86117f54c65525e2c41 Mon Sep 17 00:00:00 2001 From: RoseSecurity Date: Mon, 27 Jan 2025 09:23:35 -0500 Subject: [PATCH 2/4] feat: update get actors function --- github/util_v4_branch_protection.go | 97 ++++++++++++++++++++++------- 1 file changed, 76 insertions(+), 21 deletions(-) diff --git a/github/util_v4_branch_protection.go b/github/util_v4_branch_protection.go index a5245c65b3..5c21861e43 100644 --- a/github/util_v4_branch_protection.go +++ b/github/util_v4_branch_protection.go @@ -464,36 +464,67 @@ func setPushActorIDs(actors []PushActorTypes, data BranchProtectionResourceData, pushActors := make([]string, 0, len(actors)) orgName := meta.(*Owner).name - idMap := make(map[string]bool) - for _, v := range data.PushActorIDs { - idMap[v] = true + // Create a map to track seen IDs to prevent duplicates + seenIDs := make(map[string]struct{}) + + for _, a := range actors { + var id string + if a.Actor.Team.ID != nil { + id = a.Actor.Team.ID.(string) + } else if a.Actor.User.ID != nil { + id = a.Actor.User.ID.(string) + } else if a.Actor.App.ID != nil { + id = a.Actor.App.ID.(string) + } + + if id != "" { + if _, exists := seenIDs[id]; !exists { + pushActors = append(pushActors, id) + seenIDs[id] = struct{}{} + } + } } for _, a := range actors { - // Check for raw IDs first - if a.Actor.Team.ID != nil && idMap[a.Actor.Team.ID.(string)] { - pushActors = append(pushActors, a.Actor.Team.ID.(string)) - } else if a.Actor.User.ID != nil && idMap[a.Actor.User.ID.(string)] { - pushActors = append(pushActors, a.Actor.User.ID.(string)) - } else if a.Actor.App.ID != nil && idMap[a.Actor.App.ID.(string)] { - pushActors = append(pushActors, a.Actor.App.ID.(string)) - } else { - // Fall back to formatted strings only if no ID match + if a.Actor.Team.ID == nil && a.Actor.User.ID == nil && a.Actor.App.ID == nil { + var formattedID string if a.Actor.Team.Slug != "" { - pushActors = append(pushActors, orgName+"/"+string(a.Actor.Team.Slug)) + formattedID = orgName + "/" + string(a.Actor.Team.Slug) } else if a.Actor.User.Login != "" { - pushActors = append(pushActors, "/"+string(a.Actor.User.Login)) + formattedID = "/" + string(a.Actor.User.Login) } else if a.Actor.App != (Actor{}) { - pushActors = append(pushActors, a.Actor.App.ID.(string)) + continue + } + + if formattedID != "" { + if _, exists := seenIDs[formattedID]; !exists { + pushActors = append(pushActors, formattedID) + seenIDs[formattedID] = struct{}{} + } } } } // Sort for consistent ordering - // This is important for preventing unnecessary drift in the Terraform state sort.Strings(pushActors) - log.Printf("[DEBUG] Final sorted pushActors: %v", pushActors) - return pushActors + + // Validate against provided IDs + idMap := make(map[string]bool) + for _, v := range data.PushActorIDs { + idMap[v] = true + } + + // Only keep IDs that were in the original PushActorIDs + validPushActors := make([]string, 0, len(pushActors)) + for _, actor := range pushActors { + if idMap[actor] { + validPushActors = append(validPushActors, actor) + } + } + + sort.Strings(validPushActors) + log.Printf("[DEBUG] Final sorted and validated pushActors: %v", validPushActors) + return validPushActors } func setApprovingReviews(protection BranchProtectionRule, data BranchProtectionResourceData, meta interface{}) interface{} { @@ -545,13 +576,18 @@ func setPushes(protection BranchProtectionRule, data BranchProtectionResourceDat pushAllowances := protection.PushAllowances.Nodes pushActors := setPushActorIDs(pushAllowances, data, meta) + // If we have no push actors but restrictions are enabled, return an empty list + // rather than nil to prevent drift + if len(pushActors) == 0 && protection.RestrictsPushes { + pushActors = make([]string, 0) + } + restrictsPushes := []interface{}{ map[string]interface{}{ PROTECTION_BLOCKS_CREATIONS: protection.BlocksCreations, PROTECTION_PUSH_ALLOWANCES: pushActors, }, } - return restrictsPushes } @@ -618,15 +654,34 @@ func getBranchProtectionID(repoID githubv4.ID, pattern string, meta interface{}) func getActorIds(data []string, meta interface{}) ([]string, error) { var actors []string + log.Printf("[DEBUG] getActorIds input data: %v", data) + + // Create a map to track processed IDs and prevent duplicates + seen := make(map[string]bool) + for _, v := range data { + if v == "" { + continue + } + id, err := getNodeIDv4(v, meta) if err != nil { + log.Printf("[DEBUG] Error getting node ID for %s: %v", v, err) return []string{}, err } - log.Printf("[DEBUG] Retrieved node ID for user/team : %s - node ID : %s", v, id) - actors = append(actors, id) + + log.Printf("[DEBUG] Retrieved node ID for user/team: %s - node ID: %s", v, id) + + if !seen[id] { + actors = append(actors, id) + seen[id] = true + } else { + log.Printf("[DEBUG] Skipping duplicate ID: %s", id) + } } + sort.Strings(actors) + log.Printf("[DEBUG] Final sorted actor IDs: %v", actors) return actors, nil } From 0a15a364b6e32a941893087f3208928c89c903f9 Mon Sep 17 00:00:00 2001 From: RoseSecurity Date: Mon, 27 Jan 2025 12:01:47 -0500 Subject: [PATCH 3/4] feat: add more debugging --- github/util_v4_branch_protection.go | 1 + 1 file changed, 1 insertion(+) diff --git a/github/util_v4_branch_protection.go b/github/util_v4_branch_protection.go index 5c21861e43..e95dde98f1 100644 --- a/github/util_v4_branch_protection.go +++ b/github/util_v4_branch_protection.go @@ -579,6 +579,7 @@ func setPushes(protection BranchProtectionRule, data BranchProtectionResourceDat // If we have no push actors but restrictions are enabled, return an empty list // rather than nil to prevent drift if len(pushActors) == 0 && protection.RestrictsPushes { + log.Printf("[DEBUG] No push actors found, returning empty list") pushActors = make([]string, 0) } From a52abfa1c4c21be2443032bdee30c50b0d25c3ab Mon Sep 17 00:00:00 2001 From: Joseph LaFreniere Date: Mon, 17 Feb 2025 19:46:59 -0500 Subject: [PATCH 4/4] feat: Reduce number of iterations over `actors` --- github/util_v4_branch_protection.go | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/github/util_v4_branch_protection.go b/github/util_v4_branch_protection.go index e95dde98f1..ca16953709 100644 --- a/github/util_v4_branch_protection.go +++ b/github/util_v4_branch_protection.go @@ -475,6 +475,10 @@ func setPushActorIDs(actors []PushActorTypes, data BranchProtectionResourceData, id = a.Actor.User.ID.(string) } else if a.Actor.App.ID != nil { id = a.Actor.App.ID.(string) + } else if a.Actor.Team.Slug != "" { + formattedID = orgName + "/" + string(a.Actor.Team.Slug) + } else if a.Actor.User.Login != "" { + formattedID = "/" + string(a.Actor.User.Login) } if id != "" { @@ -485,26 +489,6 @@ func setPushActorIDs(actors []PushActorTypes, data BranchProtectionResourceData, } } - for _, a := range actors { - if a.Actor.Team.ID == nil && a.Actor.User.ID == nil && a.Actor.App.ID == nil { - var formattedID string - if a.Actor.Team.Slug != "" { - formattedID = orgName + "/" + string(a.Actor.Team.Slug) - } else if a.Actor.User.Login != "" { - formattedID = "/" + string(a.Actor.User.Login) - } else if a.Actor.App != (Actor{}) { - continue - } - - if formattedID != "" { - if _, exists := seenIDs[formattedID]; !exists { - pushActors = append(pushActors, formattedID) - seenIDs[formattedID] = struct{}{} - } - } - } - } - // Sort for consistent ordering sort.Strings(pushActors)