Avoid reviewer privilege escalation vulnerability

src/olympia/addons/tests/test_views.py

@@ -8401,6 +8401,32 @@ def test_create_validation(self):
             ]
         }
 
+    def test_sender_validation(self):
+        # cannot send invitation if not an author, even with elevated permissions
+        user = user_factory(email='reviewer@mozilla.com', display_name='reviewer')
+        self.grant_permission(user, 'Addons:Review')
+        self.client.login_api(user)
+        response = self.client.post(
+            self.list_url, data={'user_id': user.id, 'role': 'developer'}
+        )
+        assert response.status_code == 403, response.content
+        assert response.data['detail'] == (
+            'You do not have permission to perform this action.'
+        )
+        self.grant_permission(user, '*:*')
+        response = self.client.post(
+            self.list_url, data={'user_id': user.id, 'role': 'developer'}
+        )
+        assert response.status_code == 403, response.content
+        assert response.data['detail'] == (
+            'You do not have permission to perform this action.'
+        )
+        # but the author can send, even if they have elevated permissions themselves
+        self.client.login_api(self.user)
+        self.grant_permission(self.user, 'Addons:Review')
+        response = self.client.post(self.list_url, data={'user_id': user.id})
+        assert response.status_code == 201
+
     def test_update_role(self):
         self.client.login_api(self.user)
         response = self.client.patch(self.detail_url, {'role': 'developer'})

src/olympia/addons/views.py

@@ -43,10 +43,10 @@
     AllowAddonAuthor,
     AllowAddonOwner,
     AllowIfNotMozillaDisabled,
-    AllowListedViewerOrReviewer,
+    AllowListedViewerOrReviewerReadOnly,
     AllowReadOnlyIfPublic,
     AllowRelatedObjectPermissions,
-    AllowUnlistedViewerOrReviewer,
+    AllowUnlistedViewerOrReviewerReadOnly,
     AnyOf,
     APIGatePermission,
     GroupPermission,
@@ -253,8 +253,8 @@ class AddonViewSet(
         AnyOf(
             AllowReadOnlyIfPublic,
             AllowAddonAuthor,
-            AllowListedViewerOrReviewer,
-            AllowUnlistedViewerOrReviewer,
+            AllowListedViewerOrReviewerReadOnly,
+            AllowUnlistedViewerOrReviewerReadOnly,
         )
     ]
     authentication_classes = [
@@ -578,8 +578,8 @@ def check_permissions(self, request):
             # be add-on author or reviewer.
             self.permission_classes = [
                 AnyOf(
-                    AllowListedViewerOrReviewer,
-                    AllowUnlistedViewerOrReviewer,
+                    AllowListedViewerOrReviewerReadOnly,
+                    AllowUnlistedViewerOrReviewerReadOnly,
                     AllowAddonAuthor,
                 )
             ]
@@ -607,14 +607,16 @@ def check_object_permissions(self, request, obj):
             # authors..
             self.permission_classes = [
                 AllowRelatedObjectPermissions(
-                    'addon', [AnyOf(AllowUnlistedViewerOrReviewer, AllowAddonAuthor)]
+                    'addon',
+                    [AnyOf(AllowUnlistedViewerOrReviewerReadOnly, AllowAddonAuthor)],
                 )
             ]
         elif not obj.is_public():
             # If the instance is disabled, only allow reviewers and authors.
             self.permission_classes = [
                 AllowRelatedObjectPermissions(
-                    'addon', [AnyOf(AllowListedViewerOrReviewer, AllowAddonAuthor)]
+                    'addon',
+                    [AnyOf(AllowListedViewerOrReviewerReadOnly, AllowAddonAuthor)],
                 )
             ]
 
@@ -879,8 +881,8 @@ def check_permissions(self, request):
                     AllowAddonOwner
                     if self.action in self.owner_actions
                     else AllowAddonAuthor,
-                    AllowListedViewerOrReviewer,
-                    AllowUnlistedViewerOrReviewer,
+                    AllowListedViewerOrReviewerReadOnly,
+                    AllowUnlistedViewerOrReviewerReadOnly,
                 ),
             ]
         )

src/olympia/api/permissions.py

@@ -178,6 +178,8 @@ class AllowListedViewerOrReviewer(BasePermission):
       'Addons:Review' or 'Addons:ContentReview' permission.
     """
 
+    disallow_unsafe = False
+
     def has_permission(self, request, view):
         return request.user.is_authenticated
 
@@ -186,13 +188,18 @@ def has_object_permission(self, request, view, obj):
             request.method in SAFE_METHODS
             and acl.action_allowed_for(request.user, permissions.REVIEWER_TOOLS_VIEW)
         )
-        can_access_because_listed_reviewer = obj.has_listed_versions(
-            include_deleted=True
-        ) and acl.is_reviewer(request.user, obj)
-
+        can_access_because_listed_reviewer = (
+            obj.has_listed_versions(include_deleted=True)
+            and acl.is_reviewer(request.user, obj)
+            and (not self.disallow_unsafe or request.method in SAFE_METHODS)
+        )
         return can_access_because_viewer or can_access_because_listed_reviewer
 
 
+class AllowListedViewerOrReviewerReadOnly(AllowListedViewerOrReviewer):
+    disallow_unsafe = True
+
+
 class AllowUnlistedViewerOrReviewer(AllowListedViewerOrReviewer):
     """Allow unlisted reviewers to access add-ons with unlisted versions, or
     add-ons with no listed versions at all.
@@ -219,7 +226,7 @@ def has_object_permission(self, request, view, obj):
         )
         can_access_because_unlisted_reviewer = acl.is_unlisted_addons_reviewer(
             request.user
-        )
+        ) and (not self.disallow_unsafe or request.method in SAFE_METHODS)
         has_unlisted_or_no_listed = obj.has_unlisted_versions(
             include_deleted=True
         ) or not obj.has_listed_versions(include_deleted=True)
@@ -229,6 +236,10 @@ def has_object_permission(self, request, view, obj):
         )
 
 
+class AllowUnlistedViewerOrReviewerReadOnly(AllowUnlistedViewerOrReviewer):
+    disallow_unsafe = True
+
+
 class AllowAnyKindOfReviewer(BasePermission):
     """Allow access to any kind of reviewer. Use only for views that don't
     alter add-on data.

src/olympia/api/tests/test_permissions.py

@@ -25,11 +25,13 @@
     AllowIfNotMozillaDisabled,
     AllowIfPublic,
     AllowListedViewerOrReviewer,
+    AllowListedViewerOrReviewerReadOnly,
     AllowNone,
     AllowOwner,
     AllowReadOnlyIfPublic,
     AllowRelatedObjectPermissions,
     AllowUnlistedViewerOrReviewer,
+    AllowUnlistedViewerOrReviewerReadOnly,
     AnyOf,
     ByHttpMethod,
     GroupPermission,
@@ -415,6 +417,65 @@ def test_no_listed_version_reviewer(self):
             assert not self.permission.has_object_permission(request, myview, obj)
 
 
+class TestAllowListedViewerOrReviewerReadOnly(TestAllowListedViewerOrReviewer):
+    def setUp(self):
+        super().setUp()
+        self.permission = AllowListedViewerOrReviewerReadOnly()
+
+    def test_admin(self):
+        user = user_factory()
+        self.grant_permission(user, '*:*')
+        obj = Mock(spec=[])
+        obj.type = amo.ADDON_EXTENSION
+        obj.has_listed_versions = lambda include_deleted=False: True
+
+        for method in self.safe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_permission(request, myview)
+            assert self.permission.has_object_permission(request, myview, obj)
+
+        for method in self.unsafe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_permission(request, myview)
+            assert not self.permission.has_object_permission(request, myview, obj)
+
+    def test_addon_reviewer(self):
+        user = user_factory()
+        self.grant_permission(user, 'Addons:Review')
+        obj = Mock(spec=[])
+        obj.type = amo.ADDON_EXTENSION
+        obj.has_listed_versions = lambda include_deleted=False: True
+
+        for method in self.safe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_object_permission(request, myview, obj)
+
+        for method in self.unsafe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert not self.permission.has_object_permission(request, myview, obj)
+
+    def test_theme_reviewer(self):
+        user = user_factory()
+        self.grant_permission(user, 'Addons:ThemeReview')
+        obj = Mock(spec=[])
+        obj.type = amo.ADDON_STATICTHEME
+        obj.has_listed_versions = lambda include_deleted=False: True
+
+        for method in self.safe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert self.permission.has_object_permission(request, myview, obj)
+
+        for method in self.unsafe_methods:
+            request = getattr(self.request_factory, method)('/')
+            request.user = user
+            assert not self.permission.has_object_permission(request, myview, obj)
+
+
 class TestAllowAnyKindOfReviewer(TestCase):
     # Note: be careful when testing, under the hood we're using a method that
     # relies on UserProfile.groups_list, which is cached on the UserProfile
@@ -587,6 +648,45 @@ def test_object_with_no_unlisted_versions_and_no_listed_versions_viewer(self):
         assert self.permission.has_object_permission(self.request, myview, obj)
 
 
+class TestAllowUnlistedViewerOrReviewerReadOnly(TestAllowUnlistedViewerOrReviewer):
+    def setUp(self):
+        super().setUp()
+        self.permission = AllowUnlistedViewerOrReviewerReadOnly()
+
+    def test_admin(self):
+        self.request.user = user_factory()
+        self.grant_permission(self.request.user, '*:*')
+        obj = Mock(spec=[])
+        obj.has_unlisted_versions = lambda include_deleted=False: True
+
+        assert self.permission.has_permission(self.request, myview)
+        assert not self.permission.has_object_permission(self.request, myview, obj)
+        self.request.method = 'GET'
+        assert self.permission.has_object_permission(self.request, myview, obj)
+
+    def test_unlisted_reviewer(self):
+        self.request.user = user_factory()
+        self.grant_permission(self.request.user, 'Addons:ReviewUnlisted')
+        obj = Mock(spec=[])
+        obj.has_unlisted_versions = lambda include_deleted=False: True
+
+        assert self.permission.has_permission(self.request, myview)
+        assert not self.permission.has_object_permission(self.request, myview, obj)
+        self.request.method = 'GET'
+        assert self.permission.has_object_permission(self.request, myview, obj)
+
+    def test_object_with_no_unlisted_versions_and_no_listed_versions(self):
+        self.request.user = user_factory()
+        self.grant_permission(self.request.user, 'Addons:ReviewUnlisted')
+        obj = Mock(spec=[])
+        obj.has_unlisted_versions = lambda include_deleted=False: False
+        obj.has_listed_versions = lambda include_deleted=False: False
+        assert self.permission.has_permission(self.request, myview)
+        assert not self.permission.has_object_permission(self.request, myview, obj)
+        self.request.method = 'GET'
+        assert self.permission.has_object_permission(self.request, myview, obj)
+
+
 class TestAllowIfPublic(TestCase):
     def setUp(self):
         self.permission = AllowIfPublic()