[Patchew-devel] [PATCH 2/2] rest: introduce generic permission framework
Paolo Bonzini
pbonzini at redhat.com
Tue May 8 15:01:36 UTC 2018
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?
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 \
> + 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
> - 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/")
>
More information about the Patchew-devel
mailing list