AAP-52836 Fix RoleDefinition reverse-sync setting permissions to empty list (#833)

## Description
This fixes a bug where the reverse-sync created items in Gateway with
`"permissions": []` on creation, regardless of what the permissions
were. On modification, they were set to before-change.

The tests capture the reproducer for this, and were failing before the
code change.

Left manual verification documentation in related Jira.

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_role_definition, maybe_reverse_sync_unassignment
 from .queries import assignment_qs_user_to_obj, assignment_qs_user_to_obj_perm
 
 
@@ -120,9 +120,26 @@ def _error_if_managed(self, instance):
         if instance.managed is True:
             raise ValidationError(_('Role is managed by the system'))
 
+    def perform_create(self, serializer):
+        from ansible_base.resource_registry.signals.handlers import no_reverse_sync
+
+        with no_reverse_sync():
+            super().perform_create(serializer)
+
+        # Manually sync after permissions are fully saved
+        maybe_reverse_sync_role_definition(serializer.instance, "create")
+
     def perform_update(self, serializer):
+        from ansible_base.resource_registry.signals.handlers import no_reverse_sync
+
         self._error_if_managed(serializer.instance)
-        return super().perform_update(serializer)
+
+        with no_reverse_sync():
+            super().perform_update(serializer)
+
+        # Manually sync after permissions are fully saved
+        serializer.instance.refresh_from_db()
+        maybe_reverse_sync_role_definition(serializer.instance, "update")
 
     def perform_destroy(self, instance):
         self._error_if_managed(instance)

ansible_base/rbac/sync.py

@@ -3,32 +3,41 @@
 
 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
 
-def reverse_sync_enabled_all_conditions(assignment):
+logger = logging.getLogger('ansible_base.rbac.sync')
+
+
+def reverse_sync_enabled_all_conditions(instance):
     """This checks for basically all cases we do not reverse sync
-    1. object level flag for skipping the sync
-    2. environment variable to skip sync
-    3. context manager to disable sync
-    4. RESOURCE_SERVER setting not actually set
+    1. global reverse sync enabled flag (including context manager)
+    2. RESOURCE_SERVER setting not actually set
+    3. environment variable to skip sync
+    4. object level flag for skipping the sync
     """
     from ansible_base.resource_registry.apps import _should_reverse_sync
     from ansible_base.resource_registry.signals.handlers import reverse_sync_enabled
     from ansible_base.resource_registry.utils.sync_to_resource_server import should_skip_reverse_sync
 
-    if not _should_reverse_sync():
+    if not reverse_sync_enabled.enabled:
         return False
 
-    if not reverse_sync_enabled.enabled:
+    if not _should_reverse_sync():
         return False
 
-    if should_skip_reverse_sync(assignment):
-        return
+    if should_skip_reverse_sync(instance):
+        return False
 
     return True
 
@@ -51,3 +60,26 @@ 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 maybe_reverse_sync_role_definition(instance, action="update"):
+    """Manually trigger reverse-sync for a RoleDefinition if appropriate.
+
+    This should be called after the instance is fully constructed with
+    all many-to-many relationships saved.
+
+    Args:
+        instance: The RoleDefinition instance to sync
+        action: The action type ("create" or "update")
+    """
+    if not reverse_sync_enabled_all_conditions(instance):
+        logger.debug(f"Skipping reverse-sync for {instance} (action: {action})")
+        return
+
+    logger.debug(f"Performing reverse-sync for {instance} (action: {action})")
+
+    # Use the same logic as the post_save signal handler
+    from ansible_base.resource_registry.signals.handlers import sync_to_resource_server_post_save
+
+    created = action == "create"
+    sync_to_resource_server_post_save(sender=type(instance), instance=instance, created=created, update_fields=None)

test_app/tests/rbac/test_roledefinition_reverse_sync.py

@@ -0,0 +1,72 @@
+# 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
+
+# Make fixture available
+from test_app.tests.resource_registry.conftest import enable_reverse_sync  # noqa: F401
+
+
+@pytest.mark.django_db
+def test_roledefinition_create_with_permissions_reverse_sync(admin_api_client, enable_reverse_sync):  # noqa: F811
+    """
+    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