✨ GitLab: Security Policy check (ossf#2754)

* Add tarballHandler for GitLab, enabling repo download Signed-off-by: Raghav Kaul <[email protected]> * Abstract OrgSecurityPolicy details to RepoClient instead of checker Signed-off-by: Raghav Kaul <[email protected]> * Remove Org() from RepoClient Signed-off-by: Raghav Kaul <[email protected]> * Rename Signed-off-by: Raghav Kaul <[email protected]> * Don't run as part of CI tests that depend on external sites Signed-off-by: Raghav Kaul <[email protected]> --------- Signed-off-by: Raghav Kaul <[email protected]> Signed-off-by: Avishay <[email protected]>
balteravishay · Apr 14, 2023 · 6b5be75 · 6b5be75
1 parent 7e37e4b
commit 6b5be75
Show file tree

Hide file tree

Showing 20 changed files with 586 additions and 75 deletions.
diff --git a/Makefile b/Makefile
@@ -335,7 +335,7 @@ e2e-gh-token: build-scorecard check-env | $(GINKGO)
 	TOKEN_TYPE="GITHUB_TOKEN" $(GINKGO) --race -p -v -cover -coverprofile=e2e-coverage.out --keep-separate-coverprofiles ./...
 
 e2e-gitlab-token: ## Runs e2e tests that require a GITLAB_TOKEN
-	TOKEN_TYPE="GITLAB_PAT" $(GINKGO) --race -p -vv --focus '.*GitLab Token' ./...
+	TEST_GITLAB_EXTERNAL=1 TOKEN_TYPE="GITLAB_PAT" $(GINKGO) --race -p -vv --focus '.*GitLab Token' ./...
 
 e2e-gitlab: ## Runs e2e tests for GitLab only. TOKEN_TYPE is not used (since these are public APIs), but must be set to something
 	TOKEN_TYPE="GITLAB_PAT" $(GINKGO) --race -p -vv --focus '.*GitLab' ./...

diff --git a/checks/raw/security_policy.go b/checks/raw/security_policy.go
@@ -25,10 +25,8 @@ import (
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/checks/fileparser"
 	"github.com/ossf/scorecard/v4/clients"
-	"github.com/ossf/scorecard/v4/clients/githubrepo"
 	sce "github.com/ossf/scorecard/v4/errors"
 	"github.com/ossf/scorecard/v4/finding"
-	"github.com/ossf/scorecard/v4/log"
 )
 
 type securityPolicyFilesWithURI struct {
@@ -62,21 +60,18 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error)
 
 	// Check if present in parent org.
 	// https#://docs.github.com/en/github/building-a-strong-community/creating-a-default-community-health-file.
-	// TODO(1491): Make this non-GitHub specific.
-	logger := log.NewLogger(log.InfoLevel)
-	// HAD TO HARD CODE TO 30
-	dotGitHubClient := githubrepo.CreateGithubRepoClient(c.Ctx, logger)
-	err = dotGitHubClient.InitRepo(c.Repo.Org(), clients.HeadSHA, 0)
+	client, err := c.RepoClient.GetOrgRepoClient(c.Ctx)
+
 	switch {
 	case err == nil:
-		defer dotGitHubClient.Close()
-		data.uri = dotGitHubClient.URI()
-		err = fileparser.OnAllFilesDo(dotGitHubClient, isSecurityPolicyFile, &data)
+		defer client.Close()
+		data.uri = client.URI()
+		err = fileparser.OnAllFilesDo(client, isSecurityPolicyFile, &data)
 		if err != nil {
-			return checker.SecurityPolicyData{}, err
+			return checker.SecurityPolicyData{}, fmt.Errorf("unable to create github client: %w", err)
 		}
 
-	case errors.Is(err, sce.ErrRepoUnreachable):
+	case errors.Is(err, sce.ErrRepoUnreachable), errors.Is(err, clients.ErrUnsupportedFeature):
 		break
 	default:
 		return checker.SecurityPolicyData{}, err
@@ -91,7 +86,7 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error)
 			if data.files[idx].File.Type == finding.FileTypeURL {
 				filePattern = strings.Replace(filePattern, data.uri+"/", "", 1)
 			}
-			err := fileparser.OnMatchingFileContentDo(dotGitHubClient, fileparser.PathMatcher{
+			err := fileparser.OnMatchingFileContentDo(client, fileparser.PathMatcher{
 				Pattern:       filePattern,
 				CaseSensitive: false,
 			}, checkSecurityPolicyFileContent, &data.files[idx].File, &data.files[idx].Information)

diff --git a/checks/raw/security_policy_test.go b/checks/raw/security_policy_test.go
@@ -138,8 +138,6 @@ func TestSecurityPolicy(t *testing.T) {
 			mockRepo := mockrepo.NewMockRepo(ctrl)
 
 			mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(tt.files, nil).AnyTimes()
-			mockRepo.EXPECT().Org().Return(nil).AnyTimes()
-			//
 			// the revised Security Policy will immediate go for the
 			// file contents once found. This test will return that
 			// mock file, but this specific unit test is not testing

diff --git a/clients/githubrepo/client.go b/clients/githubrepo/client.go
@@ -195,6 +195,21 @@ func (client *Client) GetCreatedAt() (time.Time, error) {
 	return client.repo.CreatedAt.Time, nil
 }
 
+func (client *Client) GetOrgRepoClient(ctx context.Context) (clients.RepoClient, error) {
+	dotGithubRepo, err := MakeGithubRepo(fmt.Sprintf("%s/.github", client.repourl.owner))
+	if err != nil {
+		return nil, fmt.Errorf("error during MakeGithubRepo: %w", err)
+	}
+
+	logger := log.NewLogger(log.InfoLevel)
+	c := CreateGithubRepoClient(ctx, logger)
+	if err := c.InitRepo(dotGithubRepo, clients.HeadSHA, 0); err != nil {
+		return nil, fmt.Errorf("error during InitRepo: %w", err)
+	}
+
+	return c, nil
+}
+
 // ListWebhooks implements RepoClient.ListWebhooks.
 func (client *Client) ListWebhooks() ([]clients.Webhook, error) {
 	return client.webhook.listWebhooks()

diff --git a/clients/githubrepo/repo.go b/clients/githubrepo/repo.go
@@ -23,10 +23,6 @@ import (
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
-const (
-	githubOrgRepo = ".github"
-)
-
 type repoURL struct {
 	host, owner, repo, defaultBranch, commitSHA string
 	metadata                                    []string
@@ -85,15 +81,6 @@ func (r *repoURL) String() string {
 	return fmt.Sprintf("%s-%s-%s", r.host, r.owner, r.repo)
 }
 
-// Org implements Repo.Org.
-func (r *repoURL) Org() clients.Repo {
-	return &repoURL{
-		host:  r.host,
-		owner: r.owner,
-		repo:  githubOrgRepo,
-	}
-}
-
 // IsValid implements Repo.IsValid.
 func (r *repoURL) IsValid() error {
 	switch r.host {

diff --git a/clients/gitlabrepo/client.go b/clients/gitlabrepo/client.go
@@ -51,6 +51,7 @@ type Client struct {
 	webhook       *webhookHandler
 	languages     *languagesHandler
 	licenses      *licensesHandler
+	tarball       *tarballHandler
 	ctx           context.Context
 	commitDepth   int
 }
@@ -128,6 +129,9 @@ func (client *Client) InitRepo(inputRepo clients.Repo, commitSHA string, commitD
 	// Init languagesHandler
 	client.licenses.init(client.repourl)
 
+	// Init tarballHandler
+	client.tarball.init(client.ctx, client.repourl, repo, commitSHA)
+
 	return nil
 }
 
@@ -140,11 +144,11 @@ func (client *Client) LocalPath() (string, error) {
 }
 
 func (client *Client) ListFiles(predicate func(string) (bool, error)) ([]string, error) {
-	return nil, nil
+	return client.tarball.listFiles(predicate)
 }
 
 func (client *Client) GetFileContent(filename string) ([]byte, error) {
-	return nil, nil
+	return client.tarball.getFileContent(filename)
 }
 
 func (client *Client) ListCommits() ([]clients.Commit, error) {
@@ -183,6 +187,10 @@ func (client *Client) GetCreatedAt() (time.Time, error) {
 	return client.project.getCreatedAt()
 }
 
+func (client *Client) GetOrgRepoClient(ctx context.Context) (clients.RepoClient, error) {
+	return nil, fmt.Errorf("GetOrgRepoClient (GitLab): %w", clients.ErrUnsupportedFeature)
+}
+
 func (client *Client) ListWebhooks() ([]clients.Webhook, error) {
 	return client.webhook.listWebhooks()
 }
@@ -269,6 +277,7 @@ func CreateGitlabClientWithToken(ctx context.Context, token string, repo clients
 			glClient: client,
 		},
 		licenses: &licensesHandler{},
+		tarball:  &tarballHandler{},
 	}, nil
 }
 

diff --git a/clients/gitlabrepo/repo.go b/clients/gitlabrepo/repo.go
@@ -27,10 +27,6 @@ import (
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
-const (
-	gitlabOrgProj = ".gitlab"
-)
-
 type repoURL struct {
 	scheme        string
 	host          string
@@ -95,14 +91,6 @@ func (r *repoURL) String() string {
 	return fmt.Sprintf("%s-%s_%s", r.host, r.owner, r.project)
 }
 
-func (r *repoURL) Org() clients.Repo {
-	return &repoURL{
-		host:    r.host,
-		owner:   r.owner,
-		project: gitlabOrgProj,
-	}
-}
-
 // IsValid implements Repo.IsValid.
 func (r *repoURL) IsValid() error {
 	if strings.Contains(r.host, "gitlab.") {

diff --git a/clients/gitlabrepo/repo_test.go b/clients/gitlabrepo/repo_test.go
@@ -16,6 +16,7 @@ package gitlabrepo
 
 import (
 	"fmt"
+	"os"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -25,10 +26,11 @@ import (
 func TestRepoURL_IsValid(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
-		name     string
-		inputURL string
-		expected repoURL
-		wantErr  bool
+		name         string
+		inputURL     string
+		expected     repoURL
+		wantErr      bool
+		flagRequired bool
 	}{
 		{
 			name: "github repository",
@@ -73,7 +75,6 @@ func TestRepoURL_IsValid(t *testing.T) {
 			inputURL: "https://gitlab.com/ossf-test/scorecard-check-binary-artifacts-e2e/",
 			wantErr:  false,
 		},
-
 		{
 			name: "valid hosted gitlab project",
 			expected: repoURL{
@@ -82,12 +83,16 @@ func TestRepoURL_IsValid(t *testing.T) {
 				owner:   "webmaster-team",
 				project: "webml",
 			},
-			inputURL: "https://salsa.debian.org/webmaster-team/webwml",
-			wantErr:  false,
+			inputURL:     "https://salsa.debian.org/webmaster-team/webwml",
+			wantErr:      false,
+			flagRequired: true,
 		},
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure blow
+		if tt.flagRequired && os.Getenv("TEST_GITLAB_EXTERNAL") == "" {
+			continue
+		}
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			r := repoURL{
@@ -116,8 +121,9 @@ func TestRepoURL_IsValid(t *testing.T) {
 
 func TestRepoURL_DetectGitlab(t *testing.T) {
 	tests := []struct {
-		repouri  string
-		expected bool
+		repouri      string
+		expected     bool
+		flagRequired bool
 	}{
 		{
 			repouri:  "github.com/ossf/scorecard",
@@ -136,8 +142,9 @@ func TestRepoURL_DetectGitlab(t *testing.T) {
 			expected: true,
 		},
 		{
-			repouri:  "https://salsa.debian.org/webmaster-team/webml",
-			expected: true,
+			repouri:      "https://salsa.debian.org/webmaster-team/webml",
+			expected:     true,
+			flagRequired: true,
 		},
 		{
 			// Invalid repo
@@ -147,6 +154,9 @@ func TestRepoURL_DetectGitlab(t *testing.T) {
 	}
 
 	for _, tt := range tests {
+		if tt.flagRequired && os.Getenv("TEST_GITLAB_EXTERNAL") == "" {
+			continue
+		}
 		g := DetectGitLab(tt.repouri)
 		if g != tt.expected {
 			t.Errorf("got %s isgitlab: %t expected %t", tt.repouri, g, tt.expected)