revert the change which made RequiredPullRequestReviews a pointer

While the current approach works with the tiered scoring, it wont work for probes or if we remove tiers. Making the struct nil to signal that PRs aren't required hides some of the data we do have. This is especially problematic for repo rules, where we can infer all settings by what we see or dont see. Signed-off-by: Spencer Schrock <[email protected]>
ossf · Dec 12, 2023 · be03149 · be03149
1 parent db7b6e7
commit be03149
Show file tree

Hide file tree

Showing 10 changed files with 195 additions and 159 deletions.
diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -93,7 +93,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -112,7 +113,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -151,7 +153,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -186,7 +189,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -207,7 +211,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -242,7 +247,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -263,7 +269,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -299,7 +306,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -337,7 +345,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -285,8 +285,7 @@ func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) {
 	// Having at least 1 reviewer is twice as important as the other Tier 2 requirements.
 	const reviewerWeight = 2
 	max += reviewerWeight
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil &&
+	if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil &&
 		*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 {
 		// We do not display anything here, it's done in nonAdminThoroughReviewProtection()
 		score += reviewerWeight
@@ -329,15 +328,14 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
 	}
 
 	max++
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil {
+	if branch.BranchProtectionRule.RequiredPullRequestReviews.Required != nil &&
+		*branch.BranchProtectionRule.RequiredPullRequestReviews.Required {
 		score++
 		info(dl, log, "PRs are required in order to make changes on branch '%s'", *branch.Name)
 	} else {
 		warn(dl, log, "PRs are not required to make changes on branch '%s'; or we don't have data to detect it."+
 			"If you think it might be the latter, make sure to run Scorecard with a PAT or use Repo "+
 			"Rules (that are always public) instead of Branch Protection settings", *branch.Name)
-		// Warning it here because since `RequiredPullRequestReviews` is nil, we won't check reviewers count afterwards
-		warn(dl, log, "No reviewers required to merge changes on branch '%s'", *branch.Name)
 	}
 
 	return score, max
@@ -349,8 +347,7 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL
 	// Only log information if the branch is protected.
 	log := branch.Protected != nil && *branch.Protected
 
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
+	if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
 		// Note: we don't increase max possible score for non-admin viewers.
 		max++
 		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
@@ -391,18 +388,16 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
 
 	max++
 
-	// On this first check we exclude the case where PRs are not required to make changes,
-	// because it's already covered on adminReviewProtection function.
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
-		if *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews {
-			info(dl, log, "number of required reviewers is %d on branch '%s'",
-				*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name)
-			score++
-		} else {
-			warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d",
-				*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name, minReviews)
-		}
+	var reviewers int32
+	if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
+		reviewers = *(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount)
+	}
+	if reviewers >= minReviews {
+		info(dl, log, "number of required reviewers is %d on branch '%s'", reviewers, *branch.Name)
+		score++
+	} else {
+		warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d",
+			reviewers, *branch.Name, minReviews)
 	}
 
 	return score, max
@@ -416,8 +411,7 @@ func codeownerBranchProtection(
 
 	log := branch.Protected != nil && *branch.Protected
 
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
+	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)

diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -50,7 +50,7 @@ func TestIsBranchProtected(t *testing.T) {
 		expected        scut.TestReturn
 	}{
 		{
-			name: "Configs as they are right after creating new Branch Protection setting",
+			name: "GitHub default settings",
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         3,
@@ -62,12 +62,14 @@ func TestIsBranchProtected(t *testing.T) {
 				Name:      &branchVal,
 				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
-					AllowDeletions:             &falseVal,
-					AllowForcePushes:           &falseVal,
-					RequireLinearHistory:       &falseVal,
-					EnforceAdmins:              &falseVal,
-					RequireLastPushApproval:    &falseVal,
-					RequiredPullRequestReviews: nil,
+					AllowDeletions:          &falseVal,
+					AllowForcePushes:        &falseVal,
+					RequireLinearHistory:    &falseVal,
+					EnforceAdmins:           &falseVal,
+					RequireLastPushApproval: &falseVal,
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required: &falseVal,
+					},
 					CheckRules: clients.StatusChecksRule{
 						RequiresStatusChecks: &trueVal,
 						Contexts:             nil,
@@ -103,7 +105,8 @@ func TestIsBranchProtected(t *testing.T) {
 				Name:      &branchVal,
 				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -139,7 +142,8 @@ func TestIsBranchProtected(t *testing.T) {
 					RequireLinearHistory:    &falseVal,
 					AllowForcePushes:        &falseVal,
 					AllowDeletions:          &falseVal,
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -175,7 +179,9 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: nil,
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required: &falseVal,
+					},
 				},
 			},
 		},
@@ -202,7 +208,9 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: nil,
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required: &falseVal,
+					},
 				},
 			},
 		},
@@ -229,7 +237,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -260,7 +269,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -291,7 +301,6 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  nil,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: nil,
 				},
 			},
 		},
@@ -318,7 +327,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  nil,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          nil,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -349,7 +359,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -380,7 +391,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -412,7 +424,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -443,7 +456,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -474,7 +488,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -505,7 +520,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -537,7 +553,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,

diff --git a/clients/branch.go b/clients/branch.go
@@ -23,11 +23,7 @@ type BranchRef struct {
 
 // BranchProtectionRule captures the settings enabled on a branch for security.
 type BranchProtectionRule struct {
-	// The nil value of this struct can mean either:
-	// 1. we can't tell if PRs are required to make changes; or
-	// 2. we know PRs are not required. The first case happens when Scorecard is run without admin token on
-	// a repo that uses the old Branch Protection setting (not the new Repo Rules, that are always public).
-	RequiredPullRequestReviews *PullRequestReviewRule
+	RequiredPullRequestReviews PullRequestReviewRule
 	AllowDeletions             *bool
 	AllowForcePushes           *bool
 	RequireLinearHistory       *bool
@@ -45,6 +41,7 @@ type StatusChecksRule struct {
 
 // PullRequestReviewRule captures settings on a PullRequest.
 type PullRequestReviewRule struct {
+	Required                     *bool // are PRs required
 	RequiredApprovingReviewCount *int32
 	DismissStaleReviews          *bool
 	RequireCodeOwnerReviews      *bool