Bug 970167 - Disable passwords engine when a master password is set. r=rnewman, a=sledru
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 21 Mar 2014 14:22:02 +1100
changeset 191424 1b0d28323d93f7e0ee9b482411c5c295353483d2
parent 191423 e0cac690e8ae01f816bb0f536289ceb675e10d16
child 191425 a31b22a5e8a4bc09f1fe276af44f73ec7d5961ac
push id3503
push userraliiev@mozilla.com
push dateMon, 28 Apr 2014 18:51:11 +0000
treeherdermozilla-beta@c95ac01e332e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, sledru
bugs970167
milestone30.0a2
Bug 970167 - Disable passwords engine when a master password is set. r=rnewman, a=sledru
browser/base/content/sync/customize.js
browser/base/content/sync/customize.xul
browser/base/content/sync/utils.js
browser/components/preferences/security.js
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/modules/stages/enginesync.js
services/sync/modules/util.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,9 +1,23 @@
 /* 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 () {
   window.arguments[0].accepted = true;
 });
--- a/browser/base/content/sync/customize.xul
+++ b/browser/base/content/sync/customize.xul
@@ -39,17 +39,18 @@
                  />
 
     <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 label="&engine.passwords.label;"
+    <checkbox id="fxa-pweng-chk"
+              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,16 +82,22 @@ 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/security.js
+++ b/browser/components/preferences/security.js
@@ -168,16 +168,19 @@ var gSecurityPane = {
     // design), and it would be extremely odd to pop up that dialog when the
     // user closes the prefwindow and saves his settings
     if (!checkbox.checked)
       this._removeMasterPassword();
     else
       this.changeMasterPassword();
 
     this._initMasterPasswordUI();
+
+    // We might want to hide sync's password engine.
+    gSyncPane.updateWeavePrefs();
   },
 
   /**
    * Displays the "remove master password" dialog to allow the user to remove
    * the current master password.  When the dialog is dismissed, master password
    * UI is automatically updated.
    */
   _removeMasterPassword: function ()
--- a/browser/components/preferences/sync.js
+++ b/browser/components/preferences/sync.js
@@ -149,16 +149,27 @@ 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;
+        help.hidden = allowPasswordsEngine;
       });
     // 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
@@ -262,19 +262,33 @@
             <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"/>
-                <checkbox label="&engine.passwords.label;"
-                          accesskey="&engine.passwords.accesskey;"
-                          preference="engine.passwords"/>
+                <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">
+                      <label value=" ["/>
+                      <label class="text-link" value="?"
+                             onclick="gSyncUtils.openMPInfoPage(event);"/>
+                      <label value="]"/>
+                    </hbox>
+                    <spacer flex="1"/>
+                  </vbox>
+                </hbox>
                 <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
@@ -160,9 +160,13 @@ label.small {
   margin: 5px;
   line-height: 1.2em;
 }
 
 #noFxaAccount > label:first-child {
   margin-bottom: 0.6em;
 }
 
+#fxa-pweng-help-link > label {
+  margin: 0;
+}
+
 %endif
--- a/browser/themes/osx/preferences/preferences.css
+++ b/browser/themes/osx/preferences/preferences.css
@@ -225,9 +225,13 @@ 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;
+}
+
 %endif
--- a/browser/themes/windows/preferences/preferences.css
+++ b/browser/themes/windows/preferences/preferences.css
@@ -150,9 +150,13 @@ label.small {
   margin: 6px;
   line-height: 1.2em;
 }
 
 #noFxaAccount > label:first-child {
   margin-bottom: 0.6em;
 }
 
+#fxa-pweng-help-link > label {
+  margin: 0;
+}
+
 %endif
--- a/services/sync/Weave.js
+++ b/services/sync/Weave.js
@@ -113,16 +113,26 @@ WeaveService.prototype = {
     }
     // Currently we don't support toggling this pref after initialization -
     // except when sync is reset - but this 1 exception is enough that we can't
     // cache the value.
     return fxAccountsEnabled;
   },
 
   /**
+   * 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,16 +31,38 @@ 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()) {
--- a/services/sync/modules/stages/enginesync.js
+++ b/services/sync/modules/stages/enginesync.js
@@ -249,35 +249,47 @@ EngineSynchronizer.prototype = {
         continue;
       }
       let engine = engineManager.get(engineName);
       if (!engine) {
         // The engine doesn't exist locally. Nothing to do.
         continue;
       }
 
-      if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
-        // The engine was disabled locally. Wipe server data and
-        // disable it everywhere.
-        this._log.trace("Wiping data for " + engineName + " engine.");
-        engine.wipeServer();
-        delete meta.payload.engines[engineName];
-        meta.changed = true;             // TODO: Should we still do this?
-
-        // We also here mark the engine as declined, because the pref
-        // was explicitly changed to false.
-        // This will be reflected in meta/global in the next stage.
-        this._log.trace("Engine " + engineName + " was disabled locally. Marking as declined.");
-        toDecline.add(engineName);
-      } else {
-        // The engine was enabled remotely. Enable it locally.
+      let attemptedEnable = false;
+      // If the engine was enabled remotely, enable it locally.
+      if (!Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
         this._log.trace("Engine " + engineName + " was enabled. Marking as non-declined.");
         toUndecline.add(engineName);
         this._log.trace(engineName + " engine was enabled remotely.");
         engine.enabled = true;
+        // Note that setting engine.enabled to true might not have worked for
+        // the password engine if a master-password is enabled.  However, it's
+        // still OK that we added it to undeclined - the user *tried* to enable
+        // it remotely - so it still winds up as not being flagged as declined
+        // even though it's disabled remotely.
+        attemptedEnable = true;
+      }
+
+      // If either the engine was disabled locally or enabling the engine
+      // failed (see above re master-password) then wipe server data and
+      // disable it everywhere.
+      if (!engine.enabled) {
+        this._log.trace("Wiping data for " + engineName + " engine.");
+        engine.wipeServer();
+        delete meta.payload.engines[engineName];
+        meta.changed = true; // the new enabled state must propagate
+        // We also here mark the engine as declined, because the pref
+        // was explicitly changed to false - unless we tried, and failed,
+        // to enable it - in which case we leave the declined state alone.
+        if (!attemptedEnable) {
+          // This will be reflected in meta/global in the next stage.
+          this._log.trace("Engine " + engineName + " was disabled locally. Marking as declined.");
+          toDecline.add(engineName);
+        }
       }
     }
 
     // Any remaining engines were either enabled locally or disabled remotely.
     for each (let engineName in enabled) {
       let engine = engineManager.get(engineName);
       if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
         this._log.trace("The " + engineName + " engine was enabled locally.");
@@ -285,18 +297,18 @@ EngineSynchronizer.prototype = {
       } else {
         this._log.trace("The " + engineName + " engine was disabled remotely.");
 
         // Don't automatically mark it as declined!
         engine.enabled = false;
       }
     }
 
-    this.service.engineManager.decline(toDecline);
-    this.service.engineManager.undecline(toUndecline);
+    engineManager.decline(toDecline);
+    engineManager.undecline(toUndecline);
 
     Svc.Prefs.resetBranch("engineStatusChanged.");
     this.service._ignorePrefObserver = false;
   },
 
   _updateEnabledEngines: function () {
     let meta = this.service.recordManager.get(this.service.metaURL);
     let numClients = this.service.scheduler.numClients;
--- a/services/sync/modules/util.js
+++ b/services/sync/modules/util.js
@@ -550,16 +550,32 @@ this.Utils = {
       return foo;
     return foo.concat(Utils.arraySub(bar, foo));
   },
 
   bind2: function Async_bind2(object, method) {
     return function innerBind() { return method.apply(object, arguments); };
   },
 
+  /**
+   * Is there a master password configured, regardless of current lock state?
+   */
+  mpEnabled: function mpEnabled() {
+    let modules = Cc["@mozilla.org/security/pkcs11moduledb;1"]
+                    .getService(Ci.nsIPKCS11ModuleDB);
+    let sdrSlot = modules.findSlotByName("");
+    let status  = sdrSlot.status;
+    let slots = Ci.nsIPKCS11Slot;
+
+    return status != slots.SLOT_UNINITIALIZED && status != slots.SLOT_READY;
+  },
+
+  /**
+   * Is there a master password configured and currently locked?
+   */
   mpLocked: function mpLocked() {
     let modules = Cc["@mozilla.org/security/pkcs11moduledb;1"]
                     .getService(Ci.nsIPKCS11ModuleDB);
     let sdrSlot = modules.findSlotByName("");
     let status  = sdrSlot.status;
     let slots = Ci.nsIPKCS11Slot;
 
     if (status == slots.SLOT_READY || status == slots.SLOT_LOGGED_IN
new file mode 100644
--- /dev/null
+++ b/services/sync/tests/unit/test_password_mpenabled.js
@@ -0,0 +1,130 @@
+/* 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() {
+  Services.prefs.setBoolPref("services.sync.fxaccounts.enabled", 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.
+    Utils.mpEnabled = mpEnabledF;
+    Services.prefs.clearUserPref("services.sync.fxaccounts.enabled");
+    server.stop(run_next_test);
+  }
+});
--- a/services/sync/tests/unit/xpcshell.ini
+++ b/services/sync/tests/unit/xpcshell.ini
@@ -164,8 +164,10 @@ 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]