Add changes according to review comments

Author: hsong-rh

ansible_base/rbac/api/views.py

@@ -1,3 +1,4 @@
+import logging
 import uuid
 from collections import OrderedDict
 from typing import Type
@@ -9,7 +10,7 @@
 from django.db.models import Model
 from django.utils.translation import gettext_lazy as _
 from rest_framework import permissions
-from rest_framework.exceptions import APIException, NotFound, PermissionDenied, 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
@@ -43,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"
@@ -195,25 +198,41 @@ def remote_sync_unassignment(self, role_definition, actor, content_object):
     def perform_create(self, serializer):
         # Wrap the entire operation in a single atomic transaction
         # This ensures that if remote sync fails, the local assignment creation is rolled back
-        # eliminating the race condition where assignments briefly exist before manual deletion
+        # 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():
-                super().perform_create(serializer)
+                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)
-            error_data = {"detail": str(exc)}
+
+            # 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:
-                # For any other HTTP error (500) or HTTPError with no response
-                # Raise APIException to get a 500 Internal Server Error
-                raise APIException(error_data) from exc
+                # 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)

test_app/tests/rbac/api/test_rbac_views.py

@@ -5,7 +5,7 @@
 from django.test.utils import override_settings
 
 from ansible_base.lib.utils.response import get_relative_url
-from ansible_base.rbac.models import RoleDefinition, RoleUserAssignment
+from ansible_base.rbac.models import RoleDefinition, RoleTeamAssignment, RoleUserAssignment
 
 
 @pytest.mark.django_db
@@ -203,8 +203,8 @@ def test_role_definitions_post_disabled_by_settings(admin_api_client):
         (400, 400),  # ValidationError
         (401, 403),  # PermissionDenied
         (403, 403),  # PermissionDenied
-        (500, 500),  # Re-raised HTTPError → Internal Server Error
-        (None, 500),  # HTTPError with no response → Internal Server Error
+        (500, 500),  # Re-raised original HTTPError preserves stack trace
+        (None, 500),  # Re-raised HTTPError with no response preserves context
     ],
 )
 @pytest.mark.django_db
@@ -228,9 +228,16 @@ def test_user_assignment_remote_sync_error_handling(admin_api_client, inv_rd, ra
     with patch('ansible_base.rbac.api.views.BaseAssignmentViewSet.remote_sync_assignment') as mock_sync:
         mock_sync.side_effect = http_error
 
-        response = admin_api_client.post(url, data=data, format="json")
-        assert response.status_code == expected_status
-        assert 'detail' in response.data
+        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
@@ -260,3 +267,75 @@ def test_user_assignment_remote_sync_connection_error(admin_api_client, inv_rd,
         # 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