fix: Add proper HTTP error handling to BaseAssignmentViewSet.perform_create

ansible_base/rbac/api/views.py

@@ -1,14 +1,16 @@
+import logging
 import uuid
 from collections import OrderedDict
 from typing import Type
 
+import requests
 from django.apps import apps
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import transaction
 from django.db.models import Model
 from django.utils.translation import gettext_lazy as _
 from rest_framework import permissions
-from rest_framework.exceptions import NotFound, ValidationError
+from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError
 from rest_framework.generics import GenericAPIView
 from rest_framework.response import Response
 from rest_framework.viewsets import GenericViewSet, ModelViewSet, mixins
@@ -42,6 +44,8 @@
 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
 
+logger = logging.getLogger(__name__)
+
 
 def list_combine_values(data: dict[Type[Model], list[str]]) -> list[str]:
     "Utility method to merge everything in .values() into a single list"
@@ -192,9 +196,43 @@ def remote_sync_unassignment(self, role_definition, actor, content_object):
         maybe_reverse_sync_unassignment(role_definition, actor, content_object)
 
     def perform_create(self, serializer):
-        ret = super().perform_create(serializer)
-        self.remote_sync_assignment(serializer.instance)
-        return ret
+        # Wrap the entire operation in a single atomic transaction
+        # This ensures that if remote sync fails, the local assignment creation is rolled back
+        # automatically. Without this, there was a race condition where:
+        # 1. Local assignment is committed to the database
+        # 2. Remote sync fails
+        # 3. Assignment must be manually deleted, but may be visible to users briefly
+        # The transaction.atomic() ensures step 1 only commits if step 2 succeeds.
+        try:
+            with transaction.atomic():
+                ret = super().perform_create(serializer)
+                self.remote_sync_assignment(serializer.instance)
+            return ret
+        except requests.exceptions.HTTPError as exc:
+            # Transaction has already rolled back automatically
+            # No need to manually delete - assignment was never committed
+            status_code = getattr(exc.response, 'status_code', None)
+
+            # Preserve structured error data from gateway response
+            error_data = {"detail": str(exc)}  # Fallback
+            try:
+                if exc.response and hasattr(exc.response, 'json'):
+                    response_data = exc.response.json()
+                    # Use structured data if available, fall back to string
+                    error_data = response_data if isinstance(response_data, dict) else error_data
+            except (ValueError, AttributeError):
+                pass  # Keep fallback error_data
+
+            if status_code == 400:
+                raise ValidationError(error_data) from exc
+            elif status_code in [401, 403]:
+                raise PermissionDenied(error_data) from exc
+            else:
+                # Add logging for debugging before re-raising
+                logger.error(f"Unexpected HTTP error during remote sync: {exc}")
+                # Re-raise the original HTTPError to preserve full stack trace
+                # This avoids creating a redundant 500 error and maintains debugging context
+                raise
 
     def perform_destroy(self, instance):
         check_can_remove_assignment(self.request.user, instance)
@@ -275,7 +313,6 @@ class RoleTeamAssignmentViewSet(BaseAssignmentViewSet):
 
 
 class RoleUserAssignmentViewSet(BaseAssignmentViewSet):
-
     resource_purpose = "RBAC role grants assigning permissions to users for specific resources"
 
     serializer_class = RoleUserAssignmentSerializer

test_app/tests/rbac/api/test_rbac_views.py

@@ -1,8 +1,11 @@
+from unittest.mock import patch
+
 import pytest
+import requests
 from django.test.utils import override_settings
 
 from ansible_base.lib.utils.response import get_relative_url
-from ansible_base.rbac.models import RoleDefinition
+from ansible_base.rbac.models import RoleDefinition, RoleTeamAssignment, RoleUserAssignment
 
 
 @pytest.mark.django_db
@@ -192,3 +195,147 @@ def test_role_definitions_post_disabled_by_settings(admin_api_client):
     assert response.status_code == 200, response.data
     print(response.data)
     assert 'POST' not in response.data.get('actions', {})
+
+
+@pytest.mark.parametrize(
+    "sync_error_status, expected_status",
+    [
+        (400, 400),  # ValidationError
+        (401, 403),  # PermissionDenied
+        (403, 403),  # PermissionDenied
+        (500, 500),  # Re-raised original HTTPError preserves stack trace
+        (None, 500),  # Re-raised HTTPError with no response preserves context
+    ],
+)
+@pytest.mark.django_db
+def test_user_assignment_remote_sync_error_handling(admin_api_client, inv_rd, rando, inventory, sync_error_status, expected_status):
+    """Test that remote sync HTTP errors are handled correctly for user assignments"""
+    url = get_relative_url('roleuserassignment-list')
+    data = dict(role_definition=inv_rd.id, user=rando.id, object_id=inventory.id)
+
+    # Track initial count to verify transaction rollback
+    initial_count = RoleUserAssignment.objects.count()
+
+    # Mock the remote sync to raise HTTPError with specified status
+    if sync_error_status is not None:
+        mock_response = requests.Response()
+        mock_response.status_code = sync_error_status
+        http_error = requests.exceptions.HTTPError(response=mock_response)
+    else:
+        # Simulate HTTPError with no response (e.g., connection failed)
+        http_error = requests.exceptions.HTTPError(response=None)
+
+    with patch('ansible_base.rbac.api.views.BaseAssignmentViewSet.remote_sync_assignment') as mock_sync:
+        mock_sync.side_effect = http_error
+
+        if sync_error_status in [500, None]:
+            # For 500 errors and HTTPError with no response, we re-raise the original HTTPError
+            # which will be an unhandled exception in the test client
+            with pytest.raises(requests.exceptions.HTTPError):
+                admin_api_client.post(url, data=data, format="json")
+        else:
+            # For 400/401/403 errors, we convert to DRF exceptions with proper HTTP responses
+            response = admin_api_client.post(url, data=data, format="json")
+            assert response.status_code == expected_status
+            assert 'detail' in response.data
+
+        # Verify that no assignment was created due to transaction rollback
+        assert RoleUserAssignment.objects.count() == initial_count
+
+
+@pytest.mark.django_db
+def test_user_assignment_remote_sync_connection_error(admin_api_client, inv_rd, rando, inventory):
+    """Test that connection errors during remote sync cause unhandled exceptions but transaction rollback works"""
+    url = get_relative_url('roleuserassignment-list')
+    data = dict(role_definition=inv_rd.id, user=rando.id, object_id=inventory.id)
+
+    # Track initial count to verify transaction rollback
+    initial_count = RoleUserAssignment.objects.count()
+
+    # Mock the remote sync to raise ConnectionError (no HTTP response)
+    connection_error = requests.exceptions.ConnectionError("Connection failed")
+
+    with patch('ansible_base.rbac.api.views.BaseAssignmentViewSet.remote_sync_assignment') as mock_sync:
+        mock_sync.side_effect = connection_error
+
+        # ConnectionError is not currently caught, so it escapes as an unhandled exception
+        # The Django test client converts this to a 500 response, but we can also test
+        # by catching the exception directly to verify the rollback behavior
+        with pytest.raises(requests.exceptions.ConnectionError):
+            admin_api_client.post(url, data=data, format="json")
+
+        # Verify that no assignment was created due to transaction rollback
+        # This confirms that our transaction.atomic() fix works correctly
+        assert RoleUserAssignment.objects.count() == initial_count
+
+
+@pytest.mark.parametrize(
+    "sync_error_status, expected_status",
+    [
+        (400, 400),  # ValidationError
+        (401, 403),  # PermissionDenied
+        (403, 403),  # PermissionDenied
+        (500, 500),  # Re-raised original HTTPError preserves stack trace
+        (None, 500),  # Re-raised HTTPError with no response preserves context
+    ],
+)
+@pytest.mark.django_db
+def test_team_assignment_remote_sync_error_handling(admin_api_client, inv_rd, team, inventory, sync_error_status, expected_status):
+    """Test that remote sync HTTP errors are handled correctly for team assignments"""
+    url = get_relative_url('roleteamassignment-list')
+    data = dict(role_definition=inv_rd.id, team=team.id, object_id=inventory.id)
+
+    # Track initial count to verify transaction rollback
+    initial_count = RoleTeamAssignment.objects.count()
+
+    # Mock the remote sync to raise HTTPError with specified status
+    if sync_error_status is not None:
+        mock_response = requests.Response()
+        mock_response.status_code = sync_error_status
+        http_error = requests.exceptions.HTTPError(response=mock_response)
+    else:
+        # Simulate HTTPError with no response (e.g., connection failed)
+        http_error = requests.exceptions.HTTPError(response=None)
+
+    with patch('ansible_base.rbac.api.views.BaseAssignmentViewSet.remote_sync_assignment') as mock_sync:
+        mock_sync.side_effect = http_error
+
+        if sync_error_status in [500, None]:
+            # For 500 errors and HTTPError with no response, we re-raise the original HTTPError
+            # which will be an unhandled exception in the test client
+            with pytest.raises(requests.exceptions.HTTPError):
+                admin_api_client.post(url, data=data, format="json")
+        else:
+            # For 400/401/403 errors, we convert to DRF exceptions with proper HTTP responses
+            response = admin_api_client.post(url, data=data, format="json")
+            assert response.status_code == expected_status
+            assert 'detail' in response.data
+
+        # Verify that no assignment was created due to transaction rollback
+        assert RoleTeamAssignment.objects.count() == initial_count
+
+
+@pytest.mark.django_db
+def test_team_assignment_remote_sync_connection_error(admin_api_client, inv_rd, team, inventory):
+    """Test that connection errors during remote sync cause unhandled exceptions but transaction rollback works"""
+    url = get_relative_url('roleteamassignment-list')
+    data = dict(role_definition=inv_rd.id, team=team.id, object_id=inventory.id)
+
+    # Track initial count to verify transaction rollback
+    initial_count = RoleTeamAssignment.objects.count()
+
+    # Mock the remote sync to raise ConnectionError (no HTTP response)
+    connection_error = requests.exceptions.ConnectionError("Connection failed")
+
+    with patch('ansible_base.rbac.api.views.BaseAssignmentViewSet.remote_sync_assignment') as mock_sync:
+        mock_sync.side_effect = connection_error
+
+        # ConnectionError is not currently caught, so it escapes as an unhandled exception
+        # The Django test client converts this to a 500 response, but we can also test
+        # by catching the exception directly to verify the rollback behavior
+        with pytest.raises(requests.exceptions.ConnectionError):
+            admin_api_client.post(url, data=data, format="json")
+
+        # Verify that no assignment was created due to transaction rollback
+        # This confirms that our transaction.atomic() fix works correctly
+        assert RoleTeamAssignment.objects.count() == initial_count