Bug 970167 - Disable passwords engine when a master password is set. r=rnewman

browser/base/content/sync/customize.js

@@ -4,6 +4,20 @@
 
 "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;
 });

browser/base/content/sync/customize.xul

@@ -44,7 +44,8 @@
     <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;"

browser/base/content/sync/utils.js

@@ -87,6 +87,12 @@ let gSyncUtils = {
     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");
   },

browser/components/preferences/security.js

@@ -173,6 +173,9 @@ var gSecurityPane = {
       this.changeMasterPassword();
 
     this._initMasterPasswordUI();
+
+    // We might want to hide sync's password engine.
+    gSyncPane.updateWeavePrefs();
   },
 
   /**

browser/components/preferences/sync.js

@@ -154,6 +154,17 @@ let gSyncPane = {
         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,

browser/components/preferences/sync.xul

@@ -267,9 +267,23 @@
                 <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"/>

browser/themes/linux/preferences/preferences.css

@@ -165,4 +165,8 @@ label.small {
   margin-bottom: 0.6em;
 }
 
+#fxa-pweng-help-link > label {
+  margin: 0;
+}
+
 %endif

browser/themes/osx/preferences/preferences.css

@@ -230,4 +230,8 @@ html|a.inline-link:-moz-focusring {
   margin-bottom: 0.6em;
 }
 
+#fxa-pweng-help-link > label {
+  margin: 0;
+}
+
 %endif

browser/themes/windows/preferences/preferences.css

@@ -155,4 +155,8 @@ label.small {
   margin-bottom: 0.6em;
 }
 
+#fxa-pweng-help-link > label {
+  margin: 0;
+}
+
 %endif

services/sync/Weave.js

@@ -117,6 +117,16 @@ WeaveService.prototype = {
     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.
    *

services/sync/modules/engines/passwords.js

@@ -36,6 +36,28 @@ PasswordEngine.prototype = {
   _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);
 

services/sync/modules/stages/enginesync.js

@@ -254,25 +254,37 @@ EngineSynchronizer.prototype = {
         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);
+        }
       }
     }
 
@@ -290,8 +302,8 @@ EngineSynchronizer.prototype = {
       }
     }
 
-    this.service.engineManager.decline(toDecline);
-    this.service.engineManager.undecline(toUndecline);
+    engineManager.decline(toDecline);
+    engineManager.undecline(toUndecline);
 
     Svc.Prefs.resetBranch("engineStatusChanged.");
     this.service._ignorePrefObserver = false;

services/sync/modules/util.js

@@ -555,6 +555,22 @@ this.Utils = {
     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);

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);
+  }
+});

services/sync/tests/unit/xpcshell.ini

@@ -169,3 +169,5 @@ skip-if = debug
 
 [test_healthreport.js]
 skip-if = ! healthreport
+
+[test_password_mpenabled.js]