Last Comment Bug 465636 - Login Manager should record creation/usage timestamps
: Login Manager should record creation/usage timestamps
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9.3a4
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on: 477917
Blocks: 555755 537653
  Show dependency treegraph
 
Reported: 2008-11-18 17:11 PST by Justin Dolske [:Dolske]
Modified: 2010-03-29 10:26 PDT (History)
5 users (show)
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (WIP) (23.78 KB, patch)
2009-09-08 19:13 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.2 (WIP) (32.20 KB, patch)
2009-09-12 20:41 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.3 (WIP) (37.50 KB, patch)
2009-09-28 22:08 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.4 (WIP) (37.91 KB, patch)
2009-09-28 22:19 PDT, Justin Dolske [:Dolske]
paul: review-
Details | Diff | Review
Patch v.5 (56.16 KB, patch)
2009-12-28 20:39 PST, Justin Dolske [:Dolske]
paul: review+
vladimir: superreview+
Details | Diff | Review

Description Justin Dolske [:Dolske] 2008-11-18 17:11:53 PST
We track creation/usage timestamps on most other data stored in the browser, login manager should too.

Potential uses:
  * Sorting multiple login selection by MRU
  * Management UI to show when a login was last used
  * Allow deleting passwords by time range (CPD/CRH) [not currently wanted,
    but at least we or an extension could in the future.]

Initially we should at least track this data, even if we don't make use of it or expose it via nsILoginInfo.
Comment 1 Justin Dolske [:Dolske] 2008-11-18 17:38:58 PST
See bug 463154 for the form history equivalent of this.

The login manager work will involve:

1) DB migration to add firstUsed, lastUsed, and timesUsed columns.
2) add a heyIJustUsedThisLogin() interface to nsILoginMananger and nsILoginManagerStorage. [Maybe modifyLogin() could be used instead?]
3) Add code to onFormSubmit and the various nsIAuthPrompt[2] interfaces to twiddle the login when it's used. The code already figures out when you're using an existing login (to avoid prompting to save it again), this is easy.
4) Similarly, add code to the paths where a password is changed to twiddle the login (not in modifyLogin() directly, though, since that doesn't necessarily imply use).
5) tests!


If in Private Browsing mode, I think we probably don't want to update the lastUsed/timesUsed data. [We never save new logins in PB mode, but will use existing logins.]

If signon.rememberSignons is disabled, I think we should update lastUsed/timesUsed if there's an existing login. (Users might turn this off to avoid annoying "save login?" prompts, but can still use existing logins.)

A couple of limitations:

* Login Manager can't know when a login is used [as opposed to simply accessed, eg to provide in a selection UI], so code must explicitly tell it a login was used... This means extensions using login manger to store/use credentials will likewise have to do this, or else lastUsed/timesUsed won't be updated.

* AJAX-y login forms that don't actually submit (eg, the login button just spawns an XHR) won't be seen as using a login. I think that's ok, because such forms won't trigger the "save password?" or "change password?" paths anyway.
Comment 2 Justin Dolske [:Dolske] 2009-09-08 19:13:13 PDT
Created attachment 399382 [details] [diff] [review]
Patch v.1 (WIP)

Rough proof of concept, completely untested! :)

A couple of tricky bits left to do:

* We currently bail out early in onFormSubmit when we know we won't save the login. But now sometimes we might want to update the timestamp of an existing login, so these early-bail checks may need to occur later.

* As noted in the last comment, AJAXY forms that don't submit never trigger tyhe onFormSubmit code, so we'd never update the timeLastUsed timestamp. We *could* add a timeLastFilledIn timestamp, though I'm not sure how useful that would be (and now any code wanting to use timestamps would have to decide which field is more interesting).

I also just realized it would be nice to have a timesUsed field (and probably an easy way to increment it, like a dummy timesUsedIncrement for modifyLogin).
Comment 3 Justin Dolske [:Dolske] 2009-09-12 20:41:39 PDT
Created attachment 400324 [details] [diff] [review]
Patch v.2 (WIP)

Mostly working now, needs tests added.

Also just decided I should add another field to show when the login's password was last changed, that would be nice to have.
Comment 4 Justin Dolske [:Dolske] 2009-09-28 22:08:57 PDT
Created attachment 403426 [details] [diff] [review]
Patch v.3 (WIP)

I think this is basically complete now. Passes existing tests, but haven't written new tests yet.

Still not sure what to do about autocomplete=off, so I punted for now (will need to either fix this or have a followup). Maybe if we just change .timeLastUsed to be the last time we filled in a form, we could fix that problem and the AJAXy form problem at the same time...
Comment 5 Justin Dolske [:Dolske] 2009-09-28 22:19:37 PDT
Created attachment 403427 [details] [diff] [review]
Patch v.4 (WIP)

Oops, forgot one little piece.
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-09-29 13:33:14 PDT
Comment on attachment 403427 [details] [diff] [review]
Patch v.4 (WIP)

>+++ b/toolkit/components/passwordmgr/src/nsLoginManager.js
>@@ -852,16 +852,17 @@
> 
>         // Need at least 1 valid password field to do anything.
>         if (newPasswordField == null)
>                 return;
> 
>         // Check for autocomplete=off attribute. We don't use it to prevent
>         // autofilling (for existing logins), but won't save logins when it's
>         // present.
>+        // XXX spin out a bug that we don't update timeLastUsed in this case?

I would say file the bug. We can always close it. I imagine that it's not a super common case. If people complain it's easy enough to fix.

>+++ b/toolkit/components/passwordmgr/src/storage-mozStorage.js
>@@ -37,17 +37,17 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> const Cr = Components.results;
> 
>-const DB_VERSION = 3; // The database schema version
>+const DB_VERSION = 4; // The database schema version

Didn't apply cleanly here. Built on top of another path? (Cr isn't there on trunk)

>@@ -106,28 +106,33 @@
>+            moz_logins:         "id                  INTEGER PRIMARY KEY," +
>+                                "hostname            TEXT NOT NULL,"       +
>+                                "httpRealm           TEXT,"                +
>+                                "formSubmitURL       TEXT,"                +
>+                                "usernameField       TEXT NOT NULL,"       +
>+                                "passwordField       TEXT NOT NULL,"       +
>+                                "encryptedUsername   TEXT NOT NULL,"       +
>+                                "encryptedPassword   TEXT NOT NULL,"       +
>+                                "guid                TEXT,"                +
>+                                "encType             INTEGER,"             +
>+                                "timeCreated         INTEGER,"             +
>+                                "timeLastUsed        INTEGER,"             +
>+                                "timePasswordChanged INTEGER,"             +
>+                                "timesUsed           INTEGER",
>+            // Changes must be reflected in this._dbAreExpectedColumnsPresent(),
>+            // this._searchLogins(), and this.modifyLogin().
>+

I really hate this. I know I wrote it, but we should have a less stringy way of setting up the tables. That way we can cut out the duplication we have with this and _dbAreExpectedColumnsPresent. I keep meaning to file the bug to make that happen. Nothing to do here about it though.

It's not something we worried about too much before, but do we want to specify any default values? Maybe timesUsed=0. The migration sets them all to 1, but it's hard to tell if we might be able to get into a weird state (eg. there's a NULL timesUsed and then we try to increment it... XPCOM might guarantee that becomes a 0... maybe a test for this?)

>@@ -1262,35 +1330,24 @@
> 
> 
>     /*
>      * _dbMigrateToVersion2
>      *
>      * Version 2 adds a GUID column. Existing logins are assigned a random GUID.
>      */
>     _dbMigrateToVersion2 : function () {
>-        // Check to see if GUID column already exists.
>-        let exists = true;
>-        try { 
>-            let stmt = this._dbConnection.createStatement(
>-                           "SELECT guid FROM moz_logins");
>-            // (no need to execute statement, if it compiled we're good)
>-            stmt.finalize();
>-        } catch (e) {
>-            exists = false;
>-        }
>+        // Check to see if GUID column already exists, add if needed
>+        let query;
>+        if (!this._columnExists("guid")) {
>+            query = "ALTER TABLE moz_logins ADD COLUMN guid TEXT";
>+            this._dbConnection.executeSimpleSQL(query);
> 
>-        // Add the new column and index only if needed.
>-        if (!exists) {
>-            this._dbConnection.executeSimpleSQL(
>-                "ALTER TABLE moz_logins ADD COLUMN guid TEXT");
>-
>-            this._dbConnection.executeSimpleSQL(
>-                "CREATE INDEX IF NOT EXISTS " +
>-                    "moz_logins_guid_index ON moz_logins (guid)");
>+            query = "CREATE INDEX IF NOT EXISTS moz_logins_guid_index ON moz_logins (guid)";
>+            this._dbConnection.executeSimpleSQL(query);
>         }

I like the migration cleanup, but isn't it more appropriate to do it in a different bug which gets landed first?

>     /*
>+     * _dbMigrateToVersion4
>+     *
>+     * Version 4 adds timeCreated, timeLastUsed, timePasswordChanged,
>+     * and timesUsed columns
>+     */
>+    _dbMigrateToVersion4 : function () {
>+        let query;
>+        if (!this._columnExists("timeCreated")) {
>+            query = "ALTER TABLE moz_logins ADD COLUMN timeCreated INTEGER";
>+            this._dbConnection.executeSimpleSQL(query);
>+        }
>+        if (!this._columnExists("timeLastUsed")) {
>+            query = "ALTER TABLE moz_logins ADD COLUMN timeLastUsed INTEGER";
>+            this._dbConnection.executeSimpleSQL(query);
>+        }
>+        if (!this._columnExists("timePasswordChanged")) {
>+            query = "ALTER TABLE moz_logins ADD COLUMN timePasswordChanged INTEGER";
>+            this._dbConnection.executeSimpleSQL(query);
>+        }
>+        if (!this._columnExists("timesUsed")) {
>+            query = "ALTER TABLE moz_logins ADD COLUMN timesUsed INTEGER";
>+            this._dbConnection.executeSimpleSQL(query);
>+        }

Should we be explicit like this or can we do something more like this? If these tables are all identical it's just slightly cleaner.
> for each(let table in ["timeCreated",...]) ...

>+        try {
>+            stmt = this._dbCreateStatement(query);
>+            while (stmt.step())

s/step/executeStep/

>+        let params = {
>+            id:   null,
>+            initTime: Date.now()
>+        };

Nit: keep values lined up for consistency.

>     /*
>+     * _columnExists
>+     *
>+     * Checks to see if the named column already exists.
>+     */

Can we call this _dbColumnExists (for consistency with the other db related functions)

>+    _columnExists : function (columnName) {
>+        let exists = true;
>+        let query = "SELECT " + columnName + " FROM moz_logins";
>+        try {
>+            let stmt = this._dbConnection.createStatement(query);
>+            // (no need to execute statement, if it compiled we're good)
>+            stmt.finalize();
>+        } catch (e) {
>+            exists = false;
>+        }
>+        return exists;
>+    },

Can cut out exists here and just return false in the catch, true at the end or in the try. That's just my style preference, it's up to you. Also seen in _bagHasProperty

r- for now.
Comment 7 Justin Dolske [:Dolske] 2009-10-09 20:01:05 PDT
(In reply to comment #6)

> It's not something we worried about too much before, but do we want to specify
> any default values? Maybe timesUsed=0. The migration sets them all to 1, but
> it's hard to tell if we might be able to get into a weird state (eg. there's a
> NULL timesUsed and then we try to increment it... XPCOM might guarantee that
> becomes a 0... maybe a test for this?)

That shouldn't be a problem here. The migration ensures that all rows should have valid data, even if it was added by an earlier version of the code (which downgrades the DB version, so we know to run the upgrade code again).
Comment 8 Justin Dolske [:Dolske] 2009-12-28 20:39:02 PST
Created attachment 419380 [details] [diff] [review]
Patch v.5

Now with tests!
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-12-29 15:24:51 PST
(In reply to comment #8)
> Created an attachment (id=419380) [details]
> Patch v.5

So I'm a fan of more atomic commits, and the cleanup of migrations/_dbColumnExists is really not directly related to this bug. I'd still like to see that separated out. I'd be more OK with it if we were rushing to get this into a release, but we're not. Otherwise it looks good.
Comment 10 Justin Dolske [:Dolske] 2009-12-30 13:43:26 PST
Eh, it's just a small change and doesn't seem worth separating to me.
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-12-30 15:59:59 PST
Comment on attachment 419380 [details] [diff] [review]
Patch v.5

(In reply to comment #10)
> Eh, it's just a small change and doesn't seem worth separating to me.

It's a change I'd want made anyway, so fine. r=zpao
Comment 12 Justin Dolske [:Dolske] 2010-03-11 15:51:50 PST
Comment on attachment 419380 [details] [diff] [review]
Patch v.5

zpao's an official reviewer now, so this is good to go. Flagging vlad for a SR on the interface changes.
Comment 13 Vladimir Vukicevic [:vlad] [:vladv] 2010-03-19 15:46:24 PDT
Comment on attachment 419380 [details] [diff] [review]
Patch v.5

sr=me on idl changes
Comment 14 Justin Dolske [:Dolske] 2010-03-19 16:23:36 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/9835f96162e6

Note You need to log in before you can comment on or make changes to this bug.