Last Comment Bug 795387 - Plugin blocks don't allow simultaneous plugin version and application version filters
: Plugin blocks don't allow simultaneous plugin version and application version...
Status: VERIFIED FIXED
:
Product: Toolkit
Classification: Components
Component: Blocklisting (show other bugs)
: unspecified
: All All
: -- critical (vote)
: 2012-10-25
Assigned To: Wraithan (Chris McDonald) [:wraithan]
:
Mentors:
Depends on:
Blocks: 799546 803152
  Show dependency treegraph
 
Reported: 2012-09-28 10:30 PDT by Jorge Villalobos [:jorgev]
Modified: 2016-03-07 15:30 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments

Description Jorge Villalobos [:jorgev] 2012-09-28 10:30:28 PDT
If we want to block a plugin and restrict if by plugin version, we use the Min and Max fields, like in:
https://addons.mozilla.org/admin/models/blocklist/blocklistdetail/138/

If we want to restrict by application version, we use the GUID, Min and Max fields, like in:
https://addons.mozilla.org/admin/models/blocklist/blocklistdetail/27/

Since they use the same fields, they can't be used simultaneously.

Extension blocks work differently. You add the blocklist detail, and then you create a separate blocklist app entry (https://addons.mozilla.org/admin/models/blocklist/blocklistapp/) that handles the application version range. You can link blocklist app entries to extension blocks, but not to plugin blocks.

There are a couple of possible solutions here. One is to expand the form in the plugin section, so that it has separate Min / Max entries for application and plugin version. The other one would be to allow blocklist app entries to be associated with plugin blocks.

This was brought up in bug 793273, and it limited our options for keeping CTP blocks enabled in Firefox 16.
Comment 1 Jorge Villalobos [:jorgev] 2012-10-04 14:37:08 PDT
This is limiting our possibilities when planning plugin blocks, so we need it fixed soon.

Wil, Wraithan, can you look into this?
Comment 2 Jorge Villalobos [:jorgev] 2012-10-05 10:17:18 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #0)
> The other one would be to allow blocklist
> app entries to be associated with plugin blocks.

To answer a question from IRC, I prefer this approach because it is consistent with the way we block extensions.
Comment 3 Wil Clouser [:clouserw] 2012-10-05 10:29:40 PDT
Wraithan volunteered to check this out.
Comment 4 Wraithan (Chris McDonald) [:wraithan] 2012-10-10 14:40:54 PDT
Have working code, need to write tests and a migration.
Comment 5 Wraithan (Chris McDonald) [:wraithan] 2012-10-11 09:37:54 PDT
Wont be done with enough time for code review/QA, pushing to next week.

WIP code:
https://github.com/wraithan/zamboni/tree/795387
Comment 6 Alex Keybl [:akeybl] 2012-10-16 09:54:53 PDT
(In reply to Wraithan from comment #5)
> Wont be done with enough time for code review/QA, pushing to next week.
> 
> WIP code:
> https://github.com/wraithan/zamboni/tree/795387

Still on track for this week? This is blocking critical CTP testing on FF17.
Comment 7 Wraithan (Chris McDonald) [:wraithan] 2012-10-18 10:24:30 PDT
This slipped in favor of getting PINs working for paid apps in the Firefox Marketplace. I should have said something earlier this week and got it handed off. I apologize for that.
Comment 8 Wraithan (Chris McDonald) [:wraithan] 2012-10-23 13:20:45 PDT
https://github.com/mozilla/zamboni/commit/5e99328f6a0210554f0f8d4ec703fbc4d291dc8c
Here's the patch.
Comment 9 Wil Clouser [:clouserw] 2012-10-23 13:39:51 PDT
thanks wraithan
Comment 10 Jorge Villalobos [:jorgev] 2012-10-23 14:36:28 PDT
Reopening because https://addons-dev.allizom.org/en-US/admin/models/blocklist/blocklistapp/ isn't accessible, and I am unable to add new app items.
Comment 11 Jorge Villalobos [:jorgev] 2012-10-23 15:08:44 PDT
A migration script wasn't run when pushing. This is fixed now.
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-23 15:12:52 PDT
Will wait until this is in production for marking status-firefox17 fixed.
Comment 13 Wraithan (Chris McDonald) [:wraithan] 2012-10-25 03:21:04 PDT
Steps for testing:

You'll be entering BlocklistPlugins and BlocklistApps in the admin here:

http://addons-dev.allizom.org/en-US/admin/models/blocklist/

And verifying it works by looking at the blocklist url:

http://addons-dev.allizom.org/en-US/blocklist/2/<guid>/<version>

guid - This will be a value you come up with.
version - this is a number you come up with.

Try adding a BlocklistPlugin with just a name and details (all that is required)

Make sure it shows up in the blocklist url.

Try adding a BlocklistApp to the BlocklistPlugin, make sure to set the guid to a value you came up with and the version range (min and max) so that the version number will be inside.

Make sure it shows up in your blocklist url.

Change the version numbers in the BlocklistApp that you made to exclude the version number you came up with.
Make sure it isn't in your blocklist url.

Change the version range on the BlocklistApp that you made back to including the version.
Make sure it is in your blocklist url.

Change the guid on the BlocklistApp to something different.
Make sure it isn't in your blocklist url.
Comment 14 Jorge Villalobos [:jorgev] 2012-10-25 08:42:07 PDT
These blocks are supposed to have both filters, but they only show up the application version filter:

<pluginItem  blockID="p163">
  <match name="filename" exp="(NPSWF32\.dll)|(Flash\ Player\.plugin)" />
  <versionRange  severity="0" vulnerabilitystatus="1">
    <targetApplication  id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}">
      <versionRange  minVersion="17.0" maxVersion="*" />
    </targetApplication>
  </versionRange>
</pluginItem>

<pluginItem  blockID="p165">
  <match name="filename" exp="(NPSWF[0-9_]*\.dll)|(Flash\ Player\.plugin)" />
  <versionRange  severity="0" vulnerabilitystatus="1">
    <targetApplication  id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}">
      <versionRange  minVersion="17.0" maxVersion="*" />
    </targetApplication>
  </versionRange>
</pluginItem>
Comment 15 Wraithan (Chris McDonald) [:wraithan] 2012-10-25 14:48:58 PDT
https://github.com/mozilla/zamboni/commit/d0089feb215791eed993a362d7d83e6bc5b31b9d

forgot to account for plugin.min and plugin.max properly
Comment 16 Jorge Villalobos [:jorgev] 2012-10-30 15:09:31 PDT
Verified in prod.

Note You need to log in before you can comment on or make changes to this bug.