Implement role permissions exception the right way

Author: AlanCoding

ansible_base/rbac/models/role.py

@@ -326,15 +326,6 @@ def user_global_permissions(cls, user, permission_qs=None):
             perm_set.update(perm_qs)
         return perm_set
 
-    def save_remote_permissions(self, additional_data: list[str]):
-        """Save permissions from an external system.
-
-        This does very little more than just making the related permissions exactly equal to the set.
-        If a provided permission is not present in the local system, we pretend we did not get it.
-        However, we must still allow removing permissions from the role.
-        """
-        self.permissions.set(DABPermission.objects.filter(api_slug__in=additional_data))
-
     def summary_fields(self):
         return {'id': self.id, 'name': self.name, 'description': self.description, 'managed': self.managed}
 

ansible_base/resource_registry/registry.py

@@ -4,7 +4,7 @@
 from django.contrib.auth import authenticate
 from django.utils.translation import gettext_lazy as _
 
-from ansible_base.resource_registry.utils.resource_type_processor import ResourceTypeProcessor
+from ansible_base.resource_registry.utils.resource_type_processor import ResourceTypeProcessor, RoleDefinitionProcessor
 
 ParentResource = namedtuple("ParentResource", ["model", "field_name"])
 SharedResource = namedtuple("SharedResource", ["serializer", "is_provider"])
@@ -27,7 +27,7 @@ class ServiceAPIConfig:
         "shared.team": ResourceTypeProcessor,
         "shared.organization": ResourceTypeProcessor,
         "shared.user": ResourceTypeProcessor,
-        "shared.roledefinition": ResourceTypeProcessor,
+        "shared.roledefinition": RoleDefinitionProcessor,
     }
 
     custom_resource_processors = {}

ansible_base/resource_registry/rest_client.py

@@ -10,8 +10,8 @@
 
 ResourceRequestBody = namedtuple(
     "ResourceRequestBody",
-    ["ansible_id", "service_id", "is_partially_migrated", "resource_type", "resource_data", "additional_data"],
-    defaults=(None, None, None, None, None, None),
+    ["ansible_id", "service_id", "is_partially_migrated", "resource_type", "resource_data"],
+    defaults=(None, None, None, None, None),
 )
 
 

ansible_base/resource_registry/serializers.py

@@ -96,7 +96,7 @@ def create(self, validated_data):
 
 
 class ResourceSerializer(ResourceListSerializer):
-    additional_data = serializers.JSONField(write_only=True, required=False)
+    additional_data = serializers.SerializerMethodField()
     resource_data = ResourceDataField(source="*")
 
     class Meta:
@@ -106,37 +106,12 @@ class Meta:
             "additional_data",
         ]
 
-    def to_representation(self, instance):
-        ret = super().to_representation(instance)
-
-        additional_data = None
-        if serializer := instance.content_type.resource_type.serializer_class:
+    def get_additional_data(self, obj):
+        if serializer := obj.content_type.resource_type.serializer_class:
             if serializer.ADDITIONAL_DATA_SERIALIZER is not None:
-                additional_data = serializer.ADDITIONAL_DATA_SERIALIZER(instance.content_object).data
-
-        ret['additional_data'] = additional_data
-        return ret
-
-    def _maybe_process_additional_data(self, instance, additional_data):
-        if not additional_data:
-            return
+                return serializer.ADDITIONAL_DATA_SERIALIZER(obj.content_object).data
 
-        if serializer := instance.content_type.resource_type.serializer_class:
-            if serializer and hasattr(serializer, 'process_additional_data'):
-                logger.debug(f'Processing additional data for resource {str(instance.ansible_id)}')
-                serializer(instance.content_object).process_additional_data(instance.content_object, additional_data)
-
-    def create(self, validated_data):
-        additional_data = validated_data.pop('additional_data', None)
-        instance = super().create(validated_data)
-        self._maybe_process_additional_data(instance, additional_data)
-        return instance
-
-    def update(self, instance, validated_data):
-        additional_data = validated_data.pop('additional_data', None)
-        instance = super().update(instance, validated_data)
-        self._maybe_process_additional_data(instance, additional_data)
-        return instance
+        return None
 
 
 class ResourceTypeSerializer(serializers.ModelSerializer):

ansible_base/resource_registry/shared_types.py

@@ -1,6 +1,6 @@
 from rest_framework import serializers
 
-from ansible_base.rbac.models import DABContentType
+from ansible_base.rbac.models import DABContentType, DABPermission
 from ansible_base.resource_registry.utils.resource_type_serializers import AnsibleResourceForeignKeyField, SharedResourceTypeSerializer
 from ansible_base.resource_registry.utils.sso_provider import get_sso_provider_server
 
@@ -87,6 +87,17 @@ class RoleDefinitionPermissionsSerializer(serializers.Serializer):
     )
 
 
+class LenientPermissionSlugListField(serializers.ListField):
+    child = serializers.CharField()
+
+    def to_internal_value(self, data):
+        data = super().to_internal_value(data)
+        return list(DABPermission.objects.filter(api_slug__in=data))
+
+    def to_representation(self, value):
+        return [perm.api_slug for perm in value.all() if perm is not None]
+
+
 class RoleDefinitionType(SharedResourceTypeSerializer):
     RESOURCE_TYPE = "roledefinition"
     ADDITIONAL_DATA_SERIALIZER = RoleDefinitionPermissionsSerializer
@@ -101,6 +112,4 @@ class RoleDefinitionType(SharedResourceTypeSerializer):
         allow_null=True,
         default=None,
     )
-
-    def process_additional_data(self, instance, additional_data):
-        instance.save_remote_permissions(additional_data.get('permissions'))
+    permissions = LenientPermissionSlugListField()

ansible_base/resource_registry/utils/resource_type_processor.py

@@ -39,3 +39,18 @@ def save(self, validated_data, is_new=False):
 
         self.instance.save()
         return self.instance
+
+
+class RoleDefinitionProcessor(ResourceTypeProcessor):
+    def save(self, validated_data, is_new=False):
+        permissions = None  # many-to-many field
+        for k, val in validated_data.items():
+            if k == 'permissions':
+                permissions = val
+            else:
+                setattr(self.instance, k, val)
+
+        self.instance.save()
+        if permissions is not None:
+            self.instance.permissions.set(permissions)
+        return self.instance

test_app/tests/rbac/models/test_role_definition.py

@@ -105,12 +105,3 @@ def test_change_role_definition_member_permission(organization, inventory, org_t
     # Adding it back restores them
     member_rd.permissions.add(member_perm)
     assert [u.has_obj_perm(inventory, 'change') for u in (team_user, org_team_user)] == [True, True]
-
-
-@pytest.mark.django_db
-def test_save_remote_permissions(inv_rd):
-    "This is tested as part of resource registry in test_additional_data_write"
-    assert {perm.api_slug for perm in inv_rd.permissions.all()} == {'aap.view_inventory', 'aap.change_inventory'}
-    assert len(DABPermission.objects.filter(api_slug__in={'aap.view_inventory', 'fooland.not_here'})) == 1
-    inv_rd.permissions.set([DABPermission.objects.get(api_slug='aap.view_inventory')])
-    assert {perm.api_slug for perm in inv_rd.permissions.all()} == {'aap.view_inventory'}

test_app/tests/resource_registry/test_resources_api_rest_client.py

@@ -219,7 +219,10 @@ def test_additional_data_write(resource_client, partial):
     assert resp.status_code == 200
     ref = resp.json()
 
-    data = ResourceRequestBody(additional_data={'permissions': ['aap.view_inventory', 'fooland.action_unicorns']}, resource_data=ref['resource_data'])
+    res_data = ref['resource_data']
+    res_data['permissions'] = ['aap.view_inventory', 'fooland.action_unicorns']
+
+    data = ResourceRequestBody(resource_data=res_data)
     resp = resource_client.update_resource(ansible_id, data, partial=partial)
     assert resp.status_code == 200, resp.__dict__