🐛 Gitlab: Commit/Commitor Exceptions (ossf#3026)

* feat: Added paging for contributor/users against gitlab projects Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * refactor: Updated the bot flag for unmatched users Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * fix: Not all commit users are in the git registry instance Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * fix: Skipping check if the email is empty, as well as if the "email" doesn't contain a "." char. Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * fix: Updated to allow for commits with PRs to be accounted/added to the client.commits Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * refactor: Updated to prevent linting issue regarding nested if's Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * test: Adding coverage for commits and contributors for gitlab Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * refactor: Moved queries from the client to their own functions Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * bug: Need to pass the ProjectID value to the contributor query Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * bug: Updating project title versus projectID values for api querying Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * test: Updated tests to match expected property set for projectID Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> * revert: Reverted based on feedback during review Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> --------- Signed-off-by: Robison, Jim B <jim.b.robison@lmco.com> Signed-off-by: Avishay <avishay.balter@gmail.com>
balteravishay · May 28, 2023 · 0c638d6 · 0c638d6
1 parent e8d0cc2
commit 0c638d6
Show file tree

Hide file tree

Showing 19 changed files with 307 additions and 66 deletions.
diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go
@@ -46,21 +46,21 @@ func (handler *branchesHandler) setup() error {
 			return
 		}
 
-		proj, _, err := handler.glClient.Projects.GetProject(handler.repourl.project, &gitlab.GetProjectOptions{})
+		proj, _, err := handler.glClient.Projects.GetProject(handler.repourl.projectID, &gitlab.GetProjectOptions{})
 		if err != nil {
 			handler.errSetup = fmt.Errorf("requirest for project failed with error %w", err)
 			return
 		}
 
-		branch, _, err := handler.glClient.Branches.GetBranch(handler.repourl.project, proj.DefaultBranch)
+		branch, _, err := handler.glClient.Branches.GetBranch(handler.repourl.projectID, proj.DefaultBranch)
 		if err != nil {
 			handler.errSetup = fmt.Errorf("request for default branch failed with error %w", err)
 			return
 		}
 
 		if branch.Protected {
 			protectedBranch, resp, err := handler.glClient.ProtectedBranches.GetProtectedBranch(
-				handler.repourl.project, branch.Name)
+				handler.repourl.projectID, branch.Name)
 			if err != nil && resp.StatusCode != 403 {
 				handler.errSetup = fmt.Errorf("request for protected branch failed with error %w", err)
 				return
@@ -70,14 +70,14 @@ func (handler *branchesHandler) setup() error {
 			}
 
 			projectStatusChecks, resp, err := handler.glClient.ExternalStatusChecks.ListProjectStatusChecks(
-				handler.repourl.project, &gitlab.ListOptions{})
+				handler.repourl.projectID, &gitlab.ListOptions{})
 			if (err != nil || resp.StatusCode != 404) &&
 				resp.StatusCode != 401 { // 401: permissions. pass token authorization issues silently
 				handler.errSetup = fmt.Errorf("request for external status checks failed with error %w", err)
 				return
 			}
 
-			projectApprovalRule, resp, err := handler.glClient.Projects.GetApprovalConfiguration(handler.repourl.project)
+			projectApprovalRule, resp, err := handler.glClient.Projects.GetApprovalConfiguration(handler.repourl.projectID)
 			if err != nil && resp.StatusCode != 404 {
 				handler.errSetup = fmt.Errorf("request for project approval rule failed with %w", err)
 				return
@@ -106,24 +106,24 @@ func (handler *branchesHandler) getDefaultBranch() (*clients.BranchRef, error) {
 }
 
 func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, error) {
-	bran, _, err := handler.glClient.Branches.GetBranch(handler.repourl.project, branch)
+	bran, _, err := handler.glClient.Branches.GetBranch(handler.repourl.projectID, branch)
 	if err != nil {
 		return nil, fmt.Errorf("error getting branch in branchsHandler.getBranch: %w", err)
 	}
 
 	if bran.Protected {
-		protectedBranch, _, err := handler.glClient.ProtectedBranches.GetProtectedBranch(handler.repourl.project, bran.Name)
+		protectedBranch, _, err := handler.glClient.ProtectedBranches.GetProtectedBranch(handler.repourl.projectID, bran.Name)
 		if err != nil {
 			return nil, fmt.Errorf("request for protected branch failed with error %w", err)
 		}
 
 		projectStatusChecks, resp, err := handler.glClient.ExternalStatusChecks.ListProjectStatusChecks(
-			handler.repourl.project, &gitlab.ListOptions{})
+			handler.repourl.projectID, &gitlab.ListOptions{})
 		if err != nil && resp.StatusCode != 404 {
 			return nil, fmt.Errorf("request for external status checks failed with error %w", err)
 		}
 
-		projectApprovalRule, resp, err := handler.glClient.Projects.GetApprovalConfiguration(handler.repourl.project)
+		projectApprovalRule, resp, err := handler.glClient.Projects.GetApprovalConfiguration(handler.repourl.projectID)
 		if err != nil && resp.StatusCode != 404 {
 			return nil, fmt.Errorf("request for project approval rule failed with %w", err)
 		}

diff --git a/clients/gitlabrepo/checkruns.go b/clients/gitlabrepo/checkruns.go
@@ -34,7 +34,7 @@ func (handler *checkrunsHandler) init(repourl *repoURL) {
 
 func (handler *checkrunsHandler) listCheckRunsForRef(ref string) ([]clients.CheckRun, error) {
 	pipelines, _, err := handler.glClient.Pipelines.ListProjectPipelines(
-		handler.repourl.project, &gitlab.ListProjectPipelinesOptions{})
+		handler.repourl.projectID, &gitlab.ListProjectPipelinesOptions{})
 	if err != nil {
 		return nil, fmt.Errorf("request for pipelines returned error: %w", err)
 	}

diff --git a/clients/gitlabrepo/client.go b/clients/gitlabrepo/client.go
@@ -142,7 +142,7 @@ func (client *Client) InitRepo(inputRepo clients.Repo, commitSHA string, commitD
 }
 
 func (client *Client) URI() string {
-	return fmt.Sprintf("%s/%s/%s", client.repourl.host, client.repourl.owner, client.repourl.project)
+	return fmt.Sprintf("%s/%s/%s", client.repourl.host, client.repourl.owner, client.repourl.projectID)
 }
 
 func (client *Client) LocalPath() (string, error) {

diff --git a/clients/gitlabrepo/commits.go b/clients/gitlabrepo/commits.go
@@ -155,8 +155,11 @@ func (handler *commitsHandler) zip(commitsRaw []*gitlab.Commit, data graphqlData
 
 // Expected email form: <firstname>.<lastname>@<namespace>.com.
 func parseEmailToName(email string) string {
-	s := strings.Split(email, ".")
-	firstName := s[0]
-	lastName := strings.Split(s[1], "@")[0]
-	return firstName + " " + lastName
+	if strings.Contains(email, ".") {
+		s := strings.Split(email, ".")
+		firstName := s[0]
+		lastName := strings.Split(s[1], "@")[0]
+		return firstName + " " + lastName
+	}
+	return email
 }
diff --git a/clients/gitlabrepo/commits_test.go b/clients/gitlabrepo/commits_test.go
@@ -62,3 +62,42 @@ func Test_Setup(t *testing.T) {
 		})
 	}
 }
+
+func TestParsingEmail(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		email    string
+		expected string
+	}{
+		{
+			name:     "Perfect Match Email Parser",
+			email:    "john.doe@nowhere.com",
+			expected: "john doe",
+		},
+		{
+			name:     "Valid Email Not Formatted as expected",
+			email:    "johndoe@nowhere.com",
+			expected: "johndoe@nowhere com",
+		},
+		{
+			name:     "Invalid email format",
+			email:    "johndoe@nowherecom",
+			expected: "johndoe@nowherecom",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			result := parseEmailToName(tt.email)
+
+			if tt.expected != result {
+				t.Errorf("Parser didn't work as expected: %s != %s", tt.expected, result)
+			}
+		})
+	}
+}
diff --git a/clients/gitlabrepo/contributors.go b/clients/gitlabrepo/contributors.go
@@ -25,17 +25,63 @@ import (
 )
 
 type contributorsHandler struct {
-	glClient     *gitlab.Client
-	once         *sync.Once
-	errSetup     error
-	repourl      *repoURL
-	contributors []clients.User
+	fnContributors retrieveContributorFn
+	fnUsers        retrieveUserFn
+	glClient       *gitlab.Client
+	once           *sync.Once
+	errSetup       error
+	repourl        *repoURL
+	contributors   []clients.User
 }
 
 func (handler *contributorsHandler) init(repourl *repoURL) {
 	handler.repourl = repourl
 	handler.errSetup = nil
 	handler.once = new(sync.Once)
+	handler.fnContributors = handler.retrieveContributors
+	handler.fnUsers = handler.retrieveUsers
+}
+
+type (
+	retrieveContributorFn func(string) ([]*gitlab.Contributor, error)
+	retrieveUserFn        func(string) ([]*gitlab.User, error)
+)
+
+func (handler *contributorsHandler) retrieveContributors(project string) ([]*gitlab.Contributor, error) {
+	var contribs []*gitlab.Contributor
+	i := 1
+
+	for {
+		c, _, err := handler.glClient.Repositories.Contributors(
+			project,
+			&gitlab.ListContributorsOptions{
+				ListOptions: gitlab.ListOptions{
+					Page:    i,
+					PerPage: 100,
+				},
+			},
+		)
+		if err != nil {
+			//nolint wrapcheck
+			return nil, err
+		}
+
+		if len(c) == 0 {
+			break
+		}
+		i++
+		contribs = append(contribs, c...)
+	}
+	return contribs, nil
+}
+
+func (handler *contributorsHandler) retrieveUsers(queryName string) ([]*gitlab.User, error) {
+	users, _, err := handler.glClient.Search.Users(queryName, &gitlab.SearchOptions{})
+	if err != nil {
+		//nolint wrapcheck
+		return nil, err
+	}
+	return users, nil
 }
 
 func (handler *contributorsHandler) setup() error {
@@ -45,8 +91,8 @@ func (handler *contributorsHandler) setup() error {
 				clients.ErrUnsupportedFeature)
 			return
 		}
-		contribs, _, err := handler.glClient.Repositories.Contributors(
-			handler.repourl.project, &gitlab.ListContributorsOptions{})
+
+		contribs, err := handler.fnContributors(handler.repourl.projectID)
 		if err != nil {
 			handler.errSetup = fmt.Errorf("error during ListContributors: %w", err)
 			return
@@ -57,27 +103,35 @@ func (handler *contributorsHandler) setup() error {
 				continue
 			}
 
-			// In Gitlab users only have one registered organization which is the company they work for, this means that
-			// the organizations field will not be filled in and the companies field will be a singular value.
-			users, _, err := handler.glClient.Search.Users(contrib.Name, &gitlab.SearchOptions{})
+			users, err := handler.fnUsers(contrib.Name)
 			if err != nil {
 				handler.errSetup = fmt.Errorf("error during Users.Get: %w", err)
 				return
-			} else if len(users) == 0 {
+			} else if len(users) == 0 && contrib.Email != "" {
 				// parseEmailToName is declared in commits.go
-				users, _, err = handler.glClient.Search.Users(parseEmailToName(contrib.Email), &gitlab.SearchOptions{})
+				users, err = handler.fnUsers(parseEmailToName(contrib.Email))
 				if err != nil {
 					handler.errSetup = fmt.Errorf("error during Users.Get: %w", err)
 					return
 				}
 			}
 
+			user := &gitlab.User{}
+
+			if len(users) == 0 {
+				user.ID = 0
+				user.Organization = ""
+				user.Bot = false
+			} else {
+				user = users[0]
+			}
+
 			contributor := clients.User{
 				Login:            contrib.Email,
-				Companies:        []string{users[0].Organization},
+				Companies:        []string{user.Organization},
 				NumContributions: contrib.Commits,
-				ID:               int64(users[0].ID),
-				IsBot:            users[0].Bot,
+				ID:               int64(user.ID),
+				IsBot:            user.Bot,
 			}
 			handler.contributors = append(handler.contributors, contributor)
 		}