From ee1f9093eeff853e1c6a241c8ac2ae5ac900b35c Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Fri, 17 Jul 2020 00:15:11 +0700 Subject: [PATCH] internal/directory: use both id and name for group (#1086) Fixes #1085 --- internal/directory/azure/azure.go | 15 ++++++----- internal/directory/azure/azure_test.go | 10 +++---- internal/directory/github/github.go | 11 +++++--- internal/directory/github/github_test.go | 16 +++++------ internal/directory/gitlab/gitlab.go | 28 +++++++++++--------- internal/directory/gitlab/gitlab_test.go | 10 +++---- internal/directory/okta/okta.go | 2 +- internal/directory/okta/okta_test.go | 8 +++--- internal/directory/onelogin/onelogin.go | 2 +- internal/directory/onelogin/onelogin_test.go | 6 ++--- 10 files changed, 59 insertions(+), 49 deletions(-) diff --git a/internal/directory/azure/azure.go b/internal/directory/azure/azure.go index f058b544f..48ed772f2 100644 --- a/internal/directory/azure/azure.go +++ b/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) { diff --git a/internal/directory/azure/azure_test.go b/internal/directory/azure/azure_test.go index 1e23604d0..e2d86ae7e 100644 --- a/internal/directory/azure/azure_test.go +++ b/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": "test"}, + {"id": "admin", "name": "Admin Group"}, + {"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) } diff --git a/internal/directory/github/github.go b/internal/directory/github/github.go index 02b696e66..5f0b5532c 100644 --- a/internal/directory/github/github.go +++ b/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) diff --git a/internal/directory/github/github_test.go b/internal/directory/github/github_test.go index c6026ce74..9d55f0725 100644 --- a/internal/directory/github/github_test.go +++ b/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": "team2"}, + {"slug": "team1", "id": 1}, + {"slug": "team2", "id": 2}, }, "org2": { - {"slug": "team3"}, - {"slug": "team4"}, + {"slug": "team3", "id": 3}, + {"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/user2", "groups": ["team1", "team3"] }, - { "id": "github/user3", "groups": ["team3"] }, - { "id": "github/user4", "groups": ["team4"] } + { "id": "github/user1", "groups": ["1", "2", "3", "team1", "team2", "team3"] }, + { "id": "github/user2", "groups": ["1", "3", "team1", "team3"] }, + { "id": "github/user3", "groups": ["3", "team3"] }, + { "id": "github/user4", "groups": ["4", "team4"] } ]`, users) } diff --git a/internal/directory/gitlab/gitlab.go b/internal/directory/gitlab/gitlab.go index 8b3b32a65..36e485702 100644 --- a/internal/directory/gitlab/gitlab.go +++ b/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{} - for _, groupID := range groupIDs { + userIDToGroupIDs := map[int][]string{} + 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) { diff --git a/internal/directory/gitlab/gitlab_test.go b/internal/directory/gitlab/gitlab_test.go index ffd1991ad..5324df7e8 100644 --- a/internal/directory/gitlab/gitlab_test.go +++ b/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": 2}, + {"id": 1, "name": "Group 1"}, + {"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/12", "groups": ["2"] }, - { "id": "gitlab/13", "groups": ["2"] } + { "id": "gitlab/11", "groups": ["1", "Group 1"] }, + { "id": "gitlab/12", "groups": ["2", "Group 2"] }, + { "id": "gitlab/13", "groups": ["2", "Group 2"] } ]`, users) } diff --git a/internal/directory/okta/okta.go b/internal/directory/okta/okta.go index 51a29ae2d..8e38b6a26 100644 --- a/internal/directory/okta/okta.go +++ b/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) } } diff --git a/internal/directory/okta/okta_test.go b/internal/directory/okta/okta_test.go index fa0571ede..9361aeeda 100644 --- a/internal/directory/okta/okta_test.go +++ b/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) } diff --git a/internal/directory/onelogin/onelogin.go b/internal/directory/onelogin/onelogin.go index 00b00d103..8a630e17e 100644 --- a/internal/directory/onelogin/onelogin.go +++ b/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") } diff --git a/internal/directory/onelogin/onelogin_test.go b/internal/directory/onelogin/onelogin_test.go index 4f11ca9af..756426366 100644 --- a/internal/directory/onelogin/onelogin_test.go +++ b/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) }