From 02687453f2cf8068d6ca999cbf27eb5b761f4ca3 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 29 May 2020 11:18:22 -0400 Subject: [PATCH] Add ArrayField on ObjectPermission to store actions --- netbox/netbox/tests/test_authentication.py | 30 ++++++-------- netbox/users/admin.py | 40 +++++++++++++++++-- .../users/migrations/0007_proxy_group_user.py | 4 +- .../users/migrations/0008_objectpermission.py | 33 +++++++++++++++ netbox/users/models.py | 39 +++++------------- netbox/utilities/auth_backends.py | 16 ++++---- netbox/utilities/testing/testcases.py | 36 ++++++++--------- 7 files changed, 120 insertions(+), 78 deletions(-) create mode 100644 netbox/users/migrations/0008_objectpermission.py diff --git a/netbox/netbox/tests/test_authentication.py b/netbox/netbox/tests/test_authentication.py index ad900bdc0..bef8f004a 100644 --- a/netbox/netbox/tests/test_authentication.py +++ b/netbox/netbox/tests/test_authentication.py @@ -203,7 +203,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -227,7 +227,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -261,8 +261,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True, - can_add=True + actions=['view', 'add'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -309,8 +308,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True, - can_change=True + actions=['view', 'change'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -353,8 +351,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True, - can_delete=True + actions=['view', 'delete'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -402,7 +399,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_add=True + actions=['add'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -451,7 +448,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_change=True + actions=['change'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -495,8 +492,7 @@ class ObjectPermissionViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True, - can_delete=True + actions=['view', 'delete'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -567,7 +563,7 @@ class ObjectPermissionAPIViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -594,7 +590,7 @@ class ObjectPermissionAPIViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -621,7 +617,7 @@ class ObjectPermissionAPIViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_add=True + actions=['add'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -650,7 +646,7 @@ class ObjectPermissionAPIViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_change=True + actions=['change'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -685,7 +681,7 @@ class ObjectPermissionAPIViewTestCase(TestCase): # Assign object permission obj_perm = ObjectPermission( attrs={'site__name': 'Site 1'}, - can_delete=True + actions=['delete'] ) obj_perm.save() obj_perm.users.add(self.user) diff --git a/netbox/users/admin.py b/netbox/users/admin.py index 9482efd5c..507b75869 100644 --- a/netbox/users/admin.py +++ b/netbox/users/admin.py @@ -2,9 +2,10 @@ from django import forms from django.contrib import admin from django.contrib.auth.admin import UserAdmin as UserAdmin_ from django.contrib.auth.models import Group as StockGroup, User as StockUser +from django.core.exceptions import FieldError, ValidationError from extras.admin import order_content_types -from .models import Group, User, ObjectPermission, Token, UserConfig +from .models import AdminGroup, AdminUser, ObjectPermission, Token, UserConfig # @@ -16,7 +17,7 @@ admin.site.unregister(StockGroup) admin.site.unregister(StockUser) -@admin.register(Group) +@admin.register(AdminGroup) class GroupAdmin(admin.ModelAdmin): fields = ('name',) list_display = ('name', 'user_count') @@ -34,7 +35,7 @@ class UserConfigInline(admin.TabularInline): verbose_name = 'Preferences' -@admin.register(User) +@admin.register(AdminUser) class UserAdmin(UserAdmin_): list_display = [ 'username', 'email', 'first_name', 'last_name', 'is_superuser', 'is_staff', 'is_active' @@ -92,10 +93,41 @@ class ObjectPermissionForm(forms.ModelForm): order_content_types(self.fields['content_types']) self.fields['content_types'].choices.insert(0, ('', '---------')) + def clean(self): + content_types = self.cleaned_data['content_types'] + attrs = self.cleaned_data['attrs'] + + # Validate the specified model attributes by attempting to execute a query. We don't care whether the query + # returns anything; we just want to make sure the specified attributes are valid. + if attrs: + for ct in content_types: + model = ct.model_class() + try: + model.objects.filter(**attrs).exists() + except FieldError as e: + raise ValidationError({ + 'attrs': f'Invalid attributes for {model}: {e}' + }) + @admin.register(ObjectPermission) class ObjectPermissionAdmin(admin.ModelAdmin): form = ObjectPermissionForm list_display = [ - 'model', 'can_view', 'can_add', 'can_change', 'can_delete' + 'list_models', 'list_users', 'list_groups', 'actions', 'attrs', ] + + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('content_types', 'users', 'groups') + + def list_models(self, obj): + return ', '.join([f"{ct}" for ct in obj.content_types.all()]) + list_models.short_description = 'Models' + + def list_users(self, obj): + return ', '.join([u.username for u in obj.users.all()]) + list_users.short_description = 'Users' + + def list_groups(self, obj): + return ', '.join([g.name for g in obj.groups.all()]) + list_groups.short_description = 'Groups' diff --git a/netbox/users/migrations/0007_proxy_group_user.py b/netbox/users/migrations/0007_proxy_group_user.py index 4a72eedd2..dfd0512bd 100644 --- a/netbox/users/migrations/0007_proxy_group_user.py +++ b/netbox/users/migrations/0007_proxy_group_user.py @@ -14,7 +14,7 @@ class Migration(migrations.Migration): operations = [ migrations.CreateModel( - name='Group', + name='AdminGroup', fields=[ ], options={ @@ -28,7 +28,7 @@ class Migration(migrations.Migration): ], ), migrations.CreateModel( - name='User', + name='AdminUser', fields=[ ], options={ diff --git a/netbox/users/migrations/0008_objectpermission.py b/netbox/users/migrations/0008_objectpermission.py new file mode 100644 index 000000000..f2ecb98b0 --- /dev/null +++ b/netbox/users/migrations/0008_objectpermission.py @@ -0,0 +1,33 @@ +# Generated by Django 3.0.6 on 2020-05-29 14:59 + +from django.conf import settings +import django.contrib.postgres.fields +import django.contrib.postgres.fields.jsonb +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contenttypes', '0002_remove_content_type_name'), + ('auth', '0011_update_proxy_permissions'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('users', '0007_proxy_group_user'), + ] + + operations = [ + migrations.CreateModel( + name='ObjectPermission', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False)), + ('attrs', django.contrib.postgres.fields.jsonb.JSONField(blank=True, null=True)), + ('actions', django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=30), size=None)), + ('content_types', models.ManyToManyField(limit_choices_to={'app_label__in': ['circuits', 'dcim', 'extras', 'ipam', 'secrets', 'tenancy', 'virtualization']}, related_name='object_permissions', to='contenttypes.ContentType')), + ('groups', models.ManyToManyField(blank=True, related_name='object_permissions', to='auth.Group')), + ('users', models.ManyToManyField(blank=True, related_name='object_permissions', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'Permission', + }, + ), + ] diff --git a/netbox/users/models.py b/netbox/users/models.py index d2a4a152a..1c8775699 100644 --- a/netbox/users/models.py +++ b/netbox/users/models.py @@ -1,10 +1,9 @@ import binascii import os -from django.contrib.auth.models import Group as Group_, User as User_ +from django.contrib.auth.models import Group, User from django.contrib.contenttypes.models import ContentType -from django.contrib.postgres.fields import JSONField -from django.core.exceptions import FieldError, ValidationError +from django.contrib.postgres.fields import ArrayField, JSONField from django.core.validators import MinLengthValidator from django.db import models from django.db.models.signals import post_save @@ -25,7 +24,7 @@ __all__ = ( # Proxy models for admin # -class Group(Group_): +class AdminGroup(Group): """ Proxy contrib.auth.models.Group for the admin UI """ @@ -33,7 +32,7 @@ class Group(Group_): proxy = True -class User(User_): +class AdminUser(User): """ Proxy contrib.auth.models.User for the admin UI """ @@ -256,31 +255,13 @@ class ObjectPermission(models.Model): null=True, verbose_name='Attributes' ) - can_view = models.BooleanField( - default=False - ) - can_add = models.BooleanField( - default=False - ) - can_change = models.BooleanField( - default=False - ) - can_delete = models.BooleanField( - default=False + actions = ArrayField( + base_field=models.CharField(max_length=30), + help_text="The list of actions granted by this permission" ) + class Meta: + verbose_name = "Permission" + def __str__(self): return "Object permission" - - def clean(self): - - # Validate the specified model attributes by attempting to execute a query. We don't care whether the query - # returns anything; we just want to make sure the specified attributes are valid. - if self.attrs: - model = self.model.model_class() - try: - model.objects.filter(**self.attrs).exists() - except FieldError as e: - raise ValidationError({ - 'attrs': f'Invalid attributes for {model}: {e}' - }) diff --git a/netbox/utilities/auth_backends.py b/netbox/utilities/auth_backends.py index 36796194e..bc263480f 100644 --- a/netbox/utilities/auth_backends.py +++ b/netbox/utilities/auth_backends.py @@ -32,13 +32,12 @@ class ObjectPermissionBackend(ModelBackend): perms = dict() for obj_perm in object_permissions: for content_type in obj_perm.content_types.all(): - for action in ['view', 'add', 'change', 'delete']: - if getattr(obj_perm, f"can_{action}"): - perm_name = f"{content_type.app_label}.{action}_{content_type.model}" - if perm_name in perms: - perms[perm_name].append(obj_perm.attrs) - else: - perms[perm_name] = [obj_perm.attrs] + for action in obj_perm.actions: + perm_name = f"{content_type.app_label}.{action}_{content_type.model}" + if perm_name in perms: + perms[perm_name].append(obj_perm.attrs) + else: + perms[perm_name] = [obj_perm.attrs] return perms @@ -123,7 +122,8 @@ class RemoteUserBackend(_RemoteUserBackend): for permission_name in settings.REMOTE_AUTH_DEFAULT_PERMISSIONS: try: content_type, action = resolve_permission(permission_name) - obj_perm = ObjectPermission(**{f'can_{action}': True}) + # TODO: Merge multiple actions into a single ObjectPermission per content type + obj_perm = ObjectPermission(actions=[action]) obj_perm.save() obj_perm.users.add(user) obj_perm.content_types.add(content_type) diff --git a/netbox/utilities/testing/testcases.py b/netbox/utilities/testing/testcases.py index cde394422..3514f9060 100644 --- a/netbox/utilities/testing/testcases.py +++ b/netbox/utilities/testing/testcases.py @@ -34,7 +34,7 @@ class TestCase(_TestCase): """ for name in names: ct, action = resolve_permission(name) - obj_perm = ObjectPermission(**{f'can_{action}': True}) + obj_perm = ObjectPermission(actions=[action]) obj_perm.save() obj_perm.users.add(self.user) obj_perm.content_types.add(ct) @@ -165,7 +165,7 @@ class ViewTestCases: # Add model-level permission obj_perm = ObjectPermission( - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -181,7 +181,7 @@ class ViewTestCases: # Add object-level permission obj_perm = ObjectPermission( attrs={'pk': instance1.pk}, - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -221,7 +221,7 @@ class ViewTestCases: # Assign model-level permission obj_perm = ObjectPermission( - can_add=True + actions=['add'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -246,7 +246,7 @@ class ViewTestCases: # Assign object-level permission obj_perm = ObjectPermission( attrs={'pk__gt': 0}, # Dummy permission to allow all - can_add=True + actions=['add'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -305,7 +305,7 @@ class ViewTestCases: # Assign model-level permission obj_perm = ObjectPermission( - can_change=True + actions=['change'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -329,7 +329,7 @@ class ViewTestCases: # Assign object-level permission obj_perm = ObjectPermission( attrs={'pk': instance1.pk}, - can_change=True + actions=['change'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -382,7 +382,7 @@ class ViewTestCases: # Assign model-level permission obj_perm = ObjectPermission( - can_delete=True + actions=['delete'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -407,7 +407,7 @@ class ViewTestCases: # Assign object-level permission obj_perm = ObjectPermission( attrs={'pk': instance1.pk}, - can_delete=True + actions=['delete'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -459,7 +459,7 @@ class ViewTestCases: # Add model-level permission obj_perm = ObjectPermission( - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -481,7 +481,7 @@ class ViewTestCases: # Add object-level permission obj_perm = ObjectPermission( attrs={'pk': instance1.pk}, - can_view=True + actions=['view'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -512,7 +512,7 @@ class ViewTestCases: self.assertHttpStatus(self.client.post(**request), 403) # Assign object-level permission - obj_perm = ObjectPermission(can_add=True) + obj_perm = ObjectPermission(actions=['add']) obj_perm.save() obj_perm.users.add(self.user) obj_perm.content_types.add(ContentType.objects.get_for_model(self.model)) @@ -557,7 +557,7 @@ class ViewTestCases: # Assign model-level permission obj_perm = ObjectPermission( - can_add=True + actions=['add'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -580,7 +580,7 @@ class ViewTestCases: # Assign object-level permission obj_perm = ObjectPermission( attrs={'pk__gt': 0}, # Dummy permission to allow all - can_add=True + actions=['add'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -627,7 +627,7 @@ class ViewTestCases: # Assign model-level permission obj_perm = ObjectPermission( - can_change=True + actions=['change'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -652,7 +652,7 @@ class ViewTestCases: # Assign object-level permission obj_perm = ObjectPermission( attrs={'pk__in': list(pk_list)}, - can_change=True + actions=['change'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -697,7 +697,7 @@ class ViewTestCases: # Assign model-level permission obj_perm = ObjectPermission( - can_delete=True + actions=['delete'] ) obj_perm.save() obj_perm.users.add(self.user) @@ -719,7 +719,7 @@ class ViewTestCases: # Assign object-level permission obj_perm = ObjectPermission( attrs={'pk__in': list(pk_list)}, - can_delete=True + actions=['delete'] ) obj_perm.save() obj_perm.users.add(self.user)