Add AbuseReport.addon_install_source ; relax addon_install_method
AbuseReport.addon_install_source is added to match what Firefox has internally, and to help support future versions that may introduce new possible values, the AbuseReport serializer now accepts unknown values for addon_install_method and addon_install_source and turns them into "other" instead of returning a 400.
This commit is contained in:
Родитель
a712101c0b
Коммит
abb7719af0
|
@ -26,12 +26,19 @@ to if necessary.
|
|||
Except for the ``message``, all strings have a maximum length of 255 characters
|
||||
and should be truncated by the client where necessary.
|
||||
|
||||
.. warning::
|
||||
|
||||
For ``addon_install_method`` and ``addon_install_source`` specifically,
|
||||
if an unsupported value is sent, it will be silently changed to ``other``
|
||||
instead of raising a 400 error.
|
||||
|
||||
.. http:post:: /api/v4/abuse/report/addon/
|
||||
|
||||
:<json string addon: The id, slug, or guid of the add-on to report for abuse (required).
|
||||
:<json string message: The body/content of the abuse report (required).
|
||||
:<json string|null report_entry_point: The report entry point. The accepted values are documented in the :ref:`table below <abuse-report_entry_point-parameter>`.
|
||||
:<json string|null addon_install_method: The add-on install method. The accepted values are documented in the :ref:`table below <abuse-addon_install_method-parameter>`.
|
||||
:<json string|null addon_install_source: The add-on install source. The accepted values are documented in the :ref:`table below <abuse-addon_install_source-parameter>`.
|
||||
:<json string|null addon_install_origin: The add-on install origin.
|
||||
:<json string|null addon_name: The add-on name in the locale used by the client.
|
||||
:<json string|null addon_signature: The add-on signature state. The accepted values are documented in the :ref:`table below <abuse-addon_signature-parameter>`.
|
||||
|
@ -57,6 +64,7 @@ to if necessary.
|
|||
:>json string message: The body/content of the abuse report.
|
||||
:>json string|null report_entry_point: The report entry point.
|
||||
:>json string|null addon_install_method: The add-on install method.
|
||||
:>json string|null addon_install_source: The add-on install source.
|
||||
:>json string|null addon_install_origin: The add-on install origin.
|
||||
:>json string|null addon_name: The add-on name in the locale used by the client.
|
||||
:>json string|null addon_signature: The add-on signature state.
|
||||
|
@ -87,6 +95,10 @@ to if necessary.
|
|||
|
||||
Accepted values for the ``addon_install_method`` parameter:
|
||||
|
||||
.. note::
|
||||
|
||||
This should match what is documented for ``addonsManager.install.extra_keys.method`` in `Firefox telemetry event definition <https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Events.yaml>`_. The values are normalized by being converted to lowercase with the ``:`` and ``-`` characters converted to ``_``. In addition, extra values are supported for backwards-compatibility purposes, since Firefox before version 70 merged source and method into the same value. If an unsupported value is sent for this parameter, it will be silently changed to special ``other`` instead of raising a 400 error.
|
||||
|
||||
=========================== =================================================
|
||||
Value Description
|
||||
=========================== =================================================
|
||||
|
@ -98,11 +110,44 @@ to if necessary.
|
|||
drag_and_drop Drag & Drop
|
||||
sideload Sideload
|
||||
file_url File URL
|
||||
enterprise_policy Enterprise Policy
|
||||
url URL
|
||||
other Other
|
||||
enterprise_policy Enterprise Policy (obsolete, for backwards-compatibility)
|
||||
distribution Included in build (obsolete, for backwards-compatibility)
|
||||
system_addon System Add-on (obsolete, for backwards-compatibility)
|
||||
temporary_addon Temporary Add-on (obsolete, for backwards-compatibility)
|
||||
sync Sync (obsolete, for backwards-compatibility)
|
||||
=========================== =================================================
|
||||
|
||||
.. _abuse-addon_install_source-parameter:
|
||||
|
||||
Accepted values for the ``addon_install_source`` parameter:
|
||||
|
||||
.. note::
|
||||
|
||||
This should match what is documented for ``addonsManager.install.extra_keys.method`` in `Firefox telemetry event definition <https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Events.yaml>`_. The values are normalized by being converted to lowercase with the ``:`` and ``-`` characters converted to ``_``. We support the additional ``other`` value as a catch-all. If an unsupported value is sent for this parameter, it will be silently changed to ``other`` instead of raising a 400 error.
|
||||
|
||||
=========================== =================================================
|
||||
Value Description
|
||||
=========================== =================================================
|
||||
\about:addons Add-ons Manager
|
||||
\about:debugging Add-ons Debugging
|
||||
\about:preferences Preferences
|
||||
amo AMO
|
||||
\app:profile App Profile
|
||||
disco Disco Pane
|
||||
distribution Included in build
|
||||
system_addon System Add-on
|
||||
temporary_addon Temporary Add-on
|
||||
extension Extension
|
||||
enterprise-policy Enterprise Policy
|
||||
file-url File URL
|
||||
gmp-plugin GMP Plugin
|
||||
internal Internal
|
||||
plugin Plugin
|
||||
rtamo Return To AMO
|
||||
sync Sync
|
||||
system-addon System Add-on
|
||||
temporary-addon Temporary Add-on
|
||||
unknown Unknown
|
||||
other Other
|
||||
=========================== =================================================
|
||||
|
||||
|
|
|
@ -196,6 +196,7 @@ class AbuseReportAdmin(CommaSearchInAdminMixin, admin.ModelAdmin):
|
|||
'install_date',
|
||||
'addon_install_origin',
|
||||
'addon_install_method',
|
||||
'addon_install_source',
|
||||
'report_entry_point',
|
||||
'addon_card',
|
||||
)
|
||||
|
@ -217,6 +218,7 @@ class AbuseReportAdmin(CommaSearchInAdminMixin, admin.ModelAdmin):
|
|||
'install_date',
|
||||
'addon_install_origin',
|
||||
'addon_install_method',
|
||||
'addon_install_source',
|
||||
'report_entry_point'
|
||||
)})
|
||||
)
|
||||
|
|
|
@ -64,6 +64,9 @@ class AbuseReport(ModelBase):
|
|||
('OTHER', 127, 'Other'),
|
||||
)
|
||||
|
||||
# https://searchfox.org
|
||||
# /mozilla-central/source/toolkit/components/telemetry/Events.yaml#122-131
|
||||
# Firefox submits values in lowercase, with '-' and ':' changed to '_'.
|
||||
ADDON_INSTALL_METHODS = APIChoicesWithNone(
|
||||
('AMWEBAPI', 1, 'Add-on Manager Web API'),
|
||||
('LINK', 2, 'Direct link'),
|
||||
|
@ -72,12 +75,42 @@ class AbuseReport(ModelBase):
|
|||
('MANAGEMENT_WEBEXT_API', 5, 'Webext management API'),
|
||||
('DRAG_AND_DROP', 6, 'Drag & Drop'),
|
||||
('SIDELOAD', 7, 'Sideload'),
|
||||
# Values between 8 and 13 are obsolete, we use to merge
|
||||
# install source and method into addon_install_method before deciding
|
||||
# to split the two like Firefox does, so these 6 values are only kept
|
||||
# for backwards-compatibility with older reports and older versions of
|
||||
# Firefox that still only submit that.
|
||||
('FILE_URL', 8, 'File URL'),
|
||||
('ENTERPRISE_POLICY', 9, 'Enterprise Policy'),
|
||||
('DISTRIBUTION', 10, 'Included in build'),
|
||||
('SYSTEM_ADDON', 11, 'System Add-on'),
|
||||
('TEMPORARY_ADDON', 12, 'Temporary Add-on'),
|
||||
('SYNC', 13, 'Sync'),
|
||||
# Back to normal values.
|
||||
('URL', 14, 'URL'),
|
||||
# Our own catch-all. The serializer expects it to be called "OTHER".
|
||||
('OTHER', 127, 'Other'),
|
||||
)
|
||||
ADDON_INSTALL_SOURCES = APIChoicesWithNone(
|
||||
('ABOUT_ADDONS', 1, 'Add-ons Manager'),
|
||||
('ABOUT_DEBUGGING', 2, 'Add-ons Debugging'),
|
||||
('ABOUT_PREFERENCES', 3, 'Preferences'),
|
||||
('AMO', 4, 'AMO'),
|
||||
('APP:PROFILE', 5, 'app:profile'),
|
||||
('DISCO', 6, 'Disco Pane'),
|
||||
('DISTRIBUTION', 7, 'Included in build'),
|
||||
('EXTENSION', 8, 'Extension'),
|
||||
('ENTERPRISE_POLICY', 9, 'Enterprise Policy'),
|
||||
('FILE_URL', 10, 'File URL'),
|
||||
('GMP_PLUGIN', 11, 'GMP Plugin'),
|
||||
('INTERNAL', 12, 'Internal'),
|
||||
('PLUGIN', 13, 'Plugin'),
|
||||
('RTAMO', 14, 'Return to AMO'),
|
||||
('SYNC', 15, 'Sync'),
|
||||
('SYSTEM_ADDON', 16, 'System Add-on'),
|
||||
('TEMPORARY_ADDON', 17, 'Temporary Add-on'),
|
||||
('UNKNOWN', 18, 'Unknown'),
|
||||
# Our own catch-all. The serializer expects it to be called "OTHER".
|
||||
('OTHER', 127, 'Other'),
|
||||
)
|
||||
REPORT_ENTRY_POINTS = APIChoicesWithNone(
|
||||
|
@ -148,6 +181,9 @@ class AbuseReport(ModelBase):
|
|||
addon_install_method = models.PositiveSmallIntegerField(
|
||||
default=None, choices=ADDON_INSTALL_METHODS.choices, blank=True,
|
||||
null=True)
|
||||
addon_install_source = models.PositiveSmallIntegerField(
|
||||
default=None, choices=ADDON_INSTALL_SOURCES.choices, blank=True,
|
||||
null=True)
|
||||
report_entry_point = models.PositiveSmallIntegerField(
|
||||
default=None, choices=REPORT_ENTRY_POINTS.choices, blank=True,
|
||||
null=True)
|
||||
|
|
|
@ -2,12 +2,17 @@ from rest_framework import serializers
|
|||
|
||||
from django.utils.translation import ugettext_lazy as _
|
||||
|
||||
import olympia.core.logger
|
||||
|
||||
from olympia import amo
|
||||
from olympia.abuse.models import AbuseReport
|
||||
from olympia.accounts.serializers import BaseUserSerializer
|
||||
from olympia.api.fields import ReverseChoiceField
|
||||
|
||||
|
||||
log = olympia.core.logger.getLogger('z.abuse')
|
||||
|
||||
|
||||
class BaseAbuseReportSerializer(serializers.ModelSerializer):
|
||||
reporter = BaseUserSerializer(read_only=True)
|
||||
|
||||
|
@ -63,6 +68,9 @@ class AddonAbuseReportSerializer(BaseAbuseReportSerializer):
|
|||
addon_install_method = ReverseChoiceField(
|
||||
choices=list(AbuseReport.ADDON_INSTALL_METHODS.api_choices),
|
||||
required=False, allow_null=True)
|
||||
addon_install_source = ReverseChoiceField(
|
||||
choices=list(AbuseReport.ADDON_INSTALL_SOURCES.api_choices),
|
||||
required=False, allow_null=True)
|
||||
addon_signature = ReverseChoiceField(
|
||||
choices=list(AbuseReport.ADDON_SIGNATURES.api_choices),
|
||||
required=False, allow_null=True)
|
||||
|
@ -73,6 +81,7 @@ class AddonAbuseReportSerializer(BaseAbuseReportSerializer):
|
|||
'addon',
|
||||
'addon_install_method',
|
||||
'addon_install_origin',
|
||||
'addon_install_source',
|
||||
'addon_name',
|
||||
'addon_signature',
|
||||
'addon_summary',
|
||||
|
@ -105,7 +114,28 @@ class AddonAbuseReportSerializer(BaseAbuseReportSerializer):
|
|||
})
|
||||
return data
|
||||
|
||||
def handle_unknown_install_method_or_source(self, data, field_name):
|
||||
reversed_choices = self.fields[field_name].reversed_choices
|
||||
value = data[field_name]
|
||||
if value not in reversed_choices:
|
||||
log.warning('Unknown abuse report %s value submitted: %s',
|
||||
field_name, data[field_name])
|
||||
value = 'other'
|
||||
return value
|
||||
|
||||
def to_internal_value(self, data):
|
||||
# We want to accept unknown incoming data for `addon_install_method`
|
||||
# and `addon_install_source`, we have to transform it here, we can't
|
||||
# do it in a custom validation method because validation would be
|
||||
# skipped entirely if the value is not a valid choice.
|
||||
if 'addon_install_method' in data:
|
||||
data['addon_install_method'] = (
|
||||
self.handle_unknown_install_method_or_source(
|
||||
data, 'addon_install_method'))
|
||||
if 'addon_install_source' in data:
|
||||
data['addon_install_source'] = (
|
||||
self.handle_unknown_install_method_or_source(
|
||||
data, 'addon_install_source'))
|
||||
self.validate_target(data, 'addon')
|
||||
view = self.context.get('view')
|
||||
output = view.get_guid_and_addon()
|
||||
|
|
|
@ -80,6 +80,7 @@ class TestAbuse(TestCase):
|
|||
(11, 'System Add-on'),
|
||||
(12, 'Temporary Add-on'),
|
||||
(13, 'Sync'),
|
||||
(14, 'URL'),
|
||||
(127, 'Other')
|
||||
)
|
||||
|
||||
|
@ -98,6 +99,53 @@ class TestAbuse(TestCase):
|
|||
(11, 'system_addon'),
|
||||
(12, 'temporary_addon'),
|
||||
(13, 'sync'),
|
||||
(14, 'url'),
|
||||
(127, 'other')
|
||||
)
|
||||
|
||||
assert AbuseReport.ADDON_INSTALL_SOURCES.choices == (
|
||||
(None, 'None'),
|
||||
(1, 'Add-ons Manager'),
|
||||
(2, 'Add-ons Debugging'),
|
||||
(3, 'Preferences'),
|
||||
(4, 'AMO'),
|
||||
(5, 'app:profile'),
|
||||
(6, 'Disco Pane'),
|
||||
(7, 'Included in build'),
|
||||
(8, 'Extension'),
|
||||
(9, 'Enterprise Policy'),
|
||||
(10, 'File URL'),
|
||||
(11, 'GMP Plugin'),
|
||||
(12, 'Internal'),
|
||||
(13, 'Plugin'),
|
||||
(14, 'Return to AMO'),
|
||||
(15, 'Sync'),
|
||||
(16, 'System Add-on'),
|
||||
(17, 'Temporary Add-on'),
|
||||
(18, 'Unknown'),
|
||||
(127, 'Other')
|
||||
)
|
||||
|
||||
assert AbuseReport.ADDON_INSTALL_SOURCES.api_choices == (
|
||||
(None, None),
|
||||
(1, 'about_addons'),
|
||||
(2, 'about_debugging'),
|
||||
(3, 'about_preferences'),
|
||||
(4, 'amo'),
|
||||
(5, 'app:profile'),
|
||||
(6, 'disco'),
|
||||
(7, 'distribution'),
|
||||
(8, 'extension'),
|
||||
(9, 'enterprise_policy'),
|
||||
(10, 'file_url'),
|
||||
(11, 'gmp_plugin'),
|
||||
(12, 'internal'),
|
||||
(13, 'plugin'),
|
||||
(14, 'rtamo'),
|
||||
(15, 'sync'),
|
||||
(16, 'system_addon'),
|
||||
(17, 'temporary_addon'),
|
||||
(18, 'unknown'),
|
||||
(127, 'other')
|
||||
)
|
||||
|
||||
|
|
|
@ -33,6 +33,7 @@ class TestAddonAbuseReportSerializer(TestCase):
|
|||
'message': 'bad stuff',
|
||||
'addon_install_method': None,
|
||||
'addon_install_origin': None,
|
||||
'addon_install_source': None,
|
||||
'addon_name': None,
|
||||
'addon_signature': None,
|
||||
'addon_summary': None,
|
||||
|
@ -61,6 +62,7 @@ class TestAddonAbuseReportSerializer(TestCase):
|
|||
'message': 'bad stuff',
|
||||
'addon_install_method': None,
|
||||
'addon_install_origin': None,
|
||||
'addon_install_source': None,
|
||||
'addon_name': None,
|
||||
'addon_signature': None,
|
||||
'addon_summary': None,
|
||||
|
@ -92,8 +94,9 @@ class TestAddonAbuseReportSerializer(TestCase):
|
|||
data = {
|
||||
'addon': '@someguid',
|
||||
'message': u'I am the messagê',
|
||||
'addon_install_method': 'amwebapi',
|
||||
'addon_install_method': 'url',
|
||||
'addon_install_origin': 'http://somewhere.com/',
|
||||
'addon_install_source': 'amo',
|
||||
'addon_name': u'Fancy add-on nâme',
|
||||
'addon_signature': None,
|
||||
'addon_summary': u'A summary',
|
||||
|
@ -112,8 +115,9 @@ class TestAddonAbuseReportSerializer(TestCase):
|
|||
data, context=extra_context).to_internal_value(data)
|
||||
expected = {
|
||||
'addon': None,
|
||||
'addon_install_method': AbuseReport.ADDON_INSTALL_METHODS.AMWEBAPI,
|
||||
'addon_install_method': AbuseReport.ADDON_INSTALL_METHODS.URL,
|
||||
'addon_install_origin': 'http://somewhere.com/',
|
||||
'addon_install_source': AbuseReport.ADDON_INSTALL_SOURCES.AMO,
|
||||
'addon_name': u'Fancy add-on nâme',
|
||||
'addon_signature': None,
|
||||
'addon_summary': 'A summary',
|
||||
|
|
|
@ -214,7 +214,7 @@ class AddonAbuseViewSetTestBase(object):
|
|||
'install_date': '2004-08-15T16:23:42',
|
||||
'reason': 'spam',
|
||||
'addon_install_origin': 'http://example.com/',
|
||||
'addon_install_method': None,
|
||||
'addon_install_method': 'url',
|
||||
'report_entry_point': None,
|
||||
}
|
||||
response = self.client.post(
|
||||
|
@ -240,7 +240,9 @@ class AddonAbuseViewSetTestBase(object):
|
|||
assert report.application_locale == data['lang']
|
||||
assert report.install_date == datetime(2004, 8, 15, 16, 23, 42)
|
||||
assert report.reason == 2 # Spam / Advertising
|
||||
assert report.addon_install_method is None
|
||||
assert report.addon_install_method == (
|
||||
AbuseReport.ADDON_INSTALL_METHODS.URL)
|
||||
assert report.addon_install_source is None
|
||||
assert report.report_entry_point is None
|
||||
|
||||
def test_optional_fields_errors(self):
|
||||
|
@ -260,6 +262,7 @@ class AddonAbuseViewSetTestBase(object):
|
|||
'reason': 'Something not in reason choices',
|
||||
'addon_install_origin': 'u' * 256,
|
||||
'addon_install_method': 'Something not in install method choices',
|
||||
'addon_install_source': 'Something not in install source choices',
|
||||
'report_entry_point': 'Something not in entrypoint choices',
|
||||
}
|
||||
response = self.client.post(
|
||||
|
@ -286,11 +289,33 @@ class AddonAbuseViewSetTestBase(object):
|
|||
'instead: YYYY-MM-DDThh:mm[:ss[.uuuuuu]][+HH:MM|-HH:MM|Z].'],
|
||||
'reason': [expected_choices_message % data['reason']],
|
||||
'addon_install_origin': [expected_max_length_message % 255],
|
||||
'addon_install_method': [
|
||||
expected_choices_message % data['addon_install_method']],
|
||||
'report_entry_point': [
|
||||
expected_choices_message % data['report_entry_point']],
|
||||
}
|
||||
# Note: addon_install_method and addon_install_source silently convert
|
||||
# unknown values to "other", so the values submitted here, despite not
|
||||
# being valid choices, aren't considered errors. See
|
||||
# test_addon_unknown_install_source_and_method() below.
|
||||
|
||||
def test_addon_unknown_install_source_and_method(self):
|
||||
data = {
|
||||
'addon': '@mysteryaddon',
|
||||
'message': 'This is abusé!',
|
||||
'addon_install_method': 'something unexpected',
|
||||
'addon_install_source': 'something unexpected indeed',
|
||||
}
|
||||
response = self.client.post(self.url, data=data)
|
||||
assert response.status_code == 201, response.content
|
||||
|
||||
assert AbuseReport.objects.filter(guid=data['addon']).exists()
|
||||
report = AbuseReport.objects.get(guid=data['addon'])
|
||||
self.check_report(
|
||||
report, u'[Addon] Abuse Report for %s' % data['addon'])
|
||||
assert not report.addon # Not an add-on in database, that's ok.
|
||||
assert report.addon_install_method == (
|
||||
AbuseReport.ADDON_INSTALL_METHODS.OTHER)
|
||||
assert report.addon_install_source == (
|
||||
AbuseReport.ADDON_INSTALL_SOURCES.OTHER)
|
||||
|
||||
|
||||
class TestAddonAbuseViewSetLoggedOut(AddonAbuseViewSetTestBase, TestCase):
|
||||
|
|
|
@ -0,0 +1 @@
|
|||
ALTER TABLE `abuse_reports` ADD COLUMN `addon_install_source` smallint UNSIGNED;
|
Загрузка…
Ссылка в новой задаче