Allow app owners and admin to see ratings from non-public apps (bug 943093)

mkt/ratings/serializers.py

@@ -8,7 +8,7 @@ from reviews.models import Review, ReviewFlag
 from mkt.account.serializers import AccountSerializer
 from mkt.api.fields import SlugOrPrimaryKeyRelatedField, SplitField
 from mkt.api.exceptions import Conflict
-from mkt.regions import get_region, REGIONS_DICT
+from mkt.regions import REGIONS_DICT, get_region
 from mkt.versions.api import SimpleVersionSerializer
 from mkt.webapps.models import Webapp
 
@@ -60,25 +60,16 @@ class RatingSerializer(serializers.ModelSerializer):
         return (not self.get_is_author(obj) and
                 obj.reviewflag_set.filter(user=self.request.amo_user).exists())
 
-    @classmethod
-    def get_app_from_value(cls, value):
-        try:
-            app = Webapp.objects.valid().get(id=value)
-        except (Webapp.DoesNotExist, ValueError):
-            try:
-                app = Webapp.objects.valid().get(app_slug=value)
-            except Webapp.DoesNotExist:
-                raise serializers.ValidationError('Invalid app')
-        if not app.listed_in(region=REGIONS_DICT[get_region()]):
-            raise serializers.ValidationError(
-                'App not available in this region')
-        return app
-
     def validate(self, attrs):
         attrs['user'] = self.request.amo_user
         attrs['ip_address'] = self.request.META.get('REMOTE_ADDR', '')
 
         if not getattr(self, 'object'):
+            # If we are creating a rating, then we need to do various checks on
+            # the app. Because these checks need the version as well, we have
+            # to do them here and not in validate_app().
+
+            # If the app is packaged, add in the current version.
             if attrs['addon'].is_packaged:
                 attrs['version'] = attrs['addon'].current_version
 
@@ -88,11 +79,14 @@ class RatingSerializer(serializers.ModelSerializer):
             qs = self.context['view'].queryset.filter(addon=app, user=amo_user)
             if app.is_packaged:
                 qs = qs.filter(version=attrs['version'])
-
             if qs.exists():
                 raise Conflict('You have already reviewed this app.')
 
-            # Return 403 if the user is attempting to review their own app:
+            # Return 403 is the app is not public.
+            if not app.is_public():
+                raise PermissionDenied('The app requested is not public.')
+
+            # Return 403 if the user is attempting to review their own app.
             if app.has_author(amo_user):
                 raise PermissionDenied('You may not review your own app.')
 
@@ -100,13 +94,18 @@ class RatingSerializer(serializers.ModelSerializer):
             if app.is_premium() and not app.is_purchased(amo_user):
                 raise PermissionDenied("You may not review paid apps you "
                                        "haven't purchased.")
+
+            # Return 403 if the app is not available in the current region.
+            current_region = get_region()
+            if not app.listed_in(region=REGIONS_DICT[current_region]):
+                raise PermissionDenied('App not available in region "%s".' %
+                                       current_region)
+
         return attrs
 
     def validate_app(self, attrs, source):
-        if not getattr(self, 'object'):
+        # Don't allow users to change the app on an existing rating.
-            app = attrs[source]
+        if getattr(self, 'object'):
-            attrs[source] = RatingSerializer.get_app_from_value(app.pk)
-        else:
             attrs[source] = self.object.addon
         return attrs
 

mkt/ratings/tests/test_resources.py

@@ -161,21 +161,59 @@ class TestRatingResource(RestOAuth, amo.tests.AMOPaths):
         eq_(data['info']['slug'], self.app.app_slug)
         eq_(data['info']['current_version'], self.app.current_version.version)
 
-    def test_filter_by_invalid_app_slug(self):
+    def test_filter_by_invalid_app(self):
         Review.objects.create(addon=self.app, user=self.user, body='yes')
-        self._get_filter(app='wrongslug', expected_status=400)
+        self._get_filter(app='wrongslug', expected_status=404)
+        self._get_filter(app=2465478, expected_status=404)
 
-    def test_filter_by_nonpublic_app(self):
+    @patch('mkt.ratings.views.get_region')
+    def test_filter_by_nonpublic_app(self, get_region_mock):
+        Review.objects.create(addon=self.app, user=self.user, body='yes')
         self.app.update(status=amo.STATUS_PENDING)
-        self._get_filter(app=self.app.app_slug, expected_status=400)
+        get_region_mock.return_value = 'us'
+        res, data = self._get_filter(app=self.app.app_slug, expected_status=403)
+        eq_(data['detail'], 'The app requested is not public or not available '
+                            'in region "us".')
 
-    @patch('mkt.ratings.serializers.get_region')
+    def test_filter_by_nonpublic_app_admin(self):
+        Review.objects.create(addon=self.app, user=self.user, body='yes')
+        self.grant_permission(self.user, 'Addons:Edit')
+        self.app.update(status=amo.STATUS_PENDING)
+        self._get_filter(app=self.app.app_slug)
+
+    def test_filter_by_nonpublic_app_owner(self):
+        Review.objects.create(addon=self.app, user=self.user, body='yes')
+        AddonUser.objects.create(user=self.user, addon=self.app)
+        self.app.update(status=amo.STATUS_PENDING)
+        self._get_filter(app=self.app.app_slug)
+
+    @patch('mkt.ratings.views.get_region')
     def test_filter_by_app_excluded_in_region(self, get_region_mock):
+        Review.objects.create(addon=self.app, user=self.user, body='yes')
         AddonExcludedRegion.objects.create(addon=self.app,
                                            region=mkt.regions.BR.id)
         get_region_mock.return_value = 'br'
-        r, data = self._get_filter(app=self.app.app_slug, expected_status=400)
+        res, data = self._get_filter(app=self.app.app_slug, expected_status=403)
-        eq_(data['detail'], 'App not available in this region')
+        eq_(data['detail'], 'The app requested is not public or not available '
+                            'in region "br".')
+
+    @patch('mkt.ratings.views.get_region')
+    def test_filter_by_app_excluded_in_region_admin(self, get_region_mock):
+        Review.objects.create(addon=self.app, user=self.user, body='yes')
+        self.grant_permission(self.user, 'Addons:Edit')
+        AddonExcludedRegion.objects.create(addon=self.app,
+                                           region=mkt.regions.BR.id)
+        get_region_mock.return_value = 'br'
+        self._get_filter(app=self.app.app_slug)
+
+    @patch('mkt.ratings.views.get_region')
+    def test_filter_by_app_excluded_in_region_owner(self, get_region_mock):
+        Review.objects.create(addon=self.app, user=self.user, body='yes')
+        AddonUser.objects.create(user=self.user, addon=self.app)
+        AddonExcludedRegion.objects.create(addon=self.app,
+                                           region=mkt.regions.BR.id)
+        get_region_mock.return_value = 'br'
+        self._get_filter(app=self.app.app_slug)
 
     def test_anonymous_get_list_without_app(self):
         res, data = self._get_url(self.list_url, client=self.anon)
@@ -302,12 +340,12 @@ class TestRatingResource(RestOAuth, amo.tests.AMOPaths):
                                            region=mkt.regions.BR.id)
         get_region_mock.return_value = 'br'
         res, data = self._create()
-        eq_(400, res.status_code)
+        eq_(403, res.status_code)
 
     def test_create_for_nonpublic(self):
         self.app.update(status=amo.STATUS_PENDING)
         res, data = self._create()
-        eq_(400, res.status_code)
+        eq_(403, res.status_code)
 
     def test_create_duplicate_rating(self):
         self._create()

mkt/ratings/views.py

@@ -1,15 +1,16 @@
 from django.core.paginator import Paginator
+from django.http import Http404
 
 import commonware.log
 from rest_framework.decorators import action
 from rest_framework.exceptions import (MethodNotAllowed, NotAuthenticated,
-                                       ParseError)
+                                       PermissionDenied)
 from rest_framework.mixins import CreateModelMixin
 from rest_framework.permissions import AllowAny, IsAuthenticated
-from rest_framework.serializers import ValidationError
 from rest_framework.viewsets import GenericViewSet, ModelViewSet
 
 import amo
+from access.acl import check_addon_ownership
 from lib.metrics import record_action
 from reviews.models import Review, ReviewFlag
 
@@ -20,6 +21,8 @@ from mkt.api.authorization import (AnyOf, AllowOwner, AllowRelatedAppOwner,
                                    ByHttpMethod, GroupPermission)
 from mkt.api.base import CORSMixin
 from mkt.ratings.serializers import RatingFlagSerializer, RatingSerializer
+from mkt.regions import REGIONS_DICT, get_region
+from mkt.webapps.models import Webapp
 
 
 log = commonware.log.getLogger('z.api')
@@ -110,9 +113,19 @@ class RatingViewSet(CORSMixin, ModelViewSet):
 
     def get_app(self, ident):
         try:
-            return self.serializer_class.get_app_from_value(ident)
+            app = Webapp.objects.by_identifier(ident)
-        except ValidationError as e:
+        except Webapp.DoesNotExist:
-            raise ParseError(detail=e.messages[0])
+            raise Http404
+        current_region = get_region()
+        if ((not app.is_public()
+             or not app.listed_in(region=REGIONS_DICT[current_region]))
+             and not check_addon_ownership(self.request, app)):
+            # App owners and admin can see the app even if it's not public
+            # or not available in the current region. Regular users or
+            # anonymous users can't.
+            raise PermissionDenied('The app requested is not public or not '
+                'available in region "%s".' % current_region)
+        return app
 
     def list(self, request, *args, **kwargs):
         response = super(RatingViewSet, self).list(request, *args, **kwargs)
@@ -143,7 +156,7 @@ class RatingViewSet(CORSMixin, ModelViewSet):
                       (obj.pk, self.request.amo_user.id))
 
     def partial_update(self, *args, **kwargs):
-        # We don't need/want PATCH.
+        # We don't need/want PATCH for now.
         raise MethodNotAllowed('PATCH is not supported for this endpoint.')
 
     def get_extra_data(self, app, amo_user):