Revert the 2 commits with changes around how Scorecard detects admin run

Reverts commit 64c3521 and commit e2662b7. Both had chances around using clients/branch.go scructur to store the information of whether Scorecard was being run by admin or not. We decided to not change this structure for this purpose.
diogoteles08 · Oct 23, 2023 · d7c9d95 · d7c9d95
1 parent 520babd
commit d7c9d95
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 81 deletions.
diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -89,7 +89,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      nil,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -110,7 +109,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      nil,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -150,7 +148,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &main,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -186,7 +183,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &main,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -208,7 +204,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &rel1,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -244,7 +239,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &main,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -266,7 +260,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &rel1,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -303,7 +296,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &main,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -342,7 +334,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &main,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -379,7 +370,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &main,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &falseVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,
@@ -391,7 +381,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				{
 					Name:      &rel1,
 					Protected: &trueVal,
-					WereAllSettingsAvailable: &falseVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
 						CheckRules: clients.StatusChecksRule{
 							RequiresStatusChecks: &trueVal,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -49,10 +49,12 @@ type levelScore struct {
 	maxes  scoresInfo // Maximum possible score for a branch.
 }
 
-// Limited information is normally provided when Scorecard is run wihout admin token and
-// the branch protection was not set using the new Repo Rules(that makes all infos public).
-func wasLimitedInformationProvided(branchProtectionData *clients.BranchRef) bool {
-	return branchProtectionData.WereAllSettingsAvailable == nil || !*branchProtectionData.WereAllSettingsAvailable
+// Evaluates if Scorecard is being run with an administrator token.
+func isUserAdmin(branchProtectionData *clients.BranchRef) bool {
+	// When Scorecard is run without the admin token, Github retrieves both of the following fields as nil,
+	// so we're using them to evaluate if Scorecard is run using admin token or not.
+	return branchProtectionData.BranchProtectionRule.CheckRules.UpToDateBeforeMerge != nil ||
+		branchProtectionData.BranchProtectionRule.RequireLastPushApproval != nil
 }
 
 // BranchProtection runs Branch-Protection check.
@@ -324,8 +326,8 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
 		}
 	}
 
-	if !wasLimitedInformationProvided(branch) {
-		// If we're able to retrieve all information, we can interprete GitHub's response to say
+	if isUserAdmin(branch) {
+		// If Scorecard is run with admin token, we can interprete GitHub's response to say
 		// if the branch requires PRs prior to code changes.
 		max++
 		if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
@@ -387,12 +389,12 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
 	max++
 
 	// On this first check we exclude the case of PRs don't being required, covered on adminReviewProtection function
-	if wasLimitedInformationProvided(branch) ||
-		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
+	if !(isUserAdmin(branch) &&
+		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount == nil) {
 		switch {
 		// If not running as admin, the nil value can both mean that no reviews are required or no PR are required,
 		// so here we assume no reviews are required.
-		case (wasLimitedInformationProvided(branch) &&
+		case (!isUserAdmin(branch) &&
 			branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount == nil) ||
 			*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount == 0:
 			warn(dl, log, "number of required reviewers is 0 on branch '%s'", *branch.Name)

diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -58,9 +58,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					AllowDeletions:          &falseVal,
 					AllowForcePushes:        &falseVal,
@@ -90,9 +89,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 4,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 			},
 		},
 		{
@@ -105,9 +103,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					RequiredPullRequestReviews: clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
@@ -137,9 +134,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &falseVal,
 					RequireLastPushApproval: &falseVal,
@@ -169,9 +165,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: &trueVal,
@@ -201,9 +196,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: &trueVal,
@@ -233,9 +227,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: &trueVal,
@@ -265,9 +258,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 2,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &falseVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: nil,
@@ -297,9 +289,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 2,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &falseVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: nil,
@@ -329,9 +320,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: &falseVal,
@@ -361,9 +351,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &falseVal,
 					RequireLastPushApproval: &falseVal,
@@ -393,9 +382,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &falseVal,
 					RequireLastPushApproval: &falseVal,
@@ -426,9 +414,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &falseVal,
 					RequireLastPushApproval: &falseVal,
@@ -458,9 +445,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLinearHistory:    &trueVal,
@@ -490,9 +476,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLinearHistory:    &trueVal,
@@ -523,9 +508,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
-				Name:                     &branchVal,
-				Protected:                &trueVal,
-				WereAllSettingsAvailable: &trueVal,
+				Name:      &branchVal,
+				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: &falseVal,

diff --git a/clients/branch.go b/clients/branch.go
@@ -16,10 +16,9 @@ package clients
 
 // BranchRef represents a single branch reference and its protection rules.
 type BranchRef struct {
-	Name                     *string
-	Protected                *bool
-	WereAllSettingsAvailable *bool
-	BranchProtectionRule     BranchProtectionRule
+	Name                 *string
+	Protected            *bool
+	BranchProtectionRule BranchProtectionRule
 }
 
 // BranchProtectionRule captures the settings enabled on a branch for security.

diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go
@@ -404,13 +404,11 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef {
 	*branchRef.Protected = true
 	branchRule := &branchRef.BranchProtectionRule
 
-	branchRef.WereAllSettingsAvailable = new(bool)
 	switch {
 	// All settings are available. This typically means
 	// scorecard is run with a token that has access
 	// to admin settings.
 	case data.BranchProtectionRule != nil:
-		*branchRef.WereAllSettingsAvailable = true
 		rule := data.BranchProtectionRule
 
 		// Admin settings.
@@ -422,17 +420,10 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef {
 	// Only non-admin settings are available.
 	// https://docs.github.com/en/graphql/reference/objects#refupdaterule.
 	case data.RefUpdateRule != nil:
-		*branchRef.WereAllSettingsAvailable = false
 		rule := data.RefUpdateRule
 		copyNonAdminSettings(rule, branchRule)
 	}
 
-	// As all settings set by repo rules are public, if any rule apply to
-	// this branch, we can already say we have all settings.
-	if len(rules) > 0 {
-		*branchRef.WereAllSettingsAvailable = true
-	}
-
 	applyRepoRules(branchRef, rules)
 
 	return branchRef