Bug 1486954 - Part II, Remove OSKeyStore.isEnabled. r=MattN
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Mon, 22 Oct 2018 22:57:28 -0700
changeset 490742 21162d81c6d8252f629f537eb2b760f0bd2db8cb
parent 490741 024dfb5b2d676ee40c45c9fb8b85674b8f6a04e1
child 490743 91cb3b09931643fff6d036f70cbed3a711800a46
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersMattN
bugs1486954
milestone65.0a1
Bug 1486954 - Part II, Remove OSKeyStore.isEnabled. r=MattN Given that the new store is always considered enabled, the not-enabled code is now dead code. This patch removes them. Differential Revision: https://phabricator.services.mozilla.com/D5717
browser/extensions/formautofill/FormAutofillParent.jsm
browser/extensions/formautofill/FormAutofillStorage.jsm
browser/extensions/formautofill/FormAutofillUtils.jsm
browser/extensions/formautofill/OSKeyStore.jsm
browser/extensions/formautofill/content/manageCreditCards.xhtml
browser/extensions/formautofill/content/manageDialog.js
browser/extensions/formautofill/locales/en-US/formautofill.properties
browser/extensions/formautofill/test/browser/browser_manageCreditCardsDialog.js
browser/extensions/formautofill/test/unit/test_osKeyStore.js
toolkit/modules/tests/browser/browser_CreditCard.js
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -312,44 +312,33 @@ FormAutofillParent.prototype = {
     }
 
     let recordsInCollection = await collection.getAll();
     if (!info || !info.fieldName || !recordsInCollection.length) {
       target.sendAsyncMessage("FormAutofill:Records", recordsInCollection);
       return;
     }
 
-    let isCCAndMPEnabled = collectionName == CREDITCARDS_COLLECTION_NAME && OSKeyStore.isEnabled;
-    // We don't filter "cc-number" when OSKeyStore is set.
-    if (isCCAndMPEnabled && info.fieldName == "cc-number") {
+    let isCC = collectionName == CREDITCARDS_COLLECTION_NAME;
+    // We don't filter "cc-number"
+    if (isCC && info.fieldName == "cc-number") {
       recordsInCollection = recordsInCollection.filter(record => !!record["cc-number"]);
       target.sendAsyncMessage("FormAutofill:Records", recordsInCollection);
       return;
     }
 
     let records = [];
     let lcSearchString = searchString.toLowerCase();
 
     for (let record of recordsInCollection) {
       let fieldValue = record[info.fieldName];
       if (!fieldValue) {
         continue;
       }
 
-      // Cache the decrypted "cc-number" in each record for content to preview
-      // when OSKeyStore isn't set.
-      if (!isCCAndMPEnabled && record["cc-number-encrypted"]) {
-        record["cc-number-decrypted"] = await OSKeyStore.decrypt(record["cc-number-encrypted"]);
-      }
-
-      // Filter "cc-number" based on the decrypted one.
-      if (info.fieldName == "cc-number") {
-        fieldValue = record["cc-number-decrypted"];
-      }
-
       if (collectionName == ADDRESSES_COLLECTION_NAME && record.country
           && !FormAutofill.supportedCountries.includes(record.country)) {
         // Address autofill isn't supported for the record's country so we don't
         // want to attempt to potentially incorrectly fill the address fields.
         continue;
       }
 
       if (lcSearchString && !String(fieldValue).toLowerCase().startsWith(lcSearchString)) {
--- a/browser/extensions/formautofill/FormAutofillStorage.jsm
+++ b/browser/extensions/formautofill/FormAutofillStorage.jsm
@@ -1687,22 +1687,19 @@ class CreditCards extends AutofillRecord
     let clonedTargetCreditCard = this._clone(targetCreditCard);
     this._normalizeRecord(clonedTargetCreditCard);
     for (let creditCard of this._data) {
       let isDuplicate = await Promise.all(this.VALID_FIELDS.map(async field => {
         if (!clonedTargetCreditCard[field]) {
           return !creditCard[field];
         }
         if (field == "cc-number" && creditCard[field]) {
-          if (OSKeyStore.isEnabled) {
-            // Compare the masked numbers instead when decryption requires a password
-            // because we don't want to leak the credit card number.
-            return CreditCard.getLongMaskedNumber(clonedTargetCreditCard[field]) == creditCard[field];
-          }
-          return (clonedTargetCreditCard[field] == await OSKeyStore.decrypt(creditCard["cc-number-encrypted"]));
+          // Compare the masked numbers instead when decryption requires a password
+          // because we don't want to leak the credit card number.
+          return CreditCard.getLongMaskedNumber(clonedTargetCreditCard[field]) == creditCard[field];
         }
         return clonedTargetCreditCard[field] == creditCard[field];
       })).then(fieldResults => fieldResults.every(result => result));
       if (isDuplicate) {
         return creditCard.guid;
       }
     }
     return null;
--- a/browser/extensions/formautofill/FormAutofillUtils.jsm
+++ b/browser/extensions/formautofill/FormAutofillUtils.jsm
@@ -12,17 +12,17 @@ const ADDRESS_REFERENCES_EXT = "addressR
 
 const ADDRESSES_COLLECTION_NAME = "addresses";
 const CREDITCARDS_COLLECTION_NAME = "creditCards";
 const MANAGE_ADDRESSES_KEYWORDS = ["manageAddressesTitle", "addNewAddressTitle"];
 const EDIT_ADDRESS_KEYWORDS = [
   "givenName", "additionalName", "familyName", "organization2", "streetAddress",
   "state", "province", "city", "country", "zip", "postalCode", "email", "tel",
 ];
-const MANAGE_CREDITCARDS_KEYWORDS = ["manageCreditCardsTitle", "addNewCreditCardTitle", "showCreditCardsBtnLabel"];
+const MANAGE_CREDITCARDS_KEYWORDS = ["manageCreditCardsTitle", "addNewCreditCardTitle"];
 const EDIT_CREDITCARD_KEYWORDS = ["cardNumber", "nameOnCard", "cardExpiresMonth", "cardExpiresYear", "cardNetwork"];
 const FIELD_STATES = {
   NORMAL: "NORMAL",
   AUTO_FILLED: "AUTO_FILLED",
   PREVIEW: "PREVIEW",
 };
 const SECTION_TYPES = {
   ADDRESS: "address",
--- a/browser/extensions/formautofill/OSKeyStore.jsm
+++ b/browser/extensions/formautofill/OSKeyStore.jsm
@@ -34,27 +34,16 @@ var OSKeyStore = {
    * prompt.
    * @type {Boolean}
    */
   _isLocked: true,
 
   _pendingUnlockPromise: null,
 
   /**
-   * @returns {boolean} Always considered enabled because OS store is always
-   *                    protected via OS user login password.
-   *                    TODO: Figure out if the affacted behaviors
-   *                    (e.g. like bug 1486954 or confirming payment transaction)
-   *                    is correct or not.
-   */
-  get isEnabled() {
-    return true;
-  },
-
-  /**
    * @returns {boolean} True if logged in (i.e. decrypt(reauth = false) will
    *                    not retrigger a dialog) and false if not.
    *                    User might log out elsewhere in the OS, so even if this
    *                    is true a prompt might still pop up.
    */
   get isLoggedIn() {
     return !this._isLocked;
   },
--- a/browser/extensions/formautofill/content/manageCreditCards.xhtml
+++ b/browser/extensions/formautofill/content/manageCreditCards.xhtml
@@ -15,29 +15,27 @@
 </head>
 <body dir="&locale.dir;">
   <fieldset>
     <legend data-localization="creditCardsListHeader"/>
     <select id="credit-cards" size="9" multiple="multiple"/>
   </fieldset>
   <div id="controls-container">
     <button id="remove" disabled="disabled" data-localization="removeBtnLabel"/>
-    <button id="show-hide-credit-cards" data-localization="showCreditCardsBtnLabel"/>
     <!-- Wrapper is used to properly compute the search tooltip position -->
     <div>
       <button id="add" data-localization="addBtnLabel"/>
     </div>
     <button id="edit" disabled="disabled" data-localization="editBtnLabel"/>
   </div>
   <script type="application/javascript">
     "use strict";
     /* global ManageCreditCards */
     new ManageCreditCards({
       records: document.getElementById("credit-cards"),
       controlsContainer: document.getElementById("controls-container"),
       remove: document.getElementById("remove"),
-      showHideCreditCards: document.getElementById("show-hide-credit-cards"),
       add: document.getElementById("add"),
       edit: document.getElementById("edit"),
     });
   </script>
 </body>
 </html>
--- a/browser/extensions/formautofill/content/manageDialog.js
+++ b/browser/extensions/formautofill/content/manageDialog.js
@@ -312,31 +312,27 @@ class ManageAddresses extends ManageReco
 }
 
 class ManageCreditCards extends ManageRecords {
   constructor(elements) {
     super("creditCards", elements);
     elements.add.setAttribute("searchkeywords", FormAutofillUtils.EDIT_CREDITCARD_KEYWORDS
                                                   .map(key => FormAutofillUtils.stringBundle.GetStringFromName(key))
                                                   .join("\n"));
-    this._hasOSKeyStore = OSKeyStore.isEnabled;
     this._isDecrypted = false;
-    if (this._hasOSKeyStore) {
-      elements.showHideCreditCards.setAttribute("hidden", true);
-    }
   }
 
   /**
    * Open the edit address dialog to create/edit a credit card.
    *
    * @param  {object} creditCard [optional]
    */
   async openEditDialog(creditCard) {
     // Ask for reauth if user is trying to edit an existing credit card.
-    if (!creditCard || !this._hasOSKeyStore || await OSKeyStore.ensureLoggedIn(true)) {
+    if (!creditCard || await OSKeyStore.ensureLoggedIn(true)) {
       let decryptedCCNumObj = {};
       if (creditCard) {
         decryptedCCNumObj["cc-number"] = await OSKeyStore.decrypt(creditCard["cc-number-encrypted"]);
       }
       let decryptedCreditCard = Object.assign({}, creditCard, decryptedCCNumObj);
       this.prefWin.gSubDialog.open(EDIT_CREDIT_CARD_URL, "resizable=no", {
         record: decryptedCreditCard,
       });
@@ -357,22 +353,16 @@ class ManageCreditCards extends ManageRe
       encryptedNumber: creditCard["cc-number-encrypted"],
       number: creditCard["cc-number"],
       name: creditCard["cc-name"],
       network: creditCard["cc-type"],
     });
     return cardObj.getLabel({showNumbers: showCreditCards});
   }
 
-  async toggleShowHideCards(options) {
-    this._isDecrypted = !this._isDecrypted;
-    this.updateShowHideButtonState();
-    await this.updateLabels(options, this._isDecrypted);
-  }
-
   async updateLabels(options, isDecrypted) {
     for (let option of options) {
       option.text = await this.getLabel(option.record, isDecrypted);
     }
     // For testing only: Notify when credit cards labels have been updated
     this._elements.records.dispatchEvent(new CustomEvent("LabelsUpdated"));
   }
 
@@ -390,30 +380,15 @@ class ManageCreditCards extends ManageRe
         option.setAttribute("cc-type", record["cc-type"]);
       } else {
         option.removeAttribute("cc-type");
       }
     }
   }
 
   updateButtonsStates(selectedCount) {
-    this.updateShowHideButtonState();
     super.updateButtonsStates(selectedCount);
   }
 
-  updateShowHideButtonState() {
-    if (this._elements.records.length) {
-      this._elements.showHideCreditCards.removeAttribute("disabled");
-    } else {
-      this._elements.showHideCreditCards.setAttribute("disabled", true);
-    }
-    this._elements.showHideCreditCards.textContent =
-      this._isDecrypted ? FormAutofillUtils.stringBundle.GetStringFromName("hideCreditCardsBtnLabel") :
-                          FormAutofillUtils.stringBundle.GetStringFromName("showCreditCardsBtnLabel");
-  }
-
   handleClick(event) {
-    if (event.target == this._elements.showHideCreditCards) {
-      this.toggleShowHideCards(this._elements.records.options);
-    }
     super.handleClick(event);
   }
 }
--- a/browser/extensions/formautofill/locales/en-US/formautofill.properties
+++ b/browser/extensions/formautofill/locales/en-US/formautofill.properties
@@ -99,18 +99,16 @@ savedCreditCardsBtnLabel = Saved Credit Cards…
 # LOCALIZATION NOTE (manageAddressesTitle, manageCreditCardsTitle): The dialog title for the list of addresses or
 # credit cards in browser preferences.
 manageAddressesTitle = Saved Addresses
 manageCreditCardsTitle = Saved Credit Cards
 # LOCALIZATION NOTE (addressesListHeader, creditCardsListHeader): The header for the list of addresses or credit cards
 # in browser preferences.
 addressesListHeader = Addresses
 creditCardsListHeader = Credit Cards
-showCreditCardsBtnLabel = Show Credit Cards
-hideCreditCardsBtnLabel = Hide Credit Cards
 removeBtnLabel = Remove
 addBtnLabel = Add…
 editBtnLabel = Edit…
 # LOCALIZATION NOTE (manageDialogsWidth): This strings sets the default width for windows used to manage addresses and
 # credit cards.
 manageDialogsWidth = 560px
 
 # LOCALIZATION NOTE (addNewAddressTitle, editAddressTitle): The dialog title for creating or editing addresses
--- a/browser/extensions/formautofill/test/browser/browser_manageCreditCardsDialog.js
+++ b/browser/extensions/formautofill/test/browser/browser_manageCreditCardsDialog.js
@@ -1,32 +1,29 @@
 "use strict";
 
 const TEST_SELECTORS = {
   selRecords: "#credit-cards",
   btnRemove: "#remove",
-  btnShowHideCreditCards: "#show-hide-credit-cards",
   btnAdd: "#add",
   btnEdit: "#edit",
 };
 
 const DIALOG_SIZE = "width=600,height=400";
 
 add_task(async function test_manageCreditCardsInitialState() {
   await BrowserTestUtils.withNewTab({gBrowser, url: MANAGE_CREDIT_CARDS_DIALOG_URL}, async function(browser) {
     await ContentTask.spawn(browser, TEST_SELECTORS, (args) => {
       let selRecords = content.document.querySelector(args.selRecords);
       let btnRemove = content.document.querySelector(args.btnRemove);
-      let btnShowHideCreditCards = content.document.querySelector(args.btnShowHideCreditCards);
       let btnAdd = content.document.querySelector(args.btnAdd);
       let btnEdit = content.document.querySelector(args.btnEdit);
 
       is(selRecords.length, 0, "No credit card");
       is(btnRemove.disabled, true, "Remove button disabled");
-      is(btnShowHideCreditCards.disabled, true, "Show Credit Cards button disabled");
       is(btnAdd.disabled, false, "Add button enabled");
       is(btnEdit.disabled, true, "Edit button disabled");
     });
   });
 });
 
 add_task(async function test_cancelManageCreditCardsDialogWithESC() {
   let win = window.openDialog(MANAGE_CREDIT_CARDS_DIALOG_URL);
@@ -150,22 +147,19 @@ add_task(async function test_hasEditLogi
 
   await saveCreditCard(TEST_CREDIT_CARD_1);
 
   let win = window.openDialog(MANAGE_CREDIT_CARDS_DIALOG_URL, null, DIALOG_SIZE);
   await waitForFocusAndFormReady(win);
 
   let selRecords = win.document.querySelector(TEST_SELECTORS.selRecords);
   let btnRemove = win.document.querySelector(TEST_SELECTORS.btnRemove);
-  let btnShowHideCreditCards = win.document.querySelector(TEST_SELECTORS.btnShowHideCreditCards);
   let btnAdd = win.document.querySelector(TEST_SELECTORS.btnAdd);
   // let btnEdit = win.document.querySelector(TEST_SELECTORS.btnEdit);
 
-  is(btnShowHideCreditCards.hidden, true, "Show credit cards button is hidden");
-
   EventUtils.synthesizeMouseAtCenter(selRecords.children[0], {}, win);
 
   // Login dialog should show when trying to edit a credit card record.
   // TODO: test disabled because re-auth is not implemented yet (bug 1429265).
   /*
   let osKeyStoreLoginShown = OSKeyStoreTestUtils.waitForOSKeyStoreLogin(); // cancel
   EventUtils.synthesizeMouseAtCenter(btnEdit, {}, win);
   await osKeyStoreLoginShown;
--- a/browser/extensions/formautofill/test/unit/test_osKeyStore.js
+++ b/browser/extensions/formautofill/test/unit/test_osKeyStore.js
@@ -65,18 +65,16 @@ async function waitForReauth(login = fal
   await TestUtils.topicObserved("oskeystore-testonly-reauth",
     (subject, data) => data == value);
 }
 
 const testText = "test string";
 let cipherText;
 
 add_task(async function test_encrypt_decrypt() {
-  Assert.equal(OSKeyStore.isEnabled, true);
-
   Assert.equal(await OSKeyStore.ensureLoggedIn(), true, "Started logged in.");
 
   cipherText = await OSKeyStore.encrypt(testText);
   Assert.notEqual(testText, cipherText);
 
   let plainText = await OSKeyStore.decrypt(cipherText);
   Assert.equal(testText, plainText);
 });
--- a/toolkit/modules/tests/browser/browser_CreditCard.js
+++ b/toolkit/modules/tests/browser/browser_CreditCard.js
@@ -7,36 +7,32 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource://formautofill/OSKeyStore.jsm");
 ChromeUtils.import("resource://testing-common/OSKeyStoreTestUtils.jsm");
 
 let oldGetters = {};
 let gFakeLoggedIn = true;
 
 add_task(function setup() {
   OSKeyStoreTestUtils.setup();
-  oldGetters.isEnabled = Object.getOwnPropertyDescriptor(OSKeyStore, "isEnabled").get;
   oldGetters.isLoggedIn = Object.getOwnPropertyDescriptor(OSKeyStore, "isLoggedIn").get;
-  OSKeyStore.__defineGetter__("isEnabled", () => true);
   OSKeyStore.__defineGetter__("isLoggedIn", () => gFakeLoggedIn);
   registerCleanupFunction(async () => {
-    OSKeyStore.__defineGetter__("isEnabled", oldGetters.isEnabled);
     OSKeyStore.__defineGetter__("isLoggedIn", oldGetters.isLoggedIn);
     await OSKeyStoreTestUtils.cleanup();
 
     // CreditCard.jsm, OSKeyStore.jsm, and OSKeyStoreTestUtils.jsm are imported
     // into the global scope -- the window -- above. If they're not deleted,
     // they outlive the test and are reported as a leak.
     delete window.OSKeyStore;
     delete window.CreditCard;
     delete window.OSKeyStoreTestUtils;
   });
 });
 
 add_task(async function test_getLabel_withOSKeyStore() {
-  ok(OSKeyStore.isEnabled, "Confirm that OSKeyStore is faked and thinks it is enabled");
   ok(OSKeyStore.isLoggedIn, "Confirm that OSKeyStore is faked and thinks it is logged in");
 
   const ccNumber = "4111111111111111";
   const encryptedNumber = await OSKeyStore.encrypt(ccNumber);
   const decryptedNumber = await OSKeyStore.decrypt(encryptedNumber);
   is(decryptedNumber, ccNumber, "Decrypted CC number should match original");
 
   const name = "Foxkeh";