1229197 - Allow unlisted add-ons to be signed without passing the validator

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Add-on Validation, defect, P1)

Description

[Andy McKay]

If an add-on is:
* unlisted
* NOT side-loaded
* NOT a web extension
* and the validation fails without a critical error [1]

Then do not prevent the add-on being submitted and signed, let it continue.

Hoping we can get this through for our Thursday 3rd push. Making confidential for the moment, until Kev does the communication.

[1] The most critical errors are things like: 
- version already exists
- validation timeout
- version doesn't match
- could not parse manifest
- could not find add-on ID
- add-on id doesn't match add-on
- duplicate add-on id
- Version numbers should have fewer than 32 characters
- Version numbers should only contain letters, numbers, and these punctuation characters: +*.-_. - <em:type> doesn't match add-on
or put another way "I think it should be everything in tier 1 and the errors from files.utils.parse_addon which seem to end up on the Form objects".

Comment 1

[Andy McKay]

We can drop the "NOT side-loaded" requirement. As per feedback from Jorgev. Side-loaded add-ons still need to be manually enabled by the user in the add-ons manager.

So side-loaded unlisted addons should also be signed by the validator if they do not fail critically.

Comment 2

[Mathieu Agopian [:magopian]]

PR: https://github.com/mozilla/olympia/pull/1005

Comment 3

[Mathieu Agopian [:magopian]]

This will need a whole lot of QA: making sure there's no issues
1/ submitting new add-ons listed, unlisted, unlisted and sideload, with or without validation errors
2/ submitting new versions for add-ons
3/ submitting new files for versions
4/ submitting files to the standalone validator

Comment 4

[Dan Stillman]

Thanks, everyone. Where does this leave extensions that currently time out the validator, though? If the other critical errors are prioritized in the tests, could the output leading up to the timeout be sufficient?

Comment 5

[Andy McKay]

(In reply to Dan Stillman from comment #4)
> Thanks, everyone. Where does this leave extensions that currently time out
> the validator, though? If the other critical errors are prioritized in the
> tests, could the output leading up to the timeout be sufficient?

That's a way to go, but would be change outside of the scope of this change. At this point it would mean some critical tests have failed, but perhaps we can seperate that out. 

For the record we've upped the timeout on the validator in bug 1229060 and I tested zotero along with other add-ons to see if I could get a timeout and so have not triggered one with the new timeout.

Comment 6

[Andy McKay]

(In reply to Andy McKay [:andym] from comment #5)
> (In reply to Dan Stillman from comment #4)
> > Thanks, everyone. Where does this leave extensions that currently time out
> > the validator, though? If the other critical errors are prioritized in the
> > tests, could the output leading up to the timeout be sufficient?
> 
> That's a way to go, but would be change outside of the scope of this change.
> At this point it would mean some critical tests have failed, but perhaps we
> can seperate that out. 
> 
> For the record we've upped the timeout on the validator in bug 1229060 and I
> tested zotero along with other add-ons to see if I could get a timeout and
> so have not triggered one with the new timeout.

Sorry. Typo there "So far have not triggered a timeout one those add-ons with the new timeout setting."

Comment 7

[Mark Striemer [:mstriemer]]

I went through the errors that we register and everything that we call an error seems to be something that we'd want to block signing. So tier doesn't matter after all.

As Andy mentioned timeouts would fail, we register them as errors. It could be that we don't register any errors after a certain tier (it looks like 1 and 2 definitely have errors) but we don't know what tier we failed on and that gets interesting for inner packages since they handle tiers differently. I don't think we know enough about timeouts and I'm not sure we could know enough.

I've got a PR [1] to handle this in the signing API. Should merge it and get it into the tag tomorrow.

[1] https://github.com/mozilla/olympia/pull/1014

Comment 8

[krupa raj[:krupa]]

problem contacting the server: https://www.dropbox.com/s/lgiywdhvpahizuk/Screenshot%202015-12-01%2019.39.29.png?dl=0

version already exist: https://www.dropbox.com/s/nw6ffbr55x0mo29/Screenshot%202015-12-01%2019.53.23.png?dl=0

<em:id> is invalid
https://www.dropbox.com/s/44bpj2liidk10a2/Screenshot%202015-12-02%2014.34.54.png?dl=0

<em:version> too long
https://www.dropbox.com/s/44bpj2liidk10a2/Screenshot%202015-12-02%2014.34.54.png?dl=0

<em:version> is invalid
https://www.dropbox.com/s/4upfkhr77yhnwjo/Screenshot%202015-12-02%2014.48.43.png?dl=0

could not find add-on id
https://www.dropbox.com/s/x8rm5pfpnicz8dy/Screenshot%202015-12-02%2014.57.25.png?dl=0

install.rdf missing elements
https://www.dropbox.com/s/9eitzwu06pyb7d2/Screenshot%202015-12-02%2015.26.33.png?dl=0

could not parse uploaded file: https://www.dropbox.com/s/xaxurmaljzjt7qv/Screenshot%202015-12-01%2012.50.28.png?dl=0


We will continue testing