✨ Add CODEOWNERS branch protection check (#2057)

* Add CODEOWNERS branch protection check

* Add docs

* Make CODEOWNERS branch protection part of the 'real' score instead of
extra-credit

* Fix Github checks

* Fix lint issues for `range` operator
* Fix e2e test failure

* Incorporate CODEOWNERS check as part of Code Review checks Level 4

* Fix lint (hopefully?)

* Address PR comments - docs

checks/branch_protection_test.go

@@ -134,7 +134,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  5,
+				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
 			},
@@ -172,8 +172,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  6,
-				NumberOfInfo:  8,
+				NumberOfWarn:  7,
+				NumberOfInfo:  9,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: main,
@@ -227,7 +227,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				Error:         nil,
 				Score:         8,
 				NumberOfWarn:  2,
-				NumberOfInfo:  12,
+				NumberOfInfo:  14,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: main,
@@ -280,7 +280,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  5,
+				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
 			},

checks/evaluation/branch_protection.go

@@ -30,6 +30,7 @@ const (
 	nonAdminContextLevel        = 2 // Level 3.
 	nonAdminThoroughReviewLevel = 1 // Level 4.
 	adminThoroughReviewLevel    = 1 // Level 5.
+
 )
 
 type scoresInfo struct {
@@ -40,6 +41,7 @@ type scoresInfo struct {
 	context             int
 	thoroughReview      int
 	adminThoroughReview int
+	codeownerReview     int
 }
 
 // Maximum score depending on whether admin token is used.
@@ -76,6 +78,7 @@ func BranchProtection(name string, dl checker.DetailLogger,
 		score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(&b, dl)
 		// Do we want this?
 		score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(&b, dl)
+		score.scores.codeownerReview, score.maxes.codeownerReview = codeownersBranchProtection(&b, dl)
 
 		scores = append(scores, score)
 	}
@@ -104,60 +107,76 @@ func BranchProtection(name string, dl checker.DetailLogger,
 
 func computeNonAdminBasicScore(scores []levelScore) int {
 	score := 0
-	for _, s := range scores {
+	for i := range scores {
+		s := scores[i]
 		score += s.scores.basic
 	}
 	return score
 }
 
 func computeAdminBasicScore(scores []levelScore) int {
 	score := 0
-	for _, s := range scores {
+	for i := range scores {
+		s := scores[i]
 		score += s.scores.adminBasic
 	}
 	return score
 }
 
 func computeNonAdminReviewScore(scores []levelScore) int {
 	score := 0
-	for _, s := range scores {
+	for i := range scores {
+		s := scores[i]
 		score += s.scores.review
 	}
 	return score
 }
 
 func computeAdminReviewScore(scores []levelScore) int {
 	score := 0
-	for _, s := range scores {
+	for i := range scores {
+		s := scores[i]
 		score += s.scores.adminReview
 	}
 	return score
 }
 
 func computeNonAdminThoroughReviewScore(scores []levelScore) int {
 	score := 0
-	for _, s := range scores {
+	for i := range scores {
+		s := scores[i]
 		score += s.scores.thoroughReview
 	}
 	return score
 }
 
 func computeAdminThoroughReviewScore(scores []levelScore) int {
 	score := 0
-	for _, s := range scores {
+	for i := range scores {
+		s := scores[i]
 		score += s.scores.adminThoroughReview
 	}
 	return score
 }
 
 func computeNonAdminContextScore(scores []levelScore) int {
 	score := 0
-	for _, s := range scores {
+	for i := range scores {
+		s := scores[i]
 		score += s.scores.context
 	}
 	return score
 }
 
+func computeCodeownerThoroughReviewScore(scores []levelScore) int {
+	score := 0
+	for i := range scores {
+		s := scores[i]
+		score += s.scores.codeownerReview
+	}
+	return score
+}
+
 func noarmalizeScore(score, max, level int) float64 {
 	if max == 0 {
 		return float64(level)
@@ -204,14 +223,18 @@ func computeScore(scores []levelScore) (int, error) {
 	}
 
 	// Fourth, check the thorough non-admin reviews.
+	// Also check whether this repo requires codeowner review
 	maxThoroughReviewScore := maxScore.thoroughReview * len(scores)
+	maxCodeownerReviewScore := maxScore.codeownerReview * len(scores)
 	thoroughReviewScore := computeNonAdminThoroughReviewScore(scores)
-	score += noarmalizeScore(thoroughReviewScore, maxThoroughReviewScore, nonAdminThoroughReviewLevel)
+	codeownerReviewScore := computeCodeownerThoroughReviewScore(scores)
+	score += noarmalizeScore(thoroughReviewScore+codeownerReviewScore, maxThoroughReviewScore+maxCodeownerReviewScore,
+		nonAdminThoroughReviewLevel)
 	if thoroughReviewScore != maxThoroughReviewScore {
 		return int(score), nil
 	}
 
-	// Last, check the thorough admin review config.
+	// Lastly, check the thorough admin review config.
 	// This one is controversial and has usability issues
 	// https://github.com/ossf/scorecard/issues/1027, so we may remove it.
 	maxAdminThoroughReviewScore := maxScore.adminThoroughReview * len(scores)
@@ -412,3 +435,22 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
 	}
 	return score, max
 }
+
+func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) {
+	score := 0
+	max := 1
+
+	log := branch.Protected != nil && *branch.Protected
+
+	if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
+		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
+		case true:
+			info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)
+			score++
+		default:
+			warn(dl, log, "codeowner review is not required on branch '%s'", *branch.Name)
+		}
+	}
+
+	return score, max
+}

checks/evaluation/branch_protection_test.go

@@ -31,6 +31,7 @@ func testScore(branch *clients.BranchRef, dl checker.DetailLogger) (int, error)
 	score.scores.context, score.maxes.context = nonAdminContextProtection(branch, dl)
 	score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(branch, dl)
 	score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(branch, dl)
+	score.scores.codeownerReview, score.maxes.codeownerReview = codeownersBranchProtection(branch, dl)
 
 	return computeScore([]levelScore{score})
 }
@@ -52,7 +53,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  5,
+				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
 			},
@@ -96,7 +97,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  3,
+				NumberOfWarn:  4,
 				NumberOfInfo:  4,
 				NumberOfDebug: 0,
 			},
@@ -126,7 +127,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  4,
+				NumberOfWarn:  5,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
 			},
@@ -156,7 +157,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  4,
+				NumberOfWarn:  5,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
 			},
@@ -186,7 +187,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         3,
-				NumberOfWarn:  3,
+				NumberOfWarn:  4,
 				NumberOfInfo:  4,
 				NumberOfDebug: 0,
 			},
@@ -216,7 +217,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  4,
+				NumberOfWarn:  5,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
 			},
@@ -246,7 +247,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         1,
-				NumberOfWarn:  5,
+				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
 			},
@@ -277,7 +278,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         1,
-				NumberOfWarn:  5,
+				NumberOfWarn:  6,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
 			},
@@ -308,7 +309,7 @@ func TestIsBranchProtected(t *testing.T) {
 				Error:         nil,
 				Score:         8,
 				NumberOfWarn:  1,
-				NumberOfInfo:  6,
+				NumberOfInfo:  7,
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
@@ -332,6 +333,36 @@ func TestIsBranchProtected(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Branches are protected and require codeowner review",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         8,
+				NumberOfWarn:  2,
+				NumberOfInfo:  6,
+				NumberOfDebug: 0,
+			},
+			branch: &clients.BranchRef{
+				Name:      &branchVal,
+				Protected: &trueVal,
+				BranchProtectionRule: clients.BranchProtectionRule{
+					EnforceAdmins:        &trueVal,
+					RequireLinearHistory: &trueVal,
+					AllowForcePushes:     &falseVal,
+					AllowDeletions:       &falseVal,
+					CheckRules: clients.StatusChecksRule{
+						RequiresStatusChecks: &falseVal,
+						UpToDateBeforeMerge:  &trueVal,
+						Contexts:             []string{"foo"},
+					},
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						DismissStaleReviews:          &trueVal,
+						RequireCodeOwnerReviews:      &falseVal,
+						RequiredApprovingReviewCount: &oneVal,
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below

docs/checks.md

@@ -65,7 +65,7 @@ certain workflows for branches, such as requiring review or passing certain
 status checks before acceptance into a main branch, or preventing rewriting of
 public history.
 
-Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, and `StrictStatusCheck`. If
+Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, `StrictStatusCheck` and `RequireCodeownerReview`. If
 the provided token does not have admin access, the check will query the branch
 settings accessible to non-admins and provide results based only on these settings.
 Even so, we recommend using a non-admin token, which provides a thorough enough
@@ -116,6 +116,7 @@ Tier 4 Requirements (9/10 points):
 
 Tier 5 Requirements (10/10 points):
   - For administrators: Dismiss stale reviews
+  - For administrators: Require CODEOWNER review
 
 
 **Remediation steps**
@@ -385,7 +386,7 @@ This check will only succeed if a Github project is >90 days old. Projects
 that are younger than this are too new to assess whether they are maintained
 or not, and users should inspect the contents of those projects to ensure they
 are as expected.
-
+ 
 
 **Remediation steps**
 - There is no remediation work needed from projects with a low score; this check simply provides insight into the project activity and maintenance commitment. External users should determine whether the software is the type that would not normally need active maintenance.

docs/checks/internal/checks.yaml

@@ -153,7 +153,7 @@ checks:
       status checks before acceptance into a main branch, or preventing rewriting of
       public history.
 
-      Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, and `StrictStatusCheck`. If
+      Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, `StrictStatusCheck` and `RequireCodeownerReview`. If
       the provided token does not have admin access, the check will query the branch
       settings accessible to non-admins and provide results based only on these settings.
       Even so, we recommend using a non-admin token, which provides a thorough enough
@@ -204,6 +204,7 @@ checks:
       
       Tier 5 Requirements (10/10 points):
         - For administrators: Dismiss stale reviews
+        - For administrators: Require CODEOWNER review
 
     remediation:
       - >-

e2e/branch_protection_test.go

@@ -48,7 +48,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
 				Error:         nil,
 				Score:         6,
 				NumberOfWarn:  1,
-				NumberOfInfo:  3,
+				NumberOfInfo:  4,
 				NumberOfDebug: 3,
 			}
 			result := checks.BranchProtection(&req)
@@ -105,7 +105,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
 			expected := scut.TestReturn{
 				Error:         nil,
 				Score:         1,
-				NumberOfWarn:  2,
+				NumberOfWarn:  3,
 				NumberOfInfo:  3,
 				NumberOfDebug: 3,
 			}