[Patchew-devel] [PATCH 2/2] rest: introduce generic permission framework
Fam Zheng
famz at redhat.com
Thu May 10 08:03:19 UTC 2018
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.
Or could you simply use hasattr()?
> > + 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?
> > - 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?
Fam
More information about the Patchew-devel
mailing list