diff --git a/awx/api/serializers.py b/awx/api/serializers.py index cdb334279ce0..08986f476dbb 100644 --- a/awx/api/serializers.py +++ b/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 diff --git a/awx/api/views.py b/awx/api/views.py index c353bf551054..9f01e772eb2b 100644 --- a/awx/api/views.py +++ b/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 diff --git a/awx/main/access.py b/awx/main/access.py index 9030aa846bad..a9a9d995a65c 100644 --- a/awx/main/access.py +++ b/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. diff --git a/awx/main/models/inventory.py b/awx/main/models/inventory.py index 3cd94c344642..cd0863281d92 100644 --- a/awx/main/models/inventory.py +++ b/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) diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 08a0b46bdc7a..80173ed8d121 100644 --- a/awx/main/tasks.py +++ b/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]) diff --git a/awx/main/tests/functional/api/test_inventory.py b/awx/main/tests/functional/api/test_inventory.py index fcddef40d6d6..ddfaa4f562cb 100644 --- a/awx/main/tests/functional/api/test_inventory.py +++ b/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'] diff --git a/awx/main/tests/functional/conftest.py b/awx/main/tests/functional/conftest.py index 777c5a08e86a..050d7a63da3a 100644 --- a/awx/main/tests/functional/conftest.py +++ b/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 diff --git a/awx/main/tests/functional/models/test_inventory.py b/awx/main/tests/functional/models/test_inventory.py index 2b42a30d1c27..11c433fb436f 100644 --- a/awx/main/tests/functional/models/test_inventory.py +++ b/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( diff --git a/awx/main/tests/functional/test_tasks.py b/awx/main/tests/functional/test_tasks.py index ee80e85a7ae7..75278ff6a570 100644 --- a/awx/main/tests/functional/test_tasks.py +++ b/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]) diff --git a/docs/scm_inventory.md b/docs/scm_inventory.md index f03468a95976..b7212cbc7cdf 100644 --- a/docs/scm_inventory.md +++ b/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. +