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
---
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@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@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@gmail.com/")
--
2.17.0
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
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@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@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@gmail.com/")
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
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@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@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@gmail.com/")
> >
>
Do we want a case testing the API as importer?
Fam
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
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@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@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@gmail.com/")
>
> Do we want a case testing the API as importer?
Yes, but it's not me who should write that. :)
Paolo
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
© 2016 - 2025 Red Hat, Inc.