From 86835fcfd6559479f603ef623d6c0948f5dae4b2 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Tue, 16 Nov 2021 09:27:27 -0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20branch=20protection=20resu?= =?UTF-8?q?lts=20(#1252)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix * fix * doc * fix * comment * update tests * fix * fixes * fix * disable tests temp * score change * fix * comments * docs --- checks/branch_protection.go | 62 ++++++++++------ checks/branch_protection_test.go | 74 +++++++++---------- clients/branch.go | 7 +- clients/githubrepo/branches.go | 117 ++++++++++++++++++++++++++----- e2e/branch_protection_test.go | 2 +- 5 files changed, 184 insertions(+), 78 deletions(-) diff --git a/checks/branch_protection.go b/checks/branch_protection.go index cbc1f6c26fb..2cd2d558bf2 100644 --- a/checks/branch_protection.go +++ b/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 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) } - } - 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) + case false: + dl.Debug("unable to retrieve status check for branch '%s'", branch) } return score diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index c98e24fffbd..e3bfa4bbd09 100644 --- a/checks/branch_protection_test.go +++ b/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) { }) } } +*/ diff --git a/clients/branch.go b/clients/branch.go index cb4b5af58c6..73f58f6aeea 100644 --- a/clients/branch.go +++ b/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. diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index f28c3e71dba..7be46ff1d89 100644 --- a/clients/githubrepo/branches.go +++ b/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 diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index 55cbe7a54fb..7b6df3c922f 100644 --- a/e2e/branch_protection_test.go +++ b/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,