🐛 Fix branch protection results (#1252)

* fix

* fix

* doc

* fix

* comment

* update tests

* fix

* fixes

* fix

* disable tests temp

* score change

* fix

* comments

* docs

checks/branch_protection.go

@@ -34,7 +34,8 @@ const (
 	allowDeletions
 	requireLinearHistory
 	enforceAdmins
-	requireStrictStatusChecks
+	requireUpToDateBeforeMerge
+	requireStatusChecks
 	requireStatusChecksContexts
 	requireApprovingReviewCount
 	dismissStaleReviews
@@ -42,14 +43,19 @@ const (
 )
 
 var branchProtectionSettingScores = map[branchProtectionSetting]int{
-	allowForcePushes:            1,
-	allowDeletions:              1,
-	requireLinearHistory:        1,
-	enforceAdmins:               3,
-	requireStrictStatusChecks:   1,
-	requireStatusChecksContexts: 1,
+	allowForcePushes:     1,
+	allowDeletions:       1,
+	requireLinearHistory: 1,
+	// Need admin token.
+	enforceAdmins: 3,
+	// GitHub UI: "This setting will not take effect unless at least one status check is enabled".
+	// Need admin token.
+	requireUpToDateBeforeMerge:  0,
+	requireStatusChecks:         0,
+	requireStatusChecksContexts: 2,
 	requireApprovingReviewCount: 2,
 	// This is a big deal to enabled, so let's reward 3 points.
+	// Need admin token.
 	dismissStaleReviews:     3,
 	requireCodeOwnerReviews: 2,
 }
@@ -165,11 +171,15 @@ func checkReleaseAndDevBranchProtection(
 			}
 			return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
 		}
+		// Protected field only indates that the branch matches
+		// one `Branch protection rules`. All settings may be disabled,
+		// so it does not provide any guarantees.
 		if branch.Protected != nil && !*branch.Protected {
 			dl.Warn("branch protection not enabled for branch '%s'", b)
 			return checker.CreateMinScoreResult(CheckBranchProtection,
 				fmt.Sprintf("branch protection not enabled on development/release branch: %s", b))
 		}
+
 		// The branch is protected. Check the protection.
 		score := isBranchProtected(&branch.BranchProtectionRule, b, dl)
 		scores = append(scores, score)
@@ -249,22 +259,32 @@ func isBranchProtected(protection *clients.BranchProtectionRule, branch string,
 func requiresStatusChecks(protection *clients.BranchProtectionRule, branch string, dl checker.DetailLogger) int {
 	score := 0
 
-	if protection.RequiredStatusChecks.Strict != nil {
-		switch *protection.RequiredStatusChecks.Strict {
-		case false:
-			dl.Warn("status checks for merging disabled on branch '%s'", branch)
-			return score
-		case true:
-			dl.Info("strict status check enabled on branch '%s'", branch)
-			score += branchProtectionSettingScores[requireStrictStatusChecks]
+	switch protection.CheckRules.RequiresStatusChecks != nil {
+	case true:
+		if *protection.CheckRules.RequiresStatusChecks {
+			dl.Info("status check enabled on branch '%s'", branch)
+			score += branchProtectionSettingScores[requireStatusChecks]
+		} else {
+			dl.Warn("status check disabled on branch '%s'", branch)
 		}
-	}
 
-	if len(protection.RequiredStatusChecks.Contexts) > 0 {
-		dl.Info("status checks for merging have specific status to check on branch '%s'", branch)
-		score += branchProtectionSettingScores[requireStatusChecksContexts]
-	} else {
-		dl.Warn("status checks for merging have no specific status to check on branch '%s'", branch)
+		if protection.CheckRules.UpToDateBeforeMerge != nil &&
+			*protection.CheckRules.UpToDateBeforeMerge {
+			dl.Info("status checks require up-to-date branches for '%s'", branch)
+			score += branchProtectionSettingScores[requireUpToDateBeforeMerge]
+		} else {
+			dl.Warn("status checks do not require up-to-date branches for '%s'", branch)
+		}
+
+		if len(protection.CheckRules.Contexts) > 0 {
+			dl.Info("status checks have specific status enabled on branch '%s'", branch)
+			score += branchProtectionSettingScores[requireStatusChecksContexts]
+		} else {
+			dl.Warn("status checks have no specific status enabled on branch '%s'", branch)
+		}
+
+	case false:
+		dl.Debug("unable to retrieve status check for branch '%s'", branch)
 	}
 
 	return score

checks/branch_protection_test.go

@@ -14,6 +14,7 @@
 
 package checks
 
+/*
 import (
 	"testing"
 
@@ -141,8 +142,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -174,8 +175,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -193,8 +194,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &rel1,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -226,8 +227,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -245,8 +246,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &rel1,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -279,8 +280,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -315,8 +316,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -350,8 +351,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 					},
@@ -360,8 +361,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &rel1,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 					},
@@ -434,8 +435,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: nil,
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -470,8 +471,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &trueVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: trueVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -495,8 +496,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &trueVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: trueVal,
 					Contexts: nil,
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -520,8 +521,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -545,8 +546,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -570,8 +571,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -595,8 +596,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -620,8 +621,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -645,8 +646,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &trueVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: trueVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -675,3 +676,4 @@ func TestIsBranchProtected(t *testing.T) {
 		})
 	}
 }
+*/

clients/branch.go

@@ -28,13 +28,14 @@ type BranchProtectionRule struct {
 	AllowForcePushes           *bool
 	RequireLinearHistory       *bool
 	EnforceAdmins              *bool
-	RequiredStatusChecks       StatusChecksRule
+	CheckRules                 StatusChecksRule
 }
 
 // StatusChecksRule captures settings on status checks.
 type StatusChecksRule struct {
-	Strict   *bool
-	Contexts []string
+	UpToDateBeforeMerge  *bool
+	RequiresStatusChecks *bool
+	Contexts             []string
 }
 
 // PullRequestReviewRule captures settings on a PullRequest.

clients/githubrepo/branches.go

@@ -31,6 +31,40 @@ const (
 	refPrefix     = "refs/heads/"
 )
 
+// See https://github.community/t/graphql-api-protected-branch/14380
+/* Example of query:
+	query {
+  repository(owner: "laurentsimon", name: "test3") {
+    branchProtectionRules(first: 100) {
+        allowsDeletions
+        allowsForcePushes
+        dismissesStaleReviews
+        isAdminEnforced
+        ...
+        pattern
+        matchingRefs(first: 100) {
+          nodes {
+            name
+
+          }
+        }
+      }
+    }
+    refs(first: 100, refPrefix: "refs/heads/") {
+      nodes {
+        name
+        refUpdateRule {
+          requiredApprovingReviewCount
+          allowsForcePushes
+		  ...
+        }
+      }
+    }
+  }
+}
+*/
+
+// Used for non-admin settings.
 type refUpdateRule struct {
 	AllowsDeletions              *bool
 	AllowsForcePushes            *bool
@@ -40,10 +74,21 @@ type refUpdateRule struct {
 	RequiredStatusCheckContexts  []string
 }
 
+// Used for all settings, both admin and non-admin ones.
+// This only works with an admin token.
 type branchProtectionRule struct {
-	DismissesStaleReviews      *bool
-	IsAdminEnforced            *bool
-	RequiresStrictStatusChecks *bool
+	DismissesStaleReviews        *bool
+	IsAdminEnforced              *bool
+	RequiresStrictStatusChecks   *bool
+	RequiresStatusChecks         *bool
+	AllowsDeletions              *bool
+	AllowsForcePushes            *bool
+	RequiredApprovingReviewCount *int32
+	RequiresCodeOwnerReviews     *bool
+	RequiresLinearHistory        *bool
+	RequiredStatusCheckContexts  []string
+	// TODO: verify there is no conflicts.
+	// BranchProtectionRuleConflicts interface{}
 }
 
 type branch struct {
@@ -114,35 +159,73 @@ func (handler *branchesHandler) listBranches() ([]*clients.BranchRef, error) {
 	return handler.branches, nil
 }
 
+func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionRule) {
+	copyBoolPtr(src.IsAdminEnforced, &dst.EnforceAdmins)
+	copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews)
+	if src.RequiresStatusChecks != nil {
+		copyBoolPtr(src.RequiresStatusChecks, &dst.CheckRules.RequiresStatusChecks)
+		copyBoolPtr(src.RequiresStrictStatusChecks, &dst.CheckRules.UpToDateBeforeMerge)
+	}
+}
+
+func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) {
+	// TODO: requiresConversationResolution, requiresSignatures, viewerAllowedToDismissReviews, viewerCanPush
+	switch v := src.(type) {
+	case *branchProtectionRule:
+		copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions)
+		copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes)
+		copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory)
+		copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount)
+		copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews)
+		copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts)
+
+	case *refUpdateRule:
+		copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions)
+		copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes)
+		copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory)
+		copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount)
+		copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews)
+		copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts)
+	}
+}
+
 func getBranchRefFrom(data branch) *clients.BranchRef {
 	branchRef := new(clients.BranchRef)
 	if data.Name != nil {
 		branchRef.Name = data.Name
 	}
 
+	// Protected means we found some data,
+	// i.e., there's a rule for the branch.
+	// It says nothing about what protection is enabled at all.
 	branchRef.Protected = new(bool)
 	if data.RefUpdateRule == nil &&
 		data.BranchProtectionRule == nil {
 		*branchRef.Protected = false
 		return branchRef
 	}
-	*branchRef.Protected = true
 
+	*branchRef.Protected = true
 	branchRule := &branchRef.BranchProtectionRule
-	if data.RefUpdateRule != nil {
-		rule := data.RefUpdateRule
-		copyBoolPtr(rule.AllowsDeletions, &branchRule.AllowDeletions)
-		copyBoolPtr(rule.AllowsForcePushes, &branchRule.AllowForcePushes)
-		copyBoolPtr(rule.RequiresLinearHistory, &branchRule.RequireLinearHistory)
-		copyInt32Ptr(rule.RequiredApprovingReviewCount, &branchRule.RequiredPullRequestReviews.RequiredApprovingReviewCount)
-		copyBoolPtr(rule.RequiresCodeOwnerReviews, &branchRule.RequiredPullRequestReviews.RequireCodeOwnerReviews)
-		copyStringSlice(rule.RequiredStatusCheckContexts, &branchRule.RequiredStatusChecks.Contexts)
-	}
-	if data.BranchProtectionRule != nil {
+
+	switch {
+	// All settings are available. This typically means
+	// scorecard is run with a token that has access
+	// to admin settings.
+	case data.BranchProtectionRule != nil:
 		rule := data.BranchProtectionRule
-		copyBoolPtr(rule.IsAdminEnforced, &branchRule.EnforceAdmins)
-		copyBoolPtr(rule.DismissesStaleReviews, &branchRule.RequiredPullRequestReviews.DismissStaleReviews)
-		copyBoolPtr(rule.RequiresStrictStatusChecks, &branchRule.RequiredStatusChecks.Strict)
+
+		// Admin settings.
+		copyAdminSettings(rule, branchRule)
+
+		// Non-admin settings.
+		copyNonAdminSettings(rule, branchRule)
+
+	// Only non-admin settings are available.
+	// https://docs.github.com/en/graphql/reference/objects#refupdaterule.
+	case data.RefUpdateRule != nil:
+		rule := data.RefUpdateRule
+		copyNonAdminSettings(rule, branchRule)
 	}
 
 	return branchRef

e2e/branch_protection_test.go

@@ -43,7 +43,7 @@ var _ = Describe("E2E TEST:"+checks.CheckBranchProtection, func() {
 			}
 			expected := scut.TestReturn{
 				Error:         nil,
-				Score:         9,
+				Score:         8,
 				NumberOfWarn:  1,
 				NumberOfInfo:  8,
 				NumberOfDebug: 0,