From eb140d9e69c7d90420c395a81ddd7cad0ba8e9e8 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 14 Nov 2017 16:21:05 -0500 Subject: [PATCH 1/2] fix a permissions bug for credentials specified at JT launch time hat tip to @alancoding for spotting this one --- awx/api/views.py | 12 ++++++--- .../test_deprecated_credential_assignment.py | 27 +++++++++++++++++++ .../functional/api/test_job_runtime_params.py | 2 +- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index b873c883f1..3dbb18db29 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2797,10 +2797,14 @@ class JobTemplateLaunch(RetrieveAPIView): if request.user not in use_role: raise PermissionDenied() - for cred in prompted_fields.get('credentials', []): - new_credential = get_object_or_400(Credential, pk=cred) - if request.user not in new_credential.use_role: - raise PermissionDenied() + # For credentials that are _added_ via launch parameters, ensure the + # launching user has access + current_credentials = set(obj.credentials.values_list('id', flat=True)) + for new_cred in Credential.objects.filter(id__in=prompted_fields.get('credentials', [])): + if new_cred.pk not in current_credentials and request.user not in new_cred.use_role: + raise PermissionDenied(_( + "You do not have access to credential {}".format(new_cred.name) + )) new_job = obj.create_unified_job(**prompted_fields) result = new_job.signal_start(**passwords) diff --git a/awx/main/tests/functional/api/test_deprecated_credential_assignment.py b/awx/main/tests/functional/api/test_deprecated_credential_assignment.py index 1527100c1c..0686f3b173 100644 --- a/awx/main/tests/functional/api/test_deprecated_credential_assignment.py +++ b/awx/main/tests/functional/api/test_deprecated_credential_assignment.py @@ -338,3 +338,30 @@ def test_extra_creds_prompted_at_launch(get, post, job_template, admin, net_cred def test_invalid_mixed_credentials_specification(get, post, job_template, admin, net_credential): url = reverse('api:job_template_launch', kwargs={'pk': job_template.pk}) post(url, {'credentials': [net_credential.pk], 'extra_credentials': [net_credential.pk]}, admin, expect=400) + + +@pytest.mark.django_db +def test_rbac_default_credential_usage(get, post, job_template, alice, machine_credential): + job_template.credentials.add(machine_credential) + job_template.execute_role.members.add(alice) + job_template.save() + + # alice can launch; she's not adding any _new_ credentials, and she has + # execute access to the JT + url = reverse('api:job_template_launch', kwargs={'pk': job_template.pk}) + post(url, {'credential': machine_credential.pk}, alice, expect=201) + + # make (copy) a _new_ SSH cred + new_cred = machine_credential + new_cred.pk = None + new_cred.save() + + # alice is attempting to launch with a *different* SSH cred, but + # she does not have access to it, so she cannot launch + url = reverse('api:job_template_launch', kwargs={'pk': job_template.pk}) + post(url, {'credential': new_cred.pk}, alice, expect=403) + + # if alice has gains access to the credential, she *can* launch + new_cred.use_role.members.add(alice) + url = reverse('api:job_template_launch', kwargs={'pk': job_template.pk}) + post(url, {'credential': new_cred.pk}, alice, expect=201) diff --git a/awx/main/tests/functional/api/test_job_runtime_params.py b/awx/main/tests/functional/api/test_job_runtime_params.py index 6214db94a8..cd9d13df86 100644 --- a/awx/main/tests/functional/api/test_job_runtime_params.py +++ b/awx/main/tests/functional/api/test_job_runtime_params.py @@ -237,7 +237,7 @@ def test_job_launch_fails_without_credential_access(job_template_prompts, runtim response = post(reverse('api:job_template_launch', kwargs={'pk':job_template.pk}), dict(credentials=runtime_data['credentials']), rando, expect=403) - assert response.data['detail'] == u'You do not have permission to perform this action.' + assert response.data['detail'] == u'You do not have access to credential runtime-cred' @pytest.mark.django_db From fa09d68603e80e33fb28357c19ef9ec1f5d2a1a9 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 14 Nov 2017 16:47:28 -0500 Subject: [PATCH 2/2] add a few minor optimizations and some refactoring for multi-cred --- awx/api/views.py | 4 ++-- awx/main/models/jobs.py | 6 +++--- awx/main/tests/unit/test_tasks.py | 15 +++++++++------ 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index 3dbb18db29..bdfa7a8183 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2998,7 +2998,7 @@ class JobTemplateExtraCredentialsList(JobTemplateCredentialsList): def get_queryset(self): sublist_qs = super(JobTemplateExtraCredentialsList, self).get_queryset() - sublist_qs = sublist_qs.filter(**{'credential_type__kind__in': ['cloud', 'net']}) + sublist_qs = sublist_qs.filter(credential_type__kind__in=['cloud', 'net']) return sublist_qs def is_valid_relation(self, parent, sub, created=False): @@ -3794,7 +3794,7 @@ class JobExtraCredentialsList(JobCredentialsList): def get_queryset(self): sublist_qs = super(JobExtraCredentialsList, self).get_queryset() - sublist_qs = sublist_qs.filter(**{'credential_type__kind__in': ['cloud', 'net']}) + sublist_qs = sublist_qs.filter(credential_type__kind__in=['cloud', 'net']) return sublist_qs diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index f59e27d59e..cc438bd013 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -166,11 +166,11 @@ class JobOptions(BaseModel): @property def network_credentials(self): - return [cred for cred in self.credentials.all() if cred.credential_type.kind == 'net'] + return list(self.credentials.filter(credential_type__kind='net')) @property def cloud_credentials(self): - return [cred for cred in self.credentials.all() if cred.credential_type.kind == 'cloud'] + return list(self.credentials.filter(credential_type__kind='cloud')) @property def credential(self): @@ -186,7 +186,7 @@ class JobOptions(BaseModel): def get_deprecated_credential(self, kind): try: - return [cred for cred in self.credentials.all() if cred.credential_type.kind == kind][0] + return self.credentials.filter(credential_type__kind=kind).first() except IndexError: return None diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index 3dbc8399fb..75b1ab0207 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -245,12 +245,15 @@ class TestJobExecution: # mock the job.credentials M2M relation so we can avoid DB access job._credentials = [] - patch = mock.patch.object(UnifiedJob, 'credentials', mock.Mock( - all=lambda: job._credentials, - add=job._credentials.append, - filter=mock.Mock(return_value=job._credentials), - spec_set=['all', 'add', 'filter'] - )) + patch = mock.patch.object(UnifiedJob, 'credentials', mock.Mock(**{ + 'all': lambda: job._credentials, + 'add': job._credentials.append, + 'filter.return_value': mock.Mock( + __iter__ = lambda *args: iter(job._credentials), + first = lambda: job._credentials[0] + ), + 'spec_set': ['all', 'add', 'filter'] + })) self.patches.append(patch) patch.start()