Closed
Bug 390025
Opened 17 years ago
Closed 16 years ago
Move to LoginManager and remove wallet from SeaMonkey
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.0a3
People
(Reporter: kairo, Assigned: kairo)
References
Details
(Keywords: fixed-seamonkey2.0)
Attachments
(5 files, 10 obsolete files)
5.61 KB,
patch
|
neil
:
review+
kairo
:
superreview+
|
Details | Diff | Splinter Review |
11.77 KB,
patch
|
neil
:
review+
kairo
:
superreview+
|
Details | Diff | Splinter Review |
5.17 KB,
patch
|
kairo
:
review+
kairo
:
superreview+
|
Details | Diff | Splinter Review |
11.82 KB,
patch
|
kairo
:
review+
kairo
:
superreview+
|
Details | Diff | Splinter Review |
1010 bytes,
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
Here's the parts of this I have. Note that it breaks remembering passwords in MailNews currently.
Comment 2•17 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•17 years ago
|
||
Oh, and what happened to the "Log Out" menuitem?
Assignee | ||
Comment 4•17 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.
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•17 years ago
|
||
The code has been well modularised so I can reuse the existing JavaScript.
Comment 7•17 years ago
|
||
@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•17 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•17 years ago
|
Comment 9•17 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•17 years ago
|
Assignee | ||
Comment 13•17 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+
Comment 14•17 years ago
|
||
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•17 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•17 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•17 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)
Comment 18•17 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•17 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 20•17 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•17 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•17 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•17 years ago
|
||
* 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•17 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•17 years ago
|
Attachment #301654 -
Flags: superreview?(jag)
Attachment #301654 -
Flags: review?(bugzilla)
Assignee | ||
Comment 25•17 years ago
|
||
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•17 years ago
|
Attachment #301676 -
Flags: superreview?(neil)
Attachment #301676 -
Flags: superreview+
Attachment #301676 -
Flags: review?(neil)
Attachment #301676 -
Flags: review+
Comment 26•17 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+
Updated•17 years ago
|
Attachment #301654 -
Flags: review?(bugzilla) → review+
Comment 28•17 years ago
|
||
(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
]
Updated•17 years ago
|
Attachment #321003 -
Flags: superreview?(neil)
Attachment #321003 -
Flags: review?(neil)
Updated•17 years ago
|
Attachment #321003 -
Flags: superreview?(neil)
Attachment #321003 -
Flags: superreview+
Attachment #321003 -
Flags: review?(neil)
Attachment #321003 -
Flags: review+
Assignee | ||
Comment 29•16 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•16 years ago
|
QA Contact: ui-design
Assignee | ||
Comment 33•16 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•16 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•16 years ago
|
||
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•16 years ago
|
Attachment #301676 -
Attachment is obsolete: true
Assignee | ||
Comment 36•16 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•16 years ago
|
Attachment #321003 -
Attachment is obsolete: true
Comment 37•16 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.
Comment 38•16 years ago
|
||
Dv1a, with comment 37 suggestion(s).
Attachment #354828 -
Attachment is obsolete: true
Updated•16 years ago
|
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•16 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.
Comment 40•16 years ago
|
||
(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•16 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•16 years ago
|
Attachment #354827 -
Flags: superreview?(neil)
Attachment #354827 -
Flags: review?(neil)
Assignee | ||
Comment 42•16 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•16 years ago
|
||
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•16 years ago
|
Attachment #354825 -
Flags: review?(neil) → review+
Updated•16 years ago
|
Attachment #354826 -
Flags: review?(neil) → review+
Comment 44•16 years ago
|
||
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•16 years ago
|
||
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•16 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•16 years ago
|
Attachment #354944 -
Flags: superreview?(neil)
Attachment #354944 -
Flags: superreview+
Attachment #354944 -
Flags: review?(neil)
Attachment #354944 -
Flags: review+
Comment 47•16 years ago
|
||
Comment on attachment 354944 [details] [diff] [review]
password prefs, v2.1
OK, r+sr=me with pref-passwords.js removed.
Assignee | ||
Comment 48•16 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 49•16 years ago
|
||
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 50•16 years ago
|
||
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 51•16 years ago
|
||
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 52•16 years ago
|
||
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
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [checkins of reviewed patches waiting for dependencies]
Target Milestone: seamonkey2.0a1 → seamonkey2.0a3
Comment 53•16 years ago
|
||
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)
Attachment #357365 -
Flags: review?(kairo) → review+
Comment 54•16 years ago
|
||
Comment on attachment 357365 [details] [diff] [review]
Additional fix: ensure login manager is loaded
Pushed changeset 079d27d702c7 to comm-central.
Assignee | ||
Comment 56•15 years ago
|
||
Add fixed-seamonkey2.0 to make it go away from "unfixed blockers" query
Keywords: fixed-seamonkey2.0
You need to log in
before you can comment on or make changes to this bug.
Description
•