Closed Bug 998081 Opened 6 years ago Closed 6 years ago

Show login metadata in password manager UI

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
relnote-firefox --- 32+

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: feature)

Attachments

(1 file, 3 obsolete files)

Attached patch Patch v.1 (obsolete) — Splinter Review
Login manager has been recording various metadata (creation time, modification time, last usage time, use count) for a while now, but we've never exposed this in the UI. With the recent Heartbleed attack making everyone change passwords, this seems like an especially glaring omission.

This patch adds those fields to the UI, although only the last-changed and last-used fields are unhidden by default as those seem most useful. I also tweaked the size/flex on the <tree> so that it was less cramped with the new contents.

This patch has one more TODO... Because of our annoying security-through-obscurity behavior of the "Show Passwords" button, the column-chooser can presently bypass that. I think I'll do a part2 patch here that fixes this by setting the text contents of the password column to "<hidden>", and make the button update the contents when clicked. That way showing the password column doesn't actually reveal anything. (Alternatively I'd need to intercept when the columnchooser adds a column, but I don't know if that's even possible.)
Attached patch Patch v.2 (obsolete) — Splinter Review
Fixing the last bit of previous comment was actually trivial. The button already refreshed the list (to allow the search string to match passwords only when shown), so no extra code needed for that.
Assignee: nobody → dolske
Attachment #8408606 - Attachment is obsolete: true
Attachment #8409035 - Flags: review?(MattN+bmo)
Comment on attachment 8409035 [details] [diff] [review]
Patch v.2

Review of attachment 8409035 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea to add the columns :)

Since there were questions/comments that I'd like to know your answer to, marking r- for now.

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +11,5 @@
> +var dateFormatter = new Intl.DateTimeFormat(undefined,
> +                      { day: "numeric", month: "short", year: "numeric" });
> +var dateAndTimeFormatter = new Intl.DateTimeFormat(undefined,
> +                             { day: "numeric", month: "short", year: "numeric",
> +                               hour: "numeric", minute:"numeric" });

Nit: missing space after colon

@@ +63,5 @@
> +        time = new Date(signon.timeCreated);
> +        return dateFormatter.format(time);
> +      case "timeLastUsedCol":
> +        time = new Date(signon.timeLastUsed);
> +        return dateAndTimeFormatter.format(time);

What's the rationale for the usage of dateAndTimeFormatter here instead of dateFormatter like the others? Is the idea that you may want to use this to know if someone else is using your computer to login to your accounts by checking if lastUsed > the last known use? The inconsistency in the UI feels a bit weird so I think think this warrants a comment or someone may remove the inconsistency without understanding the rationale.

::: toolkit/components/passwordmgr/content/passwordManager.xul
@@ +53,5 @@
>      </hbox>
>  
>      <label control="signonsTree" id="signonsIntro"/>
>      <separator class="thin"/>
> +    <tree id="signonsTree" flex="1" style="height: 20em; width: 750px"

Nit: move the style attribute to a new line like the event attributes to help with future blame.

Move the 750 to a width attribute on a new line since it's in pixels and works fine there (unlike em's).

@@ +58,5 @@
>            onkeypress="HandleSignonKeyPress(event)"
>            onselect="SignonSelected();"
>            context="signonsTreeContextMenu">
>        <treecols>
> +        <treecol id="siteCol" label="&treehead.site.label;" flex="2"

Now that the column picker is shown and I'm asking for persistence of the visible columns (see below), we should add @ignoreincolumnpicker="true" to at least this column and probably username and password ones to avoid footguns.

@@ +70,5 @@
>                   onclick="SignonColumnSort('password');" persist="width"
>                   hidden="true"/>
> +        <splitter class="tree-splitter"/>
> +        <treecol id="timeCreatedCol" label="&treehead.timeCreated.label;" flex="1"
> +                 onclick="SignonColumnSort('timeCreated');" persist="width"

I think we should now persist @hidden for all of the columns in the column picker.

@@ +73,5 @@
> +        <treecol id="timeCreatedCol" label="&treehead.timeCreated.label;" flex="1"
> +                 onclick="SignonColumnSort('timeCreated');" persist="width"
> +                 hidden="true"/>
> +        <splitter class="tree-splitter"/>
> +        <treecol id="timeLastUsedCol" label="&treehead.timeLastUsed.label;" flex="1"

This column should also have more flex than the other date ones since it also has the time.

@@ +79,5 @@
> +        <splitter class="tree-splitter"/>
> +        <treecol id="timePasswordChangedCol" label="&treehead.timePasswordChanged.label;" flex="1"
> +                 onclick="SignonColumnSort('timePasswordChanged');" persist="width"/>
> +        <splitter class="tree-splitter"/>
> +        <treecol id="timesUsedCol" label="&treehead.timesUsed.label;" flex="1"

This should have a smaller flex value than all of the rest since it's only a few digits normally.

The following flex values (corresponding to columns) worked for me:
40 25 15 10 20 10 1

::: toolkit/components/passwordmgr/content/passwordManagerCommon.js
@@ +168,5 @@
>  
>    // determine if sort is to be ascending or descending
>    var ascending = (column == lastSortColumn) ? !lastSortAscending : true;
>  
> +  var compareFunc = (a,b) => {

Nit: missing space between arguments. Also, why not just use the normal function syntax here?
"function compareFunc(a, b) {"

::: toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd
@@ +11,5 @@
>  
>  <!ENTITY      treehead.site.label             "Site">
>  <!ENTITY      treehead.username.label         "Username">
>  <!ENTITY      treehead.password.label         "Password">
> +<!ENTITY      treehead.timeCreated.label         "First Used">

I wonder how useful this is and whether it adds more noise than value but I see now that it's hidden by default so we're on the same page :)
Attachment #8409035 - Flags: review?(MattN+bmo) → review-
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #2)

> > +        time = new Date(signon.timeCreated);
> > +        return dateFormatter.format(time);
> > +      case "timeLastUsedCol":
> > +        time = new Date(signon.timeLastUsed);
> > +        return dateAndTimeFormatter.format(time);
> 
> What's the rationale for the usage of dateAndTimeFormatter here instead of
> dateFormatter like the others? 

The creation/modified dates are both going to generally be old (months), and so anything more fine-grained that a day isn't really useful. I suppose they should ideally be a simple number+unit indication of age -- "2 days", "4 weeks", "7 months", etc.

It felt like last-used date would benefit from having a finer-grained date displayed, since it'll often be more recent, and can help with jogging your memory ("I logged into my bank on Monday? Oh, right, I was paying bills during lunch.")

> > +<!ENTITY      treehead.timeCreated.label         "First Used">
> 
> I wonder how useful this is and whether it adds more noise than value but I
> see now that it's hidden by default so we're on the same page :)

Yeah, it's low value but is cheap and can be mildly interesting.
Attached patch Patch v.3 (obsolete) — Splinter Review
Fixed review comments. Reverted the change made in v2 (see comment 0 / comment 1), since removing the password field from the columnpicker makes that issue moot.
Attachment #8409035 - Attachment is obsolete: true
Attachment #8426704 - Flags: review?(MattN+bmo)
Comment on attachment 8426704 [details] [diff] [review]
Patch v.3

Review of attachment 8426704 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with some simple fixes. Ship it!

::: toolkit/components/passwordmgr/content/passwordManager.xul
@@ +75,5 @@
>                   onclick="SignonColumnSort('password');" persist="width"
>                   hidden="true"/>
> +        <splitter class="tree-splitter"/>
> +        <treecol id="timeCreatedCol" label="&treehead.timeCreated.label;" flex="10"
> +                 onclick="SignonColumnSort('timeCreated');" persist="width,hidden"

@persist is supposed to be space delimited[1]. Please fix this (x4) since there a very few places in the tree that use commas instead and I don't know if they work.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/persist

@@ +82,5 @@
> +        <treecol id="timeLastUsedCol" label="&treehead.timeLastUsed.label;" flex="20"
> +                 onclick="SignonColumnSort('timeLastUsed');" persist="width,hidden"/>
> +        <splitter class="tree-splitter"/>
> +        <treecol id="timePasswordChangedCol" label="&treehead.timePasswordChanged.label;" flex="10"
> +                 onclick="SignonColumnSort('timePasswordChanged');" persist="width"/>

You missed persist="width hidden" here

::: toolkit/components/passwordmgr/content/passwordManagerCommon.js
@@ +168,5 @@
>  
>    // determine if sort is to be ascending or descending
>    var ascending = (column == lastSortColumn) ? !lastSortAscending : true;
>  
> +  function compareFunc(a,b) {

nit from last time: missing space between arguments.
Attachment #8426704 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] from comment #5)

> @persist is supposed to be space delimited[1]. Please fix this (x4) since
> there a very few places in the tree that use commas instead and I don't know
> if they work.

Curious, since it works with commas. It's most definitely broken when the attribute is removed entirely, so I guess this just somehow accidentally (?) works. Will change.
Attached patch Patch v.4Splinter Review
Attachment #8426704 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/074c77fc983b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1017883
This isn't a huge change so feel free to minus, but if we're short on relnote stuff this may be a nice addition.
relnote-firefox: --- → ?
Keywords: feature
QA Contact: andrei.vaida
I think this has been a great idea, it helps to manage your security. I'd like to add that since in the nightly versions there is a new preferences style page, maybe it would be nice to adapt the "saved passwords" menu to this style too. I think the way that the extension MaskMe do it is a good approach for this kinf of style (I put some links with screenshots below).

On the other hand, one nice thing that MaskMe show too (as you can see in the link), is the number of times that a password has been reused.

https://d1p4fa0g2cgyhv.cloudfront.net/images/web-v2/maskme/emails/panels/sync/sync_panel3.jpg

http://thewindowsclub.thewindowsclubco.netdna-cdn.com/wp-content/uploads/2013/07/password-display.png?0479ea
You need to log in before you can comment on or make changes to this bug.