390025 - Move to LoginManager and remove wallet from SeaMonkey

Status: VERIFIED FIXED

Description

[Robert Kaiser]

Now that SeaMonkey is using satchel instead of wallet's formfill after the checkins for bug 304309, we should also convert over to using toolkit's login manager and remove wallet completely.

On the browser side, this is relatively easy, even though bug 376668 would vastly improve the experience there for the multiple login scenario.

The mailnews part is harder but in bug 239131 there's work done on that, currently blocked by bug 382437 though.

Assigning to me as I have some WIP work for that move in my tree, even though mailnews does not work with it yet.

Comment 1

[Robert Kaiser]

Created attachment 274351 [details] [diff] [review]
patch, v1: remove wallet and add hook up toolkit password manager

Here's the parts of this I have. Note that it breaks remembering passwords in MailNews currently.

Comment 2

[[e-mail]]

I don't like the toolkit dialogs. Would it be too hard to adapt wallet's password dialog to work with the login manager?

Comment 3

[[e-mail]]

Oh, and what happened to the "Log Out" menuitem?

Comment 4

[Robert Kaiser]

I don't see what so different about the toolkit dialogs, other than having two dialogs instead of one with tabs...

On "Log Out" - I'm not sure. Firefox doesn't have it, and I have no idea how to implement it.

Comment 5

[Sander]

It looks like bug 222408 would also need to be fixed to get parity with current SeaMonkey (expiring master passwords). 
(IMO this is must-have functionality, as it provides a measure of protection against automatic password stealing on sites vulnerable to script injection.)

Comment 6

[[e-mail]]

Created attachment 274511 [details] [diff] [review]
Unified password manager

The code has been well modularised so I can reuse the existing JavaScript.

Comment 7

[Caio Tiago Oliveira (asrail)]

@Sander: I agreed with you it's a must have, but there's a little workaround. You can just add more than one password on every site you log-in where users can upload javascript. So, it won't autocomplete.

Comment 8

[Kai Engert (:kaie:)]

Just some documentation that helps to distinguish the two prompts, I happen to need to distinguish them for testing another bug...

When I use SeaMonkey to go to a web site and have a password remembered, I am prompted twice:

- first prompt comes from wallet, saying
  "password manager can remember... do you want to...
   never / no / yes"
  and produces file signons.txt

- second prompt comes from toolkit login manager, saying
  "do you want seamonkey to remember
   never / not now / remember"
  and produces file signons2.txt

Comment 9

[Bryce Mozilla Nesbitt]

Awesome: just a vote for this bug. The password manager popup is constantly getting in the way ("do you want to remember this password").  Any other solution is a bug report worth voting for!

Comment 13

[Robert Kaiser]

The UI work done here blocks L10n builds from fully working, therefore this bug blocks Alpha per http://home.kairo.at/blog/2008-01/seamonkey_2_alpha_criteria

Comment 14

[Mark Banner (:standard8)]

Robert/Neil, could you ensure the patches on this bug are up to date and get reviews on them please? I'm hoping to get the whole suite of PM patches in before FFb4, so we need to start getting reviews etc.

(Robert - if you could drop the confvars.sh change and I'll include it in a general build config patch that'll do the toolkit changes as well).

Additionally, if anyone wants to update the password preference pane, that would be very useful - bug 239131 has some examples of how the change is proposed for Thunderbird.

Comment 15

[Robert Kaiser]

Neil's patch seems to apply fine, I'll look into making mine call his dialog and we only need reviews then. Dropping the confvars part is easy enough, just need to leave out the file from the diff :)

Comment 16

[Robert Kaiser]

I'm getting the following entry in the error console with Neil's window when I open the exception tab:

Error: document.getElementById("filter") is null
Surce File: chrome://passwordmgr/content/passwordManager.js
Line: 220

Comment 17

[Robert Kaiser]

Created attachment 301286 [details] [diff] [review]
hook up new password manager, v2

Here's renewed patch for hooking up the new password manager, working with Neil's unified window. As we seem to have only one entry for the password manager left in the menu, I reduced the integration to a single menu item instead of a submenu.

Comment 18

[[e-mail]]

We can still use the code for "Log Out"; the C++ is easily converted to JS:
// Queries the HTTP Auth Manager and clears all sessions
Components.classes['@mozilla.org/network/http-auth-manager;1']
          .getService(Components.interfaces.nsIHttpAuthManager)
          .clearAll();

// Expires the master password
Components.classes["@mozilla.org/security/sdr;1"]
          .getService(Components.interfaces.nsISecretDecoderRing)
          .logoutAndTeardown();

Note that WALLET_ExpirePassword always returns true!

Comment 19

[Robert Kaiser]

Created attachment 301310 [details] [diff] [review]
hook up new password manager, v3

This patch put back the log out item per Neil's request and using his code.

Comment 20

[[e-mail]]

Comment on attachment 301310 [details] [diff] [review]
hook up new password manager, v3

Actually it turns out that the old "Log Out" menuitem used to alert() (!) "You are now logged out."

Comment 21

[Robert Kaiser]

Maybe we should improve the logout after that switch to LoginManager has been done to flip up a notification bar that states that the logout has been done.

Comment 22

[[e-mail]]

(In reply to comment #21)
>Maybe we should improve the logout after that switch to LoginManager has been
>done to flip up a notification bar that states that the logout has been done.
Logout is global; notifications are per-tab.

Comment 23

[[e-mail]]

Created attachment 301654 [details] [diff] [review]
Unified password manager, v2

* Updated for bitrot ("Search" field)
* Added toolkit .dtd and removed duplicate entities
* Restored help button

Note that I didn't copy the search field 100% - for instance I used the value attribute on the label, and I didn't copy the escape key handler.

Comment 24

[Robert Kaiser]

(In reply to comment #22)
> Logout is global; notifications are per-tab.

Ah, right. Still, if we want anything to notify the user of this at all, I wouldn't want something that strictly requires a user action. Better no notification than a dialog/alert.

Comment 25

[Robert Kaiser]

Created attachment 301676 [details] [diff] [review]
password prefs, v1

Here's a first take of the new passwords pref panel. This currently only has (de)activation of pwdmgr and a button to launch the pwdmgr window, I'd like us to integrate master password prefs into this pane at a later stage though.

Comment 26

[jag (Peter Annema)]

Comment on attachment 301654 [details] [diff] [review]
Unified password manager, v2

Maybe throw some whitespace around the <separator>s, or remove the blank line in the savedsignons tab code. sr=jag

Comment 28

[Serge Gautherie (:sgautherie)]

Created attachment 321003 [details] [diff] [review]
(Dv1) </suite/*/*.js> Uncomment Login Manager code

(Requested per bug 428803 comment 8.)

Untested.

***

Bug 428803 comment 2 search, confirmed/extended by:
<http://mxr.mozilla.org/seamonkey/search?string=toolkit%27s+login+manager&case=on&tree=seamonkey>
returns:

<security.js>
[
1.5 <[e-mail]> 2008-05-13 12:40
Bug 428803 – In <pageinfo/security.js> (Line: 315), "Warning: reference to undefined property Components.classes['@mozilla.org/login-manager;1']"
patch by Serge Gautherie <[e-mail]>, r=db48x, sr=neil
]

<pref-security.js>
<Sanitizer.jsm>
<browser_sanitizer.js>
[
1.1 <[e-mail]> 2008-02-27 08:15
bug 416233 - Add sanitize (clear private data) option for SeaMonkey, r+sr=Neil
]

Comment 29

[Robert Kaiser]

Still wanted for Alpha, but not blocking it any more. This blocks final though, probably even beta as it blocks fully localized builds currently.

Comment 33

[Robert Kaiser]

Created attachment 354825 [details] [diff] [review]
[checked in] hook up new password manager, v3.1

This is the unbitrotted and hg-based version of the patch for hooking up the password manager. I'm forwarding the sr from the last patch but re-requesting review as I removed all the prefs from browser-prefs.js that are nowadays already defined in all.js.

Comment 34

[Robert Kaiser]

Created attachment 354826 [details] [diff] [review]
[checked in] Unified password manager, v2.1

This is the unbitrotted and hg-based patch for the unified password manager (original work by Neil), carrying forward the sr but re-requesting review for a few adaptations of the code I needed to make to fit with current toolkit XUL.

Comment 35

[Robert Kaiser]

Created attachment 354827 [details] [diff] [review]
password prefs, v2

I needed to rework the prefs patch a lot as nowadays all the pref panels are in the new prefwindow, so re-requesting r+sr for this patch.

Comment 36

[Robert Kaiser]

Created attachment 354828 [details] [diff] [review]
(Dv1a) Uncomment Login Manager code (unbitrotted)

The patch for enabling login manager code in suite/ .js files just needed a straight-forward hg import and some minor manual merging, but didn't really change anywhere, so forwarding r+sr.

Comment 37

[Philip Chee]

>  function realmHasPasswords(location) {
>    return false;
> -  /* XXX: use code below (instead of above line) once we are using toolkit's 
> login manager

I think you have to remove the |return false;| line as well.

Comment 38

[Serge Gautherie (:sgautherie)]

Created attachment 354890 [details] [diff] [review]
[checked in] (Dv1b) </suite/*/*.js> Uncomment Login Manager code

Dv1a, with comment 37 suggestion(s).

Comment 39

[Robert Kaiser]

Serge: Why are you removing already granted reviews? And comment #37 wasn't a suggestion, I just left out something when manually applying the patch.

Comment 40

[Serge Gautherie (:sgautherie)]

(In reply to comment #39)
> Serge: Why are you removing already granted reviews? And comment #37 wasn't a

I only removed your forward on your (obsoleted) patch.
Neil's r+sr still apply (as written in my new patch).

> suggestion, I just left out something when manually applying the patch.

That's just a "generic" word I copy+paste...

Comment 41

[Robert Kaiser]

Comment on attachment 354890 [details] [diff] [review]
[checked in] (Dv1b) </suite/*/*.js> Uncomment Login Manager code

forward r+sr which has been given to that patch earlier, before it has been unbitrotted.

Comment 42

[Robert Kaiser]

Comment on attachment 354827 [details] [diff] [review]
password prefs, v2

toPasswordManager() doesn't work here, I'll do a new patch.

Comment 43

[Robert Kaiser]

Created attachment 354944 [details] [diff] [review]
password prefs, v2.1

This patch also works to show the password manager from prefs - we need to include tasksOverlay.js into the panel.

Comment 44

[[e-mail]]

Comment on attachment 354826 [details] [diff] [review]
[checked in] Unified password manager, v2.1

>+  <script src="chrome://passwordmgr/content/passwordManagerCommon.js"/>
>+  <script src="chrome://passwordmgr/content/passwordManager.js"/>
>+  <script src="chrome://passwordmgr/content/passwordManagerExceptions.js"/>
>+  <script src="chrome://help/content/contextHelp.js"/>
As some point we ought to switch these to
<script type="application/javascript"
        src="chrome://help/content/contextHelp.js"/>

>+        <!-- filter -->
>+        <hbox align="center">
>+          <label accesskey="&filter.accesskey;" control="filter">&filter.label;</label>
>+          <textbox id="filter" flex="1" type="search"
>+                   oncommand="_filterPasswords();"/>
I wonder why they bothered with a multiline label here - I would just have used <label value="&filter.label;"> assuming that's OK from an l10n point of view.

>+        </hbox>
>+
>+        <description control="signonsTree" id="signonsIntro"/>
>+        <separator class="thin"/>
I think the new filter box looks too close to this text. Perhaps an extra thin separator between the box and the text would work best.

Comment 45

[[e-mail]]

Comment on attachment 354944 [details] [diff] [review]
password prefs, v2.1

>diff --git a/suite/common/pref/pref-passwords.js b/suite/common/pref/pref-passwords.js
>--- a/suite/common/pref/pref-passwords.js
>+++ b/suite/common/pref/pref-passwords.js
>@@ -33,17 +33,9 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> function Startup()
> {
>-  Components.classes['@mozilla.org/wallet/wallet-service;1']
>-            .getService(Components.interfaces.nsIWalletService)
>-            .WALLET_InitReencryptCallback(window);
> }
>-
>-function ViewSignons()
>-{
>-  window.openDialog("chrome://communicator/content/wallet/SignonViewer.xul","_blank","chrome,resizable=yes", "S");
>-}
Why keep an empty file?

Comment 46

[Robert Kaiser]

(In reply to comment #45)
> Why keep an empty file?

I just thought there might be something else we might need to add again that could need the .js - but I guess we just should remove it.
IMHO, we should even merge the "Passwords" and "Master Password" panels in the future.

Comment 47

[[e-mail]]

Comment on attachment 354944 [details] [diff] [review]
password prefs, v2.1

OK, r+sr=me with pref-passwords.js removed.

Comment 48

[Robert Kaiser]

Created attachment 357142 [details] [diff] [review]
[checked in] password prefs, v2.2, for checkin

This patch addresses the comments Neil had on the patch, ready for checkin!

Comment 49

[Mark Banner (:standard8)]

Comment on attachment 354825 [details] [diff] [review]
[checked in] hook up new password manager, v3.1

Checked in: http://hg.mozilla.org/comm-central/rev/502d7d8bea0d

Comment 50

[Mark Banner (:standard8)]

Comment on attachment 354826 [details] [diff] [review]
[checked in] Unified password manager, v2.1

Checked in: http://hg.mozilla.org/comm-central/rev/88fcb0e79548

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 354890 [details] [diff] [review]
[checked in] (Dv1b) </suite/*/*.js> Uncomment Login Manager code

Checked in: http://hg.mozilla.org/comm-central/rev/2a55ed80338b

Comment 52

[Mark Banner (:standard8)]

Comment on attachment 357142 [details] [diff] [review]
[checked in] password prefs, v2.2, for checkin

Checked in: http://hg.mozilla.org/comm-central/rev/238fe79f54ae

Comment 53

[[e-mail]]

Created attachment 357365 [details] [diff] [review]
Additional fix: ensure login manager is loaded

We have to do this so that it will start watching for forms. Otherwise it will only kick in after HTTP auth/opening manager/opening mail etc.

Also contains an improved getNotificationBox method.

Comment 54

[[e-mail]]

Comment on attachment 357365 [details] [diff] [review]
Additional fix: ensure login manager is loaded

Pushed changeset 079d27d702c7 to comm-central.

Comment 56

[Robert Kaiser]

Add fixed-seamonkey2.0 to make it go away from "unfixed blockers" query