Fix RoleDefinition reverse-sync setting permissions to empty list

Author: AlanCoding

ansible_base/rbac/api/views.py

@@ -38,7 +38,7 @@
 from ..models import DABContentType, DABPermission, get_evaluation_model
 from ..policies import check_content_obj_permission
 from ..remote import RemoteObject, get_resource_prefix
-from ..sync import maybe_reverse_sync_assignment, maybe_reverse_sync_unassignment
+from ..sync import maybe_reverse_sync_assignment, maybe_reverse_sync_unassignment, delayed_reverse_sync
 from .queries import assignment_qs_user_to_obj, assignment_qs_user_to_obj_perm
 
 
@@ -120,6 +120,51 @@ def _error_if_managed(self, instance):
         if instance.managed is True:
             raise ValidationError(_('Role is managed by the system'))
 
+    def create(self, request, *args, **kwargs):
+        """Create a new RoleDefinition with delayed reverse-sync.
+
+        This ensures that the RoleDefinition is fully created with
+        permissions before syncing to the resource server.
+        """
+        serializer = self.get_serializer(data=request.data)
+        serializer.is_valid(raise_exception=True)
+
+        # Use delayed sync to avoid the timing issue where post_save fires
+        # before many-to-many permissions are saved
+        with delayed_reverse_sync(None, "create") as ctx:
+            # Create the instance but disable auto-sync
+            instance = serializer.save()
+            # Update the context manager with the actual instance
+            ctx.set_instance(instance)
+
+        headers = self.get_success_headers(serializer.data)
+        return Response(serializer.data, status=201, headers=headers)
+
+    def update(self, request, *args, **kwargs):
+        """Update a RoleDefinition with delayed reverse-sync.
+
+        This ensures that the RoleDefinition is fully updated with
+        permissions before syncing to the resource server.
+        """
+        partial = kwargs.pop('partial', False)
+        instance = self.get_object()
+        self._error_if_managed(instance)
+
+        serializer = self.get_serializer(instance, data=request.data, partial=partial)
+        serializer.is_valid(raise_exception=True)
+
+        # Use delayed sync to avoid the timing issue where post_save fires
+        # before many-to-many permissions are saved
+        with delayed_reverse_sync(instance, "update"):
+            serializer.save()
+
+        if getattr(instance, '_prefetched_objects_cache', None):
+            # If 'prefetch_related' has been applied to a queryset, we need to
+            # forcibly invalidate the prefetch cache on the instance.
+            instance._prefetched_objects_cache = {}
+
+        return Response(serializer.data)
+
     def perform_update(self, serializer):
         self._error_if_managed(serializer.instance)
         return super().perform_update(serializer)

ansible_base/rbac/sync.py

@@ -3,12 +3,22 @@
 
 ansible_base.resource_registry.utils.sync_to_resource_server.sync_to_resource_server
 
-However, this only deals with role assignments, which have key differences
-- totally immutable model
-- have very weird way of referencing related objects
-- must run various internal RBAC logic for rebuilding RoleEvaluation entries
+This module handles RBAC-specific reverse-sync scenarios:
+1. Role assignments - which have key differences:
+   - totally immutable model
+   - have very weird way of referencing related objects
+   - must run various internal RBAC logic for rebuilding RoleEvaluation entries
+
+2. RoleDefinition sync timing - which has timing issues:
+   - post_save fires before many-to-many relations are saved
+   - permissions need to be attached before syncing
 """
 
+import logging
+from contextlib import contextmanager
+
+logger = logging.getLogger('ansible_base.rbac.sync')
+
 
 def reverse_sync_enabled_all_conditions(assignment):
     """This checks for basically all cases we do not reverse sync
@@ -51,3 +61,88 @@ def maybe_reverse_sync_unassignment(role_definition, actor, content_object):
 
     client = get_current_user_resource_client()
     client.sync_unassignment(role_definition, actor, content_object)
+
+
+def _should_reverse_sync(instance) -> bool:
+    """Check if reverse sync should be performed for the given instance.
+
+    This checks the same conditions as the signal handler but without
+    relying on the global reverse_sync_enabled state being temporarily disabled.
+    """
+    from ansible_base.resource_registry.apps import _should_reverse_sync as _global_should_sync
+    from ansible_base.resource_registry.utils.sync_to_resource_server import should_skip_reverse_sync
+
+    # Check global RESOURCE_SERVER configuration
+    if not _global_should_sync():
+        return False
+
+    # Check instance-specific skip conditions
+    if should_skip_reverse_sync(instance):
+        return False
+
+    return True
+
+
+class DelayedReverseSyncContext:
+    """Context class for delayed reverse sync operations."""
+
+    def __init__(self, action="update"):
+        self.action = action
+        self.instance = None
+
+    def set_instance(self, instance):
+        """Set the instance to sync after the context exits."""
+        self.instance = instance
+
+
+@contextmanager
+def delayed_reverse_sync(instance, action="update"):
+    """Context manager that disables reverse-sync during execution, then manually syncs afterward.
+
+    This is useful for operations where the instance needs to be fully constructed
+    (including many-to-many relationships) before syncing to the resource server.
+
+    This solves the timing issue where post_save fires before many-to-many
+    relations are saved, causing empty permissions to be synced.
+
+    Args:
+        instance: The Django model instance to sync (can be None for create operations)
+        action: The action type ("create" or "update")
+
+    Usage:
+        # For updates
+        with delayed_reverse_sync(role_def, "update"):
+            # Perform operations that modify the instance
+            role_def.permissions.set(permissions)
+        # Sync happens here if appropriate
+
+        # For creates
+        ctx = delayed_reverse_sync(None, "create")
+        with ctx:
+            instance = serializer.save()
+            ctx.set_instance(instance)
+        # Sync happens here if appropriate
+    """
+    from ansible_base.resource_registry.signals.handlers import no_reverse_sync, sync_to_resource_server_post_save
+
+    # Create a context object that can be updated
+    context = DelayedReverseSyncContext(action)
+    context.instance = instance
+
+    with no_reverse_sync():
+        yield context
+
+    # After the context, check if we should sync and do it manually
+    final_instance = context.instance
+    if final_instance and _should_reverse_sync(final_instance):
+        logger.debug(f"Performing delayed reverse-sync for {final_instance} (action: {action})")
+        # Use the same logic as the post_save signal handler
+        created = (action == "create")
+        sync_to_resource_server_post_save(
+            sender=type(final_instance),
+            instance=final_instance,
+            created=created,
+            update_fields=None
+        )
+    else:
+        logger.debug(f"Skipping delayed reverse-sync for {final_instance} (action: {action})")

test_app/tests/rbac/test_roledefinition_reverse_sync.py

@@ -0,0 +1,77 @@
+# Generated by Claude Sonnet 4
+
+import uuid
+from unittest import mock
+
+import pytest
+from django.test.utils import override_settings
+
+from ansible_base.lib.utils.response import get_relative_url
+
+# Import the fixture from resource_registry conftest
+pytest_plugins = ["test_app.tests.resource_registry.conftest"]
+
+
+@pytest.mark.django_db
+def test_roledefinition_create_with_permissions_reverse_sync(admin_api_client, enable_reverse_sync):
+    """
+    Test that creating a RoleDefinition with permissions triggers reverse-sync correctly.
+
+    This test verifies that the delayed reverse-sync fix works properly.
+    With the fix, permissions should be included in the sync data instead of being empty.
+    """
+
+    # Enable reverse-sync and mock the resource server client's _make_request method
+    with enable_reverse_sync():
+        with override_settings(RESOURCE_SERVER={'URL': 'http://example.invalid', 'SECRET_KEY': 'test-secret-key'}):
+            with mock.patch('ansible_base.resource_registry.rest_client.ResourceAPIClient._make_request') as mock_make_request:
+                # Mock successful response from resource server
+                mock_response = mock.Mock()
+                mock_response.status_code = 201
+                mock_response.json.return_value = {
+                    'ansible_id': str(uuid.uuid4()),
+                    'service_id': str(uuid.uuid4())
+                }
+                mock_make_request.return_value = mock_response
+
+                # Create RoleDefinition via API (this should trigger reverse-sync)
+                url = get_relative_url("roledefinition-list")
+                data = {
+                    'name': 'test-reverse-sync-role',
+                    'description': 'Test role for reverse sync',
+                    'permissions': ['shared.view_organization', 'shared.change_organization'],
+                    'content_type': 'shared.organization'
+                }
+
+                response = admin_api_client.post(url, data=data, format="json")
+                assert response.status_code == 201, f"Failed to create role: {response.data}"
+
+                # Verify that _make_request was called (meaning reverse-sync was attempted)
+                assert mock_make_request.called, "Resource server sync should have been called"
+
+                # Find the POST request that creates the resource
+                create_calls = [call for call in mock_make_request.call_args_list
+                            if call[0][0].upper() == 'POST']  # First arg is HTTP method
+
+                assert len(create_calls) >= 1, (
+                    f"Should have at least one POST call to create resource. "
+                    f"Found calls: {[call[0] for call in mock_make_request.call_args_list]}"
+                )
+
+                # Get the request data from the create call
+                create_call = create_calls[0]
+                # The call structure is: (method, url, data)
+                request_data = create_call[0][2]  # Third positional argument is the data
+
+                # Extract the resource_data that would be sent to the resource server
+                resource_data = request_data.get('resource_data', {})
+                permissions_in_sync = resource_data.get('permissions', [])
+
+                # This should now PASS because the delayed sync ensures permissions are included
+                # The fix ensures that permissions are no longer empty during sync
+                expected_permissions = {'shared.view_organization', 'shared.change_organization'}
+                actual_permissions = set(permissions_in_sync)
+                assert actual_permissions == expected_permissions, (
+                    f"Expected permissions {expected_permissions} in reverse-sync data, "
+                    f"but got {actual_permissions}. The delayed sync fix should include permissions!"
+                )

test_app/tests/resource_registry/conftest.py

@@ -2,38 +2,37 @@
 from unittest import mock
 
 import pytest
+from django.test.utils import override_settings
 
 from ansible_base.rbac import permission_registry
 from ansible_base.resource_registry import apps
 from test_app.models import Organization
 
 
 @pytest.fixture
-def enable_reverse_sync(settings):
+def enable_reverse_sync():
     """
     Useful for tests that deal with testing the reverse sync logic
     """
 
     @contextmanager
     def f(mock_away_sync=False):
-        # This is kind of a dance. We don't want to break other tests by
-        # leaving the save method monkeypatched when they are expecting syncing
-        # to be disabled. So we patch the save method, yield, reset
-        # RESOURCE_SERVER_SYNC_ENABLED, undo the patch (disconnect_resource_signals),
-        # and then reconnect signals (so the resource registry stuff still works) but
-        # this time we don't monkeypatch the save method since RESOURCE_SERVER_SYNC_ENABLED
-        # is back to its original value.
-        is_enabled = settings.RESOURCE_SERVER_SYNC_ENABLED
-        settings.RESOURCE_SERVER_SYNC_ENABLED = True
-        apps.connect_resource_signals(sender=None)
-        if mock_away_sync:
-            with mock.patch('ansible_base.resource_registry.utils.sync_to_resource_server.get_resource_server_client'):
-                yield
-        else:
-            yield
-        apps.disconnect_resource_signals(sender=None)
-        settings.RESOURCE_SERVER_SYNC_ENABLED = is_enabled
-        apps.connect_resource_signals(sender=None)
+        # Use Django's override_settings to properly scope the setting change
+        # This avoids the scope issues with directly modifying the settings object
+        try:
+            with override_settings(RESOURCE_SERVER_SYNC_ENABLED=True):
+                # Connect signals with the new setting in effect
+                apps.connect_resource_signals(sender=None)
+                if mock_away_sync:
+                    with mock.patch('ansible_base.resource_registry.utils.sync_to_resource_server.get_resource_server_client'):
+                        yield
+                else:
+                    yield
+        finally:
+            # Clean up signals when exiting the context
+            apps.disconnect_resource_signals(sender=None)
+            # Reconnect signals with the original setting restored
+            apps.connect_resource_signals(sender=None)
 
     return f