374723 - Password Manager should be a JavaScript-based component

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement)

Description

[Justin Dolske [:Dolske]]

The first phase of updating Password Manager it to convert it to Javascript. [It is currently written in C++.] As a JS-based component, it will benefit from increased security and less code complexity. The code is not performance sensitive.

The goals of this phase are to make the code more modular and understandable, but to minimize unnecessary changes while porting from C++ to JS. This conversion is intended to have feature-parity with the current Password Manager. New features will be added in future phases.

Automated test suites will also be written and run against both the new and old code. This will maximize compatibility and minimize future regressions.

The new component will also provide a pony to all users who desire one.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Draft patch

Draft patch.

This looks bigger than it really is, due to a large test file and the inclusion of diffs for bug 370561 (which has not yet landed).

Not yet finished:
- Support for a master password (prompting)
- Form field auto-completing (when multiple logins are applicable). 
- Checking for duplicate logins
- A variety of little TODOs and XXXs that need rounded up
- More tests

It's otherwise functional, and all major work is complete.

[And is up-to-date with the latest changes in CVS, such as support for signons2.txt / form action URL checking]

Comment 2

[Justin Dolske [:Dolske]]

Oh, a few more comments:

- The code for profile migration hasn't been ported
- The resources/ dir under toolkit/components/passwordmgr/ has been renamed to content/, so come review time I should probably do some manual diffs that reflect the actual changes in the file there
- The pwmgr mochitests have a few failures, but I think they're just minor edge cases.
- The pwmgr .idl files are duplicated under toolkit/ and netwerk/, because I'm grumpy about having to place them under network/ due to module dependency rules. *shakes fist*

Comment 3

[Justin Dolske [:Dolske]]

Attachment: key.db needed for unit tests

This is the key3.db needed for the unit tests.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Password Manager, review ready.

Ta-da...

The patch doesn't include the key3.db binary, so if you want 'make check' to work you'll need to copy attachment 262573 [details] into toolkit/components/passwordmgr/test/unit/ first. [Note: if you forget, a bogus key3.db may get created in $DIST/bin/ and the unit tests will fail. Delete it and try again. Will fix this for the next review pass...]

The patch doesn't address SeaMonkey/Camino changes, will are almost certainly broken with this. :-)

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Pseudo patch for toolkit/components/passwordmgr/content/

These changes are already included in the previous patch. However, since files in pwmgr/resources/content/ are now in pwmgr/content/, the diffs don't really capture what changed during the move. This pseudopatch should be easier to review.

Comment 6

[Mike Kaply [:mkaply]]

Are there any plans to switch away from the secret decoder ring and use a real security device so that passwords can be encrypted with hardware encryption? Looking at this patch, it still uses the decoder ring.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to comment #6)
> Are there any plans to switch away from the secret decoder ring and use a real
> security device so that passwords can be encrypted with hardware encryption?
> Looking at this patch, it still uses the decoder ring.

Short answer: not as part of this bug. :)

Longer answer: The decoder ring appears to be built on top of PKCS#11, although I don't know if it's able to use a user-provided PKCS#11 module. With this bug, though, one could write an implementation of nsIPasswordManagerStorage which stores passwords in any arbitrary way.

Comment 8

[Justin Dolske [:Dolske]]

Oops. Mconnor accidently put this in bug 379239 comment 2...

-----8<-----8<-----8<-----

(From update of attachment 263243 [details] [diff] [review])
General notes:

1) Tabs are evil, kill them all.
2) Comments are good, please be more liberal in their use    


+interface nsILoginInfo : nsISupports {
+    attribute AString hostname;
+    attribute AString formSubmitURL;
+    attribute AString httpRealm;
+
+    attribute AString username;
+    attribute AString usernameField;
+
+    attribute AString password;
+    attribute AString passwordField;
+
+    void init(in AString hostname, in AString formSubmitURL, in AString
httpRealm,
+               in AString username, in AString password, in AString
usernameField, in AString passwordField);
+    boolean equals(in nsILoginInfo that);
+    boolean equalsIgnorePassword(in nsILoginInfo that);
+};

all of these, and the interface in general, need documentations, doxygen-style
(we actually have people using those!)

Index: netwerk/base/public/nsIPasswordManager.idl

+interface nsIURI;
+interface nsILoginInfo;
+interface nsIAutoCompleteResult;
+interface nsIDOMHTMLInputElement;
+
+[scriptable, uuid(cb9e0de8-3598-4ed7-857b-827f011ad5d8)]
+
+interface nsIPasswordManager : nsISupports {
+
+    void addLogin(in nsILoginInfo login);
+    void removeLogin(in nsILoginInfo login);
+    void modifyLogin(in nsILoginInfo oldLogin, in nsILoginInfo newLogin);
+
+    void getAllLogins(out unsigned long count, [retval, array, size_is(count)]
out nsILoginInfo logins);
+    // Crap, can't use AString here... Error: [domstring], [utf8string],
[cstring], [astring] types cannot be used in array parameters
+    void getAllDisabledLogins(out unsigned long count, [retval, array,
size_is(count)] out wstring hostname);
+
+    void disableSavingLoginsFor(in AString host);
+    void enableSavingLoginsFor(in AString host);
+    boolean canSaveLoginsFor(in AString host);
+
+    void findLogins(out unsigned long count, in AString hostname, in AString
actionURL, in AString httpRealm, [retval, array, size_is(count)] out
nsILoginInfo logins);
+
+    // XXX Ugh. This interface is directly clled by FormFillController. This
ought to be done some other way.
+    nsIAutoCompleteResult autoCompleteSearch(in AString aSearchString, in
nsIAutoCompleteResult aPreviousResult, in nsIDOMHTMLInputElement aElement);

Pretty sure we should call this nsIToolkitPasswordManager since the API is
totally different now.  Also needs docs for the API etc

Index: netwerk/base/public/nsIPasswordManagerStorage.idl

+[scriptable, uuid(e09e4ca6-276b-4bb4-8b71-0635a3a2a007)]
+
+interface nsIPasswordManagerStorage : nsISupports {
+    void init();
+    AString getAttribute(in AString propertyName);
+    void setAttribute(in AString propertyName, in AString propertyValue);
+
+    void addLogin(in nsILoginInfo login);
+    void removeLogin(in nsILoginInfo login);
+    void modifyLogin(in nsILoginInfo oldLogin, in nsILoginInfo newLogin);
+
+    void getAllLogins(out unsigned long count, [retval, array, size_is(count)]
out nsILoginInfo logins); 
+    void getAllDisabledLogins(out unsigned long count, [retval, array,
size_is(count)] out wstring hostname); 
+
+    void disableSavingLoginsFor(in AString hostname);
+    void enableSavingLoginsFor(in AString hostname);

why have two methods here, instead of a single method with a boolean?

Also, I hate naming like that, we don't tend to use it.

+    boolean canSaveLoginsFor(in AString hostname);
+
+    void findLogins(out unsigned long count, in AString hostname, in AString
actionURL, in AString httpRealm, [retval, array, size_is(count)] out
nsILoginInfo logins);

This feels awkward, why not just return an nsISimpleEnumerator of
nsILoginInfos?


@@ -801,25 +801,27 @@ nsFtpState::R_pass() {
     }
     if (mResponseCode/100 == 5 || mResponseCode==421) {
         // There is no difference between a too-many-users error,
         // a wrong-password error, or any other sort of error
         // So we need to tell wallet to forget the password if we had one,
         // and error out. That will then show the error message, and the
         // user can retry if they want to

-        if (!mPassword.IsEmpty()) {
+        if (PR_FALSE && !mPassword.IsEmpty()) {
+            // XXX I think the original code here is wrong. A server with a
+            // temporary glitch shouldn't nuke our stored login.

file a followup here and leave the behaviour intact.

             nsCOMPtr<nsIPasswordManager> pm =
                     do_GetService(NS_PASSWORDMANAGER_CONTRACTID);
             if (pm) {
                 nsCAutoString prePath;
                 nsresult rv = mChannel->URI()->GetPrePath(prePath);
                 NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to get prepath");
-                if (NS_SUCCEEDED(rv))
-                    pm->RemoveUser(prePath, EmptyString());
+                //if (NS_SUCCEEDED(rv))
+                //    pm->RemoveUser(prePath, EmptyString());

delete, don't comment out, as a general rule.

nsPasswordManager.js next!

Comment 9

[Mike Connor [:mconnor]]

more (I meant passwordManager.js, not nsPasswordManager.js, clearly

+    var rv="";

spaces around operators (change this everywhere you didn't do it)

+    if (column.id=="siteCol") {
+      rv = signons[row].hostname;
+      if (signons[row].httpRealm) { rv += " (" + signons[row].httpRealm + ")"; }
+    } else if (column.id=="userCol") {
+      rv = signons[row].username;
+    } else if (column.id=="passwordCol") {
+      rv = signons[row].password;
+    }

make this a switch please

+  // disable "remove all signons" button if there are no signons
+  var element = document.getElementById("removeAllSignons");
+  var toggle = document.getElementById("togglePasswords");
+  if (signons.length == 0 || gSelectUserInUse) {
+    element.setAttribute("disabled","true");
+    toggle.setAttribute("disabled","true");
+  } else {
+    element.removeAttribute("disabled");
+    toggle.removeAttribute("disabled");
+  }

I'd probably flip this if/else around, since signons.length will be true most of the time

+  return true;
+}

why return true here?  You're not checking success here (are all of these ops exception-free?)

+function SignonSelected() {
+  var selections = GetTreeSelections(signonsTree);
+  if (selections.length && !gSelectUserInUse) {
+    document.getElementById("removeSignon").removeAttribute("disabled");
+  }

nit: brackets around single lines

+function AskUserShowPasswords() {
+  var prompter = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);

80 chars (lather, rinse, repeat)

+  var dummy = { value: false };
+
+  // Confirm the user wants to display passwords
+  return prompter.confirmEx(window,
+          null,
+          kSignonBundle.getString("noMasterPasswordPrompt"), prompter.STD_YES_NO_BUTTONS,
+          null, null, null, null, dummy) == 0;    // 0=="Yes" button

wacky alignment

+var lastSignonSortColumn = "";
+var lastSignonSortAscending = false;
+
+function SignonColumnSort(column) {
+  lastSignonSortAscending =
+    SortTree(signonsTree, signonsTreeView, signons,
+                 column, lastSignonSortColumn, lastSignonSortAscending);

alignment again (I wish I could blame tabs)


+// interface variables
+var passwordmanager     = null;
+
+// password-manager lists
+var signons             = [];
+var rejects             = [];
+var deletedSignons      = [];
+var deletedRejects      = [];
+
+var signonsTree;
+var rejectsTree;
+
+var showingPasswords = false;

please prefix the globals with g like you did with gSelectUserInUse


+var signonReloadDisplay = {
+  observe: function(subject, topic, state) {
+    if (topic == "signonChanged") {
+      if (state == "signons") {
+        signons.length = 0;
+        if (lastSignonSortColumn == "hostname") {
+          lastSignonSortAscending = !lastSignonSortAscending; // prevents sort from being reversed
+        }
+        LoadSignons();
+      } else if (state == "rejects") {
+        rejects.length = 0;
+        if (lastRejectSortColumn == "hostname") {
+          lastRejectSortAscending = !lastRejectSortAscending; // prevents sort from being reversed
+        }
+        LoadRejects();
+      }
+    } else if (topic == "signonSelectUser") {
+      if (state == "suspend") {
+        gSelectUserInUse = true;
+        document.getElementById("removeSignon").disabled = true;
+        document.getElementById("removeAllSignons").disabled = true;
+        document.getElementById("togglePasswords").disabled = true;
+      } else if (state == "resume") {
+        gSelectUserInUse = false;
+        var selections = GetTreeSelections(signonsTree);
+        if (selections.length > 0) {
+          document.getElementById("removeSignon").disabled = false;
+        }
+        if (signons.length > 0) {
+          document.getElementById("removeAllSignons").disabled = false;
+          document.getElementById("togglePasswords").disabled = false;
+        }
+      } else if (state == "inUse") {
+        gSelectUserInUse = true;
+      }
+    }
+  }
+}

probably more readable to convert both blocks to switch statements (and faster, iirc)

reminder-nit: no brackets around single lines

+  // disable buttons
+  document.getElementById(removeButton).setAttribute("disabled", "true")
+  document.getElementById(removeAllButton).setAttribute("disabled","true");

space after comma

+/*** =================== REJECTED SIGNONS CODE =================== ***/
+
+function RejectsStartup() {
+  LoadRejects();
+}

This could call startup, instead of having both in the onload, seems cleaner.

+function LoadRejects() {
+  var logins = passwordmanager.getAllDisabledLogins({});
+  var count = 0;
+  for (var count = 0; count < logins.length; count++) {

spot the silliness!

+# Note:
+# The nsILoginInfo.idl, nsIPasswordManager.idl, and nsIPasswordManagerStorage interfaces are over in
+# mozilla/netwerk/base/public/ because they're needed for some HTTP/FTP stuff, and Netwerk isn't supposed
+# to depend on Toolkit (although the reverse is ok). *sigh*
+#
+# XXX PasswordManagerStorage.idl doesn't seem like it should need to be over there, but without it
+# browser/components/migration/src/nsSeamonkeyProfileMigrator.cpp complains. Why isn't is able to
+# find it when we put it here?

talk to bsmedberg/biesi about this, I'm not sure what the right answer is offhand

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 262589 [details] [diff] [review]
Password Manager, review ready.

Index: toolkit/components/passwordmgr/src/nsLoginInfo.js
+const Cc = Components.classes;
+const Ci = Components.interfaces;

worth adding Cr for Components.results?


+	this.hostname      = hostname;
+	this.formSubmitURL = formSubmitURL;
+	this.httpRealm     = httpRealm;
+	this.username      = username;
+	this.password      = password;
+	this.usernameField = usernameField;
+	this.passwordField = passwordField;

tabs!

+    },
+
+    equalsIgnorePassword : function (that) {
+	if (this.hostname      != that.hostname)      { return false; }
+	// If either formSubmitURL is blank (but not null), then allow the match.
+	if (this.formSubmitURL != "" && that.formSubmitURL != "" &&
+	    this.formSubmitURL != that.formSubmitURL) { return false; }
+	if (this.httpRealm     != that.httpRealm)     { return false; }
+
+	if (this.username      != that.username)      { return false; }
+	if (this.usernameField != that.usernameField) { return false; }
+
+	//if (this.password      != that.password)      { return false; }
+	if (this.passwordField != that.passwordField) { return false; }
+	return true;
+    },

what's "that" here?  docs!

+  getClassObject: function(componentManager, cid, iid) {
+    if (!iid.equals(Components.interfaces.nsIFactory))

Nit: Ci


+  /*
+   * init
+   *
+   * Initialize the Password Manager. Automatically called when service is created..
+   *
+   * Note: Service created in /browser/base/content/browser.js : delayedStartup()
+   */
+  init : function () {
+
+	// Cache references to current |this| in utility objects
+	this.webProgressListener._domEventListener = this.domEventListener;
+	this.webProgressListener._pwmgr = this;
+	this.domEventListener._pwmgr    = this;
+	this.observer._pwmgr            = this;
+
+	// Log service uses Error Console window for debug messages.
+	this._logService = Cc["@mozilla.org/consoleservice;1"].getService();
+	this._logService.QueryInterface(Ci.nsIConsoleService);

are there going to be errors frequently?  it makes more sense to only init this on first call of this.log (if (!this._logService) initLogService)

+	// Preferences. Add observer so we get notified when our branch prefs change.
+	this._prefService = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).getBranch("signon.");
+	this._prefService.QueryInterface(Ci.nsIPrefBranch2);
+	this._prefService.addObserver("", this.observer, false);
+
+	// Get current preference values.
+	if (this._prefService.prefHasUserValue("debug")) {
+		this._debug = this._prefService.getBoolPref("debug");
+	}
+	if (this._debug) { this.log("Debugging enabled."); }

So, we generally just call this.log and check whether to output in log(), and for booleans its not worth the overhead of caching, so just check the pref directly each call and drop that from the observer

I see you already check this._debug in log() so just call this.log, and make it check the pref directly.  this also means you can kill some of the pref observer code

+	this._remember = this._prefService.getBoolPref("rememberSignons");

this is probably used enough to warrant caching, but if its the only thing you're observing it might be a net neutral change.  You could do and pull this out of init altogether

get _remember() {
  this._prefService.getBoolPref("rememberSignons");
}

+	// IO service for string -> nsIURI conversion
+	this._ioService = Cc["@mozilla.org/network/io-service;1"].getService();
+	this._ioService.QueryInterface(Ci.nsIIOService);
+
+	// Prompt service for user interaction
+	this._promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"].getService();
+	this._promptService.QueryInterface(Ci.nsIPromptService2);
+
+	// Form Fill Controller service to autocompleting username fields
+	//this._formFillService = Cc["@mozilla.org/satchel/form-fill-controller;1"].getService();
+	//this._formFillService.QueryInterface(Ci.nsIFormFillController);

do we need to init all of these these right away?  Might be worth doing the following (and pulling out of init)

__ioService: null;
get _ioService() {
  if (!this.__ioService) {
    this.__ioService = Cc["@mozilla.org/network/io-service;1"].getService();
    this.__ioService.QueryInterface(Ci.nsIIOService);
  }
  
  return this.__ioService;
}

+	// Get constructor for nsILoginInfo
+	this._nsLoginInfo = new Components.Constructor("@mozilla.org/passwordmanager/loginInfo;1", Ci.nsILoginInfo);
+
+
+	// Get string bundles for localization and branding
+	var bunService = Cc["@mozilla.org/intl/stringbundle;1"].getService();
+	bunService.QueryInterface(Ci.nsIStringBundleService);
+
+	this._strBundle = bunService.createBundle("chrome://passwordmgr/locale/passwordmgr.properties");
+	if (!this._strBundle) { throw "String bundle for Password Manager not present!"; }
+
+	this._brandBundle = bunService.createBundle("chrome://branding/locale/brand.properties");
+	if (!this._brandBundle) { throw "Branding string bundle not present!"; }

When we throw here, do we handle this well elsewhere?


+  log : function (message) {
+        if (!this._debug) { return; }
+	dump("Password Manager: " + message + "\n");

debugging cruft?

+  /* ---------- Utility objects ---------- */
+
+
+  /*
+   * observer object
+   *
+   * Internal utility object, implements the nsIObserver interface.
+   * Used to receive notification for: form submission, preference changes.
+   */
+  observer : {
+	_pwmgr : null,
+
+	QueryInterface : function (iid) {
+		const interfaces = [Ci.nsIObserver, Ci.nsIFormSubmitObserver, Ci.nsISupports, Ci.nsISupportsWeakReference];
+		if (!interfaces.some( function(v) { return iid.equals(v) } ))
+			throw Components.results.NS_ERROR_NO_INTERFACE;
+		return this;
+	},
+
+
+	// nsFormSubmitObserver
+	//
+	// aWindow and actionURI (a nsURI) are ignored, because we can derive them from the form element.

why not use what the observer's giving you?

+	notify : function (formElement, aWindow, actionURI) {
+		this._pwmgr.log("observer notified for form submission.");
+
+		// We're invoked before the contents |onsubmit| handlers, so that we
+		// can grab the form data before it might be modified (see bug 257781).
+
+		//var formElement = subject.QueryInterface(Ci.nsIDOMHTMLFormElement);

don't leave stuff like this around, please

+		this._pwmgr.onFormSubmit(formElement);
+
+		return true; // Always return true, or else the form submission will be canceled.

does anything here throw?

+	},
+
+	// nsObserver
+	observe : function (subject, topic, data) {
+
+		if (topic == "nsPref:changed") {
+			var prefName = data;
+			this._pwmgr.log("observer notifed of change to " + prefName + " preference");
+
+			if (prefName == "debug") {
+				// The debug pref is hidden (no default), so check for existence first.
+				if (this._pwmgr._prefService.prefHasUserValue("debug")) {
+					var debug = this._pwmgr._prefService.getBoolPref("debug");
+				} else {
+					debug = false;
+				}
+
+				// Make sure the state-change message is always logged.
+				this._pwmgr._debug = true;
+				this._pwmgr.log("Debugging " + (debug ? "enabled." : "disabled."));
+				this._pwmgr._debug = debug;
+
+			} else if (prefName == "rememberSignons") {
+				this._pwmgr._remember = this._pwmgr._prefService.getBoolPref("rememberSignons");
+			} else {
+				this._pwmgr.log("Oops! Pref not handled -- change ignored.");

ditch this type of thing, IMO (leads to debug-spam in the general case)

+			}
+		} else {
+			this._pwmgr.log("WARNING: observer got unexpected notification: " + topic);

is this likely?  do we care enough to log it?

given my other comments in init(), I think this just all goes away anyway...



+        onStateChange : function (aWebProgress, aRequest, aStateFlags, aStatus) {
+		/*
+		this._pwmgr.log("onStateChange........... " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_IS_REQUEST)  ? "REQUEST"  : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) ? "DOCUMENT" : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK)  ? "NETWORK"  : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW)   ? "WINDOW"   : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_RESTORING)    ? "RESTOR" : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_START)        ? "START"  : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_REDIRECTING)  ? "REDIR"  : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_TRANSFERRING) ? "TRANS"  : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_NEGOTIATING)  ? "NEGOT"  : "") + " " +
+			((aStateFlags & Ci.nsIWebProgressListener.STATE_STOP)         ? "STOP"   : "") );
+		*/

if this is useful, we should uncomment it, if not we should remove it

I'm wondering if you want an int pref for signons.debug and do some bitmask stuff to log different amounts of stuff...	

+		// STATE_START is too early, doc is still the old page.
+		if (!(aStateFlags & Ci.nsIWebProgressListener.STATE_TRANSFERRING)) { return 0; }
+
+		if (!this._pwmgr._remember) { return 0; }

nit: no brackets, return on a new line.

+		var domWin  = aWebProgress.DOMWindow;
+		var domDoc  = domWin.document;

nit: extra spaces

+		// Check what loaded, so we only process things that might have HTML forms.
+		if (domDoc.contentType != "text/html" && domDoc.contentType != "application/xhtml+xml") { return 0; }

nit: return on newline, longer than 80 charts

+		// TODO: also allow application/xml and text/xml for stupid xhtml pages?
+		// or: if (!domDoc.doctype || domDoc.doctype.name != "HTML" or "html") { return 0; }

I don't know what this is for

+	// stubs for the nsIWebProgressListener interfaces which we don't use.
+        onProgressChange : function() { throw "Unexpected onProgressChange"; },
+        onLocationChange : function() { throw "Unexpected onLocationChange"; },
+        onStatusChange   : function() { throw "Unexpected onStatusChange";   },
+        onSecurityChange : function() { throw "Unexpected onSecurityChange"; },
+  },

remind me why this isn't going to throw all of the time?

+  /* ---------- Public interfaces ---------- */
+
+  /*
+   * addLogin
+   *
+   * Add a new login to login storage.
+   */
+  addLogin : function (login) {
+	// Sanity check the login
+	if (login.hostname == null || login.hostname.length == 0) {
+		throw "Can't add a login with a null or empty hostname.";
+	}
+	if (login.username == null || login.username.length == 0) {
+		throw "Can't add a login with a null or empty username.";
+	}
+	if (login.password == null || login.password.length == 0) {
+		throw "Can't add a login with a null or empty password.";
+	}
+	if (!login.httpRealm && !login.formSubmitURL) {
+		throw "Can't add a login without a httpRealm or formSubmitURL.";
+	}

nit: tabs, bogus brackets, seems kinda verbose


+  /*
+   * getAllLogins
+   *
+   * Get a dump of all stored logins. Used by the password manager UI.
+   *
+   * |count| is only needed for stupid XPCOM.

stupid XPCOM?


+  canSaveLoginsFor : function (host) {
+	this.log("Checking if logins to " + host + " can be saved.");
+	if (!this._remember) { return false; }

I know we do a bunch of this style in C++, but that at least usually has eye-catching consts to jump out at you.  Anyway, I'm going to assume that I've bitched about this enoguh by now... ;)

+	return this._storage.canSaveLoginsFor(host);
+  },
+
+
+
+
+  /* ---------- Internal methods / callbacks for document integration ---------- */

we generally use _ to prefix internal methods so people don't get sneaky and try to use them...


+
+	function handleAddingLogin(this2, newLogin) {

this2? please pick a better name...

+		if (userChoice == 2) {
+			this2.log("Disabling logins to " + hostname + " per user request.");
+			this2.disableSavingLoginsFor(hostname);
+		} else if (userChoice == 0) {
+			this2.log("Saving login for " + hostname);
+			this2.addLogin(newLogin);
+		} else {
+			// userChoice == 1 --> just ignore the login.
+			this2.log("Ignoring login.");
+		}

you know what I'm going to say here, probably!


+	// If password saving is disabled (entirely, or just for this host), bail out now.
+	if (!this._remember) { return; }

bah

+	var hostname = this.getPasswordOrigin(doc.documentURI);
+	if (!this.canSaveLoginsFor(hostname)) {
+		this.log("(form submission ignored -- saving is disabled for: " + hostname + ")");
+		return;
+	}
+	var formSubmitURL = this.getActionOrigin(form)

nit: newline after the code block

+	// Count the number of password fields in the form.
+	var numFields = 0;
+	var pwFields = new Array();
+	for (var i = 0; i < form.elements.length; i++) {
+		if (form.elements[i].type != "password") { continue; }
+		numFields++;
+		switch (numFields) {
+		    case 1:
+			pwFields[0] = { index : i, element : form.elements[i] };
+			break;
+		    case 2:
+			pwFields[1] = { index : i, element : form.elements[i] };
+			break;
+		    case 3:
+			pwFields[2] = { index : i, element : form.elements[i] };
+			break;
+		    default:
+		}
+	}

tabs here make the indenting look weird in textwranglger, I'll assume you'll fix the tabs ;)

also, numFields seems extraneous, something like the following seems less verbose and ugly, and you can also just stop right here once you get to four, since we're going to do that later anyway

  for (var i in form.elements) {
    if (form.elements[i].type != "password")
      continue;

    pwFields[pwFields.length] = { index : i, element : form.elements[i] };
    if (pwFields.length > 3) {
      this.log("(form submission ignored -- too many password fields found [" + numFields + "])");
      return;
    }
  }

+	if (numFields == 0) {
+		this.log("(form submission ignored -- no password fields found)");

hmm, that seems like log spam, I bet that'd make logging crazy-verbose

how about just returning here, then we can refactor the below a lot more freely as follows:

a) check the username field exists and has a usable value
b) do validation on the password field values
c) check for autocomplete="off"
d) do situation-specific codepaths for 1/2/3 as appropriate

please do this in two steps, I think this all can be a lot cleaner, but start there and I'll look again once you've done cleanup and its less head-wrenching



+	} else if (numFields == 2 || numFields == 3) {
+		var pw1 = pwFields[0].element.value;
+		var pw2 = pwFields[1].element.value;
+		var pw3 = (numFields == 3 ? pwFields[2].element.value : null);
+
+		// If there are blank passwords, bail out now.
+		if (pw1 == "" || pw2 == "" || pw3 == "") {
+			this.log("(form submission ignored -- one or more password fields are blank)");
+			return;
+		}

wouldn't this break sites that have the old-style login/change password like old mediawiki?  unless this is backwards compat stuff, I think it's possibly scary.

+
+		/*
+		 * If there are 2 password fields:
+		 * - could be a form for creating a new login, pw1 == p2 for typo checking
+		 * - could be a change password form (w/o typo checking? seems unusual?) prompt user to be sure
+		 *
+		 * If there are 3 password fields:
+		 * - could be an authenticated change password form (1 old pw, 2 new pw for typo checking)
+		 * - could be a create-user form (1 admin password, 2 fields for typo checking new user's password)
+		 */
+
+		// Look for two password fields with the same value (a new password)
+		if (pw1 == pw2 && pw2 == pw3) {
+			// This is a weird case. Could be a change-password (to the same value)? Or maybe a new account?
+			this.log("(form submission ignored -- the three password fields are all identical)");
+			newPasswordField = null;
+			oldPasswordField = null;
+			return;
+		} else if (pw1 == pw2) {
+			var newPasswordField = pwFields[0].element;
+			var oldPasswordField = (numFields == 3 ? pwFields[2].element : null);
+		} else if (pw2 == pw3) {
+			newPasswordField = pwFields[1].element;
+			oldPasswordField = pwFields[0].element;
+		} else  if (pw1 == pw3) {
+			// Somewhat odd case, but could make sense with the right page layout.
+			newPasswordField = pwFields[0].element;
+			oldPasswordField = pwFields[1].element;
+		} else {
+			this.log("(form submission ignored -- the three password fields are all different)");
+			newPasswordField = null;
+			oldPasswordField = null;
+			return;
+		}



+		// Look for |autocomplete=off| on userfield, new passfield, old passfield, and form. Bail out if any are true.
+		if (autocompleteDisabled(form) || autocompleteDisabled(usernameField) ||
+		    autocompleteDisabled(oldPasswordField) || autocompleteDisabled(newPasswordField))
+		{
+			this.log("(form submission ignored -- autocomplete=off attribute found)");
+			return;
+		}
+
+		if (oldPasswordField && newPasswordField) {

if !newPassword the above code failed badly, no?

+		} else if (numFields == 2) {
+			// We have 2 password fields, with different values. Probably a change-password form which doesn't
+			// confirm the new password?
+			//
+			// TODO:
+			// try to find an existing login that matches one field (name, value, or both?), that's the old password
+			// (could be multiple logins for site, try to match an existing username to form? else have user select?)
+			// always prompt the user to confirm password change
+			// note: old code assumes 2nd field is the new password?

in this case, why not just set the second field as the new password in the comparison code, and drop this block?  We'll never get to this block anyway, since you return if you don't hav etwo matching fields

+			this.log("(form submission ignored -- both password fields are different [not looking at logins yet])");
+
+			//var ok, userIndex;
+			//var usernames = ["username1", "username2"];
+			//[ok, userIndex] = promptToChangePasswordWithUsernames(win, usernames);
+		} else {
+			// We have 3 password fields, and they're all different. So, we don't know what to save.
+			// Even if we match one field name/value to an existing login, which of the other 2 fields is the new password?
+			this.log("(form submission ignored -- 3 password fields with different values)");
+		}
+

isn't this already handled in the comparison code?  if you hit this I'm shocked



+  /*
+   * getPasswordOrigin
+   *
+   * Get the parts of the URL we want for identification.
+   */
+  getPasswordOrigin : function (uriString) {
+	var realm = "";
+	try {
+		var uri = this._ioService.newURI(uriString, null, null);
+
+		realm += uri.scheme;
+		realm += "://";
+		realm += uri.hostPort;
+	} catch (e) {
+		// bug 159484 - disallow url types that don't support a hostPort. (It will throw in the JS above)
+		realm = null;
+	}
+
+	return realm;
+  },

hrm, don't we just use host for passwords in Fx2?  doesn't this cause problems?

+  fillDocument : function (doc)  {
+
+	function getElementByName (elements, name) {
+		for (var i = 0; i < elements.length; i++) {
+			var element = elements[i];
+			if (element.name && element.name == name) { return element; }
+		}
+		return null;
+	};

ew, tabs.  also fix the return on a newline and bogus brackets

+	/*
+	 * CRIKEY! The original C++ code could really use a cleanup. This function is
+	 * just a straight conversion to JS, with a few minor changes when interacting
+	 * with parts of pwmgr that have been updated.
+	 */

ok, file a followup to make this be saner?  the C++ version is fragile...

+	var forms = doc.forms;
+	if (forms.length == 0) { return; } // bail out fast if there's nothing to do.

prevailing style is generally:

  if (forms.length == 0)
    return;


skipped the rest of this function since we're going to whack it later


+  /*
+   * attachToInput
+   *
+   */
+  attachToInput : function (element) {
+	this.log("attaching autocomplete stuff");
+	element.addEventListener("blur", this.domEventListener, false);
+	element.addEventListener("DOMAutoComplete", this.domEventListener, false);
+	if (this._formFillService == null) {

except in specific cases, !this._formFillService is preferred to explicit comparisons to null, though if we do what I suggested earlier with making _formFillService abstract this all away, this is unnecessary

+		// Form Fill Controller service to autocompleting username fields
+		this._formFillService = Cc["@mozilla.org/satchel/form-fill-controller;1"].getService();
+		this._formFillService.QueryInterface(Ci.nsIFormFillController);
+	}
+	this._formFillService.tagAsPasswordManagerField(element);

don't think I flagged this, but markAs > tagAs (tag is overloaded these days)


+  /*
+   * fillPassword
+   *
+   * The user has autocompleted a username field, so fill in the password.
+   */
+  fillPassword : function (usernameField) {
+	this.log("fillPassword called for autocompleted username: " + usernameField.value);
+
+	var form = usernameField.form;
+	var doc = form.ownerDocument;
+
+	var hostname = this.getPasswordOrigin(doc.documentURI);
+	var formSubmitURL = this.getActionOrigin(form)
+
+	// Find the password field
+	var passwordField = null;
+	for (var i = 0; i < form.elements.length; i++) {
+		if (form.elements[i].type != "password") { continue; }
+		passwordField = form.elements[i];
+		break;
+		// XXX: we could probably do better on forms with 2 or 3 password fields.

should we just have _getPasswordFields as a helper function?  seems likely

+	if (!passwordField) {
+		// We should always have at least one password field, or we wouldn't be handling the form.
+		this.log("Unpossible! No password field for autocomplete password fill. Was it removed?");
+		return;
+	}

this should almost assert/dump to the error console directly, since it means we hooked up the listener without a password field.


+	this.log("AutoCompleteSearch invoked. Search string is: " + aSearchString);
+
+	var searchLength = aSearchString.length;

is this worth caching?

+	var lowerSearch = aSearchString.toLowerCase();

this is not the best var name, lowerCaseSearchString?



+	this.log("Prompting user to save/ignore login");
+	var selection = this._promptService.confirmEx(aWindow,
+				dialogTitle, dialogText, buttonFlags,
+				rememberButtonText, notNowButtonText, neverButtonText,
+				null, {});

nit: tabs, dunno if the alignment is wacky

+	return selection;
+  },
+
+
+  /*
+   * promptToChangePassword
+   *
+   * Called when we detect a password change for an existing login in a form submission,

with multiple password fields

+   * asks the user to confirm the change.
+   *
+   * Return values:
+   *   0 - Update the stored password
+   *   1 - Do not update the stored password
+   */

if this is what we're doing, do we want to refactor this interaction to actually abort the form submission if the user says "No" ?

+  promptToChangePasswordWithUsernames : function (aWindow, usernames) {
+	const buttonFlags = Ci.nsIPrompt.STD_YES_NO_BUTTONS;
+
+	var dialogText  = this.getLocalizedString("userSelectText", [username]);
+	var dialogTitle = this.getLocalizedString("passwordChangeTitle");
+	var outparam = { value: null };

username?

+	// If user selects ok, outparam.value is set to te index of the selected username.
+	var selection = this._promptService.select(aWindow,
+				dialogTitle, dialogText,
+				usernames.length, usernames,
+				outparam);

s/selection/result/ ?

Comment 11

[Mike Connor [:mconnor]]

+ * Invoked by:
+ * http://mxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/src/nsPrompt.cpp#120
+ */

linking to MXR/LXR like this can be tricky, since the link will break, maybe something like "NS_NewAuthPrompter2 in nsPrompt.cpp" is a better reference.

+  _init : false,

init is usually a function, inited or initialized works better here.

+  getPrompt : function (aWindow, aIID) {

kill the dump() calls entirely, don't just comment out.

+  _logService    : null,
+  _promptService : null,
+  _pwmgr         : null,
+  _window        : null,
+
+  _debug         : true, // XXX get from pref?
+
+
+  init : function (aPWManager, aPromptService, aWindow) {
+	// Log service to use Error Console window.
+	this._logService = Cc["@mozilla.org/consoleservice;1"].getService();
+	this._logService.QueryInterface(Ci.nsIConsoleService);
+
+	this._pwmgr = aPWManager;
+	this._promptService = aPromptService;
+	this._window = aWindow;
+
+	this.log("===== initialized =====");
+  },

same comments as elsewhere, unless there's a need to init these right away, make them all getters and init when needed

+  /*
+   * log
+   *
+   * Internal utility function for logging debug messages to the Error Console window.
+   */
+  log : function (message) {
+	if (!this._debug) { return; }
+	dump("Pwmgr Prompter: " + message + "\n");
+	this._logService.logStringMessage("Pwmgr Prompter: " + message);
+  },

should follow the same pref as the other pwmgr stuff, or a similar one, and should just be checked here (getBoolPref is really cheap)

iirc there's a debug.js that already has generic placeholders for all of this stuff, maybe check with gavin about reusing those like we did with search service


+	if (ok && rememberLogin) {
+		if (aAuthInfo.username != foundLogins[0].username ||
+		    aAuthInfo.password != foundLogins[0].password) {
+			
+			this.log("Adding login for " + aAuthInfo.username + " @ " + hostname + " (" + httpRealm + ")");
+
+			var newLogin = Cc["@mozilla.org/passwordmanager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
+			newLogin.init(hostname, null, httpRealm, username, password, "", "");
+
+			this._pwmgr.addLogin(newLogin);
+		} else {
+			this.log("Login unchanged from existing values, no further action needed.");

doesn't this mean that if you enter a different password we don't update the password ever?

+  NS_GetRealPort : function (aURI) {

the NS prefix is meaningless here, just _GetRealPort is preferred, IMO

+	var port = aURI.port;
+
+	if (port != -1) { return port; } // explicitly specified

to recap, this is bad :P

+	// NS_ASSERTION(handler, "IO Service lied");

kill this

+  // From /embedding/components/windowwatcher/public/nsPromptUtils.h
+  // nsIChannel, nsIAuthInformation, boolean, string, int
+  NS_GetAuthHostPort : function (aChannel, aAuthInfo, machineProcessing, host, port) {

we've got a mix of aFoo and foo naming here, aFoo is default style.  Same nit about NS_ vs. _

what is machineProcessing here?  Are host and port needed here (this is a private method in JS...)

+	var uri = aChannel.URI;

is there a perf reason for not just using this directly?

+	// Have to distinguish proxy auth and host auth here...
+	var flags = aAuthInfo.flags;
+
+	if (flags & (Ci.nsIAuthInformation.AUTH_PROXY)) {
+		// TODO: untested...

?


+  // From /embedding/components/windowwatcher/public/nsPromptUtils.h
+  // nsIChannel, nsIAuthInformation
+  NS_GetAuthKey : function (aChannel, aAuthInfo) {

yadda yadda s/_NS/_/ 

+  _bundle : null,
+  getLocalizedString : function (key) {
+
+	if (!this._bundle) {
+		var bunService = Cc["@mozilla.org/intl/stringbundle;1"].getService();
+		bunService.QueryInterface(Ci.nsIStringBundleService);
+		this._bundle = bunService.createBundle("chrome://passwordmgr/locale/passwordmgr.properties");
+
+		if (!this._bundle) { throw "String bundle not present!"; }
+	}
+
+	return this._bundle.GetStringFromName(key);
+  }

there's got to be a better way of reusing boilerplate stuff like this, see my earlier comment about logging

probably XUL pp #include ftw

+  init : function () {
+	this._logins  = new Object();
+	this._disabledLogins = new Object();

why not just have this where its defined? perf?  I don't remember if it matters.

+	// Stash a reference to ourself in the observer object.
+	this.observer._stor = this;

why?  its not immediately obvious this is a good thing.

+        // Log service to use Error Console window.
+        this._logService = Cc["@mozilla.org/consoleservice;1"].getService();
+        this._logService.QueryInterface(Ci.nsIConsoleService);

init as needed, not before

+	// Connect to the correct preferences branch.
+        this._prefService = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService);
+	this._prefService = this._prefService.getBranch("signon.");
+	this._prefService.QueryInterface(Ci.nsIPrefBranch2);
+	this._prefService.addObserver("", this.observer, false);

repeat: observing is bloat for now.  Just check debug in log()

if someone changes the signon filename, that's not supported ;)

+	if (this._prefService.prefHasUserValue("debug")) {
+		this._debug = this._prefService.getBoolPref("debug");
+	}
+	if (this._debug) { this.log("Debugging enabled."); }

I don't think we need this here, like in the other components


+	// Get the SecretDecoderRing for	 crypting the passwords when stored on disk.

fix the s/<tab>/en/ ;)

+	this._decoderRing = Cc["@mozilla.org/security/sdr;1"].getService(Ci.nsISecretDecoderRing);
+
+	// Get the location of the user's profile.
+	if (!this._datapath) {
+		var DIR_SERVICE = new Components.Constructor("@mozilla.org/file/directory_service;1", "nsIProperties");
+		this._datapath = (new DIR_SERVICE()).get("ProfD", Ci.nsIFile).path;
+	}
+
+	if (!this._datafile) {
+		this._datafile = this._prefService.getCharPref("SignonFileName2");
+	}

why are you storing the profile path and and filename and creating new nsILocalFile objects every time you need to do something?

this whole chunk should end up being like the below

  try {
    //xxxmpc: fixme before uploading comment, this isn't quite right
    var DIR_SERVICE = new Components.Constructor("@mozilla.org/file/directory_service;1", "nsIProperties");
    this._signonFile = (new DIR_SERVICE()).get("ProfD", Ci.nsILocalFile);
    var filename = this._prefService.getCharPref("SignonFileName2");
    this._signonFile = this._signonFile.append(filename);
  } catch (ex) {
    this.log("Failed getting profile path.  Error: " + ex);
    return;
  }
 
  if (!this._signonFile.exists()) {
    this.log("SignonFilename2 file does not exist. (file=" + this._datafile + ") path=(" + this._datapath + ")");

    // Try reading the old file
    var importFileName = this._prefService.getCharPref("SignonFileName");
    var oldSignonFile = this._signonFile.parent.Clone();
    oldSignonFile.append("filename");

    if (!oldSignonFile.exists()) {
      this.log("SignonFilename1 file does not exist. (file=" + oldSignonFile.path + ").  " +
               "Creating new signons file...");
      this.writeFile(this._signonFile);
      return;
    }
  }

  // Read in the stored login data.
  if (oldSignonFile) {
    this.log("Importing " + importFile);
    this.readFile(oldSignonFile);

    this.writeFile(this._signonFile);
  } else
    this.readFile(this._signonFile);


+  /*
+   * getAttribute
+   *
+   * Sibling of setAttribute.
+   */

+    
+  /*
+   * setAttribute
+   *
+   * Allows unit tests to force usage of a particular file.
+   *      I was thinking this might be useful for other storage modules (w/o
+   *      defining an extra interface), but I'm not sure that's really useful now.
+   */

If you want to add something so that unit tests/extensions can do alternate things, you could add an "initWithFile" that takes an nsILocalFile, sets this._signonFile and then calls init()

or, alternatively, make readFile kill the in-memory data and reload from disk, and add a method to allow init/reload from disk?


+	for (var i = 0; i < logins.length; i++) {
+		if (logins[i].equals(login)) {
+			logins.splice(i, 1); // delete that login from array.
+			break;
+			// Note that if there are duplicate entries, they'll have to be deleted one-by-one.

hmm, shouldn't we prevent duplicate entries? :)

+  modifyLogin : function (oldLogin, newLogin) {
+	// TODO: this is a bit hacky

sorta, but not really, it probably avoids a lot of useless code.

I'd probably rename to replaceLogin

+  /*
+   * getAllLogins
+   *
+   * Returns an array of nsAccontInfo.

typo?  

+  /*
+   * findLogins
+   *
+   */
+  findLogins : function (count, hostname, formSubmitURL, httpRealm) {
+	var hostLogins = this._logins[hostname];
+	if (hostLogins == null) {
+		count.value = 0; return [];

newline please

+  observer : {

if its internal, use the _ prefix.  That said, I think this actually nets out to "not needed" if all you're doing is observing the debug pref

all of these "internal" methods should be _ prefixed so people don't try to use them elsewhere.

+  // TODO:
+  // We should probably delay loading the file until we actually need the data. This might be a Ts issue (if
+  // we ever measure startup time with a real profile, including sroted passwords). Also, if you have a
+  // master password set you get prompted for it at startup (if you click cancel, you'll have to do so for
+  // every login!)

hmm, I thought we didn't decrypt at startup?  There might be a point in following on the same "init livemarks update after startup is done" discussions happening elsewhere.

+  // Utility function to avoid potential partial writes

isn't this supposed to be handled by nsISafeOutputFileStream?

+  /*
+   * writeFile
+   *
+   */
+  writeFile : function (pathname, filename) {
+	this.log("Writing passwords to " + pathname + "/" + filename);
+
+        var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
+        file.initWithPath(pathname);
+	file.append(filename + "-TEMP");
+
+        var outputStream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream );


use the safeoutputfilestream which should make your own wrapper obsolete...


+  /*
+   * doesFileExist
+   *
+   */
+  doesFileExist : function (filepath, filename) {
+	var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
+	file.initWithPath(filepath);
+	file.append(filename);
+
+	return file.exists();
+  }

this wrapper makes me sad :P

I didn't dig into the unit tests much, but I think I'd ask sayrer to review those anyway.

I'm going to ask gavin to take a second pass here, I think this is pretty solid as is, with my ridiculous density of comments addressed.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Updated patch, addresses mconnor's comments.

This patch addresses mconnor's review comments, except for 2 pending items:

* Renaming nsIPasswordMananger to nsIToolkitPasswordManager
* Switching the various .getService() calls in pwmgr.init() to be getters, so that the service hookup can be delayed until needed.

I'll address those two along with Gavin's review comments.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

I'd just remove the code from the HTTP/FTP channel instead of commenting it out

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Pseudo patch 2

Updated diffs for the code in passwordmgr/resources/content that was moved to passwordmgr/content. (without the rename, so that the changes are obvious)

Comment 15

[Justin Dolske [:Dolske]]

A few follow-up comments to mconnor's review...

(In reply to comment #8)

> 1) Tabs are evil, kill them all.

Tabs converted to spaces. Left some of the tests as-is, because some the ones already checked in used tabs. I'll do spaces in the future.

> +    void findLogins(out unsigned long count, in AString hostname, in AString
> actionURL, in AString httpRealm, [retval, array, size_is(count)] out
> nsILoginInfo logins);
> 
> This feels awkward, why not just return an nsISimpleEnumerator of
> nsILoginInfos?

After follow up discussions, we decided to leave this as-is. This allows natural array usage in C++ and JS. JS unfortunately has to provide an object for the out-param, due to the way the XPCOM/IDL stuff works. We also discussed using an nsIVariant, but felt that ended up obscuring the arg type.

> -        if (!mPassword.IsEmpty()) {
> +        if (PR_FALSE && !mPassword.IsEmpty()) {
> +            // XXX I think the original code here is wrong. A server with a
> +            // temporary glitch shouldn't nuke our stored login.
> 
> file a followup here and leave the behaviour intact.

bug 379997 filed.

> delete, don't comment out, as a general rule.

Right. I've been doing this to make manual reversions easier, but I should just take advantage of cvs...

Comment 16

[Justin Dolske [:Dolske]]

(In reply to comment #9)

> spaces around operators (change this everywhere you didn't do it)

I skipped most of the review comments here, because they applied to the code I wasn't changing. [That's why I attached the pseudo-patch.]

(In reply to comment #10)

> +  log : function (message) {
> +        if (!this._debug) { return; }
> +       dump("Password Manager: " + message + "\n");
> 
> debugging cruft?

After other discussions, we decided this was ok to leave in. The debug logging is off unless you explicitly enable the pref, and the various logging is useful if you're debugging pwmgr.


> +    // TODO: also allow application/xml and text/xml for stupid
> xhtml pages?
> +    // or: if (!domDoc.doctype || domDoc.doctype.name != "HTML" or
> "html") { return 0; }
> 
> I don't know what this is for

Discussed in followups. I checked with Jonas, and he suggested just doing what the original code did (try a QI to nsIDOMHTMLDocument). This check passes when we sometimes don't need it to (like when viewing an image), but those are edge cases.

> how about just returning here, then we can refactor the below a lot more freely
> as follows:
> 
> a) check the username field exists and has a usable value
> b) do validation on the password field values
> c) check for autocomplete="off"
> d) do situation-specific codepaths for 1/2/3 as appropriate
> 
> please do this in two steps, I think this all can be a lot cleaner, but start
> there and I'll look again once you've done cleanup and its less head-wrenching

I basically rewrote all of onFormSubmit() [the code in question here].

> +               // If there are blank passwords, bail out now.
> 
> wouldn't this break sites that have the old-style login/change password like
> old mediawiki?  unless this is backwards compat stuff, I think it's possibly
> scary.

Fixed.

> hrm, don't we just use host for passwords in Fx2?  doesn't this cause problems?

Nope, the code's correct. The format we inherited is rather crummy, though... An HTTP login is stored as "www.site.com:443 (realm string)", while a form login is stored as "http://www.site.com". This is something that future work should probably clean up some more...


> +       /*
> +        * CRIKEY! The original C++ code could really use a cleanup. This
> function is
> +        * just a straight conversion to JS, with a few minor changes when
> interacting
> +        * with parts of pwmgr that have been updated.
> +        */
> 
> ok, file a followup to make this be saner?  the C++ version is fragile...

Bug 380222 filed.

> +   * asks the user to confirm the change.
> +   *
> +   * Return values:
> +   *   0 - Update the stored password
> +   *   1 - Do not update the stored password
> +   */
> 
> if this is what we're doing, do we want to refactor this interaction to
> actually abort the form submission if the user says "No" ?

Hmm, I don't think so. This is what the old code's doing, though, and can be revisited when we change the prompting/notifications later.

Comment 17

[Justin Dolske [:Dolske]]

(In reply to comment #11)

Oops, I missed a few changes from this comment, I'll include them when I address stuff from Gavin's review.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

>Index: browser/base/content/browser.js

>+  // Ensure password manager is up and running.
>+  Cc["@mozilla.org/passwordmanager;2"].getService(Ci.nsIPasswordManager);

Didn't we used to initialize the password manager based on whether a page had password inputs? Sounds like this might regress Ts, even if only artificially.

>Index: browser/base/content/sanitize.js

>     passwords: {
>       clear: function ()
>       {

>+	var logins = pwmgr.getAllLogins({});

Oh my gosh, tabs! One here and in a few other places in this patch, please fix before landing.
I wonder if nsIPasswordManager should have a clearAll() method.

>+        for (var i = 0; i < logins.length; i++) {
>+          pwmgr.removeLogin(logins[i]);

I kinda prefer a |for each| loop in cases like these, to avoid the need for the counter variable, but not a big deal (and unneeded if you add a clearAll, though this does apply to other places in this patch).

>Index: netwerk/base/public/nsIPasswordManager.idl
>Index: netwerk/base/public/nsIPasswordManagerInternal.idl
>Index: netwerk/base/public/nsIPasswordManagerUtils.h

As mentioned on IRC, I don't think the removal of these interfaces should be made in this patch. They'll break SeaMonkey unnecessarily, and can be done separately at little cost (you could even just move them into /wallet once bug 379997 is fixed). We should add your new interfaces with a different name (nsILoginManager perhaps, as discussed on IRC), and leave these as-is.

>Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp
>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

These changes were moved to bug 379997 and are already landed, yay!

>Index: toolkit/components/passwordmgr/Makefile.in

>-DIRS = base resources
>+DIRS = public src content

This is now bug 380650, ready for seperate landing now that the CVS copies are done.

>Index: toolkit/components/passwordmgr/resources/content/passwordManager.js

>@@ -198,18 +150,17 @@ function AskUserShowPasswords() {

> function ConfirmShowPasswords() {
>   // This doesn't harm if passwords are not encrypted
>-  var tokendb = Components.classes["@mozilla.org/security/pk11tokendb;1"]
>-                    .createInstance(Components.interfaces.nsIPK11TokenDB);
>+  var tokendb = Components.classes["@mozilla.org/security/pk11tokendb;1"].createInstance(Components.interfaces.nsIPK11TokenDB);

Why this change?

>Index: toolkit/components/passwordmgr/resources/content/passwordManagerCommon.js

> function Startup() {
>   // xpconnect to password manager interfaces
>-  passwordmanager = Components.classes["@mozilla.org/passwordmanager;1"].getService(Components.interfaces.nsIPasswordManager);
>+  passwordmanager = Components.classes["@mozilla.org/passwordmanager;2"];
>+  passwordmanager = passwordmanager.getService(Components.interfaces.nsIPasswordManager);

I don't really see the point in splitting these two calls, you can just wrap the line if you're worried about the line length.

>@@ -78,23 +79,23 @@ function Shutdown() {
> function CompareLowerCase(first, second) {

This function should just be using String.localeCompare, I think (I know you didn't write this code, but might as well fix it while you're here).

>Index: toolkit/components/passwordmgr/resources/content/passwordManagerExceptions.js

>@@ -68,23 +68,20 @@ var rejectsTreeView = {

>+  var logins = passwordmanager.getAllDisabledLogins({});
>+  for (var count = 0; count < logins.length; count++) {
>+    var host = logins[count];
>+    rejects[count] = new Reject(count, host);

rejects = logins.map(function(host, i) { return new Reject(i, host) }) // ?
(this also highlights how it's weird to have this method use the term "logins" when it really means "hosts")

>Index: toolkit/components/passwordmgr/public/nsILoginInfo.idl

>+ * The Initial Developer of the Original Code is Mozilla.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Justin Dolske <[e-mail]>

I think the initial developer should be "Mozilla Corporation", and you should add an "(original author)" after your name in the contributor list (same for the other files you're creating from scratch).

>+interface nsILoginInfo : nsISupports {
>+    /**
>+     * The hostname the login applies to.
>+     *
>+     * For logins obtained from HTML forms, the login is formatted like
>+     * a URL. For example: "http://www.site.com" A port number (":123")
>+     * may be appended in some cases.
>+     *
>+     * For logins obtained from a HTTP or FTP protocol authentication, 
>+     * the hostname is not a URL format, but does always have the port
>+     * number appended. For example "www.site.com:80".
>+     */
>+    attribute AString hostname;

Worth filing a bug on the disparity between forms/HTTP-FTP, depending on the storage format change, maybe?

>+%{C++
>+
>+// {0f2f347c-1e4f-40cc-8efd-792dea70a85e}
>+#define NS_LOGININFO_CID { 0x0f2f347c, 0x1e4f, 0x40cc, { 0x8e, 0xfd, 0x79, 0x2d, 0xea, 0x70, 0xa8, 0x5e } }

It's probably not a good idea to use the same UUID for both the CID and the IID. I don't think there's much of a point of defining the CID here, either, but I guess it doesn't really hurt.

>Index: toolkit/components/passwordmgr/public/nsIPasswordManager.idl

>+interface nsIPasswordManager : nsISupports {

>+    /**
>+    * Fetch all logins in the password manager. An array is always returned,
>+    * if there are no logins the array is empty.

>+    void getAllLogins(out unsigned long count,
>+                      [retval, array, size_is(count)] out nsILoginInfo logins);

>+    /**
>+    * Obtain a list of all hosts for which password saving is disabled.

>+    void getAllDisabledLogins(out unsigned long count,
>+                      [retval, array, size_is(count)] out wstring hostnames);

The the naming of this method makes me think it will return "disabled" nsILoginInfos (even though that doesn't really make sense), given it's similarity to getAllLogins(). Perhaps something more like "getAllDisabledHosts"?

>+    /**
>+    * Search for logins matching the specified criteria.
>+    *
>+    * @param count
>+    * @param aHostname
>+    * @param aActionURL
>+    * @param aHttpRealm
>+    * @param logins
>+    *
>+    */
>+    void findLogins(out unsigned long count, in AString aHostname,
>+                    in AString aActionURL,   in AString aHttpRealm,
>+                    [retval, array, size_is(count)] out nsILoginInfo logins);

This could use some documentation - from looking at the implementation it looks like none of the parameters are optional (e.g. no way to use this to find logins with matching hostname and actionURL, but not HTTP realm), so this can only find exact duplicates (or mostly-duplicates - not sure how else they'd differ, typically).

>+    /**
>+    * Generate results for a userfield autocomplete menu.
>+    *
>+    * NOTE: This interface is provided for use only by the FormFillController,
>+    *       which calls it directly. This isn't really ideal, it should
>+    *       probably be callback registered through the FFC.

In other words, pwmgr should implement nsIAutoCompleteSearch, right? Is there a reason that's hard to do right now (not that I want you to do that as part of this patch!)?.

>+// {cb9e0de8-3598-4ed7-857b-827f011ad5d8}
>+#define NS_PASSWORDMANAGER_CID { 0xcb9e0de8, 0x3598, 0x4ed7, { 0x85, 0x7b, 0x82, 0x7f, 0x01, 0x1a, 0xd5, 0xd8 } }

Again, don't use the same UUID for the interface and the implementation, that's just confusing.

>Index: toolkit/components/passwordmgr/public/nsIPasswordManagerStorage.idl

It might be worth mentioning that this interface is not meant to be used outside of the password manager and testing/migration, right? Users might see e.g. "removeLogin" and be tempted to use this instead of nsIPasswordManager. Some of the comments about nsIPasswordManager apply to this interface as well, for the identically-named methods.

>+interface nsIPasswordManagerStorage : nsISupports {
>+    void init();
>+    AString getAttribute(in AString propertyName);
>+    void setAttribute(in AString propertyName, in AString propertyValue);

These need docs (looks like the last one is for testing only?).

>Index: toolkit/components/passwordmgr/src/Makefile.in

>+MODULE = passwordmgr
>+
>+EXTRA_COMPONENTS =	nsPasswordManager.js \
>+			nsPasswordManagerPrompter.js \
>+			storage-Legacy.js \

Naming here seems a bit inconsistent (the others all start with nsPassword), but up to you if you actually care enough to change this.

>Index: toolkit/components/passwordmgr/src/nsLoginInfo.js

Given that this interface's getters don't do anything special to any kind of validation or processing on get/set, you can just remove all the _properties and getters/setters, and just use constructor-inited properties directly.

>+    init : function (hostname, formSubmitURL, httpRealm, username, password,
>+                     usernameField, passwordField) {

I'm a fan of "a" prefixed argument names, (e.g. aHostname, etc), you seem to do this consistently in the other places.

>+// Boilerplate code for component registration...
>+var gModule = {

>+    unregisterSelf: function(componentManager, fileSpec, location) {},

I'm not sure that not implementing unregisterSelf won't cause problems (leaks, maybe?). Was this boilerplate copied from somewhere else?

>+    _objects: {
>+        service: {
>+            CID : Components.ID("{0f2f347c-1e4f-40cc-8efd-792dea70a85e}"),
>+            contractID : "@mozilla.org/passwordmanager/loginInfo;1",
>+            className  : "LoginInfo",
>+            factory    : LoginInfoFactory = {
>+                createInstance: function(aOuter, aIID) {
>+                    if (aOuter != null)
>+                        throw Components.results.NS_ERROR_NO_AGGREGATION;
>+                    var svc = new nsLoginInfo();
>+                    return svc.QueryInterface(aIID);
>+                }
>+            }
>+        }
>+    },

When I was debugging search service leaks a while ago, I ran in to leak issues when nsIFactory was implemented on the same object as nsIModule, but I don't really remember the details. Don't have to change it just because of my bad memories, but keep it in mind should you find that this patch causes an increase in leakage (it almost certainly will, to some degree, because of JS component loader suckiness - not much you can do about that).

>+                    if (this.singleton == null) {
>+                        var svc = new PasswordManager();
>+                        this.singleton = svc;
>+                        svc.init();
>+                    } else {
>+                        svc = this.singleton;
>+                    }

I don't think trying to keep this  a singleton here by storing the object actually has any effect, other than perhaps in the case of someone calling createInstance() - XPCOM takes care of that on it's own, I believe. It's probably a better idea to call init() from the object's constructor, rather than from here.

>Index: toolkit/components/passwordmgr/src/nsPasswordManagerPrompter.js

>+PasswordManagerPrompter.prototype = {

>+    _debug         : true, // XXX get from pref?

This is going to be changed to false before landing, right?

>+    NS_GetAuthKey : function (aChannel, aAuthInfo) {

>+        var host, port = -1;
>+
>+        [host, port] = this.NS_GetAuthHostPort(aChannel, aAuthInfo, true);

Is there any point in assigning a default value to port if it's just going to be overridden, right?

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js

>+PasswordManagerStorage_legacy.prototype = {

>+    _disabledLogins : null,

Same comment about the naming, s/logins/hosts/ or something like that.

>+    init : function () {
>+        this._logins  = new Object();
>+        this._disabledLogins = new Object();

nit: prefer the object literal initialization, so e.g. |this._logins = {};|.

>+        // Stash a reference to ourself in the observer object.
>+        this.observer._stor = this;

References between all these components seem to be common, is the ownership graph clear? I'm afraid of reference cycle leaks (the cycle collector might help in some cases, but I don't think all the relevant objects are added to it yet, and I'm not sure of the status of the JS object graphs traversal stuff).

>+        // Connect to the correct preferences branch.
>+        this._prefService = Cc["@mozilla.org/preferences-service;1"]
>+                                .getService(Ci.nsIPrefService);
>+        this._prefService = this._prefService.getBranch("signon.");
>+        this._prefService.QueryInterface(Ci.nsIPrefBranch2);
>+        this._prefService.addObserver("", this.observer, false);

nit: Looks like this should be named "_prefBranch" rather than "_prefService".

>+        // Get the location of the user's profile.
>+        if (!this._datapath) {
>+            var DIR_SERVICE = new Components.Constructor(
>+                    "@mozilla.org/file/directory_service;1", "nsIProperties");
>+            this._datapath = (new DIR_SERVICE()).get("ProfD", Ci.nsIFile).path;
>+        }
>...
>+        var importFile = null;
>+        if (!this.doesFileExist(this._datapath, this._datafile)) {
>...
>+        // Read in the stored login data.
>...

Is there a reason this block deals with file paths rather than nsIFiles? doesFileExist is so ugly, why is it needed?

>+    getAttribute : function (attribute) {

>+        switch (attribute) {
>+            case "datafile":

>+            case "datapath":

Could you not just expose a "dataFile" nsIFile?

>+     * setAttribute
>+     *
>+     * Allows unit tests to force usage of a particular file.
>+     * XXX remove me

Do we really plan on removing this? When? When we get rid of unit tests? ;)

>+    addLogin : function (login) {

>+        if (!this._logins[key])
>+            this._logins[key] = new Array();

nit: [] instead of |new Array()|.

>+     * getAllLogins
>+     * Returns an array of nsAccontInfo.

nit: typo

>+    getLoginSavingEnabled : function (hostname) {
>+        if (this._disabledLogins[hostname])
>+            return false;
>+
>+        return true;

|return !this.disabledLogins[hostname];|

>+    observer : {

>+        observe : function (subject, topic, data) {

>+                if (topic == "nsPref:changed") {
>+                    if (prefName == "debug") {
>+                        // The debug pref is hidden (no default)

I don't really like prefs with no defaults - if you're adding a pref for something you might as well make it easy to change for those who know how.

>+                    } else if (prefName == "SignonFileName2") {
>+                        // TODO if the filename pref changes, should we read
>+                        // it first (and reset current data?), or just
>+                        // overwrite?

The current code doesn't support live editing of this pref, so I don't see why we should. Seems to add unneeded complexity. And if the only other reason this observer exists is for the debug pref, I'd say we should probably just remove it - how often do you really need live debugging output toggling? How often are we even expecting to debug this, long term? :)

>+    readFile : function (pathname, filename) {

>+        var inputStream = Cc["@mozilla.org/network/file-input-stream;1"]
>+                                .createInstance(Ci.nsIFileInputStream);
>+        inputStream.init(file, 0x01, 0600, null); // RD_ONLY

The old code seems to default to sending "-1" for the permissions, which means "use the default", which eventually translates into "0". Not sure that it makes a difference for a readonly stream, but might as well use the same here, I figure.

>+        do {
>+             var hasMore = lineStream.readLine(line);

nit: indent off here by 1 space.

>+    writeLine : function(outputStream, data) {
>+    writeFile : function (pathname, filename) {

As mconnor mentioned you should be able to simplify these by using safe output streams.

>+        for (var hostname in this._logins) {
>+            function sortByRealm(a,b) {

Can this just use String.localeCompare()? Not sure how that handles null (also not sure why that method isn't on devmo).

>+            for (i = 0; i < this._logins[hostname].length; i++) {
>+                var login = this._logins[hostname][i];

|for each| loop?

>+    encrypt : function (plainText) {
>+        var cipherText = this._decoderRing.encryptString(plainText);
>+        return cipherText;

nit: Collapse to a single line return, avoid the temporary?

>+    decrypt : function (cipherText) {
>+        var plainText = null;
>+
>+        // TODO is there any point in supporting a pre-Mozilla-1.0 file format?
>+        if (cipherText.charAt(0) == '~') {
>+            // base64 encoded string: ignore leading '~' and decode the rest.
>+            throw "!!!!! base64 encoded passwords not supported !!!!!";
>+            return plainText;

This looks wrong - remove the comment and the return if you're just going to throw. I think it's fine to not support the old file format.

>+    doesFileExist : function (filepath, filename) {

>Index: toolkit/components/passwordmgr/test/unit/test_storage_legacy_1.js
>+const TESTDIR =  "/Users/dolske/ff/ff30/mozilla/toolkit/components/passwordmgr/test/unit/data";

>Index: toolkit/components/passwordmgr/test/unit/test_storage_legacy_2.js
>+const TESTDIR =  "/Users/dolske/ff/ff30/mozilla/toolkit/components/passwordmgr/test/unit/data";

I suspect this won't work so great on the tinderbox :)

Comment 19

[Justin Dolske [:Dolske]]

(In reply to comment #18)
> >Index: browser/base/content/browser.js
> 
> >+  // Ensure password manager is up and running.
> >+  Cc["@mozilla.org/passwordmanager;2"].getService(Ci.nsIPasswordManager);
> 
> Didn't we used to initialize the password manager based on whether a page had
> password inputs? Sounds like this might regress Ts, even if only artificially.

Yeah, that was done for form elements. I was thinking that wasn't enough for some reason, but I can't remember why. I'll revert to that and see if everything works as expected.

> I wonder if nsIPasswordManager should have a clearAll() method.

I think it should. Less chance for mistakes, and some providers (Keychain, maybe?) might make it difficult to get some logins without authenticating, even though you're just doing to turn around and delete them.

> It's probably not a good idea to use the same UUID for both the CID and the
> IID.

Hmm, good catch. Dunno why I did that.

> The the naming of this method makes me think it will return "disabled"
> nsILoginInfos (even though that doesn't really make sense), given it's
> similarity to getAllLogins(). Perhaps something more like
> "getAllDisabledHosts"?

Yeah, that's confusing. I'll switch it.

> >+    /**
> >+    * Generate results for a userfield autocomplete menu.
> >+    *
> >+    * NOTE: This interface is provided for use only by the FormFillController,
> >+    *       which calls it directly. This isn't really ideal, it should
> >+    *       probably be callback registered through the FFC.
> 
> In other words, pwmgr should implement nsIAutoCompleteSearch, right? Is there a
> reason that's hard to do right now (not that I want you to do that as part of
> this patch!)?.

I was just minimizing changes... I think the right way to do this is for code to register something with the FormFillController, and them the FormFillController uses/calls that something to get the autocomplete items. 

> >Index: toolkit/components/passwordmgr/public/nsIPasswordManagerStorage.idl
> 
> It might be worth mentioning that this interface is not meant to be used
> outside of the password manager and testing/migration, right? 

Right. In the future, there could easily be multiple implementations of this interface, and code should allow password manager to determine which one(s) are configured to be used.

> >+// Boilerplate code for component registration...
> >+var gModule = {
> 
> >+    unregisterSelf: function(componentManager, fileSpec, location) {},
> 
> I'm not sure that not implementing unregisterSelf won't cause problems (leaks,
> maybe?). Was this boilerplate copied from somewhere else?

I think it came from DevMo or another component, I'll have to track it down.

> I don't really like prefs with no defaults - if you're adding a pref for
> something you might as well make it easy to change for those who know how.

I didn't see any other ".debug" prefs, and was thus wary of adding one.

Comment 20

[Justin Dolske [:Dolske]]

Attachment: Updated to address more of mconnor's comments

Just a checkpoint... Comments from gavin not addressed here.

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Patch addressing Gavin's comments, for checkin

Patch addressing Gavin's comments, and other nits we found while testing builds.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I've landed a local version the last patch, with r=me and r=mconnor. Still need to remove the following files when it's clear this doesn't need to be backed out:

toolkit/components/passwordmgr/base/Makefile.in
toolkit/components/passwordmgr/base/nsIPassword.idl
toolkit/components/passwordmgr/base/nsIPasswordInternal.idl
toolkit/components/passwordmgr/base/nsPasswordManager.cpp
toolkit/components/passwordmgr/base/nsPasswordManager.h
toolkit/components/passwordmgr/base/nsSingleSignonPrompt.cpp
toolkit/components/passwordmgr/base/nsSingleSignonPrompt.h

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Dolske's going to be filing bugs for some remaining issues some time soon, and any noticed regressions should be filed as a new bug blocking this one.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

This introduced some leaks on fxdebug-linux-tbox:

--NEW-LEAKS-----------------------------------leaks------leaks%
nsPrefBranch                                    120     100.00%
nsVoidArray                                       8     100.00%
XPCWrappedNative                               1512      12.50%
XPCWrappedNativeProto                           672       9.09%
nsStringBuffer                                  104       8.33%
TOTAL                                          2416

If you compare with bug 378618 comment 2, I think this means that the XPCWrappedNative[Proto] and nsStringBuffer leaks are just the fault of the JS component loader, while the nsPrefBranch/nsVoidArray leaks were caused by this added code.

Comment 25

[Robert Kaiser]

(In reply to comment #18)
> They'll break SeaMonkey unnecessarily, and can be done
> separately at little cost (you could even just move them into /wallet once bug
> 379997 is fixed).

Just as a note, we'll be soon switching SeaMonkey to toolkit (bug 328887), and will be looking into killing wallet in favor of satchel and this pwdmgr as well (bug 304309), pending some autocomplete problems.

Comment 26

[Bill Gianopoulos [:WG9s]]

Since this landed, login to Yahoo! Mail via the Yahoo! Mail Notifier extension Notifier no longer seems to work correctly.  If you have a saved password it logs in just fine, but if you logout and try to re-login re specifying the password, the login attempt fails.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #26)
> Since this landed, login to Yahoo! Mail via the Yahoo! Mail Notifier extension
> Notifier no longer seems to work correctly.  If you have a saved password it
> logs in just fine, but if you logout and try to re-login re specifying the
> password, the login attempt fails.

The extension probably tries to use one of the interfaces that we've changed. File a new bug and contact the authors, I guess?

Comment 28

[Steve England [:stevee]]

On Linux fxdbug-linux-tbox Dep RLk has jumped from 4.88KB to 5.17KB

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Attachment: don't forget the installer manifests

Would you believe I've managed to forget about the installer manifests, again? :/ Also changes the module name to not conflict with the existing passwordmgr.

Comment 30

[Michael Kraft [:morac]]

First off this will break a number of addons that reference nsiPasswordManager to store passwords (including mine as seen from comment #26 above).  It also means that in order for an addon to support Firefox 2.0, Firefox 3.0 and other Mozilla based browsers there needs to be processing to use both PasswordManager and LoginManager.  Is there a way to leave the PasswordManager API hooks in and just redirect them to the new LoginManager?

Second off, I installed the Minefield nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070516 Minefield/3.0a5pre) which contains the patch here and the new Password Manager isn't working.  It's throwing up a bunch of exceptions which makes it look like the nsILoginManager component never made it into the nightly update for Minefield.

Error: Cc['@mozilla.org/login-manager;1'] has no properties
Source File: chrome://browser/content/browser.js
Line: 820

Error: BMSVC has no properties
Source File: chrome://browser/content/browser.js
Line: 89

Error: Components.classes['@mozilla.org/login-manager;1'] has no properties
Source File: chrome://passwordmgr/content/passwordManagerCommon.js
Line: 24

Error: gURLBarAutoFillPrefListener has no properties
Source File: chrome://browser/content/browser.js
Line: 980

Comment 31

[Asaf Romano (gone)]

Or you could run a check on Components.classes.

I would bet those errors are a result of the missing installer manifests update.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #22)
> I've landed a local version the last patch, with r=me and r=mconnor. Still need
> to remove the following files when it's clear this doesn't need to be backed
> out:

I've done that, now.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

I've also reverted an unintentional change to nsHTMLFormElement.cpp.

Comment 34

[Nickolay_Ponomarev]

dev-doc-needed: overview of the new interfaces and the migration from the old interface to the new ones is needed at least. Also the existing password manager documentation should be updated.

Comment 35

[Michael Kraft [:morac]]

I agree the documentation should be updated since when I converted my extension to use the new LoginManager component, I ran into a number of snags whose solutions weren't obvious from looking at the API.  The most annoying was the fact that now in order to remove a login, the original password must be specified in the removeLogin() call.  It's easy to get around this by retrieving the password first using the findLogins() call, but it was counter-intuitive especially since the old method for removing a login only required a username and URI/host.

Also I don't think the existing password manager documentation should be deleted since it will still be needed for compatibility purposes.

For now though, all the public APIs are listed in the files in the following directory:

http://lxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/public/

Comment 36

[Nickolay_Ponomarev]

> I agree the documentation should be updated since when I converted my extension
> to use the new LoginManager component, I ran into a number of snags whose
> solutions weren't obvious from looking at the API.

It would be appreciated if you took time to put your findings on developer.mozilla.org. Any format will do (code snippets, a short FAQ, whatever), as it's easier to improve existing docs than to document from scratch.

> The most annoying was the
> fact that now in order to remove a login, the original password must be
> specified in the removeLogin() call.

It looked weird to me too when I skimmed over the APIs. Justin, is this by design or a bug?

> Also I don't think the existing password manager documentation should be
> deleted since it will still be needed for compatibility purposes.
> 
Yes, I didn't suggest it should be deleted, but it should be made clear that there's the new way of doing things in Fx3+

Comment 37

[Justin Dolske [:Dolske]]

(In reply to comment #35)

>  The most annoying was the
> fact that now in order to remove a login, the original password must be
> specified in the removeLogin() call.

If you don't already have the nsILoginInfo object from a previous login manager call, how you you know that login exists? [Or otherwise know the detail of the login you want to delete?]

Comment 38

[Michael Kraft [:morac]]

(In reply to comment #36)
> It would be appreciated if you took time to put your findings on
> developer.mozilla.org. Any format will do (code snippets, a short FAQ,
> whatever), as it's easier to improve existing docs than to document from
> scratch.
> 

Okay I wrote up some documentation on how to use the LoginManager over at
http://developer.mozilla.org/en/docs/Using_nsILoginManager

I also populated the nsILoginManager and nsILoginInfo page with information,
though the nsILoginManager basically just refers to the Using nsILoginManager
page.

Comment 39

[Michael Kraft [:morac]]

(In reply to comment #37)
> If you don't already have the nsILoginInfo object from a previous login manager
> call, how you you know that login exists? [Or otherwise know the detail of the
> login you want to delete?]
> 

In my case, my extension stores password information locally. The hostname and submitURL are always the same (hard coded to the chrome URL of my extension) and only the username changes.  So I don't need to know what the old value contains since the only true variable is the user name.  

My old method was to just call the remove for the username with a try/catch clause around it to cover the case where the password didn't exist.

The new method requires me to make a call to findLogins, then search for the matching username and then immediately do a removeLogin on it which is kind of silly.

I know of a few other extensions who do something similar.

Comment 40

[Sebastian Tschan]

With the old password manager (nsIPasswordManager) the Master Password had only been required if the username or password had to be accessed. This way you could collect existing password objects and display the number of login possibilities to the user without requiring the Master Password.
This is very useful for the Secure Login extenions, e.g.:
https://addons.mozilla.org/de/firefox/addon/4429

With the new login manager (nsILoginManager), you have to enter the Master Password as soon as the findLogins method is called.
I think it's a security enhancements not to ask for the Master Password until the login credentials are filled in.
This would also affect the standard password autofill of Firefox Password Manager, e.g. for more than one saved user+pass combination.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #40)
> With the new login manager (nsILoginManager), you have to enter the Master
> Password as soon as the findLogins method is called.

This is addressed by bug 381164, I think. Could you test the patch there?

Comment 42

[Sebastian Tschan]

The provided patch (Patch for review, v.2) fixes the findLogins issue (findLogins method can be called without requiring the Master Password now).
But its still not possible to access a nsiLoginInfo object without the Master Password.
In my opinion it would be best to keep user+pass in encrypted state even if a nsLoginInfo object is returned and only decrypt if user or pass is accessed.

Comment 43

[Martijn Wargers (dead)]

+  target->AddEventListener(NS_LITERAL_STRING("pagehide"),
+                           NS_STATIC_CAST(nsIDOMFocusListener *, this),
+                           PR_TRUE);

Shouldn't that be nsIDOMLoadListener?
(although I can't seem to find any pageshow/pagehide at
http://lxr.mozilla.org/seamonkey/source/dom/public/coreEvents/ )

Comment 44

[Martijn Wargers (dead)]

Ok, according to bug 360847, apparently nsIDOMEventListener should be used.

Comment 45

[Justin Dolske [:Dolske]]

That code was basically just a cut-n-paste from the old password manager into satchel. I'm not sure why you're asking about it, but if there's some reason to change it file a new bug for that issue.

Comment 46

[Martijn Wargers (dead)]

Sorry, never mind, it just struck me as odd that nsIDOMFocusListener was used for  a pagehide event.

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

it seems the cast was just used to avoid the ambiguity when casting to nsIDOMEventListener. any of the interfaces works equally well there.

Comment 48

[Eric Shepherd [:sheppy]]

This was documented a while back.

Comment 49

[[e-mail]]

Comment on attachment 264981 [details] [diff] [review]
Patch addressing Gavin's comments, for checkin

s/existant/existent/g