From 098a407e257e3c7b775d7f26d357d966281e0c8c Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Wed, 18 Oct 2017 09:52:32 -0400 Subject: [PATCH] fix bug where JT admins could not edit spec --- awx/api/permissions.py | 16 +++++---- awx/api/views.py | 35 +++++++++---------- .../tests/functional/api/test_survey_spec.py | 16 +++++++++ awx/main/tests/functional/conftest.py | 12 +++++++ 4 files changed, 55 insertions(+), 24 deletions(-) diff --git a/awx/api/permissions.py b/awx/api/permissions.py index e01ab4d9fb..3cf18fc55b 100644 --- a/awx/api/permissions.py +++ b/awx/api/permissions.py @@ -52,14 +52,18 @@ class ModelAccessPermission(permissions.BasePermission): if not check_user_access(request.user, view.model, 'add', {view.parent_key: parent_obj}): return False return True - elif getattr(view, 'is_job_start', False): + elif hasattr(view, 'obj_permission_type'): + # Generic object-centric view permission check without object not needed if not obj: return True - return check_user_access(request.user, view.model, 'start', obj) - elif getattr(view, 'is_job_cancel', False): - if not obj: - return True - return check_user_access(request.user, view.model, 'cancel', obj) + # Permission check that happens when get_object() is called + extra_kwargs = {} + if view.obj_permission_type == 'admin': + extra_kwargs['data'] = {} + return check_user_access( + request.user, view.model, view.obj_permission_type, obj, + **extra_kwargs + ) else: if obj: return True diff --git a/awx/api/views.py b/awx/api/views.py index ade26dfacd..9771559bf4 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -1327,8 +1327,8 @@ class ProjectUpdateDetail(UnifiedJobDeletionMixin, RetrieveDestroyAPIView): class ProjectUpdateCancel(RetrieveAPIView): model = ProjectUpdate + obj_permission_type = 'cancel' serializer_class = ProjectUpdateCancelSerializer - is_job_cancel = True new_in_13 = True def post(self, request, *args, **kwargs): @@ -2440,9 +2440,9 @@ class InventoryInventorySourcesUpdate(RetrieveAPIView): view_name = _('Inventory Sources Update') model = Inventory + obj_permission_type = 'start' serializer_class = InventorySourceUpdateSerializer permission_classes = (InventoryInventorySourcesUpdatePermission,) - is_job_start = True new_in_320 = True def retrieve(self, request, *args, **kwargs): @@ -2596,8 +2596,8 @@ class InventorySourceUpdatesList(SubListAPIView): class InventorySourceUpdateView(RetrieveAPIView): model = InventorySource + obj_permission_type = 'start' serializer_class = InventorySourceUpdateSerializer - is_job_start = True new_in_14 = True def post(self, request, *args, **kwargs): @@ -2638,8 +2638,8 @@ class InventoryUpdateDetail(UnifiedJobDeletionMixin, RetrieveDestroyAPIView): class InventoryUpdateCancel(RetrieveAPIView): model = InventoryUpdate + obj_permission_type = 'cancel' serializer_class = InventoryUpdateCancelSerializer - is_job_cancel = True new_in_14 = True def post(self, request, *args, **kwargs): @@ -2690,9 +2690,9 @@ class JobTemplateDetail(RetrieveUpdateDestroyAPIView): class JobTemplateLaunch(RetrieveAPIView): model = JobTemplate + obj_permission_type = 'start' metadata_class = JobTypeMetadata serializer_class = JobLaunchSerializer - is_job_start = True always_allow_superuser = False def update_raw_data(self, data): @@ -2793,6 +2793,7 @@ class JobTemplateSchedulesList(SubListCreateAPIView): class JobTemplateSurveySpec(GenericAPIView): model = JobTemplate + obj_permission_type = 'admin' serializer_class = EmptySerializer new_in_210 = True @@ -3364,9 +3365,9 @@ class WorkflowJobTemplateLaunch(WorkflowsEnforcementMixin, RetrieveAPIView): model = WorkflowJobTemplate + obj_permission_type = 'start' serializer_class = WorkflowJobLaunchSerializer new_in_310 = True - is_job_start = True always_allow_superuser = False def update_raw_data(self, data): @@ -3384,8 +3385,6 @@ class WorkflowJobTemplateLaunch(WorkflowsEnforcementMixin, RetrieveAPIView): def post(self, request, *args, **kwargs): obj = self.get_object() - if not request.user.can_access(self.model, 'start', obj): - raise PermissionDenied() serializer = self.serializer_class(instance=obj, data=request.data) if not serializer.is_valid(): @@ -3406,8 +3405,8 @@ class WorkflowJobTemplateLaunch(WorkflowsEnforcementMixin, RetrieveAPIView): class WorkflowJobRelaunch(WorkflowsEnforcementMixin, GenericAPIView): model = WorkflowJob + obj_permission_type = 'start' serializer_class = EmptySerializer - is_job_start = True new_in_310 = True def check_object_permissions(self, request, obj): @@ -3564,8 +3563,8 @@ class WorkflowJobWorkflowNodesList(WorkflowsEnforcementMixin, SubListAPIView): class WorkflowJobCancel(WorkflowsEnforcementMixin, RetrieveAPIView): model = WorkflowJob + obj_permission_type = 'cancel' serializer_class = WorkflowJobCancelSerializer - is_job_cancel = True new_in_310 = True def post(self, request, *args, **kwargs): @@ -3619,8 +3618,8 @@ class SystemJobTemplateDetail(RetrieveAPIView): class SystemJobTemplateLaunch(GenericAPIView): model = SystemJobTemplate + obj_permission_type = 'start' serializer_class = EmptySerializer - is_job_start = True new_in_210 = True def get(self, request, *args, **kwargs): @@ -3759,8 +3758,8 @@ class JobActivityStreamList(ActivityStreamEnforcementMixin, SubListAPIView): class JobStart(GenericAPIView): model = Job + obj_permission_type = 'start' serializer_class = EmptySerializer - is_job_start = True deprecated = True def v2_not_allowed(self): @@ -3797,8 +3796,8 @@ class JobStart(GenericAPIView): class JobCancel(RetrieveAPIView): model = Job + obj_permission_type = 'cancel' serializer_class = JobCancelSerializer - is_job_cancel = True def post(self, request, *args, **kwargs): obj = self.get_object() @@ -3812,8 +3811,8 @@ class JobCancel(RetrieveAPIView): class JobRelaunch(RetrieveAPIView): model = Job + obj_permission_type = 'start' serializer_class = JobRelaunchSerializer - is_job_start = True @csrf_exempt @transaction.non_atomic_requests @@ -4053,8 +4052,8 @@ class AdHocCommandDetail(UnifiedJobDeletionMixin, RetrieveDestroyAPIView): class AdHocCommandCancel(RetrieveAPIView): model = AdHocCommand + obj_permission_type = 'cancel' serializer_class = AdHocCommandCancelSerializer - is_job_cancel = True new_in_220 = True def post(self, request, *args, **kwargs): @@ -4069,8 +4068,8 @@ class AdHocCommandCancel(RetrieveAPIView): class AdHocCommandRelaunch(GenericAPIView): model = AdHocCommand + obj_permission_type = 'start' serializer_class = AdHocCommandRelaunchSerializer - is_job_start = True new_in_220 = True # FIXME: Figure out why OPTIONS request still shows all fields. @@ -4204,8 +4203,8 @@ class SystemJobDetail(UnifiedJobDeletionMixin, RetrieveDestroyAPIView): class SystemJobCancel(RetrieveAPIView): model = SystemJob + obj_permission_type = 'cancel' serializer_class = SystemJobCancelSerializer - is_job_cancel = True new_in_210 = True def post(self, request, *args, **kwargs): @@ -4426,9 +4425,9 @@ class NotificationTemplateTest(GenericAPIView): view_name = _('Notification Template Test') model = NotificationTemplate + obj_permission_type = 'start' serializer_class = EmptySerializer new_in_300 = True - is_job_start = True def post(self, request, *args, **kwargs): obj = self.get_object() diff --git a/awx/main/tests/functional/api/test_survey_spec.py b/awx/main/tests/functional/api/test_survey_spec.py index f679d6806b..a01e5c890a 100644 --- a/awx/main/tests/functional/api/test_survey_spec.py +++ b/awx/main/tests/functional/api/test_survey_spec.py @@ -34,6 +34,22 @@ def test_survey_spec_view_denied(job_template_with_survey, get, admin_user): assert response.data['detail'] == 'Your license does not allow adding surveys.' +@mock.patch('awx.main.access.BaseAccess.check_license', mock_no_surveys) +@pytest.mark.django_db +@pytest.mark.survey +@pytest.mark.parametrize("role_field,expected_status_code", [ + ('admin_role', 200), + ('execute_role', 403), + ('read_role', 403) +]) +def test_survey_edit_access(job_template, survey_spec_factory, rando, post, role_field, expected_status_code): + survey_input_data = survey_spec_factory('new_question') + role = getattr(job_template, role_field) + role.members.add(rando) + post(reverse('api:job_template_survey_spec', kwargs={'pk': job_template.id}), + user=rando, data=survey_input_data, expect=expected_status_code) + + @mock.patch('awx.main.access.BaseAccess.check_license', mock_no_surveys) @pytest.mark.django_db @pytest.mark.survey diff --git a/awx/main/tests/functional/conftest.py b/awx/main/tests/functional/conftest.py index 456d1a409e..b6061bd33e 100644 --- a/awx/main/tests/functional/conftest.py +++ b/awx/main/tests/functional/conftest.py @@ -528,6 +528,18 @@ def _request(verb): middleware.process_response(request, response) if expect: if response.status_code != expect: + data_copy = response.data.copy() + try: + # Make translated strings printable + for key, value in response.data.items(): + if isinstance(value, list): + response.data[key] = [] + for item in value: + response.data[key].append(str(value)) + else: + response.data[key] = str(value) + except Exception: + response.data = data_copy print(response.data) assert response.status_code == expect response.render()