We should remove support for importing from the legacy signons.txt format. Also, we should drop support for legacy logins "encrypted" with base64. We stopped creating signons.txt format files after bug 288040 landed (Firefox 3.5 alpha 2). And since bug 316084 landed (Firefox 3.5 beta 4), we have reencrypted any existing base64 logins to the current SDR (SecretDecoderRing) format. Ergo, anyone who has run any Firefox release since 3.5 has already imported logins from signons.txt. And any Firefox user who has added/changed a login since Firefox 3.5 has reencrypted any base64 logins that were previously imported. (Other apps were slower to switch to the Toolkit password manager, but it's been long enough.) Removing this code will simply things in preparation for a async password manager implementation.

What about Thunderbird & Seamonkey? It looks like both had Gecko 1.9.1 releases in late 2009 (3.0 & 2.0 respectively), so should have been importing since then. Both are sort of on the rapid release train. I agree that we should remove support, but just wanted to put this out there before making the decision.

(In reply to Justin Dolske [:Dolske] from comment #0) > We should remove support for importing from the legacy signons.txt format. We should at least delete signons.txt. We shouldn't keep an unused file with the user's passwords in it in the profile.

First pass, haven't cleaned up the test stuff yet.

Seems to work locally, let's see what Try thinks. https://tbpl.mozilla.org/?tree=Try&rev=c75fa8069518

Try's green on desktop and B2G. Android 2.2 rc2 had 2 failures in testMasterPassword... But there are a bunch of orange bugs on file for them, and rc2 was green on Android 4.0. I've retriggered the 2.2 rc2 to see if it's intermittent or not.

Comment on attachment 811467 [details] [diff] [review] Patch v.1 Review of attachment 811467 [details] [diff] [review]: ----------------------------------------------------------------- It looks like the failure wasn't intermittent. ::: toolkit/components/passwordmgr/test/unit/head_common.js @@ +11,5 @@ > /* > * initStorage > * > */ > + //initStorage : function (aInputPathName, aInputFileName, I think this comment can go. @@ +12,5 @@ > * initStorage > * > */ > + //initStorage : function (aInputPathName, aInputFileName, > + // XXX do we need a form to make a copy of an input DB, and use that? I'm not sure what this comment is about. @@ +228,5 @@ > var PROFDIR = do_get_profile(); > var DATADIR = do_get_file("data/"); > // string versions... > var OUTDIR = PROFDIR.path; > +var INDIR = DATADIR.path; // XXX kill me Looks like this can be removed now.

Geoff (or Paul) -- can either of you shed some light onto what's happening with the android test failures? (see comment 4/5). For posterity, here's the error for Android 2.2 rc2. [Android 4.0 rc3 fails the same way with the same test.] Robocop derived process name: org.mozilla.fennec INFO | automation.py | Application pid: 0 0 INFO SimpleTest START 1 INFO TEST-START | testMasterPassword EventExpecter: no longer listening for Gecko:Ready waitForText timeout on ^Settings$ 2 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until both fields are filled 3 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until the Confirm password field is filled 4 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until both fields contain the same password 5 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until the Password field is filled 6 INFO TEST-PASS | testMasterPassword | Checking if no password was set if the action was canceled - No password was set 7 INFO TEST-PASS | testMasterPassword | waiting to convert the letters in dots - The letters are converted in dots 8 INFO TEST-PASS | testMasterPassword | Checking if Use master password is present - Use master password is present 9 INFO TEST-PASS | testMasterPassword | Checking if the password is enabled - The password is enabled 10 INFO TEST-PASS | testMasterPassword | waiting for urlbar text to gain focus - urlbar text gained focus 11 INFO TEST-PASS | testMasterPassword | URL typed properly - http://mochi.test:8888/tests/robocop/robocop_login.html should equal http://mochi.test:8888/tests/robocop/robocop_login.html EventExpecter: no longer listening for DOMContentLoaded 12 INFO TEST-PASS | testMasterPassword | Doorhanger notification is displayed - true should equal true 13 INFO TEST-PASS | testMasterPassword | Checking if Save option is present - Save option is present Exception caught during test! junit.framework.AssertionFailedError: Button with text: 'OK' is not found! http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testMasterPassword.java.in The test appears to be enabling a master password, loading a page, and then checking to see the "save password" prompt comes up for the page. I'm a little confused, because it seems that the test is finding the notification but not an OK button. So what's happening? Does Robocop do any kind of profile initialization? Or otherwise interact with password manager? It's surprising that the patch here would have any impact on a master password test.

(In reply to Matthew N. [:MattN] (away Sep. 16 - Oct. 2; limited avail. through Oct. 7) from comment #6) > > + //initStorage : function (aInputPathName, aInputFileName, > > + // XXX do we need a form to make a copy of an input DB, and use that? > > I'm not sure what this comment is about. 'Twas just a note to myself, wasn't initially sure if the tests were going to need the ability to initialize from a pre-existing .sqlite file (as they could from a pre-existing signons.txt file). Turns out it wasn't needed.

Fixed nits.

I can look into the testMasterPassword failure, but it may take be a couple of days to get around to it. Robocop, like most of our test suites, creates and uses a "fresh" profile for the browser, with a few customizations (mostly prefs).

Comment on attachment 811467 [details] [diff] [review] Patch v.1 Review of attachment 811467 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ +1486,5 @@ > "chromeappsstore.sqlite"); > OS.File.remove(path); > } > > + // XXX remove old signons.txt files? Are you planning on doing this in a separate bug? I think they should be removed.

Comment on attachment 814235 [details] [diff] [review] Patch v.2 r=me but see comment 11.

I verified the testMasterPassword failure. The change in behavior can be seen without running any Robocop test: - Uninstall/re-install Fennec to get a clean profile. - Start Fennec - Select Settings > Privacy > Use master password - Enter and confirm a password. Press OK. - Load robocop_login.html (see https://hg.mozilla.org/mozilla-central/file/64b497e6f593/mobile/android/base/tests/robocop_login.html; robocop_login.html references robocop_blank_01.html, so have that available at the same location) - A doorhanger is displayed with options Save / Don't save - Select "Save" on the doorhanger - Without your patch, a prompt queries for the master password and has options OK / Cancel * With your patch, no such prompt appears!

I ran through the procedure with signon.debug=true and collected the logcat - useful?

Comment on attachment 814638 [details] logcat with signon.debug=true >10-08 16:52:08.000 I/Gecko ( 8407): Login Manager: Searching for logins matching host: file:// formSubmitURL: file:// httpRealm: null >10-08 16:52:08.102 I/Gecko ( 8407): PwMgr mozStorage: _searchLogins: returning 0 logins >10-08 16:52:08.102 I/Gecko ( 8407): PwMgr mozStorage: _findLogins: returning 0 logins >10-08 16:52:08.104 I/Gecko ( 8407): Login Manager: Adding login >10-08 16:52:08.124 I/Gecko ( 8407): PwMgr cryptoSDR: SDR slot status is 4 >10-08 16:52:09.016 I/Gecko ( 8407): PwMgr cryptoSDR: SDR slot status is 4 >10-08 16:52:09.080 I/Gecko ( 8407): PwMgr mozStorage: Creating new statement for query: INSERT INTO moz_logins (hostname, httpRealm, formSubmitURL, usernameField, passwordField, encryptedUsername, encryptedPassword, guid, encType, timeCreated, timeLastUsed, timePasswordChanged, timesUsed) VALUES (:hostname, :httpRealm, :formSubmitURL, :usernameField, :passwordField, :encryptedUsername, :encryptedPassword, :guid, :encType, :timeCreated, :timeLastUsed, :timePasswordChanged, :timesUsed) So, that's the addLogin() call succeeding (from clicking "Save"). It's checking to look for duplicates, sees it's a new login, and successfully encrypts and adds it to the DB. "SDR slot status is 4" == nsIPKCS11Slot.SLOT_LOGGED_IN, which is what's expected when the master password has already been entered. Hmm.

Fascinating. I can reproduce this on desktop. I wonder if I fixed a bug -- when you set a master password in a session, I _think_ you're supposed to be logged in for that session (since you obviously know the master password!), in which case the MP prompt shouldn't even be shown. So this patch should be correct, but that leaves me wondering why this was working differently before.

Ok... The now-removed _importLegacySignons() always created an instance of the legacy storage module, and let it figure out if the relevant files existed or not (instead of duplicating code to see if signons.txt existed, and skipping import if it didn't). A side effect of spinning up the legacy module is that doing so would initialize the PKCS#11 token DB (aka softtoken, aka key3.db, aka the master password). The sqlite storage uses nsILoginManagerCrypto (lazily) and that module in turn will, when spun up, handle the PKCS#11 token DB initialization. This can happen much later in a session. I'm still not sure exactly how this ends up triggering the problem, but I can restore the previous behavior by adding a simple |this._crypto| call in storage-mozStorage.js's init() to force creation of the crypto module (and initialization of the token DB, as _importLegacySignons() was implicitly doing). We've had bugs on this kind of dumbness before, most recently bug 922034. Edge cases like this seem to cause the softtoken to be a little confused about its login state. I'm inclined to just add this workaround for now, and leave a deeper (and mostly unneeded) fix for another time.

FTR, my manual test for with/without patch was: 0) Start browser in new profile 1) Load accounts.google.com (password field ensures loginmgr & storage are created) 2) Set a master password 3) Reload accounts.google.com and enter a dummy login 4) Get door hanger, click Remember Password At that point the master password prompt used to come up.

* Added workaround * hg rm'd storage-Legacy.js, which I noticed I forgot to do. https://tbpl.mozilla.org/?tree=Try&rev=4ebb7bb90232

Comment on attachment 814676 [details] [diff] [review] Patch v.3 Try looks green. (In reply to Matthew N. [:MattN] (away Sep. 16 - Oct. 2; limited avail. through Oct. 7) from comment #11) > Are you planning on doing this in a separate bug? I think they should be > removed. Yeah, let's just do that separately in bug 925101.

Comment on attachment 814676 [details] [diff] [review] Patch v.3 Review of attachment 814676 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/passwordmgr/storage-mozStorage.js @@ +189,5 @@ > this._debug = this._prefBranch.getBoolPref("debug"); > > let isFirstRun; > try { > + // Force init of the crypto module. Nit: s/init/initialization/

Too bad I didn't see this earlier... I used this feature to inject passwords into the password manager on the profile layer of mozmill tests. I'm sure I could add logic to each such mozmill test that sets the password using JS early on, but it was so much easier to just rely on the migration here. Is there any other way to prefill the password manager? I tried to figure out the format of the new encryption scheme, but it seems I would need to rewrite the encryption in python to be able to take a plaintext password and fill it into the password manager.

You should either use the API (nsILoginManager), or stash a copy of logins.json + key3.db for reuse when creating your testing profile.

Thanks for the reply and sorry for digging up this old bug. I went with writing a wrapper that uses NSS to encrypt the data, it is a little more complex than before but it works just as well.