Bug 1673446 - Don't delete ram password when stored login changes on a different account. r=mkmelin

The bug only affects users who manually enter account passwords once per session instead
of letting the password manager store them. This is mostly seen when an oauth2
token (always stored in pwd mgr) changes and would cause normal passwords to be deleted
from ram, requiring the user to re-enter the password again during the session.
This also now avoids notification to all accounts when an oauth2 token seems to change
but really doesn't.
And it also fixes a possible security bug in that a change by the user in pwd mgr of a
stored pwd or username for a POP3 account didn't deleted the password stored in ram like
it should. This appears to be a regression caused by porting of POP3 from C++ to JS.

Differential Revision: https://phabricator.services.mozilla.com/D184660

--HG--
extra : amend_source : 6a9320ff145e2409d2e7ee9e119aebbefb3b5f95

mailnews/base/src/MsgIncomingServer.jsm

@@ -211,12 +211,14 @@ function migrateServerUris(
  *
  * @implements {nsIMsgIncomingServer}
  * @implements {nsISupportsWeakReference}
+ * @implements {nsIObserver}
  * @abstract
  */
 class MsgIncomingServer {
   QueryInterface = ChromeUtils.generateQI([
     "nsIMsgIncomingServer",
     "nsISupportsWeakReference",
+    "nsIObserver",
   ]);
 
   constructor() {
@@ -262,6 +264,58 @@ class MsgIncomingServer {
     //   this._hdrIndex.
     this._knownHdrMap = new Map();
     this._hdrIndex = 0;
+
+    Services.obs.addObserver(this, "passwordmgr-storage-changed");
+  }
+
+  /**
+   * Observe() receives notifications for all accounts, not just this server's
+   * account. So we ignore all notifications not intended for this server.
+   * When the state of the password manager changes we need to clear the
+   * this server's password from the cache in case the user just changed or
+   * removed the password or username.
+   * OAuth2 servers often automatically change the password manager's stored
+   * password (the token).
+   */
+  observe(subject, topic, data) {
+    if (topic == "passwordmgr-storage-changed") {
+      // Check that the notification is for this server and user.
+      let otherFullName = "";
+      let otherUsername = "";
+      if (subject instanceof Ci.nsILoginInfo) {
+        // The login info for a server has been removed with data being
+        // "removeLogin" or "removeAllLogins".
+        otherFullName = subject.origin;
+        otherUsername = subject.username;
+      } else if (subject instanceof Ci.nsIArray) {
+        // Probably a 2 element array containing old and new login info due to
+        // data being "modifyLogin". E.g., a user has modified the password or
+        // username in the password manager or an OAuth2 token string has
+        // automatically changed. Only need to look at names in first array
+        // element (login info before any modification) since the user might
+        // have changed the username as found in the 2nd elements. (The
+        // hostname can't be modified in the password manager.
+        otherFullName = subject.queryElementAt(0, Ci.nsISupports).origin;
+        otherUsername = subject.queryElementAt(0, Ci.nsISupports).username;
+      }
+      if (otherFullName) {
+        if (
+          otherFullName != "mailbox://" + this.hostName ||
+          otherUsername != this.username
+        ) {
+          // Not for this server; keep this server's cached password.
+          return;
+        }
+      } else if (data != "hostSavingDisabled") {
+        // "hostSavingDisabled" only occurs during test_smtpServer.js and
+        // expects the password to be removed from memory cache. Otherwise, we
+        // don't have enough information to decide to remove the cached
+        // password, so keep it.
+        return;
+      }
+      // Remove the password for this server cached in memory.
+      this.password = "";
+    }
   }
 
   /**

mailnews/base/src/OAuth2Module.jsm

@@ -123,11 +123,13 @@ OAuth2Module.prototype = {
     for (let login of logins) {
       if (login.username == this._username) {
         if (token) {
-          let propBag = Cc["@mozilla.org/hash-property-bag;1"].createInstance(
-            Ci.nsIWritablePropertyBag
-          );
-          propBag.setProperty("password", token);
-          Services.logins.modifyLogin(login, propBag);
+          if (token != login.password) {
+            let propBag = Cc["@mozilla.org/hash-property-bag;1"].createInstance(
+              Ci.nsIWritablePropertyBag
+            );
+            propBag.setProperty("password", token);
+            Services.logins.modifyLogin(login, propBag);
+          }
         } else {
           Services.logins.removeLogin(login);
         }

mailnews/base/src/nsMsgIncomingServer.cpp

@@ -49,6 +49,7 @@
 #include "nsIObserverService.h"
 #include "mozilla/Unused.h"
 #include "nsIUUIDGenerator.h"
+#include "nsArrayUtils.h"
 
 #define PORT_NOT_SET -1
 
@@ -78,38 +79,82 @@ nsMsgIncomingServer::~nsMsgIncomingServer() {}
 NS_IMPL_ISUPPORTS(nsMsgIncomingServer, nsIMsgIncomingServer,
                   nsISupportsWeakReference, nsIObserver)
 
+/**
+ * Observe() receives notifications for all accounts, not just this server's
+ * account. So we ignore all notifications not intended for this server.
+ * When the state of the password manager changes we need to clear the
+ * this server's password from the cache in case the user just changed or
+ * removed the password or username.
+ * Oauth2 servers often automatically change the password manager's stored
+ * password (the token).
+ */
 NS_IMETHODIMP
 nsMsgIncomingServer::Observe(nsISupports* aSubject, const char* aTopic,
                              const char16_t* aData) {
   nsresult rv;
-
-  // When the state of the password manager changes we need to clear the
-  // password from the cache in case the user just removed it.
   if (strcmp(aTopic, "passwordmgr-storage-changed") == 0) {
+    nsAutoString otherFullName;
+    nsAutoString otherUserName;
     // Check that the notification is for this server.
     nsCOMPtr<nsILoginInfo> loginInfo = do_QueryInterface(aSubject);
     if (loginInfo) {
-      nsAutoString hostnameInfo;
-      loginInfo->GetHostname(hostnameInfo);
-      nsAutoCString hostname;
-      GetHostName(hostname);
-      nsAutoCString fullName;
-      GetType(fullName);
-      if (fullName.EqualsLiteral("pop3")) {
-        fullName = "mailbox://"_ns + hostname;
-      } else {
-        fullName += "://"_ns + hostname;
+      // The login info for this server has been removed with aData being
+      // "removeLogin" or "removeAllLogins".
+      loginInfo->GetOrigin(otherFullName);
+      loginInfo->GetUsername(otherUserName);
+    } else {
+      // Probably a 2 element array containing old and new login info due to
+      // aData being "modifyLogin". E.g., a user has modified password or
+      // username in the password manager or an OAuth2 token string has
+      // automatically changed.
+      nsCOMPtr<nsIArray> logins = do_QueryInterface(aSubject);
+      if (logins) {
+        // Only need to look at names in first array element (login info before
+        // any modification) since the user might have changed the username as
+        // found in the 2nd elements. (The hostname can't be modified in the
+        // password manager.)
+        nsCOMPtr<nsILoginInfo> login;
+        logins->QueryElementAt(0, NS_GET_IID(nsILoginInfo),
+                               getter_AddRefs(login));
+        if (login) {
+          login->GetOrigin(otherFullName);
+          login->GetUsername(otherUserName);
+        }
       }
-      if (!fullName.Equals(NS_ConvertUTF16toUTF8(hostnameInfo))) return NS_OK;
     }
-    // When this calls nsMsgImapIncomingServer::ForgetSessionPassword with
-    // parameter modifyLogin true and if the server uses oauth2, it causes the
+    if (!otherFullName.IsEmpty()) {
+      nsAutoCString thisHostname;
+      nsAutoCString thisUsername;
+      GetHostName(thisHostname);
+      GetUsername(thisUsername);
+      nsAutoCString thisFullName;
+      GetType(thisFullName);
+      if (thisFullName.EqualsLiteral("pop3")) {
+        // Note: POP3 now handled by MsgIncomingServer.jsm so does not occur.
+        MOZ_ASSERT_UNREACHABLE("pop3 should not use nsMsgIncomingServer");
+        thisFullName = "mailbox://"_ns + thisHostname;
+      } else {
+        thisFullName += "://"_ns + thisHostname;
+      }
+      if (!thisFullName.Equals(NS_ConvertUTF16toUTF8(otherFullName)) ||
+          !thisUsername.Equals(NS_ConvertUTF16toUTF8(otherUserName))) {
+        // Not for this server; keep this server's cached password.
+        return NS_OK;
+      }
+    } else if (NS_strcmp(aData, u"hostSavingDisabled") != 0) {
+      // "hostSavingDisabled" only occurs during test_smtpServer.js and
+      // expects the password to be removed from memory cache. Otherwise, we
+      // don't have enough information to decide to remove the cached
+      // password, so keep it.
+      return NS_OK;
+    }
+    // When nsMsgImapIncomingServer::ForgetSessionPassword called with
+    // parameter modifyLogin true and if the server uses OAuth2, it causes the
     // password to not be cleared from cache. This is needed by autosync. When
     // the aData paremater of Observe() is not "modifyLogin" but is
     // e.g., "removeLogin" or "removeAllLogins", ForgetSessionPassword(false)
     // will still clear the cached password regardless of authentication method.
-    rv = ForgetSessionPassword(
-        nsDependentString(aData).EqualsLiteral("modifyLogin"));
+    rv = ForgetSessionPassword(NS_strcmp(aData, u"modifyLogin") == 0);
     NS_ENSURE_SUCCESS(rv, rv);
   } else if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
     // Now remove ourselves from the observer service as well.

mailnews/compose/src/SmtpServer.jsm

@@ -21,10 +21,14 @@ XPCOMUtils.defineLazyModuleGetters(lazy, {
  * This class represents a single SMTP server.
  *
  * @implements {nsISmtpServer}
+ * @implements {nsIObserver}
  */
 
 class SmtpServer {
-  QueryInterface = ChromeUtils.generateQI(["nsISmtpServer"]);
+  QueryInterface = ChromeUtils.generateQI([
+    "nsISmtpServer",
+    "nsIObserver",
+  ]);
 
   constructor() {
     this._key = "";
@@ -33,17 +37,52 @@ class SmtpServer {
     Services.obs.addObserver(this, "passwordmgr-storage-changed");
   }
 
+  /**
+   * Observe() receives notifications for all accounts, not just this SMTP
+   * server's * account. So we ignore all notifications not intended for this
+   * server. When the state of the password manager changes we need to clear the
+   * this server's password from the cache in case the user just changed or
+   * removed the password or username.
+   * OAuth2 servers often automatically change the password manager's stored
+   * password (the token).
+   */
   observe(subject, topic, data) {
     if (topic == "passwordmgr-storage-changed") {
-      // Check that the notification is for this server.
-      if (
-        subject instanceof Ci.nsILoginInfo &&
-        subject.hostname != "smtp://" + this.hostname
-      ) {
+      // Check that the notification is for this server and user.
+      let otherFullName = "";
+      let otherUsername = "";
+      if (subject instanceof Ci.nsILoginInfo) {
+        // The login info for a server has been removed with aData being
+        // "removeLogin" or "removeAllLogins".
+        otherFullName = subject.origin;
+        otherUsername = subject.username;
+      } else if (subject instanceof Ci.nsIArray) {
+        // Probably a 2 element array containing old and new login info due to
+        // aData being "modifyLogin". E.g., a user has modified the password or
+        // username in the password manager or an OAuth2 token string has
+        // automatically changed. Only need to look at names in first array
+        // element (login info before any modification) since the user might
+        // have changed the username as found in the 2nd elements. (The
+        // hostname can't be modified in the password manager.
+        otherFullName = subject.queryElementAt(0, Ci.nsISupports).origin;
+        otherUsername = subject.queryElementAt(0, Ci.nsISupports).username;
+      }
+      if (otherFullName) {
+        if (
+          otherFullName != "smtp://" + this.hostname ||
+          otherUsername != this.username
+        ) {
+          // Not for this account; keep this account's password.
+          return;
+        }
+      } else if (data != "hostSavingDisabled") {
+        // "hostSavingDisabled" only occurs during test_smtpServer.js and
+        // expects the password to be removed from memory cache. Otherwise, we
+        // don't have enough information to decide to remove the cached
+        // password, so keep it.
         return;
       }
-      // When the state of the password manager changes we need to clear the
-      // password from the cache in case the user just removed it.
+      // Remove the password for this server cached in memory.
       this.password = "";
     }
   }