Skip to content

Commit

Permalink
✨ feature: branch protection without admin token (#823)
Browse files Browse the repository at this point in the history
* branch protection without admin permission

Signed-off-by: Asra Ali <asraa@google.com>

* handle other errors

Signed-off-by: Asra Ali <asraa@google.com>

* fix lint

Signed-off-by: Asra Ali <asraa@google.com>

Co-authored-by: Naveen <172697+naveensrinivasan@users.noreply.github.com>
  • Loading branch information
asraa and naveensrinivasan authored Aug 12, 2021
1 parent a10baab commit cc312f2
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 12 deletions.
30 changes: 27 additions & 3 deletions checks/branch_protection.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package checks

import (
"context"
"errors"
"net/http"
"regexp"

"github.com/google/go-github/v32/github"
Expand Down Expand Up @@ -100,20 +102,32 @@ 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)
} else {
// 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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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())
}
Expand Down
43 changes: 38 additions & 5 deletions checks/branch_protection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) (
Expand All @@ -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},
Expand Down Expand Up @@ -88,6 +91,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
defaultBranch *string
releases []*string
protections map[string]*github.Protection
nonadmin bool
}{
{
name: "Only development branch",
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
Expand Down
7 changes: 3 additions & 4 deletions e2e/branch_protection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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,
}
Expand Down

0 comments on commit cc312f2

Please sign in to comment.