Closed Bug 239131 Opened 21 years ago Closed 16 years ago

Thunderbird should use the new password manager


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



(Not tracked)

Thunderbird 3.0b2


(Reporter: mscott, Assigned: standard8)


(Blocks 2 open bugs)



(1 file, 6 obsolete files)

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).
Target Milestone: --- → Thunderbird0.7
Target Milestone: Thunderbird0.7 → Thunderbird0.8
Target Milestone: Thunderbird0.8 → Thunderbird1.0
Target Milestone: Thunderbird1.0 → Thunderbird1.1
Target Milestone: Thunderbird1.1 → Thunderbird1.5
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.

I'm confused - thunderbird and mozilla handle passwords in the same way.
Flags: blocking-aviary2?
Flags: blocking-firefox2? → blocking-thunderbird2?
Blocks: 287239
We're not going to get to this on the branch. 
Flags: blocking-thunderbird2? → blocking-thunderbird2-
Target Milestone: Thunderbird1.5 → Thunderbird 3
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 (, building toolkit's password manager via toolkit/components/ (and possibly satchel which you'd have to do as an xulrunner app anyway).."

OS: Windows 2000 → All
Hardware: PC → All
QA Contact: front-end
Attached patch patch (1.8branch) (obsolete) — Splinter Review
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 ( 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.
Assignee: mscott → mozilla
Attachment #264224 - Flags: review?
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.
Blocks: 380917
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\
Attachment #264224 - Flags: review? → review?(mscott)
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!
Yes, I'm on it but still have some problem to get the correct auth prompt (bug 374723)
Depends on: 374723
I mean bug 382437 (sorry).
Depends on: 382437
No longer depends on: 374723
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 on attachment 264224 [details] [diff] [review]
patch (1.8branch)

clearing the review request since Wolfgang is working on a trunk based patch for us.
Attachment #264224 - Flags: review?(mscott)
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).
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.
Blocks: 390025
Depends on: 386150
Comment on attachment 273913 [details] [diff] [review]
wip trunk patch (not working/complete)

>Index: mailnews/base/util/
>+REQUIRES	+= loginmgr

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 :)
Depends on: 392380
Attached patch updated wip trunk patch (obsolete) — Splinter Review
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.
Attachment #273913 - Attachment is obsolete: true
Blocks: 359279
Depends on: 396316
Depends on: 402536
Depends on: 403790
Blocks: 137197
Blocks: 275547
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.
(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.
Flags: blocking-thunderbird3?
Attachment #264224 - Attachment is obsolete: true
Attachment #281872 - Attachment is obsolete: true
Blocks: 392380
No longer depends on: 392380
Depends on: 316084
Attached patch Patch v.1 (work in progress) (obsolete) — Splinter Review
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.
Attachment #303716 - Attachment is obsolete: true
(Oops, I forgot -N in my diff. 2 files are also removed with the patch:
mail/components/preferences/viewpasswords.js and viewpasswords.xul)
(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.
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?
Assignee: mozilla → dolske
Version: unspecified → Trunk
Depends on: 433316
Blocks: 306324
Blocks: 312593
Blocks: 390801
Blocks: 324062
Blocks: 302388
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Flags: blocking-thunderbird3.0b1?
Flags: blocking-thunderbird3.0b1?
Flags: blocking-thunderbird3.0b1+
Priority: -- → P2
Target Milestone: Thunderbird 3 → Thunderbird 3.0b1
Switching for b1 flags to target milestones, to avoid flag churn.
Whiteboard: [waiting on bug 413316]
Whiteboard: [waiting on bug 413316] → [waiting on bug 433316]
3.0b1 flag is going away in favour of 3.0 flag and milestone combination.
Flags: blocking-thunderbird3.0b1+
Will the new password manager include the ability to manually modify or add the usernames and password for resources (ala Bug 109810)?
(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.
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.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
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.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Blocks: 469221
Attached patch Thunderbird UISplinter Review
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.
Assignee: dolske → bugzilla
Attachment #320326 - Attachment is obsolete: true
Attachment #320337 - Attachment is obsolete: true
Attachment #355390 - Flags: review?(philringnalda)
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.
Blocks: 340523
Attachment #355390 - Flags: ui-review?(clarkbw)
Attachment #355390 - Flags: review?(philringnalda)
Attachment #355390 - Flags: review+
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.
Whiteboard: [waiting on bug 433316] → [needs review clarkbw][waiting on 472824]
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 :)
Whiteboard: [needs review clarkbw][waiting on 472824] → [waiting on bug 433316]
Attachment #355390 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 355390 [details] [diff] [review]
Thunderbird UI

ui-r+ with comments from Comment #39 addressed.
Checked in:
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [waiting on bug 433316]
Blocks: 339972
You need to log in before you can comment on or make changes to this bug.