diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index c96a902b411..69c0ddd35ff 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -86,7 +86,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { defaultBranch: main, branches: []*clients.BranchRef{ { - Name: nil, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ CheckRules: clients.StatusChecksRule{ @@ -94,7 +93,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -106,7 +105,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { }, }, { - Name: nil, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ CheckRules: clients.StatusChecksRule{ @@ -114,7 +112,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -135,7 +133,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Error: nil, Score: 3, NumberOfWarn: 7, - NumberOfInfo: 2, + NumberOfInfo: 3, NumberOfDebug: 0, }, defaultBranch: main, @@ -153,7 +151,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -174,7 +172,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Error: nil, Score: 4, NumberOfWarn: 9, - NumberOfInfo: 10, + NumberOfInfo: 12, NumberOfDebug: 0, }, defaultBranch: main, @@ -188,7 +186,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -209,7 +207,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -230,7 +228,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 4, - NumberOfInfo: 16, + NumberOfInfo: 18, NumberOfDebug: 0, }, defaultBranch: main, @@ -244,7 +242,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -265,7 +263,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -286,7 +284,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Error: nil, Score: 3, NumberOfWarn: 7, - NumberOfInfo: 2, + NumberOfInfo: 3, NumberOfDebug: 0, }, defaultBranch: main, @@ -301,7 +299,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -339,7 +337,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -357,7 +355,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 0, - NumberOfWarn: 4, + NumberOfWarn: 6, NumberOfInfo: 0, NumberOfDebug: 8, }, diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 1a66347c663..21352fb789a 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -49,6 +49,16 @@ type levelScore struct { maxes scoresInfo // Maximum possible score for a branch. } +type tier uint8 + +const ( + Tier1 tier = iota + Tier2 + Tier3 + Tier4 + Tier5 +) + // BranchProtection runs Branch-Protection check. func BranchProtection(name string, dl checker.DetailLogger, r *checker.BranchProtectionsData, @@ -85,7 +95,7 @@ func BranchProtection(name string, dl checker.DetailLogger, return checker.CreateInconclusiveResult(name, "unable to detect any development/release branches") } - score, err := computeScore(scores) + score, err := computeFinalScore(scores) if err != nil { return checker.CreateRuntimeErrorResult(name, err) } @@ -103,77 +113,34 @@ func BranchProtection(name string, dl checker.DetailLogger, } } -func computeNonAdminBasicScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.basic - } - return score -} - -func computeNonAdminReviewScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.review - } - return score -} - -func computeAdminReviewScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.adminReview - } - return score -} - -func computeNonAdminThoroughReviewScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.thoroughReview - } - return score -} - -func computeAdminThoroughReviewScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.adminThoroughReview - } - return score -} - -func computeNonAdminContextScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.context - } - return score -} - -func computeCodeownerThoroughReviewScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.codeownerReview +func sumUpScoreForTier(t tier, scoresData []levelScore) int { + sum := 0 + for i := range scoresData { + score := scoresData[i] + switch t { + case Tier1: + sum += score.scores.basic + case Tier2: + sum += score.scores.review + score.scores.adminReview + case Tier3: + sum += score.scores.context + case Tier4: + sum += score.scores.thoroughReview + score.scores.codeownerReview + case Tier5: + sum += score.scores.adminThoroughReview + } } - return score + return sum } -func noarmalizeScore(score, max, level int) float64 { +func normalizeScore(score, max, level int) float64 { if max == 0 { return float64(level) } return float64(score*level) / float64(max) } -func computeScore(scores []levelScore) (int, error) { +func computeFinalScore(scores []levelScore) (int, error) { if len(scores) == 0 { return 0, sce.WithMessage(sce.ErrScorecardInternal, "scores are empty") } @@ -183,28 +150,26 @@ func computeScore(scores []levelScore) (int, error) { // First, check if they all pass the basic (admin and non-admin) checks. maxBasicScore := maxScore.basic * len(scores) - basicScore := computeNonAdminBasicScore(scores) - score += noarmalizeScore(basicScore, maxBasicScore, basicLevel) - if basicScore != maxBasicScore { + basicScore := sumUpScoreForTier(Tier1, scores) + score += normalizeScore(basicScore, maxBasicScore, basicLevel) + if basicScore < maxBasicScore { return int(score), nil } // Second, check the (admin and non-admin) reviews. maxReviewScore := maxScore.review * len(scores) maxAdminReviewScore := maxScore.adminReview * len(scores) - reviewScore := computeNonAdminReviewScore(scores) - adminReviewScore := computeAdminReviewScore(scores) - score += noarmalizeScore(reviewScore+adminReviewScore, maxReviewScore+maxAdminReviewScore, adminNonAdminReviewLevel) - if reviewScore != maxReviewScore || - adminReviewScore != maxAdminReviewScore { + adminNonAdminReviewScore := sumUpScoreForTier(Tier2, scores) + score += normalizeScore(adminNonAdminReviewScore, maxReviewScore+maxAdminReviewScore, adminNonAdminReviewLevel) + if adminNonAdminReviewScore < maxReviewScore+maxAdminReviewScore { return int(score), nil } // Third, check the use of non-admin context. maxContextScore := maxScore.context * len(scores) - contextScore := computeNonAdminContextScore(scores) - score += noarmalizeScore(contextScore, maxContextScore, nonAdminContextLevel) - if contextScore != maxContextScore { + contextScore := sumUpScoreForTier(Tier3, scores) + score += normalizeScore(contextScore, maxContextScore, nonAdminContextLevel) + if contextScore < maxContextScore { return int(score), nil } @@ -212,11 +177,9 @@ func computeScore(scores []levelScore) (int, error) { // Also check whether this repo requires codeowner review maxThoroughReviewScore := maxScore.thoroughReview * len(scores) maxCodeownerReviewScore := maxScore.codeownerReview * len(scores) - thoroughReviewScore := computeNonAdminThoroughReviewScore(scores) - codeownerReviewScore := computeCodeownerThoroughReviewScore(scores) - score += noarmalizeScore(thoroughReviewScore+codeownerReviewScore, maxThoroughReviewScore+maxCodeownerReviewScore, - nonAdminThoroughReviewLevel) - if thoroughReviewScore != maxThoroughReviewScore { + tier4Score := sumUpScoreForTier(Tier4, scores) + score += normalizeScore(tier4Score, maxThoroughReviewScore+maxCodeownerReviewScore, nonAdminThoroughReviewLevel) + if tier4Score < maxThoroughReviewScore+maxCodeownerReviewScore { return int(score), nil } @@ -224,8 +187,8 @@ func computeScore(scores []levelScore) (int, error) { // This one is controversial and has usability issues // https://github.com/ossf/scorecard/issues/1027, so we may remove it. maxAdminThoroughReviewScore := maxScore.adminThoroughReview * len(scores) - adminThoroughReviewScore := computeAdminThoroughReviewScore(scores) - score += noarmalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel) + adminThoroughReviewScore := sumUpScoreForTier(Tier5, scores) + score += normalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel) if adminThoroughReviewScore != maxAdminThoroughReviewScore { return int(score), nil } @@ -319,11 +282,14 @@ func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) { score := 0 max := 0 - max++ - if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil && + // 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 && *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 { // We do not display anything here, it's done in nonAdminThoroughReviewProtection() - score++ + score += reviewerWeight } return score, max } @@ -362,6 +328,18 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) ( } } + max++ + if branch.BranchProtectionRule.RequiredPullRequestReviews != nil { + 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 } @@ -371,8 +349,9 @@ 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.DismissStaleReviews != nil { - // Note: we don't inrecase max possible score for non-admin viewers. + if branch.BranchProtectionRule.RequiredPullRequestReviews != nil && + branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil { + // Note: we don't increase max possible score for non-admin viewers. max++ switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews { case true: @@ -411,19 +390,21 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta log := branch.Protected != nil && *branch.Protected max++ - if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil { - switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews { - case true: + + // 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++ - default: - warn(dl, log, "number of required reviewers is only %d on branch '%s'", - *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name) + } 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) } - } else { - warn(dl, log, "number of required reviewers is 0 on branch '%s'", *branch.Name) } + return score, max } @@ -435,7 +416,8 @@ func codeownerBranchProtection( log := branch.Protected != nil && *branch.Protected - if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil { + if branch.BranchProtectionRule.RequiredPullRequestReviews != nil && + 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 index 2b45d116714..db25faf236b 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -32,9 +32,10 @@ func testScore(branch *clients.BranchRef, codeownersFiles []string, dl checker.D score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(branch, dl) score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(branch, codeownersFiles, dl) - return computeScore([]levelScore{score}) + return computeFinalScore([]levelScore{score}) } +// TODO: order of tests to have progressive scores. func TestIsBranchProtected(t *testing.T) { t.Parallel() trueVal := true @@ -49,28 +50,24 @@ func TestIsBranchProtected(t *testing.T) { expected scut.TestReturn }{ { - name: "Nothing is enabled", + name: "Configs as they are right after creating new Branch Protection setting", expected: scut.TestReturn{ Error: nil, Score: 3, - NumberOfWarn: 7, + NumberOfWarn: 6, NumberOfInfo: 2, - NumberOfDebug: 0, + NumberOfDebug: 1, }, branch: &clients.BranchRef{ Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - }, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequiredPullRequestReviews: nil, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &trueVal, Contexts: nil, @@ -84,7 +81,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 0, - NumberOfWarn: 2, + NumberOfWarn: 3, NumberOfInfo: 0, NumberOfDebug: 4, }, @@ -99,14 +96,14 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 4, NumberOfWarn: 5, - NumberOfInfo: 4, + NumberOfInfo: 5, NumberOfDebug: 0, }, branch: &clients.BranchRef{ Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -130,7 +127,7 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 4, NumberOfWarn: 6, - NumberOfInfo: 3, + NumberOfInfo: 4, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -142,7 +139,7 @@ func TestIsBranchProtected(t *testing.T) { RequireLinearHistory: &falseVal, AllowForcePushes: &falseVal, AllowDeletions: &falseVal, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -156,13 +153,13 @@ func TestIsBranchProtected(t *testing.T) { }, }, { - name: "Required pull request enabled", + name: "Admin run only preventing force pushes and deletions", expected: scut.TestReturn{ Error: nil, - Score: 4, + Score: 3, NumberOfWarn: 6, - NumberOfInfo: 3, - NumberOfDebug: 0, + NumberOfInfo: 2, + NumberOfDebug: 1, }, branch: &clients.BranchRef{ Name: &branchVal, @@ -170,15 +167,100 @@ func TestIsBranchProtected(t *testing.T) { BranchProtectionRule: clients.BranchProtectionRule{ EnforceAdmins: &falseVal, RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &falseVal, + Contexts: nil, + }, + RequiredPullRequestReviews: nil, + }, + }, + }, + { + name: "Admin run with all tier 2 requirements except require PRs and reviewers", + expected: scut.TestReturn{ + Error: nil, + Score: 4, // Should be 4.2 if we allow decimal puctuation + NumberOfWarn: 2, + NumberOfInfo: 6, + NumberOfDebug: 1, + }, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &trueVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: nil, + }, + }, + }, + { + name: "Admin run on project requiring pull requests but without approver -- best a single maintainer can do", + expected: scut.TestReturn{ + Error: nil, + Score: 4, // Should be 4.8 if we allow decimal punctuation + NumberOfWarn: 2, + NumberOfInfo: 9, + NumberOfDebug: 0, + }, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, RequireLinearHistory: &trueVal, AllowForcePushes: &falseVal, AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &trueVal, - UpToDateBeforeMerge: &falseVal, + UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + DismissStaleReviews: &trueVal, + RequireCodeOwnerReviews: &trueVal, + RequiredApprovingReviewCount: &zeroVal, + }, + }, + }, + }, + { + name: "Admin run on project with all tier 2 requirements", + expected: scut.TestReturn{ + Error: nil, + Score: 6, + NumberOfWarn: 4, + NumberOfInfo: 6, + NumberOfDebug: 0, + }, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &trueVal, + Contexts: nil, + }, + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &oneVal, @@ -186,13 +268,71 @@ func TestIsBranchProtected(t *testing.T) { }, }, }, + { + name: "Non-admin run on project that require zero reviewer (or don't require PRs at all, we can't differentiate it)", + expected: scut.TestReturn{ + Error: nil, + Score: 3, + NumberOfWarn: 3, + NumberOfInfo: 2, + NumberOfDebug: 4, + }, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: nil, + RequireLastPushApproval: nil, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: nil, + UpToDateBeforeMerge: nil, + Contexts: nil, + }, + RequiredPullRequestReviews: nil, + }, + }, + }, + { + name: "Non-admin run on project that require 1 reviewer", + expected: scut.TestReturn{ + Error: nil, + Score: 6, + NumberOfWarn: 3, + NumberOfInfo: 3, + NumberOfDebug: 4, + }, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: nil, + RequireLastPushApproval: nil, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: nil, + UpToDateBeforeMerge: nil, + Contexts: nil, + }, + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + DismissStaleReviews: nil, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &oneVal, + }, + }, + }, + }, { name: "Required admin enforcement enabled", expected: scut.TestReturn{ Error: nil, Score: 3, NumberOfWarn: 5, - NumberOfInfo: 4, + NumberOfInfo: 5, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -209,7 +349,7 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -223,7 +363,7 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 3, NumberOfWarn: 6, - NumberOfInfo: 3, + NumberOfInfo: 4, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -240,7 +380,7 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -254,7 +394,7 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 1, NumberOfWarn: 7, - NumberOfInfo: 2, + NumberOfInfo: 3, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -272,7 +412,7 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -286,7 +426,7 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 1, NumberOfWarn: 7, - NumberOfInfo: 2, + NumberOfInfo: 3, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -303,7 +443,7 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -317,7 +457,7 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 2, - NumberOfInfo: 8, + NumberOfInfo: 9, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -334,7 +474,7 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -348,7 +488,7 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 1, - NumberOfInfo: 8, + NumberOfInfo: 9, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -365,7 +505,7 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -380,7 +520,7 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 5, NumberOfWarn: 3, - NumberOfInfo: 7, + NumberOfInfo: 8, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -397,7 +537,7 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, diff --git a/clients/branch.go b/clients/branch.go index ff11e4a683e..4cd6b3d4217 100644 --- a/clients/branch.go +++ b/clients/branch.go @@ -23,7 +23,11 @@ type BranchRef struct { // BranchProtectionRule captures the settings enabled on a branch for security. type BranchProtectionRule struct { - RequiredPullRequestReviews PullRequestReviewRule + // 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 AllowDeletions *bool AllowForcePushes *bool RequireLinearHistory *bool diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 115c21964b6..b8ba122ef8e 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -323,10 +323,10 @@ func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, er return branchRef, nil } +// TODO: Move these two functions to below the GetBranchRefFrom functions, the single place they're used. func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionRule) { copyBoolPtr(src.IsAdminEnforced, &dst.EnforceAdmins) copyBoolPtr(src.RequireLastPushApproval, &dst.RequireLastPushApproval) - copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) if src.RequiresStatusChecks != nil { copyBoolPtr(src.RequiresStatusChecks, &dst.CheckRules.RequiresStatusChecks) // TODO(#3255): Update when GitHub GraphQL bug is fixed @@ -340,6 +340,15 @@ func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionR copyBoolPtr(&upToDateBeforeMerge, &dst.CheckRules.UpToDateBeforeMerge) } } + + // We're also checking if branch requires PRs because if it doesn't, + // the RequiredPullRequestReviews struct should be nil. + if src.DismissesStaleReviews != nil && branchRequiresPrs(src) { + if dst.RequiredPullRequestReviews == nil { + dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) + } + copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) + } } func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { @@ -349,20 +358,39 @@ func copyNonAdminSettings(src interface{}, dst *clients.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) + // If branch doesn't require PRs to make changes, we let the struct RequiredPullRequestReviews point to nil + if branchRequiresPrs(v) { + if dst.RequiredPullRequestReviews == nil { + dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) + } + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + } + 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) + + // Evaluate if we have data to infer that the project requires PRs to make changes. If we don't have data, we let + // the struct RequiredPullRequestReviews as nil + if valueOrZero(v.RequiredApprovingReviewCount) > 0 || valueOrZero(v.RequiresCodeOwnerReviews) { + if dst.RequiredPullRequestReviews == nil { + dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) + } + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + } } } +func branchRequiresPrs(data *branchProtectionRule) bool { + return data.RequiredApprovingReviewCount != nil +} + func getDefaultBranchNameFrom(data *ruleSetData) string { if data == nil || data.Repository.DefaultBranchRef.Name == nil { return "" @@ -411,12 +439,12 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { case data.BranchProtectionRule != nil: rule := data.BranchProtectionRule - // Admin settings. - copyAdminSettings(rule, branchRule) - // Non-admin settings. copyNonAdminSettings(rule, branchRule) + // Admin settings. + copyAdminSettings(rule, branchRule) + // Only non-admin settings are available. // https://docs.github.com/en/graphql/reference/objects#refupdaterule. case data.RefUpdateRule != nil: @@ -477,22 +505,24 @@ nextRule: } func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { - falseVal := false - trueVal := true for _, r := range rules { - adminEnforced := len(r.BypassActors.Nodes) == 0 + // Init values of base checkbox as if they're unchecked translated := clients.BranchProtectionRule{ - EnforceAdmins: &adminEnforced, + AllowDeletions: asPtr(true), + AllowForcePushes: asPtr(true), + RequireLinearHistory: asPtr(false), } + translated.EnforceAdmins = asPtr(len(r.BypassActors.Nodes) == 0) + for _, rule := range r.Rules.Nodes { switch rule.Type { case ruleDeletion: - translated.AllowDeletions = &falseVal + translated.AllowDeletions = asPtr(false) case ruleForcePush: - translated.AllowForcePushes = &falseVal + translated.AllowForcePushes = asPtr(false) case ruleLinear: - translated.RequireLinearHistory = &trueVal + translated.RequireLinearHistory = asPtr(true) case rulePullRequest: translatePullRequestRepoRule(&translated, rule) case ruleStatusCheck: @@ -504,18 +534,13 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { } func translatePullRequestRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { - if readBoolPtr(rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush) { - base.RequiredPullRequestReviews.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush - } - if readBoolPtr(rule.Parameters.PullRequestParameters.RequireCodeOwnerReview) { - base.RequiredPullRequestReviews.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview - } - if readBoolPtr(rule.Parameters.PullRequestParameters.RequireLastPushApproval) { - base.RequireLastPushApproval = rule.Parameters.PullRequestParameters.RequireLastPushApproval - } - if reviewerCount := readIntPtr(rule.Parameters.PullRequestParameters.RequiredApprovingReviewCount); reviewerCount > 0 { - base.RequiredPullRequestReviews.RequiredApprovingReviewCount = &reviewerCount - } + base.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) + + base.RequiredPullRequestReviews.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush + base.RequiredPullRequestReviews.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview + base.RequireLastPushApproval = rule.Parameters.PullRequestParameters.RequireLastPushApproval + base.RequiredPullRequestReviews.RequiredApprovingReviewCount = rule.Parameters.PullRequestParameters. + RequiredApprovingReviewCount } func translateRequiredStatusRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { @@ -523,8 +548,7 @@ func translateRequiredStatusRepoRule(base *clients.BranchProtectionRule, rule *r if len(statusParams.RequiredStatusChecks) == 0 { return } - enabled := true - base.CheckRules.RequiresStatusChecks = &enabled + base.CheckRules.RequiresStatusChecks = asPtr(true) base.CheckRules.UpToDateBeforeMerge = statusParams.StrictRequiredStatusChecksPolicy for _, chk := range statusParams.RequiredStatusChecks { if chk.Context == nil { @@ -534,14 +558,17 @@ func translateRequiredStatusRepoRule(base *clients.BranchProtectionRule, rule *r } } +// Merge strategy: +// - if both are nil, keep it nil +// - if any of them is not nil, keep the most restrictive one func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) { - if base.AllowDeletions == nil || translated.AllowDeletions != nil && !*translated.AllowDeletions { + if base.AllowDeletions == nil || (translated.AllowDeletions != nil && !*translated.AllowDeletions) { base.AllowDeletions = translated.AllowDeletions } - if base.AllowForcePushes == nil || translated.AllowForcePushes != nil && !*translated.AllowForcePushes { + if base.AllowForcePushes == nil || (translated.AllowForcePushes != nil && !*translated.AllowForcePushes) { base.AllowForcePushes = translated.AllowForcePushes } - if base.EnforceAdmins == nil || translated.EnforceAdmins != nil && !*translated.EnforceAdmins { + if base.EnforceAdmins == nil || (translated.EnforceAdmins != nil && !*translated.EnforceAdmins) { // this is an over simplification to get preliminary support for repo rules merged. // A more complete approach would process all rules without bypass actors first, // then process those with bypass actors. If no settings improve (due to rule layering), @@ -549,21 +576,21 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) // https://github.com/ossf/scorecard/issues/3480 base.EnforceAdmins = translated.EnforceAdmins } - if base.RequireLastPushApproval == nil || readBoolPtr(translated.RequireLastPushApproval) { + if base.RequireLastPushApproval == nil || valueOrZero(translated.RequireLastPushApproval) { base.RequireLastPushApproval = translated.RequireLastPushApproval } - if base.RequireLinearHistory == nil || readBoolPtr(translated.RequireLinearHistory) { + if base.RequireLinearHistory == nil || valueOrZero(translated.RequireLinearHistory) { base.RequireLinearHistory = translated.RequireLinearHistory } - mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews) mergeCheckRules(&base.CheckRules, &translated.CheckRules) + mergePullRequestReviews(&base.RequiredPullRequestReviews, translated.RequiredPullRequestReviews) } func mergeCheckRules(base, translated *clients.StatusChecksRule) { - if base.UpToDateBeforeMerge == nil || readBoolPtr(translated.UpToDateBeforeMerge) { + if base.UpToDateBeforeMerge == nil || valueOrZero(translated.UpToDateBeforeMerge) { base.UpToDateBeforeMerge = translated.UpToDateBeforeMerge } - if base.RequiresStatusChecks == nil || readBoolPtr(translated.RequiresStatusChecks) { + if base.RequiresStatusChecks == nil || valueOrZero(translated.RequiresStatusChecks) { base.RequiresStatusChecks = translated.RequiresStatusChecks } for _, context := range translated.Contexts { @@ -574,28 +601,39 @@ func mergeCheckRules(base, translated *clients.StatusChecksRule) { } } -func mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) { - if readIntPtr(translated.RequiredApprovingReviewCount) > readIntPtr(base.RequiredApprovingReviewCount) { - base.RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount +func mergePullRequestReviews(base **clients.PullRequestReviewRule, translated *clients.PullRequestReviewRule) { + switch { + case translated == nil: + // "translated" have nothing to be merged in base + return + case *base == nil: + // result would be "translated" itself + *base = translated + return } - if base.DismissStaleReviews == nil || readBoolPtr(translated.DismissStaleReviews) { - base.DismissStaleReviews = translated.DismissStaleReviews + + if (*base).RequiredApprovingReviewCount == nil || + valueOrZero((*base).RequiredApprovingReviewCount) < valueOrZero(translated.RequiredApprovingReviewCount) { + (*base).RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount + } + if (*base).DismissStaleReviews == nil || valueOrZero(translated.DismissStaleReviews) { + (*base).DismissStaleReviews = translated.DismissStaleReviews } - if base.RequireCodeOwnerReviews == nil || readBoolPtr(translated.RequireCodeOwnerReviews) { - base.RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews + if (*base).RequireCodeOwnerReviews == nil || valueOrZero(translated.RequireCodeOwnerReviews) { + (*base).RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews } } -func readBoolPtr(b *bool) bool { - if b == nil { - return false - } - return *b +// returns a pointer to the given value. Useful for constant values. +func asPtr[T any](value T) *T { + return &value } -func readIntPtr(i *int32) int32 { - if i == nil { - return 0 +// returns the pointer's value if it exists, the type's zero-value otherwise. +func valueOrZero[T any](ptr *T) T { + if ptr == nil { + var zero T + return zero } - return *i + return *ptr } diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 73a9401e141..d54b79ce750 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -180,34 +180,68 @@ func Test_applyRepoRules(t *testing.T) { t.Parallel() trueVal := true falseVal := false + zeroVal := int32(0) twoVal := int32(2) testcases := []struct { base *clients.BranchRef - ruleSet *repoRuleSet expected *clients.BranchRef ruleBypass *ruleSetBypass name string + ruleSets []*repoRuleSet }{ { - name: "block deletion no bypass", - base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion})), + name: "unchecked checkboxes have consistent values", + base: &clients.BranchRef{}, + ruleSets: []*repoRuleSet{ + ruleSet(), + }, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &trueVal, + AllowForcePushes: &trueVal, + CheckRules: clients.StatusChecksRule{ + // nil values mean that the CheckRules checkbox wasn't checked + UpToDateBeforeMerge: nil, + RequiresStatusChecks: nil, + Contexts: nil, + }, + EnforceAdmins: &trueVal, + RequireLastPushApproval: nil, // this checkbox is enabled only if require status checks + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: nil, + }, + }, + }, + { + name: "block deletion no bypass", + base: &clients.BranchRef{}, + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{Type: ruleDeletion})), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - EnforceAdmins: &trueVal, + AllowDeletions: &falseVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &trueVal, + RequiredPullRequestReviews: nil, }, }, }, { - name: "block deletion with bypass", - base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion}), withBypass()), + name: "block deletion with bypass", + base: &clients.BranchRef{}, + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{Type: ruleDeletion}), withBypass()), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - EnforceAdmins: &falseVal, + AllowDeletions: &falseVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequiredPullRequestReviews: nil, }, }, }, @@ -219,12 +253,16 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, }, }, - ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion}, &repoRule{Type: ruleForcePush}), withBypass()), + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{Type: ruleDeletion}, &repoRule{Type: ruleForcePush}), withBypass()), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: nil, }, }, }, @@ -232,16 +270,21 @@ func Test_applyRepoRules(t *testing.T) { name: "block deletion no bypass while force push is blocked with bypass", base: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowForcePushes: &falseVal, - EnforceAdmins: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, + RequireLinearHistory: &falseVal, }, }, - ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion})), + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{Type: ruleDeletion})), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &falseVal, // Maintain: deletion enforces but forcepush does not + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Maintain: deletion enforces but forcepush does not + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: nil, }, }, }, @@ -253,57 +296,103 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, }, }, - ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion})), + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{Type: ruleDeletion})), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: nil, }, }, }, { - name: "block force push no bypass", - base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: ruleForcePush})), + name: "block force push no bypass", + base: &clients.BranchRef{}, + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{Type: ruleForcePush})), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, + AllowDeletions: &trueVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: nil, }, }, }, { - name: "require linear history no bypass", - base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: ruleLinear})), + name: "require linear history no bypass", + base: &clients.BranchRef{}, + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{Type: ruleLinear})), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - RequireLinearHistory: &trueVal, - EnforceAdmins: &trueVal, + AllowDeletions: &trueVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &trueVal, + EnforceAdmins: &trueVal, + RequiredPullRequestReviews: nil, }, }, }, { - name: "require pull request no bypass", + name: "require pull request but no reviewers and no bypass", base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{ - Type: rulePullRequest, - Parameters: repoRulesParameters{ - PullRequestParameters: pullRequestRuleParameters{ - DismissStaleReviewsOnPush: &trueVal, - RequireCodeOwnerReview: &trueVal, - RequireLastPushApproval: &trueVal, - RequiredApprovingReviewCount: &twoVal, - RequiredReviewThreadResolution: &trueVal, + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{ + Type: rulePullRequest, + Parameters: repoRulesParameters{ + PullRequestParameters: pullRequestRuleParameters{ + RequireLastPushApproval: asPtr(true), + RequiredApprovingReviewCount: &zeroVal, + }, + }, + })), + }, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &trueVal, + AllowForcePushes: &trueVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredApprovingReviewCount: &zeroVal, }, }, - })), + }, + }, + { + name: "require pull request with 2 reviewers no bypass", + base: &clients.BranchRef{}, + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{ + Type: rulePullRequest, + Parameters: repoRulesParameters{ + PullRequestParameters: pullRequestRuleParameters{ + DismissStaleReviewsOnPush: &trueVal, + RequireCodeOwnerReview: &trueVal, + RequireLastPushApproval: &trueVal, + RequiredApprovingReviewCount: &twoVal, + RequiredReviewThreadResolution: &trueVal, + }, + }, + })), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &trueVal, + AllowForcePushes: &trueVal, EnforceAdmins: &trueVal, + RequireLinearHistory: &falseVal, RequireLastPushApproval: &trueVal, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &twoVal, @@ -314,27 +403,103 @@ func Test_applyRepoRules(t *testing.T) { { name: "required status checks no bypass", base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{ - Type: ruleStatusCheck, - Parameters: repoRulesParameters{ - StatusCheckParameters: requiredStatusCheckParameters{ - StrictRequiredStatusChecksPolicy: &trueVal, - RequiredStatusChecks: []statusCheck{ - { - Context: stringPtr("foo"), + ruleSets: []*repoRuleSet{ + ruleSet(withRules(&repoRule{ + Type: ruleStatusCheck, + Parameters: repoRulesParameters{ + StatusCheckParameters: requiredStatusCheckParameters{ + StrictRequiredStatusChecksPolicy: &trueVal, + RequiredStatusChecks: []statusCheck{ + { + Context: asPtr("foo"), + }, }, }, }, - }, - })), + })), + }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, + AllowDeletions: &trueVal, + AllowForcePushes: &trueVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &falseVal, CheckRules: clients.StatusChecksRule{ UpToDateBeforeMerge: &trueVal, RequiresStatusChecks: &trueVal, Contexts: []string{"foo"}, }, + RequiredPullRequestReviews: nil, + }, + }, + }, + { + name: "Multiple rules sets impacting a branch", + base: &clients.BranchRef{}, + ruleSets: []*repoRuleSet{ + ruleSet(withRules( // first a restrictive rule set, let's suppose it was built only for main. + &repoRule{Type: ruleDeletion}, + &repoRule{Type: ruleForcePush}, + &repoRule{Type: ruleLinear}, + &repoRule{ + Type: ruleStatusCheck, + Parameters: repoRulesParameters{ + StatusCheckParameters: requiredStatusCheckParameters{ + StrictRequiredStatusChecksPolicy: &trueVal, + RequiredStatusChecks: []statusCheck{ + { + Context: asPtr("foo"), + }, + }, + }, + }, + }, + &repoRule{ + Type: rulePullRequest, + Parameters: repoRulesParameters{ + PullRequestParameters: pullRequestRuleParameters{ + DismissStaleReviewsOnPush: &trueVal, + RequireCodeOwnerReview: &trueVal, + RequireLastPushApproval: &trueVal, + RequiredApprovingReviewCount: &twoVal, + RequiredReviewThreadResolution: &trueVal, + }, + }, + }, + )), + ruleSet(withRules( // Then a more permissive rule set, that might be applied to a broader range of branches. + &repoRule{Type: ruleDeletion}, + &repoRule{ + Type: rulePullRequest, + Parameters: repoRulesParameters{ + PullRequestParameters: pullRequestRuleParameters{ + DismissStaleReviewsOnPush: &falseVal, + RequireCodeOwnerReview: &falseVal, + RequireLastPushApproval: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + RequiredReviewThreadResolution: &falseVal, + }, + }, + }, + )), + }, + expected: &clients.BranchRef{ // We expect to see dominance of restrictive rules. + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + RequireLastPushApproval: &trueVal, + CheckRules: clients.StatusChecksRule{ + UpToDateBeforeMerge: &trueVal, + RequiresStatusChecks: &trueVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredApprovingReviewCount: &twoVal, + DismissStaleReviews: &trueVal, + RequireCodeOwnerReviews: &trueVal, + }, }, }, }, @@ -344,7 +509,7 @@ func Test_applyRepoRules(t *testing.T) { testcase := testcase t.Run(testcase.name, func(t *testing.T) { t.Parallel() - applyRepoRules(testcase.base, []*repoRuleSet{testcase.ruleSet}) + applyRepoRules(testcase.base, testcase.ruleSets) if !cmp.Equal(testcase.base, testcase.expected) { diff := cmp.Diff(testcase.base, testcase.expected) @@ -354,6 +519,102 @@ func Test_applyRepoRules(t *testing.T) { } } -func stringPtr(s string) *string { - return &s +func Test_translationFromGithubAPIBranchProtectionData(t *testing.T) { + t.Parallel() + trueVal := true + falseVal := false + zeroVal := int32(0) + + testcases := []struct { + branch *branch + ruleSet *repoRuleSet + expected *clients.BranchRef + name string + }{ + { + name: "Non-admin Branch Protection rule with insufficient data about requiring PRs", + branch: &branch{ + RefUpdateRule: &refUpdateRule{ + AllowsDeletions: &falseVal, + AllowsForcePushes: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + RequiresCodeOwnerReviews: &falseVal, + RequiresLinearHistory: &falseVal, + RequiredStatusCheckContexts: nil, + }, + BranchProtectionRule: nil, + }, + ruleSet: nil, + expected: &clients.BranchRef{ + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + RequireLinearHistory: &falseVal, + CheckRules: clients.StatusChecksRule{ + UpToDateBeforeMerge: nil, + RequiresStatusChecks: nil, + Contexts: []string{}, + }, + RequiredPullRequestReviews: nil, + }, + }, + }, + { + name: "Admin Branch Protection rule nothing selected", + branch: &branch{ + BranchProtectionRule: &branchProtectionRule{ + DismissesStaleReviews: &falseVal, + IsAdminEnforced: &falseVal, + RequiresStrictStatusChecks: &falseVal, + RequiresStatusChecks: &falseVal, + AllowsDeletions: &falseVal, + AllowsForcePushes: &falseVal, + RequiredApprovingReviewCount: nil, + RequiresCodeOwnerReviews: &falseVal, + RequiresLinearHistory: &falseVal, + RequireLastPushApproval: &falseVal, + RequiredStatusCheckContexts: []string{}, + }, + }, + ruleSet: nil, + expected: &clients.BranchRef{ + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + CheckRules: clients.StatusChecksRule{ + UpToDateBeforeMerge: &falseVal, + RequiresStatusChecks: &falseVal, + Contexts: []string{}, + }, + RequiredPullRequestReviews: nil, + }, + }, + }, + } + + for _, testcase := range testcases { + testcase := testcase + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + + var repoRules []*repoRuleSet + if testcase.ruleSet == nil { + repoRules = []*repoRuleSet{} + } else { + repoRules = []*repoRuleSet{testcase.ruleSet} + } + + result := getBranchRefFrom(testcase.branch, repoRules) + + if !cmp.Equal(result, testcase.expected) { + diff := cmp.Diff(result, testcase.expected) + t.Errorf("test failed: expected - %v, got - %v. \n%s", testcase.expected, result, diff) + } + }) + } } diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go index 3cfb561545f..bd69d7e613e 100644 --- a/clients/gitlabrepo/branches.go +++ b/clients/gitlabrepo/branches.go @@ -193,7 +193,7 @@ func makeBranchRefFrom(branch *gitlab.Branch, protectedBranch *gitlab.ProtectedB Contexts: makeContextsFromResp(projectStatusChecks), } - pullRequestReviewRule := clients.PullRequestReviewRule{ + pullRequestReviewRule := &clients.PullRequestReviewRule{ DismissStaleReviews: newTrue(), RequireCodeOwnerReviews: &protectedBranch.CodeOwnerApprovalRequired, } diff --git a/cron/internal/format/json_raw_results.go b/cron/internal/format/json_raw_results.go index 1a2d6e64331..9c19227c31e 100644 --- a/cron/internal/format/json_raw_results.go +++ b/cron/internal/format/json_raw_results.go @@ -210,15 +210,17 @@ func addBranchProtectionRawResults(r *jsonScorecardRawResult, bp *checker.Branch bp = &jsonBranchProtectionSettings{ AllowsDeletions: v.BranchProtectionRule.AllowDeletions, AllowsForcePushes: v.BranchProtectionRule.AllowForcePushes, - RequiresCodeOwnerReviews: v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews, RequiresLinearHistory: v.BranchProtectionRule.RequireLinearHistory, - DismissesStaleReviews: v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews, EnforcesAdmins: v.BranchProtectionRule.EnforceAdmins, RequiresStatusChecks: v.BranchProtectionRule.CheckRules.RequiresStatusChecks, RequiresUpToDateBranchBeforeMerging: v.BranchProtectionRule.CheckRules.UpToDateBeforeMerge, - RequiredApprovingReviewCount: v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts, } + if v.BranchProtectionRule.RequiredPullRequestReviews != nil { + bp.DismissesStaleReviews = v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews + bp.RequiredApprovingReviewCount = v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount + bp.RequiresCodeOwnerReviews = v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews + } } r.Results.BranchProtections = append(r.Results.BranchProtections, jsonBranchProtection{ Name: *v.Name, diff --git a/docs/checks.md b/docs/checks.md index 8d049df23ed..402544fbded 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -111,7 +111,8 @@ Tier 1 Requirements (3/10 points): - Prevent branch deletion Tier 2 Requirements (6/10 points): - - Require at least 1 reviewer for approval before merging + - Require at least 1 reviewer for approval before merging (for administrators, this requirement weights twice than the others in this tier) + - For administrators: Require PRs prior to make any code changes - For administrators: Require branch to be up to date before merging - For administrators: Require approval of the most recent reviewable push diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 7dabefcc5e8..889589c0c58 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -199,7 +199,8 @@ checks: - Prevent branch deletion Tier 2 Requirements (6/10 points): - - Require at least 1 reviewer for approval before merging + - Require at least 1 reviewer for approval before merging (for administrators, this requirement weights twice than the others in this tier) + - For administrators: Require PRs prior to make any code changes - For administrators: Require branch to be up to date before merging - For administrators: Require approval of the most recent reviewable push diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index 114412d1158..29a547157c3 100644 --- a/e2e/branch_protection_test.go +++ b/e2e/branch_protection_test.go @@ -48,7 +48,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { Error: nil, Score: 6, NumberOfWarn: 2, - NumberOfInfo: 4, + NumberOfInfo: 5, NumberOfDebug: 4, } result := checks.BranchProtection(&req) @@ -106,7 +106,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { Error: nil, Score: 1, NumberOfWarn: 3, - NumberOfInfo: 3, + NumberOfInfo: 4, NumberOfDebug: 4, } result := checks.BranchProtection(&req) @@ -163,9 +163,9 @@ var _ = Describe("E2E TEST:"+checks.CheckBranchProtection+" (repo rules)", func( expected := scut.TestReturn{ Error: nil, Score: 3, - NumberOfWarn: 2, - NumberOfInfo: 4, - NumberOfDebug: 2, + NumberOfWarn: 4, + NumberOfInfo: 5, + NumberOfDebug: 1, } result := checks.BranchProtection(&req) Expect(result.Error).Should(BeNil()) diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 5d63cacfec8..7f2ef29d135 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -712,15 +712,17 @@ func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.Branc bp = &jsonBranchProtectionSettings{ AllowsDeletions: v.BranchProtectionRule.AllowDeletions, AllowsForcePushes: v.BranchProtectionRule.AllowForcePushes, - RequiresCodeOwnerReviews: v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews, RequiresLinearHistory: v.BranchProtectionRule.RequireLinearHistory, - DismissesStaleReviews: v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews, EnforcesAdmins: v.BranchProtectionRule.EnforceAdmins, RequiresStatusChecks: v.BranchProtectionRule.CheckRules.RequiresStatusChecks, RequiresUpToDateBranchBeforeMerging: v.BranchProtectionRule.CheckRules.UpToDateBeforeMerge, - RequiredApprovingReviewCount: v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts, } + if v.BranchProtectionRule.RequiredPullRequestReviews != nil { + bp.DismissesStaleReviews = v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews + bp.RequiredApprovingReviewCount = v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount + bp.RequiresCodeOwnerReviews = v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews + } } branches = append(branches, jsonBranchProtection{ Name: *v.Name, diff --git a/pkg/json_raw_results_test.go b/pkg/json_raw_results_test.go index d9391c8c55e..098d6dfc974 100644 --- a/pkg/json_raw_results_test.go +++ b/pkg/json_raw_results_test.go @@ -1114,7 +1114,7 @@ func TestJsonScorecardRawResult(t *testing.T) { BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: boolPtr(true), AllowForcePushes: boolPtr(false), - RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredPullRequestReviews: &clients.PullRequestReviewRule{ RequireCodeOwnerReviews: boolPtr(true), DismissStaleReviews: boolPtr(true), RequiredApprovingReviewCount: intPtr(2),