fix: Minor bug in determining teams for gitlab (#5294)

Signed-off-by: Luke Massa <[email protected]>
runatlantis · Feb 3, 2025 · c2c25ae · c2c25ae
1 parent aa4a980
commit c2c25ae
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 34 deletions.
diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go
@@ -637,10 +637,10 @@ func (g *GitlabClient) GetTeamNamesForUser(logger logging.SimpleLogging, _ model
 	if err != nil {
 		return nil, errors.Wrapf(err, "GET /users returned: %d", resp.StatusCode)
 	} else if len(users) == 0 {
-		return nil, errors.Wrap(err, "GET /users returned no user")
+		return nil, errors.New("GET /users returned no user")
 	} else if len(users) > 1 {
 		// Theoretically impossible, just being extra safe
-		return nil, errors.Wrap(err, "GET /users returned more than 1 user")
+		return nil, errors.New("GET /users returned more than 1 user")
 	}
 	userID := users[0].ID
 	for _, groupName := range g.ConfiguredGroups {

diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go
@@ -1121,40 +1121,82 @@ func TestGitlabClient_GetTeamNamesForUser(t *testing.T) {
 	userSuccess, err := os.ReadFile("testdata/gitlab-user-success.json")
 	Ok(t, err)
 
-	configuredGroups := []string{"someorg/group1", "someorg/group2", "someorg/group3", "someorg/group4"}
-	testServer := httptest.NewServer(
-		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			switch r.RequestURI {
-			case "/api/v4/users?username=testuser":
-				w.WriteHeader(http.StatusOK)
-				w.Write(userSuccess) // nolint: errcheck
-			case "/api/v4/groups/someorg%2Fgroup1/members/123", "/api/v4/groups/someorg%2Fgroup2/members/123":
-				w.WriteHeader(http.StatusOK)
-				w.Write(groupMembershipSuccess) // nolint: errcheck
-			case "/api/v4/groups/someorg%2Fgroup3/members/123":
-				http.Error(w, "forbidden", http.StatusForbidden)
-			case "/api/v4/groups/someorg%2Fgroup4/members/123":
-				http.Error(w, "not found", http.StatusNotFound)
-			default:
-				t.Errorf("got unexpected request at %q", r.RequestURI)
-				http.Error(w, "not found", http.StatusNotFound)
-			}
-		}))
-	internalClient, err := gitlab.NewClient("token", gitlab.WithBaseURL(testServer.URL))
+	userEmpty, err := os.ReadFile("testdata/gitlab-user-none.json")
 	Ok(t, err)
-	client := &GitlabClient{
-		Client:           internalClient,
-		Version:          nil,
-		ConfiguredGroups: configuredGroups,
+
+	multipleUsers, err := os.ReadFile("testdata/gitlab-user-multiple.json")
+	Ok(t, err)
+
+	configuredGroups := []string{"someorg/group1", "someorg/group2", "someorg/group3", "someorg/group4"}
+
+	cases := []struct {
+		userName string
+		expErr   string
+		expTeams []string
+	}{
+		{
+			userName: "testuser",
+			expTeams: []string{"someorg/group1", "someorg/group2"},
+		},
+		{
+			userName: "none",
+			expErr:   "GET /users returned no user",
+		},
+		{
+			userName: "multiuser",
+			expErr:   "GET /users returned more than 1 user",
+		},
 	}
+	for _, c := range cases {
+		t.Run(c.userName, func(t *testing.T) {
+
+			testServer := httptest.NewServer(
+				http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					switch r.RequestURI {
+					case "/api/v4/users?username=testuser":
+						w.WriteHeader(http.StatusOK)
+						w.Write(userSuccess) // nolint: errcheck
+					case "/api/v4/users?username=none":
+						w.WriteHeader(http.StatusOK)
+						w.Write(userEmpty) // nolint: errcheck
+					case "/api/v4/users?username=multiuser":
+						w.WriteHeader(http.StatusOK)
+						w.Write(multipleUsers) // nolint: errcheck
+					case "/api/v4/groups/someorg%2Fgroup1/members/123", "/api/v4/groups/someorg%2Fgroup2/members/123":
+						w.WriteHeader(http.StatusOK)
+						w.Write(groupMembershipSuccess) // nolint: errcheck
+					case "/api/v4/groups/someorg%2Fgroup3/members/123":
+						http.Error(w, "forbidden", http.StatusForbidden)
+					case "/api/v4/groups/someorg%2Fgroup4/members/123":
+						http.Error(w, "not found", http.StatusNotFound)
+					default:
+						t.Errorf("got unexpected request at %q", r.RequestURI)
+						http.Error(w, "not found", http.StatusNotFound)
+					}
+				}))
+			internalClient, err := gitlab.NewClient("token", gitlab.WithBaseURL(testServer.URL))
+			Ok(t, err)
+			client := &GitlabClient{
+				Client:           internalClient,
+				Version:          nil,
+				ConfiguredGroups: configuredGroups,
+			}
+
+			teams, err := client.GetTeamNamesForUser(
+				logger,
+				models.Repo{
+					Owner: "someorg",
+				}, models.User{
+					Username: c.userName,
+				})
+			if c.expErr == "" {
+				Ok(t, err)
+				Equals(t, c.expTeams, teams)
+			} else {
+				ErrContains(t, c.expErr, err)
+
+			}
 
-	teams, err := client.GetTeamNamesForUser(
-		logger,
-		models.Repo{
-			Owner: "someorg",
-		}, models.User{
-			Username: "testuser",
 		})
-	Ok(t, err)
-	Equals(t, []string{"someorg/group1", "someorg/group2"}, teams)
+	}
 }
diff --git a/server/events/vcs/testdata/gitlab-user-multiple.json b/server/events/vcs/testdata/gitlab-user-multiple.json
@@ -0,0 +1,20 @@
+[
+  {
+    "id": 123,
+    "username": "multiuser",
+    "name": "Multiple User 1",
+    "state": "active",
+    "locked": false,
+    "avatar_url": "https://gitlab.com/uploads/-/system/user/avatar/123/avatar.png",
+    "web_url": "https://gitlab.com/multiuser"
+  },
+  {
+    "id": 124,
+    "username": "multiuser",
+    "name": "Multiple User 2",
+    "state": "active",
+    "locked": false,
+    "avatar_url": "https://gitlab.com/uploads/-/system/user/avatar/124/avatar.png",
+    "web_url": "https://gitlab.com/multiuser"
+  }
+]
diff --git a/server/events/vcs/testdata/gitlab-user-none.json b/server/events/vcs/testdata/gitlab-user-none.json
@@ -0,0 +1 @@
+[]