Merge pull request #1352 from diox/convert-accountresource-to-drf

Convert mkt.account.api.AccountResource to DRF (bug 910546)

docs/api/topics/accounts.rst

@@ -40,7 +40,7 @@ To update account information:
 
     No content is returned in the response.
 
-    :status 201: successfully completed.
+    :status 200: successfully completed.
 
 Fields that can be updated:
 

mkt/account/api.py

@@ -1,39 +1,9 @@
-from tastypie import http
-
 from mkt.api.authentication import (OAuthAuthentication,
                                     SharedSecretAuthentication)
 from mkt.api.authorization import OwnerAuthorization
-from mkt.api.base import CORSResource, http_error, MarketplaceModelResource
 from mkt.api.resources import AppResource
 from mkt.constants.apps import INSTALL_TYPE_USER
 from mkt.webapps.models import Webapp
-from users.models import UserProfile
-
-
-class Mine(object):
-
-    def obj_get(self, request=None, **kwargs):
-        if kwargs.get('pk') == 'mine':
-            kwargs['pk'] = request.amo_user.pk
-
-        # TODO: put in acl checks for admins to get other users information.
-        obj = super(Mine, self).obj_get(request=request, **kwargs)
-        if not OwnerAuthorization().is_authorized(request, object=obj):
-            raise http_error(http.HttpForbidden,
-                             'You do not have access to that account.')
-        return obj
-
-
-class AccountResource(Mine, CORSResource, MarketplaceModelResource):
-
-    class Meta(MarketplaceModelResource.Meta):
-        authentication = (SharedSecretAuthentication(), OAuthAuthentication())
-        authorization = OwnerAuthorization()
-        detail_allowed_methods = ['get', 'patch', 'put']
-        fields = ['display_name']
-        list_allowed_methods = []
-        queryset = UserProfile.objects.filter()
-        resource_name = 'settings'
 
 
 class InstalledResource(AppResource):

mkt/account/serializers.py

@@ -9,6 +9,12 @@ from mkt.api.base import CompatRelatedField
 from mkt.api.serializers import PotatoCaptchaSerializer
 
 
+class AccountSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = UserProfile
+        fields = ('display_name',)
+
+
 class FeedbackSerializer(PotatoCaptchaSerializer):
     feedback = fields.CharField()
     platform = fields.CharField(required=False)

mkt/account/tests/test_api.py

@@ -13,7 +13,7 @@ from nose.tools import eq_, ok_
 
 from amo.tests import TestCase
 from mkt.account.views import MineMixin
-from mkt.api.base import get_url, list_url
+from mkt.api.base import list_url
 from mkt.api.tests.test_oauth import BaseOAuth, get_absolute_url, RestOAuth
 from mkt.constants.apps import INSTALL_TYPE_REVIEWER
 from mkt.site.fixtures import fixture
@@ -123,64 +123,64 @@ class TestPermission(RestOAuth):
         ok_(res.json['permissions']['webpay'])
 
 
-class TestAccount(BaseOAuth):
+class TestAccount(RestOAuth):
     fixtures = fixture('user_2519', 'user_10482', 'webapp_337141')
 
     def setUp(self):
-        super(TestAccount, self).setUp(api_name='account')
-        self.list_url = list_url('settings')
-        self.get_url = get_url('settings', '2519')
+        super(TestAccount, self).setUp()
+        self.url = reverse('account-settings', kwargs={'pk': 2519})
         self.user = UserProfile.objects.get(pk=2519)
 
     def test_verbs(self):
-        self._allowed_verbs(self.list_url, ())
-        self._allowed_verbs(self.get_url, ('get', 'patch', 'put'))
+        self._allowed_verbs(self.url, ('get', 'patch', 'put'))
 
     def test_not_allowed(self):
-        eq_(self.anon.get(self.get_url).status_code, 401)
+        eq_(self.anon.get(self.url).status_code, 403)
 
     def test_allowed(self):
-        res = self.client.get(self.get_url)
+        res = self.client.get(self.url)
         eq_(res.status_code, 200, res.content)
         data = json.loads(res.content)
         eq_(data['display_name'], self.user.display_name)
 
     def test_other(self):
-        eq_(self.client.get(get_url('settings', '10482')).status_code, 403)
+        url = reverse('account-settings', kwargs={'pk': 10482})
+        eq_(self.client.get(url).status_code, 403)
 
     def test_own(self):
-        res = self.client.get(get_url('settings', 'mine'))
+        url = reverse('account-settings', kwargs={'pk': 'mine'})
+        res = self.client.get(url)
         eq_(res.status_code, 200)
         data = json.loads(res.content)
         eq_(data['display_name'], self.user.display_name)
 
     def test_patch(self):
-        res = self.client.patch(self.get_url,
+        res = self.client.patch(self.url,
                                 data=json.dumps({'display_name': 'foo'}))
-        eq_(res.status_code, 202)
+        eq_(res.status_code, 200)
         user = UserProfile.objects.get(pk=self.user.pk)
         eq_(user.display_name, 'foo')
 
     def test_put(self):
-        res = self.client.put(self.get_url,
+        res = self.client.put(self.url,
                               data=json.dumps({'display_name': 'foo'}))
-        eq_(res.status_code, 204)
+        eq_(res.status_code, 200)
         user = UserProfile.objects.get(pk=self.user.pk)
         eq_(user.display_name, 'foo')
         eq_(user.username, self.user.username)  # Did not change.
 
     def test_patch_extra_fields(self):
-        res = self.client.patch(self.get_url,
+        res = self.client.patch(self.url,
                                 data=json.dumps({'display_name': 'foo',
                                                  'username': 'bob'}))
-        eq_(res.status_code, 202)
+        eq_(res.status_code, 200)
         user = UserProfile.objects.get(pk=self.user.pk)
         eq_(user.display_name, 'foo')  # Got changed successfully.
         eq_(user.username, self.user.username)  # Did not change.
 
     def test_patch_other(self):
-        res = self.client.patch(get_url('settings', '10482'),
-                                data=json.dumps({'display_name': 'foo'}))
+        url = reverse('account-settings', kwargs={'pk': 10482})
+        res = self.client.patch(url, data=json.dumps({'display_name': 'foo'}))
         eq_(res.status_code, 403)
 
 

mkt/account/urls.py

@@ -2,14 +2,13 @@ from django.conf.urls import include, patterns, url
 
 from tastypie.api import Api
 
-from mkt.account.api import AccountResource, InstalledResource
-from mkt.account.views import (FeedbackView, LoginView, NewsletterView,
-                               PermissionsView)
+from mkt.account.api import InstalledResource
+from mkt.account.views import (AccountView, FeedbackView, LoginView,
+                               NewsletterView, PermissionsView)
 
 
 # Account API (old tastypie resources).
 account = Api(api_name='account')
-account.register(AccountResource())
 account.register(InstalledResource())
 
 # Account API (new DRF views).
@@ -19,6 +18,9 @@ drf_patterns = patterns('',
     url('^newsletter/', NewsletterView.as_view(), name='account-newsletter'),
     url('^permissions/(?P<pk>[^/]+)/$', PermissionsView.as_view(),
         name='account-permissions'),
+    url('^settings/(?P<pk>[^/]+)/$', AccountView.as_view(),
+        name='account-settings'),
+
 )
 
 api_patterns = patterns('',

mkt/account/views.py

@@ -10,7 +10,8 @@ import commonware.log
 from django_statsd.clients import statsd
 from rest_framework import status
 from rest_framework.exceptions import AuthenticationFailed
-from rest_framework.generics import CreateAPIView, RetrieveAPIView
+from rest_framework.generics import (CreateAPIView, RetrieveAPIView,
+                                     RetrieveUpdateAPIView)
 from rest_framework.permissions import AllowAny, IsAuthenticated
 from rest_framework.response import Response
 from rest_framework.throttling import UserRateThrottle
@@ -19,13 +20,13 @@ from amo.utils import send_mail_jinja
 from users.models import UserProfile
 from users.views import browserid_authenticate
 
-from mkt.account.serializers import (FeedbackSerializer, LoginSerializer,
-                                     NewsletterSerializer,
+from mkt.account.serializers import (AccountSerializer, FeedbackSerializer,
+                                     LoginSerializer, NewsletterSerializer,
                                      PermissionsSerializer)
 from mkt.api.authentication import (RestAnonymousAuthentication,
                                     RestOAuthAuthentication,
                                     RestSharedSecretAuthentication)
-from mkt.api.authorization import AllowSelf
+from mkt.api.authorization import AllowSelf, AllowOwner
 from mkt.api.base import CORSMixin
 
 
@@ -67,6 +68,15 @@ class CreateAPIViewWithoutModel(CreateAPIView):
         return self.response_error(request, serializer)
 
 
+class AccountView(MineMixin, CORSMixin, RetrieveUpdateAPIView):
+    authentication_classes = [RestOAuthAuthentication,
+                              RestSharedSecretAuthentication]
+    cors_allowed_methods = ['get', 'patch', 'put']
+    model = UserProfile
+    permission_classes = (AllowOwner,)
+    serializer_class = AccountSerializer
+
+
 class FeedbackView(CORSMixin, CreateAPIViewWithoutModel):
     class FeedbackThrottle(UserRateThrottle):
         THROTTLE_RATES = {

mkt/api/authorization.py

@@ -96,6 +96,11 @@ class PermissionAuthorization(Authorization):
 
 
 class AllowSelf(BasePermission):
+    """
+    Permission class to use when you are dealing with UserProfile models and
+    you want only the corresponding user to be able to access his UserProfile
+    instance.
+    """
     def has_permission(self, request, view):
         return request.user.is_authenticated()
 
@@ -117,6 +122,22 @@ class AllowNone(BasePermission):
         return False
 
 
+class AllowOwner(BasePermission):
+    """
+    Permission class to use when you are dealing with a model instance that has
+    a "user" FK pointing to an UserProfile, and you want only the corresponding
+    user to be able to access your instance.
+
+    Do not use with models pointing to an User! There is no guarantee that the
+    pk is the same between a User and an UserProfile instance.
+    """
+    def has_permission(self, request, view):
+        return request.user.is_authenticated()
+
+    def has_object_permission(self, request, view, obj):
+        return obj.user.pk == request.amo_user.pk
+
+
 class AllowAppOwner(BasePermission):
 
     def has_permission(self, request, view):

mkt/api/base.py

@@ -438,12 +438,12 @@ class CompatToOneField(ToOneField):
     Tastypie field to relate a resource to a django-rest-framework view.
     """
     def __init__(self, *args, **kwargs):
-        self.rest = kwargs.pop('rest', None)
+        self.url_name = kwargs.pop('url_name', None)
         self.extra_fields = kwargs.pop('extra_fields', None)
         return super(CompatToOneField, self).__init__(*args, **kwargs)
 
     def dehydrate_related(self, bundle, related_resource):
-        uri = reverse(self.rest + '-detail', kwargs={'pk': bundle.obj.pk})
+        uri = reverse(self.url_name, kwargs={'pk': bundle.obj.pk})
         if self.full:
             raise NotImplementedError
         elif self.extra_fields:

mkt/api/tests/test_authorization.py

@@ -5,8 +5,9 @@ from nose.tools import eq_, ok_
 from amo.tests import app_factory, TestCase
 from test_utils import RequestFactory
 
-from mkt.api.authorization import (AllowSelf, AnonymousReadOnlyAuthorization,
-                                   flag, PermissionAuthorization, switch)
+from mkt.api.authorization import (AllowOwner, AllowSelf,
+                                   AnonymousReadOnlyAuthorization, flag,
+                                   PermissionAuthorization, switch)
 from mkt.site.fixtures import fixture
 
 from .test_authentication import OwnerAuthorization
@@ -124,3 +125,37 @@ class TestAllowSelfAuthorization(TestCase):
         result = self.authorization.has_object_permission(self.request,
                                                           'myview', self.user)
         eq_(result, False)
+
+
+class TestAllowOwner(TestCase):
+    fixtures = fixture('user_2519', 'user_999')
+
+    def setUp(self):
+        self.authorization = AllowOwner()
+        self.anonymous = AnonymousUser()
+        self.user = User.objects.get(pk=2519)
+        self.request = RequestFactory().get('/')
+        self.request.user = self.anonymous
+        self.request.amo_user = None
+
+    def test_has_permission_anonymous(self):
+        eq_(self.authorization.has_permission(self.request, 'myview'), False)
+
+    def test_has_permission_user(self):
+        self.request.user = self.user
+        self.request.amo_user = self.request.user.get_profile()
+        eq_(self.authorization.has_permission(self.request, 'myview'), True)
+
+    def test_has_object_permission_user(self):
+        self.request.user = self.user
+        self.request.amo_user = self.request.user.get_profile()
+        result = self.authorization.has_object_permission(self.request,
+                                                          'myview', self)
+        eq_(result, True)
+
+    def test_has_object_permission_different_user(self):
+        self.request.user = User.objects.get(pk=999)
+        self.request.amo_user = self.request.user.get_profile()
+        result = self.authorization.has_object_permission(self.request,
+                                                          'myview', self)
+        eq_(result, False)

mkt/ratings/resources.py

@@ -10,7 +10,6 @@ import amo
 from amo import get_user
 from lib.metrics import record_action
 
-from mkt.account.api import AccountResource
 from mkt.api.authentication import (OptionalOAuthAuthentication,
                                     SharedSecretAuthentication)
 from mkt.api.authorization import (AnonymousReadOnlyAuthorization,
@@ -40,9 +39,12 @@ class RatingPaginator(paginator.Paginator):
 class RatingResource(CORSResource, MarketplaceModelResource):
 
     app = fields.ToOneField(AppResource, 'addon', readonly=True)
-    user = fields.ToOneField(AccountResource, 'user', readonly=True, full=True)
-    version = CompatToOneField(None, 'version', rest='version', readonly=True,
-                               null=True, extra_fields=('version',))
+    user = CompatToOneField(None, 'user', url_name='account-settings',
+                            readonly=True, null=True,
+                            extra_fields=('display_name',))
+    version = CompatToOneField(None, 'version', url_name='version-detail',
+                               readonly=True, null=True,
+                               extra_fields=('version',))
     report_spam = fields.CharField()
 
     class Meta(MarketplaceModelResource.Meta):