diff --git a/checker/raw_result.go b/checker/raw_result.go index e4dc93dc3f3..986ad75c15a 100644 --- 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 index 6c0c601fa3f..ee1aca1ab47 100644 --- 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 index de9925f9f63..a9ff288bb34 100644 --- 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 index f0ec934cca9..caddfa3fcc4 100644 --- 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 index 68b0fea1cd8..fee6dad65a8 100644 --- 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 index d41c735d588..8242a944447 100644 --- 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 index 91f53ceb057..18254a12b0e 100644 --- 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 index eb09b65c568..9fcabe06580 100644 --- a/clients/gitlabrepo/commits.go +++ b/clients/gitlabrepo/commits.go @@ -139,6 +139,7 @@ func (handler *commitsHandler) setup() error { 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 index 84ad78c5a97..2905e2487c9 100644 --- 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 index e9fc55f7717..f18b51a4a05 100644 --- a/e2e/code_review_test.go +++ b/e2e/code_review_test.go @@ -16,19 +16,20 @@ package e2e import ( "context" + "fmt" . "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" 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() { @@ -81,5 +82,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()) + }) }) }) diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 42d071aa4ab..eb5690c9e69 100644 --- 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, }, ) }