From edd371cf7dffe820b4458dc9f22336ad8924496f Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Tue, 24 May 2022 12:34:07 -0700 Subject: [PATCH] Replace `checker.BP` with `clients.BP` (#1953) Co-authored-by: Azeem Shaikh --- checker/raw_result.go | 22 +- checks/evaluation/branch_protection.go | 83 +++--- checks/evaluation/branch_protection_test.go | 304 ++++++++++++-------- checks/raw/branch_protection.go | 22 +- cron/format/json_raw_results.go | 24 +- pkg/json_raw_results.go | 24 +- 6 files changed, 248 insertions(+), 231 deletions(-) diff --git a/checker/raw_result.go b/checker/raw_result.go index 3b7bca03..d8c20914 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -113,27 +113,7 @@ type WebhooksData struct { // BranchProtectionsData contains the raw results // for the Branch-Protection check. type BranchProtectionsData struct { - Branches []BranchProtectionData -} - -// BranchProtectionData contains the raw results -// for one branch. -//nolint:govet -type BranchProtectionData struct { - Protected *bool - AllowsDeletions *bool - AllowsForcePushes *bool - RequiresCodeOwnerReviews *bool - RequiresLinearHistory *bool - DismissesStaleReviews *bool - EnforcesAdmins *bool - RequiresStatusChecks *bool - RequiresUpToDateBranchBeforeMerging *bool - RequiredApprovingReviewCount *int - // StatusCheckContexts is always available, so - // we don't use a pointer. - StatusCheckContexts []string - Name string + Branches []clients.BranchRef } // Tool represents a tool. diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 80326bf6..910679f4 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -18,6 +18,7 @@ import ( "fmt" "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" sce "github.com/ossf/scorecard/v4/errors" ) @@ -64,7 +65,7 @@ func BranchProtection(name string, dl checker.DetailLogger, protected := !(b.Protected != nil && !*b.Protected) if !protected { dl.Warn(&checker.LogMessage{ - Text: fmt.Sprintf("branch protection not enabled for branch '%s'", b.Name), + Text: fmt.Sprintf("branch protection not enabled for branch '%s'", *b.Name), }) } score.scores.basic, score.maxes.basic = basicNonAdminProtection(&b, dl) @@ -253,30 +254,30 @@ func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interfac }) } -func basicNonAdminProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) { +func basicNonAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 // Only log information if the branch is protected. log := branch.Protected != nil && *branch.Protected max++ - if branch.AllowsForcePushes != nil { - switch *branch.AllowsForcePushes { + if branch.BranchProtectionRule.AllowForcePushes != nil { + switch *branch.BranchProtectionRule.AllowForcePushes { case true: - warn(dl, log, "'force pushes' enabled on branch '%s'", branch.Name) + warn(dl, log, "'force pushes' enabled on branch '%s'", *branch.Name) case false: - info(dl, log, "'force pushes' disabled on branch '%s'", branch.Name) + info(dl, log, "'force pushes' disabled on branch '%s'", *branch.Name) score++ } } max++ - if branch.AllowsDeletions != nil { - switch *branch.AllowsDeletions { + if branch.BranchProtectionRule.AllowDeletions != nil { + switch *branch.BranchProtectionRule.AllowDeletions { case true: - warn(dl, log, "'allow deletion' enabled on branch '%s'", branch.Name) + warn(dl, log, "'allow deletion' enabled on branch '%s'", *branch.Name) case false: - info(dl, log, "'allow deletion' disabled on branch '%s'", branch.Name) + info(dl, log, "'allow deletion' disabled on branch '%s'", *branch.Name) score++ } } @@ -284,31 +285,31 @@ func basicNonAdminProtection(branch *checker.BranchProtectionData, dl checker.De return score, max } -func basicAdminProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) { +func basicAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 // Only log information if the branch is protected. log := branch.Protected != nil && *branch.Protected // nil typically means we do not have access to the value. - if branch.EnforcesAdmins != nil { + if branch.BranchProtectionRule.EnforceAdmins != nil { // Note: we don't inrecase max possible score for non-admin viewers. max++ - switch *branch.EnforcesAdmins { + switch *branch.BranchProtectionRule.EnforceAdmins { case true: - info(dl, log, "settings apply to administrators on branch '%s'", branch.Name) + info(dl, log, "settings apply to administrators on branch '%s'", *branch.Name) score++ case false: - warn(dl, log, "settings do not apply to administrators on branch '%s'", branch.Name) + warn(dl, log, "settings do not apply to administrators on branch '%s'", *branch.Name) } } else { - debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", branch.Name) + debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", *branch.Name) } return score, max } -func nonAdminContextProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) { +func nonAdminContextProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 // Only log information if the branch is protected. @@ -320,75 +321,75 @@ func nonAdminContextProtection(branch *checker.BranchProtectionData, dl checker. // to having no status check at all. max++ switch { - case len(branch.StatusCheckContexts) > 0: - info(dl, log, "status check found to merge onto on branch '%s'", branch.Name) + case len(branch.BranchProtectionRule.CheckRules.Contexts) > 0: + info(dl, log, "status check found to merge onto on branch '%s'", *branch.Name) score++ default: - warn(dl, log, "no status checks found to merge onto branch '%s'", branch.Name) + warn(dl, log, "no status checks found to merge onto branch '%s'", *branch.Name) } return score, max } -func nonAdminReviewProtection(branch *checker.BranchProtectionData) (int, int) { +func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) { score := 0 max := 0 max++ - if branch.RequiredApprovingReviewCount != nil && - *branch.RequiredApprovingReviewCount > 0 { + if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil && + *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 { // We do not display anything here, it's done in nonAdminThoroughReviewProtection() score++ } return score, max } -func adminReviewProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) { +func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 // Only log information if the branch is protected. log := branch.Protected != nil && *branch.Protected - if branch.RequiresUpToDateBranchBeforeMerging != nil { + if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge != nil { // Note: `This setting will not take effect unless at least one status check is enabled`. max++ - switch *branch.RequiresUpToDateBranchBeforeMerging { + switch *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge { case true: - info(dl, log, "status checks require up-to-date branches for '%s'", branch.Name) + info(dl, log, "status checks require up-to-date branches for '%s'", *branch.Name) score++ default: - warn(dl, log, "status checks do not require up-to-date branches for '%s'", branch.Name) + warn(dl, log, "status checks do not require up-to-date branches for '%s'", *branch.Name) } } else { - debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", branch.Name) + debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name) } return score, max } -func adminThoroughReviewProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) { +func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 // Only log information if the branch is protected. log := branch.Protected != nil && *branch.Protected - if branch.DismissesStaleReviews != nil { + if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil { // Note: we don't inrecase max possible score for non-admin viewers. max++ - switch *branch.DismissesStaleReviews { + switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews { case true: - info(dl, log, "Stale review dismissal enabled on branch '%s'", branch.Name) + info(dl, log, "Stale review dismissal enabled on branch '%s'", *branch.Name) score++ case false: - warn(dl, log, "Stale review dismissal disabled on branch '%s'", branch.Name) + warn(dl, log, "Stale review dismissal disabled on branch '%s'", *branch.Name) } } else { - debug(dl, log, "unable to retrieve review dismissal on branch '%s'", branch.Name) + debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name) } return score, max } -func nonAdminThoroughReviewProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) { +func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 @@ -396,18 +397,18 @@ func nonAdminThoroughReviewProtection(branch *checker.BranchProtectionData, dl c log := branch.Protected != nil && *branch.Protected max++ - if branch.RequiredApprovingReviewCount != nil { - switch *branch.RequiredApprovingReviewCount >= minReviews { + if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil { + switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews { case true: info(dl, log, "number of required reviewers is %d on branch '%s'", - *branch.RequiredApprovingReviewCount, branch.Name) + *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name) score++ default: warn(dl, log, "number of required reviewers is only %d on branch '%s'", - *branch.RequiredApprovingReviewCount, branch.Name) + *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name) } } else { - warn(dl, log, "number of required reviewers is 0 on branch '%s'", branch.Name) + warn(dl, log, "number of required reviewers is 0 on branch '%s'", *branch.Name) } return score, max } diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index 7aa0aee8..3bab8929 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -18,10 +18,11 @@ import ( "testing" "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" scut "github.com/ossf/scorecard/v4/utests" ) -func testScore(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, error) { +func testScore(branch *clients.BranchRef, dl checker.DetailLogger) (int, error) { var score levelScore score.scores.basic, score.maxes.basic = basicNonAdminProtection(branch, dl) score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(branch, dl) @@ -38,12 +39,12 @@ func TestIsBranchProtected(t *testing.T) { t.Parallel() trueVal := true falseVal := false - var zeroVal int - oneVal := 1 - + var zeroVal int32 + var oneVal int32 = 1 + branchVal := "branch-name" tests := []struct { name string - branch *checker.BranchProtectionData + branch *clients.BranchRef expected scut.TestReturn }{ { @@ -55,19 +56,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 2, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &trueVal, - RequiresUpToDateBranchBeforeMerging: &falseVal, - StatusCheckContexts: nil, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - EnforcesAdmins: &falseVal, - RequiresLinearHistory: &falseVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + }, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &trueVal, + Contexts: nil, + UpToDateBeforeMerge: &falseVal, + }, + }, }, }, { @@ -79,8 +86,8 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 0, NumberOfDebug: 3, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", + branch: &clients.BranchRef{ + Name: &branchVal, Protected: &trueVal, }, }, @@ -93,19 +100,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 4, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &trueVal, - RequiresUpToDateBranchBeforeMerging: &trueVal, - StatusCheckContexts: []string{"foo"}, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - EnforcesAdmins: &falseVal, - RequiresLinearHistory: &falseVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + }, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &trueVal, + UpToDateBeforeMerge: &trueVal, + Contexts: []string{"foo"}, + }, + EnforceAdmins: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + }, }, }, { @@ -117,19 +130,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 3, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &trueVal, - RequiresUpToDateBranchBeforeMerging: &trueVal, - StatusCheckContexts: nil, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - EnforcesAdmins: &falseVal, - RequiresLinearHistory: &falseVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + }, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &trueVal, + UpToDateBeforeMerge: &trueVal, + Contexts: nil, + }, + }, }, }, { @@ -141,19 +160,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 3, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &trueVal, - RequiresUpToDateBranchBeforeMerging: &falseVal, - StatusCheckContexts: []string{"foo"}, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &oneVal, - EnforcesAdmins: &falseVal, - RequiresLinearHistory: &trueVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &trueVal, + UpToDateBeforeMerge: &falseVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &oneVal, + }, + }, }, }, { @@ -165,19 +190,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 4, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &falseVal, - RequiresUpToDateBranchBeforeMerging: &falseVal, - StatusCheckContexts: []string{"foo"}, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - EnforcesAdmins: &trueVal, - RequiresLinearHistory: &trueVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &falseVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + }, + }, }, }, { @@ -189,19 +220,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 3, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &falseVal, - RequiresUpToDateBranchBeforeMerging: &falseVal, - StatusCheckContexts: []string{"foo"}, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - EnforcesAdmins: &falseVal, - RequiresLinearHistory: &trueVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &falseVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + }, + }, }, }, { @@ -213,19 +250,26 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 2, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &falseVal, - RequiresUpToDateBranchBeforeMerging: &falseVal, - StatusCheckContexts: []string{"foo"}, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - EnforcesAdmins: &falseVal, - RequiresLinearHistory: &falseVal, - AllowsForcePushes: &trueVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &trueVal, + AllowDeletions: &falseVal, + + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &falseVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + }, + }, }, }, { @@ -237,19 +281,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 2, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &falseVal, - RequiresUpToDateBranchBeforeMerging: &falseVal, - StatusCheckContexts: []string{"foo"}, - DismissesStaleReviews: &falseVal, - RequiresCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - EnforcesAdmins: &falseVal, - RequiresLinearHistory: &falseVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &trueVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &trueVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &falseVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &falseVal, + RequireCodeOwnerReviews: &falseVal, + RequiredApprovingReviewCount: &zeroVal, + }, + }, }, }, { @@ -261,19 +311,25 @@ func TestIsBranchProtected(t *testing.T) { NumberOfInfo: 6, NumberOfDebug: 0, }, - branch: &checker.BranchProtectionData{ - Name: "branch-name", - Protected: &trueVal, - RequiresStatusChecks: &falseVal, - RequiresUpToDateBranchBeforeMerging: &trueVal, - StatusCheckContexts: []string{"foo"}, - DismissesStaleReviews: &trueVal, - RequiresCodeOwnerReviews: &trueVal, - RequiredApprovingReviewCount: &oneVal, - EnforcesAdmins: &trueVal, - RequiresLinearHistory: &trueVal, - AllowsForcePushes: &falseVal, - AllowsDeletions: &falseVal, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &falseVal, + UpToDateBeforeMerge: &trueVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &trueVal, + RequireCodeOwnerReviews: &trueVal, + RequiredApprovingReviewCount: &oneVal, + }, + }, }, }, } diff --git a/checks/raw/branch_protection.go b/checks/raw/branch_protection.go index 7881d9b3..dee511e6 100644 --- a/checks/raw/branch_protection.go +++ b/checks/raw/branch_protection.go @@ -89,27 +89,7 @@ func BranchProtection(c clients.RepoClient) (checker.BranchProtectionsData, erro return checker.BranchProtectionsData{}, err } - // Protected field only indates that the branch matches - // one `Branch protection rules`. All settings may be disabled, - // so it does not provide any guarantees. - protected := !(branch.Protected != nil && !*branch.Protected) - bpData := checker.BranchProtectionData{Name: b} - bp := branch.BranchProtectionRule - bpData.Protected = &protected - bpData.RequiresLinearHistory = bp.RequireLinearHistory - bpData.AllowsForcePushes = bp.AllowForcePushes - bpData.AllowsDeletions = bp.AllowDeletions - bpData.EnforcesAdmins = bp.EnforceAdmins - bpData.RequiresCodeOwnerReviews = bp.RequiredPullRequestReviews.RequireCodeOwnerReviews - bpData.DismissesStaleReviews = bp.RequiredPullRequestReviews.DismissStaleReviews - bpData.RequiresUpToDateBranchBeforeMerging = bp.CheckRules.UpToDateBeforeMerge - if bp.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil { - v := int(*bp.RequiredPullRequestReviews.RequiredApprovingReviewCount) - bpData.RequiredApprovingReviewCount = &v - } - bpData.StatusCheckContexts = bp.CheckRules.Contexts - - rawData.Branches = append(rawData.Branches, bpData) + rawData.Branches = append(rawData.Branches, *branch) } // No error, return the data. diff --git a/cron/format/json_raw_results.go b/cron/format/json_raw_results.go index 5109f85c..789eb981 100644 --- a/cron/format/json_raw_results.go +++ b/cron/format/json_raw_results.go @@ -48,7 +48,7 @@ type jsonTool struct { } type jsonBranchProtectionSettings struct { - RequiredApprovingReviewCount *int `json:"requiredReviewerCount"` + RequiredApprovingReviewCount *int32 `json:"requiredReviewerCount"` AllowsDeletions *bool `json:"allowsDeletions"` AllowsForcePushes *bool `json:"allowsForcePushes"` RequiresCodeOwnerReviews *bool `json:"requiresCodeOwnerReview"` @@ -223,20 +223,20 @@ func addBranchProtectionRawResults(r *jsonScorecardRawResult, bp *checker.Branch var bp *jsonBranchProtectionSettings if v.Protected != nil && *v.Protected { bp = &jsonBranchProtectionSettings{ - AllowsDeletions: v.AllowsDeletions, - AllowsForcePushes: v.AllowsForcePushes, - RequiresCodeOwnerReviews: v.RequiresCodeOwnerReviews, - RequiresLinearHistory: v.RequiresLinearHistory, - DismissesStaleReviews: v.DismissesStaleReviews, - EnforcesAdmins: v.EnforcesAdmins, - RequiresStatusChecks: v.RequiresStatusChecks, - RequiresUpToDateBranchBeforeMerging: v.RequiresUpToDateBranchBeforeMerging, - RequiredApprovingReviewCount: v.RequiredApprovingReviewCount, - StatusCheckContexts: v.StatusCheckContexts, + 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, } } r.Results.BranchProtections = append(r.Results.BranchProtections, jsonBranchProtection{ - Name: v.Name, + Name: *v.Name, Protection: bp, }) } diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 56d2e7ef..48576529 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -53,7 +53,7 @@ type jsonTool struct { } type jsonBranchProtectionSettings struct { - RequiredApprovingReviewCount *int `json:"required-reviewer-count"` + RequiredApprovingReviewCount *int32 `json:"required-reviewer-count"` AllowsDeletions *bool `json:"allows-deletions"` AllowsForcePushes *bool `json:"allows-force-pushes"` RequiresCodeOwnerReviews *bool `json:"requires-code-owner-review"` @@ -495,20 +495,20 @@ func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.Branc var bp *jsonBranchProtectionSettings if v.Protected != nil && *v.Protected { bp = &jsonBranchProtectionSettings{ - AllowsDeletions: v.AllowsDeletions, - AllowsForcePushes: v.AllowsForcePushes, - RequiresCodeOwnerReviews: v.RequiresCodeOwnerReviews, - RequiresLinearHistory: v.RequiresLinearHistory, - DismissesStaleReviews: v.DismissesStaleReviews, - EnforcesAdmins: v.EnforcesAdmins, - RequiresStatusChecks: v.RequiresStatusChecks, - RequiresUpToDateBranchBeforeMerging: v.RequiresUpToDateBranchBeforeMerging, - RequiredApprovingReviewCount: v.RequiredApprovingReviewCount, - StatusCheckContexts: v.StatusCheckContexts, + 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, } } r.Results.BranchProtections = append(r.Results.BranchProtections, jsonBranchProtection{ - Name: v.Name, + Name: *v.Name, Protection: bp, }) }