Closed Bug 381692 Opened 15 years ago Closed 15 years ago

Page Info Security section should show site-specific saved passwords

Categories

(Firefox :: Page Info Window, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 Creative ZENcast v1.02.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4) Gecko/20070427 GranParadiso/3.0a4

The new Page Info dialog contains a Security section which tells users whether a site has stored cookies on the users' computers and/or has Firefox saved a password for the site.  In addition, there are the "View Cookies" and "View Saved Passwords" buttons, which I think the user expects to show site-specific cookies and saved passwords (instead of simply showing *all* cookies and saved passwords).  However, the current implementation in FF3.0a4 is the same to the equivalent buttons in the Options dialog.

Reproducible: Always

Steps to Reproduce:
1. Invoke the Page Info dialog for a page.
2. Navigate to the Security section.
3. Click either View Cookies or View Saved Passwords.



about:buildconfig

Build platform
target
i686-pc-cygwin

Build tools
Compiler 	Version 	Compiler flags
$(CYGWIN_WRAPPER) cl 	14.00.50727 	-TC -nologo -W3 -Gy -Fd$(PDBFILE)
$(CYGWIN_WRAPPER) cl 	14.00.50727 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fd$(PDBFILE)

Configure arguments
--enable-application=browser --enable-update-channel=beta --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-svg --enable-canvas --enable-default-toolkit=cairo-windows --enable-update-packaging --with-branding=browser/branding/unofficial
Flags: blocking-firefox3?
Version: 2.0 Branch → Trunk
I believe the cookie part of this request is addressed bug 378668. The password part has no bug that I know of, but requires a fix to bug 327048. Perhaps we could morph this bug to cover the password part of the request?
As suggested in comment 1, this bug is modified to cover the password part of the request.  I've also set a dependency on bug 327048, and I'm taking over that bug.  I think the fix to this bug would be very similar to that of bug 378668.
Severity: enhancement → normal
Depends on: 327048
Summary: Page Info Security section should show site-specific cookies and saved passwords → Page Info Security section should show site-specific saved passwords
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 beta1
Target Milestone: Firefox 3 M7 → Firefox 3 M8
This seems more like wanted, not blocking... renominating
Flags: blocking-firefox3+ → blocking-firefox3?
Taking over.  I'll post a patch when the fix for bug 327048 lands.
Assignee: nobody → ehsan.akhgari
When 327048 lands, this will become blocking, but for now it has to be wanted since the dependency doesn't block (and is a P2 on the PRD).

Makes me sad, but mconnor convinced me.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Requesting blocking now that bug 327048 landed, as per comment 5...
Status: NEW → ASSIGNED
Flags: blocking-firefox3- → blocking-firefox3?
Let's see if we can land this for M9...
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Got to be in Litmus test suite...
Flags: in-litmus?
Flags: blocking-firefox3? → blocking-firefox3-
Target Milestone: Firefox 3 M9 → Firefox 3 M11
Attached patch Patch (v1) (obsolete) — Splinter Review
Real simple patch, similar to the patch accepted for the cousin bug 378668.
Attachment #292978 - Flags: review?(mano)
Attached patch Patch (v1.1) (obsolete) — Splinter Review
Oops, caught a nit myself on trailing spaces!  :D
Attachment #292978 - Attachment is obsolete: true
Attachment #292981 - Flags: review?(mano)
Attachment #292978 - Flags: review?(mano)
Duplicate of this bug: 408489
window.arguments[0] is null when opening the passwords-manager via pref dialog and therefore no passwords would be shown.
Attachment #293310 - Flags: review?(mano)
(In reply to comment #12)
> Created an attachment (id=293310) [details]
> include fix from bug #378668 comment #31
> 
> window.arguments[0] is null when opening the passwords-manager via pref dialog
> and therefore no passwords would be shown.

Good catch Ronny, but the filtering needs to be done after the table is displayed...  This new patch addresses the issue you mentioned correctly.
Attachment #292981 - Attachment is obsolete: true
Attachment #293310 - Attachment is obsolete: true
Attachment #293313 - Flags: review?(mano)
Attachment #292981 - Flags: review?(mano)
Attachment #293310 - Flags: review?(mano)
(In reply to comment #13)
> Good catch Ronny, but the filtering needs to be done after the table is
> displayed...  This new patch addresses the issue you mentioned correctly.
> 

*gnarf* of course ... but wouldn't it be smarter to apply this pre-filter only once? Now you are also calling _filterPasswords (through setFilter) when SignonClearFilter() is called, because it calls LoadSignons() too. But who calls SignonClearFilter? _filterPasswords!
So A calls B which again calls A ...
You could end in an endless loop. The other thing is: if manually deleted the search-field, it would then be refilled by setFilter, because the window-argument is still there. So you can not clear the filter anymore, when you opened the pw-manager from the page info dialog!

Here is the part from my version of the patch, which I made for the duplicate bug 408489.


 function SignonsStartup() {
   kSignonBundle = document.getElementById("signonBundle");
   document.getElementById("togglePasswords").label = kSignonBundle.getString("showPasswords");
   document.getElementById("togglePasswords").accessKey = kSignonBundle.getString("showPasswordsAccessKey");
   LoadSignons();
   FocusFilterBox();
+  if (window.arguments && window.arguments[0] && window.arguments[0].filterString)
+    setFilter(window.arguments[0].filterString);
 }
Comment on attachment 293313 [details] [diff] [review]
Patch (v2) (including fix from bug #378668 comment #31)

r=mano, needs ui-r.
Attachment #293313 - Flags: review?(mano) → review+
(In reply to comment #15)
> (From update of attachment 293313 [details] [diff] [review])
> r=mano, needs ui-r.
> 

IMO not a good decision.

I know my writings are sometimes confusing, so let me try to make it clear again.
If this patch lands, users will not able to clear the filter (clearing the search-field either using the clear-button or by removing the text), when the password manager dialog has been previously opened from the page info dialog with a non-empty filterString as argument to window.open().

LoadSignons() gets called during startup (of the dialog) and in SignonClearFilter(), when the user clears the filter. In this case LoadSignons() will override the empty value of the search-field with the filterString-argument. If it's empty too, ok, but else ...
Attached patch patch v3 (obsolete) — Splinter Review
addressing the issue mentioned in comment #16
Attachment #293341 - Flags: review?(mano)
Attached patch Patch (v4) (obsolete) — Splinter Review
Mano: Ronny is right in comment 16.  This patch pre-filters the table only on startup.  Otherwise attachment 293341 [details] [diff] [review] is the same to my previous patch (attachment 293313 [details] [diff] [review]).  Thanks to Ronny for spotting this problem.

Another issue that my patch has was that setFilter() would get called both at startup and when the dialog is already open, so we need to check if there's already a filter string present, and clear the filter before setting it again in setFilter().

I'm also asking ui-r on this patch.
Attachment #293313 - Attachment is obsolete: true
Attachment #293341 - Attachment is obsolete: true
Attachment #293359 - Flags: ui-review?(beltzner)
Attachment #293359 - Flags: review?(mano)
Attachment #293341 - Flags: review?(mano)
Comment on attachment 293313 [details] [diff] [review]
Patch (v2) (including fix from bug #378668 comment #31)

Setting r- because of the serious problem produced by this patch, as mentioned in comment 16.
Attachment #293313 - Flags: review+ → review-
Comment on attachment 293359 [details] [diff] [review]
Patch (v4)

r=mano
Attachment #293359 - Flags: review?(mano) → review+
Whiteboard: [wanted-firefox3] → [wanted-firefox3][needs ui-r beltzner]
Comment on attachment 293359 [details] [diff] [review]
Patch (v4)

>Index: browser/base/content/pageinfo/security.js
>===================================================================

>+			{filterString : this._getSecurityInfo().hostName});

Nit: Would be nice if the tabs on this line could be replaced by spaces before landing.
Attached patch Patch (v4.1)Splinter Review
(In reply to comment #21)
> (From update of attachment 293359 [details] [diff] [review])
> >Index: browser/base/content/pageinfo/security.js
> >===================================================================
> 
> >+			{filterString : this._getSecurityInfo().hostName});
> 
> Nit: Would be nice if the tabs on this line could be replaced by spaces before
> landing.

Nit addressed.
Attachment #293359 - Attachment is obsolete: true
Attachment #293631 - Flags: ui-review?(beltzner)
Attachment #293631 - Flags: review+
Attachment #293359 - Flags: ui-review?(beltzner)
Ping...
Whiteboard: [wanted-firefox3][needs ui-r beltzner] → [wanted-firefox3][has patch][has review+][needs ui-r beltzner]
(In reply to comment #15)
> (From update of attachment 293313 [details] [diff] [review])
> r=mano, needs ui-r.
> 

Mano, do you really think we need to wait for ui-r here?  As the patch does exactly what the summary says, I would think comment 5 is enough and this only needs approval.
Comment on attachment 293631 [details] [diff] [review]
Patch (v4.1)

I can ui-r sec-ui-related things like this, and as Florian points out, we could probably get away without it anyhow.  :)
Attachment #293631 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 293631 [details] [diff] [review]
Patch (v4.1)

Asking for approval...

(According to the comment 5, this should be considered a blocker now.)
Attachment #293631 - Flags: approval1.9?
Whiteboard: [wanted-firefox3][has patch][has review+][needs ui-r beltzner] → [wanted-firefox3]
Attachment #293631 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in toolkit/components/passwordmgr/content/passwordManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/content/passwordManager.js,v  <--  passwordManager.js
new revision: 1.22; previous revision: 1.21
done
Checking in browser/base/content/pageinfo/security.js;
/cvsroot/mozilla/browser/base/content/pageinfo/security.js,v  <--  security.js
new revision: 1.14; previous revision: 1.13
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008010504 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
https://litmus.mozilla.org/show_test.cgi?id=5295 added to Litmus.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.