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

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

Merged
merged 6 commits into from
Dec 5, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion rolepermissions/admin.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,31 @@
from django.conf import settings
from django.contrib import admin, auth
from django.contrib.auth.admin import UserAdmin
from rolepermissions import roles

from django.contrib import admin
ROLEPERMISSIONS_REGISTER_ADMIN = getattr(settings, 'ROLEPERMISSIONS_REGISTER_ADMIN', False)
UserModel = auth.get_user_model()


class RolePermissionsUserAdminMixin(object):
""" Must be mixed in with an UserAdmin class"""
def save_related(self, request, form, formsets, change):
super(RolePermissionsUserAdminMixin, self).save_related(request, form, formsets, change)
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

The problem this feature is trying to address is:

  1. Edit / Create User in Admin; Assign the User to a Group
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

roles.clear_roles(user)
for g in groups :
try :
roles.assign_role(user, g.name)
except roles.RoleDoesNotExist :
pass


class RolePermissionsUserAdmin(RolePermissionsUserAdminMixin, UserAdmin):
pass

if ROLEPERMISSIONS_REGISTER_ADMIN:
admin.site.unregister(UserModel)
admin.site.register(UserModel, RolePermissionsUserAdmin)
Empty file.
Empty file.
27 changes: 27 additions & 0 deletions rolepermissions/management/commands/sync_roles.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
from django.conf import settings
from django.core.management.base import BaseCommand
from django.contrib.auth.models import Group
from django.contrib.auth import get_user_model
from rolepermissions import roles

UserModel = get_user_model()

class Command(BaseCommand):
ROLEPERMISSIONS_MODULE = getattr(settings, 'ROLEPERMISSIONS_MODULE', '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!)
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

for role in roles.RolesManager.get_roles() :
group, created = Group.objects.get_or_create(name=role.get_name())
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Will do.

if created:
self.stdout.write("Created Group: %s from Role: %s"%(group.name, role.get_name()))
# Sync auth.Permission with permissions for this role
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():
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

sync_roles  --reset_user_permissions

Willing to document this clearly.

user_roles = roles.get_user_roles(user)
roles.clear_roles(user)
for role in user_roles:
roles.assign_role(user, role)
6 changes: 2 additions & 4 deletions rolepermissions/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from rolepermissions.exceptions import (
RolePermissionScopeException, CheckerNotRegistered)
from rolepermissions.roles import get_user_roles
from rolepermissions.roles import get_user_roles, get_or_create_permission


class PermissionsManager(object):
Expand Down Expand Up @@ -37,9 +37,7 @@ def fuction_decorator(func):

def get_permission(permission_name):
"""Get a Permission object from a permission name."""
user_ct = ContentType.objects.get_for_model(get_user_model())
permission, _created = Permission.objects.get_or_create(
content_type=user_ct, codename=permission_name)
permission, created = get_or_create_permission(permission_name)

return permission

Expand Down
23 changes: 17 additions & 6 deletions rolepermissions/roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from django.contrib.auth import get_user_model
from django.contrib.contenttypes.models import ContentType

from rolepermissions.utils import camelToSnake
from rolepermissions.utils import camelToSnake, camel_or_snake_to_title
from rolepermissions.exceptions import RoleDoesNotExist


Expand All @@ -29,6 +29,10 @@ def retrieve_role(cls, role_name):
def get_roles_names(cls):
return registered_roles.keys()

@classmethod
def get_roles(cls):
return registered_roles.values()


class RolesClassRegister(type):

Expand Down Expand Up @@ -158,11 +162,11 @@ def get_or_create_permissions(cls, permission_names):
permissions = list(Permission.objects.filter(
content_type=user_ct, codename__in=permission_names).all())

if len(permissions) != len(permission_names):
for permission_name in permission_names:
permission, created = Permission.objects.get_or_create(
content_type=user_ct, codename=permission_name)
if created:
missing_permissions = set(permission_names) - set((p.codename for p in permissions))
if len(missing_permissions) > 0:
for permission_name in missing_permissions:
permission, created = get_or_create_permission(permission_name)
if created: # assert created is True
permissions.append(permission)

return permissions
Expand All @@ -172,6 +176,13 @@ def get_default(cls, permission_name):
return cls.available_permissions[permission_name]


def get_or_create_permission(permission_name):
"""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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also write a test for this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.



def retrieve_role(role_name):
"""Get a Role object from a role name."""
return RolesManager.retrieve_role(role_name)
Expand Down
91 changes: 91 additions & 0 deletions rolepermissions/tests/test_admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
from collections import namedtuple
from django.core.management import call_command
from django.test import TestCase
from django.utils.six import StringIO
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Group, Permission

from model_mommy import mommy

from rolepermissions.roles import AbstractUserRole, get_user_roles
from rolepermissions.admin import RolePermissionsUserAdminMixin


class AdminRole1(AbstractUserRole):
available_permissions = {
'admin_perm1': True,
'admin_perm2': False,
}


class UserAdminMixinTest(TestCase):
class UserAdminMock:
def save_related(self, request, form, formsets, change) :
pass
class CustomUserAdminMock(RolePermissionsUserAdminMixin, UserAdminMock):
pass

FormMock = namedtuple('FormMock', ['instance', ])

def setup(self):
pass

def test_admin_save_related_syncs_roles(self):
user = mommy.make(get_user_model())
grp1 = mommy.make(Group)
grp2 = mommy.make(Group, name=AdminRole1.get_name())
user.groups.add(grp1)
user.groups.add(grp2)
form = self.FormMock(instance=user)
self.CustomUserAdminMock().save_related(None, form, None, None)
user_roles = get_user_roles(user)
self.assertNotIn(grp1.name, (role.get_name() for role in user_roles))
self.assertIn(AdminRole1, user_roles)


class SyncRolesTest(TestCase):

def setup(self):
pass

def test_sync_group(self):
out = StringIO()
call_command('sync_roles', stdout=out)
self.assertIn('Created Group: %s'%AdminRole1.get_name(), out.getvalue())
group_names = [group['name'] for group in Group.objects.all().values('name')]
self.assertIn(AdminRole1.get_name(), group_names)

def test_sync_permissions(self):
out = StringIO()
call_command('sync_roles', stdout=out)
permissions = [perm['codename'] for perm in Permission.objects.all().values('codename')]
self.assertIn('admin_perm1', permissions)
self.assertNotIn('admin_perm2', permissions)

def test_sync_user_role_permissions(self):
user = mommy.make(get_user_model())
grp1 = mommy.make(Group)
grp2 = mommy.make(Group, name=AdminRole1.get_name())
user.groups.add(grp1)
user.groups.add(grp2)
out = StringIO()
call_command('sync_roles', stdout=out)

user_group_names = [group['name'] for group in user.groups.all().values('name')]
self.assertIn(grp1.name, user_group_names)
self.assertIn(grp2.name, user_group_names)

user_permission_names = [perm['codename'] for perm in user.user_permissions.all().values('codename')]
self.assertIn('admin_perm1', user_permission_names)
self.assertNotIn('admin_perm2', user_permission_names)

def test_sync_preserves_groups(self):
grp1 = mommy.make(Group)
grp2 = mommy.make(Group, name=AdminRole1.get_name())
out = StringIO()
call_command('sync_roles', stdout=out)
group_names = [group['name'] for group in Group.objects.all().values('name')]
self.assertIn(grp1.name, group_names)
self.assertIn(grp2.name, group_names)


17 changes: 17 additions & 0 deletions rolepermissions/tests/test_roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ class RolRole3(AbstractUserRole):
'permission6': False,
}

class RolRole4(AbstractUserRole):
available_permissions = {
'permission_number_7': True,
'PermissionNumber8': True,
}

class AbstractUserRoleTests(TestCase):

Expand Down Expand Up @@ -130,6 +135,18 @@ def test_permission_names_list(self):
self.assertIn('permission3', RolRole2.permission_names_list())
self.assertIn('permission4', RolRole2.permission_names_list())

def test_permission_labels(self):
user = mommy.make(get_user_model())

RolRole4.assign_role_to_user(user)
permissions = user.user_permissions.all()

permission_labels = [perm.name for perm in permissions]

self.assertIn('Permission Number 7', permission_labels)
self.assertIn('Permission Number8', permission_labels)
self.assertEquals(len(permissions), 2)


class RolesManagerTests(TestCase):

Expand Down
7 changes: 7 additions & 0 deletions rolepermissions/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,10 @@ def camelToSnake(s):

subbed = _underscorer1.sub(r'\1_\2', s)
return _underscorer2.sub(r'\1_\2', subbed).lower()


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

def camel_or_snake_to_title(s):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some tests for those functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Good catch.

return snake_to_title(camelToSnake(s))