✨ Make Branch-Protection score more granular (#777)

* commit

* uni tests

* full score

* typos

* update msg

* remove function

* comments

* linter

* comments

checker/check_result.go

@@ -219,21 +219,3 @@ func CreateRuntimeErrorResult(name string, e error) CheckResult {
 		Reason:  e.Error(), // Note: message already accessible by caller thru `Error`.
 	}
 }
-
-// UPGRADEv2: functions below will be renamed.
-func MakeAndResult2(checks ...CheckResult) CheckResult {
-	if len(checks) == 0 {
-		// That should never happen.
-		panic("MakeResult called with no checks")
-	}
-
-	worseResult := checks[0]
-	// UPGRADEv2: will go away after old struct is removed.
-	//nolint
-	for _, result := range checks[1:] {
-		if result.Score < worseResult.Score {
-			worseResult = result
-		}
-	}
-	return worseResult
-}

checks/branch_protection.go

@@ -16,7 +16,6 @@ package checks
 
 import (
 	"context"
-	"fmt"
 	"regexp"
 
 	"github.com/google/go-github/v32/github"
@@ -59,22 +58,20 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl
 		return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
 	}
 
-	// Get release branches
+	// Get release branches.
 	releases, _, err := r.ListReleases(ctx, ownerStr, repoStr, &github.ListOptions{})
 	if err != nil {
 		return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
 	}
 
-	var checks []checker.CheckResult
+	var scores []int
 	commit := regexp.MustCompile("^[a-f0-9]{40}$")
 	checkBranches := make(map[string]bool)
 	for _, release := range releases {
 		if release.TargetCommitish == nil {
 			// Log with a named error if target_commitish is nil.
 			e := sce.Create(sce.ErrScorecardInternal, errInternalCommitishNil.Error())
-			r := checker.CreateRuntimeErrorResult(CheckBranchProtection, e)
-			checks = append(checks, r)
-			continue
+			return checker.CreateRuntimeErrorResult(CheckBranchProtection, e)
 		}
 
 		// TODO: if this is a sha, get the associated branch. for now, ignore.
@@ -86,9 +83,7 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl
 		name, err := resolveBranchName(branches, *release.TargetCommitish)
 		if err != nil {
 			// If the commitish branch is still not found, fail.
-			r := checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
-			checks = append(checks, r)
-			continue
+			return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
 		}
 
 		// Branch is valid, add to list of branches to check.
@@ -102,25 +97,44 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl
 	}
 	checkBranches[*repo.DefaultBranch] = true
 
-	// Check protections on the branches.
+	protected := true
+	// Check protections on all the branches.
 	for b := range checkBranches {
-		protected, err := isBranchProtected(branches, b)
+		p, err := isBranchProtected(branches, b)
 		if err != nil {
-			r := checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
-			checks = append(checks, r)
+			return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
 		}
-		if !protected {
-			r := checker.CreateMinScoreResult(CheckBranchProtection,
-				fmt.Sprintf("branch protection not enabled for branch '%s'", b))
-			checks = append(checks, r)
+		if !p {
+			protected = false
+			dl.Warn("branch protection not enabled for branch '%s'", b)
 		} else {
 			// The branch is protected. Check the protection.
-			res := getProtectionAndCheck(ctx, r, dl, ownerStr, repoStr, b)
-			checks = append(checks, res)
+			score, err := getProtectionAndCheck(ctx, r, dl, ownerStr, repoStr, b)
+			if err != nil {
+				return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
+			}
+			scores = append(scores, score)
 		}
 	}
 
-	return checker.MakeAndResult2(checks...)
+	if !protected {
+		return checker.CreateMinScoreResult(CheckBranchProtection,
+			"branch protection not enabled on development/release branches")
+	}
+
+	score := checker.AggregateScores(scores...)
+	if score == checker.MinResultScore {
+		return checker.CreateMinScoreResult(CheckBranchProtection,
+			"branch protection not enabled on development/release branches")
+	}
+
+	if score == checker.MaxResultScore {
+		return checker.CreateMaxScoreResult(CheckBranchProtection,
+			"branch protection is fully enabled on development and all release branches")
+	}
+
+	return checker.CreateResultWithScore(CheckBranchProtection,
+		"branch protection is not maximal on development and all release branches", score)
 }
 
 func resolveBranchName(branches []*github.Branch, name string) (*string, error) {
@@ -153,115 +167,122 @@ func isBranchProtected(branches []*github.Branch, name string) (bool, error) {
 }
 
 func getProtectionAndCheck(ctx context.Context, r repositories, dl checker.DetailLogger, ownerStr, repoStr,
-	branch string) checker.CheckResult {
+	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)
 	if err != nil {
-		e := sce.Create(sce.ErrScorecardInternal, err.Error())
-		return checker.CreateRuntimeErrorResult(CheckBranchProtection, e)
+		//nolint
+		return checker.InconclusiveResultScore, sce.Create(sce.ErrScorecardInternal, err.Error())
 	}
 
-	return IsBranchProtected(protection, branch, dl)
+	return IsBranchProtected(protection, branch, dl), nil
 }
 
-func IsBranchProtected(protection *github.Protection, branch string, dl checker.DetailLogger) checker.CheckResult {
-	totalChecks := 10
-	totalSuccess := 0
+func IsBranchProtected(protection *github.Protection, branch string, dl checker.DetailLogger) int {
+	totalScore := 15
+	score := 0
 
-	// This is disabled by default (good).
 	if protection.GetAllowForcePushes() != nil &&
 		protection.AllowForcePushes.Enabled {
-		dl.Warn("AllowForcePushes enabled on branch '%s'", branch)
+		dl.Warn("'force pushes' enabled on branch '%s'", branch)
 	} else {
-		dl.Info("AllowForcePushes disabled on branch '%s'", branch)
-		totalSuccess++
+		dl.Info("'force pushes' disabled on branch '%s'", branch)
+		score++
 	}
 
-	// This is disabled by default (good).
 	if protection.GetAllowDeletions() != nil &&
 		protection.AllowDeletions.Enabled {
-		dl.Warn("AllowDeletions enabled on branch '%s'", branch)
+		dl.Warn("'allow deletion' enabled on branch '%s'", branch)
 	} else {
-		dl.Info("AllowDeletions disabled on branch '%s'", branch)
-		totalSuccess++
+		dl.Info("'allow deletion' disabled on branch '%s'", branch)
+		score++
 	}
 
-	// This is disabled by default (bad).
-	if protection.GetEnforceAdmins() != nil &&
-		protection.EnforceAdmins.Enabled {
-		dl.Info("EnforceAdmins disabled on branch '%s'", branch)
-		totalSuccess++
-	} else {
-		dl.Warn("EnforceAdmins disabled on branch '%s'", branch)
-	}
-
-	// This is disabled by default (bad).
 	if protection.GetRequireLinearHistory() != nil &&
 		protection.RequireLinearHistory.Enabled {
-		dl.Info("Linear history enabled on branch '%s'", branch)
-		totalSuccess++
+		dl.Info("linear history enabled on branch '%s'", branch)
+		score++
 	} else {
-		dl.Warn("Linear history disabled on branch '%s'", branch)
+		dl.Warn("linear history disabled on branch '%s'", branch)
 	}
 
-	if requiresStatusChecks(protection, branch, dl) {
-		dl.Info("Strict status check enabled on branch '%s'", branch)
-		totalSuccess++
+	score += requiresStatusChecks(protection, branch, dl)
+
+	score += requiresThoroughReviews(protection, branch, dl)
+
+	if protection.GetEnforceAdmins() != nil &&
+		protection.EnforceAdmins.Enabled {
+		dl.Info("'admininistrator' PRs need reviews before being merged on branch '%s'", branch)
+		score += 3
+	} else {
+		dl.Warn("'admininistrator' PRs are exempt from reviews on branch '%s'", branch)
 	}
 
-	if requiresThoroughReviews(protection, branch, dl) {
-		totalSuccess++
+	if score == totalScore {
+		return checker.MaxResultScore
 	}
 
-	return checker.CreateProportionalScoreResult(CheckBranchProtection,
-		"%d out of %d branch protection settings are enabled", totalSuccess, totalChecks)
+	return checker.CreateProportionalScore(score, totalScore)
 }
 
 // Returns true if several PR status checks requirements are enabled. Otherwise returns false and logs why it failed.
-func requiresStatusChecks(protection *github.Protection, branch string, dl checker.DetailLogger) bool {
-	// This is disabled by default (bad).
-	if protection.GetRequiredStatusChecks() != nil &&
-		protection.RequiredStatusChecks.Strict &&
-		len(protection.RequiredStatusChecks.Contexts) > 0 {
-		return true
+// Maximum score returned is 2.
+func requiresStatusChecks(protection *github.Protection, branch string, dl checker.DetailLogger) int {
+	score := 0
+
+	if protection.GetRequiredStatusChecks() == nil ||
+		!protection.RequiredStatusChecks.Strict {
+		dl.Warn("status checks for merging disabled on branch '%s'", branch)
+		return score
 	}
-	switch {
-	case protection.RequiredStatusChecks == nil ||
-		!protection.RequiredStatusChecks.Strict:
-		dl.Warn("Status checks for merging disabled on branch '%s'", branch)
-	case len(protection.RequiredStatusChecks.Contexts) == 0:
-		dl.Warn("Status checks for merging have no specific status to check on branch '%s'", branch)
-	default:
-		panic("Unhandled status checks error")
+
+	dl.Info("strict status check enabled on branch '%s'", branch)
+	score++
+
+	if len(protection.RequiredStatusChecks.Contexts) > 0 {
+		dl.Warn("status checks for merging have specific status to check on branch '%s'", branch)
+		score++
+	} else {
+		dl.Warn("status checks for merging have no specific status to check on branch '%s'", branch)
 	}
-	return false
+
+	return score
 }
 
 // Returns true if several PR review requirements are enabled. Otherwise returns false and logs why it failed.
-func requiresThoroughReviews(protection *github.Protection, branch string, dl checker.DetailLogger) bool {
-	// This is disabled by default (bad).
-	if protection.GetRequiredPullRequestReviews() != nil &&
-		protection.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews &&
-		protection.RequiredPullRequestReviews.DismissStaleReviews &&
-		protection.RequiredPullRequestReviews.RequireCodeOwnerReviews {
-		return true
+// Maximum score returned is 7.
+func requiresThoroughReviews(protection *github.Protection, branch string, dl checker.DetailLogger) int {
+	score := 0
+
+	if protection.GetRequiredPullRequestReviews() == nil {
+		dl.Warn("pull request reviews disabled on branch '%s'", branch)
+		return score
 	}
-	if protection.RequiredPullRequestReviews == nil {
-		dl.Warn("Pullrequest reviews disabled on branch '%s'", branch)
-		return false
-	}
-	switch {
-	case protection.RequiredPullRequestReviews.RequiredApprovingReviewCount < minReviews:
-		dl.Warn("Number of required reviewers is only %d on branch '%s'",
+
+	if protection.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews {
+		dl.Info("number of required reviewers is %d on branch '%s'",
+			protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch)
+		score += 2
+	} else {
+		score += protection.RequiredPullRequestReviews.RequiredApprovingReviewCount
+		dl.Warn("number of required reviewers is only %d on branch '%s'",
 			protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch)
-		fallthrough
-	case !protection.RequiredPullRequestReviews.DismissStaleReviews:
-		dl.Warn("Stale review dismissal disabled on branch '%s'", branch)
-		fallthrough
-	case !protection.RequiredPullRequestReviews.RequireCodeOwnerReviews:
-		dl.Warn("Owner review not required on branch '%s'", branch)
-	default:
-		panic("Unhandled pull request error")
 	}
-	return false
+
+	if protection.RequiredPullRequestReviews.DismissStaleReviews {
+		// This is a big deal to enabled, so let's reward 3 points.
+		dl.Info("Stale review dismissal enabled on branch '%s'", branch)
+		score += 3
+	} else {
+		dl.Warn("Stale review dismissal disabled on branch '%s'", branch)
+	}
+
+	if protection.RequiredPullRequestReviews.RequireCodeOwnerReviews {
+		score += 2
+		dl.Info("Owner review required on branch '%s'", branch)
+	} else {
+		dl.Warn("Owner review not required on branch '%s'", branch)
+	}
+
+	return score
 }

checks/branch_protection_test.go

@@ -93,7 +93,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			name: "Only development branch",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         2,
+				Score:         1,
 				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
@@ -141,9 +141,9 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			name: "Take worst of release and development",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         2,
-				NumberOfWarn:  9,
-				NumberOfInfo:  7,
+				Score:         5,
+				NumberOfWarn:  8,
+				NumberOfInfo:  9,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: &main,
@@ -222,9 +222,9 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			name: "Both release and development are OK",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         5,
-				NumberOfWarn:  6,
-				NumberOfInfo:  10,
+				Score:         9,
+				NumberOfWarn:  4,
+				NumberOfInfo:  14,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: &main,
@@ -303,7 +303,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			name: "Ignore a non-branch targetcommitish",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         2,
+				Score:         1,
 				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
@@ -352,8 +352,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Errors:        []error{sce.ErrScorecardInternal},
 				Score:         checker.InconclusiveResultScore,
-				NumberOfWarn:  6,
-				NumberOfInfo:  2,
+				NumberOfWarn:  0,
+				NumberOfInfo:  0,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: &main,
@@ -427,7 +427,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Nothing is enabled",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         2,
+				Score:         1,
 				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
@@ -470,7 +470,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Nothing is enabled and values in github.Protection are nil",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         2,
+				Score:         1,
 				NumberOfWarn:  4,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
@@ -481,8 +481,8 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Required status check enabled",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         3,
-				NumberOfWarn:  5,
+				Score:         2,
+				NumberOfWarn:  6,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
 			},
@@ -526,7 +526,7 @@ func TestIsBranchProtected(t *testing.T) {
 				Errors:        nil,
 				Score:         2,
 				NumberOfWarn:  6,
-				NumberOfInfo:  2,
+				NumberOfInfo:  3,
 				NumberOfDebug: 0,
 			},
 			protection: &github.Protection{
@@ -567,7 +567,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Required pull request enabled",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         3,
+				Score:         2,
 				NumberOfWarn:  5,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
@@ -653,7 +653,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Required linear history enabled",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         3,
+				Score:         2,
 				NumberOfWarn:  5,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
@@ -696,7 +696,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Allow force push enabled",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         1,
+				Score:         0,
 				NumberOfWarn:  7,
 				NumberOfInfo:  1,
 				NumberOfDebug: 0,
@@ -739,7 +739,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Allow deletions enabled",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         1,
+				Score:         0,
 				NumberOfWarn:  7,
 				NumberOfInfo:  1,
 				NumberOfDebug: 0,
@@ -782,9 +782,9 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Branches are protected",
 			expected: scut.TestReturn{
 				Errors:        nil,
-				Score:         5,
-				NumberOfWarn:  3,
-				NumberOfInfo:  5,
+				Score:         9,
+				NumberOfWarn:  2,
+				NumberOfInfo:  7,
 				NumberOfDebug: 0,
 			},
 			protection: &github.Protection{
@@ -827,8 +827,8 @@ func TestIsBranchProtected(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			dl := scut.TestDetailLogger{}
-			r := IsBranchProtected(tt.protection, "test", &dl)
-			scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl)
+			score := IsBranchProtected(tt.protection, "test", &dl)
+			scut.ValidateTestValues(t, tt.name, &tt.expected, score, nil, &dl)
 		})
 	}
 }

checks/code_review.go

@@ -167,7 +167,7 @@ func prowCodeReview(c *checker.CheckRequest) (int, string, error) {
 func commitMessageHints(c *checker.CheckRequest) (int, string, error) {
 	commits, err := c.RepoClient.ListCommits()
 	if err != nil {
-		// nolint: wraperror
+		// nolint: wrapcheck
 		return checker.InconclusiveResultScore, "",
 			sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("Client.Repositories.ListCommits: %v", err))
 	}

e2e/branch_protection_test.go

@@ -26,7 +26,7 @@ import (
 	scut "github.com/ossf/scorecard/v2/utests"
 )
 
-var _ = Describe("E2E TEST:Branch Protection", func() {
+var _ = Describe("E2E TEST:"+checks.CheckBranchProtection, func() {
 	Context("E2E TEST:Validating branch protection", func() {
 		It("Should fail to return branch protection on other repositories", func() {
 			dl := scut.TestDetailLogger{}
@@ -55,5 +55,34 @@ var _ = Describe("E2E TEST:Branch Protection", func() {
 			// New version.
 			Expect(scut.ValidateTestReturn(nil, "branch protection not accessible", &expected, &result, &dl)).Should(BeTrue())
 		})
+		Context("E2E TEST:Validating branch protection", func() {
+			It("Should fail to return branch protection on other repositories", func() {
+				dl := scut.TestDetailLogger{}
+				req := checker.CheckRequest{
+					Ctx:         context.Background(),
+					Client:      ghClient,
+					HTTPClient:  httpClient,
+					RepoClient:  nil,
+					Owner:       "ossf-tests",
+					Repo:        "scorecard-check-branch-protection-e2e",
+					GraphClient: graphClient,
+					Dlogger:     &dl,
+				}
+				expected := scut.TestReturn{
+					Errors:        nil,
+					Score:         9,
+					NumberOfWarn:  1,
+					NumberOfInfo:  8,
+					NumberOfDebug: 0,
+				}
+				result := checks.BranchProtection(&req)
+				// UPGRADEv2: to remove.
+				// Old version.
+				Expect(result.Error).Should(BeNil())
+				Expect(result.Pass).Should(BeTrue())
+				// New version.
+				Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue())
+			})
+		})
 	})
 })