Abstract OrgSecurityPolicy details to RepoClient instead of checker

Signed-off-by: Raghav Kaul <raghavkaul@google.com>

checks/raw/security_policy.go

@@ -24,11 +24,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,18 +59,15 @@ 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.GetOrgPolicyRepoClient(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), errors.Is(err, clients.ErrUnsupportedFeature):
@@ -91,7 +85,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)

checks/raw/security_policy_test.go

@@ -138,7 +138,7 @@ 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()
+			mockRepo.EXPECT().Org().Return("").AnyTimes()
 			//
 			// the revised Security Policy will immediate go for the
 			// file contents once found. This test will return that

clients/githubrepo/client.go

@@ -195,6 +195,21 @@ func (client *Client) GetCreatedAt() (time.Time, error) {
 	return client.repo.CreatedAt.Time, nil
 }
 
+func (client *Client) GetOrgPolicyRepoClient(ctx context.Context) (clients.RepoClient, error) {
+	dotGithubRepo, err := MakeGithubRepo(fmt.Sprintf("%s/.github", client.repourl.Org()))
+	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()

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
@@ -86,12 +82,8 @@ func (r *repoURL) String() string {
 }
 
 // Org implements Repo.Org.
-func (r *repoURL) Org() clients.Repo {
-	return &repoURL{
-		host:  r.host,
-		owner: r.owner,
-		repo:  githubOrgRepo,
-	}
+func (r *repoURL) Org() string {
+	return r.owner
 }
 
 // IsValid implements Repo.IsValid.

clients/gitlabrepo/client.go

@@ -29,8 +29,9 @@ import (
 )
 
 var (
-	_                clients.RepoClient = &Client{}
-	errInputRepoType                    = errors.New("input repo should be of type repoURL")
+	_                      clients.RepoClient = &Client{}
+	errInputRepoType                          = errors.New("input repo should be of type repoURL")
+	errUnsupportedInGitlab                    = errors.New("feature not supported in GitLab")
 )
 
 type Client struct {

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,12 +91,8 @@ 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,
-	}
+func (r *repoURL) Org() string {
+	return r.owner
 }
 
 // IsValid implements Repo.IsValid.

clients/localdir/client.go

@@ -259,6 +259,10 @@ func (client *localDirClient) GetCreatedAt() (time.Time, error) {
 	return time.Time{}, fmt.Errorf("GetCreatedAt: %w", clients.ErrUnsupportedFeature)
 }
 
+func (client *localDirClient) GetOrgPolicyRepoClient(ctx context.Context) (clients.RepoClient, error) {
+	return nil, fmt.Errorf("GetOrgPolicyRepoClient: %w", clients.ErrUnsupportedFeature)
+}
+
 // CreateLocalDirClient returns a client which implements RepoClient interface.
 func CreateLocalDirClient(ctx context.Context, logger *log.Logger) clients.RepoClient {
 	return &localDirClient{

clients/localdir/repo.go

@@ -47,8 +47,8 @@ func (r *repoLocal) String() string {
 }
 
 // Org implements Repo.Org.
-func (r *repoLocal) Org() clients.Repo {
-	return nil
+func (r *repoLocal) Org() string {
+	return ""
 }
 
 // IsValid implements Repo.IsValid.

clients/ossfuzz/client.go

@@ -15,6 +15,7 @@
 package ossfuzz
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -188,6 +189,11 @@ func (c *client) GetDefaultBranch() (*clients.BranchRef, error) {
 	return nil, fmt.Errorf("GetDefaultBranch: %w", clients.ErrUnsupportedFeature)
 }
 
+// GetOrgPolicyRepoClient implements RepoClient.GetOrgPolicyRepoClient.
+func (c *client) GetOrgPolicyRepoClient(ctx context.Context) (clients.RepoClient, error) {
+	return nil, fmt.Errorf("GetOrgPolicyRepoClient: %w", clients.ErrUnsupportedFeature)
+}
+
 // GetDefaultBranchName implements RepoClient.GetDefaultBranchName.
 func (c *client) GetDefaultBranchName() (string, error) {
 	return "", fmt.Errorf("GetDefaultBranchName: %w", clients.ErrUnsupportedFeature)

clients/repo.go

@@ -19,7 +19,7 @@ type Repo interface {
 	URI() string
 	Host() string
 	String() string
-	Org() Repo
+	Org() string
 	IsValid() error
 	Metadata() []string
 	AppendMetadata(metadata ...string)

clients/repo_client.go

@@ -16,6 +16,7 @@
 package clients
 
 import (
+	"context"
 	"errors"
 	"time"
 )
@@ -40,6 +41,7 @@ type RepoClient interface {
 	GetCreatedAt() (time.Time, error)
 	GetDefaultBranchName() (string, error)
 	GetDefaultBranch() (*BranchRef, error)
+	GetOrgPolicyRepoClient(context.Context) (RepoClient, error)
 	ListCommits() ([]Commit, error)
 	ListIssues() ([]Issue, error)
 	ListLicenses() ([]License, error)

e2e/e2e_suite_test.go

@@ -40,6 +40,7 @@ type tokenType int
 const (
 	patTokenType tokenType = iota
 	githubWorkflowDefaultTokenType
+	gitlabPATTokenType
 )
 
 var tokType tokenType
@@ -58,6 +59,8 @@ var _ = BeforeSuite(func() {
 		tokType = patTokenType
 	case "GITHUB_TOKEN":
 		tokType = githubWorkflowDefaultTokenType
+	case "GITLAB_PAT":
+		tokType = gitlabPATTokenType
 	default:
 		panic(fmt.Sprintf("invald TOKEN_TYPE: %s", tt))
 	}

e2e/security_policy_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/ossf/scorecard/v4/checks"
 	"github.com/ossf/scorecard/v4/clients"
 	"github.com/ossf/scorecard/v4/clients/githubrepo"
+	"github.com/ossf/scorecard/v4/clients/gitlabrepo"
 	"github.com/ossf/scorecard/v4/clients/localdir"
 	scut "github.com/ossf/scorecard/v4/utests"
 )
@@ -172,5 +173,68 @@ var _ = Describe("E2E TEST:"+checks.CheckSecurityPolicy, func() {
 			Expect(scut.ValidateTestReturn(nil, "policy found", &expected, &result, &dl)).Should(BeTrue())
 			Expect(x.Close()).Should(BeNil())
 		})
+		It("Should return valid security policy - GitLab", func() {
+			skipIfTokenIsNot(gitlabPATTokenType, "GitLab only")
+
+			dl := scut.TestDetailLogger{}
+			// project url is gitlab.com/bramw/baserow.
+			repo, err := gitlabrepo.MakeGitlabRepo("gitlab.com/ossf-test/baserow")
+			Expect(err).Should(BeNil())
+			repoClient, err := gitlabrepo.CreateGitlabClientWithToken(context.Background(), os.Getenv("GITLAB_AUTH_TOKEN"), repo)
+			Expect(err).Should(BeNil())
+			err = repoClient.InitRepo(repo, clients.HeadSHA, 0)
+			Expect(err).Should(BeNil())
+
+			req := checker.CheckRequest{
+				Ctx:        context.Background(),
+				RepoClient: repoClient,
+				Repo:       repo,
+				Dlogger:    &dl,
+			}
+			// TODO: update expected based on what is returned from gitlab project.
+			expected := scut.TestReturn{
+				Error:         nil,
+				Score:         9,
+				NumberOfWarn:  1,
+				NumberOfInfo:  3,
+				NumberOfDebug: 0,
+			}
+			result := checks.SecurityPolicy(&req)
+			// New version.
+			Expect(scut.ValidateTestReturn(nil, "policy found", &expected, &result, &dl)).Should(BeTrue())
+			Expect(repoClient.Close()).Should(BeNil())
+		})
+		It("Should return valid security policy at commitSHA - GitLab", func() {
+			skipIfTokenIsNot(gitlabPATTokenType, "GitLab only")
+
+			dl := scut.TestDetailLogger{}
+			// project url is gitlab.com/bramw/baserow.
+			repo, err := gitlabrepo.MakeGitlabRepo("gitlab.com/ossf-test/baserow")
+			Expect(err).Should(BeNil())
+			repoClient, err := gitlabrepo.CreateGitlabClientWithToken(context.Background(), os.Getenv("GITLAB_AUTH_TOKEN"), repo)
+			Expect(err).Should(BeNil())
+			// url to commit is https://gitlab.com/bramw/baserow/-/commit/28e6224b7d86f7b30bad6adb6b42f26a814c2f58
+			err = repoClient.InitRepo(repo, "28e6224b7d86f7b30bad6adb6b42f26a814c2f58", 0)
+			Expect(err).Should(BeNil())
+
+			req := checker.CheckRequest{
+				Ctx:        context.Background(),
+				RepoClient: repoClient,
+				Repo:       repo,
+				Dlogger:    &dl,
+			}
+
+			expected := scut.TestReturn{
+				Error:         nil,
+				Score:         9,
+				NumberOfWarn:  1,
+				NumberOfInfo:  3,
+				NumberOfDebug: 0,
+			}
+			result := checks.SecurityPolicy(&req)
+			// New version.
+			Expect(scut.ValidateTestReturn(nil, "policy found", &expected, &result, &dl)).Should(BeTrue())
+			Expect(repoClient.Close()).Should(BeNil())
+		})
 	})
 })