From cc312f2d1d76d48b2ddd8392c6f51c0f11cac533 Mon Sep 17 00:00:00 2001 From: asraa Date: Thu, 12 Aug 2021 11:54:28 -0400 Subject: [PATCH] :sparkles: feature: branch protection without admin token (#823) * branch protection without admin permission Signed-off-by: Asra Ali * handle other errors Signed-off-by: Asra Ali * fix lint Signed-off-by: Asra Ali Co-authored-by: Naveen <172697+naveensrinivasan@users.noreply.github.com> --- checks/branch_protection.go | 30 +++++++++++++++++++--- checks/branch_protection_test.go | 43 ++++++++++++++++++++++++++++---- e2e/branch_protection_test.go | 7 +++--- 3 files changed, 68 insertions(+), 12 deletions(-) diff --git a/checks/branch_protection.go b/checks/branch_protection.go index ebe86d68bb6..2f941926a4b 100644 --- a/checks/branch_protection.go +++ b/checks/branch_protection.go @@ -16,6 +16,8 @@ package checks import ( "context" + "errors" + "net/http" "regexp" "github.com/google/go-github/v32/github" @@ -100,12 +102,14 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl checkBranches[*repo.DefaultBranch] = true protected := true + unknown := false // Check protections on all the branches. for b := range checkBranches { p, err := isBranchProtected(branches, b) if err != nil { return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) } + // nolint if !p { protected = false dl.Warn("branch protection not enabled for branch '%s'", b) @@ -113,7 +117,17 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl // The branch is protected. Check the protection. score, err := getProtectionAndCheck(ctx, r, dl, ownerStr, repoStr, b) if err != nil { - return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) + if errors.Is(err, errInternalBranchNotFound) { + // Without an admin token, you only get information on the protection boolean. + // Add a score of 1 (minimal branch protection) for this protected branch. + unknown = true + scores = append(scores, 1) + dl.Warn("no detailed settings available for branch protection '%s'", b) + continue + } else { + // Github timeout or other error + return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) + } } scores = append(scores, score) } @@ -135,6 +149,11 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl "branch protection is fully enabled on development and all release branches") } + if unknown { + return checker.CreateResultWithScore(CheckBranchProtection, + "branch protection is enabled on development and all release branches but settings are unknown", score) + } + return checker.CreateResultWithScore(CheckBranchProtection, "branch protection is not maximal on development and all release branches", score) } @@ -170,9 +189,14 @@ func isBranchProtected(branches []*github.Branch, name string) (bool, error) { func getProtectionAndCheck(ctx context.Context, r repositories, dl checker.DetailLogger, ownerStr, repoStr, branch string) (int, error) { - // We only call this if the branch is protected. An error indicates not found. - protection, _, err := r.GetBranchProtection(ctx, ownerStr, repoStr, branch) + // We only call this if the branch is protected. + protection, resp, err := r.GetBranchProtection(ctx, ownerStr, repoStr, branch) if err != nil { + // Check the type of error. A not found error indicates that permissions are denied. + if resp.StatusCode == http.StatusNotFound { + //nolint + return 1, sce.Create(errInternalBranchNotFound, errInternalBranchNotFound.Error()) + } //nolint return checker.InconclusiveResultScore, sce.Create(sce.ErrScorecardInternal, err.Error()) } diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index 7d01a8ea35f..1f9c0fda46e 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -31,6 +31,7 @@ type mockRepos struct { protections map[string]*github.Protection defaultBranch *string releases []*string + nonadmin bool } func (m mockRepos) Get(ctx context.Context, o, r string) ( @@ -51,11 +52,13 @@ func (m mockRepos) ListReleases(ctx context.Context, owner string, func (m mockRepos) GetBranchProtection(ctx context.Context, o string, r string, b string) (*github.Protection, *github.Response, error) { - p, ok := m.protections[b] - if ok { - return p, &github.Response{ - Response: &http.Response{StatusCode: http.StatusAccepted}, - }, nil + if !m.nonadmin { + p, ok := m.protections[b] + if ok { + return p, &github.Response{ + Response: &http.Response{StatusCode: http.StatusAccepted}, + }, nil + } } return nil, &github.Response{ Response: &http.Response{StatusCode: http.StatusNotFound}, @@ -88,6 +91,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { defaultBranch *string releases []*string protections map[string]*github.Protection + nonadmin bool }{ { name: "Only development branch", @@ -395,6 +399,34 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { }, }, }, + { + name: "Non-admin check with protected release and development", + expected: scut.TestReturn{ + Errors: nil, + Score: 1, + NumberOfWarn: 2, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + nonadmin: true, + defaultBranch: &main, + branches: []*string{&rel1, &main}, + releases: []*string{&rel1}, + protections: map[string]*github.Protection{ + "main": { + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: []string{"foo"}, + }, + }, + "release/v.1": { + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: []string{"foo"}, + }, + }, + }, + }, } for _, tt := range tests { @@ -406,6 +438,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { branches: tt.branches, releases: tt.releases, protections: tt.protections, + nonadmin: tt.nonadmin, } dl := scut.TestDetailLogger{} r := checkReleaseAndDevBranchProtection(context.Background(), m, diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index 44cbb69e261..b6a5ddce35c 100644 --- a/e2e/branch_protection_test.go +++ b/e2e/branch_protection_test.go @@ -22,7 +22,6 @@ import ( "github.com/ossf/scorecard/v2/checker" "github.com/ossf/scorecard/v2/checks" - sce "github.com/ossf/scorecard/v2/errors" scut "github.com/ossf/scorecard/v2/utests" ) @@ -41,9 +40,9 @@ var _ = Describe("E2E TEST:"+checks.CheckBranchProtection, func() { Dlogger: &dl, } expected := scut.TestReturn{ - Errors: []error{sce.ErrScorecardInternal}, - Score: checker.InconclusiveResultScore, - NumberOfWarn: 0, + Errors: nil, + Score: 1, + NumberOfWarn: 3, NumberOfInfo: 0, NumberOfDebug: 0, }