Pulp container RBAC #705

newswangerd · 2021-03-29T14:00:07Z

UI PR for container model permissions: ansible/ansible-hub-ui#335

Changes

pulp_container access policies

Allow anyone to create a container namespace. This simplifies the new repo creation process by only requiring that the user have create_containerdistribution permissions to create a new container repo.
Require authentication for pulling containers
Remove group creation. The user that creates the repository is added as an owner on the repository's namespace instead of creating a new group for the user.

Groups

Filtering pulp container groups turned out to be a real challenge, so I've opted to remove the pulp container groups entirely.

Container Repo endpoint

Removed groups since permissions will be set at the namespace level
Added a my_perms filter that allows querying repos based on permissions that the user has. Ex: ?my_perms=namespace_push_containerdistribution returns all repos that the current user has push permissions on.
Updated with the following fields
- my_permissions: returns the permissions my user has on the namespace object. Used for the UI to toggle edit buttons.
- owners: list of users who have permissions on the object outside of the assigned groups. This shows who created and owns the namespace.

            "namespace": {
                "name": "a",
                "my_permissions": [
                    "namespace_modify_content_containerpushrepository",
                    "namespace_add_containerdistribution",
                    "namespace_pull_containerdistribution",
                    "view_containernamespace",
                    "namespace_delete_containerdistribution",
                    "change_containernamespace",
                    "namespace_view_containerdistribution",
                    "namespace_push_containerdistribution",
                    "namespace_change_containerdistribution",
                    "delete_containernamespace",
                    "namespace_view_containerpushrepository"
                ],
                "owners": [
                    "test"
                ]
            },

Additions

Container namespace endpoint

Add /api/automation-hub/_ui/v1/execution-environments/namespaces/ endpoint which allows viewing and updating permissions on container namespaces.

Permissions for containers can be set at two levels: container distributions and container namespaces. Namespaces grant a user access to all containers in a namespace.

We are only going to allow users to set permissions at the namespace level for the following reasons:

Simplicity: setting permissions on the container distribution level is more complicated because some permissions have to be set on the distribution and others have to be set on the container push repository, which would require extra api calls.
Consistency: permissions are set at a namespace level for collections, so it makes sense from a UX perspective to do the same
Predictability: only setting permissions at the repo level will lead to confusing situations where a user might think they have permissions on a namespace when they don't.

alikins · 2021-03-29T18:24:26Z

galaxy_ng/app/api/ui/urls.py

+    path(
+        "namespaces/<str:name>/",
+        viewsets.ContainerNamespaceViewSet.as_view({'get': 'retrieve', 'put': 'update'}),
+        name='container-repository-list'),


container-repository-detail?

alikins · 2021-03-29T18:26:58Z

galaxy_ng/app/api/ui/urls.py

+    path(
+        "namespaces/",
+        viewsets.ContainerNamespaceViewSet.as_view({'get': 'list'}),
+        name='container-repository-list'),


container-namespace-list ?

alikins · 2021-03-29T18:52:52Z

galaxy_ng/app/api/ui/serializers/execution_environment.py

@@ -73,6 +88,10 @@ def get_pulp(self, distro):
            }
        }

+    def get_owners(self, distro):
+        name = f'container.distribution.owners.{distro.pulp_id}'
+        return [user.username for user in models.User.objects.filter(groups__name=name)]


Would something like

return models.User.objects.filter(groups__name=name).values_list('username', flat=True)

Work here? Not sure if it matters on a small object like User

oh, this function should be removed. I must have missed that.

alikins

One or two requested changes (or really, verifications). Most of the rest are vague suggestions you can take or leave.

galaxy_ng/app/access_control/fields.py

galaxy_ng/app/api/ui/viewsets/execution_environment.py

alikins · 2021-03-31T16:56:24Z

galaxy_ng/app/api/ui/viewsets/execution_environment.py

@@ -36,6 +40,12 @@ class Meta:
            'description': ['exact', 'icontains', 'contains', 'startswith'],
        }

+    def has_permissions(self, queryset, name, value):


Maybe 'get_permissions'? has_permissions implies a boolean response to me.

Though, I guess it is returning a queryset of ContainerNamepaces, so 'get_permissions' doesn't seem right either...

would with_permissions make more sense?

Looks like the django_filter docs tend to name them 'filter_whatever' (https://django-filter.readthedocs.io/en/master/ref/filters.html#method).

'filter_by_permission' ? 'filter_accesible_namespaces' ?

Your call ;->

galaxy_ng/app/api/ui/viewsets/group.py

galaxy_ng/app/migrations/0019_pulp_container_access_policies.py

galaxy_ng/app/models/container.py

alikins

LGTM

mdellweg · 2021-04-01T08:35:24Z

I was asked to take a look at this and as far as i understood, almost everything looks sane to me. Especially the changes to the access_policy.
However a warning is in order: Running pulpcore-manager flush will remove the access policies and still prevent this migration to run. That is the reason, why we decided to switch creating them to a post-migrate signal.

ipanova · 2021-04-07T12:22:27Z

galaxy_ng/app/migrations/0019_pulp_container_access_policies.py

+viewsets = {
+    # Note. This is the default Pulp Continer access policy with some modifications.
+    # Our changes have been marked with comments.
+    "distributions/container/container": {


The policy for container-push repos is omitted here (tag/untag/remove image) - this will be added later whenever image management will be enabled, correct?

@ipanova are there things in the container push repo access policies that we should change? it looked like the defaults would work to me.

clarified on the irc, resolved.

ipanova · 2021-04-07T12:26:34Z

galaxy_ng/app/migrations/0019_pulp_container_access_policies.py

+                ],
+            },
+            {
+                "action": ["pull"],


there are 2 policies for pull operation - should you want to distinguish rules between private and public repo - it makes sense to keep this as it is proposed, otherwise it could be consolidated into one rule where the authentication is required regardless of the private/public repo type

{ "action": ["pull"], "principal": "authenticated", "effect": "allow", },

Issue: AAH-278

- Add my permissions to repos and namespaces - Add my_permissions filter to repo list view. - Fix access policies. Issue: AAH-278

Issue: AAH-278

newswangerd changed the title ~~Pulp container RBAC~~ [WIP] Pulp container RBAC Mar 29, 2021

newswangerd force-pushed the feature/AAH-278-pulp-container-rbac branch from 48c1885 to f84a6ba Compare March 30, 2021 20:17

newswangerd changed the title ~~[WIP] Pulp container RBAC~~ Pulp container RBAC Mar 30, 2021

newswangerd requested review from rochacbruno, alikins, awcrosby and ZitaNemeckova March 30, 2021 20:46

newswangerd force-pushed the feature/AAH-278-pulp-container-rbac branch 2 times, most recently from f950960 to a37ee63 Compare March 30, 2021 20:57

rochacbruno approved these changes Mar 31, 2021

View reviewed changes

alikins reviewed Mar 31, 2021

View reviewed changes

alikins suggested changes Mar 31, 2021

View reviewed changes

alikins approved these changes Mar 31, 2021

View reviewed changes

ipanova reviewed Apr 7, 2021

View reviewed changes

newswangerd mentioned this pull request Apr 7, 2021

Container access policies 278 #687

Closed

newswangerd added 8 commits April 9, 2021 09:39

Add initial access policy overrides.

7b93622

Issue: AAH-278

Update access policies for use by galaxy

0e0e631

Issue: AAH-278

Rebase migrations.

58df660

Issue: AAH-278

- Rebase migrations

3518b70

- Add my permissions to repos and namespaces - Add my_permissions filter to repo list view. - Fix access policies. Issue: AAH-278

Add filters for filtering out container groups.

e081d23

Issue: AAH-278

Remove pulp_container group creation.

51e51c4

Issue: AAH-278

Fix miscellaneous code quality issues.

9d393a2

Issue: AAH-278

Fix bugs in permission checking and my_permissions.

fbdfe81

Issue: AAH-278

newswangerd force-pushed the feature/AAH-278-pulp-container-rbac branch from 3cd2dfe to fbdfe81 Compare April 9, 2021 13:40

Switch access policy loading to post migration hook.

2ea72a3

Issue: AAH-278

newswangerd force-pushed the feature/AAH-278-pulp-container-rbac branch from 0190aaa to 2ea72a3 Compare April 9, 2021 17:50

newswangerd merged commit d974f6a into ansible:master Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pulp container RBAC #705

Pulp container RBAC #705

newswangerd commented Mar 29, 2021 •

edited

alikins Mar 29, 2021

alikins Mar 29, 2021

alikins Mar 29, 2021

newswangerd Mar 31, 2021

alikins left a comment

alikins Mar 31, 2021

alikins Mar 31, 2021

newswangerd Mar 31, 2021

alikins Mar 31, 2021

alikins left a comment

mdellweg commented Apr 1, 2021

ipanova Apr 7, 2021

newswangerd Apr 9, 2021

ipanova Apr 9, 2021

ipanova Apr 7, 2021

Pulp container RBAC #705

Pulp container RBAC #705

Conversation

newswangerd commented Mar 29, 2021 • edited

Changes

pulp_container access policies

Groups

Container Repo endpoint

Additions

Container namespace endpoint

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alikins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alikins left a comment

Choose a reason for hiding this comment

mdellweg commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newswangerd commented Mar 29, 2021 •

edited