🌱 Code Review: treat merging a PR as code review (ossf#2413)

* Merges on Github count as a code review by the maintainer Signed-off-by: Raghav Kaul <[email protected]> * Update Raw Results * More detailed information for Changesets * If there's no Revision ID, use the Commit SHA instead Signed-off-by: Raghav Kaul <[email protected]> * Check that pull request had atleast one reviewer that wasn't its author * Add field for Pull Request Merged-By to Github and Gitlab * Note, this check can be bypassed if an author opens a PR with other people's commits Signed-off-by: Raghav Kaul <[email protected]> Signed-off-by: Raghav Kaul <[email protected]> Signed-off-by: nathaniel.wert <[email protected]>
N8BWert · Nov 28, 2022 · 9d5e92d · 9d5e92d
1 parent 7f2d009
commit 9d5e92d
Show file tree

Hide file tree

Showing 11 changed files with 154 additions and 43 deletions.
diff --git a/checker/raw_result.go b/checker/raw_result.go
@@ -150,6 +150,8 @@ type Changeset struct {
 	ReviewPlatform string
 	RevisionID     string
 	Commits        []clients.Commit
+	Reviews        []clients.Review
+	Author         clients.User
 }
 
 // ContributorsData represents contributor information.

diff --git a/checks/code_review_test.go b/checks/code_review_test.go
@@ -73,14 +73,15 @@ func TestCodereview(t *testing.T) {
 				{
 					SHA: "sha",
 					Committer: clients.User{
-						Login: "user",
+						Login: "bob",
 					},
 					AssociatedMergeRequest: clients.PullRequest{
 						Number:   1,
 						MergedAt: time.Now(),
 						Reviews: []clients.Review{
 							{
-								State: "APPROVED",
+								Author: &clients.User{Login: "alice"},
+								State:  "APPROVED",
 							},
 						},
 					},
@@ -159,10 +160,10 @@ func TestCodereview(t *testing.T) {
 			},
 		},
 		{
-			name: "2 PRs 2 review on GitHub",
+			name: "2 PRs 1 review on GitHub",
 			commits: []clients.Commit{
 				{
-					SHA: "sha",
+					SHA: "a",
 					Committer: clients.User{
 						Login: "bob",
 					},
@@ -171,7 +172,8 @@ func TestCodereview(t *testing.T) {
 						MergedAt: time.Now(),
 						Reviews: []clients.Review{
 							{
-								State: "APPROVED",
+								Author: &clients.User{Login: "alice"},
+								State:  "APPROVED",
 							},
 						},
 					},
@@ -187,6 +189,31 @@ func TestCodereview(t *testing.T) {
 				Score: 5,
 			},
 		},
+		{
+			name: "implicit maintainer approval through merge",
+			commits: []clients.Commit{
+				{
+					SHA: "abc",
+					Committer: clients.User{
+						Login: "bob",
+					},
+					AssociatedMergeRequest: clients.PullRequest{
+						Number:   1,
+						MergedAt: time.Now(),
+						MergedBy: clients.User{Login: "alice"},
+					},
+				},
+				{
+					SHA: "def",
+					Committer: clients.User{
+						Login: "bob",
+					},
+				},
+			},
+			expected: checker.CheckResult{
+				Score: 5,
+			},
+		},
 		{
 			name: "Valid Phabricator commit",
 			commits: []clients.Commit{

diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go
@@ -61,9 +61,10 @@ func reviewScoreForChangeset(changeset *checker.Changeset) (score reviewScore) {
 		return reviewedOutsideGithub
 	}
 
-	for i := range changeset.Commits {
-		for _, review := range changeset.Commits[i].AssociatedMergeRequest.Reviews {
-			if review.State == "APPROVED" {
+	if changeset.ReviewPlatform == checker.ReviewPlatformGitHub {
+		for i := range changeset.Reviews {
+			review := changeset.Reviews[i]
+			if review.State == "APPROVED" && review.Author.Login != changeset.Author.Login {
 				return changesReviewed
 			}
 		}

diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go
@@ -71,18 +71,13 @@ func TestCodeReview(t *testing.T) {
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
 					{
+						Author:         clients.User{Login: "alice"},
 						ReviewPlatform: checker.ReviewPlatformGitHub,
 						RevisionID:     "1",
-						Commits: []clients.Commit{
+						Reviews: []clients.Review{
 							{
-								SHA: "1",
-								AssociatedMergeRequest: clients.PullRequest{
-									Reviews: []clients.Review{
-										{
-											State: "APPROVED",
-										},
-									},
-								},
+								Author: &clients.User{},
+								State:  "APPROVED",
 							},
 						},
 					},

diff --git a/checks/raw/code_review.go b/checks/raw/code_review.go
@@ -51,6 +51,20 @@ func getGithubRevisionID(c *clients.Commit) string {
 	return ""
 }
 
+func getGithubReviews(c *clients.Commit) (reviews []clients.Review) {
+	reviews = []clients.Review{}
+	reviews = append(reviews, c.AssociatedMergeRequest.Reviews...)
+
+	if !c.AssociatedMergeRequest.MergedAt.IsZero() {
+		reviews = append(reviews, clients.Review{Author: &c.AssociatedMergeRequest.MergedBy, State: "APPROVED"})
+	}
+	return
+}
+
+func getGithubAuthor(c *clients.Commit) (author clients.User) {
+	return c.AssociatedMergeRequest.Author
+}
+
 func getProwRevisionID(c *clients.Commit) string {
 	mr := c.AssociatedMergeRequest
 	if !c.AssociatedMergeRequest.MergedAt.IsZero() {
@@ -143,6 +157,9 @@ func getChangesets(commits []clients.Commit) []checker.Changeset {
 
 	for i := range commits {
 		rev := detectCommitRevisionInfo(&commits[i])
+		if rev.ID == "" {
+			rev.ID = commits[i].SHA
+		}
 
 		if changeset, ok := changesetsByRevInfo[rev]; !ok {
 			newChangeset := checker.Changeset{
@@ -151,6 +168,11 @@ func getChangesets(commits []clients.Commit) []checker.Changeset {
 				Commits:        []clients.Commit{commits[i]},
 			}
 
+			if rev.Platform == checker.ReviewPlatformGitHub {
+				newChangeset.Reviews = getGithubReviews(&commits[i])
+				newChangeset.Author = getGithubAuthor(&commits[i])
+			}
+
 			changesetsByRevInfo[rev] = newChangeset
 		} else {
 			// Part of a previously found changeset.
@@ -160,8 +182,9 @@ func getChangesets(commits []clients.Commit) []checker.Changeset {
 	}
 
 	// Changesets are returned in map order (i.e. randomized)
-	for ri, cs := range changesetsByRevInfo {
+	for ri := range changesetsByRevInfo {
 		// Ungroup all commits that don't have revision info
+		cs := changesetsByRevInfo[ri]
 		missing := revisionInfo{}
 		if ri == missing {
 			for i := range cs.Commits {

diff --git a/checks/raw/code_review_test.go b/checks/raw/code_review_test.go
@@ -34,7 +34,7 @@ func assertCommitEq(t *testing.T, actual clients.Commit, expected clients.Commit
 	}
 }
 
-func assertChangesetEq(t *testing.T, actual, expected checker.Changeset) {
+func assertChangesetEq(t *testing.T, actual, expected *checker.Changeset) {
 	t.Helper()
 
 	if actual.ReviewPlatform != expected.ReviewPlatform {
@@ -66,6 +66,7 @@ func assertChangesetEq(t *testing.T, actual, expected checker.Changeset) {
 	}
 }
 
+//nolint:gocritic
 func csless(a, b checker.Changeset) bool {
 	if cmp := strings.Compare(a.RevisionID, b.RevisionID); cmp != 0 {
 		return cmp < 0
@@ -85,7 +86,7 @@ func assertChangesetArrEq(t *testing.T, actual, expected []checker.Changeset) {
 	slices.SortFunc(expected, csless)
 
 	for i := range actual {
-		assertChangesetEq(t, actual[i], expected[i])
+		assertChangesetEq(t, &actual[i], &expected[i])
 	}
 }
 

diff --git a/clients/githubrepo/graphql.go b/clients/githubrepo/graphql.go
@@ -94,6 +94,9 @@ type graphqlData struct {
 										}
 									}
 								} `graphql:"reviews(last: $reviewsToAnalyze)"`
+								MergedBy struct {
+									Login githubv4.String
+								}
 							}
 						} `graphql:"associatedPullRequests(first: $pullRequestsToAnalyze)"`
 					}
@@ -356,6 +359,9 @@ func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commi
 				Author: clients.User{
 					Login: string(pr.Author.Login),
 				},
+				MergedBy: clients.User{
+					Login: string(pr.MergedBy.Login),
+				},
 			}
 			for _, label := range pr.Labels.Nodes {
 				associatedPR.Labels = append(associatedPR.Labels, clients.Label{

diff --git a/clients/gitlabrepo/commits.go b/clients/gitlabrepo/commits.go
@@ -128,25 +128,23 @@ func (handler *commitsHandler) setup() error {
 				})
 			}
 
-			commit := clients.Commit{
-				CommittedDate: *commit.CommittedDate,
-				Message:       commit.Message,
-				SHA:           commit.ID,
-				AssociatedMergeRequest: clients.PullRequest{
-					Number:   mergeRequest.ID,
-					MergedAt: *mergeRequest.MergedAt,
-					HeadSHA:  mergeRequest.SHA,
-					Author:   clients.User{ID: int64(mergeRequest.Author.ID)},
-					Labels:   labels,
-					Reviews:  reviews,
-				},
-			}
-
-			if user != nil {
-				commit.Committer = clients.User{ID: int64(user.ID)}
-			}
-
-			handler.commits = append(handler.commits, commit)
+			// append the commits to the handler.
+			handler.commits = append(handler.commits,
+				clients.Commit{
+					CommittedDate: *commit.CommittedDate,
+					Message:       commit.Message,
+					SHA:           commit.ID,
+					AssociatedMergeRequest: clients.PullRequest{
+						Number:   mergeRequest.ID,
+						MergedAt: *mergeRequest.MergedAt,
+						HeadSHA:  mergeRequest.SHA,
+						Author:   clients.User{ID: int64(mergeRequest.Author.ID)},
+						Labels:   labels,
+						Reviews:  reviews,
+						MergedBy: clients.User{ID: int64(mergeRequest.MergedBy.ID)},
+					},
+					Committer: clients.User{ID: int64(user.ID)},
+				})
 		}
 	})
 

diff --git a/clients/pull_request.go b/clients/pull_request.go
@@ -28,6 +28,7 @@ type PullRequest struct {
 	Author   User
 	Labels   []Label
 	Reviews  []Review
+	MergedBy User
 }
 
 // Label represents a PR label.

diff --git a/e2e/code_review_test.go b/e2e/code_review_test.go
@@ -16,21 +16,25 @@ package e2e
 
 import (
 	"context"
+<<<<<<< HEAD
 	"os"
+=======
+	"fmt"
+>>>>>>> 4063fb6 (🌱 Code Review: treat merging a PR as code review (#2413))
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/checks"
+	"github.com/ossf/scorecard/v4/checks/raw"
 	"github.com/ossf/scorecard/v4/clients"
 	"github.com/ossf/scorecard/v4/clients/githubrepo"
 	"github.com/ossf/scorecard/v4/clients/gitlabrepo"
 	scut "github.com/ossf/scorecard/v4/utests"
 )
 
 // TODO: use dedicated repo that don't change.
-// TODO: need negative results.
 var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 	Context("E2E TEST:Validating use of code reviews", func() {
 		It("Should return use of code reviews", func() {
@@ -83,6 +87,27 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
 			Expect(repoClient.Close()).Should(BeNil())
 		})
+		It("Should return use of implicit code reviews at commit", func() {
+			repo, err := githubrepo.MakeGithubRepo("spring-projects/spring-framework")
+			Expect(err).Should(BeNil())
+			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger)
+			err = repoClient.InitRepo(repo, "ca5e453f87f7e84033bb90a2fb54ee9f7fc94d61")
+			Expect(err).Should(BeNil())
+
+			reviewData, err := raw.CodeReview(repoClient)
+			Expect(err).Should(BeNil())
+			Expect(reviewData.DefaultBranchChangesets).ShouldNot(BeEmpty())
+
+			gh := 0
+			for _, cs := range reviewData.DefaultBranchChangesets {
+				if cs.ReviewPlatform == checker.ReviewPlatformGitHub {
+					fmt.Printf("found github revision %s in spring-framework", cs.RevisionID)
+					gh += 1
+				}
+			}
+			Expect(gh).Should(BeNumerically("==", 2))
+			Expect(repoClient.Close()).Should(BeNil())
+		})
 		// GitLab doesn't seem to preserve merge requests (pull requests in github) and some users had data lost in
 		// the transfer from github so this returns a different value than the above GitHub test.
 		It("Should return use of code reviews - GitLab", func() {

diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go
@@ -84,7 +84,7 @@ type jsonUser struct {
 	Organizations []jsonOrganization `json:"organization,omitempty"`
 	// Companies refer to a claim by a user in their profile.
 	Companies        []jsonCompany `json:"company,omitempty"`
-	NumContributions int           `json:"NumContributions"`
+	NumContributions int           `json:"NumContributions,omitempty"`
 }
 
 type jsonContributors struct {
@@ -540,10 +540,42 @@ func (r *jsonScorecardRawResult) setDefaultCommitData(changesets []checker.Chang
 			})
 		}
 
+		reviews := []jsonReview{}
+		for j := range cs.Commits {
+			mr := cs.Commits[j].AssociatedMergeRequest
+			if mr.Reviews == nil {
+				continue
+			}
+			for k := range mr.Reviews {
+				r := mr.Reviews[k]
+				reviews = append(reviews, jsonReview{
+					State: r.State,
+					Reviewer: jsonUser{
+						Login: r.Author.Login,
+					},
+				})
+			}
+		}
+
+		// Only add the Merge Request opener as the PR author
+		authors := []jsonUser{}
+		for j := range cs.Commits {
+			mr := cs.Commits[j].AssociatedMergeRequest
+			if mr.Author.Login != "" {
+				authors = append(authors, jsonUser{
+					Login: mr.Author.Login,
+				})
+				break
+			}
+		}
+
 		r.Results.DefaultBranchChangesets = append(r.Results.DefaultBranchChangesets,
 			jsonDefaultBranchChangeset{
-				RevisionID: cs.RevisionID,
-				Commits:    commits,
+				RevisionID:     cs.RevisionID,
+				ReviewPlatform: cs.ReviewPlatform,
+				Commits:        commits,
+				Reviews:        reviews,
+				Authors:        authors,
 			},
 		)
 	}