Skip to content

Commit

Permalink
Use ListReleases API for BranchProtection check (#937)
Browse files Browse the repository at this point in the history 
Co-authored-by: Azeem Shaikh <azeems@google.com>
  • Loading branch information
@azeemshaikh38 @azeemsgoogle
azeemshaikh38 and azeemsgoogle committed Aug 31, 2021
1 parent 9a1978a commit e305a94
Show file tree
Hide file tree
Showing 7 changed files with 133 additions and 104 deletions.
24 changes: 7 additions & 17 deletions checks/branch_protection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,10 @@
package checks

import (
"context"
"errors"
"fmt"
"regexp"

"github.com/google/go-github/v38/github"

"github.com/ossf/scorecard/v2/checker"
"github.com/ossf/scorecard/v2/clients"
sce "github.com/ossf/scorecard/v2/errors"
Expand All @@ -38,12 +35,6 @@ func init() {
registerCheck(CheckBranchProtection, BranchProtection)
}

// TODO: Use RepoClient interface instead of this.
type repositories interface {
ListReleases(ctx context.Context, owner string, repo string, opts *github.ListOptions) (
[]*github.RepositoryRelease, *github.Response, error)
}

type branchMap map[string]*clients.BranchRef

func (b branchMap) getBranchByName(name string) (*clients.BranchRef, error) {
Expand Down Expand Up @@ -75,12 +66,11 @@ func getBranchMapFrom(branches []*clients.BranchRef) branchMap {
// BranchProtection runs Branch-Protection check.
func BranchProtection(c *checker.CheckRequest) checker.CheckResult {
// Checks branch protection on both release and development branch.
return checkReleaseAndDevBranchProtection(c.Ctx, c.RepoClient, c.Client.Repositories, c.Dlogger, c.Owner, c.Repo)
return checkReleaseAndDevBranchProtection(c.RepoClient, c.Dlogger)
}

func checkReleaseAndDevBranchProtection(ctx context.Context,
repoClient clients.RepoClient, r repositories, dl checker.DetailLogger,
ownerStr, repoStr string) checker.CheckResult {
func checkReleaseAndDevBranchProtection(
repoClient clients.RepoClient, dl checker.DetailLogger) checker.CheckResult {
// Get all branches. This will include information on whether they are protected.
branches, err := repoClient.ListBranches()
if err != nil {
Expand All @@ -89,7 +79,7 @@ func checkReleaseAndDevBranchProtection(ctx context.Context,
branchesMap := getBranchMapFrom(branches)

// Get release branches.
releases, _, err := r.ListReleases(ctx, ownerStr, repoStr, &github.ListOptions{})
releases, err := repoClient.ListReleases()
if err != nil {
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
}
Expand All @@ -98,19 +88,19 @@ func checkReleaseAndDevBranchProtection(ctx context.Context,
commit := regexp.MustCompile("^[a-f0-9]{40}$")
checkBranches := make(map[string]bool)
for _, release := range releases {
if release.TargetCommitish == nil {
if release.TargetCommitish == "" {
// Log with a named error if target_commitish is nil.
e := sce.Create(sce.ErrScorecardInternal, errInternalCommitishNil.Error())
return checker.CreateRuntimeErrorResult(CheckBranchProtection, e)
}

// TODO: if this is a sha, get the associated branch. for now, ignore.
if commit.Match([]byte(*release.TargetCommitish)) {
if commit.Match([]byte(release.TargetCommitish)) {
continue
}

// Try to resolve the branch name.
b, err := branchesMap.getBranchByName(release.GetTargetCommitish())
b, err := branchesMap.getBranchByName(release.TargetCommitish)
if err != nil {
// If the commitish branch is still not found, fail.
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
Expand Down
45 changes: 17 additions & 28 deletions checks/branch_protection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@
package checks

import (
"context"
"testing"

"github.com/golang/mock/gomock"
"github.com/google/go-github/v38/github"

"github.com/ossf/scorecard/v2/checker"
"github.com/ossf/scorecard/v2/clients"
Expand All @@ -28,20 +26,6 @@ import (
scut "github.com/ossf/scorecard/v2/utests"
)

type mockRepos struct {
releases []*string
nonadmin bool
}

func (m mockRepos) ListReleases(ctx context.Context, owner string,
repo string, opts *github.ListOptions) ([]*github.RepositoryRelease, *github.Response, error) {
res := make([]*github.RepositoryRelease, len(m.releases))
for i, rel := range m.releases {
res[i] = &github.RepositoryRelease{TargetCommitish: rel}
}
return res, nil, nil
}

func getBranch(branches []*clients.BranchRef, name string) *clients.BranchRef {
for _, branch := range branches {
if branch.GetName() == name {
Expand Down Expand Up @@ -77,7 +61,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
expected scut.TestReturn
branches []*clients.BranchRef
defaultBranch string
releases []*string
releases []string
nonadmin bool
}{
{
Expand Down Expand Up @@ -191,7 +175,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
},
},
},
releases: []*string{&rel1},
releases: []string{rel1},
},
{
name: "Both release and development are OK",
Expand Down Expand Up @@ -259,7 +243,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
},
},
},
releases: []*string{&rel1},
releases: []string{rel1},
},
{
name: "Ignore a non-branch targetcommitish",
Expand All @@ -271,7 +255,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
defaultBranch: main,
releases: []*string{&sha},
releases: []string{sha},
branches: []*clients.BranchRef{
{
Name: main,
Expand Down Expand Up @@ -315,7 +299,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
defaultBranch: main,
releases: []*string{nil},
releases: []string{""},
branches: []*clients.BranchRef{
{
Name: main,
Expand Down Expand Up @@ -358,7 +342,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
nonadmin: true,
defaultBranch: main,
// branches: []*string{&rel1, &main},
releases: []*string{&rel1},
releases: []string{rel1},
branches: []*clients.BranchRef{
{
Name: main,
Expand Down Expand Up @@ -388,10 +372,6 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
tt := tt // Re-initializing variable so it is not changed while executing the closure below
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
m := mockRepos{
releases: tt.releases,
nonadmin: tt.nonadmin,
}

ctrl := gomock.NewController(t)
mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
Expand All @@ -403,6 +383,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
}
return defaultBranch, nil
}).AnyTimes()
mockRepoClient.EXPECT().ListReleases().
DoAndReturn(func() ([]clients.Release, error) {
var ret []clients.Release
for _, rel := range tt.releases {
ret = append(ret, clients.Release{
TargetCommitish: rel,
})
}
return ret, nil
}).AnyTimes()
mockRepoClient.EXPECT().ListBranches().
DoAndReturn(func() ([]*clients.BranchRef, error) {
if tt.nonadmin {
Expand All @@ -411,8 +401,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
return tt.branches, nil
}).AnyTimes()
dl := scut.TestDetailLogger{}
r := checkReleaseAndDevBranchProtection(context.Background(), mockRepoClient, m,
&dl, "testowner", "testrepo")
r := checkReleaseAndDevBranchProtection(mockRepoClient, &dl)
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) {
t.Fail()
}
Expand Down
2 changes: 1 addition & 1 deletion checks/signed_releases.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func SignedReleases(c *checker.CheckRequest) checker.CheckResult {
c.Dlogger.Warn3(&checker.LogMessage{
Path: "my URL",
Type: checker.FileTypeURL,
Text: fmt.Sprintf("release artifact %s not signed", r.TagName),
Text: fmt.Sprintf("release artifact %s not signed", r.URL),
})
}
if totalReleases >= releaseLookBack {
Expand Down
15 changes: 11 additions & 4 deletions clients/githubrepo/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type Client struct {
graphClient *graphqlHandler
contributors *contributorsHandler
branches *branchesHandler
releases *releasesHandler
search *searchHandler
ctx context.Context
tarball tarballHandler
Expand All @@ -60,13 +61,16 @@ func (client *Client) InitRepo(owner, repoName string) error {
// Setup GraphQL.
client.graphClient.init(client.ctx, client.owner, client.repoName)

// Setup contributors.
// Setup contributorsHandler.
client.contributors.init(client.ctx, client.owner, client.repoName)

// Setup branches.
// Setup branchesHandler.
client.branches.init(client.ctx, client.owner, client.repoName)

// Setup Search.
// Setup releasesHandler.
client.releases.init(client.ctx, client.owner, client.repoName)

// Setup searchHandler.
client.search.init(client.ctx, client.owner, client.repoName)

return nil
Expand Down Expand Up @@ -99,7 +103,7 @@ func (client *Client) ListCommits() ([]clients.Commit, error) {

// ListReleases implements RepoClient.ListReleases.
func (client *Client) ListReleases() ([]clients.Release, error) {
return client.graphClient.getReleases()
return client.releases.getReleases()
}

// ListContributors implements RepoClient.ListContributors.
Expand Down Expand Up @@ -148,6 +152,9 @@ func CreateGithubRepoClient(ctx context.Context,
ghClient: client,
graphClient: graphClient,
},
releases: &releasesHandler{
client: client,
},
search: &searchHandler{
ghClient: client,
},
Expand Down
62 changes: 10 additions & 52 deletions clients/githubrepo/graphql.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,10 @@ import (
)

const (
pullRequestsToAnalyze = 30
reviewsToAnalyze = 30
labelsToAnalyze = 30
commitsToAnalyze = 30
releasesToAnalyze = 30
releaseAssetsToAnalyze = 30
pullRequestsToAnalyze = 30
reviewsToAnalyze = 30
labelsToAnalyze = 30
commitsToAnalyze = 30
)

// nolint: govet
Expand Down Expand Up @@ -76,17 +74,6 @@ type graphqlData struct {
} `graphql:"latestReviews(last: $reviewsToAnalyze)"`
}
} `graphql:"pullRequests(last: $pullRequestsToAnalyze, states: MERGED)"`
Releases struct {
Nodes []struct {
TagName githubv4.String
ReleaseAssets struct {
Nodes []struct {
Name githubv4.String
URL githubv4.String
}
} `graphql:"releaseAssets(last: $releaseAssetsToAnalyze)"`
}
} `graphql:"releases(first: $releasesToAnalyze, orderBy:{field: CREATED_AT, direction:DESC})"`
} `graphql:"repository(owner: $owner, name: $name)"`
}

Expand All @@ -100,7 +87,6 @@ type graphqlHandler struct {
repo string
prs []clients.PullRequest
commits []clients.Commit
releases []clients.Release
archived bool
}

Expand All @@ -116,21 +102,18 @@ func (handler *graphqlHandler) init(ctx context.Context, owner, repo string) {
func (handler *graphqlHandler) setup() error {
handler.once.Do(func() {
vars := map[string]interface{}{
"owner": githubv4.String(handler.owner),
"name": githubv4.String(handler.repo),
"pullRequestsToAnalyze": githubv4.Int(pullRequestsToAnalyze),
"reviewsToAnalyze": githubv4.Int(reviewsToAnalyze),
"labelsToAnalyze": githubv4.Int(labelsToAnalyze),
"commitsToAnalyze": githubv4.Int(commitsToAnalyze),
"releasesToAnalyze": githubv4.Int(releasesToAnalyze),
"releaseAssetsToAnalyze": githubv4.Int(releaseAssetsToAnalyze),
"owner": githubv4.String(handler.owner),
"name": githubv4.String(handler.repo),
"pullRequestsToAnalyze": githubv4.Int(pullRequestsToAnalyze),
"reviewsToAnalyze": githubv4.Int(reviewsToAnalyze),
"labelsToAnalyze": githubv4.Int(labelsToAnalyze),
"commitsToAnalyze": githubv4.Int(commitsToAnalyze),
}
if err := handler.client.Query(handler.ctx, handler.data, vars); err != nil {
handler.errSetup = sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err))
}
handler.archived = bool(handler.data.Repository.IsArchived)
handler.prs = pullRequestsFrom(handler.data)
handler.releases = releasesFrom(handler.data)
handler.commits = commitsFrom(handler.data)
})
return handler.errSetup
Expand All @@ -150,13 +133,6 @@ func (handler *graphqlHandler) getCommits() ([]clients.Commit, error) {
return handler.commits, nil
}

func (handler *graphqlHandler) getReleases() ([]clients.Release, error) {
if err := handler.setup(); err != nil {
return nil, fmt.Errorf("error during graphqlHandler.setup: %w", err)
}
return handler.releases, nil
}

func (handler *graphqlHandler) isArchived() (bool, error) {
if err := handler.setup(); err != nil {
return false, fmt.Errorf("error during graphqlHandler.setup: %w", err)
Expand Down Expand Up @@ -190,24 +166,6 @@ func pullRequestsFrom(data *graphqlData) []clients.PullRequest {
return ret
}

func releasesFrom(data *graphqlData) []clients.Release {
// nolint: prealloc // https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices
var releases []clients.Release
for _, r := range data.Repository.Releases.Nodes {
release := clients.Release{
TagName: string(r.TagName),
}
for _, a := range r.ReleaseAssets.Nodes {
release.Assets = append(release.Assets, clients.ReleaseAsset{
Name: string(a.Name),
URL: string(a.URL),
})
}
releases = append(releases, release)
}
return releases
}

func commitsFrom(data *graphqlData) []clients.Commit {
ret := make([]clients.Commit, 0)
for _, commit := range data.Repository.DefaultBranchRef.Target.Commit.History.Nodes {
Expand Down

0 comments on commit e305a94

Please sign in to comment.