✨ Move "EnforcesAdmins" to tier 5 Branch-Protection (ossf#3502)

* Remove EnforceAdmins from tier 1. Scores in some tests either increase to 3, or 4, since EnfroceAdmins no longer keeps them in tier 1. The number of Debug, Info, and Warn messages will decrease by 1 per branch, since we're no longer logging them. Signed-off-by: Spencer Schrock <[email protected]> * move enforce admins to tier 5. Signed-off-by: Spencer Schrock <[email protected]> --------- Signed-off-by: Spencer Schrock <[email protected]> Signed-off-by: Allen Shearin <[email protected]>
kgangerlm · Nov 13, 2023 · 64c09c3 · 64c09c3
1 parent 40e5e75
commit 64c09c3
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 52 deletions.
diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -134,7 +134,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			name: "Only development branch",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         3,
 				NumberOfWarn:  7,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
@@ -173,7 +173,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			name: "Take worst of release and development",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         4,
 				NumberOfWarn:  9,
 				NumberOfInfo:  10,
 				NumberOfDebug: 0,
@@ -285,7 +285,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			name: "Ignore a non-branch targetcommitish",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         3,
 				NumberOfWarn:  7,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -25,7 +25,7 @@ import (
 const (
 	minReviews = 2
 	// Points incremented at each level.
-	adminNonAdminBasicLevel     = 3 // Level 1.
+	basicLevel                  = 3 // Level 1.
 	adminNonAdminReviewLevel    = 3 // Level 2.
 	nonAdminContextLevel        = 2 // Level 3.
 	nonAdminThoroughReviewLevel = 1 // Level 4.
@@ -35,7 +35,6 @@ const (
 
 type scoresInfo struct {
 	basic               int
-	adminBasic          int
 	review              int
 	adminReview         int
 	context             int
@@ -71,7 +70,6 @@ func BranchProtection(name string, dl checker.DetailLogger,
 			})
 		}
 		score.scores.basic, score.maxes.basic = basicNonAdminProtection(&b, dl)
-		score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(&b, dl)
 		score.scores.review, score.maxes.review = nonAdminReviewProtection(&b)
 		score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(&b, dl)
 		score.scores.context, score.maxes.context = nonAdminContextProtection(&b, dl)
@@ -114,15 +112,6 @@ func computeNonAdminBasicScore(scores []levelScore) int {
 	return score
 }
 
-func computeAdminBasicScore(scores []levelScore) int {
-	score := 0
-	for i := range scores {
-		s := scores[i]
-		score += s.scores.adminBasic
-	}
-	return score
-}
-
 func computeNonAdminReviewScore(scores []levelScore) int {
 	score := 0
 	for i := range scores {
@@ -194,12 +183,9 @@ func computeScore(scores []levelScore) (int, error) {
 
 	// First, check if they all pass the basic (admin and non-admin) checks.
 	maxBasicScore := maxScore.basic * len(scores)
-	maxAdminBasicScore := maxScore.adminBasic * len(scores)
 	basicScore := computeNonAdminBasicScore(scores)
-	adminBasicScore := computeAdminBasicScore(scores)
-	score += noarmalizeScore(basicScore+adminBasicScore, maxBasicScore+maxAdminBasicScore, adminNonAdminBasicLevel)
-	if basicScore != maxBasicScore ||
-		adminBasicScore != maxAdminBasicScore {
+	score += noarmalizeScore(basicScore, maxBasicScore, basicLevel)
+	if basicScore != maxBasicScore {
 		return int(score), nil
 	}
 
@@ -308,30 +294,6 @@ func basicNonAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger)
 	return score, max
 }
 
-func basicAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) {
-	score := 0
-	max := 0
-	// Only log information if the branch is protected.
-	log := branch.Protected != nil && *branch.Protected
-
-	// nil typically means we do not have access to the value.
-	if branch.BranchProtectionRule.EnforceAdmins != nil {
-		// Note: we don't inrecase max possible score for non-admin viewers.
-		max++
-		switch *branch.BranchProtectionRule.EnforceAdmins {
-		case true:
-			info(dl, log, "settings apply to administrators on branch '%s'", *branch.Name)
-			score++
-		case false:
-			warn(dl, log, "settings do not apply to administrators on branch '%s'", *branch.Name)
-		}
-	} else {
-		debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", *branch.Name)
-	}
-
-	return score, max
-}
-
 func nonAdminContextProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) {
 	score := 0
 	max := 0
@@ -422,6 +384,22 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL
 	} else {
 		debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name)
 	}
+
+	// nil typically means we do not have access to the value.
+	if branch.BranchProtectionRule.EnforceAdmins != nil {
+		// Note: we don't inrecase max possible score for non-admin viewers.
+		max++
+		switch *branch.BranchProtectionRule.EnforceAdmins {
+		case true:
+			info(dl, log, "settings apply to administrators on branch '%s'", *branch.Name)
+			score++
+		case false:
+			warn(dl, log, "settings do not apply to administrators on branch '%s'", *branch.Name)
+		}
+	} else {
+		debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", *branch.Name)
+	}
+
 	return score, max
 }
 

diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -25,7 +25,6 @@ import (
 func testScore(branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger) (int, error) {
 	var score levelScore
 	score.scores.basic, score.maxes.basic = basicNonAdminProtection(branch, dl)
-	score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(branch, dl)
 	score.scores.review, score.maxes.review = nonAdminReviewProtection(branch)
 	score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(branch, dl)
 	score.scores.context, score.maxes.context = nonAdminContextProtection(branch, dl)
@@ -53,7 +52,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Nothing is enabled",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         3,
 				NumberOfWarn:  7,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
@@ -98,7 +97,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Required status check enabled",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         4,
 				NumberOfWarn:  5,
 				NumberOfInfo:  4,
 				NumberOfDebug: 0,
@@ -129,7 +128,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Required status check enabled without checking for status string",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         4,
 				NumberOfWarn:  6,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
@@ -160,7 +159,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Required pull request enabled",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         4,
 				NumberOfWarn:  6,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,
@@ -222,7 +221,7 @@ func TestIsBranchProtected(t *testing.T) {
 			name: "Required linear history enabled",
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         2,
+				Score:         3,
 				NumberOfWarn:  6,
 				NumberOfInfo:  3,
 				NumberOfDebug: 0,

diff --git a/docs/checks.md b/docs/checks.md
@@ -109,7 +109,6 @@ Note: If Scorecard is run without an administrative access token, the requiremen
 Tier 1 Requirements (3/10 points):
   - Prevent force push
   - Prevent branch deletion
-  - For administrators: Include administrator for review
 
 Tier 2 Requirements (6/10 points):
   - Require at least 1 reviewer for approval before merging
@@ -125,6 +124,7 @@ Tier 4 Requirements (9/10 points):
 
 Tier 5 Requirements (10/10 points):
   - For administrators: Dismiss stale reviews and approvals when new commits are pushed
+  - For administrators: Include administrator for review
 
 GitLab Integration Status:
   - GitLab associates releases with commits and not with the branch. Releases are ignored in this portion of the scoring.

diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml
@@ -198,7 +198,6 @@ checks:
       Tier 1 Requirements (3/10 points):
         - Prevent force push
         - Prevent branch deletion
-        - For administrators: Include administrator for review
 
       Tier 2 Requirements (6/10 points):
         - Require at least 1 reviewer for approval before merging
@@ -214,6 +213,7 @@ checks:
 
       Tier 5 Requirements (10/10 points):
         - For administrators: Dismiss stale reviews and approvals when new commits are pushed
+        - For administrators: Include administrator for review
 
       GitLab Integration Status:
         - GitLab associates releases with commits and not with the branch. Releases are ignored in this portion of the scoring.