From e531bc67e448cb9e2fb2a86107345d19effabeb5 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Thu, 26 May 2016 14:39:16 -0400 Subject: [PATCH 1/4] Better control what JT admins are allowed to do This addresses #1981 which says that JT admins can make modifications to a job template freely if they're just changing non functional things like name, description, forks, verbosity, etc, while requiring them to have access to all functional components if they're going to make any changes to the functionality - in specific, any changes to the inventory, project, playbook, or credentials requires that the user have the appropriate use access on all of those things in order to make the change. --- awx/api/serializers.py | 2 +- awx/main/access.py | 53 ++++++++- awx/main/tests/factories/fixtures.py | 2 +- .../functional/api/test_job_templates.py | 109 ++++++++++++++++++ 4 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 awx/main/tests/functional/api/test_job_templates.py diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 0280b0f09f..ce203c533c 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -1766,7 +1766,7 @@ class JobTemplateSerializer(UnifiedJobTemplateSerializer, JobOptionsSerializer): notification_templates_any = reverse('api:job_template_notification_templates_any_list', args=(obj.pk,)), notification_templates_success = reverse('api:job_template_notification_templates_success_list', args=(obj.pk,)), notification_templates_error = reverse('api:job_template_notification_templates_error_list', args=(obj.pk,)), - access_list = reverse('api:job_template_access_list', args=(obj.pk,)), + access_list = reverse('api:job_template_access_list', args=(obj.pk,)), survey_spec = reverse('api:job_template_survey_spec', args=(obj.pk,)), labels = reverse('api:job_template_label_list', args=(obj.pk,)), roles = reverse('api:job_template_roles_list', args=(obj.pk,)), diff --git a/awx/main/access.py b/awx/main/access.py index 7e2289df42..c58da4bcd9 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -817,13 +817,64 @@ class JobTemplateAccess(BaseAccess): if self.user not in obj.admin_role: return False if data is not None: - data_for_change = dict(data) + data = dict(data) + + if self.changes_are_non_sensitive(obj, data): + if 'job_type' in data and data['job_type'] == PERM_INVENTORY_SCAN: + self.check_license(feature='system_tracking') + + if 'survey_enabled' in data and data['survey_enabled']: + self.check_license(feature='surveys') + print('Changes are non senstivie') + return True + print('Changes were senstivie') + for required_field in ('credential', 'cloud_credential', 'inventory', 'project'): required_obj = getattr(obj, required_field, None) if required_field not in data_for_change and required_obj is not None: data_for_change[required_field] = required_obj.pk return self.can_read(obj) and self.can_add(data_for_change) + def changes_are_non_sensitive(self, obj, data): + ''' + Returne true if the changes being made are considered nonsensitive, and + thus can be made by a job template administrator which may not have access + to the any inventory, project, or credentials associated with the template. + ''' + # We are white listing fields that can + field_whitelist = [ + 'name', 'description', 'forks', 'limit', 'verbosity', 'extra_vars', + 'job_tags', 'force_handlers', 'skip_tags', 'ask_variables_on_launch', + 'ask_tags_on_launch', 'ask_job_type_on_launch', 'ask_inventory_on_launch', + 'ask_credential_on_launch', 'survey_enabled' + ] + + for k, v in data.items(): + if hasattr(obj, k) and getattr(obj, k) != v: + if k not in field_whitelist: + return False + return True + + def can_update_sensitive_fields(self, obj, data): + project_id = data.get('project', obj.project.id if obj.project else None) + inventory_id = data.get('inventory', obj.inventory.id if obj.inventory else None) + credential_id = data.get('credential', obj.credential.id if obj.credential else None) + cloud_credential_id = data.get('cloud_credential', obj.cloud_credential.id if obj.cloud_credential else None) + network_credential_id = data.get('network_credential', obj.network_credential.id if obj.network_credential else None) + + if project_id and self.user not in Project.objects.get(pk=project_id).use_role: + return False + if inventory_id and self.user not in Inventory.objects.get(pk=inventory_id).use_role: + return False + if credential_id and self.user not in Credential.objects.get(pk=credential_id).use_role: + return False + if cloud_credential_id and self.user not in Credential.objects.get(pk=cloud_credential_id).use_role: + return False + if network_credential_id and self.user not in Credential.objects.get(pk=network_credential_id).use_role: + return False + + return True + def can_delete(self, obj): return self.user in obj.admin_role diff --git a/awx/main/tests/factories/fixtures.py b/awx/main/tests/factories/fixtures.py index 91ead460db..7273997ba5 100644 --- a/awx/main/tests/factories/fixtures.py +++ b/awx/main/tests/factories/fixtures.py @@ -107,7 +107,7 @@ def mk_job_template(name, job_type='run', organization=None, inventory=None, credential=None, persisted=True, project=None): - jt = JobTemplate(name=name, job_type=job_type) + jt = JobTemplate(name=name, job_type=job_type, playbook='mocked') jt.inventory = inventory if jt.inventory is None: diff --git a/awx/main/tests/functional/api/test_job_templates.py b/awx/main/tests/functional/api/test_job_templates.py new file mode 100644 index 0000000000..cb09ced593 --- /dev/null +++ b/awx/main/tests/functional/api/test_job_templates.py @@ -0,0 +1,109 @@ +import mock # noqa +import pytest +from awx.main.models.projects import ProjectOptions + + +from django.core.urlresolvers import reverse + +def decorators(func): + @property + def project_playbooks(self): + return ['mocked', 'othermocked'] + + return pytest.mark.django_db(mock.patch.object(ProjectOptions, "playbooks", project_playbooks)(func)) + +@decorators +@pytest.mark.parametrize( + "grant_project, grant_credential, grant_inventory, expect", [ + (True, True, True, 201), + (True, True, False, 403), + (True, False, True, 403), + (False, True, True, 403), + ] +) +def test_create(post, project, machine_credential, inventory, alice, grant_project, grant_credential, grant_inventory, expect): + if grant_project: + project.use_role.members.add(alice) + if grant_credential: + machine_credential.use_role.members.add(alice) + if grant_inventory: + inventory.use_role.members.add(alice) + + post(reverse('api:job_template_list'), { + 'name': 'Some name', + 'project': project.id, + 'credential': machine_credential.id, + 'inventory': inventory.id, + 'playbook': 'mocked', + }, alice, expect=expect) + +@decorators +@pytest.mark.parametrize( + "grant_project, grant_credential, grant_inventory, expect", [ + (True, True, True, 200), + (True, True, False, 403), + (True, False, True, 403), + (False, True, True, 403), + ] +) +def test_edit_sensitive_fields(patch, job_template_factory, alice, grant_project, grant_credential, grant_inventory, expect): + objs = job_template_factory('jt', organization='org1', project='prj', inventory='inv', credential='cred') + objs.job_template.admin_role.members.add(alice) + + if grant_project: + objs.project.use_role.members.add(alice) + if grant_credential: + objs.credential.use_role.members.add(alice) + if grant_inventory: + objs.inventory.use_role.members.add(alice) + + patch(reverse('api:job_template_detail', args=(objs.job_template.id,)), { + 'name': 'Some name', + 'project': objs.project.id, + 'credential': objs.credential.id, + 'inventory': objs.inventory.id, + 'playbook': 'othermocked', + }, alice, expect=expect) + +@decorators +def test_edit_playbook(patch, job_template_factory, alice): + objs = job_template_factory('jt', organization='org1', project='prj', inventory='inv', credential='cred') + objs.job_template.admin_role.members.add(alice) + objs.project.use_role.members.add(alice) + objs.credential.use_role.members.add(alice) + objs.inventory.use_role.members.add(alice) + + patch(reverse('api:job_template_detail', args=(objs.job_template.id,)), { + 'playbook': 'othermocked', + }, alice, expect=200) + + objs.inventory.use_role.members.remove(alice) + patch(reverse('api:job_template_detail', args=(objs.job_template.id,)), { + 'playbook': 'mocked', + }, alice, expect=403) + +@decorators +def test_edit_nonsenstive(patch, job_template_factory, alice): + objs = job_template_factory('jt', organization='org1', project='prj', inventory='inv', credential='cred') + jt = objs.job_template + jt.admin_role.members.add(alice) + + res = patch(reverse('api:job_template_detail', args=(jt.id,)), { + 'name': 'updated', + 'description': 'bar', + 'forks': 14, + 'limit': 'something', + 'verbosity': 5, + 'extra_vars': '--', + 'job_tags': 'sometags', + 'force_handlers': True, + 'skip_tags': True, + 'ask_variables_on_launch':True, + 'ask_tags_on_launch':True, + 'ask_job_type_on_launch':True, + 'ask_inventory_on_launch':True, + 'ask_credential_on_launch': True, + 'survey_enabled':True, + }, alice, expect=200) + print(res.data) + assert res.data['name'] == 'updated' From c5dfde236b66f9ae2c04911f7bef8053f13baab6 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Thu, 26 May 2016 15:18:23 -0400 Subject: [PATCH 2/4] Test "fix" Having this triggers a license feature test, which the jenkins test system apparently doesn't have.. I don't think it's particularly important to test this particular field, so meh. --- awx/main/tests/functional/api/test_job_templates.py | 1 - 1 file changed, 1 deletion(-) diff --git a/awx/main/tests/functional/api/test_job_templates.py b/awx/main/tests/functional/api/test_job_templates.py index cb09ced593..f5ca17fb0d 100644 --- a/awx/main/tests/functional/api/test_job_templates.py +++ b/awx/main/tests/functional/api/test_job_templates.py @@ -103,7 +103,6 @@ def test_edit_nonsenstive(patch, job_template_factory, alice): 'ask_job_type_on_launch':True, 'ask_inventory_on_launch':True, 'ask_credential_on_launch': True, - 'survey_enabled':True, }, alice, expect=200) print(res.data) assert res.data['name'] == 'updated' From f6da30dde3d3b4d3fb3080f7fe0a453bb8bee8be Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Fri, 27 May 2016 09:32:29 -0400 Subject: [PATCH 3/4] Avoid unnecessary license checks --- awx/main/access.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index c58da4bcd9..1674e38786 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -820,14 +820,12 @@ class JobTemplateAccess(BaseAccess): data = dict(data) if self.changes_are_non_sensitive(obj, data): - if 'job_type' in data and data['job_type'] == PERM_INVENTORY_SCAN: + if 'job_type' in data and obj.job_type != data['job_type'] and data['job_type'] == PERM_INVENTORY_SCAN: self.check_license(feature='system_tracking') - if 'survey_enabled' in data and data['survey_enabled']: + if 'survey_enabled' in data and obj.survey_enabled != data['survey_enabled'] and data['survey_enabled']: self.check_license(feature='surveys') - print('Changes are non senstivie') return True - print('Changes were senstivie') for required_field in ('credential', 'cloud_credential', 'inventory', 'project'): required_obj = getattr(obj, required_field, None) From e3fcdf9ba8976f1a8ea8f0b5f1abf1dfb528dcf7 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Fri, 27 May 2016 14:18:05 -0400 Subject: [PATCH 4/4] Unobfuscated some decorators --- .../functional/api/test_job_templates.py | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/awx/main/tests/functional/api/test_job_templates.py b/awx/main/tests/functional/api/test_job_templates.py index f5ca17fb0d..7f20a099ca 100644 --- a/awx/main/tests/functional/api/test_job_templates.py +++ b/awx/main/tests/functional/api/test_job_templates.py @@ -5,14 +5,12 @@ from awx.main.models.projects import ProjectOptions from django.core.urlresolvers import reverse -def decorators(func): - @property - def project_playbooks(self): - return ['mocked', 'othermocked'] +@property +def project_playbooks(self): + return ['mocked.yml', 'alt-mocked.yml'] - return pytest.mark.django_db(mock.patch.object(ProjectOptions, "playbooks", project_playbooks)(func)) - -@decorators +@pytest.mark.django_db +@mock.patch.object(ProjectOptions, "playbooks", project_playbooks) @pytest.mark.parametrize( "grant_project, grant_credential, grant_inventory, expect", [ (True, True, True, 201), @@ -34,10 +32,11 @@ def test_create(post, project, machine_credential, inventory, alice, grant_proje 'project': project.id, 'credential': machine_credential.id, 'inventory': inventory.id, - 'playbook': 'mocked', + 'playbook': 'mocked.yml', }, alice, expect=expect) -@decorators +@pytest.mark.django_db +@mock.patch.object(ProjectOptions, "playbooks", project_playbooks) @pytest.mark.parametrize( "grant_project, grant_credential, grant_inventory, expect", [ (True, True, True, 200), @@ -62,10 +61,11 @@ def test_edit_sensitive_fields(patch, job_template_factory, alice, grant_project 'project': objs.project.id, 'credential': objs.credential.id, 'inventory': objs.inventory.id, - 'playbook': 'othermocked', + 'playbook': 'alt-mocked.yml', }, alice, expect=expect) -@decorators +@pytest.mark.django_db +@mock.patch.object(ProjectOptions, "playbooks", project_playbooks) def test_edit_playbook(patch, job_template_factory, alice): objs = job_template_factory('jt', organization='org1', project='prj', inventory='inv', credential='cred') objs.job_template.admin_role.members.add(alice) @@ -74,18 +74,21 @@ def test_edit_playbook(patch, job_template_factory, alice): objs.inventory.use_role.members.add(alice) patch(reverse('api:job_template_detail', args=(objs.job_template.id,)), { - 'playbook': 'othermocked', + 'playbook': 'alt-mocked.yml', }, alice, expect=200) objs.inventory.use_role.members.remove(alice) patch(reverse('api:job_template_detail', args=(objs.job_template.id,)), { - 'playbook': 'mocked', + 'playbook': 'mocked.yml', }, alice, expect=403) -@decorators +@pytest.mark.django_db +@mock.patch.object(ProjectOptions, "playbooks", project_playbooks) def test_edit_nonsenstive(patch, job_template_factory, alice): objs = job_template_factory('jt', organization='org1', project='prj', inventory='inv', credential='cred') jt = objs.job_template + jt.playbook = 'mocked.yml' + jt.save() jt.admin_role.members.add(alice) res = patch(reverse('api:job_template_detail', args=(jt.id,)), {