Add option to fetch indirect group memberships from Google #2077
Conversation
Signed-off-by: Santi Saez <[email protected]>
| @@ -254,14 +259,23 @@ func (c *googleConnector) getGroups(email string) ([]string, error) { | |||
| for _, group := range groupsList.Groups { | |||
| // TODO (joelspeed): Make desired group key configurable | |||
| userGroups = append(userGroups, group.Email) | |||
|
|
|||
| if groupsIndirectFetch { | |||
| indirectGroups, err := c.getGroups(group.Email, groupsIndirectFetch) | |||
There was a problem hiding this comment.
Quoting from https://developers.google.com/admin-sdk/directory/v1/guides/manage-groups#get_all_member_groups:
The userKey can be the user's primary email address, the user's alias email address, a group's primary email address, a group's email alias, or the user's unique id
And when a group is provided as userKey the parent groups will be returned.
There was a problem hiding this comment.
Would be good to add some of this context into the function as a comment IMO
| // If this field is true, both direct and indirect group memberships will be fetched | ||
| GroupsIndirectFetch bool `json:"groupsIndirectFetch"` |
There was a problem hiding this comment.
I think it would be better to have a more explicit naming here.
Eg. FetchIndirectGroupMemberships or FetchTransitiveGroupMembership, WDYT?
| @@ -254,14 +259,23 @@ func (c *googleConnector) getGroups(email string) ([]string, error) { | |||
| for _, group := range groupsList.Groups { | |||
| // TODO (joelspeed): Make desired group key configurable | |||
| userGroups = append(userGroups, group.Email) | |||
|
|
|||
| if groupsIndirectFetch { | |||
| indirectGroups, err := c.getGroups(group.Email, groupsIndirectFetch) | |||
There was a problem hiding this comment.
Would be good to add some of this context into the function as a comment IMO
| keys := make(map[string]bool) | ||
| unique := []string{} | ||
|
|
||
| for _, group := range groups { | ||
| if _, value := keys[group]; !value { | ||
| keys[group] = true | ||
| unique = append(unique, group) | ||
| } | ||
| } | ||
|
|
||
| return unique | ||
| } |
There was a problem hiding this comment.
If you use a struct, it's technically more efficient, I would do this as (this is very much a nit)
| keys := make(map[string]bool) | |
| unique := []string{} | |
| for _, group := range groups { | |
| if _, value := keys[group]; !value { | |
| keys[group] = true | |
| unique = append(unique, group) | |
| } | |
| } | |
| return unique | |
| } | |
| keys := make(map[string]struct{}) | |
| unique := []string{} | |
| for _, group := range groups { | |
| if _, exists := keys[group]; !exists { | |
| keys[group] = struct{}{} | |
| unique = append(unique, group) | |
| } | |
| } | |
| return unique | |
| } |
|
Hey @santisaez and Dex folks, anything I can do to help to help move this PR forward? Would love to be able to use nested groups with Dex. |
Overview
Google Workspace groups (aka G Suite) have support for nesting groups, that is, add a group to another group. This is useful to automatically add members from a small team (backend, for example) to a larger department (engineering, for example).
A user in Google Workspace can have both direct and indirect group memberships. In the example above, "backend" is a direct group and "engineering" is an indirect group membership. The Google Admin SDK API only returns direct group memberships, and thus
dexonly returns direct memberships.What this PR does / why we need it
This PR extends the work done by @JoelSpeed in #1185 and adds an option to fetch the indirect group memberships from Google Workspace, aka G Suite. This feature will be useful to fetch all groups a user belongs to, both direct and indirect.
Special notes for your reviewer
cc/ @JoelSpeed because they developed the feature to fetch the groups from Google
Does this PR introduce a user-facing change?