mirror of
https://github.com/ossf/scorecard.git
synced 2024-09-17 11:57:12 +03:00
✨ Cleanup Branch Protection and add e2e tests (#1344)
* BP cleanup * linnter * e2e fix * linter * linter Co-authored-by: asraa <asraa@google.com>
This commit is contained in:
parent
3eb2e5aec8
commit
aed511670f
@ -24,8 +24,6 @@ import (
|
||||
sce "github.com/ossf/scorecard/v3/errors"
|
||||
)
|
||||
|
||||
type branchProtectionSetting int
|
||||
|
||||
const (
|
||||
// CheckBranchProtection is the exported name for Branch-Protected check.
|
||||
CheckBranchProtection = "Branch-Protection"
|
||||
@ -36,38 +34,8 @@ const (
|
||||
nonAdminContextLevel = 2 // Level 3.
|
||||
nonAdminThoroughReviewLevel = 1 // Level 4.
|
||||
adminThoroughReviewLevel = 1 // Level 5.
|
||||
// First level.
|
||||
allowForcePushes branchProtectionSetting = iota
|
||||
allowDeletions
|
||||
// Second and third level.
|
||||
requireApprovingReviewCount
|
||||
// Fourth level.
|
||||
requireStatusChecksContexts
|
||||
// Admin settings.
|
||||
// First level.
|
||||
enforceAdmins
|
||||
// Second level.
|
||||
requireUpToDateBeforeMerge
|
||||
// Fifth level.
|
||||
dismissStaleReviews
|
||||
// requireCodeOwnerReviews no longer used.
|
||||
// requireLinearHistory no longer used, see https://github.com/ossf/scorecard/issues/1027.
|
||||
)
|
||||
|
||||
// Note: we don't need these since they are all `1`, so we may remove them.
|
||||
var branchProtectionSettingScores = map[branchProtectionSetting]int{
|
||||
allowForcePushes: 1,
|
||||
allowDeletions: 1,
|
||||
// Used to check if required status checks before merging is non-empty,
|
||||
// rather than checking if requireStatusChecks is set.
|
||||
requireStatusChecksContexts: 1,
|
||||
requireApprovingReviewCount: 1,
|
||||
// Need admin token, so we cannot rely on them in general.
|
||||
enforceAdmins: 1,
|
||||
requireUpToDateBeforeMerge: 1, // Gated by requireStatusChecks=true.
|
||||
dismissStaleReviews: 1,
|
||||
}
|
||||
|
||||
type scoresInfo struct {
|
||||
basic int
|
||||
adminBasic int
|
||||
@ -80,9 +48,8 @@ type scoresInfo struct {
|
||||
|
||||
// Maximum score depending on whether admin token is used.
|
||||
type levelScore struct {
|
||||
scores scoresInfo // Score result for a branch.
|
||||
maxes scoresInfo // Maximum possible score for a branch.
|
||||
protected bool // Protection enabled on the branch.
|
||||
scores scoresInfo // Score result for a branch.
|
||||
maxes scoresInfo // Maximum possible score for a branch.
|
||||
}
|
||||
|
||||
//nolint:gochecknoinits
|
||||
@ -107,7 +74,8 @@ func (b branchMap) getBranchByName(name string) (*clients.BranchRef, error) {
|
||||
return val, nil
|
||||
}
|
||||
}
|
||||
return nil, fmt.Errorf("could not find branch name %s: %w", name, errInternalBranchNotFound)
|
||||
return nil, sce.WithMessage(sce.ErrScorecardInternal,
|
||||
fmt.Sprintf("could not find branch name %s: %v", name, errInternalBranchNotFound))
|
||||
}
|
||||
|
||||
func getBranchMapFrom(branches []*clients.BranchRef) branchMap {
|
||||
@ -134,138 +102,60 @@ func BranchProtection(c *checker.CheckRequest) checker.CheckResult {
|
||||
return checkReleaseAndDevBranchProtection(c.RepoClient, c.Dlogger)
|
||||
}
|
||||
|
||||
func validateMaxScore(s1, s2 int) error {
|
||||
if s1 != s2 {
|
||||
return sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("invalid score %d != %d",
|
||||
s1, s2))
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// This function validates that all maximum scores are the same
|
||||
// for each level and branch. An error would mean a logic bug in our
|
||||
// implementation.
|
||||
func getMaxScores(scores []levelScore) (scoresInfo, error) {
|
||||
if len(scores) == 0 {
|
||||
return scoresInfo{}, sce.WithMessage(sce.ErrScorecardInternal, "empty score")
|
||||
}
|
||||
|
||||
score := scores[0]
|
||||
for _, s := range scores[1:] {
|
||||
// Only validate the maximum scores if both entries have the same protection status.
|
||||
if s.protected != score.protected {
|
||||
continue
|
||||
}
|
||||
if err := validateMaxScore(score.maxes.basic, s.maxes.basic); err != nil {
|
||||
return scoresInfo{}, err
|
||||
}
|
||||
|
||||
if err := validateMaxScore(score.maxes.adminBasic, s.maxes.adminBasic); err != nil {
|
||||
return scoresInfo{}, err
|
||||
}
|
||||
|
||||
if err := validateMaxScore(score.maxes.review, s.maxes.review); err != nil {
|
||||
return scoresInfo{}, err
|
||||
}
|
||||
|
||||
if err := validateMaxScore(score.maxes.adminReview, s.maxes.adminReview); err != nil {
|
||||
return scoresInfo{}, err
|
||||
}
|
||||
|
||||
if err := validateMaxScore(score.maxes.context, s.maxes.context); err != nil {
|
||||
return scoresInfo{}, err
|
||||
}
|
||||
|
||||
if err := validateMaxScore(score.maxes.thoroughReview, s.maxes.thoroughReview); err != nil {
|
||||
return scoresInfo{}, err
|
||||
}
|
||||
|
||||
if err := validateMaxScore(score.maxes.adminThoroughReview,
|
||||
s.maxes.adminThoroughReview); err != nil {
|
||||
return scoresInfo{}, err
|
||||
}
|
||||
}
|
||||
|
||||
return scoresInfo{
|
||||
basic: score.maxes.basic,
|
||||
adminBasic: score.maxes.adminBasic,
|
||||
review: score.maxes.review,
|
||||
adminReview: score.maxes.adminReview,
|
||||
context: score.maxes.context,
|
||||
thoroughReview: score.maxes.thoroughReview,
|
||||
adminThoroughReview: score.maxes.adminThoroughReview,
|
||||
}, nil
|
||||
}
|
||||
|
||||
func computeNonAdminBasicScore(scores []levelScore, maxes scoresInfo) (int, int) {
|
||||
func computeNonAdminBasicScore(scores []levelScore) int {
|
||||
score := 0
|
||||
max := 0
|
||||
for _, s := range scores {
|
||||
score += s.scores.basic
|
||||
max += maxes.basic
|
||||
}
|
||||
return score, max
|
||||
return score
|
||||
}
|
||||
|
||||
func computeAdminBasicScore(scores []levelScore, maxes scoresInfo) (int, int) {
|
||||
func computeAdminBasicScore(scores []levelScore) int {
|
||||
score := 0
|
||||
max := 0
|
||||
for _, s := range scores {
|
||||
score += s.scores.adminBasic
|
||||
max += maxes.adminBasic
|
||||
}
|
||||
return score, max
|
||||
return score
|
||||
}
|
||||
|
||||
func computeNonAdminReviewScore(scores []levelScore, maxes scoresInfo) (int, int) {
|
||||
func computeNonAdminReviewScore(scores []levelScore) int {
|
||||
score := 0
|
||||
max := 0
|
||||
for _, s := range scores {
|
||||
score += s.scores.review
|
||||
max += maxes.review
|
||||
}
|
||||
return score, max
|
||||
return score
|
||||
}
|
||||
|
||||
func computeAdminReviewScore(scores []levelScore, maxes scoresInfo) (int, int) {
|
||||
func computeAdminReviewScore(scores []levelScore) int {
|
||||
score := 0
|
||||
max := 0
|
||||
for _, s := range scores {
|
||||
score += s.scores.adminReview
|
||||
max += maxes.adminReview
|
||||
}
|
||||
return score, max
|
||||
return score
|
||||
}
|
||||
|
||||
func computeNonAdminThoroughReviewScore(scores []levelScore, maxes scoresInfo) (int, int) {
|
||||
func computeNonAdminThoroughReviewScore(scores []levelScore) int {
|
||||
score := 0
|
||||
max := 0
|
||||
for _, s := range scores {
|
||||
score += s.scores.thoroughReview
|
||||
max += maxes.thoroughReview
|
||||
}
|
||||
return score, max
|
||||
return score
|
||||
}
|
||||
|
||||
func computeAdminThoroughReviewScore(scores []levelScore, maxes scoresInfo) (int, int) {
|
||||
func computeAdminThoroughReviewScore(scores []levelScore) int {
|
||||
score := 0
|
||||
max := 0
|
||||
for _, s := range scores {
|
||||
score += s.scores.adminThoroughReview
|
||||
max += maxes.adminThoroughReview
|
||||
}
|
||||
return score, max
|
||||
return score
|
||||
}
|
||||
|
||||
func computeNonAdminContextScore(scores []levelScore, maxes scoresInfo) (int, int) {
|
||||
func computeNonAdminContextScore(scores []levelScore) int {
|
||||
score := 0
|
||||
max := 0
|
||||
for _, s := range scores {
|
||||
score += s.scores.context
|
||||
max += maxes.context
|
||||
}
|
||||
return score, max
|
||||
return score
|
||||
}
|
||||
|
||||
func noarmalizeScore(score, max, level int) float64 {
|
||||
@ -276,17 +166,18 @@ func noarmalizeScore(score, max, level int) float64 {
|
||||
}
|
||||
|
||||
func computeScore(scores []levelScore) (int, error) {
|
||||
// Validate and retrieve the maximum scores.
|
||||
maxScores, err := getMaxScores(scores)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
if len(scores) == 0 {
|
||||
return 0, sce.WithMessage(sce.ErrScorecardInternal, "scores are empty")
|
||||
}
|
||||
|
||||
score := float64(0)
|
||||
maxScore := scores[0].maxes
|
||||
|
||||
// First, check if they all pass the basic (admin and non-admin) checks.
|
||||
basicScore, maxBasicScore := computeNonAdminBasicScore(scores, maxScores)
|
||||
adminBasicScore, maxAdminBasicScore := computeAdminBasicScore(scores, maxScores)
|
||||
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 {
|
||||
@ -294,8 +185,10 @@ func computeScore(scores []levelScore) (int, error) {
|
||||
}
|
||||
|
||||
// Second, check the (admin and non-admin) reviews.
|
||||
reviewScore, maxReviewScore := computeNonAdminReviewScore(scores, maxScores)
|
||||
adminReviewScore, maxAdminReviewScore := computeAdminReviewScore(scores, maxScores)
|
||||
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 {
|
||||
@ -303,14 +196,16 @@ func computeScore(scores []levelScore) (int, error) {
|
||||
}
|
||||
|
||||
// Third, check the use of non-admin context.
|
||||
contextScore, maxContextScore := computeNonAdminContextScore(scores, maxScores)
|
||||
maxContextScore := maxScore.context * len(scores)
|
||||
contextScore := computeNonAdminContextScore(scores)
|
||||
score += noarmalizeScore(contextScore, maxContextScore, nonAdminContextLevel)
|
||||
if contextScore != maxContextScore {
|
||||
return int(score), nil
|
||||
}
|
||||
|
||||
// Fourth, check the thorough non-admin reviews.
|
||||
thoroughReviewScore, maxThoroughReviewScore := computeNonAdminThoroughReviewScore(scores, maxScores)
|
||||
maxThoroughReviewScore := maxScore.thoroughReview * len(scores)
|
||||
thoroughReviewScore := computeNonAdminThoroughReviewScore(scores)
|
||||
score += noarmalizeScore(thoroughReviewScore, maxThoroughReviewScore, nonAdminThoroughReviewLevel)
|
||||
if thoroughReviewScore != maxThoroughReviewScore {
|
||||
return int(score), nil
|
||||
@ -319,7 +214,8 @@ func computeScore(scores []levelScore) (int, error) {
|
||||
// Last, check the thorough admin review config.
|
||||
// This one is controversial and has usability issues
|
||||
// https://github.com/ossf/scorecard/issues/1027, so we may remove it.
|
||||
adminThoroughReviewScore, maxAdminThoroughReviewScore := computeAdminThoroughReviewScore(scores, maxScores)
|
||||
maxAdminThoroughReviewScore := maxScore.adminThoroughReview * len(scores)
|
||||
adminThoroughReviewScore := computeAdminThoroughReviewScore(scores)
|
||||
score += noarmalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel)
|
||||
if adminThoroughReviewScore != maxAdminThoroughReviewScore {
|
||||
return int(score), nil
|
||||
@ -328,6 +224,30 @@ func computeScore(scores []levelScore) (int, error) {
|
||||
return int(score), nil
|
||||
}
|
||||
|
||||
func info(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) {
|
||||
if !doLogging {
|
||||
return
|
||||
}
|
||||
|
||||
dl.Info(desc, args...)
|
||||
}
|
||||
|
||||
func debug(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) {
|
||||
if !doLogging {
|
||||
return
|
||||
}
|
||||
|
||||
dl.Debug(desc, args...)
|
||||
}
|
||||
|
||||
func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) {
|
||||
if !doLogging {
|
||||
return
|
||||
}
|
||||
|
||||
dl.Warn(desc, args...)
|
||||
}
|
||||
|
||||
func checkReleaseAndDevBranchProtection(
|
||||
repoClient clients.RepoClient, dl checker.DetailLogger) checker.CheckResult {
|
||||
// Get all branches. This will include information on whether they are protected.
|
||||
@ -395,23 +315,24 @@ func checkReleaseAndDevBranchProtection(
|
||||
// Protected field only indates that the branch matches
|
||||
// one `Branch protection rules`. All settings may be disabled,
|
||||
// so it does not provide any guarantees.
|
||||
if branch.Protected != nil && !*branch.Protected {
|
||||
protected := !(branch.Protected != nil && !*branch.Protected)
|
||||
if !protected {
|
||||
dl.Warn("branch protection not enabled for branch '%s'", b)
|
||||
scores = append(scores, score)
|
||||
continue
|
||||
}
|
||||
|
||||
// The branch is protected. Check the protection.
|
||||
score.protected = true
|
||||
score.scores.basic, score.maxes.basic = basicNonAdminProtection(&branch.BranchProtectionRule, b, dl)
|
||||
score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(&branch.BranchProtectionRule, b, dl)
|
||||
score.scores.review, score.maxes.review = nonAdminReviewProtection(&branch.BranchProtectionRule)
|
||||
score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(&branch.BranchProtectionRule, b, dl)
|
||||
score.scores.context, score.maxes.context = nonAdminContextProtection(&branch.BranchProtectionRule, b, dl)
|
||||
score.scores.basic, score.maxes.basic =
|
||||
basicNonAdminProtection(&branch.BranchProtectionRule, b, dl, protected)
|
||||
score.scores.adminBasic, score.maxes.adminBasic =
|
||||
basicAdminProtection(&branch.BranchProtectionRule, b, dl, protected)
|
||||
score.scores.review, score.maxes.review =
|
||||
nonAdminReviewProtection(&branch.BranchProtectionRule)
|
||||
score.scores.adminReview, score.maxes.adminReview =
|
||||
adminReviewProtection(&branch.BranchProtectionRule, b, dl, protected)
|
||||
score.scores.context, score.maxes.context =
|
||||
nonAdminContextProtection(&branch.BranchProtectionRule, b, dl, protected)
|
||||
score.scores.thoroughReview, score.maxes.thoroughReview =
|
||||
nonAdminThoroughReviewProtection(&branch.BranchProtectionRule, b, dl)
|
||||
nonAdminThoroughReviewProtection(&branch.BranchProtectionRule, b, dl, protected)
|
||||
score.scores.adminThoroughReview, score.maxes.adminThoroughReview =
|
||||
adminThoroughReviewProtection(&branch.BranchProtectionRule, b, dl) // Do we want this?
|
||||
adminThoroughReviewProtection(&branch.BranchProtectionRule, b, dl, protected) // Do we want this?
|
||||
|
||||
scores = append(scores, score)
|
||||
}
|
||||
@ -439,29 +360,29 @@ func checkReleaseAndDevBranchProtection(
|
||||
}
|
||||
|
||||
func basicNonAdminProtection(protection *clients.BranchProtectionRule,
|
||||
branch string, dl checker.DetailLogger) (int, int) {
|
||||
branch string, dl checker.DetailLogger, doLogging bool) (int, int) {
|
||||
score := 0
|
||||
max := 0
|
||||
|
||||
max += branchProtectionSettingScores[allowForcePushes]
|
||||
max++
|
||||
if protection.AllowForcePushes != nil {
|
||||
switch *protection.AllowForcePushes {
|
||||
case true:
|
||||
dl.Warn("'force pushes' enabled on branch '%s'", branch)
|
||||
warn(dl, doLogging, "'force pushes' enabled on branch '%s'", branch)
|
||||
case false:
|
||||
dl.Info("'force pushes' disabled on branch '%s'", branch)
|
||||
score += branchProtectionSettingScores[allowForcePushes]
|
||||
info(dl, doLogging, "'force pushes' disabled on branch '%s'", branch)
|
||||
score++
|
||||
}
|
||||
}
|
||||
|
||||
max += branchProtectionSettingScores[allowDeletions]
|
||||
max++
|
||||
if protection.AllowDeletions != nil {
|
||||
switch *protection.AllowDeletions {
|
||||
case true:
|
||||
dl.Warn("'allow deletion' enabled on branch '%s'", branch)
|
||||
warn(dl, doLogging, "'allow deletion' enabled on branch '%s'", branch)
|
||||
case false:
|
||||
dl.Info("'allow deletion' disabled on branch '%s'", branch)
|
||||
score += branchProtectionSettingScores[allowDeletions]
|
||||
info(dl, doLogging, "'allow deletion' disabled on branch '%s'", branch)
|
||||
score++
|
||||
}
|
||||
}
|
||||
|
||||
@ -469,43 +390,43 @@ func basicNonAdminProtection(protection *clients.BranchProtectionRule,
|
||||
}
|
||||
|
||||
func basicAdminProtection(protection *clients.BranchProtectionRule,
|
||||
branch string, dl checker.DetailLogger) (int, int) {
|
||||
branch string, dl checker.DetailLogger, doLogging bool) (int, int) {
|
||||
score := 0
|
||||
max := 0
|
||||
|
||||
// nil typically means we do not have access to the value.
|
||||
if protection.EnforceAdmins != nil {
|
||||
// Note: we don't inrecase max possible score for non-admin viewers.
|
||||
max += branchProtectionSettingScores[enforceAdmins]
|
||||
max++
|
||||
switch *protection.EnforceAdmins {
|
||||
case true:
|
||||
dl.Info("settings apply to administrators on branch '%s'", branch)
|
||||
score += branchProtectionSettingScores[enforceAdmins]
|
||||
info(dl, doLogging, "settings apply to administrators on branch '%s'", branch)
|
||||
score++
|
||||
case false:
|
||||
dl.Warn("settings do not apply to administrators on branch '%s'", branch)
|
||||
warn(dl, doLogging, "settings do not apply to administrators on branch '%s'", branch)
|
||||
}
|
||||
} else {
|
||||
dl.Debug("unable to retrieve whether or not settings apply to administrators on branch '%s'", branch)
|
||||
debug(dl, doLogging, "unable to retrieve whether or not settings apply to administrators on branch '%s'", branch)
|
||||
}
|
||||
|
||||
return score, max
|
||||
}
|
||||
|
||||
func nonAdminContextProtection(protection *clients.BranchProtectionRule, branch string,
|
||||
dl checker.DetailLogger) (int, int) {
|
||||
dl checker.DetailLogger, doLogging bool) (int, int) {
|
||||
score := 0
|
||||
max := 0
|
||||
// This means there are specific checks enabled.
|
||||
// If only `Requires status check to pass before merging` is enabled
|
||||
// but no specific checks are declared, it's equivalent
|
||||
// to having no status check at all.
|
||||
max += branchProtectionSettingScores[requireStatusChecksContexts]
|
||||
max++
|
||||
switch {
|
||||
case len(protection.CheckRules.Contexts) > 0:
|
||||
dl.Info("status check found to merge onto on branch '%s'", branch)
|
||||
score += branchProtectionSettingScores[requireStatusChecksContexts]
|
||||
info(dl, doLogging, "status check found to merge onto on branch '%s'", branch)
|
||||
score++
|
||||
default:
|
||||
dl.Warn("no status checks found to merge onto branch '%s'", branch)
|
||||
warn(dl, doLogging, "no status checks found to merge onto branch '%s'", branch)
|
||||
}
|
||||
return score, max
|
||||
}
|
||||
@ -514,75 +435,75 @@ func nonAdminReviewProtection(protection *clients.BranchProtectionRule) (int, in
|
||||
score := 0
|
||||
max := 0
|
||||
|
||||
max += branchProtectionSettingScores[requireApprovingReviewCount]
|
||||
max++
|
||||
if protection.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil &&
|
||||
*protection.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 {
|
||||
// We do not display anything here, it's done in nonAdminThoroughReviewProtection()
|
||||
score += branchProtectionSettingScores[requireApprovingReviewCount]
|
||||
score++
|
||||
}
|
||||
return score, max
|
||||
}
|
||||
|
||||
func adminReviewProtection(protection *clients.BranchProtectionRule, branch string,
|
||||
dl checker.DetailLogger) (int, int) {
|
||||
dl checker.DetailLogger, doLogging bool) (int, int) {
|
||||
score := 0
|
||||
max := 0
|
||||
|
||||
if protection.CheckRules.UpToDateBeforeMerge != nil {
|
||||
// Note: `This setting will not take effect unless at least one status check is enabled`.
|
||||
max += branchProtectionSettingScores[requireUpToDateBeforeMerge]
|
||||
max++
|
||||
switch *protection.CheckRules.UpToDateBeforeMerge {
|
||||
case true:
|
||||
dl.Info("status checks require up-to-date branches for '%s'", branch)
|
||||
score += branchProtectionSettingScores[requireUpToDateBeforeMerge]
|
||||
info(dl, doLogging, "status checks require up-to-date branches for '%s'", branch)
|
||||
score++
|
||||
default:
|
||||
dl.Warn("status checks do not require up-to-date branches for '%s'", branch)
|
||||
warn(dl, doLogging, "status checks do not require up-to-date branches for '%s'", branch)
|
||||
}
|
||||
} else {
|
||||
dl.Debug("unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", branch)
|
||||
debug(dl, doLogging, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", branch)
|
||||
}
|
||||
|
||||
return score, max
|
||||
}
|
||||
|
||||
func adminThoroughReviewProtection(protection *clients.BranchProtectionRule, branch string,
|
||||
dl checker.DetailLogger) (int, int) {
|
||||
dl checker.DetailLogger, doLogging bool) (int, int) {
|
||||
score := 0
|
||||
max := 0
|
||||
if protection.RequiredPullRequestReviews.DismissStaleReviews != nil {
|
||||
// Note: we don't inrecase max possible score for non-admin viewers.
|
||||
max += branchProtectionSettingScores[dismissStaleReviews]
|
||||
max++
|
||||
switch *protection.RequiredPullRequestReviews.DismissStaleReviews {
|
||||
case true:
|
||||
dl.Info("Stale review dismissal enabled on branch '%s'", branch)
|
||||
score += branchProtectionSettingScores[dismissStaleReviews]
|
||||
info(dl, doLogging, "Stale review dismissal enabled on branch '%s'", branch)
|
||||
score++
|
||||
case false:
|
||||
dl.Warn("Stale review dismissal disabled on branch '%s'", branch)
|
||||
warn(dl, doLogging, "Stale review dismissal disabled on branch '%s'", branch)
|
||||
}
|
||||
} else {
|
||||
dl.Debug("unable to retrieve review dismissal on branch '%s'", branch)
|
||||
debug(dl, doLogging, "unable to retrieve review dismissal on branch '%s'", branch)
|
||||
}
|
||||
return score, max
|
||||
}
|
||||
|
||||
func nonAdminThoroughReviewProtection(protection *clients.BranchProtectionRule, branch string,
|
||||
dl checker.DetailLogger) (int, int) {
|
||||
dl checker.DetailLogger, doLogging bool) (int, int) {
|
||||
score := 0
|
||||
max := 0
|
||||
|
||||
max += branchProtectionSettingScores[requireApprovingReviewCount]
|
||||
max++
|
||||
if protection.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
|
||||
switch *protection.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews {
|
||||
case true:
|
||||
dl.Info("number of required reviewers is %d on branch '%s'",
|
||||
info(dl, doLogging, "number of required reviewers is %d on branch '%s'",
|
||||
*protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch)
|
||||
score += branchProtectionSettingScores[requireApprovingReviewCount]
|
||||
score++
|
||||
default:
|
||||
dl.Warn("number of required reviewers is only %d on branch '%s'",
|
||||
warn(dl, doLogging, "number of required reviewers is only %d on branch '%s'",
|
||||
*protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch)
|
||||
}
|
||||
} else {
|
||||
dl.Warn("number of required reviewers is 0 on branch '%s'", branch)
|
||||
warn(dl, doLogging, "number of required reviewers is 0 on branch '%s'", branch)
|
||||
}
|
||||
return score, max
|
||||
}
|
||||
|
@ -53,15 +53,15 @@ func scrubBranches(branches []*clients.BranchRef) []*clients.BranchRef {
|
||||
func testScore(protection *clients.BranchProtectionRule,
|
||||
branch string, dl checker.DetailLogger) (int, error) {
|
||||
var score levelScore
|
||||
score.scores.basic, score.maxes.basic = basicNonAdminProtection(protection, branch, dl)
|
||||
score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(protection, branch, dl)
|
||||
score.scores.basic, score.maxes.basic = basicNonAdminProtection(protection, branch, dl, true)
|
||||
score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(protection, branch, dl, true)
|
||||
score.scores.review, score.maxes.review = nonAdminReviewProtection(protection)
|
||||
score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(protection, branch, dl)
|
||||
score.scores.context, score.maxes.context = nonAdminContextProtection(protection, branch, dl)
|
||||
score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(protection, branch, dl, true)
|
||||
score.scores.context, score.maxes.context = nonAdminContextProtection(protection, branch, dl, true)
|
||||
score.scores.thoroughReview, score.maxes.thoroughReview =
|
||||
nonAdminThoroughReviewProtection(protection, branch, dl)
|
||||
nonAdminThoroughReviewProtection(protection, branch, dl, true)
|
||||
score.scores.adminThoroughReview, score.maxes.adminThoroughReview =
|
||||
adminThoroughReviewProtection(protection, branch, dl)
|
||||
adminThoroughReviewProtection(protection, branch, dl, true)
|
||||
|
||||
return computeScore([]levelScore{score})
|
||||
}
|
||||
|
@ -54,6 +54,66 @@ var _ = Describe("E2E TEST:"+checks.CheckBranchProtection, func() {
|
||||
Expect(result.Error).Should(BeNil())
|
||||
Expect(result.Pass).Should(BeFalse())
|
||||
|
||||
// New version.
|
||||
Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue())
|
||||
Expect(repoClient.Close()).Should(BeNil())
|
||||
})
|
||||
It("Should fail to return branch protection on other repositories", func() {
|
||||
dl := scut.TestDetailLogger{}
|
||||
repo, err := githubrepo.MakeGithubRepo("ossf-tests/scorecard-check-branch-protection-e2e-none")
|
||||
Expect(err).Should(BeNil())
|
||||
repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger)
|
||||
err = repoClient.InitRepo(repo)
|
||||
Expect(err).Should(BeNil())
|
||||
req := checker.CheckRequest{
|
||||
Ctx: context.Background(),
|
||||
RepoClient: repoClient,
|
||||
Repo: repo,
|
||||
Dlogger: &dl,
|
||||
}
|
||||
expected := scut.TestReturn{
|
||||
Error: nil,
|
||||
Score: 0,
|
||||
NumberOfWarn: 2,
|
||||
NumberOfInfo: 0,
|
||||
NumberOfDebug: 0,
|
||||
}
|
||||
result := checks.BranchProtection(&req)
|
||||
// UPGRADEv2: to remove.
|
||||
// Old version.
|
||||
Expect(result.Error).Should(BeNil())
|
||||
Expect(result.Pass).Should(BeFalse())
|
||||
|
||||
// New version.
|
||||
Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue())
|
||||
Expect(repoClient.Close()).Should(BeNil())
|
||||
})
|
||||
It("Should fail to return branch protection on other repositories", func() {
|
||||
dl := scut.TestDetailLogger{}
|
||||
repo, err := githubrepo.MakeGithubRepo("ossf-tests/scorecard-check-branch-protection-e2e-patch-1")
|
||||
Expect(err).Should(BeNil())
|
||||
repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger)
|
||||
err = repoClient.InitRepo(repo)
|
||||
Expect(err).Should(BeNil())
|
||||
req := checker.CheckRequest{
|
||||
Ctx: context.Background(),
|
||||
RepoClient: repoClient,
|
||||
Repo: repo,
|
||||
Dlogger: &dl,
|
||||
}
|
||||
expected := scut.TestReturn{
|
||||
Error: nil,
|
||||
Score: 1,
|
||||
NumberOfWarn: 2,
|
||||
NumberOfInfo: 3,
|
||||
NumberOfDebug: 3,
|
||||
}
|
||||
result := checks.BranchProtection(&req)
|
||||
// UPGRADEv2: to remove.
|
||||
// Old version.
|
||||
Expect(result.Error).Should(BeNil())
|
||||
Expect(result.Pass).Should(BeFalse())
|
||||
|
||||
// New version.
|
||||
Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue())
|
||||
Expect(repoClient.Close()).Should(BeNil())
|
||||
|
Loading…
Reference in New Issue
Block a user