🐛 Ensure CODEOWNERS file exists for corresponding Branch-Protection c…

…heck (#2463) * Ensure CODEOWNERS file exists for corresponding Branch-Protection check * If CODEOWNERS file doesn't exist, CODEOWNERS branch protection is not in effect even if the setting is enabled Signed-off-by: Raghav Kaul <[email protected]> * cr comments Signed-off-by: Raghav Kaul <[email protected]> Signed-off-by: Raghav Kaul <[email protected]> Co-authored-by: laurentsimon <[email protected]>
ossf · Dec 14, 2022 · 746b6e9 · 746b6e9
1 parent 93ef087
commit 746b6e9
Show file tree

Hide file tree

Showing 8 changed files with 132 additions and 21 deletions.
diff --git a/checker/raw_result.go b/checker/raw_result.go
@@ -252,7 +252,8 @@ type WebhooksData struct {
 // BranchProtectionsData contains the raw results
 // for the Branch-Protection check.
 type BranchProtectionsData struct {
-	Branches []clients.BranchRef
+	Branches        []clients.BranchRef
+	CodeownersFiles []string
 }
 
 // Tool represents a tool.

diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -73,6 +73,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 		defaultBranch string
 		releases      []string
 		nonadmin      bool
+		repoFiles     []string
 	}{
 		{
 			name: "Nil release and main branch names",
@@ -172,7 +173,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  7,
+				NumberOfWarn:  8,
 				NumberOfInfo:  9,
 				NumberOfDebug: 0,
 			},
@@ -226,7 +227,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         8,
-				NumberOfWarn:  2,
+				NumberOfWarn:  4,
 				NumberOfInfo:  14,
 				NumberOfDebug: 0,
 			},
@@ -412,6 +413,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				DoAndReturn(func(b string) (*clients.BranchRef, error) {
 					return getBranch(tt.branches, b, tt.nonadmin), nil
 				}).AnyTimes()
+			mockRepoClient.EXPECT().ListFiles(gomock.Any()).AnyTimes().Return(tt.repoFiles, nil)
 			dl := scut.TestDetailLogger{}
 			req := checker.CheckRequest{
 				Dlogger:    &dl,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -78,7 +78,7 @@ func BranchProtection(name string, dl checker.DetailLogger,
 		score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(&b, dl)
 		// Do we want this?
 		score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(&b, dl)
-		score.scores.codeownerReview, score.maxes.codeownerReview = codeownersBranchProtection(&b, dl)
+		score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(&b, r.CodeownersFiles, dl)
 
 		scores = append(scores, score)
 	}
@@ -436,7 +436,9 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
 	return score, max
 }
 
-func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) {
+func codeownerBranchProtection(
+	branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger,
+) (int, int) {
 	score := 0
 	max := 1
 
@@ -446,7 +448,11 @@ func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogg
 		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
 		case true:
 			info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)
-			score++
+			if len(codeownersFiles) == 0 {
+				warn(dl, log, "codeowners branch protection is being ignored - but no codeowners file found in repo")
+			} else {
+				score++
+			}
 		default:
 			warn(dl, log, "codeowner review is not required on branch '%s'", *branch.Name)
 		}

diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -22,7 +22,7 @@ import (
 	scut "github.com/ossf/scorecard/v4/utests"
 )
 
-func testScore(branch *clients.BranchRef, dl checker.DetailLogger) (int, error) {
+func testScore(branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger) (int, error) {
 	var score levelScore
 	score.scores.basic, score.maxes.basic = basicNonAdminProtection(branch, dl)
 	score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(branch, dl)
@@ -31,7 +31,7 @@ func testScore(branch *clients.BranchRef, dl checker.DetailLogger) (int, error)
 	score.scores.context, score.maxes.context = nonAdminContextProtection(branch, dl)
 	score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(branch, dl)
 	score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(branch, dl)
-	score.scores.codeownerReview, score.maxes.codeownerReview = codeownersBranchProtection(branch, dl)
+	score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(branch, codeownersFiles, dl)
 
 	return computeScore([]levelScore{score})
 }
@@ -44,9 +44,10 @@ func TestIsBranchProtected(t *testing.T) {
 	var oneVal int32 = 1
 	branchVal := "branch-name"
 	tests := []struct {
-		name     string
-		branch   *clients.BranchRef
-		expected scut.TestReturn
+		name            string
+		branch          *clients.BranchRef
+		codeownersFiles []string
+		expected        scut.TestReturn
 	}{
 		{
 			name: "Nothing is enabled",
@@ -308,7 +309,7 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         8,
-				NumberOfWarn:  1,
+				NumberOfWarn:  2,
 				NumberOfInfo:  7,
 				NumberOfDebug: 0,
 			},
@@ -335,11 +336,42 @@ func TestIsBranchProtected(t *testing.T) {
 		},
 		{
 			name: "Branches are protected and require codeowner review",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         8,
+				NumberOfWarn:  1,
+				NumberOfInfo:  7,
+				NumberOfDebug: 0,
+			},
+			branch: &clients.BranchRef{
+				Name:      &branchVal,
+				Protected: &trueVal,
+				BranchProtectionRule: clients.BranchProtectionRule{
+					EnforceAdmins:        &trueVal,
+					RequireLinearHistory: &trueVal,
+					AllowForcePushes:     &falseVal,
+					AllowDeletions:       &falseVal,
+					CheckRules: clients.StatusChecksRule{
+						RequiresStatusChecks: &trueVal,
+						UpToDateBeforeMerge:  &trueVal,
+						Contexts:             []string{"foo"},
+					},
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						DismissStaleReviews:          &trueVal,
+						RequireCodeOwnerReviews:      &trueVal,
+						RequiredApprovingReviewCount: &oneVal,
+					},
+				},
+			},
+			codeownersFiles: []string{".github/CODEOWNERS"},
+		},
+		{
+			name: "Branches are protected and require codeowner review, but file is not present",
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         8,
 				NumberOfWarn:  2,
-				NumberOfInfo:  6,
+				NumberOfInfo:  7,
 				NumberOfDebug: 0,
 			},
 			branch: &clients.BranchRef{
@@ -357,7 +389,7 @@ func TestIsBranchProtected(t *testing.T) {
 					},
 					RequiredPullRequestReviews: clients.PullRequestReviewRule{
 						DismissStaleReviews:          &trueVal,
-						RequireCodeOwnerReviews:      &falseVal,
+						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,
 					},
 				},
@@ -369,7 +401,7 @@ func TestIsBranchProtected(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			dl := scut.TestDetailLogger{}
-			score, err := testScore(tt.branch, &dl)
+			score, err := testScore(tt.branch, tt.codeownersFiles, &dl)
 			actual := &checker.CheckResult{
 				Score: score,
 				Error: err,

diff --git a/checks/raw/branch_protection.go b/checks/raw/branch_protection.go
@@ -16,9 +16,11 @@ package raw
 
 import (
 	"fmt"
+	"reflect"
 	"regexp"
 
 	"github.com/ossf/scorecard/v4/checker"
+	"github.com/ossf/scorecard/v4/checks/fileparser"
 	"github.com/ossf/scorecard/v4/clients"
 )
 
@@ -105,12 +107,54 @@ func BranchProtection(c clients.RepoClient) (checker.BranchProtectionsData, erro
 		// Branch doesn't exist or was deleted. Continue.
 	}
 
+	codeownersFiles := []string{}
+	if err := collectCodeownersFiles(c, &codeownersFiles); err != nil {
+		return checker.BranchProtectionsData{}, err
+	}
+
 	// No error, return the data.
 	return checker.BranchProtectionsData{
-		Branches: branches.set,
+		Branches:        branches.set,
+		CodeownersFiles: codeownersFiles,
 	}, nil
 }
 
+func collectCodeownersFiles(c clients.RepoClient, codeownersFiles *[]string) error {
+	return fileparser.OnMatchingFileContentDo(c, fileparser.PathMatcher{
+		Pattern:       "CODEOWNERS",
+		CaseSensitive: true,
+	}, addCodeownersFile, codeownersFiles)
+}
+
+var addCodeownersFile fileparser.DoWhileTrueOnFileContent = func(
+	path string,
+	content []byte,
+	args ...interface{},
+) (bool, error) {
+	fmt.Printf("got codeowners file at %s\n", path)
+
+	if len(args) != 1 {
+		return false, fmt.Errorf(
+			"addCodeownersFile requires exactly 1 arguments: got %v: %w",
+			len(args), errInvalidArgLength)
+	}
+
+	codeownersFiles := dataAsStringSlicePtr(args[0])
+
+	*codeownersFiles = append(*codeownersFiles, path)
+
+	return true, nil
+}
+
+func dataAsStringSlicePtr(data interface{}) *[]string {
+	pdata, ok := data.(*[]string)
+	if !ok {
+		// panic if it is not correct type
+		panic(fmt.Sprintf("expected type *[]string, got %v", reflect.TypeOf(data)))
+	}
+	return pdata
+}
+
 func branchRedirect(name string) string {
 	// Ideally, we should check using repositories.GetBranch if there was a branch redirect.
 	// See https://github.com/google/go-github/issues/1895

diff --git a/checks/raw/branch_protection_test.go b/checks/raw/branch_protection_test.go
@@ -67,6 +67,7 @@ func TestBranchProtection(t *testing.T) {
 	tests := []struct {
 		name        string
 		branches    branchesArg
+		repoFiles   []string
 		releases    []clients.Release
 		releasesErr error
 		want        checker.BranchProtectionsData
@@ -80,6 +81,9 @@ func TestBranchProtection(t *testing.T) {
 					err:  errBPTest,
 				},
 			},
+			want: checker.BranchProtectionsData{
+				CodeownersFiles: []string{},
+			},
 		},
 		{
 			name: "null-default-branch-only",
@@ -90,6 +94,9 @@ func TestBranchProtection(t *testing.T) {
 					branchRef:     nil,
 				},
 			},
+			want: checker.BranchProtectionsData{
+				CodeownersFiles: []string{},
+			},
 		},
 		{
 			name: "default-branch-only",
@@ -108,6 +115,7 @@ func TestBranchProtection(t *testing.T) {
 						Name: &defaultBranchName,
 					},
 				},
+				CodeownersFiles: []string{},
 			},
 		},
 		{
@@ -117,6 +125,9 @@ func TestBranchProtection(t *testing.T) {
 		},
 		{
 			name: "no-releases",
+			want: checker.BranchProtectionsData{
+				CodeownersFiles: []string{},
+			},
 		},
 		{
 			name: "empty-targetcommitish",
@@ -155,6 +166,9 @@ func TestBranchProtection(t *testing.T) {
 					branchRef: nil,
 				},
 			},
+			want: checker.BranchProtectionsData{
+				CodeownersFiles: []string{},
+			},
 		},
 		{
 			name: "add-release-branch",
@@ -177,6 +191,7 @@ func TestBranchProtection(t *testing.T) {
 						Name: &releaseBranchName,
 					},
 				},
+				CodeownersFiles: []string{},
 			},
 		},
 		{
@@ -200,6 +215,7 @@ func TestBranchProtection(t *testing.T) {
 						Name: &mainBranchName,
 					},
 				},
+				CodeownersFiles: []string{},
 			},
 		},
 		{
@@ -233,6 +249,7 @@ func TestBranchProtection(t *testing.T) {
 						Name: &releaseBranchName,
 					},
 				},
+				CodeownersFiles: []string{},
 			},
 		},
 		// TODO: Add tests for commitSHA regex matching.
@@ -255,7 +272,7 @@ func TestBranchProtection(t *testing.T) {
 				DoAndReturn(func() ([]clients.Release, error) {
 					return tt.releases, tt.releasesErr
 				})
-
+			mockRepoClient.EXPECT().ListFiles(gomock.Any()).AnyTimes().Return(tt.repoFiles, nil)
 			rawData, err := BranchProtection(mockRepoClient)
 			if !errors.Is(err, tt.wantErr) {
 				t.Errorf("failed. expected: %v, got: %v", tt.wantErr, err)

diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go
@@ -47,7 +47,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
 			expected := scut.TestReturn{
 				Error:         nil,
 				Score:         6,
-				NumberOfWarn:  1,
+				NumberOfWarn:  2,
 				NumberOfInfo:  4,
 				NumberOfDebug: 3,
 			}

diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go
@@ -73,6 +73,11 @@ type jsonBranchProtection struct {
 	Name       string                        `json:"name"`
 }
 
+type jsonBranchProtectionMetadata struct {
+	Branches        []jsonBranchProtection `json:"branches"`
+	CodeownersFiles []string               `json:"codeownersFiles"`
+}
+
 type jsonReview struct {
 	State    string   `json:"state"`
 	Reviewer jsonUser `json:"reviewer"`
@@ -265,7 +270,7 @@ type jsonRawResults struct {
 	// Note: we return one at most.
 	DependencyUpdateTools []jsonTool `json:"dependencyUpdateTools"`
 	// Branch protection settings for development and release branches.
-	BranchProtections []jsonBranchProtection `json:"branchProtections"`
+	BranchProtections jsonBranchProtectionMetadata `json:"branchProtections"`
 	// Contributors. Note: we could use the list of commits instead to store this data.
 	// However, it's harder to get statistics using commit list, so we have a dedicated
 	// structure for it.
@@ -711,7 +716,7 @@ func (r *jsonScorecardRawResult) addDependencyUpdateToolRawResults(dut *checker.
 
 //nolint:unparam
 func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.BranchProtectionsData) error {
-	r.Results.BranchProtections = []jsonBranchProtection{}
+	branches := []jsonBranchProtection{}
 	for _, v := range bp.Branches {
 		var bp *jsonBranchProtectionSettings
 		if v.Protected != nil && *v.Protected {
@@ -728,11 +733,15 @@ func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.Branc
 				StatusCheckContexts:                 v.BranchProtectionRule.CheckRules.Contexts,
 			}
 		}
-		r.Results.BranchProtections = append(r.Results.BranchProtections, jsonBranchProtection{
+		branches = append(branches, jsonBranchProtection{
 			Name:       *v.Name,
 			Protection: bp,
 		})
 	}
+	r.Results.BranchProtections.Branches = branches
+
+	r.Results.BranchProtections.CodeownersFiles = bp.CodeownersFiles
+
 	return nil
 }