Skip replicating empty relations when deleting role (#1429)

* Skip replicating empty relations when deleting role

* Skip replication for created role with empty access

* Add loggings for the cases of expected empty relations

* Skip group relation replication if not needed

* Skip replicating relations of group if there is no roles to add

* Add logging when there is empty relation to replicate when expiring cross account request

* Move the logic into view

rbac/management/group/definer.py

@@ -182,6 +182,8 @@ def add_roles(group, roles_or_role_ids, tenant, user=None):
         group_role_change_notification_handler(user, group, role, "added")
         added_roles.append(role)
 
+    if not added_roles:
+        return
     if tenant.tenant_name != "public":
         dual_write_handler = RelationApiDualWriteGroupHandler(group, ReplicationEventType.ASSIGN_ROLE)
         dual_write_handler.generate_relations_to_add_roles(added_roles)

rbac/management/group/relation_api_dual_write_group_handler.py

@@ -44,6 +44,7 @@ class RelationApiDualWriteGroupHandler(RelationApiDualWriteSubjectHandler):
     """Class to handle Dual Write for group bindings and membership."""
 
     group: Group
+    _expected_empty_relation_reason = None
 
     def __init__(
         self,
@@ -109,6 +110,9 @@ def replicate_removed_principals(self, principals: list[Principal]):
     def _replicate(self):
         if not self.replication_enabled():
             return
+        if self._expected_empty_relation_reason:
+            logger.info(f"[Dual Write] Skipping empty replication event. {self._expected_empty_relation_reason}")
+            return
         try:
             self._replicator.replicate(
                 ReplicationEvent(
@@ -170,11 +174,10 @@ def remove_group_from_binding(mapping: BindingMapping):
             role, update_mapping=remove_group_from_binding, create_default_mapping_for_system_role=None
         )
 
-    def prepare_to_delete_group(self):
+    def prepare_to_delete_group(self, roles):
         """Generate relations to delete."""
         if not self.replication_enabled():
             return
-        roles = Role.objects.filter(policies__group=self.group)
 
         system_roles = roles.public_tenant_only()
 
@@ -211,3 +214,7 @@ def _default_binding(self, mapping: Optional[TenantMapping] = None) -> Relations
             str(mapping.default_role_binding_uuid),
             "binding",
         )
+
+    def set_expected_empty_relation_reason(self, reason):
+        """Set expected empty relation reason."""
+        self._expected_empty_relation_reason = reason

rbac/management/group/view.py

@@ -35,7 +35,6 @@
     remove_roles,
     set_system_flag_before_update,
 )
-from management.group.model import Group
 from management.group.relation_api_dual_write_group_handler import (
     RelationApiDualWriteGroupHandler,
 )
@@ -47,7 +46,7 @@
     GroupSerializer,
     RoleMinimumSerializer,
 )
-from management.models import AuditLog
+from management.models import AuditLog, Group, Role
 from management.notifications.notification_handlers import (
     group_obj_change_notification_handler,
     group_principal_change_notification_handler,
@@ -399,7 +398,16 @@ def destroy(self, request, *args, **kwargs):
                 self.protect_group_with_user_access_admin_role(group.roles_with_access(), "remove_group")
 
             dual_write_handler = RelationApiDualWriteGroupHandler(group, ReplicationEventType.DELETE_GROUP)
-            dual_write_handler.prepare_to_delete_group()
+            roles = Role.objects.filter(policies__group=group)
+            if not group.platform_default and group.principals.exists() and not roles.exists():
+                expected_empty_relation_reason = (
+                    f"No principal or role found for group({group.uuid}): '{group.name}'. "
+                    "Assuming no current relations exist. "
+                    f"event_type='{ReplicationEventType.DELETE_GROUP}'",
+                )
+                dual_write_handler.set_expected_empty_relation_reason(expected_empty_relation_reason)
+            else:
+                dual_write_handler.prepare_to_delete_group(roles)
 
             response = super().destroy(request=request, args=args, kwargs=kwargs)
 

rbac/management/role/model.py

@@ -182,6 +182,10 @@ def remove_user_from_bindings(self, user_id: str) -> Optional[Relationship]:
         """Remove user from mappings."""
         self.mappings["users"].remove(user_id)
         if user_id in self.mappings["users"]:
+            logging.info(
+                f"[Dual Write] user {user_id} still in mappings of bindingmapping {self.pk}, "
+                "therefore, no relation to remove. "
+            )
             return None
         return role_binding_user_subject_tuple(self.mappings["id"], user_id)
 

rbac/management/role/relation_api_dual_write_handler.py

@@ -49,6 +49,8 @@ class BaseRelationApiDualWriteHandler(ABC):
     # TODO: continue factoring common behavior into this base class, and potentially into a higher base class
     # for the general pattern
 
+    _expected_empty_relation_reason = None
+
     def __init__(self, replicator: Optional[RelationReplicator] = None):
         """Initialize SeedingRelationApiDualWriteHandler."""
         if not self.replication_enabled():
@@ -60,6 +62,10 @@ def replication_enabled(self):
         """Check whether replication enabled."""
         return settings.REPLICATION_TO_RELATION_ENABLED is True
 
+    def set_expected_empty_relation_reason_to_replicator(self, reason: str):
+        """Set expected empty relation reason to replicator."""
+        self._expected_empty_relation_reason = reason
+
 
 class SeedingRelationApiDualWriteHandler(BaseRelationApiDualWriteHandler):
     """Class to handle Dual Write API related operations specific to the seeding process."""
@@ -280,11 +286,17 @@ def replicate_deleted_role(self):
         """Replicate removal of current role state."""
         if not self.replication_enabled():
             return
+
         self._replicate()
 
     def _replicate(self):
         if not self.replication_enabled():
             return
+
+        if self._expected_empty_relation_reason:
+            logger.info(f"[Dual Write] Skipping empty replication event. {self._expected_empty_relation_reason}")
+            return
+
         try:
             self._replicator.replicate(
                 ReplicationEvent(

rbac/management/role/view.py

@@ -479,6 +479,14 @@ def perform_create(self, serializer):
         role = serializer.save()
 
         dual_write_handler = RelationApiDualWriteHandler(role, ReplicationEventType.CREATE_CUSTOM_ROLE)
+        # No need to replicate if creating role with empty access, which won't have any relationships
+        if not role.access.all():
+            expected_empty_relation_reason = (
+                f"No access found for role({role.uuid}): '{role.name}'. "
+                "Assuming no relations to create. "
+                f"event_type='{ReplicationEventType.CREATE_CUSTOM_ROLE}'"
+            )
+            dual_write_handler.set_expected_empty_relation_reason_to_replicator(expected_empty_relation_reason)
         dual_write_handler.replicate_new_or_updated_role(role)
 
         role_obj_change_notification_handler(role, "created", self.request.user)
@@ -520,7 +528,16 @@ def perform_destroy(self, instance: Role):
             raise serializers.ValidationError(error)
 
         dual_write_handler = RelationApiDualWriteHandler(instance, ReplicationEventType.DELETE_CUSTOM_ROLE)
-        dual_write_handler.prepare_for_update()
+        # Check emptiness
+        if not instance.access.all():
+            expected_empty_relation_reason = (
+                f"No access found for role({instance.uuid}): '{instance.name}'. "
+                "Assuming no relations to create. "
+                f"event_type='{ReplicationEventType.DELETE_CUSTOM_ROLE}'"
+            )
+            dual_write_handler.set_expected_empty_relation_reason_to_replicator(expected_empty_relation_reason)
+        else:
+            dual_write_handler.prepare_for_update()
 
         self.delete_policies_if_no_role_attached(instance)
         instance.delete()

tests/management/group/test_view.py

@@ -2288,6 +2288,20 @@ def test_add_group_multiple_roles_not_found_success(self):
         self.assertCountEqual([self.roleB], list(groupC.roles()))
         self.assertEqual(response.status_code, status.HTTP_200_OK)
 
+    @patch("management.group.relation_api_dual_write_subject_handler.OutboxReplicator.replicate")
+    def test_add_group_role_not_found_will_not_replicate(self, replicate_mock):
+        """Test that adding roles to a group skips ids not found, and returns success."""
+        groupC = Group.objects.create(name="groupC", tenant=self.tenant)
+        url = reverse("v1_management:group-roles", kwargs={"uuid": groupC.uuid})
+        client = APIClient()
+        test_data = {"roles": [self.dummy_role_id]}
+
+        response = client.post(url, test_data, format="json", **self.headers)
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertCountEqual([], list(groupC.roles()))
+        replicate_mock.assert_not_called()
+
     def test_remove_group_roles_success(self):
         """Test that removing a role from a group returns successfully."""
         url = reverse("v1_management:group-roles", kwargs={"uuid": self.group.uuid})

tests/management/role/test_dual_write.py

@@ -51,6 +51,7 @@
 
 
 from api.models import Tenant
+from unittest.mock import patch
 
 
 @override_settings(REPLICATION_TO_RELATION_ENABLED=True)
@@ -214,7 +215,8 @@ def given_group_removed(self, group: Group):
             ReplicationEventType.DELETE_GROUP,
             replicator=InMemoryRelationReplicator(self.tuples),
         )
-        dual_write_handler.prepare_to_delete_group()
+        roles = Role.objects.filter(policies__group=group)
+        dual_write_handler.prepare_to_delete_group(roles)
         group.delete()
         dual_write_handler.replicate()
 
@@ -846,6 +848,12 @@ def test_delete_role(self):
         """Delete the role and its bindings when deleting a custom role."""
         pass
 
+    @patch("management.role.relation_api_dual_write_handler.OutboxReplicator.replicate")
+    def test_create_role_with_empty_access(self, replicate_mock):
+        """Create a role and its bindings when creating a custom role."""
+        self.given_v1_role("role_without_access", [])
+        replicate_mock.assert_not_called()
+
     def test_remove_resource_removes_role_binding(self):
         """Remove the role binding when removing the resource from attribute filter."""
         role = self.given_v1_role(

tests/management/role/test_view.py

@@ -252,7 +252,7 @@ def create_role(self, role_name, role_display="", in_access_data=None):
             },
             {"permission": "app:*:read", "resourceDefinitions": []},
         ]
-        if in_access_data:
+        if in_access_data is not None:
             access_data = in_access_data
         test_data = {"name": role_name, "display_name": role_display, "access": access_data}
 
@@ -1665,6 +1665,22 @@ def test_delete_system_role_in_public_tenant(self):
         response = client.get(url, **self.headers)
         self.assertEqual(response.status_code, status.HTTP_200_OK)
 
+    @patch("management.role.relation_api_dual_write_handler.OutboxReplicator.replicate")
+    def test_delete_custom_role_without_bindingmappins(self, replicate_mock):
+        role_name = "role_without_bindingmapping"
+        access_data = []
+        response = self.create_role(role_name, in_access_data=access_data)
+        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+        replicate_mock.reset_mock()
+        role_uuid = response.data.get("uuid")
+        url = reverse("v1_management:role-detail", kwargs={"uuid": role_uuid})
+        client = APIClient()
+        response = client.delete(url, **self.headers)
+        self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT)
+
+        replicate_mock.assert_not_called()
+
     def test_update_admin_default_role(self):
         """Test that admin default roles are protected from deletion"""