Bug 1013064 (part 5) - stop disabling the password engine when MP enabled. r=ttaubert
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 12 Jun 2014 18:19:49 +1000
changeset 197957 0ce8cf754d327957015d9f9cc6a8bd033d7c8f28
parent 197956 da30483ac566e29299ecffab2f7db8c4dfcd3e65
child 197958 c3192146ea941319b335ec62ef377827ba8096bc
push id27256
push userkwierso@gmail.com
push dateWed, 06 Aug 2014 00:06:20 +0000
treeherdermozilla-central@6cbdd4d523a7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert
bugs1013064
milestone34.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1013064 (part 5) - stop disabling the password engine when MP enabled. r=ttaubert From 07aa9cc1fcd5479976effe29f6adf5ad5ba7a8f8 Mon Sep 17 00:00:00 2001
browser/base/content/sync/customize.js
browser/base/content/sync/customize.xul
browser/base/content/sync/utils.js
browser/components/preferences/in-content/sync.js
browser/components/preferences/in-content/sync.xul
browser/components/preferences/sync.js
browser/components/preferences/sync.xul
browser/themes/linux/preferences/preferences.css
browser/themes/osx/preferences/preferences.css
browser/themes/windows/preferences/preferences.css
services/sync/Weave.js
services/sync/modules/engines/passwords.js
services/sync/tests/unit/test_password_mpenabled.js
services/sync/tests/unit/xpcshell.ini
--- a/browser/base/content/sync/customize.js
+++ b/browser/base/content/sync/customize.js
@@ -1,25 +1,11 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
-const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
-
-Cu.import("resource://gre/modules/Services.jsm");
-
-let service = Cc["@mozilla.org/weave/service;1"]
-                .getService(Ci.nsISupports)
-                .wrappedJSObject;
-
-if (!service.allowPasswordsEngine) {
-  let checkbox = document.getElementById("fxa-pweng-chk");
-  checkbox.checked = false;
-  checkbox.disabled = true;
-}
-
 addEventListener("dialogaccept", function () {
   let pane = document.getElementById("sync-customize-pane");
   pane.writePreferences(true);
   window.arguments[0].accepted = true;
 });
--- a/browser/base/content/sync/customize.xul
+++ b/browser/base/content/sync/customize.xul
@@ -40,18 +40,17 @@
 
   <vbox align="start">
       <checkbox label="&engine.tabs.label;"
                 accesskey="&engine.tabs.accesskey;"
                 preference="engine.tabs"/>
       <checkbox label="&engine.bookmarks.label;"
                 accesskey="&engine.bookmarks.accesskey;"
                 preference="engine.bookmarks"/>
-      <checkbox id="fxa-pweng-chk"
-                label="&engine.passwords.label;"
+      <checkbox label="&engine.passwords.label;"
                 accesskey="&engine.passwords.accesskey;"
                 preference="engine.passwords"/>
       <checkbox label="&engine.history.label;"
                 accesskey="&engine.history.accesskey;"
                 preference="engine.history"/>
       <checkbox label="&engine.addons.label;"
                 accesskey="&engine.addons.accesskey;"
                 preference="engine.addons"/>
--- a/browser/base/content/sync/utils.js
+++ b/browser/base/content/sync/utils.js
@@ -82,22 +82,16 @@ let gSyncUtils = {
     this._openLink(Weave.Svc.Prefs.get(root + "termsURL"));
   },
 
   openPrivacyPolicy: function () {
     let root = this.fxAccountsEnabled ? "fxa." : "";
     this._openLink(Weave.Svc.Prefs.get(root + "privacyURL"));
   },
 
-  openMPInfoPage: function (event) {
-    event.stopPropagation();
-    let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
-    this._openLink(baseURL + "sync-master-password");
-  },
-
   openFirstSyncProgressPage: function () {
     this._openLink("about:sync-progress");
   },
 
   /**
    * Prepare an invisible iframe with the passphrase backup document.
    * Used by both the print and saving methods.
    *
--- a/browser/components/preferences/in-content/sync.js
+++ b/browser/components/preferences/in-content/sync.js
@@ -149,27 +149,16 @@ let gSyncPane = {
         document.getElementById("fxaEmailAddress1").textContent = data.email;
         document.getElementById("fxaEmailAddress2").textContent = data.email;
         document.getElementById("fxaEmailAddress3").textContent = data.email;
         document.getElementById("fxaSyncComputerName").value = Weave.Service.clientsEngine.localName;
         let engines = document.getElementById("fxaSyncEngines")
         for (let checkbox of engines.querySelectorAll("checkbox")) {
           checkbox.disabled = enginesListDisabled;
         }
-
-        let checkbox = document.getElementById("fxa-pweng-chk");
-        let help = document.getElementById("fxa-pweng-help");
-        let allowPasswordsEngine = service.allowPasswordsEngine;
-
-        if (!allowPasswordsEngine) {
-          checkbox.checked = false;
-        }
-
-        checkbox.disabled = !allowPasswordsEngine || enginesListDisabled;
-        help.hidden = allowPasswordsEngine || enginesListDisabled;
       });
     // If fxAccountEnabled is false and we are in a "not configured" state,
     // then fxAccounts is probably fully disabled rather than just unconfigured,
     // so handle this case.  This block can be removed once we remove support
     // for fxAccounts being disabled.
     } else if (Weave.Status.service == Weave.CLIENT_NOT_CONFIGURED ||
                Weave.Svc.Prefs.get("firstSync", "") == "notReady") {
       this.page = PAGE_NO_ACCOUNT;
--- a/browser/components/preferences/in-content/sync.xul
+++ b/browser/components/preferences/in-content/sync.xul
@@ -271,30 +271,19 @@
       <hbox id="fxaSyncEngines">
         <vbox align="start">
           <checkbox label="&engine.tabs.label;"
                     accesskey="&engine.tabs.accesskey;"
                     preference="engine.tabs"/>
           <checkbox label="&engine.bookmarks.label;"
                     accesskey="&engine.bookmarks.accesskey;"
                     preference="engine.bookmarks"/>
-          <hbox>
-            <checkbox id="fxa-pweng-chk"
-                      label="&engine.passwords.label;"
-                      accesskey="&engine.passwords.accesskey;"
-                      preference="engine.passwords"/>
-
-            <vbox id="fxa-pweng-help">
-              <spacer flex="1"/>
-              <hbox id="fxa-pweng-help-link">
-                <image onclick="gSyncUtils.openMPInfoPage(event);" />
-              </hbox>
-              <spacer flex="1"/>
-            </vbox>
-          </hbox>
+          <checkbox label="&engine.passwords.label;"
+                    accesskey="&engine.passwords.accesskey;"
+                    preference="engine.passwords"/>
           <checkbox label="&engine.history.label;"
                     accesskey="&engine.history.accesskey;"
                     preference="engine.history"/>
           <checkbox label="&engine.addons.label;"
                     accesskey="&engine.addons.accesskey;"
                     preference="engine.addons"/>
           <checkbox label="&engine.prefs.label;"
                     accesskey="&engine.prefs.accesskey;"
--- a/browser/components/preferences/sync.js
+++ b/browser/components/preferences/sync.js
@@ -149,27 +149,16 @@ let gSyncPane = {
         document.getElementById("fxaEmailAddress1").textContent = data.email;
         document.getElementById("fxaEmailAddress2").textContent = data.email;
         document.getElementById("fxaEmailAddress3").textContent = data.email;
         document.getElementById("fxaSyncComputerName").value = Weave.Service.clientsEngine.localName;
         let engines = document.getElementById("fxaSyncEngines")
         for (let checkbox of engines.querySelectorAll("checkbox")) {
           checkbox.disabled = enginesListDisabled;
         }
-
-        let checkbox = document.getElementById("fxa-pweng-chk");
-        let help = document.getElementById("fxa-pweng-help");
-        let allowPasswordsEngine = service.allowPasswordsEngine;
-
-        if (!allowPasswordsEngine) {
-          checkbox.checked = false;
-        }
-
-        checkbox.disabled = !allowPasswordsEngine || enginesListDisabled;
-        help.hidden = allowPasswordsEngine || enginesListDisabled;
       });
     // If fxAccountEnabled is false and we are in a "not configured" state,
     // then fxAccounts is probably fully disabled rather than just unconfigured,
     // so handle this case.  This block can be removed once we remove support
     // for fxAccounts being disabled.
     } else if (Weave.Status.service == Weave.CLIENT_NOT_CONFIGURED ||
                Weave.Svc.Prefs.get("firstSync", "") == "notReady") {
       this.page = PAGE_NO_ACCOUNT;
--- a/browser/components/preferences/sync.xul
+++ b/browser/components/preferences/sync.xul
@@ -260,30 +260,19 @@
             <hbox id="fxaSyncEngines">
               <vbox>
                 <checkbox label="&engine.tabs.label;"
                           accesskey="&engine.tabs.accesskey;"
                           preference="engine.tabs"/>
                 <checkbox label="&engine.bookmarks.label;"
                           accesskey="&engine.bookmarks.accesskey;"
                           preference="engine.bookmarks"/>
-                <hbox>
-                  <checkbox id="fxa-pweng-chk"
-                            label="&engine.passwords.label;"
-                            accesskey="&engine.passwords.accesskey;"
-                            preference="engine.passwords"/>
-
-                  <vbox id="fxa-pweng-help">
-                    <spacer flex="1"/>
-                    <hbox id="fxa-pweng-help-link">
-                      <image onclick="gSyncUtils.openMPInfoPage(event);" />
-                    </hbox>
-                    <spacer flex="1"/>
-                  </vbox>
-                </hbox>
+                <checkbox label="&engine.passwords.label;"
+                          accesskey="&engine.passwords.accesskey;"
+                          preference="engine.passwords"/>
                 <checkbox label="&engine.history.label;"
                           accesskey="&engine.history.accesskey;"
                           preference="engine.history"/>
                 <checkbox label="&engine.addons.label;"
                           accesskey="&engine.addons.accesskey;"
                           preference="engine.addons"/>
                 <checkbox label="&engine.prefs.label;"
                           accesskey="&engine.prefs.accesskey;"
--- a/browser/themes/linux/preferences/preferences.css
+++ b/browser/themes/linux/preferences/preferences.css
@@ -166,17 +166,9 @@ label.small {
   margin: 5px;
   line-height: 1.2em;
 }
 
 #noFxaAccount > label:first-child {
   margin-bottom: 0.6em;
 }
 
-#fxa-pweng-help-link > label {
-  margin: 0;
-}
-
-#fxa-pweng-help-link > image {
-  list-style-image: url("chrome://global/skin/icons/question-16.png");
-}
-
 %endif
--- a/browser/themes/osx/preferences/preferences.css
+++ b/browser/themes/osx/preferences/preferences.css
@@ -228,25 +228,9 @@ html|a.inline-link:-moz-focusring {
   margin: 12px 4px;
   line-height: 1.2em;
 }
 
 #noFxaAccount > label:first-child {
   margin-bottom: 0.6em;
 }
 
-#fxa-pweng-help-link > label {
-  margin: 0;
-}
-
-#fxa-pweng-help-link > image {
-  width: 16px;
-  height: 16px;
-  list-style-image: url("chrome://global/skin/icons/question-16.png");
-}
-
-@media (min-resolution: 2dppx) {
-  #fxa-pweng-help-link > image {
-    list-style-image: url("chrome://global/skin/icons/question-32.png");
-  }
-}
-
 %endif
--- a/browser/themes/windows/preferences/preferences.css
+++ b/browser/themes/windows/preferences/preferences.css
@@ -156,17 +156,9 @@ label.small {
   margin: 6px;
   line-height: 1.2em;
 }
 
 #noFxaAccount > label:first-child {
   margin-bottom: 0.6em;
 }
 
-#fxa-pweng-help-link > label {
-  margin: 0;
-}
-
-#fxa-pweng-help-link > image {
-  list-style-image: url("chrome://global/skin/icons/question-16.png");
-}
-
 %endif
--- a/services/sync/Weave.js
+++ b/services/sync/Weave.js
@@ -104,26 +104,16 @@ WeaveService.prototype = {
       let username = Services.prefs.getCharPref(SYNC_PREFS_BRANCH + "username");
       return !username || username.contains('@');
     } catch (_) {
       return true; // No username == only allow FxA to be configured.
     }
   },
 
   /**
-   * Returns whether the password engine is allowed. We explicitly disallow
-   * the password engine when a master password is used to ensure those can't
-   * be accessed without the master key.
-   */
-  get allowPasswordsEngine() {
-    // This doesn't apply to old-style sync, it's only an issue for FxA.
-    return !this.fxAccountsEnabled || !Utils.mpEnabled();
-  },
-
-  /**
    * Whether Sync appears to be enabled.
    *
    * This returns true if all the Sync preferences for storing account
    * and server configuration are populated.
    *
    * It does *not* perform a robust check to see if the client is working.
    * For that, you'll want to check Weave.Status.checkSetup().
    */
--- a/services/sync/modules/engines/passwords.js
+++ b/services/sync/modules/engines/passwords.js
@@ -31,38 +31,16 @@ this.PasswordEngine = function PasswordE
 }
 PasswordEngine.prototype = {
   __proto__: SyncEngine.prototype,
   _storeObj: PasswordStore,
   _trackerObj: PasswordTracker,
   _recordObj: LoginRec,
   applyIncomingBatchSize: PASSWORDS_STORE_BATCH_SIZE,
 
-  get isAllowed() {
-    return Cc["@mozilla.org/weave/service;1"]
-             .getService(Ci.nsISupports)
-             .wrappedJSObject
-             .allowPasswordsEngine;
-  },
-
-  get enabled() {
-    // If we are disabled due to !isAllowed(), we must take care to ensure the
-    // engine has actually had the enabled setter called which reflects this state.
-    let prefVal = SyncEngine.prototype.__lookupGetter__("enabled").call(this);
-    let newVal = this.isAllowed && prefVal;
-    if (newVal != prefVal) {
-      this.enabled = newVal;
-    }
-    return newVal;
-  },
-
-  set enabled(val) {
-    SyncEngine.prototype.__lookupSetter__("enabled").call(this, this.isAllowed && val);
-  },
-
   _syncFinish: function _syncFinish() {
     SyncEngine.prototype._syncFinish.call(this);
 
     // Delete the weave credentials from the server once
     if (!Svc.Prefs.get("deletePwdFxA", false)) {
       try {
         let ids = [];
         for (let host of Utils.getSyncCredentialsHosts()) {
deleted file mode 100644
--- a/services/sync/tests/unit/test_password_mpenabled.js
+++ /dev/null
@@ -1,137 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-Cu.import("resource://gre/modules/Log.jsm");
-Cu.import("resource://services-sync/stages/enginesync.js");
-Cu.import("resource://services-sync/util.js");
-Cu.import("resource://services-sync/engines/passwords.js");
-Cu.import("resource://services-sync/service.js");
-Cu.import("resource://testing-common/services/sync/utils.js");
-
-function run_test() {
-  initTestLogging("Trace");
-  run_next_test();
-}
-
-add_test(function test_simple() {
-  ensureLegacyIdentityManager();
-  // Stub fxAccountsEnabled
-  let xpcs = Cc["@mozilla.org/weave/service;1"]
-             .getService(Components.interfaces.nsISupports)
-             .wrappedJSObject;
-  let fxaEnabledGetter = xpcs.__lookupGetter__("fxAccountsEnabled");
-  xpcs.__defineGetter__("fxAccountsEnabled", () => true);
-
-  // Stub mpEnabled.
-  let mpEnabledF = Utils.mpEnabled;
-  let mpEnabled = false;
-  Utils.mpEnabled = function() mpEnabled;
-
-  let manager = Service.engineManager;
-
-  Service.engineManager.register(PasswordEngine);
-  let engine = Service.engineManager.get("passwords");
-  let wipeCount = 0;
-  let engineWipeServerF = engine.wipeServer;
-  engine.wipeServer = function() {
-    ++wipeCount;
-  }
-
-  // A server for the metadata.
-  let server  = new SyncServer();
-  let johndoe = server.registerUser("johndoe", "password");
-  johndoe.createContents({
-    meta: {global: {engines: {passwords: {version: engine.version,
-                                          syncID: engine.syncID}}}},
-    crypto: {},
-    clients: {}
-  });
-  server.start();
-  setBasicCredentials("johndoe", "password", "abcdeabcdeabcdeabcdeabcdea");
-  Service.serverURL = server.baseURI;
-  Service.clusterURL = server.baseURI;
-
-  let engineSync = new EngineSynchronizer(Service);
-  engineSync._log.level = Log.Level.Trace;
-
-  function assertEnabled(expected, message) {
-    Assert.strictEqual(engine.enabled, expected, message);
-    // The preference *must* reflect the actual state.
-    Assert.strictEqual(Svc.Prefs.get("engine." + engine.prefName), expected,
-                       message + " (pref should match enabled state)");
-  }
-
-  try {
-    assertEnabled(true, "password engine should be enabled by default")
-    let engineMeta = Service.recordManager.get(engine.metaURL);
-    // This engine should be in the meta/global
-    Assert.notStrictEqual(engineMeta.payload.engines[engine.name], undefined,
-                          "The engine should appear in the metadata");
-    Assert.ok(!engineMeta.changed, "the metadata for the password engine hasn't changed");
-
-    // (pretend to) enable a master-password
-    mpEnabled = true;
-    // The password engine should be locally disabled...
-    assertEnabled(false, "if mp is locked the engine should be disabled");
-    // ...but not declined.
-    Assert.ok(!manager.isDeclined("passwords"), "password engine is not declined");
-    // Next time a sync would happen, we call _updateEnabledEngines(), which
-    // would remove the engine from the metadata - call that now.
-    engineSync._updateEnabledEngines();
-    // The global meta should no longer list the engine.
-    engineMeta = Service.recordManager.get(engine.metaURL);
-    Assert.strictEqual(engineMeta.payload.engines[engine.name], undefined,
-                       "The engine should have vanished");
-    // And we should have wiped the server data.
-    Assert.strictEqual(wipeCount, 1, "wipeServer should have been called");
-
-    // Now simulate an incoming meta/global indicating the engine should be
-    // enabled.  We should fail to actually enable it - the pref should remain
-    // false and we wipe the server for anything another device might have
-    // stored.
-    let meta = {
-      payload: {
-        engines: {
-          "passwords": {"version":1,"syncID":"yfBi2v7PpFO2"},
-        },
-      },
-    };
-    engineSync._updateEnabledFromMeta(meta, 3, manager);
-    Assert.strictEqual(wipeCount, 2, "wipeServer should have been called");
-    Assert.ok(!manager.isDeclined("passwords"), "password engine is not declined");
-    assertEnabled(false, "engine still not enabled locally");
-
-    // Let's turn the MP off - but *not* re-enable it locally.
-    mpEnabled = false;
-    // Just disabling the MP isn't enough to force it back to enabled.
-    assertEnabled(false, "engine still not enabled locally");
-    // Another incoming metadata record with the engine enabled should cause
-    // it to be enabled locally.
-    meta = {
-      payload: {
-        engines: {
-          "passwords": 1,
-        },
-      },
-    };
-    engineSync._updateEnabledFromMeta(meta, 3, manager);
-    Assert.strictEqual(wipeCount, 2, "wipeServer should *not* have been called again");
-    Assert.ok(!manager.isDeclined("passwords"), "password engine is not declined");
-    // It should be enabled locally.
-    assertEnabled(true, "engine now enabled locally");
-    // Next time a sync starts it should magically re-appear in our meta/global
-    engine._syncStartup();
-    //engineSync._updateEnabledEngines();
-    engineMeta = Service.recordManager.get(engine.metaURL);
-    Assert.equal(engineMeta.payload.engines[engine.name].version, engine.version,
-                 "The engine should re-appear in the metadata");
-  } finally {
-    // restore the damage we did above...
-    engine.wipeServer = engineWipeServerF;
-    engine._store.wipe();
-    // Un-stub mpEnabled and fxAccountsEnabled
-    Utils.mpEnabled = mpEnabledF;
-    xpcs.__defineGetter__("fxAccountsEnabled", fxaEnabledGetter);
-    server.stop(run_next_test);
-  }
-});
--- a/services/sync/tests/unit/xpcshell.ini
+++ b/services/sync/tests/unit/xpcshell.ini
@@ -164,10 +164,8 @@ skip-if = debug
 [test_prefs_store.js]
 [test_prefs_tracker.js]
 [test_tab_engine.js]
 [test_tab_store.js]
 [test_tab_tracker.js]
 
 [test_healthreport.js]
 skip-if = ! healthreport
-
-[test_password_mpenabled.js]