239131 - Thunderbird should use the new password manager

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P2)

Description

[Scott MacGregor]

I need to convert thunderbird over to Brian's new password manager.
Unfortunately the new password manager was written to interact with satchel.

I either need to figure out how tight that relationship is and see if it can
also work (ifdef?) via the old autocomplete APIs or finally bite the bullet and
figure out how to make satchel work with mail style auto complete (Joe told me
this was a big task a while ago).

Comment 1

[Mark Dowling]

Will this new password manager have a "remember password" checkbox so users can
disable password storage entirely? This is how we configure Mozilla Suite and
it's hindering use of TB as an alternative mail client.

Comment 2

[David :Bienvenu]

I'm confused - thunderbird and mozilla handle passwords in the same way.

Comment 3

[Scott MacGregor]

We're not going to get to this on the branch. 

Comment 4

[Scott MacGregor]

Some updated notes from Mark Banner on how to do this:

"
Whilst we can build it, satchel (form filler) doesn't work because of the autocomplete implementation that we still rely on. However password manager does seem to work for mailnews with only one problem as far as I could tell - this would be when a password has been saved, the toolkit password manager still brings up a prompt with the saved password (in *) displayed (pretty annoying from a mailnews point of view). I think that could possibly be simply fixed. The only other possible problem I could think of is if it is possible to use forms etc within tb especially for passwords - I don't think it is.

So I was thinking that TB could possibly pick up that code fairly easily and get the benefit of slighty better maintained code sooner (especially now that 2.0 is almost out).

Unfortunately I'm not able to spend time on doing this for you, but in short, it'd mean not building wallet as an extension (configure.in), building toolkit's password manager via toolkit/components/Makefile.in (and possibly satchel which you'd have to do as an xulrunner app anyway).."

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch (1.8branch)

I'm sorry that I stumbled over this just this week and therefore far too late for 2.0. Nevertheless the attached patch is based on the 1.8 branch. I expect that porting to trunk would mainly be build config (toolkit-tiers.mk etc.)

The patch works for me so far but probably I missed something. I would appreciate any hints before I start to port to trunk.

SeaMonkey people might want to check if everything still works for them since I obviously had to touch a bit of shared code in mailnews.

Comment 6

[Robert Kaiser]

SeaMonkey is working on moving away from wallet and to the new pwdmgr+satchel in bug 304309 - we should probably cooperate on getting all that stuff into trunk, as SeaMonkey really would like to get away from the old, more or less unmaintained wallet code as well.

Comment 7

[Scott MacGregor]

Comment on attachment 264224 [details] [diff] [review]
patch (1.8branch)

wolfgang, this is the kind of drive by patch I like to get!

I think this patch is in good enough shape if you can do a trunk port. Nothing big jumps out at me.

we should add the wallet files to mail\installer\removed-files.in

Comment 8

[Justin Dolske [:Dolske]]

Hmm, note that this patch is using the "old new" password mananger.

The "new new" password manager is now "Login Manager" / nsILoginMananger. [Why we changed it, I can't say, maybe people just liked it better that way.] But I believe from IRC that Wolfgang is already working on getting things working on the new interfaces, and switching away from wallet was probably most of the work anyway. Yay!

Comment 9

[Wolfgang Rosenauer [:wolfiR]]

Yes, I'm on it but still have some problem to get the correct auth prompt (bug 374723)

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

I mean bug 382437 (sorry).

Comment 11

[Mark Banner (:standard8)]

I've only just seen this bug and the patch.

One comment I would have is that it may be worth Thunderbird "picking" up satchel at the same time as password manager. I know Thunderbird doesn't need satchel, however, when Thunderbird gets to being an xulrunner application its going to get it anyway (application specific ifdefs won't be applicable in the toolkit directory).

So seeing as you don't actually need to do much to pick it up, then I think it'd be worth Thunderbird doing it now and then you also know earlier if you get any problems because of it.

Comment 12

[Scott MacGregor]

Comment on attachment 264224 [details] [diff] [review]
patch (1.8branch)

clearing the review request since Wolfgang is working on a trunk based patch for us.

Comment 13

[Robert Kaiser]

Wolfgang, any progress on this?

SeaMonkey is getting close to being able to ditch wallet for form saving in bug 304309, but we can't remove wallet without being able to preserve password management, which needs this patch for mailnews. Not sure we need for browser, I hope LoginManager is a drop-in replacement for pwdmgr there (pwdmgr seems to be just that for wallet's password management).

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

Sorry, no progress on that since it's still blocked by bug 382437. I have no clue about this nsIAuthPrompt stuff and probably won't have time to dive into it in the near future.
I'll attach what I did weeks ago before I ran into the issue but it's not complete nor working but just a snapshot.

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

Attachment: wip trunk patch (not working/complete)

Comment 16

[Robert Kaiser]

Comment on attachment 273913 [details] [diff] [review]
wip trunk patch (not working/complete)

>Index: mailnews/base/util/Makefile.in
>===================================================================
>+ifdef MOZ_THUNDERBIRD
>+REQUIRES	+= loginmgr
>+endif

Please don't add those ifdefs here and elsewhere, just switch mailnews to loginmgr completely, SeaMonkey wants this as well. And I guess not adding the ifdefs makes the patch easier and keeps the code cleaner as well :)

Comment 17

[Mark Banner (:standard8)]

Attachment: updated wip trunk patch

I've updated Wolfgang's original patch to undo the bitrot, and remove the ifdefs so that SeaMonkey can pick up the login manager at the same time.

The suite/ directory stuff in the patch is all from bug 390025 - I put them in one patch to make it easier for us to test.

The main reason I've updated this patch is that so we can hopefully use it to help us drive through bug 382437 - hence why I've included some stuff from elsewhere.

Comment 18

[Mark Banner (:standard8)]

Attachment: Build config and installer parts only

This patch includes the build config and installer parts only. I'm currently working on updating the main mailnews parts (other SM Specific parts covered in bug 390025)

Wolfgang would you be check that the TB UI parts are up to date and get review on them please? I'm hoping to get this all in place within the next week or so.

Comment 19

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #18)
> Wolfgang would you be check that the TB UI parts are up to date and get review
> on them please? I'm hoping to get this all in place within the next week or so.

I try to look into it. No guarantees though since I'm quite busy coming week.

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (work in progress)

I have some cycles free, so took a shot at cleaning some of this up...

This is the TB-only build/config bits from the earlier patch, plus a conversion of the prefs dialog.

Comment 22

[Justin Dolske [:Dolske]]

(Oops, I forgot -N in my diff. 2 files are also removed with the patch:
mail/components/preferences/viewpasswords.js and viewpasswords.xul)

Comment 23

[Mark Banner (:standard8)]

(In reply to comment #21)
> Created an attachment (id=320326) [details]
> Patch v.1 (work in progress)
> 
> I have some cycles free, so took a shot at cleaning some of this up...
> 
> This is the TB-only build/config bits from the earlier patch, plus a conversion
> of the prefs dialog.
> 
Thanks for looking at this Justin. I haven't tried it yet. I have already split out the patches into slightly different areas, because when mailnews migrates both SeaMonkey and Thunderbird are going to have to migrate at the same time.

I probably should have posted these earlier. At the moment I have:

build changes (both TB + SM)
TB UI changes
SM UI changes (see bug 390025)
MailNews backend changes.

I'll post the updated build changes patch in a moment. I am currently working on the MailNews backend changes.

If you're willing to complete the update to the TB UI changes that would be useful.

Comment 24

[Mark Banner (:standard8)]

Attachment: Build and config changes (SM + TB)

Comment 25

[Justin Dolske [:Dolske]]

Sure, I can go ahead and take care of the TB UI bits. WolfiR, steal this back if you really want to own it instead. :)

Is there a bug for the backend changes?

Comment 27

[David Ascher (:davida)]

Switching for b1 flags to target milestones, to avoid flag churn.

Comment 28

[Mark Banner (:standard8)]

3.0b1 flag is going away in favour of 3.0 flag and milestone combination.

Comment 29

[IU]

Will the new password manager include the ability to manually modify or add the usernames and password for resources (ala Bug 109810)?

Comment 30

[Mark Banner (:standard8)]

(In reply to comment #29)
> Will the new password manager include the ability to manually modify or add the
> usernames and password for resources (ala Bug 109810)?

No, we are only picking up the existing toolkit/Firefox functionality.

Comment 31

[Mark Banner (:standard8)]

The backend work isn't done on this, and we're going into string freeze tomorrow. Therefore we've decided to move this work out until beta 2.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

While we'd love to get this for beta 1, we wouldn't actually hold the release on it, so moving the target milestone out.

Comment 35

[Mark Banner (:standard8)]

Attachment: Thunderbird UI

This is the current version of the UI patch for Thunderbird. I'm not sure its changed much (apart from bitrot) since the earlier versions on this bug.

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 355390 [details] [diff] [review]
Thunderbird UI

Time to get reviews going. This is basically the UI parts of Wolfgang's patch with a few bitrot fixes.

See bug 433316 comment 30 for details on what patches to apply to test this.

Comment 38

[Phil Ringnalda (:philor)]

Comment on attachment 355390 [details] [diff] [review]
Thunderbird UI

>+   * selects
>+   * the master password button to show, and enables/disables it as necessary.
>+   * The master password is controlled by various bits of NSS functionality,
>+   * so

Whatever happened with the wrapping here, please make it unhappen.

>+    var button = document.getElementById("changeMasterPassword");
>+    button.disabled = noMP;
>+
>+    var checkbox = document.getElementById("useMasterPassword");
>+    checkbox.checked = !noMP;

No point in the vars, which will just add to the confusion if getElem fails, might as well just do document.getElementById("changeMasterPassword").disabled = noMP;

>+      // XXX I have no bloody idea what this means

But at least it's on the way to being Waldo's most-copied line of code :)

>+   * Shows the sites where the user has saved passwords and the associated
>+   * login
>+   * information.

Eww again.

>+          <!-- XXX button to do a showExceptions()? -->
>+

We don't *do* exceptions, do we?

>-<!ENTITY savedPasswords.intro         "&brandShortName; can remember password information for all of your accounts so you do not need to re-enter your login details.">
>-<!ENTITY  encryptEnabled.label        "Use a master password to encrypt stored passwords">
>-<!ENTITY  encryptEnabled.accesskey    "s">
>-<!ENTITY  masterPassword.intro        "When set, the Master Password protects all your passwords - but you must enter it once per session.">
>-<!ENTITY  setMasterPassword.label     "Master Password">
>-<!ENTITY  setMasterPassword.accesskey "M">
>-<!ENTITY  removeMasterPassword.label  "Remove Master Password…">
>-<!ENTITY  removeMasterPassword.accesskey "R">
>-<!ENTITY  editPasswords.label         "Edit Saved Passwords">
>-<!ENTITY  editPasswords.accesskey     "i">
>+<!ENTITY savedPasswords.intro           "&brandShortName; can remember passwords for all of your accounts.">

Some better, but I still get the feeling it's one of the pointless strings that we only have in the prefs to keep a button from being directly up against the tabs. If I didn't read it as saying "Lorem ipsum whatever yadda yadda" I'd think "Okay, I'm in preferences, and you're telling me that, so where's the checkbox to enable it? And if I just want it to remember for some, but not all, of my accounts, where do I set that?"

Better to wait for Bryan to chime in, but I'd say we should remove the .intro, and put the Passwords button at the bottom left below the master password stuff.

>+<!ENTITY savedPasswords.label           "Saved Passwords…">

Despite their change from Show Passwords... - Show Passwords to Saved Passwords... Show Passwords, I'm no more fond of this as a place to follow Firefox than I was at the time of bug 361434 comment 40. The ellipsis is just wrong: further input is not required before we will Saved Passwords, and users don't need to be reassured that they can click the button without worrying that we will immediately Saved Passwords. Delete Passwords... would call for an ellipsis, Saved Passwords does not.

And despite LpSolit's concerns, I haven't seen a big post-2.0 influx of bugs saying that the password editor is broken because they clicked Edit Saved Passwords and then couldn't change a typo in one letter of a password, so I don't see any need to change the button label.

Comment 39

[Bryan Clark (DevTools PM) [:clarkbw]]

I had planned on flattening this tab out and organizing with description titles instead; from 4 sub tabs into (hopefully) one.  Likely that future will use a <description>Passwords</description> and drop the ( Saved Passwords... ) to underneath the master passwords like in Fx.  So the current organization is likely to change but I think the strings will work out.

>+<!ENTITY savedPasswords.label           "Saved Passwords…">

I know this isn't a straight forward 'further input is required' case, however lets keep the elipsis.  The action of the buttons is a bit ambiguous because it's in the past tense such that I don't think people are confident what action is going to happen.  Are we going to "saved passwords" to their Thunderbird?  It's like an ( All Your Base ) button, almost an action but not really :)

Comment 40

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 355390 [details] [diff] [review]
Thunderbird UI

ui-r+ with comments from Comment #39 addressed.

Comment 41

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/12e714b4861c