Justin Dolske [:Dolske] Assignee
	Description • 10 years ago

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

	Justin Dolske [:Dolske] Assignee
	Comment 1 • 10 years ago

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.

	Matthew N. [:MattN]
	Comment 2 • 10 years ago

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 :)

	Justin Dolske [:Dolske] Assignee
	Comment 3 • 10 years ago

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

	Justin Dolske [:Dolske] Assignee
	Comment 4 • 10 years ago

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.

	Matthew N. [:MattN]
	Comment 5 • 10 years ago

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.

	Justin Dolske [:Dolske] Assignee
	Comment 6 • 10 years ago

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

	Justin Dolske [:Dolske] Assignee
	Comment 8 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/074c77fc983b

	Carsten Book [:Tomcat]
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/074c77fc983b

	Matthew N. [:MattN]
	Comment 10 • 10 years ago

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.

	u474331
	Comment 11 • 10 years ago

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