Add support for addon submission api

[eviljeff]

fixes #2451

Comments on this patch: It's large, but I couldn't see a way to split it into pieces. I attempted to add tests to cover the functionality in each function but I probably missed a lot of the detail. There are also two big things I realised was missing when I was writing the tests that will need to be addressed - I guess in follow-ups because this is already a lot for a single pr - before it's useful for end-users:

• The existing signing API has the ability to specify a guid for an add-on in the PUT url and it will be used to create the new add-on with that GUID. The submission API requires that if a guid is specified in the url it is in the manifest. So web-ext will need to patch the manifest in the xpi that is uploaded beforehand. The code in sign.js also saves and then uses the guid from the first submission to be able to submit subsequent versions under the same guid - the same limitation would apply here - it must be in the manifest. (The normal way of submitting extra versions to the same add-on, that wouldn't require the guid be specified in the manifest, is submitting to a different endpoint, POSTing to /addons/addon/<addon-id>/versions/, but a) web-ext doesn't have a way of the user specifying "this is a new add-on" vs. "this is a new version" and b) the patch I'm submitting doesn't support that endpoint currently.)
• There's no support for specifying additional metadata via the command line, nor tests for it being passed down to the relevant functions. As you need to provide extra metadata to create the first listed version on an add-on (or a new add-on with a listed version) - at least categories and version.license - this means anyone trying to use it for listed versions would be unable to.

[rpl]

@eviljeff thanks for creating a draft PR for this set of changes! The following are just a couple of comments from an initial quick feedback pass, I haven't looked yet the new module and changes to the sign command in detail, we (likely @willdurand or I) will do a proper feedback / review pass asap.

[eviljeff]

I think it's ready for review(!)

Two of the tests I added I just couldn't get working - I marked them .skip -ideas welcome! Also now CI is failing in two of the jobs (but not all of them) with fsevents not accessible from chokidar 🤷

[rpl]

I think it's ready for review(!)

Two of the tests I added I just couldn't get working - I marked them .skip -ideas welcome! Also now CI is failing in two of the jobs (but not all of them) with fsevents not accessible from chokidar shrug

I just tried to pinpoint how the package-lock.json got to trigger that unexpected error, I believe that may have been due to a previous version of this branch having npm installed as a dependency.

That may have left some traces into your package-lock.json and it ended up into this odd inconsistent state.

I'm not sure how technically those have been left there after you removed the dependency, I'd need to know the exact sequence of steps that you did locally when you removed the npm dependency.

Anyway, in the past I had my share of issues with the package-lock.json entering odd corner cases, from what I recall at the time I got the impression that a previous package-lock.json file or node_modules dir may affect the changes applied to the package-lock.json file from the exact same pacakge.json file.

And so, to be 100% sure I'm always recomputing the package-lock.json in a reproducible way, when I rebuilt the lock file locally I usually:

1. remove the package-lock.json and node_modules dir first
2. and then I run npm install to recreate it (or npx npm@SOMEVERSION install if I need to be sure a certain npm version is used to recompute the lock file)

I tried exactly that locally and that was enough to make npm ci to work as expected on npm v7 (which was the version I was using locally with node v14 by default, and it was able to trigger the exact same issue as the failed CI jobs).

I also merged some PR updating the package.json and package-lock.json file today, and so I also tried that again after rebasing this branch on today's master branch and the conflict on the package-lock.json file was also the only conflict to resolve in the rebase and so I added a copy of the fixed package-lock.json file I got locally in this gist (the gist is long and so you may not notice the second file, but you can clone the gist locally and get the one you want to try in this branch, with the one I created after rebasing you could also rebase the branch which may also be good):

• https://gist.github.com/rpl/7ed9480e5f21bcf7b29cd8d2e8c418c6

@eviljeff would you mind to give that a try and update this PR so that we can confirm that the CI job failures are going to be fixed with the package-lock.json files recreated with the approach described above?

[rpl]

@eviljeff I haven't looked the entire PR yet (in particular I have only took an initial look the new submit-addon.js module, but I plan to give it another more detailed review pass on it and I'd like that module to also be reviewed by @willdurand, because he may be able to spot some other details I may be missing, and I haven't looked yet to the test cases at all).

But in the meantime I wanted to send you at least the comments and questions that I collected so far.

Feel free to ping me for any of the points in this review pass that you'd like to discuss more about.

[rpl]

I noticed right before submitting this round of comments that the answer to the question below may actually be provided in the pull request description (where i mention that The submission API requires that if a guid is specified in the url it is in the manifest).

We may end up to agree to file a followup and take care of that separately from this PR, but in the meantime I'll keep this comment as a reminder to ourselves that we need to discuss about this and agreed on how we want to proceed.

---

Question: looking to Bug 1755872 - Make add-on ID mandatory in the manifest for MV3 and the related addons-linter issue mozilla/addons-linter#4303 and pull request mozilla/addons-linter#4315 it looks that we are requiring the mandatory addon id explicitly specified in the manifest only for manifest_version 3 extensions, but based on these checks it seems that the new Addon Submission API is requiring that indipendently from the manifest_version, is that a requirement for the new Addon Submission API?

[eviljeff]

Yes, it's an additional requirement we added, but only for the PUT requests, because the GUID is specified in the url. We were thinking of the mv3 requirements though - and how if we were going to add this endpoint then may as well align with the future.

[rpl]

I took a look to the windows CI job failure, as I mentioned yesterday I was pretty sure to have seen that and fixed it not long ago, and confirmed that it has been already fixed in #2444 along with the dependency update that started to trigger it, in particular it has been fixed by this change here:

web-ext/tests/functional/common.js

Lines 101 to 119 in 5e3b214

	export function execWebExt(
	argv: Array<string>,
	spawnOptions: child_process$spawnOpts,
	): RunningWebExt {
	if (spawnOptions.env) {
	spawnOptions.env = {
	// Propagate the current environment when redefining it from the `spawnOptions`
	// otherwise it may trigger unexpected failures due to missing variables that
	// may be expected (e.g. #2444 was failing only on Windows because
	// @pnpm/npm-conf, a transitive dependencies for update-notifier, was expecting
	// process.env.APPDATA to be defined when running on Windows).
	...process.env,
	...spawnOptions.env,
	};
	}
	const spawnedProcess = spawn(
	process.execPath, [webExt, ...argv], spawnOptions
	);

The reason why the fixed failure came back seems to be due to the fact that the package.json file may have been updated manually to match how it is in 7.1.1 (after #2444 was merged), but the fix in tests/functional/common.js isn't there because the branch has not be rebased.

e.g. I can see it being missing here:

web-ext/tests/functional/common.js

Lines 101 to 108 in df79392

	export function execWebExt(
	argv: Array<string>,
	spawnOptions: child_process$spawnOpts,
	): RunningWebExt {
	const spawnedProcess = spawn(
	process.execPath, [webExt, ...argv], spawnOptions
	);

@eviljeff do you confirm this is the case? (the package.json manually aligned to the one in 7.1.1 but the rest of the working tree still branched from 7.1.0)

[eviljeff]

I took a look to the windows CI job failure, as I mentioned yesterday I was pretty sure to have seen that and fixed it not long ago, and confirmed that it has been already fixed in #2444 along with the dependency update that started to trigger it

I copied over the changed lines and it's working now 👍 (I did try to cherry-pick the whole commit, but package-lock.json 🤷‍♂️ )

[willdurand]

Some feedback, I haven't looked at the tests yet.

Also, I am trying to see how we could avoid the Promise + setTimeout soup in the waitForXXX methods because that really isn't great (legacy from sign-addon I suppose).

[eviljeff]

Also, I am trying to see how we could avoid the Promise + setTimeout soup in the waitForXXX methods because that really isn't great (legacy from sign-addon I suppose).

I had a try, but the need to reject/resolve from functions inside the timeout seems to stop us from removing the new Promise and just relying on standard return xxx and throw instead. I'm now looking into alternatives that would support the delay+retry functionality further up the stack.

[eviljeff]

Also, I am trying to see how we could avoid the Promise + setTimeout soup in the waitForXXX methods because that really isn't great (legacy from sign-addon I suppose).

I had a try, but the need to reject/resolve from functions inside the timeout seems to stop us from removing the new Promise and just relying on standard return xxx and throw instead. I'm now looking into alternatives that would support the delay+retry functionality further up the stack.

The packages I found that said they added retry to fetch were more for when the request encountered http errors (non-200, etc, error codes) rather than arbitrary retry when we decide the exact json response isn't what we want. So I refactored the retry functionality into a separate function - it doesn't relieve us from the Promise + setTimeout soup but does at least split it up somewhat, and allow validation & approval steps to share some code.

[willdurand]

The implementation looks good to me, thank you!

The test suite probably needs so more work, we cannot keep skipped tests for instance. Also, I am not sure about adding nock as a dev dependency, but I don't have another proposal to offer. I'll let @rpl look at the implementation and then we can take a look at the tests and we should be good.

[rpl]

Follows a new review pass from my perspective, I still plan to take a closer look to the rest of this PR, but in the meantime I wanted to submit this group of comments so that we can update the PR to ensure we run the currently skipped tests and that the PR pass successfully all CI jobs.

[rpl]

@eviljeff apologies for letting you wait for another full review pass from my side, I've looked in more detail the rest of this PR and collected some more review comments.

In the review comments below, some thoughts related to the hardcoded api url prefix, some changes to the new test file to get rid of nock, and some question related to an issue with the waitRetry method that I noticed while I was trying to rework some of the new test cases.

[rpl]

This versions looks good to me (thanks also for the additional tweaks using URL to resolve urls relative to the API host, that looks also nicer).

There is still that detail about the apiHost parameter that I'd like to discuss a bit and potentially reconsider, besides what I mentioned in the comment related to it as part of a previous review pass, if I'm not reading it wrong a user may be passing --api-host but not --use-submission-api and at that point apiHost would be actually unused and may be a bit confusing for the user.

But for reconsidering that I'd prefer to touch base with Will once he is going to be back, and avoid to ask you to go back and forth about that detail (and to be fair we only need to have finalized that decision before we would be releasing a new version and expose those new options to the users).

In the meantime I'm approving this version on my side (and if we would be merging anything that would be introducing new conflict with this PR, I'm going to merge it as is to prevent this PR to get new merge conflicts to solve, we can deal with further work about the new module in separate PRs if we need to, e.g. to followup on that apiHost).

[rpl]

In this version it seems we are going to validate the apiHost even when it is unused, not a big deal on its own given that its default value is valid.