[RFC,03/11] REST: Add Projects to the API

This exports projects via the REST API.

Security Constraints:
 * Anyone (logged in or not) can read all objects.
 * No one can create/delete objects.
 * Project maintainers are allowed to update (ie "patch"
   attributes)

Signed-off-by: Andy Doan <andy.doan@linaro.org>
Inspired-by: Damien Lespiau <damien.lespiau@intel.com>
---
 patchwork/tests/test_rest_api.py | 108 +++++++++++++++++++++++++++++++++++++++
 patchwork/views/rest_api.py      |  44 +++++++++++++++-
 2 files changed, 150 insertions(+), 2 deletions(-)
 create mode 100644 patchwork/tests/test_rest_api.py

On 15 Apr 13:23, Andy Doan wrote:
> This exports projects via the REST API.
> 
> Security Constraints:
>  * Anyone (logged in or not) can read all objects.
>  * No one can create/delete objects.
>  * Project maintainers are allowed to update (ie "patch"
>    attributes)
> 
> Signed-off-by: Andy Doan <andy.doan@linaro.org>
> Inspired-by: Damien Lespiau <damien.lespiau@intel.com>

Some small changes, but nothing significant IMO.

Stephen

> ---
>  patchwork/tests/test_rest_api.py | 108 +++++++++++++++++++++++++++++++++++++++
>  patchwork/views/rest_api.py      |  44 +++++++++++++++-
>  2 files changed, 150 insertions(+), 2 deletions(-)
>  create mode 100644 patchwork/tests/test_rest_api.py
> 
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> new file mode 100644
> index 0000000..6f2f988
> --- /dev/null
> +++ b/patchwork/tests/test_rest_api.py
> @@ -0,0 +1,108 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2016 Linaro Corporation
> +#
> +# This file is part of the Patchwork package.
> +#
> +# Patchwork is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# Patchwork is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Patchwork; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +import unittest
> +
> +from django.conf import settings
> +
> +from rest_framework import status
> +from rest_framework.test import APITestCase
> +
> +from patchwork.models import Project
> +from patchwork.tests.utils import defaults, create_maintainer, create_user
> +
> +
> +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestProjectAPI(APITestCase):
> +    fixtures = ['default_states']
> +
> +    def test_list_simple(self):
> +        """Validate we can list the default test project."""
> +        defaults.project.save()
> +        resp = self.client.get('/api/1.0/projects/')

Q: Do we want to hardcode URLs or use 'reverse'? We don't do this
consistently yet, but I'm moving towards it. Some users may customize
urls.py and we should probably handle that.

> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +        self.assertEqual(1, resp.data['count'])
> +        proj = resp.data['results'][0]
> +        self.assertEqual(defaults.project.linkname, proj['linkname'])
> +        self.assertEqual(defaults.project.name, proj['name'])
> +        self.assertEqual(defaults.project.listid, proj['listid'])
> +
> +    def test_get(self):
> +        """Validate we can get a specific project."""
> +        defaults.project.save()
> +        resp = self.client.get('/api/1.0/projects/1/')
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +        self.assertEqual(defaults.project.name, resp.data['name'])
> +
> +    def test_anonymous_writes(self):
> +        """Ensure anonymous "write" operations are rejected."""
> +        defaults.project.save()
> +        # create
> +        resp = self.client.post(
> +            '/api/1.0/projects/',
> +            {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        # update
> +        resp = self.client.patch('/api/1.0/projects/1/', {'linkname': 'foo'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        # delete
> +        resp = self.client.delete('/api/1.0/projects/1/')
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> +    def test_create(self):
> +        """Ensure creations are rejected."""
> +        defaults.project.save()
> +
> +        user = create_maintainer(defaults.project)
> +        user.is_superuser = True
> +        user.save()
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.post(
> +            '/api/1.0/projects/',
> +            {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)

This is something we may wish to enable in the future, but for now
having it disabled seems like a good move.

> +    def test_update(self):
> +        """Ensure updates can be performed maintainers."""
> +        defaults.project.save()
> +
> +        # A maintainer can update
> +        user = create_maintainer(defaults.project)
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.patch('/api/1.0/projects/1/', {'linkname': 'TEST'})
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +
> +        # A normal user can't
> +        user = create_user()
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.patch('/api/1.0/projects/1/', {'linkname': 'TEST'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> +    def test_delete(self):
> +        """Ensure deletions are rejected."""
> +        defaults.project.save()
> +
> +        # Even an admin can't remove a project
> +        user = create_maintainer(defaults.project)
> +        user.is_superuser = True
> +        user.save()
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.delete('/api/1.0/projects/1/')
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(1, Project.objects.all().count())
> diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py
> index e451770..a4f6886 100644
> --- a/patchwork/views/rest_api.py
> +++ b/patchwork/views/rest_api.py
> @@ -19,9 +19,49 @@
>  
>  from django.conf.urls import url, include
>  
> -from rest_framework import routers
> +from patchwork.models import Project
>  
> -router = routers.DefaultRouter()
> +from rest_framework import permissions
> +from rest_framework.pagination import PageNumberPagination
> +from rest_framework.routers import DefaultRouter
> +from rest_framework.serializers import ModelSerializer
> +from rest_framework.viewsets import ModelViewSet
> +
> +
> +class PageSizePagination(PageNumberPagination):
> +    """Overide base class to enable the "page_size" query parameter."""
> +    page_size = 30
> +    page_size_query_param = 'page_size'

I'm fine with 'page_size', but the spec needs to be updated if so (it
uses 'per_page'). Alternatively, use 'per_page'?

> +
> +
> +class PatchworkPermission(permissions.BasePermission):
> +    """This permission works for Project and Patch model objects"""
> +    def has_permission(self, request, view):
> +        if request.method in ('POST', 'DELETE'):
> +            return False
> +        return super(PatchworkPermission, self).has_permission(request, view)
> +
> +    def has_object_permission(self, request, view, obj):
> +        # read only for everyone
> +        if request.method in permissions.SAFE_METHODS:
> +            return True
> +        return obj.is_editable(request.user)
> +
> +
> +class ProjectSerializer(ModelSerializer):
> +    class Meta:
> +        model = Project

IMO, the serializers belong in their own file. This should help
make this file more manageable as it grows.

> +
> +class ProjectViewSet(ModelViewSet):
> +    permission_classes = (PatchworkPermission,)
> +    queryset = Project.objects.all()
> +    serializer_class = ProjectSerializer
> +    pagination_class = PageSizePagination
> +
> +
> +router = DefaultRouter()
> +router.register('projects', ProjectViewSet, 'project')
>  
>  urlpatterns = [
>      url(r'^api/1.0/', include(router.urls)),

This comment belonged in the previous change, but I'm not sure about
including urlpatterns in a file outside of 'urls.py' - it seems like
it would be easier to debug if kept in the one location. Thoughts?

On 05/09/2016 08:25 AM, Finucane, Stephen wrote:

>> +++ b/patchwork/tests/test_rest_api.py

>> +    def test_list_simple(self):
>> +        """Validate we can list the default test project."""
>> +        defaults.project.save()
>> +        resp = self.client.get('/api/1.0/projects/')
>
> Q: Do we want to hardcode URLs or use 'reverse'? We don't do this
> consistently yet, but I'm moving towards it. Some users may customize
> urls.py and we should probably handle that.

I wasn't a big fan of what I did there. Version 2 uses "reverse" for this.

>> +    def test_create(self):
>> +        """Ensure creations are rejected."""
>> +        defaults.project.save()
>> +
>> +        user = create_maintainer(defaults.project)
>> +        user.is_superuser = True
>> +        user.save()
>> +        self.client.force_authenticate(user=user)
>> +        resp = self.client.post(
>> +            '/api/1.0/projects/',
>> +            {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
>> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
>
> This is something we may wish to enable in the future, but for now
> having it disabled seems like a good move.

Yep - I just wanted to try and tests to validate all security assumptions.

>> diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py

>> +class PageSizePagination(PageNumberPagination):
>> +    """Overide base class to enable the "page_size" query parameter."""
>> +    page_size = 30
>> +    page_size_query_param = 'page_size'
>
> I'm fine with 'page_size', but the spec needs to be updated if so (it
> uses 'per_page'). Alternatively, use 'per_page'?

per_page is correct, that was just typo.


>> +class PatchworkPermission(permissions.BasePermission):
>> +    """This permission works for Project and Patch model objects"""
>> +    def has_permission(self, request, view):
>> +        if request.method in ('POST', 'DELETE'):
>> +            return False
>> +        return super(PatchworkPermission, self).has_permission(request, view)
>> +
>> +    def has_object_permission(self, request, view, obj):
>> +        # read only for everyone
>> +        if request.method in permissions.SAFE_METHODS:
>> +            return True
>> +        return obj.is_editable(request.user)
>> +
>> +
>> +class ProjectSerializer(ModelSerializer):
>> +    class Meta:
>> +        model = Project
>
> IMO, the serializers belong in their own file. This should help
> make this file more manageable as it grows.

That's fine. I'd been on the fence with this.

>> +class ProjectViewSet(ModelViewSet):
>> +    permission_classes = (PatchworkPermission,)
>> +    queryset = Project.objects.all()
>> +    serializer_class = ProjectSerializer
>> +    pagination_class = PageSizePagination
>> +
>> +
>> +router = DefaultRouter()
>> +router.register('projects', ProjectViewSet, 'project')
>>
>>   urlpatterns = [
>>       url(r'^api/1.0/', include(router.urls)),
>
> This comment belonged in the previous change, but I'm not sure about
> including urlpatterns in a file outside of 'urls.py' - it seems like
> it would be easier to debug if kept in the one location. Thoughts?

Makes sense, I'd been on the fence, but your suggestion is more 
consistent with how everything else is set up.