Skip to content
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.

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

feat: install and run an extension with custom Firefox preferences #658

Merged
merged 8 commits into from
Dec 19, 2016
15 changes: 11 additions & 4 deletions src/cmd/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const log = createLogger(__filename);

// Import objects that are only used as Flow types.
import type FirefoxProfile from 'firefox-profile';
import type {FirefoxPreferences} from '../firefox/preferences';
import type {OnSourceChangeFn} from '../watcher';
import type Watchpack from 'watchpack';
import type {
Expand Down Expand Up @@ -151,6 +152,7 @@ export type CmdRunParams = {
firefoxProfile: string,
preInstall: boolean,
noReload: boolean,
customPrefs?: FirefoxPreferences,
};

export type CmdRunOptions = {
Expand All @@ -163,6 +165,7 @@ export default async function run(
{
sourceDir, artifactsDir, firefox, firefoxProfile,
preInstall = false, noReload = false,
customPrefs,
}: CmdRunParams,
{
firefoxApp = defaultFirefoxApp,
Expand Down Expand Up @@ -195,6 +198,7 @@ export default async function run(
firefox,
manifestData,
profilePath: firefoxProfile,
customPrefs,
});

profile = await runner.getProfile();
Expand Down Expand Up @@ -265,6 +269,7 @@ export type ExtensionRunnerParams = {
profilePath: string,
firefoxApp: typeof defaultFirefoxApp,
firefox: string,
customPrefs?: FirefoxPreferences,
};

export class ExtensionRunner {
Expand All @@ -273,29 +278,31 @@ export class ExtensionRunner {
profilePath: string;
firefoxApp: typeof defaultFirefoxApp;
firefox: string;
customPrefs: FirefoxPreferences;

constructor(
{
firefoxApp, sourceDir, manifestData,
profilePath, firefox,
profilePath, firefox, customPrefs = {},
}: ExtensionRunnerParams
) {
this.sourceDir = sourceDir;
this.manifestData = manifestData;
this.profilePath = profilePath;
this.firefoxApp = firefoxApp;
this.firefox = firefox;
this.customPrefs = customPrefs;
}

getProfile(): Promise<FirefoxProfile> {
const {firefoxApp, profilePath} = this;
const {firefoxApp, profilePath, customPrefs} = this;
return new Promise((resolve) => {
if (profilePath) {
log.debug(`Copying Firefox profile from ${profilePath}`);
resolve(firefoxApp.copyProfile(profilePath));
resolve(firefoxApp.copyProfile(profilePath, {customPrefs}));
} else {
log.debug('Creating new Firefox profile');
resolve(firefoxApp.createProfile());
resolve(firefoxApp.createProfile({customPrefs}));
}
});
}
Expand Down
35 changes: 25 additions & 10 deletions src/firefox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import type {
} from './preferences';

import type {ExtensionManifest} from '../util/manifest';
import type {FirefoxPreferences} from '../firefox/preferences';


// defaultRemotePortFinder types and implementation.
Expand Down Expand Up @@ -190,6 +191,7 @@ export async function run(
export type ConfigureProfileOptions = {
app?: PreferencesAppName,
getPrefs?: PreferencesGetterFn,
customPrefs?: FirefoxPreferences,
};

export type ConfigureProfileFn = (
Expand All @@ -208,16 +210,22 @@ export function configureProfile(
{
app = 'firefox',
getPrefs = defaultPrefGetter,
}: ConfigureProfileOptions = {}
customPrefs = {},
}: ConfigureProfileOptions = {},
): Promise<FirefoxProfile> {
// Set default preferences. Some of these are required for the add-on to
// operate, such as disabling signatures.
// TODO: support custom preferences.
// https://github.com/mozilla/web-ext/issues/88
let prefs = getPrefs(app);
Object.keys(prefs).forEach((pref) => {
profile.setPreference(pref, prefs[pref]);
});
if (Object.keys(customPrefs).length > 0) {
const customPrefsStr = JSON.stringify(customPrefs, null, 2);
log.info(`Setting custom Firefox preferences: ${customPrefsStr}`);
Object.keys(customPrefs).forEach((custom) => {
profile.setPreference(custom, customPrefs[custom]);
});
}
profile.updatePreferences();
return Promise.resolve(profile);
}
Expand All @@ -228,6 +236,7 @@ export function configureProfile(
export type CreateProfileParams = {
app?: PreferencesAppName,
configureThisProfile?: ConfigureProfileFn,
customPrefs?: FirefoxPreferences,
};

/*
Expand All @@ -236,19 +245,24 @@ export type CreateProfileParams = {
* The profile will be deleted when the system process exits.
*/
export async function createProfile(
{app, configureThisProfile = configureProfile}: CreateProfileParams = {}
{
app,
configureThisProfile = configureProfile,
customPrefs = {},
}: CreateProfileParams = {},
): Promise<FirefoxProfile> {
const profile = new FirefoxProfile();
return await configureThisProfile(profile, {app});
return await configureThisProfile(profile, {app, customPrefs});
}


// copyProfile types and implementation.

export type CopyProfileOptions = {
app?: PreferencesAppName,
copyFromUserProfile?: Function,
configureThisProfile?: ConfigureProfileFn,
copyFromUserProfile?: Function,
customPrefs?: FirefoxPreferences,
};

/*
Expand All @@ -266,10 +280,11 @@ export type CopyProfileOptions = {
export async function copyProfile(
profileDirectory: string,
{
copyFromUserProfile = defaultUserProfileCopier,
configureThisProfile = configureProfile,
app,
Copy link
Member

Choose a reason for hiding this comment

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

oh thanks! it is better if we order the properties by name, it is unrelated with the goal of this PR but, given that we are here, do you mind if we also move the configureThisProfile before copyFromUserProfile so that they are alphabetically ordered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I sorted them in function definition as well for consistency, will apply the same order in tests.

}: CopyProfileOptions = {}
configureThisProfile = configureProfile,
copyFromUserProfile = defaultUserProfileCopier,
customPrefs = {},
}: CopyProfileOptions = {},
): Promise<FirefoxProfile> {

let copy = promisify(FirefoxProfile.copy);
Expand All @@ -288,7 +303,7 @@ export async function copyProfile(
profile = await copyByName({name: profileDirectory});
}

return configureThisProfile(profile, {app});
return configureThisProfile(profile, {app, customPrefs});
} catch (error) {
throw new WebExtError(
`Could not copy Firefox profile from ${profileDirectory}: ${error}`);
Expand Down
40 changes: 39 additions & 1 deletion src/firefox/preferences.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
/* @flow */
import {WebExtError} from '../errors';
import {WebExtError, UsageError} from '../errors';
import {createLogger} from '../util/logger';

const log = createLogger(__filename);
export const nonOverridablePreferences = [
'devtools.debugger.remote-enabled', 'devtools.debugger.prompt-connection',
'xpinstall.signatures.required',
];

// Flow Types

Expand Down Expand Up @@ -117,3 +123,35 @@ export function getPrefs(
...appPrefs,
};
}

export function coerceCLICustomPreference(
Copy link
Member

Choose a reason for hiding this comment

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

To make flow happy about the type checking of this function, it needs a small rewrite:

export function coerceCLICustomPreference(
  cliPrefs: string | Array<string>
): FirefoxPreferences {
  let customPrefs = {};

  if (!Array.isArray(cliPrefs)) {
    cliPrefs = [cliPrefs];
  }

  for (let pref of cliPrefs) {
    let [key, value] = pref.split('=');
    // Check the property key.
    if (/[^\w{@}.-]/.test(key)) {
      throw new UsageError(`Invalid custom preference name: ${key}`);
    }

    if (value === `${parseInt(value)}`) {
      value = parseInt(value, 10);
    } else if (value === 'true' || value === 'false') {
      value = (value === 'true');
    }

    customPrefs[`${key}`] = value;
  }

  return customPrefs;
}

The function esplicitly return a FirefoxPreferences type, and flow is able using its type inference capability that the for...of loop produce an object and its properties have the expected value types (boolean, number or string), which is the definition of the type FirefoxPreferences.

cliPrefs: string | Array<string>
): FirefoxPreferences {
let customPrefs = {};

if (!Array.isArray(cliPrefs)) {
cliPrefs = [cliPrefs];
}

for (let pref of cliPrefs) {
let [key, value] = pref.split('=');

if (/[^\w{@}.-]/.test(key)) {
throw new UsageError(`Invalid custom preference name: ${key}`);
}

if (value === `${parseInt(value)}`) {
value = parseInt(value, 10);
} else if (value === 'true' || value === 'false') {
value = (value === 'true');
}

if (nonOverridablePreferences.includes(key)) {
log.warn(`'${key}' preference cannot be customized.`);
continue;
}
customPrefs[`${key}`] = value;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it would be reasonable to skip (and maybe print a warning message) the custom properties that are used internally to make "web-ext run" able to work correctly, in particular the following:

  // Allow remote connections to the debugger.
  'devtools.debugger.remote-enabled': true,
  // Disable the prompt for allowing connections.
  'devtools.debugger.prompt-connection': false,
  // Allow unsigned add-ons.
 'xpinstall.signatures.required': false,

}

return customPrefs;
}
12 changes: 12 additions & 0 deletions src/program.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import yargs from 'yargs';
import defaultCommands from './cmd';
import {UsageError} from './errors';
import {createLogger, consoleStream as defaultLogStream} from './util/logger';
import {coerceCLICustomPreference} from './firefox/preferences';

const log = createLogger(__filename);
const envPrefix = 'WEB_EXT';
Expand Down Expand Up @@ -94,6 +95,9 @@ export class Program {
const argv = this.yargs.argv;
const cmd = argv._[0];

// Command line option (pref) renamed for internal use (customPref).
argv.customPrefs = argv.pref;
Copy link
Member

Choose a reason for hiding this comment

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

nit, this line can probably use a comment (something like, "Rename the simplified command line option name pref to the option name used internally.").


let runCommand = this.commands[cmd];

if (argv.verbose) {
Expand Down Expand Up @@ -286,6 +290,14 @@ Example: $0 --help run.
demand: false,
type: 'boolean',
},
'pref': {
describe: 'Launch firefox with custom preferences. Lightweight ' +
'alternative to creating custom profile.',
demand: false,
Copy link
Member

Choose a reason for hiding this comment

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

we should also add requiresArg: true here
(http://yargs.js.org/docs/#requiresArg).

requiresArg: true,
type: 'string',
coerce: coerceCLICustomPreference,
},
})
.command('lint', 'Validate the web extension source', commands.lint, {
'output': {
Expand Down
18 changes: 17 additions & 1 deletion tests/unit/test-firefox/test.firefox.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ describe('firefox', () => {
sinon.spy((profile) => Promise.resolve(profile));

return firefox.copyProfile(baseProfile.path(),
{configureThisProfile, app})
{app, configureThisProfile})
.then((profile) => {
assert.equal(configureThisProfile.called, true);
assert.equal(configureThisProfile.firstCall.args[0], profile);
Expand Down Expand Up @@ -345,6 +345,22 @@ describe('firefox', () => {
}
));

it('writes custom preferences', () => withTempProfile(
(profile) => {
const customPrefs = {'extensions.checkCompatibility.nightly': true};
return firefox.configureProfile(profile, {customPrefs})
.then((profile) => fs.readFile(path.join(profile.path(), 'user.js')))
.then((prefFile) => {
// Check for custom pref set by configureProfile().
assert.include(prefFile.toString(),
Copy link
Member

Choose a reason for hiding this comment

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

it would be probably nice to also check that one of the default preferences has been set in the prefFile when there are customPrefs.

'"extensions.checkCompatibility.nightly", true');
// Check that one of the default preferences is set as well
assert.include(prefFile.toString(),
'"devtools.debugger.remote-enabled", true');
Copy link
Member

Choose a reason for hiding this comment

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

👍 perfect!

});
}
));

});

describe('installExtension', () => {
Expand Down
56 changes: 54 additions & 2 deletions tests/unit/test-firefox/test.preferences.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
import {describe, it} from 'mocha';
import {assert} from 'chai';

import {WebExtError} from '../../../src/errors';
import {getPrefs} from '../../../src/firefox/preferences';
import {WebExtError, UsageError} from '../../../src/errors';
import {
getPrefs, coerceCLICustomPreference, nonOverridablePreferences,
} from '../../../src/firefox/preferences';


describe('firefox/preferences', () => {
Expand Down Expand Up @@ -34,4 +36,54 @@ describe('firefox/preferences', () => {

});

describe('coerceCLICustomPreference', () => {

it('converts a single --pref cli option from string to object', () => {
const prefs = coerceCLICustomPreference('valid.preference=true');
assert.isObject(prefs);
assert.equal(prefs['valid.preference'], true);
});

it('converts array of --pref cli option values into object', () => {
const prefs = coerceCLICustomPreference([
'valid.preference=true', 'valid.preference2=false',
]);
assert.isObject(prefs);
assert.equal(prefs['valid.preference'], true);
assert.equal(prefs['valid.preference2'], false);
});

it('converts boolean values', () => {
const prefs = coerceCLICustomPreference('valid.preference=true');
assert.equal(prefs['valid.preference'], true);
});

it('converts number values', () => {
Copy link
Member

Choose a reason for hiding this comment

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

we should also test that strings that contains floats converted into strings (e.g. 'valid.preference=45.5' is turned into {"valid.preference": "45.5"})

const prefs = coerceCLICustomPreference('valid.preference=455');
assert.equal(prefs['valid.preference'], 455);
});

it('converts float values', () => {
const prefs = coerceCLICustomPreference('valid.preference=4.55');
assert.equal(prefs['valid.preference'], '4.55');
});

it('does not allow certain default preferences to be customized', () => {
const nonChangeablePrefs = nonOverridablePreferences.map((prop) => {
return prop += '=true';
});
const prefs = coerceCLICustomPreference(nonChangeablePrefs);
for (let pref of nonChangeablePrefs) {
assert.isUndefined(prefs[pref], `${pref} should be undefined`);
}
});

it('throws an error for invalid preferences', () => {
assert.throws(() => coerceCLICustomPreference('*&%£=true'),
UsageError,
'UsageError: Invalid custom preference name: *&%£');
});

});

});
15 changes: 15 additions & 0 deletions tests/unit/test.program.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,21 @@ describe('program.main', () => {
});
});

it('converts custom preferences into an object', () => {
const fakeCommands = fake(commands, {
run: () => Promise.resolve(),
});
return execProgram(
['run', '--pref', 'prop=true', '--pref', 'prop2=value2'],
{commands: fakeCommands})
.then(() => {
const {customPrefs} = fakeCommands.run.firstCall.args[0];
assert.isObject(customPrefs);
assert.equal(customPrefs.prop, true);
Copy link
Member

Choose a reason for hiding this comment

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

we should also add an assert on the prop2 preference.

assert.equal(customPrefs.prop2, 'value2');
Copy link
Member

Choose a reason for hiding this comment

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

👍

});
});

});


Expand Down