Move to LoginManager and remove wallet from SeaMonkey

VERIFIED FIXED in seamonkey2.0a3

Status

defect
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: kairo, Assigned: kairo)

Tracking

({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
: review+
Details | Diff | Splinter Review
11.77 KB, patch
neil
: review+
Details | Diff | Splinter Review
5.17 KB, patch
kairo
: review+
Details | Diff | Splinter Review
11.82 KB, patch
kairo
: review+
Details | Diff | Splinter Review
1010 bytes, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 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

12 years ago
Here's the parts of this I have. Note that it breaks remembering passwords in MailNews currently.
I don't like the toolkit dialogs. Would it be too hard to adapt wallet's password dialog to work with the login manager?
Oh, and what happened to the "Log Out" menuitem?
(Assignee)

Comment 4

12 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

12 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.)
Posted patch Unified password manager (obsolete) — Splinter Review
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.
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

12 years ago
Depends on: 394502
(Assignee)

Updated

12 years ago
Blocks: 394502
No longer depends on: 394502
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

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

Updated

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

Updated

12 years ago
Blocks: 366673
(Assignee)

Updated

12 years ago
Duplicate of this bug: 400591

Updated

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

Comment 13

11 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

11 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

11 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

11 years ago
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)
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

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

11 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.
(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.
Posted patch Unified password manager, v2 (obsolete) — Splinter Review
* 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

11 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

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

Comment 25

11 years ago
Posted patch password prefs, v1 (obsolete) — Splinter Review
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

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

Updated

11 years ago
Blocks: 416235
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

11 years ago
Duplicate of this bug: 423616

Updated

11 years ago
Blocks: 426971
Blocks: 336874
Depends on: 433316
(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

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

Updated

11 years ago
Blocks: 444169

Updated

11 years ago
Blocks: 444170
(Assignee)

Comment 29

11 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

11 years ago
No longer blocks: 444169

Updated

11 years ago
No longer blocks: 444170
No longer blocks: 366673
Blocks: 389254
Duplicate of this bug: 461498
Blocks: 451909

Updated

11 years ago
Blocks: 192165
No longer blocks: 389254
(Assignee)

Comment 33

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

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

10 years ago
Posted patch password prefs, v2 (obsolete) — Splinter Review
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

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

Comment 36

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

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

Comment 37

10 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.
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

10 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

10 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

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

Comment 42

10 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

10 years ago
Posted patch password prefs, v2.1 (obsolete) — Splinter Review
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

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

Updated

10 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

10 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

10 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

10 years ago
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: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkins of reviewed patches waiting for dependencies]
Target Milestone: seamonkey2.0a1 → seamonkey2.0a3
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

10 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

10 years ago
Blocks: 293887

Updated

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

Updated

10 years ago
Duplicate of this bug: 479635

Updated

10 years ago
Depends on: 483960
(Assignee)

Comment 56

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

Updated

10 years ago
Blocks: 517478

Updated

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

Updated

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