From 066dc4a5f004d0c7c5b416b9b9da46c0d1c21ba6 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Fri, 23 Jun 2017 15:16:37 -0400 Subject: [PATCH 1/2] Separate Inventory Source API vs RBAC testing Introduced new decorator for patching an access method, does legwork of inserting into access registry so that tests that test the API can mock RBAC behavior. --- awx/main/access.py | 2 +- awx/main/tests/conftest.py | 18 +++++++++++++++++ .../tests/functional/api/test_inventory.py | 20 +++++++++---------- .../tests/functional/test_rbac_inventory.py | 15 ++++++++++++++ 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index 7e98d4afcc..59995f3b53 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -95,7 +95,7 @@ def check_user_access(user, model_class, action, *args, **kwargs): continue result = access_method(*args, **kwargs) logger.debug('%s.%s %r returned %r', access_instance.__class__.__name__, - access_method.__name__, args, result) + getattr(access_method, '__name__', 'unknown'), args, result) if result: return result return False diff --git a/awx/main/tests/conftest.py b/awx/main/tests/conftest.py index de1f2bdd97..35f77c1f1d 100644 --- a/awx/main/tests/conftest.py +++ b/awx/main/tests/conftest.py @@ -2,6 +2,8 @@ # Python import time import pytest +import mock +from contextlib import contextmanager from awx.main.tests.factories import ( create_organization, @@ -14,6 +16,22 @@ from awx.main.tests.factories import ( ) +@pytest.fixture +def mock_access(): + @contextmanager + def access_given_class(TowerClass): + try: + mock_instance = mock.MagicMock(__name__='foobar') + MockAccess = mock.MagicMock(return_value=mock_instance) + the_patch = mock.patch.dict('awx.main.access.access_registry', + {TowerClass: [MockAccess]}, clear=False) + the_patch.__enter__() + yield mock_instance + finally: + the_patch.__exit__() + return access_given_class + + @pytest.fixture def job_template_factory(): return create_job_template diff --git a/awx/main/tests/functional/api/test_inventory.py b/awx/main/tests/functional/api/test_inventory.py index 3266c3f27c..66b38910b5 100644 --- a/awx/main/tests/functional/api/test_inventory.py +++ b/awx/main/tests/functional/api/test_inventory.py @@ -210,18 +210,18 @@ def test_delete_inventory_host(delete, host, alice, role_field, expected_status_ delete(reverse('api:host_detail', kwargs={'pk': host.id}), alice, expect=expected_status_code) -@pytest.mark.parametrize("role_field,expected_status_code", [ - (None, 403), - ('admin_role', 202), - ('update_role', 202), - ('adhoc_role', 403), - ('use_role', 403) +# See companion test in tests/functional/test_rbac_inventory.py::test_inventory_source_update +@pytest.mark.parametrize("start_access,expected_status_code", [ + (True, 202), + (False, 403) ]) @pytest.mark.django_db -def test_inventory_source_update(post, inventory_source, alice, role_field, expected_status_code): - if role_field: - getattr(inventory_source.inventory, role_field).members.add(alice) - post(reverse('api:inventory_source_update_view', kwargs={'pk': inventory_source.id}), {}, alice, expect=expected_status_code) +def test_inventory_update_access_called(post, inventory_source, alice, mock_access, start_access, expected_status_code): + with mock_access(InventorySource) as mock_instance: + mock_instance.can_start = mock.MagicMock(return_value=start_access) + post(reverse('api:inventory_source_update_view', kwargs={'pk': inventory_source.id}), + {}, alice, expect=expected_status_code) + mock_instance.can_start.assert_called_once_with(inventory_source) @pytest.mark.django_db diff --git a/awx/main/tests/functional/test_rbac_inventory.py b/awx/main/tests/functional/test_rbac_inventory.py index 7b85e1d44a..99937899a8 100644 --- a/awx/main/tests/functional/test_rbac_inventory.py +++ b/awx/main/tests/functional/test_rbac_inventory.py @@ -93,6 +93,21 @@ def test_inventory_update_org_admin(inventory_update, org_admin): assert access.can_delete(inventory_update) +# See companion test in tests/functional/api/test_inventory.py::test_inventory_update_access_called +@pytest.mark.parametrize("role_field,allowed", [ + (None, False), + ('admin_role', True), + ('update_role', True), + ('adhoc_role', False), + ('use_role', False) +]) +@pytest.mark.django_db +def test_inventory_source_update(inventory_source, alice, role_field, allowed): + if role_field: + getattr(inventory_source.inventory, role_field).members.add(alice) + assert allowed == InventorySourceAccess(alice).can_start(inventory_source), '{} test failed'.format(role_field) + + @pytest.mark.django_db def test_host_access(organization, inventory, group, user, group_factory): other_inventory = organization.inventories.create(name='other-inventory') From 4298d6067929276d2dcb221561b7f519bf479bfa Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Mon, 26 Jun 2017 09:18:55 -0400 Subject: [PATCH 2/2] fix bug in response of inventory update with update_on_project_update --- awx/api/views.py | 28 +++++++----- .../tests/functional/api/test_inventory.py | 45 ++++++++++++++----- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index 65848364cf..0368685e99 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2635,23 +2635,31 @@ class InventorySourceUpdateView(RetrieveAPIView): is_job_start = True new_in_14 = True - def _build_update_response(self, update, request): - if not update: + def _update_dependent_project(self, obj, request): + if not self.request.user or not self.request.user.can_access(Project, 'start', obj.source_project): + raise PermissionDenied(detail=_( + 'You do not have permission to update project `{}`.'.format(obj.source_project.name))) + project_update = obj.source_project.update() + if not project_update: return Response({}, status=status.HTTP_400_BAD_REQUEST) else: - headers = {'Location': update.get_absolute_url(request=request)} - return Response(dict(inventory_update=update.id), - status=status.HTTP_202_ACCEPTED, headers=headers) + headers = {'Location': project_update.get_absolute_url(request=request)} + return Response(dict( + detail=_('Request to update dependent project has been accepted.'), inventory_update=None), + status=status.HTTP_202_ACCEPTED, headers=headers) def post(self, request, *args, **kwargs): obj = self.get_object() if obj.can_update: if obj.source == 'scm' and obj.update_on_project_update: - if not self.request.user or not self.request.user.can_access(Project, 'start', obj.source_project): - raise PermissionDenied(detail=_( - 'You do not have permission to update project `{}`.'.format(obj.source_project.name))) - return self._build_update_response(obj.source_project.update(), request) - return self._build_update_response(obj.update(), request) + return self._update_dependent_project(obj, request) + update = obj.update() + if not update: + return Response({}, status=status.HTTP_400_BAD_REQUEST) + else: + headers = {'Location': update.get_absolute_url(request=request)} + return Response(dict(inventory_update=update.id), + status=status.HTTP_202_ACCEPTED, headers=headers) else: return self.http_method_not_allowed(request, *args, **kwargs) diff --git a/awx/main/tests/functional/api/test_inventory.py b/awx/main/tests/functional/api/test_inventory.py index 66b38910b5..508b5bf079 100644 --- a/awx/main/tests/functional/api/test_inventory.py +++ b/awx/main/tests/functional/api/test_inventory.py @@ -3,7 +3,16 @@ import mock from awx.api.versioning import reverse -from awx.main.models import InventorySource +from awx.main.models import InventorySource, Project, ProjectUpdate + + +@pytest.fixture +def scm_inventory(inventory, project): + with mock.patch.object(project, 'update'): + inventory.inventory_sources.create( + name='foobar', update_on_project_update=True, source='scm', + source_project=project, scm_last_revision=project.scm_revision) + return inventory @pytest.mark.django_db @@ -224,6 +233,31 @@ def test_inventory_update_access_called(post, inventory_source, alice, mock_acce mock_instance.can_start.assert_called_once_with(inventory_source) +@pytest.mark.django_db +class TestUpdateOnProjUpdate: + + def test_no_access_update_denied(self, admin_user, scm_inventory, mock_access, post): + inv_src = scm_inventory.inventory_sources.first() + with mock_access(Project) as mock_access: + mock_access.can_start = mock.MagicMock(return_value=False) + r = post(reverse('api:inventory_source_update_view', kwargs={'pk': inv_src.id}), + {}, admin_user, expect=403) + assert 'You do not have permission to update project' in r.data['detail'] + + def test_no_access_update_allowed(self, admin_user, scm_inventory, mock_access, post): + inv_src = scm_inventory.inventory_sources.first() + inv_src.source_project.scm_type = 'git' + inv_src.source_project.save() + with mock.patch('awx.api.views.InventorySourceUpdateView.get_object') as get_object: + get_object.return_value = inv_src + with mock.patch.object(inv_src.source_project, 'update') as mock_update: + mock_update.return_value = ProjectUpdate(pk=48, id=48) + r = post(reverse('api:inventory_source_update_view', kwargs={'pk': inv_src.id}), + {}, admin_user, expect=202) + assert 'dependent project' in r.data['detail'] + assert not r.data['inventory_update'] + + @pytest.mark.django_db def test_inventory_source_vars_prohibition(post, inventory, admin_user): with mock.patch('awx.api.serializers.settings') as mock_settings: @@ -235,15 +269,6 @@ def test_inventory_source_vars_prohibition(post, inventory, admin_user): assert 'FOOBAR' in r.data['source_vars'][0] -@pytest.fixture -def scm_inventory(inventory, project): - with mock.patch.object(project, 'update'): - inventory.inventory_sources.create( - name='foobar', update_on_project_update=True, source='scm', - source_project=project, scm_last_revision=project.scm_revision) - return inventory - - @pytest.mark.django_db class TestControlledBySCM: '''