Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix display of indirect access permissions. #4529

Merged
merged 2 commits into from
Sep 5, 2019
Merged

Fix display of indirect access permissions. #4529

merged 2 commits into from
Sep 5, 2019

Conversation

wenottingham
Copy link
Contributor

For indirect roles, we need to actually show the derived roles, not the
details of the role that gives us the derived roles. This means that
we can get multiple derived roles from a single indirect role, so
we have to expand the list.

UI-only implementation of #4514

Will help with #3903 and #4108.

Example for an inventory:
'admin' has indirect access by being the sysadmin.
'joe' has indirect access by being an admin of the org the inventory belongs to.
'fred', from outside the org, has been granted 'read' and 'update' explicitly to his outside-the-org team.
'fred' and 'bob' are org admins of fred's org, and therefore get indirect access to the team permission of fred's team.

Screenshot from 2019-08-20 12-19-40

@wenottingham
Copy link
Contributor Author

Before this patch, bob and fred would be shown with just admin, which is misleading to the user.

@wenottingham
Copy link
Contributor Author

Note: this patch does not address awx-pf/ui-next at all, sorry.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@jakemcdermott
Copy link
Contributor

cc @AlanCoding

@AlanCoding
Copy link
Member

Before this change:

Screen Shot 2019-08-20 at 1 30 06 PM

After this change:

Screen Shot 2019-08-20 at 1 32 16 PM

The first problem is that system administrators are now shown as ADMIN, which I don't like. System auditors may have a similar issue.

rando has execute access to this JT via the organization execute role. That has the READ role added to it... which I guess is what it is.

The team entries all have tooltip text. None of the indirect access forms do, but there is more information available from the API that could at least do a little to address the real issue that the user isn't getting enough information to act (to revoke permissions, for example).

@wenottingham
Copy link
Contributor Author

The first problem is that system administrators are now shown as ADMIN, which I don't like. System auditors may have a similar issue.

Will investigate.

@wenottingham
Copy link
Contributor Author

Updated. Erred on the side of excessive comments, because RBAC.

Now, for the following:

  • sysadmin admin
  • sys auditor auditor
  • org admin bob
  • org auditor orgaudit
  • org inventory admin robin
  • outside org user fred delegated Use via a team
  • org admin joe of fred's org

Screenshot from 2019-08-20 20-07-11

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

For indirect roles, we need to actually show the derived roles, not the
details of the role that gives us the derived roles. This means that
we can get multiple derived roles from a single indirect role, so
we have to expand the list.
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member

The specific instructions for delegating permissions have become too much for me. I have written this script to fully populate the permission access types for an organization.

    def test_full_house(self, factories):
        """Makes every object in an organization and delegates all permissions to everything"""
        o = factories.organization(name='Full house!!! {}'.format(random_title()))
        print('Adding people to organization roles, org name: {}'.format(o.name))
        for role in o.get_related('object_roles').results:
            u = factories.user()
            role_field = role['name']
            o.set_object_roles(u, role_field)
            if role_field.lower() not in ('admin', 'member'):
                team = factories.team()
                o.set_object_roles(team, role_field)
                to = team.ds.organization
                u2 = factories.user()
                to.set_object_roles(u2, 'admin')
                u3 = factories.user()
                team.set_object_roles(u3, 'member')
                u4 = factories.user()
                team.set_object_roles(u3, 'admin')

        print('Creating Resources:')
        for res in dir(factories):
            if res.startswith('__') or res in ('clear', 'copy', 'fromkeys', 'get', 'items', 'keys', 'pop', 'popitem',
                                               'setdefault', 'update', 'values',
                                               'access_token', 'ad_hoc_command', 'credential_type',
                                               'group', 'host', 'instance_group', 'inventory_source',
                                               'organization', 'workflow_job_template_node'):
                continue
            print('  {}'.format(res))
            if res == 'job_template':
                obj = factories.job_template(project=factories.project(organization=o))
            else:
                obj = getattr(factories, res)(organization=o)
            if res not in ('user', 'job_template'):
                assert obj.organization == o.id
            if res not in ('user',):
                if 'object_roles' not in obj.related:
                    print('   resource {} does not have roles??? okay?'.format(res))
                    continue
                obj_roles = obj.get_related('object_roles').results
                for role in obj_roles:
                    if res == 'credential':
                        u = factories.user(organization=o)
                        team = factories.team(organization=o)
                    else:
                        u = factories.user()
                        team = factories.team()
                    role_field = role['name']
                    obj.set_object_roles(u, role_field)
                    obj.set_object_roles(team, role_field)
                    to = team.ds.organization
                    u2 = factories.user()
                    to.set_object_roles(u2, 'admin')
                    u3 = factories.user()
                    team.set_object_roles(u3, 'member')
                    u4 = factories.user()
                    team.set_object_roles(u3, 'admin')

For a given role, you can have direct access to that role. Alternatively, you can be a member of a team, alternatively, you can be the admin of that team, alternatively, you can be org admin for the team's organization. Repeat for every possible role for every possible type of resource.

Here is prior behavior before this PR for a job template (the list is very long, this is just a part of it):

Screen Shot 2019-08-21 at 1 42 45 PM

Your PR is effective in converting the incorrect "ADMIN" entries to things like "READ" or "EXECUTE", but this still leaves the user with no options and no information. Basically, this is not using the access list at all, all it's doing it echoing out the permission levels with nothing else. This is still a very frustrating situation for the user.

I made a PR to your branch:

https://github.com/wenottingham/awx/pull/1

This adds the most basic tooltip that I can conceive of. Here is your PR combined with that change:

Screen Shot 2019-08-21 at 1 57 58 PM

This at least gives the user a chance at figuring out who to talk to about why the permissions are the way they are.

Note that the organization mentioned in the tooltip is not the organization of the JT's project or inventory.

@AlanCoding
Copy link
Member

I made a small error in the script, the 2 instances of team.set_object_roles(u3, 'admin') should use u4 instead. That is the cause of the double-team permissions.

Copy link
Member

@AlanCoding AlanCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/wenottingham/awx/pull/1, the current form would still be very hard for the user to cope with.

@wenottingham
Copy link
Contributor Author

Updated based on Alan's PR:

Screenshot from 2019-08-30 16-31-58

cc @AlanCoding @jlmitch5

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member

Yeah that looks pretty good.

The icon follows the parameters of the solution discussed with @jlmitch5. If he's happy with this, your screenshot demonstrates enough cases for me to say shipit.

@jlmitch5
Copy link
Contributor

jlmitch5 commented Sep 3, 2019

conceptually I think this make sense, the tooltip only shows for the ones that are indirect right?

cc @trahman73 about the look of it

@trahman73
Copy link

@jlmitch5 works for me

Copy link
Contributor

@jlmitch5 jlmitch5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants