AAP-58622 Assure that ansible_id is prefered for content_object in all cases (#882)

We had some bugs due to confusion of `object_id` and `object_ansible_id`
in internal syncing. The only reason to use object_id is if the
ansible_id doesn't exist, and this hopes to make that more consistent
and fix the bugs we have seen.

Author: AlanCoding

ansible_base/rbac/service_api/serializers.py

@@ -91,7 +91,8 @@ def validate(self, attrs):
             if not self.partial and not has_object_id and not has_object_ansible_id:
                 raise serializers.ValidationError("You must provide either 'object_id' or 'object_ansible_id'.")
             # If object_ansible_id was provided and converted, use that for object_id
-            if has_object_ansible_id and not has_object_id:
+            # Prioritize object_ansible_id when both are provided (defensive behavior)
+            if has_object_ansible_id:
                 attrs['object_id'] = attrs['object_ansible_id']
         else:
             if has_object_id or has_object_ansible_id:

ansible_base/resource_registry/rest_client.py

@@ -202,7 +202,15 @@ def sync_assignment(self, assignment):
         else:
             serializer = ServiceRoleTeamAssignmentSerializer(assignment)
 
-        return self._sync_assignment(serializer.data)
+        data = serializer.data
+
+        # Remove object_id if object_ansible_id is present to avoid sending both
+        # For registered objects: send only object_ansible_id
+        # For non-registered objects: send only object_id
+        if data.get('object_ansible_id') is not None:
+            data.pop('object_id', None)
+
+        return self._sync_assignment(data)
 
     def sync_unassignment(self, role_definition, actor, content_object):
         _check_rbac_installed()

test_app/tests/rbac/remote/test_service_api.py

@@ -493,6 +493,116 @@ def test_serializer_allows_null_values_in_validation(self, admin_api_client, ran
         assert 'created_by' not in validated_data or validated_data.get('created_by') is None
 
 
+@pytest.mark.django_db
+class TestRestClientSyncAssignment:
+    """
+    Test that rest_client.sync_assignment only sends the appropriate ID field.
+
+    For objects registered with resource registry: send only object_ansible_id
+    For objects NOT registered: send only object_id
+
+    Generated by Claude Sonnet 4.5
+    """
+
+    def test_sync_assignment_sends_only_object_ansible_id_for_registered_objects(self, rando, organization, org_admin_rd):
+        """Test that sync_assignment removes object_id when object_ansible_id is present"""
+        from unittest.mock import MagicMock, patch
+
+        from ansible_base.resource_registry.rest_client import ResourceAPIClient
+
+        # Create an assignment to an organization (which has a resource)
+        assignment = org_admin_rd.give_permission(rando, organization)
+
+        # Create a mock client
+        client = ResourceAPIClient(service_url='http://example.com', service_path='/api/v1/service-index/')
+
+        # Mock the _sync_assignment method to capture what data is sent
+        with patch.object(client, '_sync_assignment', return_value=MagicMock()) as mock_sync:
+            # Call sync_assignment
+            client.sync_assignment(assignment)
+
+            # Verify _sync_assignment was called
+            assert mock_sync.called
+            sent_data = mock_sync.call_args[0][0]
+
+            # Should have object_ansible_id
+            assert 'object_ansible_id' in sent_data
+            assert sent_data['object_ansible_id'] == str(organization.resource.ansible_id)
+
+            # Should NOT have object_id (removed by sync_assignment)
+            assert 'object_id' not in sent_data, "object_id should not be sent for registered objects"
+
+    def test_sync_assignment_sends_only_object_id_for_non_registered_objects(self, rando, inventory, inv_rd):
+        """Test that sync_assignment keeps object_id when object_ansible_id is None"""
+        from unittest.mock import MagicMock, patch
+
+        from ansible_base.resource_registry.rest_client import ResourceAPIClient
+
+        # Create an assignment to an inventory (which doesn't have a resource)
+        assignment = inv_rd.give_permission(rando, inventory)
+
+        # Create a mock client
+        client = ResourceAPIClient(service_url='http://example.com', service_path='/api/v1/service-index/')
+
+        # Mock the _sync_assignment method to capture what data is sent
+        with patch.object(client, '_sync_assignment', return_value=MagicMock()) as mock_sync:
+            # Call sync_assignment
+            client.sync_assignment(assignment)
+
+            # Verify _sync_assignment was called
+            assert mock_sync.called
+            sent_data = mock_sync.call_args[0][0]
+
+            # Should have object_id
+            assert 'object_id' in sent_data
+            assert sent_data['object_id'] == str(inventory.id)
+
+            # object_ansible_id should either be absent or None
+            assert sent_data.get('object_ansible_id') is None
+
+
+@pytest.mark.django_db
+class TestObjectIdVsAnsibleId:
+    """
+    Test server-side defensive behavior: object_ansible_id takes precedence when both are provided.
+
+    This is a defensive measure for the edge case where a client incorrectly sends both fields
+    that point to different objects. In this case, object_ansible_id should win.
+
+    Generated by Claude Sonnet 4.5
+    """
+
+    def test_object_ansible_id_takes_precedence_when_both_differ(self, admin_api_client, rando, org_admin_rd, organization):
+        """
+        Test DESIRED defensive server-side behavior: object_ansible_id wins when both are provided but differ.
+
+        This should not happen in normal operation (the client should only send one), but if it does,
+        object_ansible_id should take precedence since it's the canonical identifier for cross-service sync.
+        """
+        from test_app.models import Organization
+
+        # Create a second organization
+        org2 = Organization.objects.create(name='Other Organization')
+
+        url = get_relative_url('serviceuserassignment-assign')
+
+        # Provide both with valid but different values (simulating a buggy client)
+        # object_id points to org2, object_ansible_id points to organization
+        data = {
+            "role_definition": org_admin_rd.name,
+            "user_ansible_id": str(rando.resource.ansible_id),
+            "object_id": str(org2.id),  # Wrong object
+            "object_ansible_id": str(organization.resource.ansible_id),  # Should take precedence
+        }
+
+        response = admin_api_client.post(url, data=data)
+        assert response.status_code == 201, f"Expected 201 but got {response.status_code}: {response.data}"
+
+        # Verify the assignment was made to organization (from object_ansible_id), not org2
+        assert rando.has_obj_perm(organization, 'change'), "object_ansible_id should take precedence"
+        assert not rando.has_obj_perm(org2, 'change'), "object_id should be ignored when both provided"
+
+
 @pytest.mark.django_db
 class TestValidationErrors:
     """Test validation error cases in service API serializers"""