From 07aa9cc1fcd5479976effe29f6adf5ad5ba7a8f8 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Thu, 12 Jun 2014 18:19:49 +1000 Subject: Bug 1013064 (part 5) - stop disabling the password engine when MP enabled. r=ttaubert diff --git a/browser/base/content/sync/customize.js b/browser/base/content/sync/customize.js --- 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; }); diff --git a/browser/base/content/sync/customize.xul b/browser/base/content/sync/customize.xul --- a/browser/base/content/sync/customize.xul +++ b/browser/base/content/sync/customize.xul @@ -40,18 +40,17 @@ - diff --git a/browser/base/content/sync/utils.js b/browser/base/content/sync/utils.js --- 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. * diff --git a/browser/components/preferences/in-content/sync.js b/browser/components/preferences/in-content/sync.js --- 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; diff --git a/browser/components/preferences/in-content/sync.xul b/browser/components/preferences/in-content/sync.xul --- a/browser/components/preferences/in-content/sync.xul +++ b/browser/components/preferences/in-content/sync.xul @@ -278,30 +278,19 @@ - - - - - - - - - - - + - - - - - - - - - - - + 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 diff --git a/browser/themes/osx/preferences/preferences.css b/browser/themes/osx/preferences/preferences.css --- 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 diff --git a/browser/themes/windows/preferences/preferences.css b/browser/themes/windows/preferences/preferences.css --- 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 diff --git a/services/sync/Weave.js b/services/sync/Weave.js --- 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(). */ diff --git a/services/sync/modules/engines/passwords.js b/services/sync/modules/engines/passwords.js --- 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()) { diff --git a/services/sync/tests/unit/test_password_mpenabled.js b/services/sync/tests/unit/test_password_mpenabled.js 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); - } -}); diff --git a/services/sync/tests/unit/xpcshell.ini b/services/sync/tests/unit/xpcshell.ini --- 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]