diff --git a/ansible_base/rbac/api/views.py b/ansible_base/rbac/api/views.py index 4b3c698f1..0b4463cd0 100644 --- a/ansible_base/rbac/api/views.py +++ b/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 diff --git a/test_app/tests/rbac/api/test_rbac_views.py b/test_app/tests/rbac/api/test_rbac_views.py index 7be9d21ba..7fb98a42f 100644 --- a/test_app/tests/rbac/api/test_rbac_views.py +++ b/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