From 7662b02f3d887c8adb99cdf1dac6e9212f93a7d3 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Fri, 3 Aug 2018 13:11:12 +0100 Subject: [PATCH] resource/github_*: Prevent crashing on invalid ID format --- github/resource_github_branch_protection.go | 17 +++++-- .../resource_github_branch_protection_test.go | 11 ++++- github/resource_github_issue_label.go | 11 ++++- github/resource_github_issue_label_test.go | 11 ++++- github/resource_github_membership.go | 18 ++------ github/resource_github_membership_test.go | 15 ++++-- ...resource_github_repository_collaborator.go | 5 +- ...rce_github_repository_collaborator_test.go | 16 +++++-- .../resource_github_repository_deploy_key.go | 10 +++- ...ource_github_repository_deploy_key_test.go | 12 ++++- github/resource_github_team_membership.go | 5 +- .../resource_github_team_membership_test.go | 16 +++++-- github/resource_github_team_repository.go | 5 +- .../resource_github_team_repository_test.go | 10 +++- github/util.go | 18 ++------ github/util_test.go | 46 ++----------------- 16 files changed, 129 insertions(+), 97 deletions(-) diff --git a/github/resource_github_branch_protection.go b/github/resource_github_branch_protection.go index 4d05f4f74d..d41ec49ce0 100644 --- a/github/resource_github_branch_protection.go +++ b/github/resource_github_branch_protection.go @@ -147,7 +147,10 @@ func resourceGithubBranchProtectionCreate(d *schema.ResourceData, meta interface func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - r, b := parseTwoPartID(d.Id()) + r, b, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } githubProtection, _, err := client.Repositories.GetBranchProtection(context.TODO(), meta.(*Organization).name, r, b) if err != nil { @@ -180,7 +183,10 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} func resourceGithubBranchProtectionUpdate(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - r, b := parseTwoPartID(d.Id()) + r, b, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } protectionRequest, err := buildProtectionRequest(d) if err != nil { @@ -206,9 +212,12 @@ func resourceGithubBranchProtectionUpdate(d *schema.ResourceData, meta interface func resourceGithubBranchProtectionDelete(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - r, b := parseTwoPartID(d.Id()) + r, b, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } - _, err := client.Repositories.RemoveBranchProtection(context.TODO(), meta.(*Organization).name, r, b) + _, err = client.Repositories.RemoveBranchProtection(context.TODO(), meta.(*Organization).name, r, b) return err } diff --git a/github/resource_github_branch_protection_test.go b/github/resource_github_branch_protection_test.go index 08feebdf17..add94f38db 100644 --- a/github/resource_github_branch_protection_test.go +++ b/github/resource_github_branch_protection_test.go @@ -123,7 +123,10 @@ func testAccCheckGithubProtectedBranchExists(n, id string, protection *github.Pr conn := testAccProvider.Meta().(*Organization).client o := testAccProvider.Meta().(*Organization).name - r, b := parseTwoPartID(rs.Primary.ID) + r, b, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } githubProtection, _, err := conn.Repositories.GetBranchProtection(context.TODO(), o, r, b) if err != nil { @@ -246,7 +249,11 @@ func testAccGithubBranchProtectionDestroy(s *terraform.State) error { } o := testAccProvider.Meta().(*Organization).name - r, b := parseTwoPartID(rs.Primary.ID) + r, b, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } + protection, res, err := conn.Repositories.GetBranchProtection(context.TODO(), o, r, b) if err == nil { diff --git a/github/resource_github_issue_label.go b/github/resource_github_issue_label.go index 51f094837a..096367bf3a 100644 --- a/github/resource_github_issue_label.go +++ b/github/resource_github_issue_label.go @@ -74,7 +74,11 @@ func resourceGithubIssueLabelCreateOrUpdate(d *schema.ResourceData, meta interfa if d.Id() == "" { oname = n } else { - _, oname = parseTwoPartID(d.Id()) + var err error + _, oname, err = parseTwoPartID(d.Id()) + if err != nil { + return err + } } _, _, err := client.Issues.EditLabel(context.TODO(), o, r, oname, label) @@ -99,7 +103,10 @@ func resourceGithubIssueLabelCreateOrUpdate(d *schema.ResourceData, meta interfa func resourceGithubIssueLabelRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - r, n := parseTwoPartID(d.Id()) + r, n, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } log.Printf("[DEBUG] Reading label: %s/%s", r, n) githubLabel, _, err := client.Issues.GetLabel(context.TODO(), meta.(*Organization).name, r, n) diff --git a/github/resource_github_issue_label_test.go b/github/resource_github_issue_label_test.go index a73a209e1b..78ccff3a73 100644 --- a/github/resource_github_issue_label_test.go +++ b/github/resource_github_issue_label_test.go @@ -96,7 +96,10 @@ func testAccCheckGithubIssueLabelExists(n string, label *github.Label) resource. conn := testAccProvider.Meta().(*Organization).client o := testAccProvider.Meta().(*Organization).name - r, n := parseTwoPartID(rs.Primary.ID) + r, n, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } githubLabel, _, err := conn.Issues.GetLabel(context.TODO(), o, r, n) if err != nil { @@ -131,7 +134,11 @@ func testAccGithubIssueLabelDestroy(s *terraform.State) error { } o := testAccProvider.Meta().(*Organization).name - r, n := parseTwoPartID(rs.Primary.ID) + r, n, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } + label, res, err := conn.Issues.GetLabel(context.TODO(), o, r, n) if err == nil { diff --git a/github/resource_github_membership.go b/github/resource_github_membership.go index e28d833553..3f8672413b 100644 --- a/github/resource_github_membership.go +++ b/github/resource_github_membership.go @@ -15,7 +15,7 @@ func resourceGithubMembership() *schema.Resource { Update: resourceGithubMembershipUpdate, Delete: resourceGithubMembershipDelete, Importer: &schema.ResourceImporter{ - State: resourceGithubMembershipImport, + State: schema.ImportStatePassthrough, }, Schema: map[string]*schema.Schema{ @@ -52,7 +52,10 @@ func resourceGithubMembershipCreate(d *schema.ResourceData, meta interface{}) er func resourceGithubMembershipRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - _, n := parseTwoPartID(d.Id()) + _, n, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } membership, _, err := client.Organizations.GetOrgMembership(context.TODO(), n, meta.(*Organization).name) if err != nil { @@ -89,14 +92,3 @@ func resourceGithubMembershipDelete(d *schema.ResourceData, meta interface{}) er return err } - -func resourceGithubMembershipImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { - // All we do here is validate that the import string is in a correct enough - // format to be parsed. parseTwoPartID will panic if it's missing elements, - // and is used otherwise in places where that should never happen, so we want - // to keep it that way. - if err := validateTwoPartID(d.Id()); err != nil { - return nil, err - } - return []*schema.ResourceData{d}, nil -} diff --git a/github/resource_github_membership_test.go b/github/resource_github_membership_test.go index a2b98d3d76..e7dd06e9c9 100644 --- a/github/resource_github_membership_test.go +++ b/github/resource_github_membership_test.go @@ -54,7 +54,10 @@ func testAccCheckGithubMembershipDestroy(s *terraform.State) error { if rs.Type != "github_membership" { continue } - o, u := parseTwoPartID(rs.Primary.ID) + o, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } membership, resp, err := conn.Organizations.GetOrgMembership(context.TODO(), u, o) @@ -84,7 +87,10 @@ func testAccCheckGithubMembershipExists(n string, membership *github.Membership) } conn := testAccProvider.Meta().(*Organization).client - o, u := parseTwoPartID(rs.Primary.ID) + o, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } githubMembership, _, err := conn.Organizations.GetOrgMembership(context.TODO(), u, o) if err != nil { @@ -107,7 +113,10 @@ func testAccCheckGithubMembershipRoleState(n string, membership *github.Membersh } conn := testAccProvider.Meta().(*Organization).client - o, u := parseTwoPartID(rs.Primary.ID) + o, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } githubMembership, _, err := conn.Organizations.GetOrgMembership(context.TODO(), u, o) if err != nil { diff --git a/github/resource_github_repository_collaborator.go b/github/resource_github_repository_collaborator.go index 1645399bd7..9b99a09456 100644 --- a/github/resource_github_repository_collaborator.go +++ b/github/resource_github_repository_collaborator.go @@ -59,7 +59,10 @@ func resourceGithubRepositoryCollaboratorCreate(d *schema.ResourceData, meta int func resourceGithubRepositoryCollaboratorRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - r, u := parseTwoPartID(d.Id()) + r, u, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } // First, check if the user has been invited but has not yet accepted invitation, err := findRepoInvitation(client, meta.(*Organization).name, r, u) diff --git a/github/resource_github_repository_collaborator_test.go b/github/resource_github_repository_collaborator_test.go index c842bfe44f..4372eebc34 100644 --- a/github/resource_github_repository_collaborator_test.go +++ b/github/resource_github_repository_collaborator_test.go @@ -60,7 +60,11 @@ func testAccCheckGithubRepositoryCollaboratorDestroy(s *terraform.State) error { } o := testAccProvider.Meta().(*Organization).name - r, u := parseTwoPartID(rs.Primary.ID) + r, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } + isCollaborator, _, err := conn.Repositories.IsCollaborator(context.TODO(), o, r, u) if err != nil { @@ -90,7 +94,10 @@ func testAccCheckGithubRepositoryCollaboratorExists(n string) resource.TestCheck conn := testAccProvider.Meta().(*Organization).client o := testAccProvider.Meta().(*Organization).name - r, u := parseTwoPartID(rs.Primary.ID) + r, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } invitations, _, err := conn.Repositories.ListInvitations(context.TODO(), o, r, nil) if err != nil { @@ -126,7 +133,10 @@ func testAccCheckGithubRepositoryCollaboratorPermission(n string) resource.TestC conn := testAccProvider.Meta().(*Organization).client o := testAccProvider.Meta().(*Organization).name - r, u := parseTwoPartID(rs.Primary.ID) + r, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } invitations, _, err := conn.Repositories.ListInvitations(context.TODO(), o, r, nil) if err != nil { diff --git a/github/resource_github_repository_deploy_key.go b/github/resource_github_repository_deploy_key.go index 8fd832eccb..082e92b814 100644 --- a/github/resource_github_repository_deploy_key.go +++ b/github/resource_github_repository_deploy_key.go @@ -77,7 +77,10 @@ func resourceGithubRepositoryDeployKeyRead(d *schema.ResourceData, meta interfac client := meta.(*Organization).client owner := meta.(*Organization).name - repo, id := parseTwoPartID(d.Id()) + repo, id, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } i, err := strconv.ParseInt(id, 10, 64) if err != nil { @@ -101,7 +104,10 @@ func resourceGithubRepositoryDeployKeyDelete(d *schema.ResourceData, meta interf client := meta.(*Organization).client owner := meta.(*Organization).name - repo, id := parseTwoPartID(d.Id()) + repo, id, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } i, err := strconv.ParseInt(id, 10, 64) if err != nil { diff --git a/github/resource_github_repository_deploy_key_test.go b/github/resource_github_repository_deploy_key_test.go index 06babe4fe3..d187f1393a 100644 --- a/github/resource_github_repository_deploy_key_test.go +++ b/github/resource_github_repository_deploy_key_test.go @@ -62,7 +62,11 @@ func testAccCheckGithubRepositoryDeployKeyDestroy(s *terraform.State) error { } o := testAccProvider.Meta().(*Organization).name - r, i := parseTwoPartID(rs.Primary.ID) + r, i, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } + id, err := strconv.ParseInt(i, 10, 64) if err != nil { return err @@ -92,7 +96,11 @@ func testAccCheckGithubRepositoryDeployKeyExists(n string) resource.TestCheckFun conn := testAccProvider.Meta().(*Organization).client o := testAccProvider.Meta().(*Organization).name - r, i := parseTwoPartID(rs.Primary.ID) + r, i, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } + id, err := strconv.ParseInt(i, 10, 64) if err != nil { return err diff --git a/github/resource_github_team_membership.go b/github/resource_github_team_membership.go index ca54f1e95e..27a5a80b76 100644 --- a/github/resource_github_team_membership.go +++ b/github/resource_github_team_membership.go @@ -61,7 +61,10 @@ func resourceGithubTeamMembershipCreate(d *schema.ResourceData, meta interface{} func resourceGithubTeamMembershipRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - t, n := parseTwoPartID(d.Id()) + t, n, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } membership, _, err := client.Organizations.GetTeamMembership(context.TODO(), toGithubID(t), n) diff --git a/github/resource_github_team_membership_test.go b/github/resource_github_team_membership_test.go index 654793de11..7c48850b66 100644 --- a/github/resource_github_team_membership_test.go +++ b/github/resource_github_team_membership_test.go @@ -84,7 +84,11 @@ func testAccCheckGithubTeamMembershipDestroy(s *terraform.State) error { continue } - t, u := parseTwoPartID(rs.Primary.ID) + t, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } + membership, resp, err := conn.Organizations.GetTeamMembership(context.TODO(), toGithubID(t), u) if err == nil { if membership != nil { @@ -111,7 +115,10 @@ func testAccCheckGithubTeamMembershipExists(n string, membership *github.Members } conn := testAccProvider.Meta().(*Organization).client - t, u := parseTwoPartID(rs.Primary.ID) + t, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } teamMembership, _, err := conn.Organizations.GetTeamMembership(context.TODO(), toGithubID(t), u) @@ -135,7 +142,10 @@ func testAccCheckGithubTeamMembershipRoleState(n, expected string, membership *g } conn := testAccProvider.Meta().(*Organization).client - t, u := parseTwoPartID(rs.Primary.ID) + t, u, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } teamMembership, _, err := conn.Organizations.GetTeamMembership(context.TODO(), toGithubID(t), u) if err != nil { diff --git a/github/resource_github_team_repository.go b/github/resource_github_team_repository.go index 7a13cef1be..e3102f6461 100644 --- a/github/resource_github_team_repository.go +++ b/github/resource_github_team_repository.go @@ -58,7 +58,10 @@ func resourceGithubTeamRepositoryCreate(d *schema.ResourceData, meta interface{} func resourceGithubTeamRepositoryRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*Organization).client - t, r := parseTwoPartID(d.Id()) + t, r, err := parseTwoPartID(d.Id()) + if err != nil { + return err + } repo, _, repoErr := client.Organizations.IsTeamRepo(context.TODO(), toGithubID(t), meta.(*Organization).name, r) diff --git a/github/resource_github_team_repository_test.go b/github/resource_github_team_repository_test.go index a3721c8197..f805a8f29c 100644 --- a/github/resource_github_team_repository_test.go +++ b/github/resource_github_team_repository_test.go @@ -114,7 +114,10 @@ func testAccCheckGithubTeamRepositoryExists(n string, repository *github.Reposit } conn := testAccProvider.Meta().(*Organization).client - t, r := parseTwoPartID(rs.Primary.ID) + t, r, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } repo, _, err := conn.Organizations.IsTeamRepo(context.TODO(), toGithubID(t), @@ -135,7 +138,10 @@ func testAccCheckGithubTeamRepositoryDestroy(s *terraform.State) error { if rs.Type != "github_team_repository" { continue } - t, r := parseTwoPartID(rs.Primary.ID) + t, r, err := parseTwoPartID(rs.Primary.ID) + if err != nil { + return err + } repo, resp, err := conn.Organizations.IsTeamRepo(context.TODO(), toGithubID(t), diff --git a/github/util.go b/github/util.go index 8e9085e884..26a1d14fc5 100644 --- a/github/util.go +++ b/github/util.go @@ -1,7 +1,6 @@ package github import ( - "errors" "fmt" "strconv" "strings" @@ -42,22 +41,13 @@ func validateValueFunc(values []string) schema.SchemaValidateFunc { } // return the pieces of id `a:b` as a, b -func parseTwoPartID(id string) (string, string) { +func parseTwoPartID(id string) (string, string, error) { parts := strings.SplitN(id, ":", 2) - return parts[0], parts[1] -} - -// validateTwoPartID performs a quick validation of a two-part ID, designed for -// use when validation has not been previously possible, such as importing. -func validateTwoPartID(id string) error { - if id == "" { - return errors.New("no ID supplied. Please supply an ID format matching organization:username") - } - parts := strings.Split(id, ":") if len(parts) != 2 { - return fmt.Errorf("incorrectly formatted ID %q. Please supply an ID format matching organization:username", id) + return "", "", fmt.Errorf("Unexpected ID format (%q). Expected organization:name", id) } - return nil + + return parts[0], parts[1], nil } // format the strings into an id `a:b` diff --git a/github/util_test.go b/github/util_test.go index fd4f4a42b2..00b29900d7 100644 --- a/github/util_test.go +++ b/github/util_test.go @@ -43,7 +43,10 @@ func TestAccGithubUtilTwoPartID(t *testing.T) { t.Fatalf("Expected two part id to be foo:bar, actual: %s", id) } - parsedPartOne, parsedPartTwo := parseTwoPartID(id) + parsedPartOne, parsedPartTwo, err := parseTwoPartID(id) + if err != nil { + t.Fatal(err) + } if parsedPartOne != "foo" { t.Fatalf("Expected parsed part one foo, actual: %s", parsedPartOne) @@ -53,44 +56,3 @@ func TestAccGithubUtilTwoPartID(t *testing.T) { t.Fatalf("Expected parsed part two bar, actual: %s", parsedPartTwo) } } - -func TestAccValidateTwoPartID(t *testing.T) { - cases := []struct { - name string - id string - expectedErr string - }{ - { - name: "valid", - id: "foo:bar", - }, - { - name: "blank ID", - id: "", - expectedErr: "no ID supplied. Please supply an ID format matching organization:username", - }, - { - name: "not enough parts", - id: "foo", - expectedErr: "incorrectly formatted ID \"foo\". Please supply an ID format matching organization:username", - }, - { - name: "too many parts", - id: "foo:bar:baz", - expectedErr: "incorrectly formatted ID \"foo:bar:baz\". Please supply an ID format matching organization:username", - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - err := validateTwoPartID(tc.id) - switch { - case err != nil && tc.expectedErr == "": - t.Fatalf("expected no error, got %q", err) - case err != nil && tc.expectedErr != "": - if err.Error() != tc.expectedErr { - t.Fatalf("expected error to be %q, got %q", tc.expectedErr, err.Error()) - } - } - }) - } -}