Merge pull request #6720 from AlanCoding/scm_inv_update_resp

Accuracy edit for inventory update response when updating project

5 files changed, 97 insertions, 31 deletions

diff --git a/awx/api/views.py b/awx/api/views.py
index e5647791c5..fb5e54565b 100644
--- a/awx/api/views.py
+++ b/awx/api/views.py
@@ -2637,23 +2637,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/access.py b/awx/main/access.py
index 220609a7c4..6ce7600026 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,
@@ -15,6 +17,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 1cd93182f9..831c7afc2d 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
@@ -210,18 +219,43 @@ 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
+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
@@ -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:
     '''
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')