From 553785b7bcb82027ee56b675ff4fe0fc1108b289 Mon Sep 17 00:00:00 2001 From: Dion Gionet Mallet Date: Wed, 12 Oct 2022 13:40:42 -0400 Subject: [PATCH] feat(branch-protection): Push/Reviewer actors can be specified by name (#1020) * feat(branch-protection): Push/Reviewer actors can be specified by name feat: Added getNodeIDv4 feat(branch-protection): Added prefix requirement for teams/users The / prefix defines a user, the orgname/ prefix defines a team. (Ex.: testorg/team) * test(branch-protection): Added push restriction test with username * doc(branch-protection): Updated documentation to mention actor names * feat(branch-protection): updated to latest version * fix: env nil pointer dereference includes a nil check on branch protection resources * fix(branch-protection): added error handling Also removed loop labels --- github/resource_github_branch_protection.go | 53 ++++- .../resource_github_branch_protection_test.go | 54 +++++ .../resource_github_repository_environment.go | 8 +- github/util_v4_branch_protection.go | 219 ++++++++++++++---- .../docs/r/branch_protection.html.markdown | 10 +- 5 files changed, 295 insertions(+), 49 deletions(-) diff --git a/github/resource_github_branch_protection.go b/github/resource_github_branch_protection.go index 73162cdc65..4e9c48bae8 100644 --- a/github/resource_github_branch_protection.go +++ b/github/resource_github_branch_protection.go @@ -154,6 +154,27 @@ func resourceGithubBranchProtectionCreate(d *schema.ResourceData, meta interface if err != nil { return err } + + var reviewIds, pushIds, bypassIds []string + reviewIds, err = getActorIds(data.ReviewDismissalActorIDs, meta) + if err != nil { + return err + } + + pushIds, err = getActorIds(data.PushActorIDs, meta) + if err != nil { + return err + } + + bypassIds, err = getActorIds(data.BypassPullRequestActorIDs, meta) + if err != nil { + return err + } + + data.PushActorIDs = pushIds + data.ReviewDismissalActorIDs = reviewIds + data.BypassPullRequestActorIDs = bypassIds + input := githubv4.CreateBranchProtectionRuleInput{ AllowsDeletions: githubv4.NewBoolean(githubv4.Boolean(data.AllowsDeletions)), AllowsForcePushes: githubv4.NewBoolean(githubv4.Boolean(data.AllowsForcePushes)), @@ -255,7 +276,11 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_REQUIRES_CONVERSATION_RESOLUTION, protection.Repository.Name, protection.Pattern, d.Id()) } - approvingReviews := setApprovingReviews(protection) + approvingReviews, err := setApprovingReviews(protection, d, meta) + if err != nil { + return err + } + err = d.Set(PROTECTION_REQUIRES_APPROVING_REVIEWS, approvingReviews) if err != nil { log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_REQUIRES_APPROVING_REVIEWS, protection.Repository.Name, protection.Pattern, d.Id()) @@ -267,7 +292,10 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_REQUIRES_STATUS_CHECKS, protection.Repository.Name, protection.Pattern, d.Id()) } - restrictsPushes := setPushes(protection) + restrictsPushes, err := setPushes(protection, d, meta) + if err != nil { + return err + } err = d.Set(PROTECTION_RESTRICTS_PUSHES, restrictsPushes) if err != nil { log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_RESTRICTS_PUSHES, protection.Repository.Name, protection.Pattern, d.Id()) @@ -288,6 +316,27 @@ func resourceGithubBranchProtectionUpdate(d *schema.ResourceData, meta interface if err != nil { return err } + + var reviewIds, pushIds, bypassIds []string + reviewIds, err = getActorIds(data.ReviewDismissalActorIDs, meta) + if err != nil { + return err + } + + pushIds, err = getActorIds(data.PushActorIDs, meta) + if err != nil { + return err + } + + bypassIds, err = getActorIds(data.BypassPullRequestActorIDs, meta) + if err != nil { + return err + } + + data.PushActorIDs = pushIds + data.ReviewDismissalActorIDs = reviewIds + data.BypassPullRequestActorIDs = bypassIds + input := githubv4.UpdateBranchProtectionRuleInput{ BranchProtectionRuleID: d.Id(), AllowsDeletions: githubv4.NewBoolean(githubv4.Boolean(data.AllowsDeletions)), diff --git a/github/resource_github_branch_protection_test.go b/github/resource_github_branch_protection_test.go index 492a7d935e..f5e50e85a4 100644 --- a/github/resource_github_branch_protection_test.go +++ b/github/resource_github_branch_protection_test.go @@ -380,6 +380,60 @@ func TestAccGithubBranchProtection(t *testing.T) { }) + t.Run("configures branch push restrictions with username", func(t *testing.T) { + + user := fmt.Sprintf("/%s", testOwnerFunc()) + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-%s" + auto_init = true + } + + resource "github_branch_protection" "test" { + + repository_id = github_repository.test.name + pattern = "main" + + push_restrictions = [ + "%s", + ] + + } + `, randomID, user) + + check := resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr( + "github_branch_protection.test", "push_restrictions.#", "1", + ), + ) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + Check: check, + }, + }, + }) + } + + t.Run("with an anonymous account", func(t *testing.T) { + t.Skip("anonymous account not supported for this operation") + }) + + t.Run("with an individual account", func(t *testing.T) { + t.Skip("individual account not supported for this operation") + }) + + t.Run("with an organization account", func(t *testing.T) { + testCase(t, organization) + }) + + }) + t.Run("configures force pushes and deletions", func(t *testing.T) { config := fmt.Sprintf(` diff --git a/github/resource_github_repository_environment.go b/github/resource_github_repository_environment.go index 90e33a29f8..9c460efabf 100644 --- a/github/resource_github_repository_environment.go +++ b/github/resource_github_repository_environment.go @@ -134,9 +134,13 @@ func resourceGithubRepositoryEnvironmentRead(d *schema.ResourceData, meta interf for _, r := range pr.Reviewers { switch *r.Type { case "Team": - teams = append(teams, *r.Reviewer.(*github.Team).ID) + if r.Reviewer.(*github.Team).ID != nil { + teams = append(teams, *r.Reviewer.(*github.Team).ID) + } case "User": - users = append(users, *r.Reviewer.(*github.User).ID) + if r.Reviewer.(*github.User).ID != nil { + users = append(users, *r.Reviewer.(*github.User).ID) + } } } d.Set("reviewers", []interface{}{ diff --git a/github/util_v4_branch_protection.go b/github/util_v4_branch_protection.go index a06cd22d47..58f1600eff 100644 --- a/github/util_v4_branch_protection.go +++ b/github/util_v4_branch_protection.go @@ -3,6 +3,8 @@ package github import ( "context" "fmt" + "log" + "strings" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "github.com/shurcooL/githubv4" @@ -11,27 +13,34 @@ import ( type Actor struct { ID githubv4.ID Name githubv4.String + Slug githubv4.String +} + +type ActorUser struct { + ID githubv4.ID + Name githubv4.String + Login githubv4.String } type DismissalActorTypes struct { Actor struct { - Team Actor `graphql:"... on Team"` - User Actor `graphql:"... on User"` + Team Actor `graphql:"... on Team"` + User ActorUser `graphql:"... on User"` } } type BypassPullRequestActorTypes struct { Actor struct { - Team Actor `graphql:"... on Team"` - User Actor `graphql:"... on User"` + Team Actor `graphql:"... on Team"` + User ActorUser `graphql:"... on User"` } } type PushActorTypes struct { Actor struct { - App Actor `graphql:"... on App"` - Team Actor `graphql:"... on Team"` - User Actor `graphql:"... on User"` + App Actor `graphql:"... on App"` + Team Actor `graphql:"... on Team"` + User ActorUser `graphql:"... on User"` } } @@ -223,60 +232,116 @@ func branchProtectionResourceData(d *schema.ResourceData, meta interface{}) (Bra return data, nil } -func setDismissalActorIDs(actors []DismissalActorTypes) []string { - pushActors := make([]string, 0, len(actors)) +func setDismissalActorIDs(actors []DismissalActorTypes, d *schema.ResourceData, meta interface{}) ([]string, error) { + dismissalActors := make([]string, 0, len(actors)) + data, err := branchProtectionResourceData(d, meta) + if err != nil { + return []string{}, err + } + orgName := meta.(*Owner).name + for _, a := range actors { - if a.Actor.Team != (Actor{}) { - pushActors = append(pushActors, a.Actor.Team.ID.(string)) + IsID := false + for _, v := range data.ReviewDismissalActorIDs { + if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) { + dismissalActors = append(dismissalActors, v) + IsID = true + break + } } - if a.Actor.User != (Actor{}) { - pushActors = append(pushActors, a.Actor.User.ID.(string)) + if !IsID { + if a.Actor.Team.Slug != "" { + dismissalActors = append(dismissalActors, orgName+"/"+string(a.Actor.Team.Slug)) + continue + } + if a.Actor.User.Login != "" { + dismissalActors = append(dismissalActors, "/"+string(a.Actor.User.Login)) + } } } - - return pushActors + return dismissalActors, nil } -func setBypassPullRequestActorIDs(actors []BypassPullRequestActorTypes) []string { +func setBypassPullRequestActorIDs(actors []BypassPullRequestActorTypes, d *schema.ResourceData, meta interface{}) ([]string, error) { bypassActors := make([]string, 0, len(actors)) + data, err := branchProtectionResourceData(d, meta) + if err != nil { + return []string{}, err + } + orgName := meta.(*Owner).name + for _, a := range actors { - if a.Actor.Team != (Actor{}) { - bypassActors = append(bypassActors, a.Actor.Team.ID.(string)) + IsID := false + for _, v := range data.BypassPullRequestActorIDs { + if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) { + bypassActors = append(bypassActors, v) + IsID = true + break + } } - if a.Actor.User != (Actor{}) { - bypassActors = append(bypassActors, a.Actor.User.ID.(string)) + if !IsID { + if a.Actor.Team.Slug != "" { + bypassActors = append(bypassActors, orgName+"/"+string(a.Actor.Team.Slug)) + continue + } + if a.Actor.User.Login != "" { + bypassActors = append(bypassActors, "/"+string(a.Actor.User.Login)) + } } } - - return bypassActors + return bypassActors, nil } -func setPushActorIDs(actors []PushActorTypes) []string { +func setPushActorIDs(actors []PushActorTypes, d *schema.ResourceData, meta interface{}) ([]string, error) { pushActors := make([]string, 0, len(actors)) + data, err := branchProtectionResourceData(d, meta) + if err != nil { + return []string{}, err + } + orgName := meta.(*Owner).name + for _, a := range actors { - if a.Actor.Team != (Actor{}) { - pushActors = append(pushActors, a.Actor.Team.ID.(string)) - } - if a.Actor.User != (Actor{}) { - pushActors = append(pushActors, a.Actor.User.ID.(string)) + 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 a.Actor.App != (Actor{}) { - pushActors = append(pushActors, a.Actor.App.ID.(string)) + if !IsID { + if a.Actor.Team.Slug != "" { + pushActors = append(pushActors, orgName+"/"+string(a.Actor.Team.Slug)) + continue + } + if a.Actor.User.Login != "" { + pushActors = append(pushActors, "/"+string(a.Actor.User.Login)) + continue + } + if a.Actor.App != (Actor{}) { + pushActors = append(pushActors, a.Actor.App.ID.(string)) + } } } - - return pushActors + return pushActors, nil } -func setApprovingReviews(protection BranchProtectionRule) interface{} { +func setApprovingReviews(protection BranchProtectionRule, d *schema.ResourceData, meta interface{}) (interface{}, error) { if !protection.RequiresApprovingReviews { - return nil + return nil, nil } dismissalAllowances := protection.ReviewDismissalAllowances.Nodes - dismissalActors := setDismissalActorIDs(dismissalAllowances) + dismissalActors, err := setDismissalActorIDs(dismissalAllowances, d, meta) + if err != nil { + return nil, err + } bypassPullRequestAllowances := protection.BypassPullRequestAllowances.Nodes - bypassPullRequestActors := setBypassPullRequestActorIDs(bypassPullRequestAllowances) + bypassPullRequestActors, err := setBypassPullRequestActorIDs(bypassPullRequestAllowances, d, meta) + if err != nil { + return nil, err + } + approvalReviews := []interface{}{ map[string]interface{}{ PROTECTION_REQUIRED_APPROVING_REVIEW_COUNT: protection.RequiredApprovingReviewCount, @@ -288,7 +353,7 @@ func setApprovingReviews(protection BranchProtectionRule) interface{} { }, } - return approvalReviews + return approvalReviews, nil } func setStatusChecks(protection BranchProtectionRule) interface{} { @@ -306,14 +371,16 @@ func setStatusChecks(protection BranchProtectionRule) interface{} { return statusChecks } -func setPushes(protection BranchProtectionRule) []string { +func setPushes(protection BranchProtectionRule, d *schema.ResourceData, meta interface{}) ([]string, error) { if !protection.RestrictsPushes { - return nil + return nil, nil } pushAllowances := protection.PushAllowances.Nodes - pushActors := setPushActorIDs(pushAllowances) - - return pushActors + pushActors, err := setPushActorIDs(pushAllowances, d, meta) + if err != nil { + return nil, err + } + return pushActors, nil } func getBranchProtectionID(repoID githubv4.ID, pattern string, meta interface{}) (githubv4.ID, error) { @@ -366,3 +433,71 @@ func getBranchProtectionID(repoID githubv4.ID, pattern string, meta interface{}) return nil, fmt.Errorf("Could not find a branch protection rule with the pattern '%s'.", pattern) } + +func getActorIds(data []string, meta interface{}) ([]string, error) { + var actors []string + for _, v := range data { + id, err := getNodeIDv4(v, meta) + if err != nil { + return []string{}, err + } + log.Printf("[DEBUG] Retrieved node ID for user/team : %s - node ID : %s", v, id) + actors = append(actors, id) + } + + return actors, nil +} + +// Given a string that is either a username or team slug, return the +// node id of the user or team it is referring to. Team slugs must be provided +// with the organization name as prefix (Ex.: exampleorg/exampleteam). Usernames +// must be provided with the "/" prefix otherwise getNodeIDv4 assumes that +// the provided string is a node ID. +func getNodeIDv4(userOrSlug string, meta interface{}) (string, error) { + orgName := meta.(*Owner).name + ctx := context.Background() + client := meta.(*Owner).v4client + + if strings.HasPrefix(userOrSlug, orgName+"/") { + var queryTeam struct { + Organization struct { + Team struct { + ID string + } `graphql:"team(slug: $slug)"` + } `graphql:"organization(login: $organization)"` + } + teamName := strings.TrimPrefix(userOrSlug, orgName+"/") + variablesTeam := map[string]interface{}{ + "slug": githubv4.String(teamName), + "organization": githubv4.String(orgName), + } + + err := client.Query(ctx, &queryTeam, variablesTeam) + if err != nil { + return "", err + } + log.Printf("[DEBUG] Retrieved node ID for team %s. ID is %s", userOrSlug, queryTeam.Organization.Team.ID) + return queryTeam.Organization.Team.ID, nil + } else if strings.HasPrefix(userOrSlug, "/") { + // The "/" prefix indicates a username + var queryUser struct { + User struct { + ID string + } `graphql:"user(login: $user)"` + } + userName := strings.TrimPrefix(userOrSlug, "/") + variablesUser := map[string]interface{}{ + "user": githubv4.String(userName), + } + + err := client.Query(ctx, &queryUser, variablesUser) + if err != nil { + return "", err + } + log.Printf("[DEBUG] Retrieved node ID for user %s. ID is %s", userOrSlug, queryUser.User.ID) + return queryUser.User.ID, nil + } else { + // If userOrSlug does not contain the team or username prefix, assume it is a node ID + return userOrSlug, nil + } +} diff --git a/website/docs/r/branch_protection.html.markdown b/website/docs/r/branch_protection.html.markdown index a2b6d8dddf..c6310c0d58 100644 --- a/website/docs/r/branch_protection.html.markdown +++ b/website/docs/r/branch_protection.html.markdown @@ -38,11 +38,15 @@ resource "github_branch_protection" "example" { dismissal_restrictions = [ data.github_user.example.node_id, github_team.example.node_id, + "/exampleuser", + "exampleorganization/exampleteam", ] } push_restrictions = [ data.github_user.example.node_id, + "/exampleuser", + "exampleorganization/exampleteam", # limited to a list of one type of restriction (user, team, app) # github_team.example.node_id ] @@ -80,7 +84,7 @@ The following arguments are supported: * `require_conversation_resolution` - (Optional) Boolean, setting this to `true` requires all conversations on code must be resolved before a pull request can be merged. * `required_status_checks` - (Optional) Enforce restrictions for required status checks. See [Required Status Checks](#required-status-checks) below for details. * `required_pull_request_reviews` - (Optional) Enforce restrictions for pull request reviews. See [Required Pull Request Reviews](#required-pull-request-reviews) below for details. -* `push_restrictions` - (Optional) The list of actor IDs that may push to the branch. +* `push_restrictions` - (Optional) The list of actor Names/IDs that may push to the branch. Actor names must either begin with a "/" for users or the organization name followed by a "/" for teams. * `allows_deletions` - (Optional) Boolean, setting this to `true` to allow the branch to be deleted. * `allows_force_pushes` - (Optional) Boolean, setting this to `true` to allow force pushes on the branch. * `blocks_creations` - (Optional) Boolean, setting this to `true` to block creating the branch. @@ -98,8 +102,8 @@ The following arguments are supported: * `dismiss_stale_reviews`: (Optional) Dismiss approved reviews automatically when a new commit is pushed. Defaults to `false`. * `restrict_dismissals`: (Optional) Restrict pull request review dismissals. -* `dismissal_restrictions`: (Optional) The list of actor IDs with dismissal access. If not empty, `restrict_dismissals` is ignored. -* `pull_request_bypassers`: (Optional) The list of actor IDs that are allowed to bypass pull request requirements. +* `dismissal_restrictions`: (Optional) The list of actor Names/IDs with dismissal access. If not empty, `restrict_dismissals` is ignored. Actor names must either begin with a "/" for users or the organization name followed by a "/" for teams. +* `pull_request_bypassers`: (Optional) The list of actor Names/IDs that are allowed to bypass pull request requirements. Actor names must either begin with a "/" for users or the organization name followed by a "/" for teams. * `require_code_owner_reviews`: (Optional) Require an approved review in pull requests including files with a designated code owner. Defaults to `false`. * `required_approving_review_count`: (Optional) Require x number of approvals to satisfy branch protection requirements. If this is specified it must be a number between 0-6. This requirement matches GitHub's API, see the upstream [documentation](https://developer.github.com/v3/repos/branches/#parameters-1) for more information.