Rest interface to manage roles on objects #1693

mdellweg · 2021-10-21T11:12:02Z

This is based on #1627 .

pulpcore/app/role_util.py

newswangerd · 2021-11-12T19:49:40Z

pulpcore/app/role_util.py

+    return qs.distinct()
+
+
+def get_groups_with_perms_roles(obj, attach_perms=False, only_with_perms_in=None):


Can we get a version of this that has attach_roles? I'd like to know what roles a group has on an object.

I dislike the fact, that by switching the a flag, your functions return type changes. It is here in that way only because guardian introduced this interface. I think, i am going to make a second function for this.

+1 to putting this in a different function

This is what I'm using right now: https://github.com/ansible/galaxy_ng/pull/1057/files#diff-f8da9f185c5feab360dce962eae8b91d337ebf9210018e5a5c5b629202be2ed9R11

Oh, that one is different. That one is attaching the role names. This here is attaching the corresponding permissions (to match the guardian interface). Maybe both are interesting to us.

Do the get_{users,groups}_with_perms_attached_{perms, roles} serve your needs?

newswangerd · 2021-11-12T19:56:31Z

pulpcore/app/role_util.py

+from pulpcore.app.models.role import GroupRole, Role, UserRole
+
+User = get_user_model()
+


Could you add aget_perms_for_model that returns a list of permissions for a model (should be pretty easy: https://stackoverflow.com/questions/38391729/how-to-retrieve-all-permissions-of-a-specific-model-in-django)?

How would you return the Permissions? Would a QuerySet be OK?

@mdellweg a queryset is fine. This is one of the guardian functions that we use. https://django-guardian.readthedocs.io/en/stable/api/guardian.shortcuts.html#get-perms-for-model

Should be readily available now.

newswangerd · 2021-11-15T19:18:43Z

pulpcore/app/role_util.py

+User = get_user_model()
+
+
+def assign_role(rolename, entity, obj=None):


I've noticed that this allows setting roles on objects that don't have any permissions related to the object. For example, I can create a role with perms = [galaxy.change_namespace] to a pulp container repository, which doesn't make any sense.

It might be a good idea for this function to either fail if none of the permissions are relevant to the object.

it raised an exception now.

newswangerd · 2021-11-15T19:25:49Z

pulpcore/app/viewsets/user.py

@@ -440,3 +475,157 @@ def destroy(self, request, group_pk, pk):
        group.user_set.remove(user)
        group.save()
        return Response(status=status.HTTP_204_NO_CONTENT)
+
+
+class RoleFilter(BaseFilterSet):


I'd like to request a filter where we can specify a list of permissions and return any role that has at least one permission in the list.

Thinking forward to the UI changes we'll need to make, our UI is going to have to query the API for a list of roles whenever a user assigns a role to a specific object, and the user experience will be pretty bad if the UI has to display all the roles in the system.

If you can express this as a filter() expression on the Role.objects manager, i can turn it into a drf_filter. I just don't see it from the top of my head.
Querying all roles with a single permission should be manageble somehow.

newswangerd · 2021-11-15T20:16:39Z

pulpcore/app/role_util.py

+    return qs.filter(Q(pk__in=user_role_pks) | Q(pk__in=group_role_pks))
+
+
+def get_objects_for_user(user, permission_name, qs):


Looks like we need get_objects_for_group too (https://django-guardian.readthedocs.io/en/stable/api/guardian.shortcuts.html#get-objects-for-group)

implemented now.

newswangerd · 2021-11-15T20:20:58Z

Here's the PoC so far for removing guardian: ansible/galaxy_ng#1057

So far everything is working. We're just missing get_objects_for_group

Required PR: pulp/pulpcore#1693 [noissue]

bmbouter · 2021-12-02T16:51:41Z

pulpcore/app/viewsets/base.py

+class RolesMixin:
+    @extend_schema(description="List user roles assigned to this object.")
+    @action(detail=True, methods=["get"], serializer_class=NestedUserRoleSerializer)
+    def list_user_roles(self, request, pk):


Since we're adding groups I imagined the API call names would be:

add_roles
remove_roles
list_roles
my_permissions

bmbouter · 2021-12-03T17:35:37Z

One thing I realized as I wrote this story was that we need to use these interfaces for managing role assignment to both objects directly and "model-level" roles. So the 4 methods in RoleMixin need to be callable both on specific objects for detail views and also non-detail views for model-level roles. See the story for more details on this, and let me know what you think about this also.

mdellweg · 2021-12-03T18:23:00Z

One thing I realized as I wrote this story was that we need to use these interfaces for managing role assignment to both objects directly and "model-level" roles. So the 4 methods in RoleMixin need to be callable both on specific objects for detail views and also non-detail views for model-level roles. See the story for more details on this, and let me know what you think about this also.

The module level roles is covered by the corresponding viewsets nested under user and groups. (They are obviously admin interfaces.)

pulpbot · 2021-12-06T16:10:10Z

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

bmbouter

Overall this is looking good to me. I left one little nitpick question about the name of a role. I ran this and the openAPI schema it produces looks good too. Thanks!

goosemania

I left some non-blocking comments. Up to you whether they are of any concern.
Thanks for your work!

goosemania · 2021-12-08T20:00:42Z

pulpcore/plugin/viewsets/__init__.py

@@ -4,6 +4,7 @@
 # Import Viewsets in platform that are potentially useful to plugin writers
 from pulpcore.app.viewsets import (  # noqa
    AsyncUpdateMixin,
+    RolesMixin,


The list of imports is in alphabetic order.

Then AlternateContentSourceViewSet should have gone first. I figured Mixins in alphabetical order, then viewsets.

it should have been first, yes

:'<,'>!sort
There were more inconsistencies. 😆

goosemania · 2021-12-08T20:06:59Z

pulpcore/app/viewsets/base.py

+            UserRole.objects.bulk_create(
+                [
+                    UserRole(
+                        content_object=serializer.validated_data["content_object"],
+                        user=user,
+                        role=serializer.validated_data["role"],
+                    )
+                    for user in serializer.validated_data["users"]
+                ]
+            )


If I read it correctly, if I try to add the same role for the same object, this bulk create will fail. Am I right? Is it intentional? Should we use ignore_duplicates=True?

Is there a chance for a race if we rely here on the check in the serializer?

Failing is purposeful. You should not create a roles association that already exists.
Can you explain more what you expect / how you see it fail?

I think so, there is a race if both reach the serializer at the same time, one will succeed in the bulk create and the other will 500 error.

Yup, 500 error is the one I'm concerned about.

I think this is a common thing in the whole pulpcore codebase.

I did git grep bulk_create in pulpcore. One potentially racy place is in artifact_stages when remote artifacts are being created, the rest of bulk_create usage is done in a safe manner, imo. JFYI, I'm fine if it's not taken care of in this PR, probability of that race is low, hard to imagine a concurrent role addition to the same object.

Alas, the 500 is not particularly nice to users, but with the transaction, i added now, at least it's either success, or nothing happened, and the user will probably retry immediately.

pulpcore/app/viewsets/base.py

pulpcore/app/migrations/0082_add_manage_roles_permissions.py

CHANGES/9604.feature

CHANGES/plugin_api/9604.feature

pulpcore/app/serializers/user.py

gerrod3 · 2021-12-08T22:31:13Z

pulpcore/app/serializers/user.py

+            for user in data["users"]:
+                qs = UserRole.objects.filter(
+                    content_type_id=obj_type.id, object_id=obj.pk, user=user, role=data["role"]
+                )


Mental note that this is an area for improvement if we find that our users are supplying this endpoint with hundreds of users or groups.

pulpcore/app/viewsets/base.py

gerrod3 · 2021-12-08T22:43:03Z

pulpcore/app/viewsets/base.py

+            if group_role.role.name not in roles:
+                roles[group_role.role.name] = {
+                    "role": user_role.role.name,
+                    "users": [],
+                    "groups": [],
+                }


Suggested change

if group_role.role.name not in roles:

roles[group_role.role.name] = {

"role": user_role.role.name,

"users": [],

"groups": [],

}

if group_role.role.name not in roles:

roles[group_role.role.name] = {

"role": group_role.role.name,

"users": [],

"groups": [],

}

I wonder if this could be shorten with a defaultdict. eg.

roles = defaultdict(lambda: dict(users=list(), groups=list())) for user_role in user_qs: roles[user_role.role.name]["users"].append(user_role.user.username) for group_role in group_qs: roles[group_role.role.name]["groups"].append(group_role.group.name) result = {"roles": [dict(users_and_groups, role=role) for role, users_and_groups in roles.items()]}

If the default dict could inject the new key in the lambda, that would be cool. Also i am a bit weary this may reuse one of the dicts or lists from your lambda.
I'm on the fence.

pulpcore/app/serializers/user.py

gerrod3 · 2021-12-08T23:06:52Z

pulpcore/app/viewsets/base.py

+            UserRole.objects.bulk_create(
+                [
+                    UserRole(
+                        content_object=serializer.validated_data["content_object"],
+                        user=user,
+                        role=serializer.validated_data["role"],
+                    )
+                    for user in serializer.validated_data["users"]
+                ]
+            )


I think so, there is a race if both reach the serializer at the same time, one will succeed in the bulk create and the other will 500 error.

pulpcore/app/viewsets/base.py

This mixin provides endpoints to assign and revoke roles on objects to users or groups. fixes #9604

[noissue]

re #9604

mdellweg force-pushed the roles_rest branch from aad1b0c to 2556bbf Compare October 21, 2021 11:59

mdellweg force-pushed the roles_rest branch 2 times, most recently from 41b8627 to 6bad367 Compare November 9, 2021 12:25

mdellweg force-pushed the roles_rest branch 10 times, most recently from 789efc5 to a24e479 Compare November 12, 2021 10:58

newswangerd reviewed Nov 12, 2021

View reviewed changes

pulpcore/app/role_util.py Outdated Show resolved Hide resolved

newswangerd reviewed Nov 12, 2021

View reviewed changes

mdellweg force-pushed the roles_rest branch 2 times, most recently from 365776b to 2967765 Compare November 15, 2021 13:45

mdellweg marked this pull request as ready for review November 15, 2021 13:46

newswangerd reviewed Nov 15, 2021

View reviewed changes

mdellweg force-pushed the roles_rest branch from 2967765 to c05d670 Compare November 16, 2021 09:36

mdellweg changed the title ~~WIP/RFC Rest interface to manage roles on objects~~ RFC Rest interface to manage roles on objects Nov 16, 2021

mdellweg force-pushed the roles_rest branch 3 times, most recently from 60b8037 to 2b22993 Compare November 16, 2021 13:12

mdellweg added a commit to mdellweg/pulp_container that referenced this pull request Nov 17, 2021

WIP/PoC Roles support

9b4a9c1

Required PR: pulp/pulpcore#1693 [noissue]

mdellweg added a commit to mdellweg/pulp_container that referenced this pull request Nov 17, 2021

WIP/PoC Roles support

09d815b

Required PR: pulp/pulpcore#1693 [noissue]

mdellweg force-pushed the roles_rest branch from 450fa18 to 7d02d03 Compare December 1, 2021 17:59

mdellweg changed the title ~~RFC Rest interface to manage roles on objects~~ Rest interface to manage roles on objects Dec 2, 2021

bmbouter reviewed Dec 2, 2021

View reviewed changes

mdellweg force-pushed the roles_rest branch 2 times, most recently from bc4581e to a35dc18 Compare December 3, 2021 17:05

mdellweg force-pushed the roles_rest branch from a35dc18 to 2107c76 Compare December 6, 2021 16:06

pulp deleted a comment from pulpbot Dec 6, 2021

mdellweg force-pushed the roles_rest branch 2 times, most recently from 22cb46d to 8223177 Compare December 6, 2021 16:29

bmbouter approved these changes Dec 7, 2021

View reviewed changes

mdellweg force-pushed the roles_rest branch 2 times, most recently from ea0d712 to cad5d66 Compare December 8, 2021 18:38

goosemania approved these changes Dec 8, 2021

View reviewed changes

gerrod3 reviewed Dec 8, 2021

View reviewed changes

mdellweg force-pushed the roles_rest branch 4 times, most recently from 77934bc to fbcbb3c Compare December 9, 2021 13:35

gerrod3 reviewed Dec 9, 2021

View reviewed changes

pulpcore/app/viewsets/base.py Outdated Show resolved Hide resolved

gerrod3 reviewed Dec 9, 2021

View reviewed changes

pulpcore/app/viewsets/base.py Outdated Show resolved Hide resolved

mdellweg added 3 commits December 9, 2021 15:39

Add RolesMixin for viewsets

3d06887

This mixin provides endpoints to assign and revoke roles on objects to users or groups. fixes #9604

Add RolesMixin to task view set

4c619b8

[noissue]

Add RolesMixin to group view set

07a20d0

re #9604

mdellweg force-pushed the roles_rest branch from fbcbb3c to 07a20d0 Compare December 9, 2021 14:39

gerrod3 approved these changes Dec 9, 2021

View reviewed changes

mdellweg merged commit 1e8203f into pulp:main Dec 9, 2021

mdellweg deleted the roles_rest branch December 9, 2021 15:22

		return qs.distinct()


		def get_groups_with_perms_roles(obj, attach_perms=False, only_with_perms_in=None):

		from pulpcore.app.models.role import GroupRole, Role, UserRole

		User = get_user_model()

		User = get_user_model()


		def assign_role(rolename, entity, obj=None):

		return qs.filter(Q(pk__in=user_role_pks) \| Q(pk__in=group_role_pks))


		def get_objects_for_user(user, permission_name, qs):

Rest interface to manage roles on objects #1693

Rest interface to manage roles on objects #1693

Conversation

mdellweg commented Oct 21, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newswangerd commented Nov 15, 2021

Choose a reason for hiding this comment

bmbouter commented Dec 3, 2021

mdellweg commented Dec 3, 2021

pulpbot commented Dec 6, 2021

bmbouter left a comment

Choose a reason for hiding this comment

goosemania 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

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

Choose a reason for hiding this comment

goosemania Dec 9, 2021 • edited Loading

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

gerrod3 Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosemania Dec 9, 2021 •

edited

Loading

gerrod3 Dec 8, 2021 •

edited

Loading