Adds granular permissions for push repositories #207

dkliban · 2021-01-18T22:42:25Z

The RegistryAccessPolicy is a combination of AccessPolicies for ContainerNamespace,
ContainerDistribution, and ContainerPushRepository viewsets.

The default policy requires users to have "container.add_containernamespace" and
"container.add_containerdistribution" permissions in order to push a new repository
into a new namespace. The permissions also enable user to use the Pulp API to create
a namespace and distribution.

The ContainerNamespaceViewset provides a create_namespace_group() function that can
be used to assign permissions to a group of users. The default permission assignment
for a namespace creates three groups Owners, Collaborators, and Consumers.

Namespace Owners get the following permissions:

     "container.view_containernamespace",
     "container.delete_containernamespace",
     "container.namespace_add_containerdistribution",
     "container.namespace_delete_containerdistribution",
     "container.namespace_view_containerdistribution",
     "container.namespace_pull_containerdistribution",
     "container.namespace_push_containerdistribution",
     "container.namespace_change_containerdistribution",
     "container.namespace_view_containerpushrepository",

Namespace Collaborators get the following permissions:

     "container.view_containernamespace",
     "container.namespace_add_containerdistribution",
     "container.namespace_delete_containerdistribution",
     "container.namespace_view_containerdistribution",
     "container.namespace_pull_containerdistribution",
     "container.namespace_push_containerdistribution",
     "container.namespace_change_containerdistribution",
     "container.namespace_view_containerpushrepository",

Namespace Consumers get the following permissions:

     "container.view_containernamespace",
     "container.namespace_view_containerdistribution",
     "container.namespace_pull_containerdistribution",
     "container.namespace_view_containerpushrepository",

The ContainerDistributionsViewset provides a create_distribution_group() function
that can be used to assign permissions to a group of users. The default permission
assignment for a newly created ContainerDistribution consists of three groups:
Owners, Collaborators, and Consumers.

ContainerDistribution Owners get the following permissions:

     "container.view_containerdistribution",
     "container.pull_containerdistribution",
     "container.push_containerdistribution",
     "container.delete_containerdistribution",
     "container.change_containerdistribution",
     "container.view_containerpushrepository",

ContainerDistribution Collaborators get the following permissions:

     "container.view_containerdistribution",
     "container.pull_containerdistribution",
     "container.push_containerdistribution",
     "container.view_containerpushrepository",

ContainerDistribution Consumers get the following permissions:

     "container.view_containerdistribution",
     "container.pull_containerdistribution",
     "container.view_containerpushrepository",

The ContainerPushRepositoryViewset provides an add_perms_to_distribution_group()
function to assign ContainerPushRepository permissions to the groups associated
with the ContainerDistribution that serves the specific ContainerPushRepository.

closes: #8101
https://pulp.plan.io/issues/8101

pulpbot · 2021-01-18T23:06:55Z

Attached issue: https://pulp.plan.io/issues/8101

goosemania · 2021-01-19T09:24:23Z

pulp_container/app/models.py

@@ -186,7 +189,39 @@ class Meta:
        unique_together = (("name", "tagged_manifest"),)


-class ContainerNamespace(BaseModel, AutoAddObjPermsMixin, AutoDeleteObjPermsMixin):
+class CreateGroupAndAddPermissionsMixin(AutoAddObjPermsMixin):


Nice!
Should we have the same for group removal? To ensure that no perms are inherited from the removed objects with the same field value?
It's critical for namespaces if we use its name and not pk.

+1 it would be good to remove groups and perms whenever namespace/distribution also gets deleted.

We should. I only considered the removal of the permissions which already occurs. However, the groups themselves need to be removed also. I'll add another mixin for that.

mdellweg · 2021-01-27T13:00:30Z

pulp_container/app/access_policy.py

-            # check model permissions for namespace creation
-            return user.has_perm("container.add_containernamespace")
+            # Anyone can create a new namespace
+            return True


Na, we don't want that. If anyone should be able to create a new namespace, than the access_policy needs to reflect that.

Yeah, I'd appreciate some background behind the decision of allowing creation of namespaces by anyone.

mdellweg · 2021-01-27T13:03:11Z

pulp_container/app/models.py

+        """Delete all groups associated with this object and user object permissions."""
+
+        group_pks = [o.group_id for o in GroupObjectPermission.objects.filter(object_pk=self.pk)]
+        Group.objects.filter(pk__in=group_pks).delete()


This will delete all groups with any permission on that object. Even the ones not created by us.

I'll add better filtering for this

How about deleting only groups which are created automatically by us? all the default groups: owner, collaborator, consumer?
I'm not sure we want to touch groups created by the users, even if the only permission which is left is for the object which just got removed.
Imagine I have a group of users, teamA, and I give them only read access to a specific object. When this object is removed, they no longer have access to anything but that's fine, I don't want to recreate the group and add all the users again, I just want to give them new permissions eventually.

I agree, let's auto remove only groups we have created. Rest of the groups would need to be deleted by the owner of the groups 'manually'

mdellweg · 2021-01-27T13:04:39Z

pulp_container/app/models.py

+            ("namespace_view_containerdistribution", "View a distribution in a namespace"),
+            ("namespace_pull_containerdistribution", "Pull from all distributions in a namespace"),
+            ("namespace_push_containerdistribution", "Push to all distributions in a namespace"),
+            ("namespace_change_containerdistribution", "Change all distributions in a namespace"),


unfortunately, it's the maximum recursive depth for the thumb-ups 😞

dkliban · 2021-01-28T21:03:13Z

pulp_container/app/viewsets.py

@@ -587,6 +589,29 @@ class ContainerPushRepositoryViewSet(ReadOnlyRepositoryViewSet):
        ],
    }

+    def get_queryset(self):


I had to add custom filtering for the repositories so that we could consider permissions on the namespace and the private field on the distribution. We will need to do something similar for the distributions viewset.

+1 not necessarily part of this pr, but commenting now for the sake of capturing a TODO

i filed an issue https://pulp.plan.io/issues/8206

dkliban · 2021-01-29T03:38:39Z

@ipanova @mdellweg @goosemania Even though there are still some test failures, the functionality we discussed is there.

I discovered that during a push the hook for ContainerPushRepositories is called before the on create hook for ContainerDistributions so the logic in the group permission assignment is a bit weird.

Please provide feedback on how to improve this.

goosemania · 2021-01-29T09:46:43Z

pulp_container/app/models.py

@@ -239,7 +326,9 @@ def finalize_new_version(self, new_version):
        validate_repo_version(new_version)


-class ContainerPushRepository(Repository, AutoAddObjPermsMixin, AutoDeleteObjPermsMixin):
+class ContainerPushRepository(
+    Repository, CreateGroupAndAddPermissionsMixin, AutoDeleteObjPermsMixin


We are not deleting groups here because all groups are related to the ContainerDistribution, is my understanding correct?

goosemania · 2021-01-29T09:53:04Z

pulp_container/app/models.py

@@ -358,7 +447,7 @@ class Meta:


 class ContainerDistribution(
-    RepositoryVersionDistribution, AutoAddObjPermsMixin, AutoDeleteObjPermsMixin
+    RepositoryVersionDistribution, CreateGroupAndAddPermissionsMixin, AutoDeleteGroupsMixin


Should we keep AutoAddObjPermsMixin, AutoDeleteObjPermsMixin everywhere as well?
So permission auto-assignment works for users who change the access policy.
And even without a change to access policy the permissions are cleaned properly from the groups created and assigned to by a user.
What do you think?

Can 2 hooks of the same type be performed? (2 before_delete)?

ipanova · 2021-01-29T11:24:38Z

@ipanova @mdellweg @goosemania Even though there are still some test failures, the functionality we discussed is there.

I discovered that during a push the hook for ContainerPushRepositories is called before the on create hook for ContainerDistributions so the logic in the group permission assignment is a bit weird.

Please provide feedback on how to improve this.

We can try flip the order here https://github.com/pulp/pulp_container/blob/master/pulp_container/app/registry_api.py#L284 if that won't help we could try having nested transaction where we'd be able to control the order of execution

ipanova · 2021-02-01T12:14:36Z

pulp_container/app/models.py

+
+        The parameters are specified as a dictionary with the following keys:
+
+        "name_prefix" - the first part of the group name


group_type is missing

ipanova · 2021-02-01T12:16:03Z

pulp_container/app/authorization.py

            )
        # this is an existing distribution. User should have model or object perms
-        if self.user.has_perm("container.push_containerdistribution") or self.user.has_perm(
-            "container.push_containerdistribution", distribution
+        if (


we should probably add model perms of container.namespace_push_containerdistribution to this logical statement

mdellweg · 2021-02-02T13:27:06Z

pulp_container/app/viewsets.py

-                "condition": [
-                    "has_model_perms:container.add_containerdistribution",
-                    "has_manage_namespace_dist_perms:container.manage_namespace_distributions",
-                ],


Having no condition here, means every authenticated user can create whatever not yet existing distribution (even in existing namespaces).
It should be
(namespace does not exist) or (user.has_namespace_perm(create_distribution))

I added the condition back

mdellweg · 2021-02-02T13:56:55Z

pulp_container/app/access_policy.py


    def obj_exists(self, request, view, action):
        """
        Check if the distribution exists.
        """
        return view.get_object() is not None

-    def is_private(self, request, view, action):
+    def is_public(self, request, view, action):


I think this is very confusing, that you need to set the private flag on the distribution to trigger this is_public check.

mdellweg · 2021-02-03T10:14:11Z

pulp_container/tests/unit/test_serializers.py

+
+    def tearDown(self):
+        """Delete the user."""
+        super(TestContainerDistributionSerializer, self).tearDown()


Suggested change

super(TestContainerDistributionSerializer, self).tearDown()

super().tearDown()

Welcome to world of python 3! ;)

mdellweg · 2021-02-03T10:15:45Z

pulp_container/tests/functional/utils.py

+        .list(name="container.namespace.{}.{}".format(group_type, namespace_name))
+        .results[0]
+    )
+    as_user["group_users_api"].create(


This requires RBAC for the group endpoints, right?

yes it does

ipanova · 2021-02-03T12:02:21Z

The RegistryAccessPolicy is a combination of AccessPolicies for ContainerNamespace,
ContainerDistribution, and ContainerPushRepository viewsets.

The default policy requires users to have "container.add_containernamespace" and
"container.add_containerdistribution" permissions in order to push a new repository
into a new namespace. The permissions also enable user to use the Pulp API to create
a namespace and distribution.

The ContainerNamespaceViewset provides a create_namespace_group() function that can
be used to assign permissions to a group of users. The default permission assignment
for a namespace creates three groups Owners, Collaborators, and Consumers.

Namespace Owners get the following permissions:

     "container.view_containernamespace",
     "container.delete_containernamespace",
     "container.namespace_add_containerdistribution",
     "container.namespace_delete_containerdistribution",
     "container.namespace_view_containerdistribution",
     "container.namespace_pull_containerdistribution",
     "container.namespace_push_containerdistribution",
     "container.namespace_change_containerdistribution",
     "container.namespace_view_containerpushrepository",

Namespace Collaborators get the following permissions:

     "container.view_containernamespace",
     "container.namespace_add_containerdistribution",
     "container.namespace_delete_containerdistribution",
     "container.namespace_view_containerdistribution",
     "container.namespace_pull_containerdistribution",
     "container.namespace_push_containerdistribution",
     "container.namespace_change_containerdistribution",
     "container.namespace_view_containerpushrepository",

Namespace Consumers get the following permissions:

     "container.view_containernamespace",
     "container.namespace_view_containerdistribution",
     "container.namespace_pull_containerdistribution",
     "container.namespace_view_containerpushrepository",

The ContainerDistributionsViewset provides a create_distribution_group() function
that can be used to assign permissions to a group of users. The default permission
assignment for a newly created ContainerDistribution consists of three groups:
Owners, Collaborators, and Consumers.

ContainerDistribution Owners get the following permissions:

     "container.view_containerdistribution",
     "container.pull_containerdistribution",
     "container.push_containerdistribution",
     "container.delete_containerdistribution",
     "container.change_containerdistribution",
     "container.view_containerpushrepository",

ContainerDistribution Collaborators get the following permissions:

     "container.view_containerdistribution",
     "container.pull_containerdistribution",
     "container.push_containerdistribution",
     "container.view_containerpushrepository",

ContainerDistribution Consumers get the following permissions:

     "container.view_containerdistribution",
     "container.pull_containerdistribution",
     "container.view_containerpushrepository",

The ContainerPushRepositoryViewset provides an add_perms_to_distribution_group()
function to assign ContainerPushRepository permissions to the groups associated
with the ContainerDistribution that serves the specific ContainerPushRepository.

closes: #8101
https://pulp.plan.io/issues/8101

Thank you for summarizing, this is very helpful! we should definitely capture this in our docs.
Couple of questions/suggestions:
Namespace Owners and Namespace Collaborators should also get container.namespace_modify_content_containerpushrepository otherwise they would not be able to tag/untag repos

Having container.view_containerpushrepository on the distribution viewset. how does that work? since this permission is defined on the push repository model and you can;t really re-use on another model

dkliban · 2021-02-03T14:24:22Z

@ipanova The logic for adding container.view_containerpushrepository permission is a bit confusing in the code. I actually now think that I should add another method to the ContainerDistribution model that will accept ContainerPushRepository related permissions separately. Right now the create_distribution_group() method on the ContainerDistribution model checks if the permission name ends with containerpushrepository and separates those permissions into a list of their own. Then it checks if there is a ContainerPushRepository associated with the distribution. If there is, it adds the permissions for that object.

pulp_container/app/access_policy.py

pulp_container/app/authorization.py

pulp_container/app/migrations/0017_add_granular_perms.py

pulp_container/app/models.py

pulp_container/app/viewsets.py

ipanova · 2021-02-04T10:58:48Z

pulp_container/app/access_policy.py

@@ -21,8 +21,10 @@ def has_namespace_obj_perms(self, request, view, action, permission):
        if type(obj) == models.ContainerDistribution:
            namespace = obj.namespace
        else:
-            distribution = models.ContainerDistribution.objects.get(repository=obj)
-            namespace = distribution.namespace
+            dists_qs = models.ContainerDistribution.objects.filter(repository=obj)


def has_namespace_obj_perms(self, request, view, action, permission): """ Check if a user has object-level perms on the namespace associated with the distribution or repository. """ obj = view.get_object() if type(obj) == models.ContainerDistribution: namespace = obj.namespace return request.user.has_perm(permission, namespace) else: dists_qs = models.ContainerDistribution.objects.filter(repository=obj) for dist in dists_qs: if request.user.has_perm(permission, dist.namespace): return True return False

ipanova · 2021-02-04T11:18:23Z

pulp_container/tests/functional/api/test_push_content.py

@@ -184,26 +194,6 @@ def test_push_with_view_perms(self):
        with self.assertRaises(exceptions.CalledProcessError):
            self._push(image_path, local_url, self.user_reader)

-    def test_push_with_no_perms(self):


why this test got removed?

ipanova · 2021-02-04T13:55:16Z

@dkliban this looks good to me! thank you forgetting this big chunk of work done!

The RegistryAccessPolicy is a combination of AccessPolicies for ContainerNamespace, ContainerDistribution, and ContainerPushRepository viewsets. The default policy requires users to have "container.add_containernamespace" and "container.add_containerdistribution" permissions in order to push a new repository into a new namespace. The permissions also enable user to use the Pulp API to create a namespace and distribution. The ContainerNamespaceViewset provides a create_namespace_group() function that can be used to assign permissions to a group of users. The default permission assignment for a namespace creates three groups Owners, Collaborators, and Consumers. Namespace Owners get the following permissions: "container.view_containernamespace", "container.delete_containernamespace", "container.namespace_add_containerdistribution", "container.namespace_delete_containerdistribution", "container.namespace_view_containerdistribution", "container.namespace_pull_containerdistribution", "container.namespace_push_containerdistribution", "container.namespace_change_containerdistribution", "container.namespace_view_containerpushrepository", "container.namespace_modify_content_containerpushrepository" Namespace Collaborators get the following permissions: "container.view_containernamespace", "container.namespace_add_containerdistribution", "container.namespace_delete_containerdistribution", "container.namespace_view_containerdistribution", "container.namespace_pull_containerdistribution", "container.namespace_push_containerdistribution", "container.namespace_change_containerdistribution", "container.namespace_view_containerpushrepository", "container.namespace_modify_content_containerpushrepository" Namespace Consumers get the following permissions: "container.view_containernamespace", "container.namespace_view_containerdistribution", "container.namespace_pull_containerdistribution", "container.namespace_view_containerpushrepository", The ContainerDistributionsViewset provides a create_distribution_group() function that can be used to assign permissions to a group of users. The default permission assignment for a newly created ContainerDistribution consists of three groups: Owners, Collaborators, and Consumers. ContainerDistribution Owners get the following permissions: "container.view_containerdistribution", "container.pull_containerdistribution", "container.push_containerdistribution", "container.delete_containerdistribution", "container.change_containerdistribution", "container.view_containerpushrepository", "container.modify_content_containerpushrepository" ContainerDistribution Collaborators get the following permissions: "container.view_containerdistribution", "container.pull_containerdistribution", "container.push_containerdistribution", "container.view_containerpushrepository", "container.modify_content_containerpushrepository" ContainerDistribution Consumers get the following permissions: "container.view_containerdistribution", "container.pull_containerdistribution", "container.view_containerpushrepository", The ContainerPushRepositoryViewset provides an add_perms_to_distribution_group() function to assign ContainerPushRepository permissions to the groups associated with the ContainerDistribution that serves the specific ContainerPushRepository. closes: #8101 https://pulp.plan.io/issues/8101

dkliban changed the base branch from master to rbac January 18, 2021 22:42

dkliban force-pushed the 8101 branch from 7810435 to 7896fcf Compare January 18, 2021 22:45

goosemania reviewed Jan 19, 2021

View reviewed changes

dkliban force-pushed the 8101 branch 6 times, most recently from 27228db to 00246f6 Compare January 27, 2021 01:44

mdellweg reviewed Jan 27, 2021

View reviewed changes

dkliban force-pushed the 8101 branch 4 times, most recently from 7157f81 to 7286aa0 Compare January 28, 2021 20:58

dkliban commented Jan 28, 2021

View reviewed changes

dkliban force-pushed the 8101 branch 3 times, most recently from 5744c5f to acd159e Compare January 29, 2021 03:20

goosemania reviewed Jan 29, 2021

View reviewed changes

ipanova reviewed Feb 1, 2021

View reviewed changes

dkliban force-pushed the 8101 branch 3 times, most recently from 011bf7f to 10fd005 Compare February 1, 2021 22:44

mdellweg reviewed Feb 2, 2021

View reviewed changes

dkliban force-pushed the 8101 branch 4 times, most recently from 851cfdd to a6a3cdd Compare February 3, 2021 02:26

dkliban changed the title ~~Adds Groups with object level permissions for Namespaces and Distributions~~ Adds granular permissions for push repositories Feb 3, 2021

mdellweg reviewed Feb 3, 2021

View reviewed changes

dkliban force-pushed the 8101 branch 2 times, most recently from 1b08084 to cd112a2 Compare February 3, 2021 14:17

dkliban force-pushed the 8101 branch from bea4768 to f84e277 Compare February 3, 2021 17:05