Skip to content

Commit

Permalink
✨ Move "EnforcesAdmins" to tier 5 Branch-Protection (#3502)
Browse files Browse the repository at this point in the history 
* 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 <sschrock@google.com>

* move enforce admins to tier 5.

Signed-off-by: Spencer Schrock <sschrock@google.com>

---------

Signed-off-by: Spencer Schrock <sschrock@google.com>
  • Loading branch information
@spencerschrock
spencerschrock authored Sep 25, 2023
1 parent 6aa3bcc commit 8752511
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 52 deletions.
6 changes: 3 additions & 3 deletions checks/branch_protection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
60 changes: 19 additions & 41 deletions checks/evaluation/branch_protection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -35,7 +35,6 @@ const (

type scoresInfo struct {
basic int
adminBasic int
review int
adminReview int
context int
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
11 changes: 5 additions & 6 deletions checks/evaluation/branch_protection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion docs/checks.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion docs/checks/internal/checks.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down

0 comments on commit 8752511

Please sign in to comment.