internal/directory: use both id and name for group (#1086)

Fixes #1085

internal/directory/azure/azure.go

@@ -112,14 +112,14 @@ func (p *Provider) UserGroups(ctx context.Context) ([]*directory.User, error) {
 	}
 
 	userIDToGroupIDs := map[string][]string{}
-	for _, groupID := range groupIDs {
+	for groupID, groupName := range groupIDs {
 		userIDs, err := p.listGroupMembers(ctx, groupID)
 		if err != nil {
 			return nil, err
 		}
 
 		for _, userID := range userIDs {
-			userIDToGroupIDs[userID] = append(userIDToGroupIDs[userID], groupID)
+			userIDToGroupIDs[userID] = append(userIDToGroupIDs[userID], groupID, groupName)
 		}
 	}
 
@@ -137,15 +137,18 @@ func (p *Provider) UserGroups(ctx context.Context) ([]*directory.User, error) {
 	return users, nil
 }
 
-func (p *Provider) listGroups(ctx context.Context) (groupIDs []string, err error) {
+// listGroups returns a map, with key is group ID, element is group name.
+func (p *Provider) listGroups(ctx context.Context) (map[string]string, error) {
 	nextURL := p.cfg.graphURL.ResolveReference(&url.URL{
 		Path: "/v1.0/groups",
 	}).String()
 
+	groups := make(map[string]string)
 	for nextURL != "" {
 		var result struct {
 			Value []struct {
-				ID string `json:"id"`
+				ID   string `json:"id"`
+				Name string `json:"name"`
 			} `json:"value"`
 			NextLink string `json:"@odata.nextLink"`
 		}
@@ -154,12 +157,12 @@ func (p *Provider) listGroups(ctx context.Context) (groupIDs []string, err error
 			return nil, err
 		}
 		for _, v := range result.Value {
-			groupIDs = append(groupIDs, v.ID)
+			groups[v.ID] = v.Name
 		}
 		nextURL = result.NextLink
 	}
 
-	return groupIDs, nil
+	return groups, nil
 }
 
 func (p *Provider) listGroupMembers(ctx context.Context, groupID string) (userIDs []string, err error) {

internal/directory/azure/azure_test.go

@@ -45,8 +45,8 @@ func newMockAPI(t *testing.T, srv *httptest.Server) http.Handler {
 		r.Get("/groups", func(w http.ResponseWriter, r *http.Request) {
 			_ = json.NewEncoder(w).Encode(M{
 				"value": []M{
-					{"id": "admin"},
+					{"id": "admin", "name": "Admin Group"},
-					{"id": "test"},
+					{"id": "test", "name": "Test Group"},
 				},
 			})
 		})
@@ -90,15 +90,15 @@ func Test(t *testing.T) {
 	assert.Equal(t, []*directory.User{
 		{
 			Id:     "azure/user-1",
-			Groups: []string{"admin"},
+			Groups: []string{"Admin Group", "admin"},
 		},
 		{
 			Id:     "azure/user-2",
-			Groups: []string{"test"},
+			Groups: []string{"Test Group", "test"},
 		},
 		{
 			Id:     "azure/user-3",
-			Groups: []string{"test"},
+			Groups: []string{"Test Group", "test"},
 		},
 	}, users)
 }

internal/directory/github/github.go

@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"net/url"
 	"sort"
+	"strconv"
 
 	"github.com/rs/zerolog"
 	"github.com/tomnomnom/linkheader"
@@ -103,14 +104,14 @@ func (p *Provider) UserGroups(ctx context.Context) ([]*directory.User, error) {
 			return nil, err
 		}
 
-		for _, teamSlug := range teamSlugs {
+		for teamSlug, teamID := range teamSlugs {
 			userLogins, err := p.listTeamMembers(ctx, orgSlug, teamSlug)
 			if err != nil {
 				return nil, err
 			}
 
 			for _, userLogin := range userLogins {
-				userLoginToGroups[userLogin] = append(userLoginToGroups[userLogin], teamSlug)
+				userLoginToGroups[userLogin] = append(userLoginToGroups[userLogin], teamSlug, strconv.Itoa(teamID))
 			}
 		}
 	}
@@ -154,13 +155,15 @@ func (p *Provider) listOrgs(ctx context.Context) (orgSlugs []string, err error)
 	return orgSlugs, nil
 }
 
-func (p *Provider) listTeams(ctx context.Context, orgSlug string) (teamSlugs []string, err error) {
+func (p *Provider) listTeams(ctx context.Context, orgSlug string) (map[string]int, error) {
 	nextURL := p.cfg.url.ResolveReference(&url.URL{
 		Path: fmt.Sprintf("/orgs/%s/teams", orgSlug),
 	}).String()
 
+	teamSlugs := make(map[string]int)
 	for nextURL != "" {
 		var results []struct {
+			ID   int    `json:"id"`
 			Slug string `json:"slug"`
 		}
 		hdrs, err := p.api(ctx, "GET", nextURL, nil, &results)
@@ -169,7 +172,7 @@ func (p *Provider) listTeams(ctx context.Context, orgSlug string) (teamSlugs []s
 		}
 
 		for _, result := range results {
-			teamSlugs = append(teamSlugs, result.Slug)
+			teamSlugs[result.Slug] = result.ID
 		}
 
 		nextURL = getNextLink(hdrs)

internal/directory/github/github_test.go

@@ -38,12 +38,12 @@ func newMockAPI(t *testing.T, srv *httptest.Server) http.Handler {
 	r.Get("/orgs/{org_id}/teams", func(w http.ResponseWriter, r *http.Request) {
 		teams := map[string][]M{
 			"org1": {
-				{"slug": "team1"},
+				{"slug": "team1", "id": 1},
-				{"slug": "team2"},
+				{"slug": "team2", "id": 2},
 			},
 			"org2": {
-				{"slug": "team3"},
+				{"slug": "team3", "id": 3},
-				{"slug": "team4"},
+				{"slug": "team4", "id": 4},
 			},
 		}
 		orgID := chi.URLParam(r, "org_id")
@@ -96,10 +96,10 @@ func Test(t *testing.T) {
 	users, err := p.UserGroups(context.Background())
 	assert.NoError(t, err)
 	testutil.AssertProtoJSONEqual(t, `[
-		{ "id": "github/user1", "groups": ["team1", "team2", "team3"] },
+		{ "id": "github/user1", "groups": ["1", "2", "3", "team1", "team2", "team3"] },
-		{ "id": "github/user2", "groups": ["team1", "team3"] },
+		{ "id": "github/user2", "groups": ["1", "3", "team1", "team3"] },
-		{ "id": "github/user3", "groups": ["team3"] },
+		{ "id": "github/user3", "groups": ["3", "team3"] },
-		{ "id": "github/user4", "groups": ["team4"] }
+		{ "id": "github/user4", "groups": ["4", "team4"] }
 	]`, users)
 }
 

internal/directory/gitlab/gitlab.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/url"
 	"sort"
+	"strconv"
 
 	"github.com/rs/zerolog"
 	"github.com/tomnomnom/linkheader"
@@ -90,31 +91,31 @@ func (p *Provider) UserGroups(ctx context.Context) ([]*directory.User, error) {
 
 	p.log.Info().Msg("getting user groups")
 
-	groupIDs, err := p.listGroupIDs(ctx)
+	groups, err := p.listGroups(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	userIDToGroupIDs := map[int][]int{}
+	userIDToGroupIDs := map[int][]string{}
-	for _, groupID := range groupIDs {
+	for groupID, groupName := range groups {
 		userIDs, err := p.listGroupMemberIDs(ctx, groupID)
 		if err != nil {
 			return nil, err
 		}
 
 		for _, userID := range userIDs {
-			userIDToGroupIDs[userID] = append(userIDToGroupIDs[userID], groupID)
+			userIDToGroupIDs[userID] = append(userIDToGroupIDs[userID], strconv.Itoa(groupID), groupName)
 		}
 	}
 
 	var users []*directory.User
-	for userID, groupIDs := range userIDToGroupIDs {
+	for userID, groups := range userIDToGroupIDs {
 		user := &directory.User{
 			Id: databroker.GetUserID(Name, fmt.Sprint(userID)),
 		}
-		for _, groupID := range groupIDs {
+
-			user.Groups = append(user.Groups, fmt.Sprint(groupID))
+		user.Groups = append(user.Groups, groups...)
-		}
+
 		sort.Strings(user.Groups)
 		users = append(users, user)
 	}
@@ -124,13 +125,16 @@ func (p *Provider) UserGroups(ctx context.Context) ([]*directory.User, error) {
 	return users, nil
 }
 
-func (p *Provider) listGroupIDs(ctx context.Context) (groupIDs []int, err error) {
+// listGroups returns a map, with key is group ID, element is group name.
+func (p *Provider) listGroups(ctx context.Context) (map[int]string, error) {
 	nextURL := p.cfg.url.ResolveReference(&url.URL{
 		Path: "/api/v4/groups",
 	}).String()
+	groups := make(map[int]string)
 	for nextURL != "" {
 		var result []struct {
-			ID int `json:"id"`
+			ID   int    `json:"id"`
+			Name string `json:"name"`
 		}
 		hdrs, err := p.apiGet(ctx, nextURL, &result)
 		if err != nil {
@@ -138,12 +142,12 @@ func (p *Provider) listGroupIDs(ctx context.Context) (groupIDs []int, err error)
 		}
 
 		for _, r := range result {
-			groupIDs = append(groupIDs, r.ID)
+			groups[r.ID] = r.Name
 		}
 
 		nextURL = getNextLink(hdrs)
 	}
-	return groupIDs, nil
+	return groups, nil
 }
 
 func (p *Provider) listGroupMemberIDs(ctx context.Context, groupID int) (userIDs []int, err error) {

internal/directory/gitlab/gitlab_test.go

@@ -32,8 +32,8 @@ func newMockAPI(t *testing.T, srv *httptest.Server) http.Handler {
 		})
 		r.Get("/groups", func(w http.ResponseWriter, r *http.Request) {
 			_ = json.NewEncoder(w).Encode([]M{
-				{"id": 1},
+				{"id": 1, "name": "Group 1"},
-				{"id": 2},
+				{"id": 2, "name": "Group 2"},
 			})
 		})
 		r.Get("/groups/{group_name}/members", func(w http.ResponseWriter, r *http.Request) {
@@ -69,9 +69,9 @@ func Test(t *testing.T) {
 	users, err := p.UserGroups(context.Background())
 	assert.NoError(t, err)
 	testutil.AssertProtoJSONEqual(t, `[
-		{ "id": "gitlab/11", "groups": ["1"] },
+		{ "id": "gitlab/11", "groups": ["1", "Group 1"] },
-		{ "id": "gitlab/12", "groups": ["2"] },
+		{ "id": "gitlab/12", "groups": ["2", "Group 2"] },
-		{ "id": "gitlab/13", "groups": ["2"] }
+		{ "id": "gitlab/13", "groups": ["2", "Group 2"] }
 	]`, users)
 }
 

internal/directory/okta/okta.go

@@ -108,7 +108,7 @@ func (p *Provider) UserGroups(ctx context.Context) ([]*directory.User, error) {
 			return nil, err
 		}
 		for _, id := range ids {
-			userIDToGroups[id] = append(userIDToGroups[id], groupName)
+			userIDToGroups[id] = append(userIDToGroups[id], groupID, groupName)
 		}
 	}
 

internal/directory/okta/okta_test.go

@@ -53,7 +53,7 @@ func newMockOkta(srv *httptest.Server, userEmailToGroups map[string][]string) ht
 				result = append(result, M{
 					"id": groups[i],
 					"profile": M{
-						"name": groups[i],
+						"name": groups[i] + "-name",
 					},
 				})
 				break
@@ -120,15 +120,15 @@ func TestProvider_UserGroups(t *testing.T) {
 	assert.Equal(t, []*directory.User{
 		{
 			Id:     "okta/a@example.com",
-			Groups: []string{"admin", "user"},
+			Groups: []string{"admin", "admin-name", "user", "user-name"},
 		},
 		{
 			Id:     "okta/b@example.com",
-			Groups: []string{"test", "user"},
+			Groups: []string{"test", "test-name", "user", "user-name"},
 		},
 		{
 			Id:     "okta/c@example.com",
-			Groups: []string{"user"},
+			Groups: []string{"user", "user-name"},
 		},
 	}, users)
 }

internal/directory/onelogin/onelogin.go

@@ -121,7 +121,7 @@ func (p *Provider) UserGroups(ctx context.Context) ([]*directory.User, error) {
 	for userID, groupIDs := range userIDToGroupIDs {
 		for _, groupID := range groupIDs {
 			if groupName, ok := groupIDToName[groupID]; ok {
-				userIDToGroupNames[userID] = append(userIDToGroupNames[userID], groupName)
+				userIDToGroupNames[userID] = append(userIDToGroupNames[userID], strconv.Itoa(groupID), groupName)
 			} else {
 				userIDToGroupNames[userID] = append(userIDToGroupNames[userID], "NOGROUP")
 			}

internal/directory/onelogin/onelogin_test.go

@@ -152,15 +152,15 @@ func TestProvider_UserGroups(t *testing.T) {
 	assert.Equal(t, []*directory.User{
 		{
 			Id:     "onelogin/111",
-			Groups: []string{"admin"},
+			Groups: []string{"0", "admin"},
 		},
 		{
 			Id:     "onelogin/222",
-			Groups: []string{"test"},
+			Groups: []string{"1", "test"},
 		},
 		{
 			Id:     "onelogin/333",
-			Groups: []string{"user"},
+			Groups: []string{"2", "user"},
 		},
 	}, users)
 }