[v2,08/13] REST: Make 'User.first_name', 'last_name' editable

These would be valuable to change via the API.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Cc: Andy Doan <andy.doan@linaro.org>
---
 patchwork/api/user.py            | 20 ++++++++++++++++----
 patchwork/tests/test_rest_api.py | 16 +++++++++++-----
 2 files changed, 27 insertions(+), 9 deletions(-)

LGTM

Reviewed-by: Daniel Axtens <dja@axtens.net>

Regards,
Daniel

Stephen Finucane <stephen@that.guru> writes:

> These would be valuable to change via the API.
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> Cc: Andy Doan <andy.doan@linaro.org>
> ---
>  patchwork/api/user.py            | 20 ++++++++++++++++----
>  patchwork/tests/test_rest_api.py | 16 +++++++++++-----
>  2 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/patchwork/api/user.py b/patchwork/api/user.py
> index 8fe3e74..c5f7c05 100644
> --- a/patchwork/api/user.py
> +++ b/patchwork/api/user.py
> @@ -19,16 +19,28 @@
>  
>  from django.contrib.auth.models import User
>  from rest_framework.generics import ListAPIView
> -from rest_framework.generics import RetrieveAPIView
> -from rest_framework.permissions import IsAuthenticated
> +from rest_framework.generics import RetrieveUpdateAPIView
> +from rest_framework import permissions
>  from rest_framework.serializers import HyperlinkedModelSerializer
>  
>  
> +class IsOwnerOrReadOnly(permissions.BasePermission):
> +
> +    def has_object_permission(self, request, view, obj):
> +        if request.method in permissions.SAFE_METHODS:
> +            return True
> +
> +        return obj == request.user
> +
> +
>  class UserSerializer(HyperlinkedModelSerializer):
>  
>      class Meta:
>          model = User
>          fields = ('url', 'username', 'first_name', 'last_name', 'email')
> +        # we don't allow updating of emails via the API, as we need to
> +        # validate that the User actually owns said email first
> +        read_only_fields = ('username', 'email')
>          extra_kwargs = {
>              'url': {'view_name': 'api-user-detail'},
>          }
> @@ -37,7 +49,7 @@ class UserSerializer(HyperlinkedModelSerializer):
>  class UserMixin(object):
>  
>      queryset = User.objects.all()
> -    permission_classes = (IsAuthenticated,)
> +    permission_classes = (permissions.IsAuthenticated, IsOwnerOrReadOnly)
>      serializer_class = UserSerializer
>  
>  
> @@ -47,7 +59,7 @@ class UserList(UserMixin, ListAPIView):
>      pass
>  
>  
> -class UserDetail(UserMixin, RetrieveAPIView):
> +class UserDetail(UserMixin, RetrieveUpdateAPIView):
>      """Show a user."""
>  
>      pass
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index e8eb71f..667f9f3 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -233,19 +233,25 @@ class TestUserAPI(APITestCase):
>          self.assertNotIn('password', resp.data[0])
>          self.assertNotIn('is_superuser', resp.data[0])
>  
> -    def test_readonly(self):
> +    def test_update(self):
>          user = create_maintainer()
>          user.is_superuser = True
>          user.save()
>          self.client.force_authenticate(user=user)
>  
> -        resp = self.client.delete(self.api_url(1))
> -        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> +        resp = self.client.patch(self.api_url(user.id), {'first_name': 'Tan'})
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>  
> -        resp = self.client.patch(self.api_url(1), {'email': 'foo@f.com'})
> +    def test_create_delete(self):
> +        user = create_maintainer()
> +        user.is_superuser = True
> +        user.save()
> +        self.client.force_authenticate(user=user)
> +
> +        resp = self.client.delete(self.api_url(user.id))
>          self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
> -        resp = self.client.post(self.api_url(), {'email': 'foo@f.com'})
> +        resp = self.client.post(self.api_url(user.id), {'email': 'foo@f.com'})
>          self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>  
> -- 
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

On 11/19/2016 10:51 AM, Stephen Finucane wrote:
> These would be valuable to change via the API.
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> Cc: Andy Doan <andy.doan@linaro.org>
> ---

I never anticipated much value from this. ie - if someone wants to 
change their name, I'd think they'd look to do this from the UI. 
However, the code is simple and secure enough. So I'll give a reviewed-by:

Reviewed-by: Andy Doan <andy.doan@linaro.org>

>  patchwork/api/user.py            | 20 ++++++++++++++++----
>  patchwork/tests/test_rest_api.py | 16 +++++++++++-----
>  2 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/patchwork/api/user.py b/patchwork/api/user.py
> index 8fe3e74..c5f7c05 100644
> --- a/patchwork/api/user.py
> +++ b/patchwork/api/user.py
> @@ -19,16 +19,28 @@
>
>  from django.contrib.auth.models import User
>  from rest_framework.generics import ListAPIView
> -from rest_framework.generics import RetrieveAPIView
> -from rest_framework.permissions import IsAuthenticated
> +from rest_framework.generics import RetrieveUpdateAPIView
> +from rest_framework import permissions
>  from rest_framework.serializers import HyperlinkedModelSerializer
>
>
> +class IsOwnerOrReadOnly(permissions.BasePermission):
> +
> +    def has_object_permission(self, request, view, obj):
> +        if request.method in permissions.SAFE_METHODS:
> +            return True
> +
> +        return obj == request.user
> +
> +
>  class UserSerializer(HyperlinkedModelSerializer):
>
>      class Meta:
>          model = User
>          fields = ('url', 'username', 'first_name', 'last_name', 'email')
> +        # we don't allow updating of emails via the API, as we need to
> +        # validate that the User actually owns said email first
> +        read_only_fields = ('username', 'email')
>          extra_kwargs = {
>              'url': {'view_name': 'api-user-detail'},
>          }
> @@ -37,7 +49,7 @@ class UserSerializer(HyperlinkedModelSerializer):
>  class UserMixin(object):
>
>      queryset = User.objects.all()
> -    permission_classes = (IsAuthenticated,)
> +    permission_classes = (permissions.IsAuthenticated, IsOwnerOrReadOnly)
>      serializer_class = UserSerializer
>
>
> @@ -47,7 +59,7 @@ class UserList(UserMixin, ListAPIView):
>      pass
>
>
> -class UserDetail(UserMixin, RetrieveAPIView):
> +class UserDetail(UserMixin, RetrieveUpdateAPIView):
>      """Show a user."""
>
>      pass
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index e8eb71f..667f9f3 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -233,19 +233,25 @@ class TestUserAPI(APITestCase):
>          self.assertNotIn('password', resp.data[0])
>          self.assertNotIn('is_superuser', resp.data[0])
>
> -    def test_readonly(self):
> +    def test_update(self):
>          user = create_maintainer()
>          user.is_superuser = True
>          user.save()
>          self.client.force_authenticate(user=user)
>
> -        resp = self.client.delete(self.api_url(1))
> -        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> +        resp = self.client.patch(self.api_url(user.id), {'first_name': 'Tan'})
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
>
> -        resp = self.client.patch(self.api_url(1), {'email': 'foo@f.com'})
> +    def test_create_delete(self):
> +        user = create_maintainer()
> +        user.is_superuser = True
> +        user.save()
> +        self.client.force_authenticate(user=user)
> +
> +        resp = self.client.delete(self.api_url(user.id))
>          self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>
> -        resp = self.client.post(self.api_url(), {'email': 'foo@f.com'})
> +        resp = self.client.post(self.api_url(user.id), {'email': 'foo@f.com'})
>          self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>
>
>

On Tue, 2016-11-22 at 12:13 -0600, Andy Doan wrote:
> On 11/19/2016 10:51 AM, Stephen Finucane wrote:
> > 
> > These would be valuable to change via the API.
> > 
> > Signed-off-by: Stephen Finucane <stephen@that.guru>
> > Cc: Andy Doan <andy.doan@linaro.org>
> > ---
> 
> I never anticipated much value from this. ie - if someone wants to 
> change their name, I'd think they'd look to do this from the UI. 
> However, the code is simple and secure enough. So I'll give a
> reviewed-by:
> 
> Reviewed-by: Andy Doan <andy.doan@linaro.org>

True, but it's cheap to do and it would allow a greatly expanded
pwclient for users who live in the terminal. Why not.

Stephen