Manage roles and permissions via the django admin UserAdmin Groups and Permissions. #72

powderflask · 2017-09-20T21:05:19Z

An alternate patch for #71
This patch abstracts the logic for creating permissions so it can be shared in both places that is done.
It also tries to create a human-friendly name from the permission codename.
The tests pass in all tox environments from py27-django17 on. Test for earlier tox envs seem to fail for unrelated reasons.

See: vintasoftware#71

powderflask · 2017-09-20T21:09:34Z

I think this patch also resolves issue #46, although in a simplified form from what was suggested by @filipeximenes
Ideally, I think this feature is part of larger feature-set to make role-permission play nicer with django admin in general, as discussed in #66, but I wanted to submit it as a separate patch in case it is more useful as stand-alone.

Add sync_roles management command to sync auth.Group to UserRoles

powderflask · 2017-09-21T17:30:36Z

This PR now introduces 3 related features that allow role-permissions to play nice with the django UserAdmin:

Add permission labels (permission.name) when permissions are created. Fixes [Enhancement] Add name value to Permissions object creation in AbstractUserRole. #46 and [WIP] Prevent empty name attributes for Permission objects #71
Optional custom UserAdmin that syncs user's roles with their Groups on save. This allows you to add / remove roles, while maintaining non-role groups, via the django admin. Fixes Add an admin widget to manage Roles #66
Management command to sync auth.Group and auth.Permissions with the roles specified in ROLEPERMISSIONS_MODULE. This give developer a quick easy way to push permission changes made in code out to the DB, and make new Roles immediately available as Groups in the UserAdmin.

(1) does not change API - it simply uses the permission codename to formulate a human-friendly label.
(2) and (3) are "opt-in" features -- they have no impact on existing installs.

Tests included for all 3 features.
Some documentation will be required for (2) and (3) -- I will take a stab at that if/when that's required.

filipeximenes · 2017-10-02T20:13:49Z

rolepermissions/management/commands/sync_roles.py

+    help = "Synchronize auth Groups and Permissions with UserRoles defined in %s."%ROLEPERMISSIONS_MODULE
+
+    def handle(self, *args, **options):
+        # Sync auth.Group with current registered roles (leaving existing groups intact!)


filipeximenes · 2017-10-02T20:17:28Z

rolepermissions/management/commands/sync_roles.py

+            role.get_default_true_permissions()
+
+        # Push any permission changes made to roles and remove any unregistered roles from all auth.Users
+        for user in UserModel.objects.all():


This is dangerous. Id rather have this as an example script in the documentation. This is something you only want to run if you are absolutely sure about what the script is doing. It could go right after the docs for the command. What do you think?

Sure. I think you are just talking about the second part that syncs User permissions, right? Creating a Group for each defined Role so they are immediately available in the Admin seems harmless?

Yes, the first part is cool. Only worried about the second (from line 22 on).

I pulled out the second part, made it an optional parameter:

sync_roles --reset_user_permissions

Willing to document this clearly.

filipeximenes · 2017-10-02T20:20:56Z

rolepermissions/roles.py

+    """Get a Permission object from a permission name."""
+    user_ct = ContentType.objects.get_for_model(get_user_model())
+    return Permission.objects.get_or_create(
+            content_type=user_ct, codename=permission_name, name=camel_or_snake_to_title(permission_name))


I think name would be better as a defaults parameter. This way it would be possible to change it manually after creation without the risk of breaking the functionality.

Can you also write a test for this behavior?

Oh! That's such a good idea! thanks.
Yes - sorry, didn't see that was missing.

I see -- this code would cause problems when looking up existing permissions whose name were set by some other method (or not at all, e.g.) Will fix and add test cases for backwards compat.

filipeximenes · 2017-10-02T20:22:53Z

rolepermissions/utils.py

+def snake_to_title(s) :
+    return ' '.join(x.capitalize() for x in s.split('_'))
+
+def camel_or_snake_to_title(s):


Can you provide some tests for those functions?

Yes. Good catch.

filipeximenes · 2017-10-02T20:31:46Z

rolepermissions/admin.py

+        # re-load and take a copy of user's newly saved roles
+        user = UserModel.objects.get(pk=form.instance.pk)
+        groups = list(user.groups.all())
+        # reset user's roles to match the list of groups just saved


This is dangerous because people might be managing permissions regardless of user roles. So this will make those individual changes to be lost. This is rather a "role reset" rather than a simple "sync". Would it be possible to overwrite the adding and removing of groups and replace them with assign and remove role functions?

Also, this will need a very descriptive documentation so people understand what is going on under the hood :)

I agree, some good docs are required for all 3 features in this PR :-)

The problem this feature is trying to address is:

Edit / Create User in Admin; Assign the User to a Group

Group Permissions (defined by Role in code) are not assigned to the User.

I have very limited experience with the use-cases for rolepermissions, but for my use-case, this is important. I'm not clear what you mean by

"Would it be possible to overwrite the adding and removing of groups and replace them with assign and remove role functions?"

Doesn't the code below use the roles functions? I'm happy to re-write this in any way that makes sense.

Got it. Ok, so despite the ability to manage user permissions by assigning a role to a user, it's also possible to manage permissions individually for a user regardless of its role. Example:

class Nurse(AbstractUserRole): available_permissions = { 'enter_surgery_room`: True, }

In the common case Nurses are permitted to enter the surgery room. But let's say we are instantiating a novice Nurse we could do this:

new_nurse = User.objects.create_user(...) assign_role(new_nurse, Nurse) revoke_permission(new_nurse, 'enter_surgery_room')

This way the user still has a Nurse role, but is not [yet] allowed in the surgery room.

Now, if you run this script in that situation, the novice nurse will end up with access to the surgery room.

Clear -- thanks!
Yes - I can see my code is a like a sledge hammer where something more delicate is required. I'll give it another shot.

Would it be possible to overwrite the adding and removing of groups and replace them with assign and remove role functions?

This was such a good suggestion @filipeximenes
Code does exactly this now - basically overrides the adding and removing of groups with logic that assigns and removes roles. Works very much as expected, even in tricky cases where roles with overlapping permissions are added/removed in the same operation.

Will push changes to the PR shortly. Thanks for the excellent code review.

@filipeximenes

@filipeximenes

filipeximenes

Looking good! There a are few minor comments. Let's add some docs and ship it!

filipeximenes · 2017-10-23T14:31:09Z

rolepermissions/admin.py

+        old_user_roles = set(r.get_name() for r in roles.get_user_roles(user))
+        super(RolePermissionsUserAdminMixin, self).save_related(request, form, formsets, change)
+
+        new_user_groups = set(g.name for g in user.groups.all())


Cool approach, liked it!

filipeximenes · 2017-10-23T14:34:08Z

rolepermissions/management/commands/sync_roles.py

+            action='store_true',
+            dest='reset_user_permissions',
+            default=False,
+            help='Re-assign all User roles -- resets user Permissions to defaults defined by role(s) !! CAUTION !!',


filipeximenes · 2017-10-23T14:46:58Z

rolepermissions/management/commands/sync_roles.py

+    def handle(self, *args, **options):
+        # Sync auth.Group with current registered roles (leaving existing groups intact!)
+        for role in roles.RolesManager.get_roles() :
+            group, created = Group.objects.get_or_create(name=role.get_name())


Let's create a method in AbstractUserRole to deal with the creation of Groups. We can call it here and here

Good idea. Will do.

filipeximenes · 2017-10-23T14:53:05Z

rolepermissions/roles.py

+    """
+    user_ct = ContentType.objects.get_for_model(get_user_model())
+    # Careful here - don't use name to lookup existing permissions
+    try:


Should we use a get_or_create with codename in the defaults here?

Like...

perm, created = Permission.objects.get_or_create(content_type=user_ct, codename=codename) if created: perm.name=name(codename) if callable(name) else name perm.save()

?

@powderflask get_or_create allows a defaults value that will to the trick. See here https://docs.djangoproject.com/en/1.11/ref/models/querysets/#get-or-create

name = name(codename) if callable(name) else name perm, created = Permission.objects.get_or_create( content_type=user_ct, codename=codename defaults={'name': name})

Ha ha - new to me, go figure !! :-P so simple, duh...

return Permission.objects.get_or_create(content_type=user_ct, codename=codename, defaults={'name':name(codename) if callable(name) else name})

Thanks.

powderflask · 2017-10-24T07:26:47Z

I think all* docs could fit under heading: Admin Integration
I'm thinking a separate page, but might also fit as a new sub-section in Utils?

aside from addition of one Setting

filipeximenes · 2017-10-27T21:15:35Z

@powderflask 'Admin Integration' seems like a good place to have it!

powderflask · 2017-11-04T22:39:42Z

New Admin Integration section added to docs for these 3 new features.
Fixed get_or_create logic goof.

filipeximenes · 2017-12-05T20:08:32Z

Back from vacations, looking good! Thanks very much!

powderflask · 2017-12-05T20:14:10Z

Thank you for all the attentive and helpful code review and suggestions - been fun to work on this.

filipeximenes · 2017-12-05T20:17:03Z

Happy to help! 😄

kavdev

solid work

breneser · 2019-06-05T12:11:01Z

I can't seem to get anything change on Admin site, using a custom model so I sued RolesPermissionsUserAdminMixin to register the role, but can't see any difference. Do i still need to use 'ROLEPERMISSIONS_REGISTER_ADMIN = True' ? If i do I get 'django.contrib.admin.sites.NotRegistered: The model User is not registered'

powderflask · 2019-06-15T06:21:56Z

@breneser - the admin integration has no visible UI elements - it simply hooks role-permissions logic into existing admin actions for managing groups and perms.
Please see docs: https://django-role-permissions.readthedocs.io/en/stable/admin.html
And consider asking for help over on stackoverflow: https://stackoverflow.com/questions/tagged/django
good luck.

Add permission labels (permission.name) when permissions are created.

31456e3

See: vintasoftware#71

powderflask added 2 commits September 21, 2017 01:04

Add custom UserAdmin mixin to sync user's roles to user.groups

8913102

Add sync_roles management command to sync auth.Group to UserRoles

Merge in tox config that resolved model mommy issues

1009050

powderflask changed the title ~~Add permission labels (permission.name) when permissions are created.~~ Manage roles and permissions via the django admin UserAdmin Groups and Permissions. Sep 21, 2017

filipeximenes requested changes Oct 2, 2017

View reviewed changes

PR vintasoftware#72 - improve django-admin integration, suggestion from

e2a783c

@filipeximenes

filipeximenes requested changes Oct 23, 2017

View reviewed changes

powderflask added 2 commits October 28, 2017 11:31

DRY group and permission get_or_create

27ecf53

Add docs for admin integration

845819e

filipeximenes merged commit 6c3600d into vintasoftware:master Dec 5, 2017

kavdev reviewed Dec 5, 2017

View reviewed changes

Manage roles and permissions via the django admin UserAdmin Groups and Permissions. #72

Manage roles and permissions via the django admin UserAdmin Groups and Permissions. #72

Conversation

powderflask commented Sep 20, 2017

powderflask commented Sep 20, 2017

powderflask commented Sep 21, 2017

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

Choose a reason for hiding this comment

filipeximenes 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

powderflask commented Oct 24, 2017

filipeximenes commented Oct 27, 2017

powderflask commented Nov 4, 2017

filipeximenes commented Dec 5, 2017

powderflask commented Dec 5, 2017

filipeximenes commented Dec 5, 2017

kavdev left a comment

Choose a reason for hiding this comment

breneser commented Jun 5, 2019

powderflask commented Jun 15, 2019