Blocklist: allow mulitple apps per plugin (bug 795387)

apps/blocklist/admin.py

@@ -8,6 +8,10 @@ class PluginAdmin(admin.ModelAdmin):
     form = forms.BlocklistPluginForm
 
 
+class AppAdmin(admin.ModelAdmin):
+    form = forms.BlocklistAppForm
+
+
 ms = models.BlocklistItem, models.BlocklistPlugin, models.BlocklistGfx
 inlines = [type(cls.__name__ + 'Inline', (admin.StackedInline,),
                 {'model': cls})
@@ -18,7 +22,7 @@ class DetailAdmin(admin.ModelAdmin):
     inlines = inlines
 
 
-admin.site.register(models.BlocklistApp)
+admin.site.register(models.BlocklistApp, AppAdmin)
 admin.site.register(models.BlocklistCA)
 admin.site.register(models.BlocklistItem)
 admin.site.register(models.BlocklistPlugin, PluginAdmin)

apps/blocklist/forms.py

@@ -1,6 +1,6 @@
 from django import forms
 
-from .models import BlocklistPlugin
+from .models import BlocklistApp, BlocklistPlugin
 
 
 class BlocklistPluginForm(forms.ModelForm):
@@ -12,7 +12,22 @@ def clean(self):
         vulnerability = self.cleaned_data.get('vulnerability_status')
 
         if severity and vulnerability:
-            raise forms.ValidationError(
-                'Vulnerability status must be blank if Severity is non zero')
+            raise forms.ValidationError('Vulnerability status must be blank if'
+                                        'Severity is non zero')
+
+        return self.cleaned_data
+
+
+class BlocklistAppForm(forms.ModelForm):
+    class Meta:
+        model = BlocklistApp
+
+    def clean(self):
+        blthings = [self.cleaned_data.get('blitem'),
+                    self.cleaned_data.get('blplugin')]
+
+        if all(blthings) or not any(blthings):
+            raise forms.ValidationError('One and only one of BlocklistPlugin'
+                                        'and BlocklistItem must be set.')
 
         return self.cleaned_data

apps/blocklist/models.py

@@ -5,7 +5,10 @@
 
 
 class BlocklistApp(amo.models.ModelBase):
-    blitem = models.ForeignKey('BlocklistItem', related_name='app')
+    blitem = models.ForeignKey('BlocklistItem', related_name='app', blank=True,
+                               null=True)
+    blplugin = models.ForeignKey('BlocklistPlugin', related_name='app',
+                                 blank=True, null=True)
     guid = models.CharField(max_length=255, blank=True, db_index=True,
                             null=True)
     min = models.CharField(max_length=255, blank=True, null=True)
@@ -83,7 +86,8 @@ def flush_urls(self):
 class BlocklistPlugin(BlocklistBase, amo.models.ModelBase):
     _type = 'p'
     name = models.CharField(max_length=255, blank=True, null=True)
-    guid = models.CharField(max_length=255, blank=True, null=True)
+    guid = models.CharField(max_length=255, blank=True, db_index=True,
+                            null=True)
     min = models.CharField(max_length=255, blank=True, null=True)
     max = models.CharField(max_length=255, blank=True, null=True)
     os = models.CharField(max_length=255, blank=True, null=True)
@@ -92,17 +96,17 @@ class BlocklistPlugin(BlocklistBase, amo.models.ModelBase):
     filename = models.CharField(max_length=255, blank=True, null=True)
     severity = models.SmallIntegerField(blank=True, null=True)
     vulnerability_status = models.SmallIntegerField(blank=True, null=True,
-                                                    choices=
-                                                    ((1, 'update available'),
-                                                     (2, 'update unavailable')))
+                                                    choices=(
+                                                    (1, 'update available'),
+                                                    (2, 'update unavailable')))
     details = models.OneToOneField(BlocklistDetail, null=True)
 
     class Meta(amo.models.ModelBase.Meta):
         db_table = 'blplugins'
 
     def __unicode__(self):
         return '%s: %s - %s' % (self.name or self.guid or self.filename,
-                                    self.min, self.max)
+                                self.min, self.max)
 
     @property
     def get_vulnerability_status(self):
@@ -111,7 +115,7 @@ def get_vulnerability_status(self):
         Returns None when criteria aren't met so jinja2 excludes it from when
         using the attrs filter.
         """
-        if self.severity == 0 and self.vulnerability_status in (1,2):
+        if self.severity == 0 and self.vulnerability_status in (1, 2):
             return self.vulnerability_status
 
     def flush_urls(self):

apps/blocklist/templates/blocklist/blocklist.xml

@@ -30,13 +30,17 @@
       {% if plugin.name %}<match name="name" exp="{{ plugin.name }}" />{% endif %}
       {% if plugin.description %}<match name="description" exp="{{ plugin.description }}" />{% endif %}
       {% if plugin.filename %}<match name="filename" exp="{{ plugin.filename }}" />{% endif %}
-      {% if plugin.severity or plugin.min or plugin.max or plugin.get_vulnerability_status %}
-        {% if plugin.guid %}
+      {% if plugin.severity or plugin.apps or plugin.get_vulnerability_status %}
+        {% if plugin.app.exists() %}
         <versionRange {{ attrs(severity=plugin.severity, vulnerabilitystatus=plugin.get_vulnerability_status) }}>
-          {% if apiver > 2 and plugin.min and plugin.max %}
-            <targetApplication id="{{ plugin.guid }}">
-              <versionRange {{ attrs(minVersion=plugin.min, maxVersion=plugin.max) }} />
-            </targetApplication>
+          {% if apiver > 2 %}
+            {% for app in plugin.app.all() %}
+              <targetApplication {{ attrs(id=app.guid) }}>
+                {% if app.min and app.max %}
+                  <versionRange {{ attrs(minVersion=app.min, maxVersion=app.max) }} />
+                {% endif %}
+              </targetApplication>
+            {% endfor %}
           {% endif %}
         </versionRange>
         {% elif apiver > 2 and plugin.min and plugin.max %}

apps/blocklist/tests/test_forms.py

@@ -0,0 +1,33 @@
+from amo.tests import TestCase
+from blocklist import forms
+from blocklist.models import BlocklistItem, BlocklistPlugin
+
+
+class BlocklistFormTest(TestCase):
+
+    def setUp(self):
+        super(BlocklistFormTest, self).setUp()
+        self.blitem = BlocklistItem.objects.create()
+        self.blplugin = BlocklistPlugin.objects.create()
+
+    def test_app_form_only_blitem(self):
+        data = {'blitem': self.blitem.pk, 'blplugin': None}
+        form = forms.BlocklistAppForm(data)
+        assert form.is_valid()
+
+    def test_app_form_only_blplugin(self):
+        data = {'blplugin': self.blplugin.pk, 'blitem': None}
+        form = forms.BlocklistAppForm(data)
+        assert form.is_valid()
+
+    def test_app_form_neither_blplugin_and_blitem(self):
+        data = {'blitem': None, 'blplugin': None}
+        form = forms.BlocklistAppForm(data)
+        assert not form.is_valid()
+        assert 'One and only one' in str(form.errors)
+
+    def test_app_form_both_blplugin_and_blitem(self):
+        data = {'blitem': self.blitem.pk, 'blplugin': self.blplugin.pk}
+        form = forms.BlocklistAppForm(data)
+        assert not form.is_valid()
+        assert 'One and only one' in str(form.errors)

apps/blocklist/tests.py → apps/blocklist/tests/test_views.py

@@ -10,8 +10,8 @@
 import amo
 import amo.tests
 from amo.urlresolvers import reverse
-from .models import (BlocklistApp, BlocklistCA, BlocklistDetail,
-                     BlocklistItem, BlocklistGfx, BlocklistPlugin)
+from blocklist.models import (BlocklistApp, BlocklistCA, BlocklistDetail,
+                              BlocklistGfx, BlocklistItem, BlocklistPlugin)
 
 base_xml = """
 <?xml version="1.0"?>
@@ -20,16 +20,25 @@
 """
 
 
-class BlocklistTest(amo.tests.TestCase):
+class BlocklistViewTest(amo.tests.TestCase):
 
     def setUp(self):
-        super(BlocklistTest, self).setUp()
+        super(BlocklistViewTest, self).setUp()
         self.fx4_url = reverse('blocklist', args=[3, amo.FIREFOX.guid, '4.0'])
         self.fx2_url = reverse('blocklist', args=[2, amo.FIREFOX.guid, '2.0'])
+        self.tb4_url = reverse('blocklist', args=[3, amo.THUNDERBIRD.guid,
+                                                  '4.0'])
         self.mobile_url = reverse('blocklist', args=[2, amo.MOBILE.guid, '.9'])
         cache.clear()
         self.details = BlocklistDetail.objects.create()
 
+    def create_blplugin(self, app_guid=None, app_min=None, app_max=None,
+                        *args, **kw):
+        plugin = BlocklistPlugin.objects.create(*args, **kw)
+        app = BlocklistApp.objects.create(blplugin=plugin, guid=app_guid,
+                                          min=app_min, max=app_max)
+        return plugin, app
+
     def normalize(self, s):
         return '\n'.join(x.strip() for x in s.split())
 
@@ -41,7 +50,7 @@ def dom(self, url):
         return minidom.parseString(r.content)
 
 
-class BlocklistItemTest(BlocklistTest):
+class BlocklistItemTest(BlocklistViewTest):
 
     def setUp(self):
         super(BlocklistItemTest, self).setUp()
@@ -81,7 +90,7 @@ def find_lastupdate():
         self.item.save()
         eq(find_lastupdate(), self.item.modified)
 
-        plugin = BlocklistPlugin.objects.create(guid=amo.FIREFOX.guid)
+        plugin, app = self.create_blplugin(app_guid=amo.FIREFOX.guid)
         eq(find_lastupdate(), plugin.created)
         plugin.save()
         eq(find_lastupdate(), plugin.modified)
@@ -246,15 +255,15 @@ def test_item_target_empty_version_range(self):
         eq_(app[0].getElementsByTagName('versionRange'), [])
 
 
-class BlocklistPluginTest(BlocklistTest):
+class BlocklistPluginTest(BlocklistViewTest):
 
     def setUp(self):
         super(BlocklistPluginTest, self).setUp()
-        self.plugin = BlocklistPlugin.objects.create(guid=amo.FIREFOX.guid,
+        self.plugin, self.app = self.create_blplugin(app_guid=amo.FIREFOX.guid,
                                                      details=self.details)
 
     def test_no_plugins(self):
-        dom = BlocklistTest.dom(self, self.mobile_url)
+        dom = BlocklistViewTest.dom(self, self.mobile_url)
         children = dom.getElementsByTagName('blocklist')[0].childNodes
         # There are only text nodes.
         assert all(e.nodeType == 3 for e in children)
@@ -316,7 +325,8 @@ def test_plugin_severity_zero(self):
         eq_(v.getAttribute('severity'), '0')
 
     def test_plugin_no_target_app(self):
-        self.plugin.update(guid=None, severity=1, min='1', max='2')
+        self.plugin.update(severity=1, min='1', max='2')
+        self.app.delete()
         vr = self.dom().getElementsByTagName('versionRange')[0]
         eq_(vr.getElementsByTagName('targetApplication'), [],
             'There should not be a <targetApplication> if there was no app')
@@ -325,7 +335,26 @@ def test_plugin_no_target_app(self):
         eq_(vr.getAttribute('maxVersion'), '2')
 
     def test_plugin_with_target_app(self):
-        self.plugin.update(guid=amo.FIREFOX.guid, severity=1, min='1', max='2')
+        self.plugin.update(severity=1)
+        self.app.update(guid=amo.FIREFOX.guid, min='1', max='2')
+        vr = self.dom().getElementsByTagName('versionRange')[0]
+        eq_(vr.getAttribute('severity'), '1')
+        assert not vr.getAttribute('vulnerabilitystatus')
+
+        app = vr.getElementsByTagName('targetApplication')[0]
+        eq_(app.getAttribute('id'), amo.FIREFOX.guid)
+
+        vr = app.getElementsByTagName('versionRange')[0]
+        eq_(vr.getAttribute('minVersion'), '1')
+        eq_(vr.getAttribute('maxVersion'), '2')
+
+
+    def test_plugin_with_multiple_target_apps(self):
+        self.plugin.update(severity=1)
+        self.app.update(guid=amo.FIREFOX.guid, min='1', max='2')
+        tb_app = BlocklistApp.objects.create(guid=amo.THUNDERBIRD.guid,
+                                             min='3', max='4',
+                                             blplugin=self.plugin)
         vr = self.dom().getElementsByTagName('versionRange')[0]
         eq_(vr.getAttribute('severity'), '1')
         assert not vr.getAttribute('vulnerabilitystatus')
@@ -337,9 +366,20 @@ def test_plugin_with_target_app(self):
         eq_(vr.getAttribute('minVersion'), '1')
         eq_(vr.getAttribute('maxVersion'), '2')
 
+        vr = self.dom(self.tb4_url).getElementsByTagName('versionRange')[0]
+        eq_(vr.getAttribute('severity'), '1')
+        assert not vr.getAttribute('vulnerabilitystatus')
+
+        app = vr.getElementsByTagName('targetApplication')[0]
+        eq_(app.getAttribute('id'), amo.FIREFOX.guid)
+
+        vr = app.getElementsByTagName('versionRange')[0]
+        eq_(vr.getAttribute('minVersion'), '1')
+        eq_(vr.getAttribute('maxVersion'), '2')
+
     def test_plugin_with_target_app_with_vulnerability(self):
-        self.plugin.update(guid=amo.FIREFOX.guid, severity=0, min='1', max='2',
-                           vulnerability_status=2)
+        self.plugin.update(severity=0, vulnerability_status=2)
+        self.app.update(guid=amo.FIREFOX.guid, min='1', max='2')
         vr = self.dom().getElementsByTagName('versionRange')[0]
         eq_(vr.getAttribute('severity'), '0')
         eq_(vr.getAttribute('vulnerabilitystatus'), '2')
@@ -352,7 +392,8 @@ def test_plugin_with_target_app_with_vulnerability(self):
         eq_(vr.getAttribute('maxVersion'), '2')
 
     def test_plugin_with_severity_only(self):
-        self.plugin.update(guid=None, severity=1)
+        self.plugin.update(severity=1)
+        self.app.delete()
         vr = self.dom().getElementsByTagName('versionRange')[0]
         eq_(vr.getAttribute('severity'), '1')
         assert not vr.getAttribute('vulnerabilitystatus')
@@ -363,16 +404,18 @@ def test_plugin_with_severity_only(self):
             'There should not be a <targetApplication> if there was no app')
 
     def test_plugin_without_severity_and_with_vulnerability(self):
-        self.plugin.update(guid=None, severity=0, vulnerability_status=1)
+        self.plugin.update(severity=0, vulnerability_status=1)
+        self.app.delete()
         vr = self.dom().getElementsByTagName('versionRange')[0]
         eq_(vr.getAttribute('severity'), '0')
         eq_(vr.getAttribute('vulnerabilitystatus'), '1')
         eq_(vr.getAttribute('minVersion'), '')
         eq_(vr.getAttribute('maxVersion'), '')
 
     def test_plugin_without_severity_and_with_vulnerability_and_minmax(self):
-        self.plugin.update(guid=None, severity=0, vulnerability_status=1,
-                           min='2.0', max='3.0')
+        self.plugin.update(severity=0, vulnerability_status=1, min='2.0',
+                           max='3.0')
+        self.app.delete()
         vr = self.dom().getElementsByTagName('versionRange')[0]
         eq_(vr.getAttribute('severity'), '0')
         eq_(vr.getAttribute('vulnerabilitystatus'), '1')
@@ -387,17 +430,17 @@ def test_plugin_apiver_lt_3(self):
         eq_(e.getElementsByTagName('targetApplication'), [])
 
         # The app version is not in range.
-        self.plugin.update(min='3.0', max='4.0')
+        self.app.update(min='3.0', max='4.0')
         self.assertRaises(IndexError, self.dom, self.fx2_url)
 
         # The app is back in range.
-        self.plugin.update(min='1.1')
+        self.app.update(min='1.1')
         e = self.dom(self.fx2_url).getElementsByTagName('versionRange')[0]
         eq_(e.getAttribute('severity'), '2')
         eq_(e.getElementsByTagName('targetApplication'), [])
 
 
-class BlocklistGfxTest(BlocklistTest):
+class BlocklistGfxTest(BlocklistViewTest):
 
     def setUp(self):
         super(BlocklistGfxTest, self).setUp()
@@ -454,7 +497,7 @@ def test_block_id(self):
         eq_(item.getAttribute('blockID'), 'g' + str(self.details.id))
 
 
-class BlocklistCATest(BlocklistTest):
+class BlocklistCATest(BlocklistViewTest):
 
     def setUp(self):
         super(BlocklistCATest, self).setUp()

apps/blocklist/views.py

@@ -95,7 +95,7 @@ def get_items(apiver, app, appver=None):
             rs = list(rs)
             rr.append(rs[0])
             rs[0].apps = [App(r.app_guid, r.app_min, r.app_max)
-                           for r in rs if r.app_guid]
+                          for r in rs if r.app_guid]
         os = [r.os for r in rr if r.os]
         items[guid] = BlItem(rr, os[0] if os else None, rows[0].modified,
                              rows[0].block_id)
@@ -107,14 +107,19 @@ def get_plugins(apiver, app, appver=None):
     # API versions < 3 ignore targetApplication entries for plugins so only
     # block the plugin if the appver is within the block range.
     plugins = (BlocklistPlugin.uncached.select_related('details')
-               .filter(Q(guid__isnull=True) | Q(guid=app)))
+               .filter(Q(app__isnull=True) | Q(app__guid=app) |
+                       Q(app__guid__isnull=True))
+               .extra(select={'app_guid': 'blapps.guid',
+                              'app_min': 'blapps.min',
+                              'app_max': 'blapps.max'}))
     if apiver < 3 and appver is not None:
         def between(ver, min, max):
             if not (min and max):
                 return True
             return version_int(min) < ver < version_int(max)
         app_version = version_int(appver)
-        plugins = [p for p in plugins if between(app_version, p.min, p.max)]
+        plugins = [p for p in plugins if between(app_version, p.app_min,
+                                                 p.app_max)]
     return list(plugins)
 
 

migrations/493-add-blplugin-to-blapp.sql

@@ -0,0 +1,5 @@
+ALTER TABLE `blapps` ADD COLUMN `blplugin_id` int(11) unsigned DEFAULT NULL;
+
+ALTER TABLE `blapps` ADD CONSTRAINT `blplugin_id_apps` FOREIGN KEY (`blplugin_id`) REFERENCES `blplugins` (`id`);
+
+ALTER TABLE `blapps` MODIFY `blitem_id` int(11) unsigned DEFAULT NULL;