From bd82ab9fb077650455875c0106785a7150d747e7 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Wed, 4 Jan 2017 13:39:14 -0500 Subject: [PATCH 1/2] use ParentMixin machinery to check access_list parent obj permissions --- awx/api/generics.py | 6 ++---- awx/api/serializers.py | 3 +-- awx/api/views.py | 16 ++++++++-------- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/awx/api/generics.py b/awx/api/generics.py index 73b92cfcc5..f09eb90c38 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -558,14 +558,12 @@ class DestroyAPIView(GenericAPIView, generics.DestroyAPIView): pass -class ResourceAccessList(ListAPIView): +class ResourceAccessList(ParentMixin, ListAPIView): serializer_class = ResourceAccessListElementSerializer def get_queryset(self): - self.object_id = self.kwargs['pk'] - resource_model = getattr(self, 'resource_model') - obj = get_object_or_404(resource_model, pk=self.object_id) + obj = self.get_parent_object() content_type = ContentType.objects.get_for_model(obj) roles = set(Role.objects.filter(content_type=content_type, object_id=obj.id)) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index eec91af831..05dca74cb6 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -1586,8 +1586,7 @@ class ResourceAccessListElementSerializer(UserSerializer): the resource. ''' ret = super(ResourceAccessListElementSerializer, self).to_representation(user) - object_id = self.context['view'].object_id - obj = self.context['view'].resource_model.objects.get(pk=object_id) + obj = self.context['view'].get_parent_object() if self.context['view'].request is not None: requesting_user = self.context['view'].request.user else: diff --git a/awx/api/views.py b/awx/api/views.py index de81c451cd..4cfd089e6c 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -872,7 +872,7 @@ class OrganizationNotificationTemplatesSuccessList(SubListCreateAttachDetachAPIV class OrganizationAccessList(ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = Organization + parent_model = Organization new_in_300 = True @@ -1007,7 +1007,7 @@ class TeamActivityStreamList(ActivityStreamEnforcementMixin, SubListAPIView): class TeamAccessList(ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = Team + parent_model = Team new_in_300 = True @@ -1201,7 +1201,7 @@ class ProjectUpdateNotificationsList(SubListAPIView): class ProjectAccessList(ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = Project + parent_model = Project new_in_300 = True @@ -1414,7 +1414,7 @@ class UserDetail(RetrieveUpdateDestroyAPIView): class UserAccessList(ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = User + parent_model = User new_in_300 = True @@ -1521,7 +1521,7 @@ class CredentialActivityStreamList(ActivityStreamEnforcementMixin, SubListAPIVie class CredentialAccessList(ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = Credential + parent_model = Credential new_in_300 = True @@ -1615,7 +1615,7 @@ class InventoryActivityStreamList(ActivityStreamEnforcementMixin, SubListAPIView class InventoryAccessList(ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = Inventory + parent_model = Inventory new_in_300 = True @@ -2689,7 +2689,7 @@ class JobTemplateJobsList(SubListCreateAPIView): class JobTemplateAccessList(ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = JobTemplate + parent_model = JobTemplate new_in_300 = True @@ -3035,7 +3035,7 @@ class WorkflowJobTemplateNotificationTemplatesSuccessList(WorkflowsEnforcementMi class WorkflowJobTemplateAccessList(WorkflowsEnforcementMixin, ResourceAccessList): model = User # needs to be User for AccessLists's - resource_model = WorkflowJobTemplate + parent_model = WorkflowJobTemplate new_in_310 = True From 4e135bb4069f168dc7e1674931608138f5d33115 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Wed, 4 Jan 2017 14:46:23 -0500 Subject: [PATCH 2/2] add unit test to assure permission is checked --- awx/main/tests/unit/api/test_generics.py | 48 +++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/awx/main/tests/unit/api/test_generics.py b/awx/main/tests/unit/api/test_generics.py index b10b1c6c54..579440b201 100644 --- a/awx/main/tests/unit/api/test_generics.py +++ b/awx/main/tests/unit/api/test_generics.py @@ -6,9 +6,16 @@ import mock # DRF from rest_framework import status from rest_framework.response import Response +from rest_framework.exceptions import PermissionDenied # AWX -from awx.api.generics import ParentMixin, SubListCreateAttachDetachAPIView, DeleteLastUnattachLabelMixin +from awx.api.generics import ( + ParentMixin, + SubListCreateAttachDetachAPIView, + DeleteLastUnattachLabelMixin, + ResourceAccessList +) +from awx.main.models import Organization @pytest.fixture @@ -29,6 +36,11 @@ def mock_response_new(mocker): return m +@pytest.fixture +def mock_organization(): + return Organization(pk=4, name="Unsaved Org") + + @pytest.fixture def parent_relationship_factory(mocker): def rf(serializer_class, relationship_name, relationship_value=mocker.Mock()): @@ -178,3 +190,37 @@ class TestParentMixin: get_object_or_404.assert_called_with(parent_mixin.parent_model, **parent_mixin.kwargs) assert get_object_or_404.return_value == return_value + + +class TestResourceAccessList: + + def mock_request(self): + return mock.MagicMock( + user=mock.MagicMock( + is_anonymous=mock.MagicMock(return_value=False), + is_superuser=False + ), method='GET') + + + def mock_view(self): + view = ResourceAccessList() + view.parent_model = Organization + view.kwargs = {'pk': 4} + return view + + + def test_parent_access_check_failed(self, mocker, mock_organization): + with mocker.patch('awx.api.permissions.get_object_or_400', return_value=mock_organization): + mock_access = mocker.MagicMock(__name__='for logger', return_value=False) + with mocker.patch('awx.main.access.BaseAccess.can_read', mock_access): + with pytest.raises(PermissionDenied): + self.mock_view().check_permissions(self.mock_request()) + mock_access.assert_called_once_with(mock_organization) + + + def test_parent_access_check_worked(self, mocker, mock_organization): + with mocker.patch('awx.api.permissions.get_object_or_400', return_value=mock_organization): + mock_access = mocker.MagicMock(__name__='for logger', return_value=True) + with mocker.patch('awx.main.access.BaseAccess.can_read', mock_access): + self.mock_view().check_permissions(self.mock_request()) + mock_access.assert_called_once_with(mock_organization)