From a673015f86e18a4e795bea26082e281635dd5b79 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 27 Mar 2024 16:12:14 -0400 Subject: [PATCH] Move user & group M2M assignments for ObjectPermission --- netbox/users/api/serializers_/permissions.py | 25 +--- netbox/users/api/serializers_/users.py | 23 +++- netbox/users/forms/model_forms.py | 26 ++-- .../0008_flip_objectpermission_assignments.py | 122 ++++++++++++++++++ netbox/users/models.py | 20 +-- netbox/users/tests/test_api.py | 109 +++++++++------- 6 files changed, 234 insertions(+), 91 deletions(-) create mode 100644 netbox/users/migrations/0008_flip_objectpermission_assignments.py diff --git a/netbox/users/api/serializers_/permissions.py b/netbox/users/api/serializers_/permissions.py index ff5a1f212..066834db0 100644 --- a/netbox/users/api/serializers_/permissions.py +++ b/netbox/users/api/serializers_/permissions.py @@ -1,11 +1,9 @@ -from django.contrib.auth import get_user_model from rest_framework import serializers from core.models import ObjectType -from netbox.api.fields import ContentTypeField, SerializedPKRelatedField +from netbox.api.fields import ContentTypeField from netbox.api.serializers import ValidatedModelSerializer -from users.models import Group, ObjectPermission -from .users import GroupSerializer, UserSerializer +from users.models import ObjectPermission __all__ = ( 'ObjectPermissionSerializer', @@ -18,27 +16,12 @@ class ObjectPermissionSerializer(ValidatedModelSerializer): queryset=ObjectType.objects.all(), many=True ) - groups = SerializedPKRelatedField( - queryset=Group.objects.all(), - serializer=GroupSerializer, - nested=True, - required=False, - many=True - ) - users = SerializedPKRelatedField( - queryset=get_user_model().objects.all(), - serializer=UserSerializer, - nested=True, - required=False, - many=True - ) class Meta: model = ObjectPermission fields = ( - 'id', 'url', 'display', 'name', 'description', 'enabled', 'object_types', 'groups', 'users', 'actions', - 'constraints', + 'id', 'url', 'display', 'name', 'description', 'enabled', 'object_types', 'actions', 'constraints', ) brief_fields = ( - 'id', 'url', 'display', 'name', 'description', 'enabled', 'object_types', 'groups', 'users', 'actions', + 'id', 'url', 'display', 'name', 'description', 'enabled', 'object_types', 'actions', ) diff --git a/netbox/users/api/serializers_/users.py b/netbox/users/api/serializers_/users.py index 4aa2b5a8c..ded5dd1d9 100644 --- a/netbox/users/api/serializers_/users.py +++ b/netbox/users/api/serializers_/users.py @@ -5,7 +5,8 @@ from rest_framework import serializers from netbox.api.fields import SerializedPKRelatedField from netbox.api.serializers import ValidatedModelSerializer -from users.models import Group +from users.models import Group, ObjectPermission +from .permissions import ObjectPermissionSerializer __all__ = ( 'GroupSerializer', @@ -16,10 +17,18 @@ __all__ = ( class GroupSerializer(ValidatedModelSerializer): url = serializers.HyperlinkedIdentityField(view_name='users-api:group-detail') user_count = serializers.IntegerField(read_only=True) + permissions = SerializedPKRelatedField( + source='object_permissions', + queryset=ObjectPermission.objects.all(), + serializer=ObjectPermissionSerializer, + nested=True, + required=False, + many=True + ) class Meta: model = Group - fields = ('id', 'url', 'display', 'name', 'user_count') + fields = ('id', 'url', 'display', 'name', 'permissions', 'user_count') brief_fields = ('id', 'url', 'display', 'name') @@ -32,12 +41,20 @@ class UserSerializer(ValidatedModelSerializer): required=False, many=True ) + permissions = SerializedPKRelatedField( + source='object_permissions', + queryset=ObjectPermission.objects.all(), + serializer=ObjectPermissionSerializer, + nested=True, + required=False, + many=True + ) class Meta: model = get_user_model() fields = ( 'id', 'url', 'display', 'username', 'password', 'first_name', 'last_name', 'email', 'is_staff', 'is_active', - 'date_joined', 'last_login', 'groups', + 'date_joined', 'last_login', 'groups', 'permissions', ) brief_fields = ('id', 'url', 'display', 'username') extra_kwargs = { diff --git a/netbox/users/forms/model_forms.py b/netbox/users/forms/model_forms.py index 458d091ec..2d4431f96 100644 --- a/netbox/users/forms/model_forms.py +++ b/netbox/users/forms/model_forms.py @@ -203,9 +203,6 @@ class UserForm(forms.ModelForm): super().__init__(*args, **kwargs) if self.instance.pk: - # Populate assigned permissions - self.fields['object_permissions'].initial = self.instance.object_permissions.values_list('id', flat=True) - # Password fields are optional for existing Users self.fields['password'].required = False self.fields['confirm_password'].required = False @@ -213,9 +210,6 @@ class UserForm(forms.ModelForm): def save(self, *args, **kwargs): instance = super().save(*args, **kwargs) - # Update assigned permissions - instance.object_permissions.set(self.cleaned_data['object_permissions']) - # On edit, check if we have to save the password if self.cleaned_data.get('password'): instance.set_password(self.cleaned_data.get('password')) @@ -260,14 +254,12 @@ class GroupForm(forms.ModelForm): # Populate assigned users and permissions if self.instance.pk: self.fields['users'].initial = self.instance.users.values_list('id', flat=True) - self.fields['object_permissions'].initial = self.instance.object_permissions.values_list('id', flat=True) def save(self, *args, **kwargs): instance = super().save(*args, **kwargs) - # Update assigned users and permissions + # Update assigned users instance.users.set(self.cleaned_data['users']) - instance.object_permissions.set(self.cleaned_data['object_permissions']) return instance @@ -335,9 +327,10 @@ class ObjectPermissionForm(forms.ModelForm): # Make the actions field optional since the form uses it only for non-CRUD actions self.fields['actions'].required = False - # Order group and user fields - self.fields['groups'].queryset = self.fields['groups'].queryset.order_by('name') - self.fields['users'].queryset = self.fields['users'].queryset.order_by('username') + # Populate assigned users and groups + if self.instance.pk: + self.fields['groups'].initial = self.instance.groups.values_list('id', flat=True) + self.fields['users'].initial = self.instance.users.values_list('id', flat=True) # Check the appropriate checkboxes when editing an existing ObjectPermission if self.instance.pk: @@ -381,3 +374,12 @@ class ObjectPermissionForm(forms.ModelForm): raise forms.ValidationError({ 'constraints': _('Invalid filter for {model}: {error}').format(model=model, error=e) }) + + def save(self, *args, **kwargs): + instance = super().save(*args, **kwargs) + + # Update assigned users and groups + instance.users.set(self.cleaned_data['users']) + instance.groups.set(self.cleaned_data['groups']) + + return instance diff --git a/netbox/users/migrations/0008_flip_objectpermission_assignments.py b/netbox/users/migrations/0008_flip_objectpermission_assignments.py new file mode 100644 index 000000000..5f197a290 --- /dev/null +++ b/netbox/users/migrations/0008_flip_objectpermission_assignments.py @@ -0,0 +1,122 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0007_objectpermission_update_object_types'), + ] + + operations = [ + # Flip M2M assignments for ObjectPermission to Groups + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.RemoveField( + model_name='objectpermission', + name='groups', + ), + migrations.AddField( + model_name='group', + name='object_permissions', + field=models.ManyToManyField(blank=True, related_name='groups', to='users.objectpermission'), + ), + ], + database_operations=[ + # Rename table + migrations.RunSQL( + "ALTER TABLE users_objectpermission_groups" + " RENAME TO users_group_object_permissions" + ), + migrations.RunSQL( + "ALTER TABLE users_objectpermission_groups_id_seq" + " RENAME TO users_group_object_permissions_id_seq" + ), + + # Rename constraints + migrations.RunSQL( + "ALTER TABLE users_group_object_permissions RENAME CONSTRAINT " + "users_objectpermissi_group_id_fb7ba6e0_fk_users_gro TO " + "users_group_object_p_group_id_90dd183a_fk_users_gro" + ), + migrations.RunSQL( + "ALTER TABLE users_group_object_permissions RENAME CONSTRAINT " + "users_objectpermissi_objectpermission_id_2f7cc117_fk_users_obj TO " + "users_group_object_p_objectpermission_id_dd489dc4_fk_users_obj" + ), + + # Rename indexes + migrations.RunSQL( + "ALTER INDEX users_objectpermission_groups_pkey " + " RENAME TO users_group_object_permissions_pkey" + ), + migrations.RunSQL( + "ALTER INDEX users_objectpermission_g_objectpermission_id_grou_3b62a39c_uniq " + " RENAME TO users_group_object_permi_group_id_objectpermissio_db1f8cbe_uniq" + ), + migrations.RunSQL( + "ALTER INDEX users_objectpermission_groups_group_id_fb7ba6e0" + " RENAME TO users_group_object_permissions_group_id_90dd183a" + ), + migrations.RunSQL( + "ALTER INDEX users_objectpermission_groups_objectpermission_id_2f7cc117" + " RENAME TO users_group_object_permissions_objectpermission_id_dd489dc4" + ), + ] + ), + + # Flip M2M assignments for ObjectPermission to Users + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.RemoveField( + model_name='objectpermission', + name='users', + ), + migrations.AddField( + model_name='user', + name='object_permissions', + field=models.ManyToManyField(blank=True, related_name='users', to='users.objectpermission'), + ), + ], + database_operations=[ + # Rename table + migrations.RunSQL( + "ALTER TABLE users_objectpermission_users" + " RENAME TO users_user_object_permissions" + ), + migrations.RunSQL( + "ALTER TABLE users_objectpermission_users_id_seq" + " RENAME TO users_user_object_permissions_id_seq" + ), + + # Rename constraints + migrations.RunSQL( + "ALTER TABLE users_user_object_permissions RENAME CONSTRAINT " + "users_objectpermissi_objectpermission_id_78a9c2e6_fk_users_obj TO " + "users_user_object_pe_objectpermission_id_29b431b4_fk_users_obj" + ), + migrations.RunSQL( + "ALTER TABLE users_user_object_permissions RENAME CONSTRAINT " + "users_objectpermission_users_user_id_16c0905d_fk_auth_user_id TO " + "users_user_object_permissions_user_id_9d647aac_fk_users_user_id" + ), + + # Rename indexes + migrations.RunSQL( + "ALTER INDEX users_objectpermission_users_pkey " + " RENAME TO users_user_object_permissions_pkey" + ), + migrations.RunSQL( + "ALTER INDEX users_objectpermission_u_objectpermission_id_user_3a7db108_uniq " + " RENAME TO users_user_object_permis_user_id_objectpermission_0a98550e_uniq" + ), + migrations.RunSQL( + "ALTER INDEX users_objectpermission_users_user_id_16c0905d" + " RENAME TO users_user_object_permissions_user_id_9d647aac" + ), + migrations.RunSQL( + "ALTER INDEX users_objectpermission_users_objectpermission_id_78a9c2e6" + " RENAME TO users_user_object_permissions_objectpermission_id_29b431b4" + ), + ] + ), + ] diff --git a/netbox/users/models.py b/netbox/users/models.py index f580bd17b..d9a86e88b 100644 --- a/netbox/users/models.py +++ b/netbox/users/models.py @@ -53,6 +53,11 @@ class Group(models.Model): max_length=200, blank=True ) + object_permissions = models.ManyToManyField( + to='users.ObjectPermission', + blank=True, + related_name='groups' + ) # Replicate legacy Django permissions support from stock Group model # to ensure authentication backend compatibility @@ -92,6 +97,11 @@ class User(AbstractUser): related_name='users', related_query_name='user' ) + object_permissions = models.ManyToManyField( + to='users.ObjectPermission', + blank=True, + related_name='users' + ) objects = UserManager() @@ -387,16 +397,6 @@ class ObjectPermission(models.Model): limit_choices_to=OBJECTPERMISSION_OBJECT_TYPES, related_name='object_permissions' ) - groups = models.ManyToManyField( - to='users.Group', - blank=True, - related_name='object_permissions' - ) - users = models.ManyToManyField( - to=get_user_model(), - blank=True, - related_name='object_permissions' - ) actions = ArrayField( base_field=models.CharField(max_length=30), help_text=_("The list of actions granted by this permission") diff --git a/netbox/users/tests/test_api.py b/netbox/users/tests/test_api.py index 7c9710f64..0bf1eb0b1 100644 --- a/netbox/users/tests/test_api.py +++ b/netbox/users/tests/test_api.py @@ -16,29 +16,13 @@ class AppTest(APITestCase): url = reverse('users-api:api-root') response = self.client.get(f'{url}?format=api', **self.header) - self.assertEqual(response.status_code, 200) class UserTest(APIViewTestCases.APIViewTestCase): model = User - view_namespace = 'users' brief_fields = ['display', 'id', 'url', 'username'] validation_excluded_fields = ['password'] - create_data = [ - { - 'username': 'User_4', - 'password': 'password4', - }, - { - 'username': 'User_5', - 'password': 'password5', - }, - { - 'username': 'User_6', - 'password': 'password6', - }, - ] bulk_update_data = { 'email': 'test@example.com', } @@ -46,6 +30,16 @@ class UserTest(APIViewTestCases.APIViewTestCase): @classmethod def setUpTestData(cls): + permissions = ( + ObjectPermission(name='Permission 1', actions=['view']), + ObjectPermission(name='Permission 2', actions=['view']), + ObjectPermission(name='Permission 3', actions=['view']), + ) + ObjectPermission.objects.bulk_create(permissions) + permissions[0].object_types.add(ObjectType.objects.get_by_natural_key('dcim', 'site')) + permissions[1].object_types.add(ObjectType.objects.get_by_natural_key('dcim', 'location')) + permissions[2].object_types.add(ObjectType.objects.get_by_natural_key('dcim', 'rack')) + users = ( User(username='User_1', password='password1'), User(username='User_2', password='password2'), @@ -53,6 +47,24 @@ class UserTest(APIViewTestCases.APIViewTestCase): ) User.objects.bulk_create(users) + cls.create_data = [ + { + 'username': 'User_4', + 'password': 'password4', + 'permissions': [permissions[0].pk], + }, + { + 'username': 'User_5', + 'password': 'password5', + 'permissions': [permissions[1].pk], + }, + { + 'username': 'User_6', + 'password': 'password6', + 'permissions': [permissions[2].pk], + }, + ] + def test_that_password_is_changed(self): """ Test that password is changed @@ -76,41 +88,56 @@ class UserTest(APIViewTestCases.APIViewTestCase): 'password': 'newpassword' } url = reverse('users-api:user-detail', kwargs={'pk': user.id}) - response = self.client.patch(url, data, format='json', **self.header) - self.assertEqual(response.status_code, 200) - - updated_user = User.objects.get(id=user.id) - - self.assertTrue(updated_user.check_password(data['password'])) + user.refresh_from_db() + self.assertTrue(user.check_password(data['password'])) class GroupTest(APIViewTestCases.APIViewTestCase): model = Group - view_namespace = 'users' brief_fields = ['display', 'id', 'name', 'url'] - create_data = [ - { - 'name': 'Group 4', - }, - { - 'name': 'Group 5', - }, - { - 'name': 'Group 6', - }, - ] @classmethod def setUpTestData(cls): - users = ( + permissions = ( + ObjectPermission(name='Permission 1', actions=['view']), + ObjectPermission(name='Permission 2', actions=['view']), + ObjectPermission(name='Permission 3', actions=['view']), + ) + ObjectPermission.objects.bulk_create(permissions) + permissions[0].object_types.add(ObjectType.objects.get_by_natural_key('dcim', 'site')) + permissions[1].object_types.add(ObjectType.objects.get_by_natural_key('dcim', 'location')) + permissions[2].object_types.add(ObjectType.objects.get_by_natural_key('dcim', 'rack')) + + groups = ( Group(name='Group 1'), Group(name='Group 2'), Group(name='Group 3'), ) - Group.objects.bulk_create(users) + Group.objects.bulk_create(groups) + + cls.create_data = [ + { + 'name': 'Group 4', + 'permissions': [permissions[0].pk], + }, + { + 'name': 'Group 5', + 'permissions': [permissions[1].pk], + }, + { + 'name': 'Group 6', + 'permissions': [permissions[2].pk], + }, + ] + + def model_to_dict(self, instance, *args, **kwargs): + # Overwrite permissions attr to work around the serializer field having a different name + data = super().model_to_dict(instance, *args, **kwargs) + data['permissions'] = list(instance.object_permissions.values_list('id', flat=True)) + return data def test_bulk_update_objects(self): """ @@ -240,9 +267,7 @@ class ObjectPermissionTest( APIViewTestCases.DeleteObjectViewTestCase ): model = ObjectPermission - brief_fields = [ - 'actions', 'description', 'display', 'enabled', 'groups', 'id', 'name', 'object_types', 'url', 'users', - ] + brief_fields = ['actions', 'description', 'display', 'enabled', 'id', 'name', 'object_types', 'url'] @classmethod def setUpTestData(cls): @@ -278,24 +303,18 @@ class ObjectPermissionTest( { 'name': 'Permission 4', 'object_types': ['dcim.site'], - 'groups': [groups[0].pk], - 'users': [users[0].pk], 'actions': ['view', 'add', 'change', 'delete'], 'constraints': {'name': 'TEST4'}, }, { 'name': 'Permission 5', 'object_types': ['dcim.site'], - 'groups': [groups[1].pk], - 'users': [users[1].pk], 'actions': ['view', 'add', 'change', 'delete'], 'constraints': {'name': 'TEST5'}, }, { 'name': 'Permission 6', 'object_types': ['dcim.site'], - 'groups': [groups[2].pk], - 'users': [users[2].pk], 'actions': ['view', 'add', 'change', 'delete'], 'constraints': {'name': 'TEST6'}, },