Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

WIP: Export settings #11639

Closed
wants to merge 3 commits into from
Closed

WIP: Export settings #11639

wants to merge 3 commits into from

Conversation

Hainish
Copy link
Member

@Hainish Hainish commented Aug 3, 2017

Partially resolves #10007.

Note: This PR does not include the import code, which I am still working on.

This allows users to export their HTTPS Everywhere settings to a json file. The json file includes five top-level keys:

  1. prefs: Two high-level preferences - whether the extension is turned on at all, and whether 2. Block all unencrypted requests is enabled.
  2. rule_toggle: This is an object of rulesets, keyed by ruleset name, the value for each being either true or false depending on if the user enabled or disabled the ruleset. If a ruleset has a default value, it is not included in this object.
  3. custom_rulesets: A list of the full text of all custom rulesets in the HTTPSEverywhereCustomUserRules profile path.
  4. changed: Whether this user has changed default settings at all. Not strictly needed for the export, but will be useful come time for imports.

The exported json file is readily understandable and I encourage any reviewer to look at the export to ensure it matches expectations.

There are two ways to export this file.

  1. The PR adds a menu option, Export Settings, which opens a dialogue to save the settings.
  2. The PR adds a new resource, opened in a tab, which warns users of the fact that they are about to lose their settings. This tab is opened once, and a pref is set so that it will not be opened again. Once a user clicks "OK" in the tab, they will be prompted to save the settings file.

This adds several strings that should be translated into our supported languages quickly. I request that any reviewer look at the text of these strings carefully first. Transifex needs these translations commited into master before translations can begin, so it is a priority to get the English version into master ASAP, even if the underlying functionality changes. This need for expediency explains why some of the strings to be translated are not present in the extension yet, and only expected to appear once the import functionality is complete.

When testing, you may want to test if the popup persists. In order to do so, you can create a persisting Firefox profile directory for testing. Run mktemp -d. This generates a random temporary directory, which you can apply with a patch:

diff --git a/test/firefox.sh b/test/firefox.sh
index 0ff398c..5fccc6c 100755
--- a/test/firefox.sh
+++ b/test/firefox.sh
@@ -18,8 +18,8 @@ source utils/mktemp.sh
 TEST_ADDON_PATH=./test/firefox/
 
 # We'll create a Firefox profile here and install HTTPS Everywhere into it.
-PROFILE_DIRECTORY="$(mktemp -d)"
-trap 'rm -r "$PROFILE_DIRECTORY"' EXIT
+PROFILE_DIRECTORY="/tmp/tmp.7CPWfjEIb6"
+#trap 'rm -r "$PROFILE_DIRECTORY"' EXIT
 HTTPSE_INSTALL_DIRECTORY=$PROFILE_DIRECTORY/extensions/https-everywhere-eff@eff.org
 
 # Build the XPI to run all the validations in makexpi.sh, and to ensure that
diff --git a/test/firefox/test_profile_skeleton/prefs.js b/test/firefox/test_profile_skeleton/prefs.js
index 42b3f0a..07110f0 100644
--- a/test/firefox/test_profile_skeleton/prefs.js
+++ b/test/firefox/test_profile_skeleton/prefs.js
@@ -4,7 +4,7 @@ user_pref("extensions.https_everywhere._observatory.enabled", true);
 user_pref("extensions.https_everywhere._observatory.popup_shown", true);
 user_pref("extensions.https_everywhere.toolbar_hint_shown", true);
 // Show all logs.
-user_pref("extensions.https_everywhere.LogLevel", 0);
+user_pref("extensions.https_everywhere.LogLevel", 5);
 user_pref("extensions.https_everywhere.log_to_stdout", true);
 // Make it quicker to make manual config changes.
 user_pref("general.warnOnAboutConfig", false);

This way you can run ./test/firefox.sh --justrun for the first run, and firefox -profile /tmp/tmp.7CPWfjEIb6/ for the second run.

@Hainish
Copy link
Member Author

Hainish commented Aug 3, 2017

The latest commit auto-saves settings in $PROFILE_DIRECTORY/https_everywhere_settings_autosave.json. I still think it's useful to have the export prompt, so that users will know where they manually saved the settings file.

@jeremyn
Copy link
Contributor

jeremyn commented Aug 3, 2017

@Hainish I'm having trouble installing the add-on. I checked out your Hainish/export-settings branch and ran bash makexpi.sh which generated the following output:

Generating ruleset DB
Validation of included rulesets completed.

Validation of rulesets against relaxng.xml succeeded.
Total included rules: 22827
Rules disabled by default: 6414
Created pkg/https-everywhere-5.2.21~eef0663-eff.xpi and pkg/https-everywhere-5.2.21~eef0663-amo.xpi

When I try to install https-everywhere-5.2.21~eef0663-eff.xpi into a new profile using about:addons > (gear) > Install Add-on From File..., I get an "add-on is corrupt" error. The md5sum is:

e3c0b60aaa8505f297301b5761d94476  https-everywhere-5.2.21~eef0663-eff.xpi

Any idea what's wrong?

@Hainish
Copy link
Member Author

Hainish commented Aug 3, 2017 via email

@jeremyn
Copy link
Contributor

jeremyn commented Aug 3, 2017

I got it installed in Firefox Developer Edition but it looks like the legacy/XPCOM version of the extension.

@Hainish
Copy link
Member Author

Hainish commented Aug 3, 2017

@jeremyn that's because it is the legacy extension. This PR is to export settings from the old extension, so that once the WebExtensions version is pushed users can then import those settings.

@Hainish
Copy link
Member Author

Hainish commented Aug 3, 2017

@jeremyn since the Export tab is only opened when settings are changed, you might want to try adding something to the HTTPSEverywhereUserRules path between the first and second run.

@jeremyn
Copy link
Contributor

jeremyn commented Aug 3, 2017

Okay, that explains why I'm seeing the XPCOM extension.

I disabled the Freerangekitten.com rule but I'm not getting any prompt to export settings, even if I restart Firefox.

@Hainish
Copy link
Member Author

Hainish commented Aug 3, 2017

@jeremyn what about when you block all unencrypted connections and restart?

@Hainish
Copy link
Member Author

Hainish commented Aug 3, 2017

I've finished the import code, it's available in the webextensions branch as the latest commit, "Allow import of json settings data".

@J0WI
Copy link
Contributor

J0WI commented Aug 4, 2017

I was not able to figure out why, but something went wrong here. I'm able to build and install the extension, but I can not interact with HTTPS-Everywhere. I have no Icon or menu entry.
But I verified that the rewrite of requests to https works.
No https_everywhere_settings_autosave.json has been created in my profile dir.

@Hainish
Copy link
Member Author

Hainish commented Aug 4, 2017

@J0WI this is concerning. It probably means that there was an unhandled exception. Can you navigate to browser console by typing ctrl-shift-j and tell me if anything particular stands out?

@Hainish
Copy link
Member Author

Hainish commented Aug 4, 2017

@J0WI also any details on what browser version you're using would be helpful.

@Hainish
Copy link
Member Author

Hainish commented Aug 4, 2017

Commit f384cfd on master adds the strings necessary for this PR. Doing a git pull origin https_everywhere within the translations path will pull in all the localized import/export strings.

@J0WI this may resolve the issue you've encountered.

@jsha
Copy link
Member

jsha commented Aug 4, 2017

Talked about this a bunch in person, and @Hainish and I came to the conclusion that we probably can use the Embedded WebExtension approach. It requires a bootstrapped (aka restartless) XPCOM extension, but that doesn't mean we need to rewrite all of HTTPS Everywhere to be bootstrapped; we just need to write a thin XPCOM wrapper capable of copying settings through the message-passing API. The Embedded WebExtension would just be the same WebExtension code we are planning to run, plus code to ask the wrapper for settings if it is available. There are some examples at https://github.com/mdn/webextensions-examples/tree/master/embedded-webextension-bootstrapped. @Hainish is going to take a stab at this approach.

The main benefits will be:

  1. Don't need to annoy users with prompts
  2. We start using the WebExtension code ASAP so it gets more live testing before the hard deadline in November
  3. We get a longer window during which settings can be copied

(3) is especially important because we are currently a restart-requiring extension, so nobody will get the new version until they restart their browser. People often go weeks or months without a browser restart. And some of our settings, in particular "Block all HTTP Requests" are security-sensitive, so we want to really minimize the occurrence of people getting upgraded to a new version and finding all their settings are reverted. A long migration period helps with that.

},

readFromString: function(data, rule_store, ruleset_id) {
readFromString: function(data, rule_store, ruleset_id, custom) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than parsing user rules before exporting them (soon to be: copying them via the message-passing API), I think it would be simpler to have the export function read the raw file contents, and send them across as a list of strings. That way the thin-shell bootstrapped XPCOM extension won't need to include all the HTTPSRules parsing code.

@J0WI
Copy link
Contributor

J0WI commented Aug 5, 2017

@Hainish I was now able to build the extension and install it on Firefox Nightly. The first export was successful, but extensions.https_everywhere.enable_mixed_rulesets has not been exported.

No https_everywhere_settings_autosave.json has been created in my profile dir.

After I played a bit with custom rulesets I was not able to export my settings anymore and got this error:

TypeError: HTTPSRules.rulesetsByName[rule_toggle_key] is undefined[Learn More]  https-everywhere.js:848:1
	exportSettings jar:file:///tmp/tmp.mozilla/extensions/https-everywhere-eff@eff.org.xpi!/components/https-everywhere.js:848:1
	exportSettingsToFile jar:file:///tmp/tmp.mozilla/extensions/https-everywhere-eff@eff.org.xpi!/components/https-everywhere.js:869:35
	exportSettingsToFile chrome://https-everywhere/content/toolbar_button.js:444:3
	oncommand chrome://browser/content/browser.xul:1:1

@jeremyn
Copy link
Contributor

jeremyn commented Aug 5, 2017

@Hainish #11639 (comment):

@jeremyn what about when you block all unencrypted connections and restart?

I enabled this setting, restarted the browser, and still did not get a prompt.

@jsha #11639 (comment): I don't understand the implications here. Are you saying that you want to replace the internals of the XPCOM extension with the new WebExtensions code, and migrate the settings from XPCOM to WebExtensions internally when the user upgrades to this WebExtensions-in-XPCOM version?

I think any process where settings are migrated should happen with or in addition to an export to file, so that the user's settings are safe. We shouldn't make it so that if something goes wrong, the user's settings are just totally gone.

@jsha
Copy link
Member

jsha commented Aug 5, 2017

Are you saying that you want to replace the internals of the XPCOM extension with the new WebExtensions code, and migrate the settings from XPCOM to WebExtensions internally when the user upgrades to this WebExtensions-in-XPCOM version?

Correct. The term Mozilla has been using for this technique is "Embedded WebExtension," and it's the most common / recommended solution for this type of settings migration. @Hainish had previously dismissed it because it looked like he would have to port all of the HTTPS Everywhere guts to a restartless version, which it turned out was not the case.

I think any process where settings are migrated should happen with or in addition to an export to file, so that the user's settings are safe. We shouldn't make it so that if something goes wrong, the user's settings are just totally gone.

The settings that reflect the most effort are user-defined rules, and those are already stored in a file and are totally safe. For the rule toggles and "block all HTTP" settings, they won't be deleted from preferences on migration. The WebExtensions code would just store a migratedVersion: 1 setting or something similar so it would know not to re-migrate. If we discovered a bug, we could release a subsequent version that would re-migrate anyhow in order to fix any issues.

@jeremyn
Copy link
Contributor

jeremyn commented Aug 6, 2017

@jsha Thanks for the explanation. For anyone else reading along, this documentation is useful too: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Embedded_WebExtensions#Migrating_data_from_legacy_add-ons

@jeremyn
Copy link
Contributor

jeremyn commented Aug 6, 2017

Also, for what it's worth, I agree the Embedded WebExtensions approach is the way to go, particularly because it seems to be the mechanism Mozilla wants add-on makers to use.

@Hainish Hainish mentioned this pull request Aug 7, 2017
@Hainish Hainish mentioned this pull request Aug 8, 2017
@Hainish
Copy link
Member Author

Hainish commented Aug 8, 2017

Superceded by #11760

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate settings to WebExtensions
4 participants