Filter versions API by channel, and expose channel property
This is a backwards-incompatible change, the values for the `filter` parameter are changing. In addition, permission checks are enforced more strictly, returning 401/403 when trying to use a filter that the client does not have the right permissions for.
This commit is contained in:
Mathieu Pillard
Родитель
b7f093626f
Коммит
ec9aa9e76d
|
|
@ -242,22 +242,25 @@ This endpoint allows you to list all versions belonging to a specific add-on.
|
|||
|
||||
.. _version-filtering-param:
|
||||
|
||||
By default, the version list API will only return versions with valid statuses
|
||||
By default, the version list API will only return public versions
|
||||
(excluding versions that have incomplete, disabled, deleted, rejected or
|
||||
flagged for further review files) - you can change that with the ``filter``
|
||||
query parameter, which may require authentication and specific permissions
|
||||
depending on the value:
|
||||
|
||||
================ ========================================================
|
||||
Value Description
|
||||
================ ========================================================
|
||||
all Show all versions attached to this add-on. Requires
|
||||
either reviewer permissions or a user account listed as
|
||||
a developer of the add-on.
|
||||
all_with_deleted Show all versions attached to this add-on, including
|
||||
deleted ones. Requires admin permissions.
|
||||
beta_only Show beta versions only.
|
||||
================ ========================================================
|
||||
==================== =====================================================
|
||||
Value Description
|
||||
==================== =====================================================
|
||||
all_without_unlisted Show all listed versions attached to this add-on.
|
||||
Requires either reviewer permissions or a user
|
||||
account listed as a developer of the add-on.
|
||||
all_with_unlisted Show all versions (including unlisted) attached to
|
||||
this add-on. Requires either reviewer permissions or
|
||||
a user account listed as a developer of the add-on.
|
||||
all_with_deleted Show all versions attached to this add-on, including
|
||||
deleted ones. Requires admin permissions.
|
||||
only_beta Show beta versions only.
|
||||
==================== =====================================================
|
||||
|
||||
--------------
|
||||
Version Detail
|
||||
|
|
@ -273,6 +276,7 @@ This endpoint allows you to fetch a single version belonging to a specific add-o
|
|||
|
||||
:query string lang: Activate translations in the specific language for that query. (See :ref:`translated fields <api-overview-translations>`)
|
||||
:>json int id: The version id.
|
||||
:>json string channel: The version channel, which determines its visibility on the site. Can be either ``unlisted`` or ``listed``.
|
||||
:>json string edit_url: The URL to the developer edit page for the version.
|
||||
:>json array files: Array holding information about the files for the version.
|
||||
:>json int files[].id: The id for a file.
|
||||
|
|
|
|||
|
|
@ -112,13 +112,14 @@ class SimpleVersionSerializer(serializers.ModelSerializer):
|
|||
|
||||
|
||||
class VersionSerializer(SimpleVersionSerializer):
|
||||
channel = ReverseChoiceField(choices=amo.CHANNEL_CHOICES_API.items())
|
||||
license = LicenseSerializer()
|
||||
release_notes = TranslationSerializerField(source='releasenotes')
|
||||
|
||||
class Meta:
|
||||
model = Version
|
||||
fields = ('id', 'compatibility', 'edit_url', 'files', 'license',
|
||||
'release_notes', 'reviewed', 'url', 'version')
|
||||
fields = ('id', 'channel', 'compatibility', 'edit_url', 'files',
|
||||
'license', 'release_notes', 'reviewed', 'url', 'version')
|
||||
|
||||
|
||||
class AddonEulaPolicySerializer(serializers.ModelSerializer):
|
||||
|
|
|
|||
|
|
@ -41,7 +41,8 @@ class AddonSerializerOutputTestMixin(object):
|
|||
result_file = data['files'][0]
|
||||
file_ = version.files.latest('pk')
|
||||
assert result_file['id'] == file_.pk
|
||||
assert result_file['created'] == file_.created.isoformat() + 'Z'
|
||||
assert result_file['created'] == (
|
||||
file_.created.replace(microsecond=0).isoformat() + 'Z')
|
||||
assert result_file['hash'] == file_.hash
|
||||
assert result_file['platform'] == (
|
||||
amo.PLATFORM_CHOICES_API[file_.platform])
|
||||
|
|
@ -159,9 +160,8 @@ class AddonSerializerOutputTestMixin(object):
|
|||
assert result['is_listed'] == self.addon.is_listed
|
||||
assert result['is_source_public'] == self.addon.view_source
|
||||
assert result['last_updated'] == (
|
||||
self.addon.last_updated.isoformat() + 'Z')
|
||||
self.addon.last_updated.replace(microsecond=0).isoformat() + 'Z')
|
||||
assert result['name'] == {'en-US': self.addon.name}
|
||||
|
||||
assert result['previews']
|
||||
assert len(result['previews']) == 2
|
||||
|
||||
|
|
@ -490,7 +490,7 @@ class TestVersionSerializerOutput(TestCase):
|
|||
|
||||
assert result['files'][0]['id'] == first_file.pk
|
||||
assert result['files'][0]['created'] == (
|
||||
first_file.created.isoformat() + 'Z')
|
||||
first_file.created.replace(microsecond=0).isoformat() + 'Z')
|
||||
assert result['files'][0]['hash'] == first_file.hash
|
||||
assert result['files'][0]['platform'] == 'windows'
|
||||
assert result['files'][0]['size'] == first_file.size
|
||||
|
|
@ -499,13 +499,14 @@ class TestVersionSerializerOutput(TestCase):
|
|||
|
||||
assert result['files'][1]['id'] == second_file.pk
|
||||
assert result['files'][1]['created'] == (
|
||||
second_file.created.isoformat() + 'Z')
|
||||
second_file.created.replace(microsecond=0).isoformat() + 'Z')
|
||||
assert result['files'][1]['hash'] == second_file.hash
|
||||
assert result['files'][1]['platform'] == 'mac'
|
||||
assert result['files'][1]['size'] == second_file.size
|
||||
assert result['files'][1]['status'] == 'public'
|
||||
assert result['files'][1]['url'] == second_file.get_url_path(src='')
|
||||
|
||||
assert result['channel'] == 'listed'
|
||||
assert result['edit_url'] == absolutify(addon.get_dev_url(
|
||||
'versions.edit', args=[self.version.pk], prefix_only=True))
|
||||
assert result['release_notes'] == {
|
||||
|
|
@ -520,9 +521,17 @@ class TestVersionSerializerOutput(TestCase):
|
|||
},
|
||||
'url': 'http://license.example.com/',
|
||||
}
|
||||
assert result['reviewed'] == now.isoformat() + 'Z'
|
||||
assert result['reviewed'] == (
|
||||
now.replace(microsecond=0).isoformat() + 'Z')
|
||||
assert result['url'] == absolutify(self.version.get_url_path())
|
||||
|
||||
def test_unlisted(self):
|
||||
addon = addon_factory()
|
||||
self.version = version_factory(
|
||||
addon=addon, channel=amo.RELEASE_CHANNEL_UNLISTED)
|
||||
result = self.serialize()
|
||||
assert result['channel'] == 'unlisted'
|
||||
|
||||
def test_no_license(self):
|
||||
addon = addon_factory()
|
||||
self.version = addon.current_version
|
||||
|
|
|
|||
|
|
@ -1749,7 +1749,7 @@ class TestAddonViewSetDetail(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
assert result['name'] == {'en-US': u'My Addôn'}
|
||||
assert result['slug'] == 'my-addon'
|
||||
assert result['last_updated'] == (
|
||||
self.addon.last_updated.isoformat() + 'Z')
|
||||
self.addon.last_updated.replace(microsecond=0).isoformat() + 'Z')
|
||||
return result
|
||||
|
||||
def _set_tested_url(self, param):
|
||||
|
|
@ -1852,14 +1852,14 @@ class TestVersionViewSetDetail(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
def test_disabled_version_anonymous(self):
|
||||
self.version.files.update(status=amo.STATUS_DISABLED)
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 404
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_disabled_version_user_but_not_author(self):
|
||||
user = UserProfile.objects.create(username='simpleuser')
|
||||
self.client.login_api(user)
|
||||
self.version.files.update(status=amo.STATUS_DISABLED)
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 404
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_deleted_version_reviewer(self):
|
||||
user = UserProfile.objects.create(username='reviewer')
|
||||
|
|
@ -1946,7 +1946,7 @@ class TestVersionViewSetList(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
self._test_url_only_contains_old_version()
|
||||
|
||||
# A reviewer can see disabled versions when explicitly asking for them.
|
||||
self._test_url(filter='all')
|
||||
self._test_url(filter='all_without_unlisted')
|
||||
|
||||
def test_disabled_version_author(self):
|
||||
user = UserProfile.objects.create(username='author')
|
||||
|
|
@ -1956,7 +1956,7 @@ class TestVersionViewSetList(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
self._test_url_only_contains_old_version()
|
||||
|
||||
# An author can see disabled versions when explicitly asking for them.
|
||||
self._test_url(filter='all')
|
||||
self._test_url(filter='all_without_unlisted')
|
||||
|
||||
def test_disabled_version_admin(self):
|
||||
user = UserProfile.objects.create(username='admin')
|
||||
|
|
@ -1966,19 +1966,29 @@ class TestVersionViewSetList(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
self._test_url_only_contains_old_version()
|
||||
|
||||
# An admin can see disabled versions when explicitly asking for them.
|
||||
self._test_url(filter='all')
|
||||
self._test_url(filter='all_without_unlisted')
|
||||
|
||||
def test_disabled_version_anonymous(self):
|
||||
self.version.files.update(status=amo.STATUS_DISABLED)
|
||||
self._test_url_only_contains_old_version()
|
||||
self._test_url_only_contains_old_version(filter='all')
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_without_unlisted'})
|
||||
assert response.status_code == 401
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_with_deleted'})
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_disabled_version_user_but_not_author(self):
|
||||
user = UserProfile.objects.create(username='simpleuser')
|
||||
self.client.login_api(user)
|
||||
self.version.files.update(status=amo.STATUS_DISABLED)
|
||||
self._test_url_only_contains_old_version()
|
||||
self._test_url_only_contains_old_version(filter='all')
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_without_unlisted'})
|
||||
assert response.status_code == 403
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_with_deleted'})
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_deleted_version_reviewer(self):
|
||||
user = UserProfile.objects.create(username='reviewer')
|
||||
|
|
@ -1986,8 +1996,10 @@ class TestVersionViewSetList(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
self.client.login_api(user)
|
||||
self.version.delete()
|
||||
self._test_url_only_contains_old_version()
|
||||
self._test_url_only_contains_old_version(filter='all')
|
||||
self._test_url_only_contains_old_version(filter='all_with_deleted')
|
||||
self._test_url_only_contains_old_version(filter='all_without_unlisted')
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_with_deleted'})
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_deleted_version_author(self):
|
||||
user = UserProfile.objects.create(username='author')
|
||||
|
|
@ -1995,8 +2007,10 @@ class TestVersionViewSetList(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
self.client.login_api(user)
|
||||
self.version.delete()
|
||||
self._test_url_only_contains_old_version()
|
||||
self._test_url_only_contains_old_version(filter='all')
|
||||
self._test_url_only_contains_old_version(filter='all_with_deleted')
|
||||
self._test_url_only_contains_old_version(filter='all_without_unlisted')
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_with_deleted'})
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_deleted_version_admin(self):
|
||||
user = UserProfile.objects.create(username='admin')
|
||||
|
|
@ -2004,7 +2018,7 @@ class TestVersionViewSetList(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
self.client.login_api(user)
|
||||
self.version.delete()
|
||||
self._test_url_only_contains_old_version()
|
||||
self._test_url_only_contains_old_version(filter='all')
|
||||
self._test_url_only_contains_old_version(filter='all_without_unlisted')
|
||||
|
||||
# An admin can see deleted versions when explicitly asking for them.
|
||||
self._test_url(filter='all_with_deleted')
|
||||
|
|
@ -2012,20 +2026,29 @@ class TestVersionViewSetList(AddonAndVersionViewSetDetailMixin, TestCase):
|
|||
def test_deleted_version_anonymous(self):
|
||||
self.version.delete()
|
||||
self._test_url_only_contains_old_version()
|
||||
self._test_url_only_contains_old_version(filter='all')
|
||||
self._test_url_only_contains_old_version(filter='all_with_deleted')
|
||||
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_without_unlisted'})
|
||||
assert response.status_code == 401
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_with_deleted'})
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_deleted_version_user_but_not_author(self):
|
||||
user = UserProfile.objects.create(username='simpleuser')
|
||||
self.client.login_api(user)
|
||||
self.version.delete()
|
||||
self._test_url_only_contains_old_version()
|
||||
self._test_url_only_contains_old_version(filter='all')
|
||||
self._test_url_only_contains_old_version(filter='all_with_deleted')
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_without_unlisted'})
|
||||
assert response.status_code == 403
|
||||
response = self.client.get(
|
||||
self.url, data={'filter': 'all_with_deleted'})
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_beta_version(self):
|
||||
self.old_version.files.update(status=amo.STATUS_BETA)
|
||||
self._test_url_only_contains_old_version(filter='beta_only')
|
||||
self._test_url_only_contains_old_version(filter='only_beta')
|
||||
|
||||
|
||||
class TestAddonViewSetFeatureCompatibility(TestCase):
|
||||
|
|
@ -2135,7 +2158,8 @@ class TestAddonSearchView(ESTestCase):
|
|||
assert result['id'] == addon.pk
|
||||
assert result['name'] == {'en-US': u'My Addôn'}
|
||||
assert result['slug'] == 'my-addon'
|
||||
assert result['last_updated'] == addon.last_updated.isoformat() + 'Z'
|
||||
assert result['last_updated'] == (
|
||||
addon.last_updated.replace(microsecond=0).isoformat() + 'Z')
|
||||
|
||||
# latest_unlisted_version should never be exposed in public search.
|
||||
assert 'latest_unlisted_version' not in result
|
||||
|
|
|
|||
|
|
@ -49,7 +49,8 @@ from olympia import paypal
|
|||
from olympia.api.paginator import ESPageNumberPagination
|
||||
from olympia.api.permissions import (
|
||||
AllowAddonAuthor, AllowReadOnlyIfReviewedAndListed,
|
||||
AllowRelatedObjectPermissions, AllowReviewer, AllowReviewerUnlisted, AnyOf)
|
||||
AllowRelatedObjectPermissions, AllowReviewer, AllowReviewerUnlisted, AnyOf,
|
||||
GroupPermission)
|
||||
from olympia.reviews.forms import ReviewForm
|
||||
from olympia.reviews.models import Review, GroupedRating
|
||||
from olympia.search.filters import (
|
||||
|
|
@ -706,51 +707,87 @@ class AddonChildMixin(object):
|
|||
|
||||
class AddonVersionViewSet(AddonChildMixin, RetrieveModelMixin,
|
||||
ListModelMixin, GenericViewSet):
|
||||
# Permissions are checked against the parent add-on.
|
||||
permission_classes = [
|
||||
AllowRelatedObjectPermissions('addon', AddonViewSet.permission_classes)
|
||||
]
|
||||
serializer_class = VersionSerializer
|
||||
# Since permission checks are done on the parent add-on, we rely on
|
||||
# queryset filtering to hide non-valid versions. get_queryset() might
|
||||
# override this if we are asked to see non-valid versions explicitly.
|
||||
# Permissions are always checked against the parent add-on in
|
||||
# get_addon_object() using AddonViewSet.permission_classes so we don't need
|
||||
# to set any here. Some extra permission classes are added dynamically
|
||||
# below in check_permissions() and check_object_permissions() depending on
|
||||
# what the client is requesting to see.
|
||||
permission_classes = []
|
||||
# By default, we rely on queryset filtering to hide non-public/unlisted
|
||||
# versions. get_queryset() might override this if we are asked to see
|
||||
# non-valid, deleted and/or unlisted versions explicitly.
|
||||
queryset = Version.objects.filter(
|
||||
files__status__in=amo.VALID_FILE_STATUSES).distinct()
|
||||
files__status=amo.STATUS_PUBLIC,
|
||||
channel=amo.RELEASE_CHANNEL_LISTED).distinct()
|
||||
serializer_class = VersionSerializer
|
||||
|
||||
def check_permissions(self, request):
|
||||
if self.action == 'list':
|
||||
requested = self.request.GET.get('filter')
|
||||
if requested == 'all_with_deleted':
|
||||
# To see deleted versions, you need Admin:%.
|
||||
self.permission_classes = [GroupPermission('Admin', '%')]
|
||||
elif requested == 'all_with_unlisted':
|
||||
# To see unlisted versions, you need to be add-on author or
|
||||
# unlisted reviewer.
|
||||
self.permission_classes = [AnyOf(
|
||||
AllowReviewerUnlisted, AllowAddonAuthor)]
|
||||
elif requested == 'all_without_unlisted':
|
||||
# To see all listed versions (not just public ones) you need to
|
||||
# be add-on author or reviewer.
|
||||
self.permission_classes = [AnyOf(
|
||||
AllowReviewer, AllowAddonAuthor)]
|
||||
# When listing, we can't use AllowRelatedObjectPermissions() with
|
||||
# check_permissions(), because AllowAddonAuthor needs an author to
|
||||
# do the actual permission check. To work around that, we call
|
||||
# super + check_object_permission() ourselves, passing down the
|
||||
# addon object directly.
|
||||
return super(AddonVersionViewSet, self).check_object_permissions(
|
||||
request, self.get_addon_object())
|
||||
super(AddonVersionViewSet, self).check_permissions(request)
|
||||
|
||||
def check_object_permissions(self, request, obj):
|
||||
# If the instance is marked as deleted and the client is not allowed to
|
||||
# see deleted instances, we want to return a 404, behaving as if it
|
||||
# does not exist.
|
||||
if (obj.deleted and
|
||||
not GroupPermission('Admin', '%').has_object_permission(
|
||||
request, self, obj)):
|
||||
raise http.Http404
|
||||
|
||||
if obj.channel == amo.RELEASE_CHANNEL_UNLISTED:
|
||||
# If the instance is unlisted, only allow unlisted reviewers and
|
||||
# authors..
|
||||
self.permission_classes = [
|
||||
AllowRelatedObjectPermissions(
|
||||
'addon', [AnyOf(AllowReviewerUnlisted, AllowAddonAuthor)])
|
||||
]
|
||||
elif not obj.is_public():
|
||||
# If the instance is disabled, only allow reviewers and authors.
|
||||
self.permission_classes = [
|
||||
AllowRelatedObjectPermissions(
|
||||
'addon', [AnyOf(AllowReviewer, AllowAddonAuthor)])
|
||||
]
|
||||
|
||||
super(AddonVersionViewSet, self).check_object_permissions(request, obj)
|
||||
|
||||
def get_queryset(self):
|
||||
"""Return the right base queryset depending on the situation. Note that
|
||||
permissions checks still apply on top of that, against the add-on
|
||||
as per check_object_permissions() above."""
|
||||
"""Return the right base queryset depending on the situation."""
|
||||
requested = self.request.GET.get('filter')
|
||||
|
||||
# By default we restrict to valid versions. However:
|
||||
#
|
||||
# When accessing a single version or if requesting it explicitly when
|
||||
# listing, admins can access all versions, including deleted ones.
|
||||
should_access_all_versions_included_deleted = (
|
||||
(requested == 'all_with_deleted' or self.action != 'list') and
|
||||
self.request.user.is_authenticated() and
|
||||
self.request.user.is_staff)
|
||||
|
||||
# When accessing a single version or if requesting it explicitly when
|
||||
# listing, reviewers and add-on authors can access all non-deleted
|
||||
# versions.
|
||||
should_access_all_versions = (
|
||||
(requested == 'all' or self.action != 'list') and
|
||||
(AllowReviewer().has_permission(self.request, self) or
|
||||
AllowAddonAuthor().has_object_permission(
|
||||
self.request, self, self.get_addon_object())))
|
||||
|
||||
# Everyone can see (non deleted) beta version when they request it
|
||||
# explicitly.
|
||||
should_access_only_beta_versions = (requested == 'beta_only')
|
||||
|
||||
if should_access_all_versions_included_deleted:
|
||||
# By default we restrict to valid, listed versions. Some filtering
|
||||
# options are available when listing, and in addition, when returning
|
||||
# a single instance, we don't filter at all.
|
||||
if requested == 'all_with_deleted' or self.action != 'list':
|
||||
self.queryset = Version.unfiltered.all()
|
||||
elif should_access_all_versions:
|
||||
elif requested == 'all_with_unlisted':
|
||||
self.queryset = Version.objects.all()
|
||||
elif should_access_only_beta_versions:
|
||||
elif requested == 'all_without_unlisted':
|
||||
self.queryset = Version.objects.filter(
|
||||
channel=amo.RELEASE_CHANNEL_LISTED)
|
||||
elif requested == 'only_beta':
|
||||
self.queryset = Version.objects.filter(
|
||||
channel=amo.RELEASE_CHANNEL_LISTED,
|
||||
files__status=amo.STATUS_BETA).distinct()
|
||||
|
||||
# Now that the base queryset has been altered, call super() to use it.
|
||||
|
|
|
|||
|
|
@ -130,8 +130,8 @@ class AllowReviewerUnlisted(AllowReviewer):
|
|||
|
||||
class AllowIfReviewedAndListed(BasePermission):
|
||||
"""
|
||||
Allow access when the object's is_public() method and is_listed property
|
||||
both return True.
|
||||
Allow access when the object's is_reviewed() method and is_listed property
|
||||
both return True, and the disabled_by_user property is False.
|
||||
"""
|
||||
def has_permission(self, request, view):
|
||||
return True
|
||||
|
|
|
|||
|
|
@ -88,6 +88,11 @@ RELEASE_CHANNEL_CHOICES = (
|
|||
(RELEASE_CHANNEL_LISTED, _(u'Listed')),
|
||||
)
|
||||
|
||||
CHANNEL_CHOICES_API = {
|
||||
RELEASE_CHANNEL_UNLISTED: 'unlisted',
|
||||
RELEASE_CHANNEL_LISTED: 'listed',
|
||||
}
|
||||
|
||||
CHANNEL_CHOICES_LOOKUP = {
|
||||
'unlisted': RELEASE_CHANNEL_UNLISTED,
|
||||
'listed': RELEASE_CHANNEL_LISTED,
|
||||
|
|
|
|||
|
|
@ -263,7 +263,7 @@ class Version(OnChangeMixin, ModelBase):
|
|||
@property
|
||||
def is_user_disabled(self):
|
||||
return self.files.filter(status=amo.STATUS_DISABLED).exclude(
|
||||
original_status=amo.STATUS_NULL).count()
|
||||
original_status=amo.STATUS_NULL).exists()
|
||||
|
||||
@is_user_disabled.setter
|
||||
def is_user_disabled(self, disable):
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче