Skip to content

Commit 621449f

Browse files
raghavkaulraghavkaul
authored
✨ Add CODEOWNERS branch protection check (#2057)
* Add CODEOWNERS branch protection check * Add docs * Make CODEOWNERS branch protection part of the 'real' score instead of extra-credit * Fix Github checks * Fix lint issues for `range` operator * Fix e2e test failure * Incorporate CODEOWNERS check as part of Code Review checks Level 4 * Fix lint (hopefully?) * Address PR comments - docs
1 parent 6fc08e7 commit 621449f

File tree

6 files changed

+103
-28
lines changed

6 files changed

+103
-28
lines changed

‎checks/branch_protection_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
134134
expected: scut.TestReturn{
135135
Error: nil,
136136
Score: 2,
137-
NumberOfWarn: 5,
137+
NumberOfWarn: 6,
138138
NumberOfInfo: 2,
139139
NumberOfDebug: 0,
140140
},
@@ -172,8 +172,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
172172
expected: scut.TestReturn{
173173
Error: nil,
174174
Score: 2,
175-
NumberOfWarn: 6,
176-
NumberOfInfo: 8,
175+
NumberOfWarn: 7,
176+
NumberOfInfo: 9,
177177
NumberOfDebug: 0,
178178
},
179179
defaultBranch: main,
@@ -227,7 +227,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
227227
Error: nil,
228228
Score: 8,
229229
NumberOfWarn: 2,
230-
NumberOfInfo: 12,
230+
NumberOfInfo: 14,
231231
NumberOfDebug: 0,
232232
},
233233
defaultBranch: main,
@@ -280,7 +280,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
280280
expected: scut.TestReturn{
281281
Error: nil,
282282
Score: 2,
283-
NumberOfWarn: 5,
283+
NumberOfWarn: 6,
284284
NumberOfInfo: 2,
285285
NumberOfDebug: 0,
286286
},

‎checks/evaluation/branch_protection.go‎

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ const (
3030
nonAdminContextLevel = 2 // Level 3.
3131
nonAdminThoroughReviewLevel = 1 // Level 4.
3232
adminThoroughReviewLevel = 1 // Level 5.
33+
3334
)
3435

3536
type scoresInfo struct {
@@ -40,6 +41,7 @@ type scoresInfo struct {
4041
context int
4142
thoroughReview int
4243
adminThoroughReview int
44+
codeownerReview int
4345
}
4446

4547
// Maximum score depending on whether admin token is used.
@@ -76,6 +78,7 @@ func BranchProtection(name string, dl checker.DetailLogger,
7678
score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(&b, dl)
7779
// Do we want this?
7880
score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(&b, dl)
81+
score.scores.codeownerReview, score.maxes.codeownerReview = codeownersBranchProtection(&b, dl)
7982

8083
scores = append(scores, score)
8184
}
@@ -104,60 +107,76 @@ func BranchProtection(name string, dl checker.DetailLogger,
104107

105108
func computeNonAdminBasicScore(scores []levelScore) int {
106109
score := 0
107-
for _, s := range scores {
110+
for i := range scores {
111+
s := scores[i]
108112
score += s.scores.basic
109113
}
110114
return score
111115
}
112116

113117
func computeAdminBasicScore(scores []levelScore) int {
114118
score := 0
115-
for _, s := range scores {
119+
for i := range scores {
120+
s := scores[i]
116121
score += s.scores.adminBasic
117122
}
118123
return score
119124
}
120125

121126
func computeNonAdminReviewScore(scores []levelScore) int {
122127
score := 0
123-
for _, s := range scores {
128+
for i := range scores {
129+
s := scores[i]
124130
score += s.scores.review
125131
}
126132
return score
127133
}
128134

129135
func computeAdminReviewScore(scores []levelScore) int {
130136
score := 0
131-
for _, s := range scores {
137+
for i := range scores {
138+
s := scores[i]
132139
score += s.scores.adminReview
133140
}
134141
return score
135142
}
136143

137144
func computeNonAdminThoroughReviewScore(scores []levelScore) int {
138145
score := 0
139-
for _, s := range scores {
146+
for i := range scores {
147+
s := scores[i]
140148
score += s.scores.thoroughReview
141149
}
142150
return score
143151
}
144152

145153
func computeAdminThoroughReviewScore(scores []levelScore) int {
146154
score := 0
147-
for _, s := range scores {
155+
for i := range scores {
156+
s := scores[i]
148157
score += s.scores.adminThoroughReview
149158
}
150159
return score
151160
}
152161

153162
func computeNonAdminContextScore(scores []levelScore) int {
154163
score := 0
155-
for _, s := range scores {
164+
for i := range scores {
165+
s := scores[i]
156166
score += s.scores.context
157167
}
158168
return score
159169
}
160170

171+
func computeCodeownerThoroughReviewScore(scores []levelScore) int {
172+
score := 0
173+
for i := range scores {
174+
s := scores[i]
175+
score += s.scores.codeownerReview
176+
}
177+
return score
178+
}
179+
161180
func noarmalizeScore(score, max, level int) float64 {
162181
if max == 0 {
163182
return float64(level)
@@ -204,14 +223,18 @@ func computeScore(scores []levelScore) (int, error) {
204223
}
205224

206225
// Fourth, check the thorough non-admin reviews.
226+
// Also check whether this repo requires codeowner review
207227
maxThoroughReviewScore := maxScore.thoroughReview * len(scores)
228+
maxCodeownerReviewScore := maxScore.codeownerReview * len(scores)
208229
thoroughReviewScore := computeNonAdminThoroughReviewScore(scores)
209-
score += noarmalizeScore(thoroughReviewScore, maxThoroughReviewScore, nonAdminThoroughReviewLevel)
230+
codeownerReviewScore := computeCodeownerThoroughReviewScore(scores)
231+
score += noarmalizeScore(thoroughReviewScore+codeownerReviewScore, maxThoroughReviewScore+maxCodeownerReviewScore,
232+
nonAdminThoroughReviewLevel)
210233
if thoroughReviewScore != maxThoroughReviewScore {
211234
return int(score), nil
212235
}
213236

214-
// Last, check the thorough admin review config.
237+
// Lastly, check the thorough admin review config.
215238
// This one is controversial and has usability issues
216239
// https://github.com/ossf/scorecard/issues/1027, so we may remove it.
217240
maxAdminThoroughReviewScore := maxScore.adminThoroughReview * len(scores)
@@ -412,3 +435,22 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
412435
}
413436
return score, max
414437
}
438+
439+
func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) {
440+
score := 0
441+
max := 1
442+
443+
log := branch.Protected != nil && *branch.Protected
444+
445+
if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
446+
switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
447+
case true:
448+
info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)
449+
score++
450+
default:
451+
warn(dl, log, "codeowner review is not required on branch '%s'", *branch.Name)
452+
}
453+
}
454+
455+
return score, max
456+
}

‎checks/evaluation/branch_protection_test.go‎

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ func testScore(branch *clients.BranchRef, dl checker.DetailLogger) (int, error)
3131
score.scores.context, score.maxes.context = nonAdminContextProtection(branch, dl)
3232
score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(branch, dl)
3333
score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(branch, dl)
34+
score.scores.codeownerReview, score.maxes.codeownerReview = codeownersBranchProtection(branch, dl)
3435

3536
return computeScore([]levelScore{score})
3637
}
@@ -52,7 +53,7 @@ func TestIsBranchProtected(t *testing.T) {
5253
expected: scut.TestReturn{
5354
Error: nil,
5455
Score: 2,
55-
NumberOfWarn: 5,
56+
NumberOfWarn: 6,
5657
NumberOfInfo: 2,
5758
NumberOfDebug: 0,
5859
},
@@ -96,7 +97,7 @@ func TestIsBranchProtected(t *testing.T) {
9697
expected: scut.TestReturn{
9798
Error: nil,
9899
Score: 2,
99-
NumberOfWarn: 3,
100+
NumberOfWarn: 4,
100101
NumberOfInfo: 4,
101102
NumberOfDebug: 0,
102103
},
@@ -126,7 +127,7 @@ func TestIsBranchProtected(t *testing.T) {
126127
expected: scut.TestReturn{
127128
Error: nil,
128129
Score: 2,
129-
NumberOfWarn: 4,
130+
NumberOfWarn: 5,
130131
NumberOfInfo: 3,
131132
NumberOfDebug: 0,
132133
},
@@ -156,7 +157,7 @@ func TestIsBranchProtected(t *testing.T) {
156157
expected: scut.TestReturn{
157158
Error: nil,
158159
Score: 2,
159-
NumberOfWarn: 4,
160+
NumberOfWarn: 5,
160161
NumberOfInfo: 3,
161162
NumberOfDebug: 0,
162163
},
@@ -186,7 +187,7 @@ func TestIsBranchProtected(t *testing.T) {
186187
expected: scut.TestReturn{
187188
Error: nil,
188189
Score: 3,
189-
NumberOfWarn: 3,
190+
NumberOfWarn: 4,
190191
NumberOfInfo: 4,
191192
NumberOfDebug: 0,
192193
},
@@ -216,7 +217,7 @@ func TestIsBranchProtected(t *testing.T) {
216217
expected: scut.TestReturn{
217218
Error: nil,
218219
Score: 2,
219-
NumberOfWarn: 4,
220+
NumberOfWarn: 5,
220221
NumberOfInfo: 3,
221222
NumberOfDebug: 0,
222223
},
@@ -246,7 +247,7 @@ func TestIsBranchProtected(t *testing.T) {
246247
expected: scut.TestReturn{
247248
Error: nil,
248249
Score: 1,
249-
NumberOfWarn: 5,
250+
NumberOfWarn: 6,
250251
NumberOfInfo: 2,
251252
NumberOfDebug: 0,
252253
},
@@ -277,7 +278,7 @@ func TestIsBranchProtected(t *testing.T) {
277278
expected: scut.TestReturn{
278279
Error: nil,
279280
Score: 1,
280-
NumberOfWarn: 5,
281+
NumberOfWarn: 6,
281282
NumberOfInfo: 2,
282283
NumberOfDebug: 0,
283284
},
@@ -308,7 +309,7 @@ func TestIsBranchProtected(t *testing.T) {
308309
Error: nil,
309310
Score: 8,
310311
NumberOfWarn: 1,
311-
NumberOfInfo: 6,
312+
NumberOfInfo: 7,
312313
NumberOfDebug: 0,
313314
},
314315
branch: &clients.BranchRef{
@@ -332,6 +333,36 @@ func TestIsBranchProtected(t *testing.T) {
332333
},
333334
},
334335
},
336+
{
337+
name: "Branches are protected and require codeowner review",
338+
expected: scut.TestReturn{
339+
Error: nil,
340+
Score: 8,
341+
NumberOfWarn: 2,
342+
NumberOfInfo: 6,
343+
NumberOfDebug: 0,
344+
},
345+
branch: &clients.BranchRef{
346+
Name: &branchVal,
347+
Protected: &trueVal,
348+
BranchProtectionRule: clients.BranchProtectionRule{
349+
EnforceAdmins: &trueVal,
350+
RequireLinearHistory: &trueVal,
351+
AllowForcePushes: &falseVal,
352+
AllowDeletions: &falseVal,
353+
CheckRules: clients.StatusChecksRule{
354+
RequiresStatusChecks: &falseVal,
355+
UpToDateBeforeMerge: &trueVal,
356+
Contexts: []string{"foo"},
357+
},
358+
RequiredPullRequestReviews: clients.PullRequestReviewRule{
359+
DismissStaleReviews: &trueVal,
360+
RequireCodeOwnerReviews: &falseVal,
361+
RequiredApprovingReviewCount: &oneVal,
362+
},
363+
},
364+
},
365+
},
335366
}
336367
for _, tt := range tests {
337368
tt := tt // Re-initializing variable so it is not changed while executing the closure below

‎docs/checks.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ certain workflows for branches, such as requiring review or passing certain
6565
status checks before acceptance into a main branch, or preventing rewriting of
6666
public history.
6767

68-
Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, and `StrictStatusCheck`. If
68+
Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, `StrictStatusCheck` and `RequireCodeownerReview`. If
6969
the provided token does not have admin access, the check will query the branch
7070
settings accessible to non-admins and provide results based only on these settings.
7171
Even so, we recommend using a non-admin token, which provides a thorough enough
@@ -116,6 +116,7 @@ Tier 4 Requirements (9/10 points):
116116

117117
Tier 5 Requirements (10/10 points):
118118
- For administrators: Dismiss stale reviews
119+
- For administrators: Require CODEOWNER review
119120

120121

121122
**Remediation steps**
@@ -385,7 +386,7 @@ This check will only succeed if a Github project is >90 days old. Projects
385386
that are younger than this are too new to assess whether they are maintained
386387
or not, and users should inspect the contents of those projects to ensure they
387388
are as expected.
388-
389+
389390

390391
**Remediation steps**
391392
- There is no remediation work needed from projects with a low score; this check simply provides insight into the project activity and maintenance commitment. External users should determine whether the software is the type that would not normally need active maintenance.

‎docs/checks/internal/checks.yaml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ checks:
153153
status checks before acceptance into a main branch, or preventing rewriting of
154154
public history.
155155
156-
Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, and `StrictStatusCheck`. If
156+
Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, `StrictStatusCheck` and `RequireCodeownerReview`. If
157157
the provided token does not have admin access, the check will query the branch
158158
settings accessible to non-admins and provide results based only on these settings.
159159
Even so, we recommend using a non-admin token, which provides a thorough enough
@@ -204,6 +204,7 @@ checks:
204204
205205
Tier 5 Requirements (10/10 points):
206206
- For administrators: Dismiss stale reviews
207+
- For administrators: Require CODEOWNER review
207208
208209
remediation:
209210
- >-

‎e2e/branch_protection_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
4848
Error: nil,
4949
Score: 6,
5050
NumberOfWarn: 1,
51-
NumberOfInfo: 3,
51+
NumberOfInfo: 4,
5252
NumberOfDebug: 3,
5353
}
5454
result := checks.BranchProtection(&req)
@@ -105,7 +105,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
105105
expected := scut.TestReturn{
106106
Error: nil,
107107
Score: 1,
108-
NumberOfWarn: 2,
108+
NumberOfWarn: 3,
109109
NumberOfInfo: 3,
110110
NumberOfDebug: 3,
111111
}

0 commit comments

Comments
 (0)