From df6d21514e4360e8170e93686d98a497618bd830 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Thu, 31 Aug 2023 20:43:47 +0100 Subject: [PATCH 1/4] implement source file upload --- src/cmd/sign.js | 2 + src/program.js | 5 + src/util/submit-addon.js | 49 +++++- tests/unit/test-cmd/test.sign.js | 17 +++ tests/unit/test-util/test.submit-addon.js | 173 +++++++++++++++++++++- 5 files changed, 241 insertions(+), 5 deletions(-) diff --git a/src/cmd/sign.js b/src/cmd/sign.js index 9b1ecf2317..724b88d8b0 100644 --- a/src/cmd/sign.js +++ b/src/cmd/sign.js @@ -40,6 +40,7 @@ export default function sign( verbose, channel, amoMetadata, + versionSource, webextVersion, }, { @@ -152,6 +153,7 @@ export default function sign( validationCheckTimeout: timeout, approvalCheckTimeout: approvalTimeout !== undefined ? approvalTimeout : timeout, + versionSource, }); } else { const { diff --git a/src/program.js b/src/program.js index d958b64889..ce3575ba20 100644 --- a/src/program.js +++ b/src/program.js @@ -601,6 +601,11 @@ Example: $0 --help run. 'Only used with `use-submission-api`', type: 'string', }, + 'version-source': { + describe: + 'Path to a zip file containing human readable source code for a version. ' + + 'Only used with `use-submission-api`', + }, }, ) .command('run', 'Run the extension', commands.run, { diff --git a/src/util/submit-addon.js b/src/util/submit-addon.js index 709406c150..ee2c058c01 100644 --- a/src/util/submit-addon.js +++ b/src/util/submit-addon.js @@ -185,6 +185,27 @@ export default class Client { return this.fetchJson(url, 'PUT', JSON.stringify(jsonData)); } + async doFormDataPatch(data, addonId, versionId) { + const patchUrl = new URL( + `addon/${addonId}/versions/${versionId}/`, + this.apiUrl, + ); + try { + const formData = new FormData(); + for (const field in data) { + formData.set(field, data[field]); + } + + const response = await this.fetch(patchUrl, 'PATCH', formData); + if (!response.ok) { + throw new Error(`response status was ${response.status}`); + } + } catch (error) { + log.info(`Upload of ${Object.keys(data)} failed: ${error}.`); + throw new Error(`Uploading ${Object.keys(data)} failed`); + } + } + async doAfterSubmit(addonId, newVersionId, editUrl) { if (this.approvalCheckTimeout > 0) { const fileUrl = new URL( @@ -237,7 +258,7 @@ export default class Client { } async fetch(url, method = 'GET', body) { - log.info(`Fetching URL: ${url.href}`); + log.info(`${method}ing URL: ${url.href}`); let headers = { Authorization: await this.apiAuth.getAuthHeader(), Accept: 'application/json', @@ -350,6 +371,7 @@ export default class Client { uploadUuid, savedIdPath, metaDataJson, + versionPatchData, saveIdToFileFunc = saveIdToFile, ) { const { @@ -357,6 +379,11 @@ export default class Client { version: { id: newVersionId, edit_url: editUrl }, } = await this.doNewAddonSubmit(uploadUuid, metaDataJson); + if (versionPatchData) { + log.info('Submitting source zip'); + await this.doFormDataPatch(versionPatchData, addonId, newVersionId); + } + await saveIdToFileFunc(savedIdPath, addonId); log.info(`Generated extension ID: ${addonId}.`); log.info('You must add the following to your manifest:'); @@ -365,11 +392,15 @@ export default class Client { return this.doAfterSubmit(addonId, newVersionId, editUrl); } - async putVersion(uploadUuid, addonId, metaDataJson) { + async putVersion(uploadUuid, addonId, metaDataJson, versionPatchData) { const { version: { id: newVersionId, edit_url: editUrl }, } = await this.doNewAddonOrVersionSubmit(addonId, uploadUuid, metaDataJson); + if (versionPatchData) { + log.info('Submitting source zip'); + await this.doFormDataPatch(versionPatchData, addonId, newVersionId); + } return this.doAfterSubmit(addonId, newVersionId, editUrl); } } @@ -388,6 +419,7 @@ export async function signAddon({ savedIdPath, savedUploadUuidPath, metaDataJson = {}, + versionSource, userAgentString, SubmitClient = Client, ApiAuthClass = JwtApiAuth, @@ -423,14 +455,23 @@ export async function signAddon({ channel, savedUploadUuidPath, ); + // if we have a source file we need to upload we patch after the create + const versionPatchData = versionSource + ? { source: client.fileFromSync(versionSource) } + : undefined; // We specifically need to know if `id` has not been passed as a parameter because // it's the indication that a new add-on should be created, rather than a new version. if (id === undefined) { - return client.postNewAddon(uploadUuid, savedIdPath, metaDataJson); + return client.postNewAddon( + uploadUuid, + savedIdPath, + metaDataJson, + versionPatchData, + ); } - return client.putVersion(uploadUuid, id, metaDataJson); + return client.putVersion(uploadUuid, id, metaDataJson, versionPatchData); } export async function saveIdToFile(filePath, id) { diff --git a/tests/unit/test-cmd/test.sign.js b/tests/unit/test-cmd/test.sign.js index b5283b1561..6b5e811364 100644 --- a/tests/unit/test-cmd/test.sign.js +++ b/tests/unit/test-cmd/test.sign.js @@ -384,6 +384,23 @@ describe('sign', () => { }); })); + it('passes the versionSource parameter to submissionAPI signer', () => + withTempDir((tmpDir) => { + const stubs = getStubs(); + const versionSource = 'path/to/source.zip'; + return sign(tmpDir, stubs, { + extraArgs: { + versionSource, + useSubmissionApi: true, + channel: 'unlisted', + }, + }).then(() => { + sinon.assert.called(stubs.signingOptions.submitAddon); + sinon.assert.calledWithMatch(stubs.signingOptions.submitAddon, { + versionSource, + }); + }); + })); it('returns a signing result', () => withTempDir((tmpDir) => { const stubs = getStubs(); diff --git a/tests/unit/test-util/test.submit-addon.js b/tests/unit/test-util/test.submit-addon.js index bfff61fea7..f429dbec0d 100644 --- a/tests/unit/test-util/test.submit-addon.js +++ b/tests/unit/test-util/test.submit-addon.js @@ -50,7 +50,9 @@ describe('util.submit-addon', () => { let getPreviousUuidOrUploadXpiStub; let postNewAddonStub; let putVersionStub; + let fileFromSyncStub; const uploadUuid = '{some-upload-uuid}'; + const fakeFileFromSync = new File([], 'foo.xpi'); beforeEach(() => { statStub = sinon @@ -61,6 +63,9 @@ describe('util.submit-addon', () => { .resolves(uploadUuid); postNewAddonStub = sinon.stub(Client.prototype, 'postNewAddon'); putVersionStub = sinon.stub(Client.prototype, 'putVersion'); + fileFromSyncStub = sinon + .stub(Client.prototype, 'fileFromSync') + .returns(fakeFileFromSync); }); afterEach(() => { @@ -68,6 +73,7 @@ describe('util.submit-addon', () => { getPreviousUuidOrUploadXpiStub.restore(); postNewAddonStub.restore(); putVersionStub.restore(); + fileFromSyncStub.restore(); }); const signAddonDefaults = { @@ -122,6 +128,7 @@ describe('util.submit-addon', () => { downloadDir, userAgentString, }); + sinon.assert.notCalled(fileFromSyncStub); }); it('calls postNewAddon if `id` is undefined', async () => { @@ -187,6 +194,42 @@ describe('util.submit-addon', () => { metaDataJson, ); }); + + it('includes source data to be patched if versionSource defined for new addon', async () => { + const versionSource = 'path/to/source/zip'; + await signAddon({ + ...signAddonDefaults, + versionSource, + }); + + sinon.assert.calledWith(fileFromSyncStub, versionSource); + sinon.assert.calledWith( + postNewAddonStub, + uploadUuid, + signAddonDefaults.savedIdPath, + {}, + { source: fakeFileFromSync }, + ); + }); + + it('includes source data to be patched if versionSource defined for new version', async () => { + const versionSource = 'path/to/source/zip'; + const id = '@thisID'; + await signAddon({ + ...signAddonDefaults, + versionSource, + id, + }); + + sinon.assert.calledWith(fileFromSyncStub, versionSource); + sinon.assert.calledWith( + putVersionStub, + uploadUuid, + id, + {}, + { source: fakeFileFromSync }, + ); + }); }); describe('Client', () => { @@ -738,6 +781,42 @@ describe('util.submit-addon', () => { }); }); + describe('doFormDataPatch', () => { + const addonId = 'some-addon-id'; + const versionId = 123456; + const dataField1 = 'someField'; + const dataField2 = 'otherField'; + const data = { dataField1: 'value', dataField2: 0 }; + const formData = new FormData(); + formData.append(dataField1, data[dataField1]); + formData.append(dataField2, data[dataField2]); + + it('creates the url from addon and version', async () => { + const client = new Client(clientDefaults); + const fetchStub = sinon + .stub(client, 'fetch') + .resolves(new Response('', { ok: true, status: 200 })); + await client.doFormDataPatch(data, addonId, versionId); + const patchUrl = new URL( + `addon/${addonId}/versions/${versionId}/`, + client.apiUrl, + ); + + sinon.assert.calledWith(fetchStub, patchUrl, 'PATCH', formData); + }); + + it('catches and throws for non ok responses', async () => { + const client = new Client(clientDefaults); + sinon.stub(client, 'fetch').resolves(); + const response = client.doFormDataPatch(data, addonId, versionId); + + assert.isRejected( + response, + `Uploading ${dataField1}${dataField2} failed`, + ); + }); + }); + describe('waitForApproval', () => { it('aborts approval wait after timeout', async () => { const client = new Client({ @@ -902,7 +981,13 @@ describe('util.submit-addon', () => { [{ body: sampleAddonDetail, status: 200 }], ); addApprovalMocks(versionId); - await client.postNewAddon(uploadUuid, idFile, {}, saveIdStub); + await client.postNewAddon( + uploadUuid, + idFile, + {}, + undefined, + saveIdStub, + ); sinon.assert.calledWith(saveIdStub, idFile, sampleAddonDetail.guid); }); @@ -920,6 +1005,92 @@ describe('util.submit-addon', () => { await client.putVersion(uploadUuid, `${addonId}`, {}); }); + describe('doFormDataPatch called correctly', () => { + const versionPatchData = { source: 'somesource' }; + const metaDataJson = { some: 'metadata' }; + const newVersionId = 123456; + const editUrl = 'http://some/url'; + const stubbedClient = new Client(clientDefaults); + + const submitResponse = { + guid: addonId, + version: { id: newVersionId, edit_url: editUrl }, + }; + sinon + .stub(stubbedClient, 'doNewAddonOrVersionSubmit') + .resolves(submitResponse); + sinon.stub(stubbedClient, 'doNewAddonSubmit').resolves(submitResponse); + sinon.stub(stubbedClient, 'doAfterSubmit').resolves(); + + let doFormDataPatchStub; + + before(() => { + doFormDataPatchStub = sinon + .stub(stubbedClient, 'doFormDataPatch') + .resolves(); + }); + + afterEach(() => { + doFormDataPatchStub.reset(); + }); + + it('calls doFormDataPatch if versionPatchData is defined for postNewAddon', async () => { + const saveIdToFileStub = sinon.stub().resolves(); + const savedIdPath = 'some/saved/id/path'; + await stubbedClient.postNewAddon( + uploadUuid, + savedIdPath, + metaDataJson, + versionPatchData, + saveIdToFileStub, + ); + + sinon.assert.calledWith( + doFormDataPatchStub, + versionPatchData, + addonId, + newVersionId, + ); + }); + + it('calls doFormDataPatch if versionPatchData is defined for putVersion', async () => { + await stubbedClient.putVersion( + uploadUuid, + addonId, + metaDataJson, + versionPatchData, + ); + + sinon.assert.called(doFormDataPatchStub); + sinon.assert.calledWith( + doFormDataPatchStub, + versionPatchData, + addonId, + newVersionId, + ); + }); + + it('does not call doFormDataPatch is versionPatchData is undefined for postNewAddon', async () => { + const saveIdToFileStub = sinon.stub().resolves(); + const savedIdPath = 'some/saved/id/path'; + await stubbedClient.postNewAddon( + uploadUuid, + savedIdPath, + metaDataJson, + undefined, + saveIdToFileStub, + ); + + sinon.assert.notCalled(doFormDataPatchStub); + }); + + it('does not call doFormDataPatch is versionPatchData is undefined for putVersion', async () => { + await stubbedClient.putVersion(uploadUuid, addonId, metaDataJson); + + sinon.assert.notCalled(doFormDataPatchStub); + }); + }); + describe('doAfterSubmit', () => { const downloadUrl = 'https://a.download/url'; let approvalStub; From ec6794273c001fdb357838b919fe19f03e35ff86 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 4 Sep 2023 19:35:59 +0100 Subject: [PATCH 2/4] version-source to upload-source-code; reword helptext; versionSource in submit-addon.js to submissionSource; versionPatchData to patchData.version --- src/cmd/sign.js | 4 +-- src/program.js | 9 +++-- src/util/submit-addon.js | 26 ++++++++------- tests/unit/test-cmd/test.sign.js | 8 ++--- tests/unit/test-util/test.submit-addon.js | 40 +++++++++++------------ 5 files changed, 46 insertions(+), 41 deletions(-) diff --git a/src/cmd/sign.js b/src/cmd/sign.js index 724b88d8b0..15d88d426d 100644 --- a/src/cmd/sign.js +++ b/src/cmd/sign.js @@ -40,7 +40,7 @@ export default function sign( verbose, channel, amoMetadata, - versionSource, + uploadSourceCode, webextVersion, }, { @@ -153,7 +153,7 @@ export default function sign( validationCheckTimeout: timeout, approvalCheckTimeout: approvalTimeout !== undefined ? approvalTimeout : timeout, - versionSource, + submissionSource: uploadSourceCode, }); } else { const { diff --git a/src/program.js b/src/program.js index ce3575ba20..50a6b5f7b1 100644 --- a/src/program.js +++ b/src/program.js @@ -601,10 +601,13 @@ Example: $0 --help run. 'Only used with `use-submission-api`', type: 'string', }, - 'version-source': { + 'upload-source-code': { describe: - 'Path to a zip file containing human readable source code for a version. ' + - 'Only used with `use-submission-api`', + 'Path to an archive file containing human readable source code of this submission, ' + + 'if the code in --source-dir has been processed to make it unreadable. ' + + 'See https://extensionworkshop.com/documentation/publish/source-code-submission/ for ' + + 'details. Only used with `use-submission-api`', + type: 'string', }, }, ) diff --git a/src/util/submit-addon.js b/src/util/submit-addon.js index ee2c058c01..2c43706869 100644 --- a/src/util/submit-addon.js +++ b/src/util/submit-addon.js @@ -371,7 +371,7 @@ export default class Client { uploadUuid, savedIdPath, metaDataJson, - versionPatchData, + patchData, saveIdToFileFunc = saveIdToFile, ) { const { @@ -379,9 +379,9 @@ export default class Client { version: { id: newVersionId, edit_url: editUrl }, } = await this.doNewAddonSubmit(uploadUuid, metaDataJson); - if (versionPatchData) { + if (patchData && patchData.version) { log.info('Submitting source zip'); - await this.doFormDataPatch(versionPatchData, addonId, newVersionId); + await this.doFormDataPatch(patchData.version, addonId, newVersionId); } await saveIdToFileFunc(savedIdPath, addonId); @@ -392,14 +392,14 @@ export default class Client { return this.doAfterSubmit(addonId, newVersionId, editUrl); } - async putVersion(uploadUuid, addonId, metaDataJson, versionPatchData) { + async putVersion(uploadUuid, addonId, metaDataJson, patchData) { const { version: { id: newVersionId, edit_url: editUrl }, } = await this.doNewAddonOrVersionSubmit(addonId, uploadUuid, metaDataJson); - if (versionPatchData) { + if (patchData && patchData.version) { log.info('Submitting source zip'); - await this.doFormDataPatch(versionPatchData, addonId, newVersionId); + await this.doFormDataPatch(patchData.version, addonId, newVersionId); } return this.doAfterSubmit(addonId, newVersionId, editUrl); } @@ -419,7 +419,7 @@ export async function signAddon({ savedIdPath, savedUploadUuidPath, metaDataJson = {}, - versionSource, + submissionSource, userAgentString, SubmitClient = Client, ApiAuthClass = JwtApiAuth, @@ -456,9 +456,11 @@ export async function signAddon({ savedUploadUuidPath, ); // if we have a source file we need to upload we patch after the create - const versionPatchData = versionSource - ? { source: client.fileFromSync(versionSource) } - : undefined; + const patchData = { + version: submissionSource + ? { source: client.fileFromSync(submissionSource) } + : undefined, + }; // We specifically need to know if `id` has not been passed as a parameter because // it's the indication that a new add-on should be created, rather than a new version. @@ -467,11 +469,11 @@ export async function signAddon({ uploadUuid, savedIdPath, metaDataJson, - versionPatchData, + patchData, ); } - return client.putVersion(uploadUuid, id, metaDataJson, versionPatchData); + return client.putVersion(uploadUuid, id, metaDataJson, patchData); } export async function saveIdToFile(filePath, id) { diff --git a/tests/unit/test-cmd/test.sign.js b/tests/unit/test-cmd/test.sign.js index 6b5e811364..3b0d5a0acc 100644 --- a/tests/unit/test-cmd/test.sign.js +++ b/tests/unit/test-cmd/test.sign.js @@ -384,20 +384,20 @@ describe('sign', () => { }); })); - it('passes the versionSource parameter to submissionAPI signer', () => + it('passes the uploadSourceCode parameter to submissionAPI signer as submissionSource', () => withTempDir((tmpDir) => { const stubs = getStubs(); - const versionSource = 'path/to/source.zip'; + const uploadSourceCode = 'path/to/source.zip'; return sign(tmpDir, stubs, { extraArgs: { - versionSource, + uploadSourceCode, useSubmissionApi: true, channel: 'unlisted', }, }).then(() => { sinon.assert.called(stubs.signingOptions.submitAddon); sinon.assert.calledWithMatch(stubs.signingOptions.submitAddon, { - versionSource, + submissionSource: uploadSourceCode, }); }); })); diff --git a/tests/unit/test-util/test.submit-addon.js b/tests/unit/test-util/test.submit-addon.js index f429dbec0d..c430d9c098 100644 --- a/tests/unit/test-util/test.submit-addon.js +++ b/tests/unit/test-util/test.submit-addon.js @@ -195,39 +195,39 @@ describe('util.submit-addon', () => { ); }); - it('includes source data to be patched if versionSource defined for new addon', async () => { - const versionSource = 'path/to/source/zip'; + it('includes source data to be patched if submissionSource defined for new addon', async () => { + const submissionSource = 'path/to/source/zip'; await signAddon({ ...signAddonDefaults, - versionSource, + submissionSource, }); - sinon.assert.calledWith(fileFromSyncStub, versionSource); + sinon.assert.calledWith(fileFromSyncStub, submissionSource); sinon.assert.calledWith( postNewAddonStub, uploadUuid, signAddonDefaults.savedIdPath, {}, - { source: fakeFileFromSync }, + { version: { source: fakeFileFromSync } }, ); }); - it('includes source data to be patched if versionSource defined for new version', async () => { - const versionSource = 'path/to/source/zip'; + it('includes source data to be patched if submissionSource defined for new version', async () => { + const submissionSource = 'path/to/source/zip'; const id = '@thisID'; await signAddon({ ...signAddonDefaults, - versionSource, + submissionSource, id, }); - sinon.assert.calledWith(fileFromSyncStub, versionSource); + sinon.assert.calledWith(fileFromSyncStub, submissionSource); sinon.assert.calledWith( putVersionStub, uploadUuid, id, {}, - { source: fakeFileFromSync }, + { version: { source: fakeFileFromSync } }, ); }); }); @@ -1006,7 +1006,7 @@ describe('util.submit-addon', () => { }); describe('doFormDataPatch called correctly', () => { - const versionPatchData = { source: 'somesource' }; + const patchData = { version: { source: 'somesource' } }; const metaDataJson = { some: 'metadata' }; const newVersionId = 123456; const editUrl = 'http://some/url'; @@ -1034,57 +1034,57 @@ describe('util.submit-addon', () => { doFormDataPatchStub.reset(); }); - it('calls doFormDataPatch if versionPatchData is defined for postNewAddon', async () => { + it('calls doFormDataPatch if patchData.version is defined for postNewAddon', async () => { const saveIdToFileStub = sinon.stub().resolves(); const savedIdPath = 'some/saved/id/path'; await stubbedClient.postNewAddon( uploadUuid, savedIdPath, metaDataJson, - versionPatchData, + patchData, saveIdToFileStub, ); sinon.assert.calledWith( doFormDataPatchStub, - versionPatchData, + patchData.version, addonId, newVersionId, ); }); - it('calls doFormDataPatch if versionPatchData is defined for putVersion', async () => { + it('calls doFormDataPatch if patchData.version is defined for putVersion', async () => { await stubbedClient.putVersion( uploadUuid, addonId, metaDataJson, - versionPatchData, + patchData, ); sinon.assert.called(doFormDataPatchStub); sinon.assert.calledWith( doFormDataPatchStub, - versionPatchData, + patchData.version, addonId, newVersionId, ); }); - it('does not call doFormDataPatch is versionPatchData is undefined for postNewAddon', async () => { + it('does not call doFormDataPatch is patchData.version is undefined for postNewAddon', async () => { const saveIdToFileStub = sinon.stub().resolves(); const savedIdPath = 'some/saved/id/path'; await stubbedClient.postNewAddon( uploadUuid, savedIdPath, metaDataJson, - undefined, + {}, saveIdToFileStub, ); sinon.assert.notCalled(doFormDataPatchStub); }); - it('does not call doFormDataPatch is versionPatchData is undefined for putVersion', async () => { + it('does not call doFormDataPatch is patchData.version is undefined for putVersion', async () => { await stubbedClient.putVersion(uploadUuid, addonId, metaDataJson); sinon.assert.notCalled(doFormDataPatchStub); From 52656dc16b768d6a5f87816fc983640eed458c55 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 5 Sep 2023 11:08:43 +0100 Subject: [PATCH 3/4] refactor so call to doFormDataPatch isn't duplicated --- src/util/submit-addon.js | 19 ++-- tests/unit/test-util/test.submit-addon.js | 119 ++++++---------------- 2 files changed, 40 insertions(+), 98 deletions(-) diff --git a/src/util/submit-addon.js b/src/util/submit-addon.js index 2c43706869..add240e24a 100644 --- a/src/util/submit-addon.js +++ b/src/util/submit-addon.js @@ -206,7 +206,11 @@ export default class Client { } } - async doAfterSubmit(addonId, newVersionId, editUrl) { + async doAfterSubmit(addonId, newVersionId, editUrl, patchData) { + if (patchData && patchData.version) { + log.info(`Submitting ${Object.keys(patchData.version)} to version`); + await this.doFormDataPatch(patchData.version, addonId, newVersionId); + } if (this.approvalCheckTimeout > 0) { const fileUrl = new URL( await this.waitForApproval(addonId, newVersionId), @@ -379,17 +383,12 @@ export default class Client { version: { id: newVersionId, edit_url: editUrl }, } = await this.doNewAddonSubmit(uploadUuid, metaDataJson); - if (patchData && patchData.version) { - log.info('Submitting source zip'); - await this.doFormDataPatch(patchData.version, addonId, newVersionId); - } - await saveIdToFileFunc(savedIdPath, addonId); log.info(`Generated extension ID: ${addonId}.`); log.info('You must add the following to your manifest:'); log.info(`"browser_specific_settings": {"gecko": {"id": "${addonId}"}}`); - return this.doAfterSubmit(addonId, newVersionId, editUrl); + return this.doAfterSubmit(addonId, newVersionId, editUrl, patchData); } async putVersion(uploadUuid, addonId, metaDataJson, patchData) { @@ -397,11 +396,7 @@ export default class Client { version: { id: newVersionId, edit_url: editUrl }, } = await this.doNewAddonOrVersionSubmit(addonId, uploadUuid, metaDataJson); - if (patchData && patchData.version) { - log.info('Submitting source zip'); - await this.doFormDataPatch(patchData.version, addonId, newVersionId); - } - return this.doAfterSubmit(addonId, newVersionId, editUrl); + return this.doAfterSubmit(addonId, newVersionId, editUrl, patchData); } } diff --git a/tests/unit/test-util/test.submit-addon.js b/tests/unit/test-util/test.submit-addon.js index c430d9c098..713c4b1e8f 100644 --- a/tests/unit/test-util/test.submit-addon.js +++ b/tests/unit/test-util/test.submit-addon.js @@ -1005,63 +1005,50 @@ describe('util.submit-addon', () => { await client.putVersion(uploadUuid, `${addonId}`, {}); }); - describe('doFormDataPatch called correctly', () => { + describe('doAfterSubmit', () => { + const downloadUrl = 'https://a.download/url'; + const newVersionId = sampleVersionDetail.id; + const editUrl = sampleVersionDetail.editUrl; const patchData = { version: { source: 'somesource' } }; - const metaDataJson = { some: 'metadata' }; - const newVersionId = 123456; - const editUrl = 'http://some/url'; - const stubbedClient = new Client(clientDefaults); - - const submitResponse = { - guid: addonId, - version: { id: newVersionId, edit_url: editUrl }, - }; - sinon - .stub(stubbedClient, 'doNewAddonOrVersionSubmit') - .resolves(submitResponse); - sinon.stub(stubbedClient, 'doNewAddonSubmit').resolves(submitResponse); - sinon.stub(stubbedClient, 'doAfterSubmit').resolves(); + let approvalStub; + let downloadStub; let doFormDataPatchStub; before(() => { + approvalStub = sinon + .stub(client, 'waitForApproval') + .resolves(downloadUrl); + downloadStub = sinon.stub(client, 'downloadSignedFile').resolves(); doFormDataPatchStub = sinon - .stub(stubbedClient, 'doFormDataPatch') + .stub(client, 'doFormDataPatch') .resolves(); }); afterEach(() => { - doFormDataPatchStub.reset(); + approvalStub.resetHistory(); + downloadStub.resetHistory(); + doFormDataPatchStub.resetHistory(); }); - it('calls doFormDataPatch if patchData.version is defined for postNewAddon', async () => { - const saveIdToFileStub = sinon.stub().resolves(); - const savedIdPath = 'some/saved/id/path'; - await stubbedClient.postNewAddon( - uploadUuid, - savedIdPath, - metaDataJson, - patchData, - saveIdToFileStub, - ); + it('skips download if approval timeout is 0', async () => { + client.approvalCheckTimeout = 0; + await client.doAfterSubmit(addonId, newVersionId, editUrl); + sinon.assert.notCalled(approvalStub); + sinon.assert.notCalled(downloadStub); + }); - sinon.assert.calledWith( - doFormDataPatchStub, - patchData.version, - addonId, - newVersionId, - ); + it('downloads the signed xpi if approval timeout > 0', async () => { + client.approvalCheckTimeout = 1; + await client.doAfterSubmit(addonId, newVersionId, editUrl); + sinon.assert.calledWith(approvalStub, addonId, newVersionId); + sinon.assert.calledWith(downloadStub, new URL(downloadUrl), addonId); }); - it('calls doFormDataPatch if patchData.version is defined for putVersion', async () => { - await stubbedClient.putVersion( - uploadUuid, - addonId, - metaDataJson, - patchData, - ); + it('calls doFormDataPatch if patchData.version is defined', async () => { + client.approvalCheckTimeout = 0; + await client.doAfterSubmit(addonId, newVersionId, editUrl, patchData); - sinon.assert.called(doFormDataPatchStub); sinon.assert.calledWith( doFormDataPatchStub, patchData.version, @@ -1070,53 +1057,13 @@ describe('util.submit-addon', () => { ); }); - it('does not call doFormDataPatch is patchData.version is undefined for postNewAddon', async () => { - const saveIdToFileStub = sinon.stub().resolves(); - const savedIdPath = 'some/saved/id/path'; - await stubbedClient.postNewAddon( - uploadUuid, - savedIdPath, - metaDataJson, - {}, - saveIdToFileStub, - ); - - sinon.assert.notCalled(doFormDataPatchStub); - }); - - it('does not call doFormDataPatch is patchData.version is undefined for putVersion', async () => { - await stubbedClient.putVersion(uploadUuid, addonId, metaDataJson); - - sinon.assert.notCalled(doFormDataPatchStub); - }); - }); - - describe('doAfterSubmit', () => { - const downloadUrl = 'https://a.download/url'; - let approvalStub; - let downloadStub; - const newVersionId = sampleVersionDetail.id; - const editUrl = sampleVersionDetail.editUrl; - - before(() => { - approvalStub = sinon - .stub(client, 'waitForApproval') - .resolves(downloadUrl); - downloadStub = sinon.stub(client, 'downloadSignedFile').resolves(); - }); - - it('skips download if approval timeout is 0', async () => { + it('does not call doFormDataPatch is patchData.version is undefined', async () => { client.approvalCheckTimeout = 0; - await client.doAfterSubmit(addonId, newVersionId, editUrl); - sinon.assert.notCalled(approvalStub); - sinon.assert.notCalled(downloadStub); - }); + await client.doAfterSubmit(addonId, newVersionId, editUrl, { + version: undefined, + }); - it('downloads the signed xpi if approval timeout > 0', async () => { - client.approvalCheckTimeout = 1; - await client.doAfterSubmit(addonId, newVersionId, editUrl); - sinon.assert.calledWith(approvalStub, addonId, newVersionId); - sinon.assert.calledWith(downloadStub, new URL(downloadUrl), addonId); + sinon.assert.notCalled(doFormDataPatchStub); }); }); }); From 26efa493111b7ce80e6b794ffc4704e9c9898ac0 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 25 Sep 2023 20:07:13 +0100 Subject: [PATCH 4/4] validate submissionSource and raise if not valid --- src/util/submit-addon.js | 20 +++++++++----- tests/unit/test-util/test.submit-addon.js | 32 +++++++++++++++++++++++ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/util/submit-addon.js b/src/util/submit-addon.js index add240e24a..160cbf9dbf 100644 --- a/src/util/submit-addon.js +++ b/src/util/submit-addon.js @@ -423,7 +423,7 @@ export async function signAddon({ const stats = await fsPromises.stat(xpiPath); if (!stats.isFile()) { - throw new Error(`not a file: ${xpiPath}`); + throw new Error('not a file'); } } catch (statError) { throw new Error(`error with ${xpiPath}: ${statError}`); @@ -450,12 +450,20 @@ export async function signAddon({ channel, savedUploadUuidPath, ); + const patchData = {}; // if we have a source file we need to upload we patch after the create - const patchData = { - version: submissionSource - ? { source: client.fileFromSync(submissionSource) } - : undefined, - }; + if (submissionSource) { + try { + const stats2 = await fsPromises.stat(submissionSource); + + if (!stats2.isFile()) { + throw new Error('not a file'); + } + } catch (statError) { + throw new Error(`error with ${submissionSource}: ${statError}`); + } + patchData.version = { source: client.fileFromSync(submissionSource) }; + } // We specifically need to know if `id` has not been passed as a parameter because // it's the indication that a new add-on should be created, rather than a new version. diff --git a/tests/unit/test-util/test.submit-addon.js b/tests/unit/test-util/test.submit-addon.js index 713c4b1e8f..70850722ea 100644 --- a/tests/unit/test-util/test.submit-addon.js +++ b/tests/unit/test-util/test.submit-addon.js @@ -57,7 +57,9 @@ describe('util.submit-addon', () => { beforeEach(() => { statStub = sinon .stub(fsPromises, 'stat') + .onFirstCall() .resolves({ isFile: () => true }); + statStub.callThrough(); getPreviousUuidOrUploadXpiStub = sinon .stub(Client.prototype, 'getPreviousUuidOrUploadXpi') .resolves(uploadUuid); @@ -197,6 +199,7 @@ describe('util.submit-addon', () => { it('includes source data to be patched if submissionSource defined for new addon', async () => { const submissionSource = 'path/to/source/zip'; + statStub.onSecondCall().resolves({ isFile: () => true }); await signAddon({ ...signAddonDefaults, submissionSource, @@ -214,6 +217,7 @@ describe('util.submit-addon', () => { it('includes source data to be patched if submissionSource defined for new version', async () => { const submissionSource = 'path/to/source/zip'; + statStub.onSecondCall().resolves({ isFile: () => true }); const id = '@thisID'; await signAddon({ ...signAddonDefaults, @@ -230,6 +234,34 @@ describe('util.submit-addon', () => { { version: { source: fakeFileFromSync } }, ); }); + + it('throws error if submissionSource is not found', async () => { + const submissionSource = 'path/to/source/zip'; + const signAddonPromise = signAddon({ + ...signAddonDefaults, + submissionSource, + }); + await assert.isRejected( + signAddonPromise, + `error with ${submissionSource}: ` + + 'Error: ENOENT: no such file or directory', + ); + }); + + it('throws error if submissionSource is a directory', async () => { + await withTempDir(async (tmpDir) => { + const submissionSource = path.join(tmpDir.path(), 'someDirectory'); + await fsPromises.mkdir(submissionSource); + const signAddonPromise = signAddon({ + ...signAddonDefaults, + submissionSource, + }); + await assert.isRejected( + signAddonPromise, + `error with ${submissionSource}: ` + 'Error: not a file', + ); + }); + }); }); describe('Client', () => {