From 8752511a3c368e4712c5e0e89c913bb04672965b Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 25 Sep 2023 12:10:23 -0700 Subject: [PATCH] :sparkles: Move "EnforcesAdmins" to tier 5 Branch-Protection (#3502) * Remove EnforceAdmins from tier 1. Scores in some tests either increase to 3, or 4, since EnfroceAdmins no longer keeps them in tier 1. The number of Debug, Info, and Warn messages will decrease by 1 per branch, since we're no longer logging them. Signed-off-by: Spencer Schrock * move enforce admins to tier 5. Signed-off-by: Spencer Schrock --------- Signed-off-by: Spencer Schrock --- checks/branch_protection_test.go | 6 +-- checks/evaluation/branch_protection.go | 60 +++++++-------------- checks/evaluation/branch_protection_test.go | 11 ++-- docs/checks.md | 2 +- docs/checks/internal/checks.yaml | 2 +- 5 files changed, 29 insertions(+), 52 deletions(-) diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index 929c20768c8..035120a1486 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -134,7 +134,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { name: "Only development branch", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 3, NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, @@ -173,7 +173,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { name: "Take worst of release and development", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 4, NumberOfWarn: 9, NumberOfInfo: 10, NumberOfDebug: 0, @@ -285,7 +285,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { name: "Ignore a non-branch targetcommitish", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 3, NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 7703cab920c..1a66347c663 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -25,7 +25,7 @@ import ( const ( minReviews = 2 // Points incremented at each level. - adminNonAdminBasicLevel = 3 // Level 1. + basicLevel = 3 // Level 1. adminNonAdminReviewLevel = 3 // Level 2. nonAdminContextLevel = 2 // Level 3. nonAdminThoroughReviewLevel = 1 // Level 4. @@ -35,7 +35,6 @@ const ( type scoresInfo struct { basic int - adminBasic int review int adminReview int context int @@ -71,7 +70,6 @@ func BranchProtection(name string, dl checker.DetailLogger, }) } score.scores.basic, score.maxes.basic = basicNonAdminProtection(&b, dl) - score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(&b, dl) score.scores.review, score.maxes.review = nonAdminReviewProtection(&b) score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(&b, dl) score.scores.context, score.maxes.context = nonAdminContextProtection(&b, dl) @@ -114,15 +112,6 @@ func computeNonAdminBasicScore(scores []levelScore) int { return score } -func computeAdminBasicScore(scores []levelScore) int { - score := 0 - for i := range scores { - s := scores[i] - score += s.scores.adminBasic - } - return score -} - func computeNonAdminReviewScore(scores []levelScore) int { score := 0 for i := range scores { @@ -194,12 +183,9 @@ func computeScore(scores []levelScore) (int, error) { // First, check if they all pass the basic (admin and non-admin) checks. maxBasicScore := maxScore.basic * len(scores) - maxAdminBasicScore := maxScore.adminBasic * len(scores) basicScore := computeNonAdminBasicScore(scores) - adminBasicScore := computeAdminBasicScore(scores) - score += noarmalizeScore(basicScore+adminBasicScore, maxBasicScore+maxAdminBasicScore, adminNonAdminBasicLevel) - if basicScore != maxBasicScore || - adminBasicScore != maxAdminBasicScore { + score += noarmalizeScore(basicScore, maxBasicScore, basicLevel) + if basicScore != maxBasicScore { return int(score), nil } @@ -308,30 +294,6 @@ func basicNonAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger) return score, max } -func basicAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { - score := 0 - max := 0 - // Only log information if the branch is protected. - log := branch.Protected != nil && *branch.Protected - - // nil typically means we do not have access to the value. - if branch.BranchProtectionRule.EnforceAdmins != nil { - // Note: we don't inrecase max possible score for non-admin viewers. - max++ - switch *branch.BranchProtectionRule.EnforceAdmins { - case true: - info(dl, log, "settings apply to administrators on branch '%s'", *branch.Name) - score++ - case false: - warn(dl, log, "settings do not apply to administrators on branch '%s'", *branch.Name) - } - } else { - debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", *branch.Name) - } - - return score, max -} - func nonAdminContextProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 @@ -422,6 +384,22 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL } else { debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name) } + + // nil typically means we do not have access to the value. + if branch.BranchProtectionRule.EnforceAdmins != nil { + // Note: we don't inrecase max possible score for non-admin viewers. + max++ + switch *branch.BranchProtectionRule.EnforceAdmins { + case true: + info(dl, log, "settings apply to administrators on branch '%s'", *branch.Name) + score++ + case false: + warn(dl, log, "settings do not apply to administrators on branch '%s'", *branch.Name) + } + } else { + debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", *branch.Name) + } + return score, max } diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index 5fd5e541edb..2b45d116714 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -25,7 +25,6 @@ import ( func testScore(branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger) (int, error) { var score levelScore score.scores.basic, score.maxes.basic = basicNonAdminProtection(branch, dl) - score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(branch, dl) score.scores.review, score.maxes.review = nonAdminReviewProtection(branch) score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(branch, dl) score.scores.context, score.maxes.context = nonAdminContextProtection(branch, dl) @@ -53,7 +52,7 @@ func TestIsBranchProtected(t *testing.T) { name: "Nothing is enabled", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 3, NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, @@ -98,7 +97,7 @@ func TestIsBranchProtected(t *testing.T) { name: "Required status check enabled", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 4, NumberOfWarn: 5, NumberOfInfo: 4, NumberOfDebug: 0, @@ -129,7 +128,7 @@ func TestIsBranchProtected(t *testing.T) { name: "Required status check enabled without checking for status string", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 4, NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, @@ -160,7 +159,7 @@ func TestIsBranchProtected(t *testing.T) { name: "Required pull request enabled", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 4, NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, @@ -222,7 +221,7 @@ func TestIsBranchProtected(t *testing.T) { name: "Required linear history enabled", expected: scut.TestReturn{ Error: nil, - Score: 2, + Score: 3, NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, diff --git a/docs/checks.md b/docs/checks.md index a7a130e03a6..7800bd13bab 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -109,7 +109,6 @@ Note: If Scorecard is run without an administrative access token, the requiremen Tier 1 Requirements (3/10 points): - Prevent force push - Prevent branch deletion - - For administrators: Include administrator for review Tier 2 Requirements (6/10 points): - Require at least 1 reviewer for approval before merging @@ -125,6 +124,7 @@ Tier 4 Requirements (9/10 points): Tier 5 Requirements (10/10 points): - For administrators: Dismiss stale reviews and approvals when new commits are pushed + - For administrators: Include administrator for review GitLab Integration Status: - GitLab associates releases with commits and not with the branch. Releases are ignored in this portion of the scoring. diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 352cc1ad107..2dbfc750a9c 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -198,7 +198,6 @@ checks: Tier 1 Requirements (3/10 points): - Prevent force push - Prevent branch deletion - - For administrators: Include administrator for review Tier 2 Requirements (6/10 points): - Require at least 1 reviewer for approval before merging @@ -214,6 +213,7 @@ checks: Tier 5 Requirements (10/10 points): - For administrators: Dismiss stale reviews and approvals when new commits are pushed + - For administrators: Include administrator for review GitLab Integration Status: - GitLab associates releases with commits and not with the branch. Releases are ignored in this portion of the scoring.