From cb6d7dfe6905b631d24a87f910590272425f94bb Mon Sep 17 00:00:00 2001 From: Yunfan Zhang Date: Mon, 23 Jul 2018 16:35:03 -0400 Subject: [PATCH] Fix credential leak when copying Job Templates. Signed-off-by: Yunfan Zhang --- awx/api/generics.py | 9 ++++++-- awx/main/access.py | 21 +++++++++++++++++++ awx/main/tests/functional/test_copy.py | 6 ++++++ .../test_job_template_serializers.py | 3 +-- 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/awx/api/generics.py b/awx/api/generics.py index ea2186675f..871f2a462d 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -93,7 +93,7 @@ class LoggedLoginView(auth_views.LoginView): current_user = JSONRenderer().render(current_user.data) current_user = urllib.quote('%s' % current_user, '') ret.set_cookie('current_user', current_user) - + return ret else: ret.status_code = 401 @@ -755,6 +755,7 @@ class DeleteLastUnattachLabelMixin(object): when the last disassociate is called should inherit from this class. Further, the model should implement is_detached() ''' + def unattach(self, request, *args, **kwargs): (sub_id, res) = super(DeleteLastUnattachLabelMixin, self).unattach_validate(request) if res: @@ -945,7 +946,9 @@ class CopyAPIView(GenericAPIView): create_kwargs = self._build_create_dict(obj) for key in create_kwargs: create_kwargs[key] = getattr(create_kwargs[key], 'pk', None) or create_kwargs[key] - return Response({'can_copy': request.user.can_access(self.model, 'add', create_kwargs)}) + can_copy = request.user.can_access(self.model, 'add', create_kwargs) and \ + request.user.can_access(self.model, 'copy_related', obj) + return Response({'can_copy': can_copy}) def post(self, request, *args, **kwargs): if get_request_version(request) < 2: @@ -957,6 +960,8 @@ class CopyAPIView(GenericAPIView): create_kwargs_check[key] = getattr(create_kwargs[key], 'pk', None) or create_kwargs[key] if not request.user.can_access(self.model, 'add', create_kwargs_check): raise PermissionDenied() + if not request.user.can_access(self.model, 'copy_related', obj): + raise PermissionDenied() serializer = self.get_serializer(data=request.data) if not serializer.is_valid(): return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) diff --git a/awx/main/access.py b/awx/main/access.py index 2377abeef7..e493ea075e 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -6,6 +6,7 @@ import os import sys import logging import six +from functools import reduce # Django from django.conf import settings @@ -218,6 +219,15 @@ class BaseAccess(object): def can_copy(self, obj): return self.can_add({'reference_obj': obj}) + def can_copy_related(self, obj): + ''' + can_copy_related() should only be used to check if the user have access to related + many to many credentials in when copying the object. It does not check if the user + has permission for any other related objects. Therefore, when checking if the user + can copy an object, it should always be used in conjunction with can_add() + ''' + return True + def can_attach(self, obj, sub_obj, relationship, data, skip_sub_obj_read_check=False): if skip_sub_obj_read_check: @@ -1328,6 +1338,17 @@ class JobTemplateAccess(BaseAccess): return self.user in project.use_role else: return False + + @check_superuser + def can_copy_related(self, obj): + ''' + Check if we have access to all the credentials related to Job Templates. + Does not verify the user's permission for any other related fields (projects, inventories, etc). + ''' + + # obj.credentials.all() is accessible ONLY when object is saved (has valid id) + credential_manager = getattr(obj, 'credentials', None) if getattr(obj, 'id', False) else Credentials.objects.none() + return reduce(lambda prev, cred: prev and self.user in cred.use_role, credential_manager.all(), True) def can_start(self, obj, validate_license=True): # Check license. diff --git a/awx/main/tests/functional/test_copy.py b/awx/main/tests/functional/test_copy.py index bdfc4936c2..1b017c8e10 100644 --- a/awx/main/tests/functional/test_copy.py +++ b/awx/main/tests/functional/test_copy.py @@ -18,6 +18,8 @@ def test_job_template_copy(post, get, project, inventory, machine_credential, va job_template_with_survey_passwords.credentials.add(machine_credential) job_template_with_survey_passwords.credentials.add(vault_credential) job_template_with_survey_passwords.admin_role.members.add(alice) + project.admin_role.members.add(alice) + inventory.admin_role.members.add(alice) assert get( reverse('api:job_template_copy', kwargs={'pk': job_template_with_survey_passwords.pk}), alice, expect=200 @@ -26,6 +28,10 @@ def test_job_template_copy(post, get, project, inventory, machine_credential, va reverse('api:job_template_copy', kwargs={'pk': job_template_with_survey_passwords.pk}), admin, expect=200 ).data['can_copy'] is True + post( + reverse('api:job_template_copy', kwargs={'pk': job_template_with_survey_passwords.pk}), + {'name': 'new jt name'}, alice, expect=403 + ) jt_copy_pk = post( reverse('api:job_template_copy', kwargs={'pk': job_template_with_survey_passwords.pk}), {'name': 'new jt name'}, admin, expect=201 diff --git a/awx/main/tests/unit/api/serializers/test_job_template_serializers.py b/awx/main/tests/unit/api/serializers/test_job_template_serializers.py index 29c0512256..a6f41debb9 100644 --- a/awx/main/tests/unit/api/serializers/test_job_template_serializers.py +++ b/awx/main/tests/unit/api/serializers/test_job_template_serializers.py @@ -97,7 +97,6 @@ class TestJobTemplateSerializerGetSummaryFields(): are put into the serializer user_capabilities""" jt_obj = job_template_factory('testJT', project='proj1', persisted=False).job_template - jt_obj.id = 5 jt_obj.admin_role = Role(id=9, role_field='admin_role') jt_obj.execute_role = Role(id=8, role_field='execute_role') jt_obj.read_role = Role(id=7, role_field='execute_role') @@ -115,7 +114,7 @@ class TestJobTemplateSerializerGetSummaryFields(): with mocker.patch("awx.api.serializers.role_summary_fields_generator", return_value='Can eat pie'): with mocker.patch("awx.main.access.JobTemplateAccess.can_change", return_value='foobar'): - with mocker.patch("awx.main.access.JobTemplateAccess.can_add", return_value='foo'): + with mocker.patch("awx.main.access.JobTemplateAccess.can_copy", return_value='foo'): with mock.patch.object(jt_obj.__class__, 'get_deprecated_credential', return_value=None): response = serializer.get_summary_fields(jt_obj)