Last Comment Bug 390025 - Move to LoginManager and remove wallet from SeaMonkey
: Move to LoginManager and remove wallet from SeaMonkey
Status: VERIFIED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: seamonkey2.0a3
Assigned To: Robert Kaiser
:
Mentors:
: 119731 151303 171756 399243 400591 423616 461498 479635 (view as bug list)
Depends on: 239131 304309 382437 425145 433316 474159 474161 474253 477369 483960
Blocks: 192165 286110 293887 334048 336874 375881 379227 380917 394502 400194 404308 416235 426971 443161 451909 491673 517478
  Show dependency treegraph
 
Reported: 2007-07-29 05:56 PDT by Robert Kaiser
Modified: 2010-12-07 10:38 PST (History)
39 users (show)
kairo: blocking‑seamonkey2.0+
kairo: blocking‑seamonkey2.0a1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1: remove wallet and add hook up toolkit password manager (6.71 KB, patch)
2007-07-29 06:38 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
Unified password manager (10.93 KB, patch)
2007-07-30 14:21 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
hook up new password manager, v2 (5.24 KB, patch)
2008-02-04 08:02 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
hook up new password manager, v3 (6.25 KB, patch)
2008-02-04 10:08 PST, Robert Kaiser
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Unified password manager, v2 (11.00 KB, patch)
2008-02-06 05:49 PST, neil@parkwaycc.co.uk
standard8: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
password prefs, v1 (10.48 KB, patch)
2008-02-06 08:53 PST, Robert Kaiser
neil: review+
neil: superreview+
Details | Diff | Splinter Review
(Dv1) </suite/*/*.js> Uncomment Login Manager code (5.84 KB, patch)
2008-05-14 16:54 PDT, Serge Gautherie (:sgautherie)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
[checked in] hook up new password manager, v3.1 (5.61 KB, patch)
2008-12-30 07:09 PST, Robert Kaiser
neil: review+
kairo: superreview+
Details | Diff | Splinter Review
[checked in] Unified password manager, v2.1 (11.77 KB, patch)
2008-12-30 07:12 PST, Robert Kaiser
neil: review+
kairo: superreview+
Details | Diff | Splinter Review
password prefs, v2 (3.75 KB, patch)
2008-12-30 07:14 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
(Dv1a) Uncomment Login Manager code (unbitrotted) (4.85 KB, patch)
2008-12-30 07:16 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
[checked in] (Dv1b) </suite/*/*.js> Uncomment Login Manager code (5.17 KB, patch)
2008-12-30 15:09 PST, Serge Gautherie (:sgautherie)
kairo: review+
kairo: superreview+
Details | Diff | Splinter Review
password prefs, v2.1 (4.22 KB, patch)
2008-12-31 04:23 PST, Robert Kaiser
neil: review+
neil: superreview+
Details | Diff | Splinter Review
[checked in] password prefs, v2.2, for checkin (11.82 KB, patch)
2009-01-15 05:53 PST, Robert Kaiser
kairo: review+
kairo: superreview+
Details | Diff | Splinter Review
Additional fix: ensure login manager is loaded (1010 bytes, patch)
2009-01-16 09:06 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2007-07-29 05:56:51 PDT
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 2007-07-29 06:38:25 PDT
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 neil@parkwaycc.co.uk 2007-07-29 12:30:44 PDT
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 neil@parkwaycc.co.uk 2007-07-29 12:31:45 PDT
Oh, and what happened to the "Log Out" menuitem?
Comment 4 Robert Kaiser 2007-07-29 16:20:33 PDT
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 2007-07-30 13:20:26 PDT
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 neil@parkwaycc.co.uk 2007-07-30 14:21:10 PDT
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) 2007-07-30 14:28:23 PDT
@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) 2007-08-01 08:59:53 PDT
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 2007-08-31 20:31:13 PDT
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 10 Robert Kaiser 2007-10-09 18:12:49 PDT
*** Bug 399243 has been marked as a duplicate of this bug. ***
Comment 11 Robert Kaiser 2007-10-21 07:44:40 PDT
*** Bug 400591 has been marked as a duplicate of this bug. ***
Comment 12 Oleg Rekutin 2007-11-20 16:58:05 PST
*** Bug 151303 has been marked as a duplicate of this bug. ***
Comment 13 Robert Kaiser 2008-01-03 11:19:20 PST
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) 2008-02-04 04:59:46 PST
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 2008-02-04 07:21:30 PST
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 2008-02-04 07:54:25 PST
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 2008-02-04 08:02:31 PST
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 neil@parkwaycc.co.uk 2008-02-04 09:11:03 PST
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 2008-02-04 10:08:05 PST
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 neil@parkwaycc.co.uk 2008-02-05 09:44:15 PST
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 2008-02-05 10:09:19 PST
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 neil@parkwaycc.co.uk 2008-02-06 05:09:19 PST
(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 neil@parkwaycc.co.uk 2008-02-06 05:49:32 PST
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 2008-02-06 05:54:08 PST
(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 2008-02-06 08:53:43 PST
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) 2008-02-17 02:48:52 PST
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 27 Robert Kaiser 2008-03-18 06:14:17 PDT
*** Bug 423616 has been marked as a duplicate of this bug. ***
Comment 28 Serge Gautherie (:sgautherie) 2008-05-14 16:54:53 PDT
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 <db48x@yahoo.com> 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 <sgautherie.bz@free.fr>, r=db48x, sr=neil
]

<pref-security.js>
<Sanitizer.jsm>
<browser_sanitizer.js>
[
1.1 <kairo@kairo.at> 2008-02-27 08:15
bug 416233 - Add sanitize (clear private data) option for SeaMonkey, r+sr=Neil
]
Comment 29 Robert Kaiser 2008-08-26 05:30:45 PDT
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 30 Mark Banner (:standard8) 2008-10-24 04:37:47 PDT
*** Bug 461498 has been marked as a duplicate of this bug. ***
Comment 33 Robert Kaiser 2008-12-30 07:09:32 PST
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 2008-12-30 07:12:22 PST
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 2008-12-30 07:14:03 PST
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 2008-12-30 07:16:35 PST
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 2008-12-30 07:39:23 PST
>  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) 2008-12-30 15:09:11 PST
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 2008-12-30 18:19:53 PST
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) 2008-12-30 23:01:32 PST
(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 2008-12-31 03:56:15 PST
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 2008-12-31 03:57:17 PST
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 2008-12-31 04:23:05 PST
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 neil@parkwaycc.co.uk 2009-01-01 05:27:50 PST
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 neil@parkwaycc.co.uk 2009-01-01 05:38:23 PST
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 2009-01-02 05:09:10 PST
(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 neil@parkwaycc.co.uk 2009-01-02 13:46:26 PST
Comment on attachment 354944 [details] [diff] [review]
password prefs, v2.1

OK, r+sr=me with pref-passwords.js removed.
Comment 48 Robert Kaiser 2009-01-15 05:53:20 PST
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) 2009-01-15 12:55:17 PST
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) 2009-01-15 12:55:49 PST
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) 2009-01-15 12:56:31 PST
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) 2009-01-15 12:57:20 PST
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 neil@parkwaycc.co.uk 2009-01-16 09:06:25 PST
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 neil@parkwaycc.co.uk 2009-01-16 17:16:30 PST
Comment on attachment 357365 [details] [diff] [review]
Additional fix: ensure login manager is loaded

Pushed changeset 079d27d702c7 to comm-central.
Comment 55 Robert Kaiser 2009-02-22 09:32:51 PST
*** Bug 479635 has been marked as a duplicate of this bug. ***
Comment 56 Robert Kaiser 2009-09-18 06:16:31 PDT
Add fixed-seamonkey2.0 to make it go away from "unfixed blockers" query
Comment 57 Jens Hatlak (:InvisibleSmiley) 2010-01-31 15:33:06 PST
*** Bug 171756 has been marked as a duplicate of this bug. ***
Comment 58 Robert Kaiser 2010-12-07 10:38:24 PST
*** Bug 119731 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.