Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Move to LoginManager and remove wallet from SeaMonkey

VERIFIED FIXED in seamonkey2.0a3

Status

SeaMonkey
UI Design
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

({fixed-seamonkey2.0})

Trunk
seamonkey2.0a3
fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
blocking-seamonkey2.0 +
blocking-seamonkey2.0a1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 10 obsolete attachments)

5.61 KB, patch
neil@parkwaycc.co.uk
: review+
Robert Kaiser
: superreview+
Details | Diff | Splinter Review
11.77 KB, patch
neil@parkwaycc.co.uk
: review+
Robert Kaiser
: superreview+
Details | Diff | Splinter Review
5.17 KB, patch
Robert Kaiser
: review+
Robert Kaiser
: superreview+
Details | Diff | Splinter Review
11.82 KB, patch
Robert Kaiser
: review+
Robert Kaiser
: superreview+
Details | Diff | Splinter Review
1010 bytes, patch
Ian Neal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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

10 years ago
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

10 years ago
Oh, and what happened to the "Log Out" menuitem?
(Assignee)

Comment 4

10 years ago
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

10 years ago
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

10 years ago
Created attachment 274511 [details] [diff] [review]
Unified password manager

The code has been well modularised so I can reuse the existing JavaScript.
@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

10 years ago
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
(Assignee)

Updated

10 years ago
Depends on: 394502
(Assignee)

Updated

10 years ago
Blocks: 394502
No longer depends on: 394502

Comment 9

10 years ago
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!
(Assignee)

Updated

10 years ago
Blocks: 286110
No longer depends on: 286110
(Assignee)

Updated

10 years ago
Duplicate of this bug: 399243
(Assignee)

Updated

10 years ago
Blocks: 366673
(Assignee)

Updated

10 years ago
Duplicate of this bug: 400591

Updated

10 years ago
Duplicate of this bug: 151303
(Assignee)

Comment 13

10 years ago
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
Flags: blocking-seamonkey2.0a1+
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.
(Assignee)

Comment 15

10 years ago
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 :)
(Assignee)

Comment 16

10 years ago
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
(Assignee)

Comment 17

10 years ago
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.
Attachment #274351 - Attachment is obsolete: true
Attachment #301286 - Flags: superreview?(neil)
Attachment #301286 - Flags: review?(neil)

Comment 18

10 years ago
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!
(Assignee)

Comment 19

10 years ago
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.
Attachment #301286 - Attachment is obsolete: true
Attachment #301310 - Flags: superreview?(neil)
Attachment #301310 - Flags: review?(neil)
Attachment #301286 - Flags: superreview?(neil)
Attachment #301286 - Flags: review?(neil)

Comment 20

10 years ago
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."
Attachment #301310 - Flags: superreview?(neil)
Attachment #301310 - Flags: superreview+
Attachment #301310 - Flags: review?(neil)
Attachment #301310 - Flags: review+
(Assignee)

Comment 21

10 years ago
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

10 years ago
(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

10 years ago
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.
Attachment #274511 - Attachment is obsolete: true
(Assignee)

Comment 24

10 years ago
(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.
Whiteboard: [checkins of reviewed patches waiting for dependencies]

Updated

10 years ago
Attachment #301654 - Flags: superreview?(jag)
Attachment #301654 - Flags: review?(bugzilla)
(Assignee)

Comment 25

10 years ago
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.
Attachment #301676 - Flags: superreview?(neil)
Attachment #301676 - Flags: review?(neil)

Updated

10 years ago
Attachment #301676 - Flags: superreview?(neil)
Attachment #301676 - Flags: superreview+
Attachment #301676 - Flags: review?(neil)
Attachment #301676 - Flags: review+

Updated

10 years ago
Blocks: 416235

Comment 26

10 years ago
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
Attachment #301654 - Flags: superreview?(jag) → superreview+
Attachment #301654 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

10 years ago
Duplicate of this bug: 423616

Updated

9 years ago
Blocks: 426971
Blocks: 336874
Depends on: 433316
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
]
Attachment #321003 - Flags: superreview?(neil)
Attachment #321003 - Flags: review?(neil)

Updated

9 years ago
Attachment #321003 - Flags: superreview?(neil)
Attachment #321003 - Flags: superreview+
Attachment #321003 - Flags: review?(neil)
Attachment #321003 - Flags: review+
Depends on: 425145

Updated

9 years ago
Blocks: 444169

Updated

9 years ago
Blocks: 444170
(Assignee)

Comment 29

9 years ago
Still wanted for Alpha, but not blocking it any more. This blocks final though, probably even beta as it blocks fully localized builds currently.
Flags: blocking-seamonkey2.0a1-
Flags: blocking-seamonkey2.0a1+
Flags: blocking-seamonkey2+

Updated

9 years ago
No longer blocks: 444169

Updated

9 years ago
No longer blocks: 444170
QA Contact: ui-design
No longer blocks: 366673

Updated

9 years ago
Blocks: 389254
Duplicate of this bug: 461498
Blocks: 334048
Blocks: 451909

Updated

9 years ago
Blocks: 192165

Updated

9 years ago
No longer blocks: 389254
(Assignee)

Comment 33

9 years ago
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.
Attachment #301310 - Attachment is obsolete: true
Attachment #354825 - Flags: superreview+
Attachment #354825 - Flags: review?(neil)
(Assignee)

Comment 34

9 years ago
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.
Attachment #301654 - Attachment is obsolete: true
Attachment #354826 - Flags: superreview+
Attachment #354826 - Flags: review?(neil)
(Assignee)

Comment 35

9 years ago
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.
Attachment #354827 - Flags: superreview?(neil)
Attachment #354827 - Flags: review?(neil)
(Assignee)

Updated

9 years ago
Attachment #301676 - Attachment is obsolete: true
(Assignee)

Comment 36

9 years ago
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.
Attachment #354828 - Flags: superreview+
Attachment #354828 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #321003 - Attachment is obsolete: true

Comment 37

9 years ago
>  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.
Created attachment 354890 [details] [diff] [review]
[checked in] (Dv1b) </suite/*/*.js> Uncomment Login Manager code

Dv1a, with comment 37 suggestion(s).
Attachment #354828 - Attachment is obsolete: true
Attachment #354828 - Attachment description: Uncomment Login Manager code (unbitrotted) → (Dv1a) Uncomment Login Manager code (unbitrotted)
Attachment #354828 - Flags: superreview+
Attachment #354828 - Flags: review+
(Assignee)

Comment 39

9 years ago
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.
(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...
(Assignee)

Comment 41

9 years ago
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.
Attachment #354890 - Flags: superreview+
Attachment #354890 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #354827 - Flags: superreview?(neil)
Attachment #354827 - Flags: review?(neil)
(Assignee)

Comment 42

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

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

Comment 43

9 years ago
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.
Attachment #354827 - Attachment is obsolete: true
Attachment #354944 - Flags: superreview?(neil)
Attachment #354944 - Flags: review?(neil)

Updated

9 years ago
Attachment #354825 - Flags: review?(neil) → review+

Updated

9 years ago
Attachment #354826 - Flags: review?(neil) → review+
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 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?
(Assignee)

Comment 46

9 years ago
(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.

Updated

9 years ago
Attachment #354944 - Flags: superreview?(neil)
Attachment #354944 - Flags: superreview+
Attachment #354944 - Flags: review?(neil)
Attachment #354944 - Flags: review+
Comment on attachment 354944 [details] [diff] [review]
password prefs, v2.1

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

Comment 48

9 years ago
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!
Attachment #354944 - Attachment is obsolete: true
Attachment #357142 - Flags: superreview+
Attachment #357142 - Flags: review+
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
Attachment #354825 - Attachment description: hook up new password manager, v3.1 → [checked in] hook up new password manager, v3.1
Comment on attachment 354826 [details] [diff] [review]
[checked in] Unified password manager, v2.1

Checked in: http://hg.mozilla.org/comm-central/rev/88fcb0e79548
Attachment #354826 - Attachment description: Unified password manager, v2.1 → [checked in] Unified password manager, v2.1
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
Attachment #354890 - Attachment description: (Dv1b) </suite/*/*.js> Uncomment Login Manager code → [checked in] (Dv1b) </suite/*/*.js> Uncomment Login Manager code
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
Attachment #357142 - Attachment description: password prefs, v2.2, for checkin → [checked in] password prefs, v2.2, for checkin
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [checkins of reviewed patches waiting for dependencies]
Target Milestone: seamonkey2.0a1 → seamonkey2.0a3
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.
Attachment #357365 - Flags: review?(kairo)

Updated

9 years ago
Attachment #357365 - Flags: review?(kairo) → review+
Comment on attachment 357365 [details] [diff] [review]
Additional fix: ensure login manager is loaded

Pushed changeset 079d27d702c7 to comm-central.
Depends on: 474159
Depends on: 474161
Depends on: 474253

Updated

9 years ago
Blocks: 293887

Updated

9 years ago
Blocks: 379227
Blocks: 400194
Blocks: 474879
No longer blocks: 474879
Depends on: 477369
(Assignee)

Updated

9 years ago
Duplicate of this bug: 479635

Updated

9 years ago
Depends on: 483960
Blocks: 491673
(Assignee)

Comment 56

8 years ago
Add fixed-seamonkey2.0 to make it go away from "unfixed blockers" query
Keywords: fixed-seamonkey2.0

Updated

8 years ago
Blocks: 517478
Blocks: 443161
Blocks: 404308

Updated

8 years ago
Status: RESOLVED → VERIFIED
Duplicate of this bug: 171756
(Assignee)

Updated

7 years ago
Duplicate of this bug: 119731
You need to log in before you can comment on or make changes to this bug.