auth: walk nested groups #149
Conversation
jphines
force-pushed
the
recursively-walk-nested-groups
branch
from
02450d6
to
a4ac0c4
Compare
| @@ -109,6 +109,17 @@ func (gs *GoogleAdminService) GetMembers(groupName string) ([]string, error) { | |||
|
|
|||
| for _, member := range r.Members { | |||
| members = append(members, member.Email) | |||
There was a problem hiding this comment.
do we want to append the member.Email if the member type is a group?
There was a problem hiding this comment.
updated this logic with a switch to not add if its a group
jphines
force-pushed
the
recursively-walk-nested-groups
branch
6 times, most recently
from
73dd940
to
36a0158
Compare
| if len(allGroups) == 0 { | ||
| return p.AdminService.GetGroups(email) | ||
| return []string{}, nil |
There was a problem hiding this comment.
This change seems to invalidate the docstring for this function, which says
If
allGroupsis an empty list it returns all the groups that the user belongs to.
The implications of this change aren't obvious to me, so it's not clear if it's a necessary part of this feature, or something unrelated. Would you mind explaining?
There was a problem hiding this comment.
We added this originally to ensure backward compatibility when the proxy didn't use to ask for explicit groups, but instead asked for membership of all groups -- will update the docstring.
| // GetGroups gets the groups that a user with a given email address belongs to. | ||
| func (gs *GoogleAdminService) GetGroups(email string) ([]string, error) { | ||
| var groups []string | ||
| // HasMember checks if the user has membership in the given groups |
There was a problem hiding this comment.
Nit: HasMember strikes me as an odd name for a function that returns a subset of the given groups that the given email is in. Basically, I'd expect HasMember to return a bool. What about something like CheckMembership instead?
(Also, it might be good to expand the docstring here to more accurately describe what this function is doing w/r/t returning a subset of the given groups.)
There was a problem hiding this comment.
I wanted to reflect the underlying API endpoint we're leveraging on the google side (which is a bool endpoint, but is nice because it does nested group resolution).
I agree this seems to imply this would be a bool by name along -- not sure what would be better.
jphines
force-pushed
the
recursively-walk-nested-groups
branch
from
68f0509
to
63ce4bd
Compare
|
I have the AD provider set up to do this too for the same reason. |
jphines
force-pushed
the
recursively-walk-nested-groups
branch
from
2e8cf5f
to
7177147
Compare
jphines
force-pushed
the
recursively-walk-nested-groups
branch
from
7177147
to
77d7221
Compare
|
Fixes #133 |
Problem
We have nested google groups we need to unroll in-order to properly leverage them
Solution
Recursively resolve nested groups up to maxDepth (default is 4)