diff --git a/src/olympia/addons/tests/test_views.py b/src/olympia/addons/tests/test_views.py index fae091b03c21..d13d8108d10e 100644 --- a/src/olympia/addons/tests/test_views.py +++ b/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'}) diff --git a/src/olympia/addons/views.py b/src/olympia/addons/views.py index 40ef667d7d7d..f661b9b1fdfc 100644 --- a/src/olympia/addons/views.py +++ b/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, ), ] ) diff --git a/src/olympia/api/permissions.py b/src/olympia/api/permissions.py index 55cdf6f9bd36..d7bf9f1bccdf 100644 --- a/src/olympia/api/permissions.py +++ b/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. diff --git a/src/olympia/api/tests/test_permissions.py b/src/olympia/api/tests/test_permissions.py index 6f17e43a330e..0038f667b944 100644 --- a/src/olympia/api/tests/test_permissions.py +++ b/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()