Closed
Bug 540028
Opened 15 years ago
Closed 10 years ago
Allow developers to upload the source code for binary/obfuscated code in the submission process
Categories
(addons.mozilla.org Graveyard :: Developer Pages, defect, P1)
addons.mozilla.org Graveyard
Developer Pages
Tracking
(Not tracked)
VERIFIED
FIXED
2014-08
People
(Reporter: jorgev, Assigned: yboniface)
References
Details
(Whiteboard: [ReviewTeam:P1])
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•13 years ago
|
||
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?
Reporter | ||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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
Assignee: nobody → mbasta
Target Milestone: 4.x (triaged) → 6.1.1
Updated•13 years ago
|
Priority: P4 → P3
Target Milestone: 6.1.1 → 6.1.2
Updated•13 years ago
|
Target Milestone: 6.1.2 → 6.1.3
Updated•13 years ago
|
Target Milestone: 6.1.3 → 6.1.4
Updated•13 years ago
|
Target Milestone: 6.1.4 → 6.1.5
Updated•13 years ago
|
Target Milestone: 6.1.5 → 6.1.6
Updated•13 years ago
|
Target Milestone: 6.1.6 → 6.1.7
Updated•13 years ago
|
Target Milestone: 6.1.7 → Q3 2011
Reporter | ||
Updated•13 years ago
|
Assignee: mattbasta → nobody
Priority: P3 → P2
Whiteboard: [z] [required amo-editors] → [required amo-editors]
Target Milestone: Q3 2011 → Q1 2012
Reporter | ||
Comment 4•12 years ago
|
||
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam:P2]
Reporter | ||
Comment 5•12 years ago
|
||
Resetting a bunch of missed milestones. Sorry for the bugspam.
Target Milestone: Q1 2012 → ---
Updated•10 years ago
|
Target Milestone: --- → 2014-04
Updated•10 years ago
|
Target Milestone: 2014-04 → 2014-06
Reporter | ||
Comment 6•10 years ago
|
||
Bumping up, since the current email system is becoming a bit unwieldy.
Priority: P2 → P1
Whiteboard: [ReviewTeam:P2] → [ReviewTeam:P1]
Reporter | ||
Comment 7•10 years ago
|
||
Some of the work on bug 958677 might be reusable for this.
Updated•10 years ago
|
Target Milestone: 2014-06 → 2014-07
Assignee | ||
Comment 8•10 years ago
|
||
@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) ?
Assignee: nobody → yboniface
Flags: needinfo?(jorge)
Flags: needinfo?(clouserw)
Reporter | ||
Comment 10•10 years ago
|
||
(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.
Flags: needinfo?(jorge)
Assignee | ||
Comment 11•10 years ago
|
||
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?
Flags: needinfo?(jorge)
Assignee | ||
Comment 12•10 years ago
|
||
Also: what kind of file are we supposed to accept? Only 'zip'? Any archive? Anything? Thanks :)
Reporter | ||
Comment 13•10 years ago
|
||
(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.
Flags: needinfo?(jorge)
Assignee | ||
Comment 14•10 years ago
|
||
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! :)
Flags: needinfo?(jorge)
Flags: needinfo?(clouserw)
Reporter | ||
Comment 15•10 years ago
|
||
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.
Flags: needinfo?(jorge)
Assignee | ||
Comment 16•10 years ago
|
||
> 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?
Flags: needinfo?(jorge)
Reporter | ||
Comment 17•10 years ago
|
||
(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.
Flags: needinfo?(jorge)
Assignee | ||
Comment 18•10 years ago
|
||
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!
Assignee | ||
Comment 19•10 years ago
|
||
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
Flags: needinfo?(jorge)
Reporter | ||
Comment 20•10 years ago
|
||
(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.
Flags: needinfo?(jorge)
Assignee | ||
Comment 21•10 years ago
|
||
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! :)
Flags: needinfo?(jorge)
Reporter | ||
Comment 22•10 years ago
|
||
(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.
Flags: needinfo?(jorge)
Updated•10 years ago
|
Target Milestone: 2014-07 → 2014-08
Assignee | ||
Comment 24•10 years ago
|
||
Fixed in https://github.com/yohanboniface/olympia/commit/15a841ba7137a11e07e3a1797a19347a2d2d5a3a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 25•10 years ago
|
||
When the " View current" link is clicked, a 404 page is displayed. Please see screencast http://screencast.com/t/MedervUdUK
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 26•10 years ago
|
||
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•10 years ago
|
||
(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
Assignee | ||
Comment 28•10 years ago
|
||
PR about the 404: https://github.com/mozilla/olympia/pull/212
Assignee | ||
Comment 29•10 years ago
|
||
@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 :)
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 30•10 years ago
|
||
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?
Flags: needinfo?(yboniface)
Assignee | ||
Comment 31•10 years ago
|
||
Good catch! Fixed in https://github.com/yohanboniface/olympia/commit/1f4812ced626a6f5a22f72dcbeabcdc5d43a7a2d
Flags: needinfo?(yboniface)
Comment 32•10 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 33•10 years ago
|
||
@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•10 years ago
|
||
(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
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 35•10 years ago
|
||
Verified as fixed in FF 32 (Win 7). Closing bug.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•