Skip to content

write id to file upon successful submission #2511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 14, 2022
Merged

write id to file upon successful submission #2511

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fe826f
write id to file upon successful submission
eviljeff Sep 12, 2022
bbf1863
update comment to say saveIdToFile
eviljeff Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 20 additions & 28 deletions src/cmd/sign.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import {prepareArtifactsDir} from '../util/artifacts.js';
import {createLogger} from '../util/logger.js';
import getValidatedManifest, {getManifestId} from '../util/manifest.js';
import type {ExtensionManifest} from '../util/manifest.js';
import { signAddon as defaultSubmitAddonSigner } from '../util/submit-addon.js';
import {
signAddon as defaultSubmitAddonSigner,
saveIdToFile,
} from '../util/submit-addon.js';
import type { SignResult } from '../util/submit-addon.js';
import {withTempDir} from '../util/temp-dir.js';

Expand Down Expand Up @@ -81,6 +84,7 @@ export default function sign(
await prepareArtifactsDir(artifactsDir);

let manifestData;
const savedIdPath = path.join(sourceDir, extensionIdFile);

if (preValidatedManifest) {
manifestData = preValidatedManifest;
Expand All @@ -91,7 +95,7 @@ export default function sign(
const [buildResult, idFromSourceDir] = await Promise.all([
build({sourceDir, ignoreFiles, artifactsDir: tmpDir.path()},
{manifestData, showReadyMessage: false}),
getIdFromSourceDir(sourceDir),
getIdFromFile(savedIdPath),
]);

const manifestId = getManifestId(manifestData);
Expand Down Expand Up @@ -169,10 +173,14 @@ export default function sign(
let result;
try {
if (useSubmissionApi) {
result = await submitAddon(
result = await submitAddon({
...signSubmitArgs,
amoBaseUrl,
// $FlowIgnore: we verify 'channel' is set above
{...signSubmitArgs, amoBaseUrl, channel, metaDataJson}
);
channel,
savedIdPath,
metaDataJson,
});
} else {
const { success, id: newId, downloadedFiles } = await signAddon({
...signSubmitArgs,
Expand All @@ -185,28 +193,26 @@ export default function sign(
throw new Error('The extension could not be signed');
}
result = { id: newId, downloadedFiles };
// All information about the downloaded files would have already been
// logged by signAddon(). submitAddon() calls saveIdToSourceDir itself.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, saveIdToSourceDir => saveIdToFile in this inline comment?

await saveIdToFile(savedIdPath, newId);
log.info(`Extension ID: ${newId}`);
log.info('SUCCESS');
}
} catch (clientError) {
log.info('FAIL');
throw new WebExtError(clientError.message);
}

// All information about the downloaded files would have
// already been logged by signAddon() or submitAddon().
await saveIdToSourceDir(sourceDir, result.id);
log.info(`Extension ID: ${result.id}`);
log.info('SUCCESS');
return result;
});
}


export async function getIdFromSourceDir(
sourceDir: string,
export async function getIdFromFile(
filePath: string,
asyncFsReadFile: typeof defaultAsyncFsReadFile = defaultAsyncFsReadFile,
): Promise<string | void> {
const filePath = path.join(sourceDir, extensionIdFile);

let content;

try {
Expand Down Expand Up @@ -236,17 +242,3 @@ export async function getIdFromSourceDir(

return id;
}


export async function saveIdToSourceDir(
sourceDir: string, id: string
): Promise<void> {
const filePath = path.join(sourceDir, extensionIdFile);
await fs.writeFile(filePath, [
'# This file was created by https://github.com/mozilla/web-ext',
'# Your auto-generated extension ID for addons.mozilla.org is:',
id.toString(),
].join('\n'));

log.debug(`Saved auto-generated ID ${id} to ${filePath}`);
}
25 changes: 23 additions & 2 deletions src/util/submit-addon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,9 @@ export default class Client {
async postNewAddon(
xpiPath: string,
channel: string,
metaDataJson: Object
savedIdPath: string,
metaDataJson: Object,
saveIdToFileFunc: (string, string) => Promise<void> = saveIdToFile,
): Promise<SignResult> {
const uploadUuid = await this.doUploadSubmit(xpiPath, channel);

Expand All @@ -297,6 +299,11 @@ export default class Client {
[versionObject]: {id: newVersionId},
} = await this.doNewAddonSubmit(uploadUuid, metaDataJson);

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": "${addonId}"}`);
Comment on lines +303 to +305
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move saveIdToSourceDir() somewhere else (could be in this file even), then those logs could also be moved to saveIdToSourceDir(). If there is a chance that this message does not apply, we could update the content a bit (e.g. from "You must add [...]" to "Make sure you have [...] in your manifest:").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id is never needed for the signAddon route, while AMO doesn't allow MV3 submissions. (If we're specifically adding support for MV3 I'd prefer that in a separate PR).

We only call the function for id-less submissions for submit-addon, so it's always needed.


const fileUrl = new URL(await this.waitForApproval(addonId, newVersionId));

return this.downloadSignedFile(fileUrl, addonId);
Expand Down Expand Up @@ -332,6 +339,7 @@ type signAddonParams = {|
xpiPath: string,
downloadDir: string,
channel: string,
savedIdPath: string,
metaDataJson?: Object,
SubmitClient?: typeof Client,
ApiAuthClass?: typeof JwtApiAuth,
Expand All @@ -346,6 +354,7 @@ export async function signAddon({
xpiPath,
downloadDir,
channel,
savedIdPath,
metaDataJson = {},
SubmitClient = Client,
ApiAuthClass = JwtApiAuth,
Expand Down Expand Up @@ -378,8 +387,20 @@ export async function signAddon({
// 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(xpiPath, channel, metaDataJson);
return client.postNewAddon(xpiPath, channel, savedIdPath, metaDataJson);
}

return client.putVersion(xpiPath, channel, id, metaDataJson);
}

export async function saveIdToFile(
filePath: string, id: string
): Promise<void> {
await fsPromises.writeFile(filePath, [
'# This file was created by https://github.com/mozilla/web-ext',
'# Your auto-generated extension ID for addons.mozilla.org is:',
id.toString(),
].join('\n'));

log.debug(`Saved auto-generated ID ${id} to ${filePath}`);
}
56 changes: 16 additions & 40 deletions tests/unit/test-cmd/test.sign.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import * as sinon from 'sinon';

import {UsageError, WebExtError} from '../../../src/errors.js';
import {getManifestId} from '../../../src/util/manifest.js';
import {saveIdToFile} from '../../../src/util/submit-addon.js';
import {withTempDir} from '../../../src/util/temp-dir.js';
import completeSignCommand, {
extensionIdFile, getIdFromSourceDir, saveIdToSourceDir,
extensionIdFile, getIdFromFile,
} from '../../../src/cmd/sign.js';
import {
basicManifest,
Expand Down Expand Up @@ -208,9 +209,10 @@ describe('sign', () => {
() => withTempDir(
async (tmpDir) => {
const sourceDir = path.join(tmpDir.path(), 'source-dir');
const idFile = path.join(sourceDir, extensionIdFile);
const stubs = getStubs();
await fs.mkdir(sourceDir);
await saveIdToSourceDir(sourceDir, 'some-other-id');
await saveIdToFile(idFile, 'some-other-id');
// Now, make a signing call with a custom ID.
const promiseSigned = sign(tmpDir, stubs, {
extraArgs: {
Expand Down Expand Up @@ -242,11 +244,12 @@ describe('sign', () => {
it('prefers a custom ID over an ID file', () => withTempDir(
(tmpDir) => {
const sourceDir = path.join(tmpDir.path(), 'source-dir');
const idFile = path.join(sourceDir, extensionIdFile);
const customId = 'some-custom-id';
const stubs = getStubs();
// First, save an extension ID like a previous signing call.
return fs.mkdir(sourceDir)
.then(() => saveIdToSourceDir(sourceDir, 'some-other-id'))
.then(() => saveIdToFile(idFile, 'some-other-id'))
// Now, make a signing call with a custom ID.
.then(() => sign(tmpDir, stubs, {
extraArgs: {
Expand Down Expand Up @@ -532,40 +535,13 @@ describe('sign', () => {
}
));

describe('saveIdToSourceDir', () => {

it('saves an extension ID to file', () => withTempDir(
(tmpDir) => {
const sourceDir = tmpDir.path();
return saveIdToSourceDir(sourceDir, 'some-id')
.then(() => fs.readFile(path.join(sourceDir, extensionIdFile)))
.then((content) => {
assert.include(content.toString(), 'some-id');
});
}
));

it('will overwrite an existing file', () => withTempDir(
(tmpDir) => {
const sourceDir = tmpDir.path();
return saveIdToSourceDir(sourceDir, 'first-id')
.then(() => saveIdToSourceDir(sourceDir, 'second-id'))
.then(() => getIdFromSourceDir(sourceDir))
.then((savedId) => {
assert.equal(savedId, 'second-id');
});
}
));

});

describe('getIdFromSourceDir', () => {
describe('getIdFromFile', () => {

it('gets a saved extension ID', () => withTempDir(
(tmpDir) => {
const sourceDir = tmpDir.path();
return saveIdToSourceDir(sourceDir, 'some-id')
.then(() => getIdFromSourceDir(sourceDir))
const idFile = path.join(tmpDir.path(), extensionIdFile);
return saveIdToFile(idFile, 'some-id')
.then(() => getIdFromFile(idFile))
.then((extensionId) => {
assert.equal(extensionId, 'some-id');
});
Expand All @@ -574,9 +550,9 @@ describe('sign', () => {

it('throws an error for empty files', () => withTempDir(
async (tmpDir) => {
const sourceDir = tmpDir.path();
await fs.writeFile(path.join(sourceDir, extensionIdFile), '');
const getIdPromise = getIdFromSourceDir(sourceDir);
const idFile = path.join(tmpDir.path(), extensionIdFile);
await fs.writeFile(idFile, '');
const getIdPromise = getIdFromFile(idFile);
await assert.isRejected(getIdPromise, UsageError);
await assert.isRejected(
getIdPromise, /No ID found in extension ID file/
Expand All @@ -586,8 +562,8 @@ describe('sign', () => {

it('returns empty ID when extension file does not exist', () => withTempDir(
(tmpDir) => {
const sourceDir = tmpDir.path();
return getIdFromSourceDir(sourceDir)
const idFile = path.join(tmpDir.path(), extensionIdFile);
return getIdFromFile(idFile)
.then((savedId) => {
assert.strictEqual(savedId, undefined);
});
Expand All @@ -599,7 +575,7 @@ describe('sign', () => {
throw new Error('Unexpected fs.readFile error');
});
await assert.isRejected(
getIdFromSourceDir('fakeSourceDir', fakeAsyncFsReadFile),
getIdFromFile('fakeIdFile', fakeAsyncFsReadFile),
/Unexpected fs.readFile error/);

sinon.assert.calledOnce(fakeAsyncFsReadFile);
Expand Down
64 changes: 52 additions & 12 deletions tests/unit/test-util/test.submit-addon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { File, FormData, Response } from 'node-fetch';

import Client, {
JwtApiAuth,
saveIdToFile,
signAddon,
} from '../../../src/util/submit-addon.js';
import { withTempDir } from '../../../src/util/temp-dir.js';
Expand Down Expand Up @@ -75,6 +76,7 @@ describe('util.submit-addon', () => {
downloadDir: '/some-dir/',
xpiPath: '/some.xpi',
channel: 'some-channel',
savedIdPath: '.id-file',
};

it('creates Client with parameters', async () => {
Expand Down Expand Up @@ -118,13 +120,17 @@ describe('util.submit-addon', () => {
it('calls postNewAddon if `id` is undefined', async () => {
const xpiPath = 'this/path/xpi.xpi';
const channel = 'thisChannel';
const savedIdPath = '.some.id.file';
await signAddon({
...signAddonDefaults,
xpiPath,
channel,
savedIdPath,
});
sinon.assert.notCalled(putVersionStub);
sinon.assert.calledWith(postNewAddonStub, xpiPath, channel, {});
sinon.assert.calledWith(
postNewAddonStub, xpiPath, channel, savedIdPath, {}
);
});

it('calls putVersion if `id` is defined', async () => {
Expand Down Expand Up @@ -171,6 +177,7 @@ describe('util.submit-addon', () => {
postNewAddonStub,
signAddonDefaults.xpiPath,
signAddonDefaults.channel,
signAddonDefaults.savedIdPath,
metaDataJson
);
});
Expand Down Expand Up @@ -620,17 +627,23 @@ describe('util.submit-addon', () => {
{channel: 'listed', versionId: sampleVersionDetail.id},
{channel: 'unlisted', versionId: sampleVersionDetail2.id},
].forEach(({channel, versionId}) =>
it('uploads new listed add-on; downloads the signed xpi', async () => {
addUploadMocks();
mockNodeFetch(
nodeFetchStub,
new URL('/addons/addon/', baseUrl),
'POST',
[{ body: sampleAddonDetail, status: 200 }]
);
addApprovalMocks(versionId);
await client.postNewAddon(xpiPath, channel, {});
}));
it(
`uploads new ${channel} add-on; downloads the signed xpi`,
async () => {
addUploadMocks();
const saveIdStub = sinon.stub();
saveIdStub.resolves();
const idFile = 'id.file';
mockNodeFetch(
nodeFetchStub,
new URL('/addons/addon/', baseUrl),
'POST',
[{ body: sampleAddonDetail, status: 200 }]
);
addApprovalMocks(versionId);
await client.postNewAddon(xpiPath, channel, idFile, {}, saveIdStub);
sinon.assert.calledWith(saveIdStub, idFile, sampleAddonDetail.guid);
}));

it('uploads a new version; then downloads the signed xpi', async () => {
const channel = 'listed';
Expand Down Expand Up @@ -766,4 +779,31 @@ describe('util.submit-addon', () => {
});
});
});

describe('saveIdToFile', () => {

it('saves an extension ID to file', () => withTempDir(
(tmpDir) => {
const idFile = path.join(tmpDir.path(), 'extensionId.File');
return saveIdToFile(idFile, 'some-id')
.then(() => fs.readFile(idFile))
.then((content) => {
assert.include(content.toString(), 'some-id');
});
}
));

it('will overwrite an existing file', () => withTempDir(
(tmpDir) => {
const idFile = path.join(tmpDir.path(), 'extensionId.File');
return saveIdToFile(idFile, 'first-id')
.then(() => saveIdToFile(idFile, 'second-id'))
.then(() => fs.readFile(idFile))
.then((content) => {
assert.include(content.toString(), 'second-id');
});
}
));
});

});