branch protection: requiring PRs gives partial credit (#3499)

* feat(branch-protection): consider if project requires PRs prior to make changes

As discussed at the issue #2727, we're adding the "require PRs prior
to make changes" as another requirement to tier 2. In addition to that,
we're changing the weight of the tier 2 requirements so that
"requiring 1 reviewer" has weight 2, while the other tier 2 requirements
have weight 1

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): increment and adapt testing

1. Adapt previous test cases to consider that now we'll have an aditional
Info log telling that the project requires PRs to make changes.
2. Add more cases to test relevant use cases on the tier 2 level of
branch protection

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* docs(branch-protection-check): adapt check description to consider requirement of require PRs to make changes

It adds the new tier 2 requirement, but also specify that the
"require at least 1 reviewer" will have doubled weight.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection-check): avoid duplicate funcions and enhance readability

Made some nice-to-have improvements on project readability,
making it easier easier to  understand how the branch-protection
score is computed. Also unified 8 different functions that were
doing basically the same thing.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>


* feat(branch-protection): standardize values received on evaluation

Previously, at the evaluation part of branch protetion, the
values nil and false or zero were sort of interchangeble. This commit
changes the code to set as nil only the data that could not be retrieved
from github -- all the others would have values as false, zero, true, etc

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(github-client): adapt and add tests to check if nil values are coherent

1. Add new test to evaluate how we're interpreting a rule with all
checkboxes unchecked (most shouldn't be nil)
2. Adapt existent tests to expect non-nil values for unchecked
   checkboxes

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* feat(client-github): avoid reusing bool pointers

Changes some pieces of code to prefer using pointers of
bool instantiated independently. If reusing bool pointers, at some piece
of code the value of the bool could inadvertently changed and it would change the
value of all other fields reusing that pointer.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* feat(branch-protection): enhance evaluation if scorecard was run by admin

At the evaluation step we were using some non untrusted fieldds of the
resposte to evaluate if Scorecard was run as admin or not. Now we're
using a field provided directly from the client file.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): adapt testings to say if they have admin info or not

After last commit, the client will tell the evaluation files if
Scorecard was run by administrator or not (i.e., if we have all the
infos). This commit adapts the testings to also provide this info.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(e2e-branch-protection): adapt number of logs after changes

- 2 warns (for 'last push approval' and 'codeowners review' disabled) were added because now those informations come as 'not-nil' at the evaluation part.
- 1 info was added to say that PRs are required to make changes
- 1 debug was removed because it said that we couldn't retrieve 'last push approval' information, but we actually can. It was just incorrectly set as nil

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* Revert the 2 commits with changes around how Scorecard detects admin run

Reverts commit 64c3521d89a6493e0d8c7527aa011f98c3e35719 and commit e2662b7173ef90b44b2d72c37614230440e8a919.
Both had chances around using clients/branch.go scructur to store the
information of whether Scorecard was being run by admin or not. We
decided to not change this structure for this purpose.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): change data structure to use pointer instead of value

At clients.BranchProtectionRule struct, changing
RequiredPullRequestReviews to be a pointer instead of a struct value.
This will allow the usage of the nil value of this structure to mean
that we can't say if the repository requires reviews or not.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* feat(branch-protection): use nil pointer on reviewers struct to mean
we don't know if they require PRs

The nil value of the struct RequiredPullRequestReviews will now mean
that we can't tell whether the project requires PRs to make changes or not.

When we get this case, we're printing a debug informing that we don't have
this data, but also printing a warn saying that they don't require
reviews, because that will be true at this case.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): if we're setting the reviewers struct to nil
when needed

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* doc(branch-protection): add code comment explaining different weight on tier 2 scores

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): avoid duplicate if branches on reviewers num comparation

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* docs(branch-protection): clarify commentings around data structure

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor: clean code on parsing GitHub BP data

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* feat(branch-protection): ressignify the nil PullRequestReviewRule to mean PR not required

Adapt translation of data from GitHub API, now for our internal data
modeling, having a nil PullRequestReviewRule structure will mean that
PRs are not required on the repo (can also mean we don't have data to
ensure that).

It also changes the order of the calls of copyNonAdminSettings and
copyAdminSettings to make the first one be called first. This eases the
code because the PullRequestReviewRule can be always instantiated at
this function.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): ensure we translate GitHub BP data as expected

Ensure we're correctly translating GitHub data from the old Branch
Protection config.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* feat(branch-protection): adapt score evaluation after 2efeee6512

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): adapt testings to changes of last commits

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* docs(branch-protection): add TODO comments pointing refactor opportunities

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* fix: avoid penalyzing non-admin for dismissStaleReview

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* fix(branch-protection): prevent false value from API field to become nil

When translating the API results, if the specific field `DismissesStaleReviews`
had a false value, it was not being initiated in our data model and was
remaining nil.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor: clarify different weight on first reviewer

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor: enhance clarity of loggings and comments

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): new test to cover different rules affecting same branch

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* docs(branch-protection): change requirements ordering to keep admin ones together

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): simplify auxiliary function

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): fix code format to linter requirements

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): avoid unnecessary initializations and rename function

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): adapt test that was forgotten on commit 6858790a3e

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): use enums to represent tiers

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): remove nil fields of struct initialization when they dont contribute for clarification

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): simplify functions by using generics

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* docs(branch-protection): update docs after generate-docs run

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* fix(branch-protection): fix duplicated line on code

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* fix(branch-protection): stop exporting Tier enum

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* refactor(branch-protection): changing unchanged var to const

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

* test(branch-protection): Rename test and adapt it to be consistent with its purpose

I also changed the test to not require PRs, as it's how it is when a new GitHub
Branch Protection config is created. The changes on the loggings numbers are due
to:
1. A warning for not having DismissStaleReviews became a debug
2. Removed the warning we had for not requiring CodeOwners
3. Have a new warning for not requiring PRe

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>

---------

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
This commit is contained in:
Diogo Teles Sant'Anna 2023-12-12 03:39:02 -03:00 committed by GitHub
parent 30ef6b1026
commit db7b6e70af
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 723 additions and 294 deletions

View File

@ -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,
},

View File

@ -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
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 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
}
return score
}
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)

View File

@ -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,

View File

@ -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

View File

@ -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.RequireCodeOwnerReviews == nil || readBoolPtr(translated.RequireCodeOwnerReviews) {
base.RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews
if (*base).DismissStaleReviews == nil || valueOrZero(translated.DismissStaleReviews) {
(*base).DismissStaleReviews = translated.DismissStaleReviews
}
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
}

View File

@ -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: &falseVal,
EnforceAdmins: &trueVal,
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 with bypass",
base: &clients.BranchRef{},
ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion}), withBypass()),
name: "block deletion no bypass",
base: &clients.BranchRef{},
ruleSets: []*repoRuleSet{
ruleSet(withRules(&repoRule{Type: ruleDeletion})),
},
expected: &clients.BranchRef{
BranchProtectionRule: clients.BranchProtectionRule{
AllowDeletions: &falseVal,
EnforceAdmins: &falseVal,
AllowDeletions: &falseVal,
AllowForcePushes: &trueVal,
RequireLinearHistory: &falseVal,
EnforceAdmins: &trueVal,
RequiredPullRequestReviews: nil,
},
},
},
{
name: "block deletion with bypass",
base: &clients.BranchRef{},
ruleSets: []*repoRuleSet{
ruleSet(withRules(&repoRule{Type: ruleDeletion}), withBypass()),
},
expected: &clients.BranchRef{
BranchProtectionRule: clients.BranchProtectionRule{
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})),
expected: &clients.BranchRef{
BranchProtectionRule: clients.BranchProtectionRule{
AllowForcePushes: &falseVal,
EnforceAdmins: &trueVal,
},
},
},
{
name: "require linear history no bypass",
base: &clients.BranchRef{},
ruleSet: ruleSet(withRules(&repoRule{Type: ruleLinear})),
expected: &clients.BranchRef{
BranchProtectionRule: clients.BranchProtectionRule{
RequireLinearHistory: &trueVal,
EnforceAdmins: &trueVal,
},
},
},
{
name: "require pull request no bypass",
name: "block force push 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: ruleForcePush})),
},
expected: &clients.BranchRef{
BranchProtectionRule: clients.BranchProtectionRule{
AllowDeletions: &trueVal,
AllowForcePushes: &falseVal,
EnforceAdmins: &trueVal,
RequireLinearHistory: &falseVal,
RequiredPullRequestReviews: nil,
},
},
},
{
name: "require linear history no bypass",
base: &clients.BranchRef{},
ruleSets: []*repoRuleSet{
ruleSet(withRules(&repoRule{Type: ruleLinear})),
},
expected: &clients.BranchRef{
BranchProtectionRule: clients.BranchProtectionRule{
AllowDeletions: &trueVal,
AllowForcePushes: &trueVal,
RequireLinearHistory: &trueVal,
EnforceAdmins: &trueVal,
RequiredPullRequestReviews: nil,
},
},
},
{
name: "require pull request but no reviewers and no bypass",
base: &clients.BranchRef{},
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,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
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{
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)
}
})
}
}

View File

@ -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,
}

View File

@ -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,

View File

@ -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

View File

@ -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

View File

@ -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())

View File

@ -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,

View File

@ -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),