[Patchew-devel] [PATCH 2/2] rest: introduce generic permission framework
Paolo Bonzini
pbonzini at redhat.com
Thu May 10 10:53:52 UTC 2018
On 10/05/2018 10:03, Fam Zheng wrote:
> On Tue, 05/08 17:01, Paolo Bonzini wrote:
>> On 08/05/2018 16:37, Paolo Bonzini wrote:
>>> Until now, the REST API didn't really have a good permission system. In
>>> particular, it did not know about user groups. We will need to allow
>>> the "importer" group to add messages to any project, so it's time to
>>> improve the permissions. Similar to the old API, all the knowledge of
>>> permissions is encapsulated in one class, in this case a DRF Permission
>>> subclass. The new class replaces the old IsAdminUserOrReadOnly and
>>> IsMaintainerUserOrReadOnly permissions.
>>>
>>> The hierarchy is:
>>>
>>> - safe requests, or requests from superusers, are always allowed
>>>
>>> - group membership grants authorization for an operation on any project,
>>> but only if the view allows those groups
>>>
>>> - maintainership grants authorization for an operation on maintained projects
>>
>> Fam, can you review this one and check that it matches the intended
>> behavior of the old API?
>
> Looks good to me, except a few small questions below.
>
>>
>> Thanks,
>>
>> Paolo
>>
>>> ---
>>> api/rest.py | 67 ++++++++++++++++++++++++++++++++++------------
>>> tests/test_rest.py | 10 +++++++
>>> 2 files changed, 60 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/api/rest.py b/api/rest.py
>>> index ed40a10..3e9a4a6 100644
>>> --- a/api/rest.py
>>> +++ b/api/rest.py
>>> @@ -28,23 +28,46 @@ SEARCH_PARAM = 'q'
>>>
>>> # patchew-specific permission classes
>>>
>>> -class IsAdminUserOrReadOnly(permissions.BasePermission):
>>> +class PatchewPermission(permissions.BasePermission):
>>> """
>>> - Allows access only to admin users.
>>> + Generic code to lookup for permissions based on message and project objects.
>>> + The nested router's "project_pk" keyword is taken as a hint that the view
>>> + also has a "project" property that returns a Django object. has_permission
>>> + then checks that property too.
>>> """
>>> +
>>> + allowed_groups = ()
>>> +
>>> + def is_superuser(self, request):
>>> + return request.user and request.user.is_superuser
>>> +
>>> + def has_project_permission(self, request, view, obj):
>>> + return obj.maintained_by(request.user)
>>> +
>>> + def has_group_permission(self, request, view):
>>> + for grp in request.user.groups.all():
>>> + if grp.name in self.allowed_groups:
>>> + return True
>>> + return False
>>> +
>>> + def has_generic_permission(self, request, view):
>>> + return (request.method in permissions.SAFE_METHODS) or \
>>> + self.is_superuser(request) or \
>>> + self.has_group_permission(request, view)
>>> +
>>> def has_permission(self, request, view):
>>> - return request.method in permissions.SAFE_METHODS or \
>>> - (request.user and request.user.is_superuser)
>>> + return self.has_generic_permission(request, view) or \
>>> + ('projects_pk' in view.kwargs and \
>
> projects_pk or project_pk? Either the class doc string or here is wrong.
projects_pk.
> Or could you simply use hasattr()?
You mean to check for view.project? Yeah, I could do that too.
>>> + self.has_project_permission(request, view, view.project))
>>>
>>> -class IsMaintainerOrReadOnly(permissions.BasePermission):
>>> - """
>>> - Allows access only to admin users or maintainers.
>>> - """
>>> def has_object_permission(self, request, view, obj):
>>> if isinstance(obj, Message):
>>> obj = obj.project
>
> It is not obvious that obj is always either Project or Message. Do we want a
> comment or assert here?
No, we want a
isinstance(obj, Project) and self.has_project_permission(...)
below, so that later we can add a has_user_permission or something like
that.
>>> - return request.method in permissions.SAFE_METHODS or \
>>> - obj.maintained_by(request.user)
>>> + return self.has_generic_permission(request, view) or \
>>> + self.has_project_permission(request, view, obj)
>>> +
>>> +class ImportPermission(PatchewPermission):
>>> + allowed_groups = ('importers',)
>>>
>>> # pluggable field for plugin support
>>>
>>> @@ -85,7 +108,7 @@ class UserSerializer(serializers.HyperlinkedModelSerializer):
>>> class UsersViewSet(viewsets.ModelViewSet):
>>> queryset = User.objects.all().order_by('id')
>>> serializer_class = UserSerializer
>>> - permission_classes = (IsAdminUserOrReadOnly,)
>>> + permission_classes = (PatchewPermission,)
>>>
>>> # Projects
>>>
>>> @@ -108,7 +131,7 @@ class ProjectSerializer(serializers.HyperlinkedModelSerializer):
>>> class ProjectsViewSet(viewsets.ModelViewSet):
>>> queryset = Project.objects.all().order_by('id')
>>> serializer_class = ProjectSerializer
>>> - permission_classes = (IsMaintainerOrReadOnly,)
>>> + permission_classes = (PatchewPermission,)
>>>
>>> # Common classes for series and messages
>>>
>>> @@ -153,7 +176,7 @@ class BaseMessageSerializer(serializers.ModelSerializer):
>>> class BaseMessageViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
>>> serializer_class = BaseMessageSerializer
>>> queryset = Message.objects.all()
>>> - permission_classes = ()
>>> + permission_classes = (ImportPermission,)
>>> lookup_field = 'message_id'
>>> lookup_value_regex = '[^/]+'
>>>
>>> @@ -162,11 +185,21 @@ class ProjectMessagesViewSetMixin(mixins.RetrieveModelMixin):
>>> def get_queryset(self):
>>> return self.queryset.filter(project=self.kwargs['projects_pk'])
>>>
>>> - def get_serializer_context(self):
>>> + @property
>>> + def project(self):
>>> + if hasattr(self, '__project'):
>>> + return self.__project
>>> try:
>>> - return {'project': Project.objects.get(id=self.kwargs['projects_pk']), 'request': self.request}
>>> - except:
>>> + self.__project = Project.objects.get(id=self.kwargs['projects_pk'])
>>> + except:
>>> + self.__project = None
>>> + return self.__project
>>> +
>>> + def get_serializer_context(self):
>>> + if self.project is None:
>>> return Http404
>>> + return {'project': self.project, 'request': self.request}
>>> +
>>> # Series
>>>
>>> class ReplySerializer(BaseMessageSerializer):
>>> @@ -249,7 +282,6 @@ class SeriesViewSet(BaseMessageViewSet):
>>> queryset = Message.objects.filter(is_series_head=True).order_by('-last_reply_date')
>>> filter_backends = (PatchewSearchFilter,)
>>> search_fields = (SEARCH_PARAM,)
>>> - permission_classes = (IsMaintainerOrReadOnly,)
>>>
>>>
>>> class ProjectSeriesViewSet(ProjectMessagesViewSetMixin,
>>> @@ -362,6 +394,7 @@ class ResultSerializerFull(ResultSerializer):
>>> class ResultsViewSet(viewsets.ViewSet, generics.GenericAPIView):
>>> lookup_field = 'name'
>>> lookup_value_regex = '[^/]+'
>>> + permission_classes = (PatchewPermission,)
>>>
>>> def get_serializer_class(self, *args, **kwargs):
>>> if self.lookup_field in self.kwargs:
>>> diff --git a/tests/test_rest.py b/tests/test_rest.py
>>> index 3baadd5..ace58ab 100755
>>> --- a/tests/test_rest.py
>>> +++ b/tests/test_rest.py
>>> @@ -78,10 +78,18 @@ class RestTest(PatchewTestCase):
>>> self.assertEquals(resp.data['mailing_list'], "qemu-block at nongnu.org")
>>> self.assertEquals(resp.data['parent_project'], self.PROJECT_BASE)
>>>
>>> + def test_project_post_no_login(self):
>>> + data = {
>>> + 'name': 'keycodemapdb',
>>> + }
>>> + resp = self.api_client.post(self.REST_BASE + 'projects/', data=data)
>>> + self.assertEquals(resp.status_code, 403)
>>> +
>>> def test_project_post_minimal(self):
>>> data = {
>>> 'name': 'keycodemapdb',
>>> }
>>> + self.api_client.login(username=self.user, password=self.password)
>>> resp = self.api_client.post(self.REST_BASE + 'projects/', data=data)
>>> self.assertEquals(resp.status_code, 201)
>>> self.assertEquals(resp.data['resource_uri'].startswith(self.REST_BASE + 'projects/'), True)
>>> @@ -91,6 +99,7 @@ class RestTest(PatchewTestCase):
>>> self.assertEquals(resp.data['name'], data['name'])
>>>
>>> def test_project_post(self):
>>> + self.api_client.login(username=self.user, password=self.password)
>>> data = {
>>> 'name': 'keycodemapdb',
>>> 'mailing_list': 'qemu-devel at nongnu.org',
>>> @@ -261,6 +270,7 @@ class RestTest(PatchewTestCase):
>>> dp = self.get_data_path("0022-another-simple-patch.json.gz")
>>> with open(dp, "r") as f:
>>> data = f.read()
>>> + self.api_client.login(username=self.user, password=self.password)
>>> resp = self.api_client.post(self.PROJECT_BASE + "messages/", data, content_type='application/json')
>>> self.assertEqual(resp.status_code, 201)
>>> resp_get = self.api_client.get(self.PROJECT_BASE + "messages/20171023201055.21973-11-andrew.smirnov at gmail.com/")
>
> Do we want a case testing the API as importer?
Yes, but it's not me who should write that. :)
Paolo
More information about the Patchew-devel
mailing list