Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

[Feature Request] Ability to sync admins that are in a nested group for superadmins_group by default #71

Open
CoreyGriffin opened this issue Sep 11, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@CoreyGriffin
Copy link

CoreyGriffin commented Sep 11, 2019

Expected Behaviour

Setting SUPERADMINS_GROUP to "[email protected]" will provide proper permissions to everyone in the group for the GnG web app

Actual Behaviour

Potentially not an issue but more so an edge case we ran into where we have a bridge group containing two separate google groups for 2 teams having their own group. Since both teams require the same permissions, we are setting the parent group as the SUPERADMINS_GROUP the current get_all_users_in_group method does not support nested groups so unless the users are in the top level group...they will not get access

TLDR

[email protected]

Only members of [email protected] (random-team-member 1 &2) get web app admin permissions. Members of the nested groups do not.

Steps to Reproduce

Add a nested group to your super admins group and try to perform an admin action in the web app as a member of the nested group

Potential Workaround

We implemented a potential workaround for this use case by adjusting the get_all_users_in_group and _users_in_group methods ... we're currently still testing out possible solutions.

1. Overwrite the Group Member Fields mask

The _users_in_group method utilizes a mask defined in constants.py for the response which only returns the member's email. We need the member's type as well, so we just used a variable to replace this mask (see below) could probably update the constants.py file but assuming that would require another bootstrap run?

    def _users_in_group(self, group_email, page_token=None):
        try:
            GROUP_MEMBER_FIELDS_MASK_OVERRIDE = "members/email,members/type,nextPageToken"
            return self._client.members().list(
                groupKey=group_email,
                pageToken=page_token,
                fields=GROUP_MEMBER_FIELDS_MASK_OVERRIDE).execute() <--- Use mask override

        except errors.HttpError as err:
            logging.error(
                'Directory API group members failed with a %s exception because %s.',
                str(type(err)), err.resp.reason)
            raise DirectoryRPCError(err.resp.reason)

2. Update GET users method to utilize the newly available title field

    def get_all_users_in_group(self, group_email, users=[]):
        """Retrieves all of the users in a particular Google Group.

        Args:
          group_email: str, the email address of the group.

        Returns:
          A list of all user's email addresses in the group provided.
        """

        nested_groups = []
        response = self._users_in_group(group_email)
        # There is no next page so build users list and return it
        if not response.get(_NEXT_PAGE):
            members = response.get('members')
            if members:
                for member in members:
                    if member.get("type") == "GROUP":
                        nested_groups.append(member['email'].lower())
                    else:
                        users.append(member['email'])

            if nested_groups:
                for group in nested_groups[::-1]:
                    self.get_all_users_in_group(group, users)
                    nested_groups.pop()
            return set(users)

        # Need to paginate
        while response.get(_NEXT_PAGE):
            for member in response['members']:
                if member.get("type") == "GROUP":
                    nested_groups.append(member['email'].lower())
                else:
                    users.append(member['email'])
            response = self._users_in_group(
                group_email, response.get(_NEXT_PAGE))

        for member in response['members']:
            if member.get("type") == "GROUP":
                nested_groups.append(member['email'].lower())
            else:
                users.append(member['email'])

        if nested_groups:
            for group in nested_groups[::-1]:
                self.get_all_users_in_group(group, users)
                nested_groups.pop()
        return set(users)

3. Deploy and re-run /_cron/sync_user_roles as needed

@CoreyGriffin CoreyGriffin changed the title [Feature Request] Ability to sync admins that are in a nested distro for superadmins_group by default [Feature Request] Ability to sync admins that are in a nested group for superadmins_group by default Sep 11, 2019
@gyoshi02
Copy link

I tried to deploy with this script, but my nested groups don't have access still.
Is there another version of this?
Or is the "work around" going to be not having nested groups?
Thanks,
Grant

@CoreyGriffin
Copy link
Author

hey @gyoshi02 as mentioned in the issue we're currently still testing out possible solutions since this was an edge case we ran into (you may be in the same position). But yeah, as long the users are in the root admin group they would have proper access since that's how the code written by the maintainers works. Just out of curiosity since you tested with the script above...did you get any errors or did it just not work? There could be other changes that need to be made that we haven't discovered yet.

@gyoshi02
Copy link

Hey @CoreyGriffin
I didn't get any error while running the deployment script.
I can send over any logs you'd like, or any info within cloud console.
Only thing that I had noticed was that nested groups (which I moved my user account to and from) didn't work still.
Happy to test any other alternative code updates.

@CoreyGriffin
Copy link
Author

@gyoshi02 had some time to look at this again today...the reason that script doesn’t work as is was because they are masking the response in constants.py which only pulls the email and pageToken ... you can either add members/type to that mask variable which I believe (not certain) would require a bootstrap or you can just overwrite it in the private _users_in_group method ... I updated the solution above with the changes I made...they appear to have done the trick for me

@gyoshi02
Copy link

Hey @CoreyGriffin
Just wanted to check back in here.
I'm about to run through another deploy. Pls confirm that I just need to add your amended code to the web_app/backend/clients/directory.py file, or is there a change that must also be made in constants.py?

@CoreyGriffin
Copy link
Author

@gyoshi02 the code above should work, you may want to double check the indentation if you're just copying and pasting before deploying. I did not change the constants file...just the code in the directory.py file

@gyoshi02
Copy link

@CoreyGriffin
Yup, there was a slight tab issue when I copied, but was able to get it working after a couple of re-deploys. I think our nested groups are working now, but will report back, after testing further tomorrow.

@CoreyGriffin
Copy link
Author

@gyoshi02 everything end up working for you?

@gyoshi02
Copy link

@CoreyGriffin
I’m all good with my nested google groups.
Was able to confirm the users that should have access based on the groups.
Thanks for quick response, and follow up.

@helfrichmichael helfrichmichael added the enhancement New feature or request label Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants