SCM inventory acceptance and bug updates

* fixed a problem where the SCM last revision was not getting reset
* change the 400 editing error to just reset the last revision
* handle `source_vars` in the same way as custom scripts
* block unallowed `source_vars` in the validator
* hide POST in OPTIONS for inventory source sublist if not allowed

awx/api/serializers.py

@@ -1474,7 +1474,11 @@ def get_related(self, obj):
         return res
 
     def validate_source_vars(self, value):
-        return vars_validate_or_raise(value)
+        ret = vars_validate_or_raise(value)
+        for env_k in parse_yaml_or_json(value):
+            if env_k in settings.INV_ENV_VARIABLE_BLACKLIST:
+                raise serializers.ValidationError(_("`{}` is a prohibited environment variable".format(env_k)))
+        return ret
 
     def validate(self, attrs):
         # TODO: Validate source, validate source_regions

awx/api/views.py

@@ -1120,7 +1120,7 @@ class ProjectSchedulesList(SubListCreateAPIView):
     new_in_148 = True
 
 
-class ProjectScmInventorySources(SubListCreateAPIView):
+class ProjectScmInventorySources(SubListAPIView):
 
     view_name = _("Project SCM Inventory Sources")
     model = InventorySource
@@ -1689,32 +1689,31 @@ def get_queryset(self):
 
 class ControlledByScmMixin(object):
     '''
-    Special method to lock-down items managed by SCM inventory source via
-    project update, which are not protected by the task manager.
+    Special method to reset SCM inventory commit hash
+    if anything that it manages changes.
     '''
 
-    def _raise_if_unallowed(self, obj):
-        if (self.request.method not in SAFE_METHODS and obj and
-                obj.inventory_sources.filter(
-                    update_on_project_update=True, source='scm').exists()):
+    def _reset_inv_src_rev(self, obj):
+        if self.request.method in SAFE_METHODS or not obj:
+            return
+        project_following_sources = obj.inventory_sources.filter(
+            update_on_project_update=True, source='scm')
+        if project_following_sources:
             # Allow inventory changes unrelated to variables
             if self.model == Inventory and (
                     not self.request or not self.request.data or
                     parse_yaml_or_json(self.request.data.get('variables', '')) == parse_yaml_or_json(obj.variables)):
                 return
-            raise PermissionDenied(detail=_(
-                'This object is managed by updates to the project '
-                'of its inventory source. Remove the inventory source '
-                'in order to make this edit.'))
+            project_following_sources.update(scm_last_revision='')
 
     def get_object(self):
         obj = super(ControlledByScmMixin, self).get_object()
-        self._raise_if_unallowed(obj)
+        self._reset_inv_src_rev(obj)
         return obj
 
     def get_parent_object(self):
         obj = super(ControlledByScmMixin, self).get_parent_object()
-        self._raise_if_unallowed(obj)
+        self._reset_inv_src_rev(obj)
         return obj
 
 
@@ -2288,6 +2287,8 @@ class InventoryInventorySourcesList(SubListCreateAPIView):
     model = InventorySource
     serializer_class = InventorySourceSerializer
     parent_model = Inventory
+    # Sometimes creation blocked by SCM inventory source restrictions
+    always_allow_superuser = False
     relationship = 'inventory_sources'
     parent_key = 'inventory'
     new_in_14 = True
@@ -2297,6 +2298,7 @@ class InventorySourceList(ListCreateAPIView):
 
     model = InventorySource
     serializer_class = InventorySourceSerializer
+    always_allow_superuser = False
     new_in_14 = True
 
 

awx/main/access.py

@@ -376,8 +376,6 @@ def get_method_capability(self, method, obj, parent_obj):
             return access_method(obj)
         elif method in ['start']:
             return self.can_start(obj, validate_license=False)
-        elif method in ['add']: # 2 args with data
-            return self.can_add({})
         elif method in ['attach', 'unattach']: # parent/sub-object call
             access_method = getattr(self, "can_%s" % method)
             if type(parent_obj) == Team:
@@ -739,21 +737,25 @@ def can_read(self, obj):
         else:
             return False
 
-    @check_superuser
     def can_add(self, data):
         if not data or 'inventory' not in data:
             return False
-        if not self.check_related('source_project', Project, data, role_field='admin_role'):
+        if not self.check_related('source_project', Project, data, role_field='use_role'):
             return False
         # Checks for admin or change permission on inventory.
-        return self.check_related('inventory', Inventory, data)
+        return (
+            self.check_related('inventory', Inventory, data) and
+            not InventorySource.objects.filter(
+                inventory=data.get('inventory'),
+                update_on_project_update=True, source='scm').exists())
 
     def can_change(self, obj, data):
         # Checks for admin or change permission on group.
         if obj and obj.inventory:
             return (
                 self.user.can_access(Inventory, 'change', obj.inventory, None) and
-                self.check_related('credential', Credential, data, obj=obj, role_field='use_role')
+                self.check_related('credential', Credential, data, obj=obj, role_field='use_role') and
+                self.check_related('source_project', Project, data, obj=obj, role_field='use_role')
             )
         # Can't change inventory sources attached to only the inventory, since
         # these are created automatically from the management command.

awx/main/models/inventory.py

@@ -1182,9 +1182,9 @@ def save(self, *args, **kwargs):
             before_is = self.__class__.objects.get(pk=self.pk)
             if before_is.source_path != self.source_path or before_is.source_project_id != self.source_project_id:
                 # Reset the scm_revision if file changed to force update
-                self.scm_revision = None
-                if 'scm_revision' not in update_fields:
-                    update_fields.append('scm_revision')
+                self.scm_last_revision = ''
+                if 'scm_last_revision' not in update_fields:
+                    update_fields.append('scm_last_revision')
 
         # Do the actual save.
         super(InventorySource, self).save(*args, **kwargs)

awx/main/tasks.py

@@ -1723,10 +1723,7 @@ def build_env(self, inventory_update, **kwargs):
             env['FOREMAN_INI_PATH'] = cloud_credential
         elif inventory_update.source == 'cloudforms':
             env['CLOUDFORMS_INI_PATH'] = cloud_credential
-        elif inventory_update.source == 'scm':
-            # Parse source_vars to dict, update env.
-            env.update(parse_yaml_or_json(inventory_update.source_vars))
-        elif inventory_update.source == 'custom':
+        elif inventory_update.source in ['scm', 'custom']:
             for env_k in inventory_update.source_vars_dict:
                 if str(env_k) not in env and str(env_k) not in settings.INV_ENV_VARIABLE_BLACKLIST:
                     env[str(env_k)] = unicode(inventory_update.source_vars_dict[env_k])

awx/main/tests/functional/api/test_inventory.py

@@ -1,7 +1,10 @@
 import pytest
+import mock
 
 from awx.api.versioning import reverse
 
+from awx.main.models import InventorySource
+
 
 @pytest.mark.django_db
 def test_inventory_source_notification_on_cloud_only(get, post, inventory_source_factory, user, notification_template):
@@ -199,11 +202,23 @@ def test_inventory_source_update(post, inventory_source, alice, role_field, expe
     post(reverse('api:inventory_source_update_view', kwargs={'pk': inventory_source.id}), {}, alice, expect=expected_status_code)
 
 
+@pytest.mark.django_db
+def test_inventory_source_vars_prohibition(post, inventory, admin_user):
+    with mock.patch('awx.api.serializers.settings') as mock_settings:
+        mock_settings.INV_ENV_VARIABLE_BLACKLIST = ('FOOBAR',)
+        r = post(reverse('api:inventory_source_list'),
+                 {'name': 'new inv src', 'source_vars': '{\"FOOBAR\": \"val\"}', 'inventory': inventory.pk},
+                 admin_user, expect=400)
+    assert 'prohibited environment variable' in r.data['source_vars'][0]
+    assert 'FOOBAR' in r.data['source_vars'][0]
+
+
 @pytest.fixture
 def scm_inventory(inventory, project):
-    inventory.inventory_sources.create(
-        name='foobar', update_on_project_update=True, source='scm',
-        source_project=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
 
 
@@ -216,37 +231,43 @@ class TestControlledBySCM:
     def test_safe_method_works(self, get, options, scm_inventory, admin_user):
         get(scm_inventory.get_absolute_url(), admin_user, expect=200)
         options(scm_inventory.get_absolute_url(), admin_user, expect=200)
+        assert InventorySource.objects.get(inventory=scm_inventory.pk).scm_last_revision != ''
 
-    def test_vars_edit_prohibited(self, patch, scm_inventory, admin_user):
+    def test_vars_edit_reset(self, patch, scm_inventory, admin_user):
         patch(scm_inventory.get_absolute_url(), {'variables': 'hello: world'},
-              admin_user, expect=403)
+              admin_user, expect=200)
+        assert InventorySource.objects.get(inventory=scm_inventory.pk).scm_last_revision == ''
 
     def test_name_edit_allowed(self, patch, scm_inventory, admin_user):
         patch(scm_inventory.get_absolute_url(), {'variables': '---', 'name': 'newname'},
               admin_user, expect=200)
+        assert InventorySource.objects.get(inventory=scm_inventory.pk).scm_last_revision != ''
 
-    def test_host_associations_prohibited(self, post, scm_inventory, admin_user):
+    def test_host_associations_reset(self, post, scm_inventory, admin_user):
         inv_src = scm_inventory.inventory_sources.first()
         h = inv_src.hosts.create(name='barfoo', inventory=scm_inventory)
         g = inv_src.groups.create(name='fooland', inventory=scm_inventory)
         post(reverse('api:host_groups_list', kwargs={'pk': h.id}), {'id': g.id},
-             admin_user, expect=403)
+             admin_user, expect=204)
         post(reverse('api:group_hosts_list', kwargs={'pk': g.id}), {'id': h.id},
-             admin_user, expect=403)
+             admin_user, expect=204)
+        assert InventorySource.objects.get(inventory=scm_inventory.pk).scm_last_revision == ''
 
-    def test_group_group_associations_prohibited(self, post, scm_inventory, admin_user):
+    def test_group_group_associations_reset(self, post, scm_inventory, admin_user):
         inv_src = scm_inventory.inventory_sources.first()
         g1 = inv_src.groups.create(name='barland', inventory=scm_inventory)
         g2 = inv_src.groups.create(name='fooland', inventory=scm_inventory)
         post(reverse('api:group_children_list', kwargs={'pk': g1.id}), {'id': g2.id},
-             admin_user, expect=403)
+             admin_user, expect=204)
+        assert InventorySource.objects.get(inventory=scm_inventory.pk).scm_last_revision == ''
 
-    def test_host_group_delete_prohibited(self, delete, scm_inventory, admin_user):
+    def test_host_group_delete_reset(self, delete, scm_inventory, admin_user):
         inv_src = scm_inventory.inventory_sources.first()
         h = inv_src.hosts.create(name='barfoo', inventory=scm_inventory)
         g = inv_src.groups.create(name='fooland', inventory=scm_inventory)
-        delete(h.get_absolute_url(), admin_user, expect=403)
-        delete(g.get_absolute_url(), admin_user, expect=403)
+        delete(h.get_absolute_url(), admin_user, expect=204)
+        delete(g.get_absolute_url(), admin_user, expect=204)
+        assert InventorySource.objects.get(inventory=scm_inventory.pk).scm_last_revision == ''
 
     def test_remove_scm_inv_src(self, delete, scm_inventory, admin_user):
         inv_src = scm_inventory.inventory_sources.first()
@@ -255,4 +276,14 @@ def test_remove_scm_inv_src(self, delete, scm_inventory, admin_user):
 
     def test_adding_inv_src_prohibited(self, post, scm_inventory, admin_user):
         post(reverse('api:inventory_inventory_sources_list', kwargs={'pk': scm_inventory.id}),
-             {'name': 'new inv src'}, admin_user, expect=400)
+             {'name': 'new inv src'}, admin_user, expect=403)
+
+    def test_adding_inv_src_without_proj_access_prohibited(self, post, project, inventory, rando):
+        inventory.admin_role.members.add(rando)
+        post(reverse('api:inventory_inventory_sources_list', kwargs={'pk': inventory.id}),
+             {'name': 'new inv src', 'source_project': project.pk}, rando, expect=403)
+
+    def test_no_post_in_options(self, options, scm_inventory, admin_user):
+        r = options(reverse('api:inventory_inventory_sources_list', kwargs={'pk': scm_inventory.id}),
+                    admin_user, expect=200)
+        assert 'POST' not in r.data['actions']

awx/main/tests/functional/conftest.py

@@ -241,13 +241,17 @@ def inventory(organization):
 
 @pytest.fixture
 def scm_inventory_source(inventory, project):
-    return InventorySource.objects.create(
+    inv_src = InventorySource(
         name="test-scm-inv",
         source_project=project,
         source='scm',
         source_path='inventory_file',
         update_on_project_update=True,
-        inventory=inventory)
+        inventory=inventory,
+        scm_last_revision=project.scm_revision)
+    with mock.patch.object(inv_src.source_project, 'update'):
+        inv_src.save()
+    return inv_src
 
 
 @pytest.fixture

awx/main/tests/functional/models/test_inventory.py

@@ -23,6 +23,13 @@ def test_automatic_project_update_on_create(self, inventory, project):
             inv_src.save()
             mck_update.assert_called_once_with()
 
+    def test_reset_scm_revision(self, scm_inventory_source):
+        starting_rev = scm_inventory_source.scm_last_revision
+        assert starting_rev != ''
+        scm_inventory_source.source_path = '/newfolder/newfile.ini'
+        scm_inventory_source.save()
+        assert scm_inventory_source.scm_last_revision == ''
+
     def test_source_location(self, scm_inventory_source):
         # Combines project directory with the inventory file specified
         inventory_update = InventoryUpdate(

awx/main/tests/functional/test_tasks.py

@@ -36,6 +36,7 @@ def test_no_unwanted_dependent_inventory_updates(self, project, scm_revision_fil
 
     def test_dependent_inventory_updates(self, scm_inventory_source):
         task = RunProjectUpdate()
+        scm_inventory_source.scm_last_revision = ''
         proj_update = ProjectUpdate.objects.create(project=scm_inventory_source.source_project)
         with mock.patch.object(RunInventoryUpdate, 'run') as iu_run_mock:
             task._update_dependent_inventories(proj_update, [scm_inventory_source])

docs/scm_inventory.md

@@ -1,7 +1,7 @@
 # SCM Inventory
 
 Users can create inventory sources that use content in the source tree of
-a project as an Ansible inventory file.
+a project as an Ansible inventory source.
 
 ## Usage Details
 
@@ -34,7 +34,7 @@ _unless the scm revision of the project changes_.
 
 ### RBAC
 
-User needs `admin` role to the project in order to use it as a source
+User needs `use` role to the project in order to use it as a source
 project for inventory (this entails permission to run arbitrary scripts).
 To update the project, they need `update` permission to the project,
 even if the update is done indirectly.
@@ -58,6 +58,15 @@ inside of an inventory all update with a single button click. When this
 happens for an inventory containing an SCM inventory source, it should
 update the project.
 
+### Inventory Source Restriction
+
+Since automatic inventory updates (triggered by a project update) do not
+go through the task system, typical protection against conflicting updates
+is not available. To avoid problems, only 1 inventory source is allowed for
+inventories that use this feature. That means that if an inventory source
+has `source=scm` and `update_on_project_update=true`, it can be the only
+inventory source for its inventory.
+
 ## Supported File Syntax
 
 > Any Inventory Ansible supports should be supported by this feature
@@ -73,6 +82,25 @@ https://github.com/ansible/ansible-inventory-backport
 Because the internal mechanism is different, we need some coverage
 testing with Ansible versions pre-2.4 and after.
 
+### Vars Restrictions
+
+When creating any `scm` type inventory source, the `overwrite_vars` field
+must be set to `true`. This should be enforced by API validation and
+the UI should also force this setting.
+
+Why? This is because `ansible-inventory` is planned to
+return group vars at the group-level in its JSON output, but the backported
+script returns them on the host-level. In Ansible 2.4, inventory modules are
+being refactored into plugins, and showing vars on the group-level depends on
+this. Since it is not possible to _also_ backport the inventory module
+refactor to prior Ansible versions, this discrepancy can not be resolved.
+While "flattening" the group vars down to the host-level is functionally
+equivalent, upgrading Ansible would cause an anomaly in variable precedence.
+
+This future variable precedence problem is avoided by forcing overwriting
+of variables until Ansible 2.3 is deprecated, after which all updates
+will consistently utilize group-level variables.
+
 # Acceptance Criteria Use Cases
 
 Some test scenarios to look at:
@@ -126,3 +154,18 @@ until the inventory update is finished.
 
 Note that a failed inventory update does not mark the project as failed.
 
+## Restricting Instance Group to Run Script On
+
+Since SCM inventory sources are running scripts written by people with
+access to the source-control, a user may want to restrict which instance
+groups the inventory update runs on.
+
+If the inventory source is set to update on project update, it will run
+on the same instance (inside of the Tower cluster) as the project update.
+This can be restricted by limiting the instance groups of the organization
+that contains the `source_project` of the inventory source.
+
+If the inventory source is not configured to update on project update,
+then it will inherit the allowed instance groups from its inventory,
+like all other inventory syncs.
+