540028 - Allow developers to upload the source code for binary/obfuscated code in the submission process

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect, P1)

Description

[Jorge Villalobos [:jorgev] (he/him)]

The binary/admin review process is very awkward at the moment. It'd be best if developers were able to upload a zipped version of their clear source files as part of the submission process.

Submissions of this type should be automatically flagged for admin review. The source code should be downloadable and viewable with the diff tool, but only for admin users.

Comment 1

[Wil Clouser [:clouserw]]

Does it ever expire or get removed?

By "diff tool" do you mean that we're diffing to the previous zip file they sent us?  If the file structure is different (different top level, etc.) that isn't going to work well.  Perhaps the file viewer would be a better option?

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

The idea is to provide an additional upload option for binary add-on developers to submit their zipped sources. Just like with the rest of the add-on's code, we should be able to view the file or diff the file against previous versions. Just like with extension source code, file structure can change in any way and we'll have to deal with that.

For now I don't think it's necessary to have an expiration system, but it would be good to have an admin tool that allows admins and the add-on's developer to delete these files if necessary.

I also think we need a new group on AMO to give access to this feature, given that only Andrew and I are allowed to look at these sources at the time.

Comment 3

[Wil Clouser [:clouserw]]

The conclusion is:

- We add a new section under "Notes for Reviewers" on https://addons.allizom.org/en-US/developers/addon/threadbubble/versions/34223

- There can be 1 "binary file archive" (probably a zip) per version of an add-on

- We'll store these files on disk somewhere (a new directory - let's use NETAPP_STORAGE/addons-source)

- simple CRUD for the UI - re-use as many of the other elements in the devhub as possible

- We can split the viewer and the differ part of this bug into separate bugs (I think those are 2 separate, additional bugs).  If we do that, this bug would just show a download link like the "file" section at the top of that page.

- Let's make a new group called Editors:BinarySource for this

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

Resetting a bunch of missed milestones. Sorry for the bugspam.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

Bumping up, since the current email system is becoming a bit unwieldy.

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

Some of the work on bug 958677 might be reusable for this.

Comment 8

[Yohan Boniface [:ybon]]

@jorge, some questions:

- as described in Comment 1, the source zip can be added only while editing the version, and not during the Addon submission process or the "Upload new version" process?

- in the edit version page, do we show uploading of the source only if "binary" has been set to True in the Version file?

@wil:

- should we take the opportunity to use a proper Django FileField (and then maybe land this only when staticfile PR has landed) ?

Comment 9

[Wil Clouser [:clouserw]]

Using FileField makes sense to me

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Yohan Boniface [:ybon] from comment #8)
> @jorge, some questions:
> 
> - as described in Comment 1, the source zip can be added only while editing
> the version, and not during the Addon submission process or the "Upload new
> version" process?

It should definitely be part of the new version upload process. However, it's something that could be added to the "Edit Version" page, which is where the developer ends up right after finishing the version upload.

> - in the edit version page, do we show uploading of the source only if
> "binary" has been set to True in the Version file?

No, it should be available for all add-ons.

Comment 11

[Yohan Boniface [:ybon]]

Just to be sure I understand correctly:

> It should definitely be part of the new version upload process.
> However, it's something that could be added to the "Edit Version" page,
> which is where the developer ends up right after finishing the version upload.

The ideal is that we add the new field to the "Upload a new version" form, but if for some reason we can't, adding it only to the "Edit Version" page is still ok. Is that right?

Now one more question: if we add the field to the "Upload a new version" form, should we also add it to the "Upload a new addon" form?

Comment 12

[Yohan Boniface [:ybon]]

Also: what kind of file are we supposed to accept? Only 'zip'? Any archive? Anything?

Thanks :)

Comment 13

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Yohan Boniface [:ybon] from comment #11)
> The ideal is that we add the new field to the "Upload a new version" form,
> but if for some reason we can't, adding it only to the "Edit Version" page
> is still ok. Is that right?

Yes.

> Now one more question: if we add the field to the "Upload a new version"
> form, should we also add it to the "Upload a new addon" form?

Yes.

(In reply to Yohan Boniface [:ybon] from comment #12)
> Also: what kind of file are we supposed to accept? Only 'zip'? Any archive?
> Anything?

Zip at the very least. If it's possible to accept other archive formats like 7z or tar.gz, then I think that's good, but not required.

Comment 14

[Yohan Boniface [:ybon]]

Thanks jorgev for those infos :)

@wil, @jorgev: we basically have two options I think now:

1. I focus on a first iteration with the upload available on the "Edit version" page (with perms, etc. working), and we create other tickets for enhancements (add to other forms, differ, viewer, etc.); this **should** be ready for the push of next week (freezing this week)

2. we have a higher MVP, and we prefer to push for example when all the forms are ready ("Submit a new addon", "Add a new version"), and thus I think it will be a bit hard to have it ready/reviewed for the push of next week (not sure about the UI/UX impacts on the addon submit process for example).

According to the previous comments on this thread, I think you will vote for option 1, but I prefer you to confirm your opinion.

Thanks! :)

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

I don't mind waiting an extra cycle if we can get more of the feature done by then. I actually prefer going with option 2.

Comment 16

[Yohan Boniface [:ybon]]

> Submissions of this type should be automatically flagged for admin review

Not sure to get that. Both "Submit a new addon" and "Add a new version" processes indeed already flag the addon for a review, so I can understand your point as "if a user change or add the source file of a version from the Edit Version page, we should reset the version for admin review". Is that correct, or is it something else?

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Yohan Boniface [:ybon] from comment #16)
> > Submissions of this type should be automatically flagged for admin review
> 
> Not sure to get that. Both "Submit a new addon" and "Add a new version"
> processes indeed already flag the addon for a review, so I can understand
> your point as "if a user change or add the source file of a version from the
> Edit Version page, we should reset the version for admin review". Is that
> correct, or is it something else?

It's something else. Add-ons have an additional flag that indicates the add-on should only be reviewed by an admin. It's stored in the adminreview column of the addons table, set to 1 when it's flagged.

Comment 18

[Yohan Boniface [:ybon]]

Thanks for that, it's clearer now :)

Some quick elements about adding source field to the various upload forms:

The main difficulty is that the upload is sort of asynchronous: when one hits the big blue button "Select a File", and once a file selected, an ajax is sent, which creates a FileUpload instance.
This instance contains the file that will then be used to create the Addon, Version and File.
Once the ajax returns, the upload input is populated by the javascript with the new FileUpload id. And then a "normal" form is sent when hitting "Continue".

Roughly, I see four options;

- make that the ajax also send the source file, which would need to be added as a new FileUpload property
- send the source file with the "normal" form, and then attach it to the FileUpload instance (which is then used to create the Addon, Version and File instances)
- send the source file with the "normal" form, and pass it as new parameter to the "from_upload" chain
- send the source file with the "normal" form, and attach it to the Version/File instance directly, once they have been created through the "from_upload" chain

At this point, I'm tempted to plug in the "from_upload" chain, without impacting the FileUpload model. But I will have a better look on it tomorrow!

Comment 19

[Yohan Boniface [:ybon]]

PR (WIP): https://github.com/mozilla/olympia/pull/139

@jorgev (not looking at the design details), can you check the principles and give some feedback (like the text label, do we want an expendable area, do we want the field somewhere else on the form, etc.):

http://imgur.com/a/YFDg3

Comment 20

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Yohan Boniface [:ybon] from comment #19)
> @jorgev (not looking at the design details), can you check the principles
> and give some feedback (like the text label, do we want an expendable area,
> do we want the field somewhere else on the form, etc.):
> 
> http://imgur.com/a/YFDg3

* In the edit page, the label should be "Delete" instead of "Clear". Maybe "Replace" instead of "Change".
* In the review page, the Download Source option should only be available to Admin Reviewers (group 50005). I would also use "Additional sources" so it's clearly distinguishable from the sources in the XPI.
* In the upload forms, the source code form should appear after the upload and validation for the XPI succeeds. Otherwise it might confuse developers.
* I would change the label "If your add-on contains binary or obfuscated code other than known libraries, upload its sources for review. Read more about the source code review policy." The 'Read more' text should link to https://addons.mozilla.org/developers/docs/policies/reviews#section-binary and open in a new tab.

Comment 21

[Yohan Boniface [:ybon]]

Thanks Jorge :)

> In the edit page, the label should be "Delete" instead of "Clear". Maybe "Replace" instead of "Change".

Those are default Django labels, I'm not sure it's a good idea to override them on the HTML side. Maybe this can be handle by the various English translations?

> In the review page, the Download Source option should only be available to Admin Reviewers (group 50005). I would also use "Additional sources" so it's clearly distinguishable from the sources in the XPI.

I'm relaying on the the "is_admin" flag already set by this view. This flag is set by checking for ('Addons', 'Edit') rule. I've had a look on prod, and this does point on the 50005 group, but also the 50000 and the 50061 groups. Does that work?

If not, should I change the rule checked for the is_admin flag or should I add another dedicated flag?

The "is_admin" flag is for example also used to show or not the "Clear Admin Review Flag" checkbox.

* In the upload forms, the source code form should appear after the upload and validation for the XPI succeeds. Otherwise it might confuse developers.

Done! :)

* I would change the label "If your add-on contains binary or obfuscated code other than known libraries, upload its sources for review. Read more about the source code review policy." The 'Read more' text should link to https://addons.mozilla.org/developers/docs/policies/reviews#section-binary and open in a new tab.

Done! :)

Comment 22

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Yohan Boniface [:ybon] from comment #21)
> Thanks Jorge :)
> 
> > In the edit page, the label should be "Delete" instead of "Clear". Maybe "Replace" instead of "Change".
> 
> Those are default Django labels, I'm not sure it's a good idea to override
> them on the HTML side. Maybe this can be handle by the various English
> translations?

It's fine.

> > In the review page, the Download Source option should only be available to Admin Reviewers (group 50005). I would also use "Additional sources" so it's clearly distinguishable from the sources in the XPI.
> 
> I'm relaying on the the "is_admin" flag already set by this view. This flag
> is set by checking for ('Addons', 'Edit') rule. I've had a look on prod, and
> this does point on the 50005 group, but also the 50000 and the 50061 groups.
> Does that work?

Yeah, that's good.

Comment 23

[Wil Clouser [:clouserw]]

Clearing my needinfo.  Jorge is on it.

Comment 24

[Yohan Boniface [:ybon]]

Fixed in https://github.com/yohanboniface/olympia/commit/15a841ba7137a11e07e3a1797a19347a2d2d5a3a

Comment 25

[Iulian Timis]

When the " View current" link is clicked, a 404 page is displayed.
Please see screencast http://screencast.com/t/MedervUdUK

Comment 26

[Iulian Timis]

Also, I am able to request Preliminary Review for an add-on that I have uploaded the source code, and also grant it, logged in as an admin, from the Editor Tools. According to this page https://addons.allizom.org/en-US/developers/docs/policies/reviews#section-binary it says that "These add-ons are ineligible for preliminary review and must choose the full review process."

Comment 27

[Andrew Williamson [:eviljeff]]

(In reply to Iulian Timis from comment #26)
> Also, I am able to request Preliminary Review for an add-on that I have
> uploaded the source code, and also grant it, logged in as an admin, from the
> Editor Tools. According to this page
> https://addons.allizom.org/en-US/developers/docs/policies/reviews#section-
> binary it says that "These add-ons are ineligible for preliminary review and
> must choose the full review process."

obfuscated addons are still eligible

Comment 28

[Yohan Boniface [:ybon]]

PR about the 404: https://github.com/mozilla/olympia/pull/212

Comment 29

[Yohan Boniface [:ybon]]

@Iulian: 404 is fixed in dev now, and will be in next push to stage.
About the type of review, as said by Andrew, the source file is also available for addon with obfuscated code, which are still eligible for preliminarily review, so I'm keeping as is. Please reopen if I've missed something :)

Comment 30

[Iulian Timis]

I have uploaded a .zip file, but when I click the "View current" link in order to download it, the extension for the file is missing. Is this the same issue?

Comment 31

[Yohan Boniface [:ybon]]

Good catch!

Fixed in https://github.com/yohanboniface/olympia/commit/1f4812ced626a6f5a22f72dcbeabcdc5d43a7a2d

Comment 32

[Iulian Timis]

I can still reproduce the issue described in Comment 30 in both AMO-dev and AMO-stage
Please see screencast http://screencast.com/t/Oz6yAIoo3PI

Comment 33

[Yohan Boniface [:ybon]]

@Julian, can you try again on stage?

For the record, issue seems to only appears on FF Windows (not Chrome on Windows, nor FF on Linux).

And this is the fix I've pushed: https://github.com/yohanboniface/olympia/commit/43137c5d8fbba83a2ff8c1367e0bff87509bd3b0

Comment 34

[Iulian Timis]

(In reply to Yohan Boniface [:ybon] from comment #33)
> @Julian, can you try again on stage?
> 
> For the record, issue seems to only appears on FF Windows (not Chrome on
> Windows, nor FF on Linux).
> 
> And this is the fix I've pushed:
> https://github.com/yohanboniface/olympia/commit/
> 43137c5d8fbba83a2ff8c1367e0bff87509bd3b0

Great, it's working now! http://screencast.com/t/feVgRoo6a8Q

Comment 35

[Iulian Timis]

Verified as fixed in FF 32 (Win 7).
Closing bug.