778365 - update blocklist web form to handle click-to-play plugins

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

We can now make plugins click-to-play using the blocklist. However, apparently the web form to do so has not been updated. A plugin is marked as click-to-play if a corresponding entry in the blocklist has 'severity="0"' and 'vulnerabilitystatus="1"' or 'vulnerabilitystatus="2"' (1 is for when we know of an update to the plugin, and 2 is when there is no update available).
So, if whatever tool generates the blocklist.xml files could be updated to handle this, that'd be great.

Comment 1

[Lucas Adamski [:ladamski]]

This is a blocker for deploying click to play.

Comment 2

[Wil Clouser [:clouserw]]

This bug is adding a field to the admin scaffolding called "vulnerabilitystatus" and then showing that in the XML.

Details:

- This only affects the plugins db table, scaffolding, and XML (NOT items, gfx, etc.)
- This field can contain a 1 or 2 or nothing
- If (and only if) this field has a 1 or 2 in it, it should show up in the XML on the <versionRange> element.  If it doesn't have a value it shouldn't be in the XML.
- default to blank

Comment 3

[Wraithan (Chris McDonald) [:wraithan]]

A few questions:

* Is set to None the same as set to 0 for severity?

* In the cases where there is a GUID, there is a versionRange nested in a targetApplication that is nested in a versionRange. The external most versionRange has severity on it, the internal most has min and max version, which one do you want the vulnerabilitystatus on? ( https://github.com/mozilla/zamboni/blob/master/apps/blocklist/templates/blocklist/blocklist.xml#L34 ) 

* Other multiple word attributes and elements are camel cased, should this be as well? (the quoted strings mentioned do not camel case it but that breaks style for the XML)

* The only place I can find that we can edit blocklist is the django admin. Is this correct or am I missing something?

Comment 4

[Wil Clouser [:clouserw]]

(In reply to Wraithan from comment #3)
> * The only place I can find that we can edit blocklist is the django admin.
> Is this correct or am I missing something?

That's correct.  I'll let David answer the others.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Wraithan from comment #3)
> A few questions:
> 
> * Is set to None the same as set to 0 for severity?

I'm not sure what you're asking. If severity is 0 and vulnerabilitystatus is anything but 1 or 2, currently the browser will interpret that plugin as outdated. If severity is anything other than 0, vulnerabilitystatus has no meaning and is not interpreted by the browser. Does that help?

> * In the cases where there is a GUID, there is a versionRange nested in a
> targetApplication that is nested in a versionRange. The external most
> versionRange has severity on it, the internal most has min and max version,
> which one do you want the vulnerabilitystatus on? (
> https://github.com/mozilla/zamboni/blob/master/apps/blocklist/templates/
> blocklist/blocklist.xml#L34 )

vulnerabilitystatus should be with severity, but I bet there's a bug somewhere in there. This would be nice to test (as in, if we have a guid, does ctp blocklisting still work in either case?)
 
> * Other multiple word attributes and elements are camel cased, should this
> be as well? (the quoted strings mentioned do not camel case it but that
> breaks style for the XML)

I may have misunderstood the last comment in this review: https://bugzilla.mozilla.org/show_bug.cgi?id=760625#c16 but as it is implemented, vulnerabilitystatus is expected to be all lowercase in the xml file.

Comment 6

[Wraithan (Chris McDonald) [:wraithan]]

1) Got it.

2) Got it, I'll mention the testing part to krupa when this hits QA

3) Thanks for the link to that ticket, unfortunately that person who said the attribute should not be camelcased did not look at that rest of the XML, so there will be maxVersion, minVersion, blockID, and then vulnerabilitystatus :( I'll implement it lowercase to match that patch.

4) Got it.

Thanks clouserw and David.

Comment 7

[Wraithan (Chris McDonald) [:wraithan]]

https://github.com/mozilla/zamboni/commit/533a80ff5ae139b72c6b53e9e3997fd9e8e1d663

Added a form to admin that validates that the vulnerabilitystatus matches the rules set forth here. 1 == 'update available' 2 == 'update unavailable'

Comment 8

[krupa raj[:krupa]]

Can this wait until next week to go live?

Comment 9

[Wraithan (Chris McDonald) [:wraithan]]

Adding bug 787222 as a dependency as it is changing the templates (and tests) and is a bit more urgent than this so I need to get that solid. 

This was reverted and isn't on master anymore. I will revert the revert and bump migration numbers and such then post the new commit in here.

Comment 10

[Wil Clouser [:clouserw]]

(In reply to krupa raj 82[:krupa] from comment #8)
> Can this wait until next week to go live?

I don't have a clear urgency on this bug.  David - when do you need this?  I'd rather not break the blocklist before a long weekend.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Wil Clouser [:clouserw] from comment #10)
> (In reply to krupa raj 82[:krupa] from comment #8)
> > Can this wait until next week to go live?
> 
> I don't have a clear urgency on this bug.  David - when do you need this? 
> I'd rather not break the blocklist before a long weekend.

Right - sorry. I spoke with krupa on irc but that conversation didn't make it here. We don't need this immediately, but we do need to be able to test this before 16 ships. So, we don't need this today, but sometime in the next week or two would be best.

Comment 12

[Wraithan (Chris McDonald) [:wraithan]]

Putting on next week's milestone, downgrading from critical. I'll have this back on master after all the criticals are done for today. I'll ping krupa to make sure this gets QA'd for next week's push.

Comment 13

[Wraithan (Chris McDonald) [:wraithan]]

https://github.com/mozilla/zamboni/commit/890505f

didn't have to bump the migration number as it is still the newest migration. Has already been reviewed. QA time.

Comment 14

[krupa raj[:krupa]]

Attachment: screenshot for reopening

STR:
1. Load https://addons-dev.allizom.org/en-US/admin/models/blocklist/blocklistplugin/7/
2. Notice that severity=0 and vulnerabilitystatus= update available
3. Load https://addons-dev.allizom.org/blocklist/3/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/15.0/Firefox/20110217/Darwin/en-US/release/Darwin/default/default and compare the XML for that plugin block


observed behavior:
vulnerabilitystatus is missing. I might very well be doing something wrong. But reopening for investigation.

Comment 15

[Asa Dotzler [:asa]]

Wraithan, can you take a look at this ASAP? This is a critical piece of this much needed feature and we're trying to have it all wrapped up in the next couple of weeks.

Comment 16

[Wraithan (Chris McDonald) [:wraithan]]

Last time I spoke to Krupa this was working, she was going to do another test and get back to me. Krupa, could you toss an update into here?

Comment 17

[krupa raj[:krupa]]

This works fine. Moving to resolved

Comment 18

[krupa raj[:krupa]]

I verified the following scenarios:

1) vulnerabilitystatus shows up in the blocklist XML: http://cl.ly/image/0U2n3Y401i2Z
2) vulnerabilitystatus with GUID set http://cl.ly/image/3A2r2Y1L452UCheck severity=0 and vulnerabilitystatus=1 http://cl.ly/image/0U2n3Y401i2Z
3) severity=0 and vulnerabilitystatus=2 http://cl.ly/image/0I0i2Z2A452X
4) severity=0 and vulnerabilitystatus=-- (gets ignored)
5) vulnerabilitystatus is ignored if severity is not set to "0" http://cl.ly/image/2W2H0m3l3u2P
6) Check that the value is in lowercase http://cl.ly/image/0U2n3Y401i2Z

Note that i have not checked the client side implementation of this. I have made sure that things show up in the XML.