1058438 - Password manager should use the permission manager to manage domains marked as "never remember"

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

As part of bug 771630, we need the PasswordManager to record a timestamp for when the "never remember for this site" was changed, and the ability to reset that flag for all domains where it was set after the specified time.

This might be slightly tricky, as there are a number of interfaces involved which are heavily used by important addons (ie, password managers).  At a minimum, the implementation will need to guard against any new methods not existing on both nsILoginManager and nsILoginManagerStorage.

For our implementation of these interfaces:

* The table moz_disabledHosts will need a new field (probably with index) and code to perform a schema migration and relevant tests.

* Ensure the new timestamp is updated whenever that table is updated.

* Add new methods to both nsILoginManager and nsILoginManagerStorage to allow removal of sites that were disabled after a specified time.  This is likely to require a refactor of _queryDisabledHosts.

I'm sure I'm looking at this naively, so needinfo dolske for any insights he has into additional considerations given 3rd party password managers.

Comment 1

[Justin Dolske [:Dolske]]

(In reply to Mark Hammond [:markh] from comment #0)
> As part of bug 771630, we need the PasswordManager to record a timestamp for
> when the "never remember for this site" was changed, and the ability to
> reset that flag for all domains where it was set after the specified time.

I'd concur with MattN's bug 771630 comment 7 statement -- we should probably just go ahead and move these pwmgr exceptions into nsIPermissionManager. It's just a historical quirk that they're separate.

Its not a hard requirement, but I think the amount of work to do so is smaller or equal to adding a timestamp to the pwmgr permissions.

(Hmm, does Sync sync pwmgr expections? I don't think so...)

> This might be slightly tricky, as there are a number of interfaces involved
> which are heavily used by important addons (ie, password managers).  At a
> minimum, the implementation will need to guard against any new methods not
> existing on both nsILoginManager and nsILoginManagerStorage.

I'm not too worried about this. AFAIK 3rd-party password managers don't even use the nsILogin* interfaces. I'd be more concerned about random addons' usage of nsILoginManager for storing various credentials, but those APIs are pretty distinct from get/setLoginSavingEnabled()... A quick peruse of the Addons MXR shows fairly limited usage (amusingly, a number are using it completely wrong, by passing in a _username_!). We could even retain the functions of these APIs if we really want to, since the addition of a timestame / switch to nsIPermissionManager shouldn't impede their basic purpose. Worth thinking about, but my initial impression is that it's not a concern / worth doing.

> * The table moz_disabledHosts will need a new field (probably with index)
> and code to perform a schema migration and relevant tests.

Why an index? Seems like it wouldn't help much, since I'd expect these lists to generally be short.

> * Ensure the new timestamp is updated whenever that table is updated.

Yes, although this should be extremely infrequent. And we should get it for free, since there's no reason to call setLoginSavingEnabled(false) on an already disabled host, and setLoginSavingEnabled(true) should just clear out the entry if it does exist.

> * Add new methods to both nsILoginManager and nsILoginManagerStorage to
> allow removal of sites that were disabled after a specified time.  This is
> likely to require a refactor of _queryDisabledHosts.

Could just be a separate new function like clearDisabledTimeRange(min,max) that executes a trivial SQL statement.

Comment 2

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #1)
> (In reply to Mark Hammond [:markh] from comment #0)
> > As part of bug 771630, we need the PasswordManager to record a timestamp for
> > when the "never remember for this site" was changed, and the ability to
> > reset that flag for all domains where it was set after the specified time.
> 
> I'd concur with MattN's bug 771630 comment 7 statement -- we should probably
> just go ahead and move these pwmgr exceptions into nsIPermissionManager.
> It's just a historical quirk that they're separate.

Sounds good, but then this is blocked on bug 1058433... Marco, can you update the spreadsheet accordingly?

Comment 3

[Marco Mucci [:MarcoM]]

Spreadsheet updated and block added.

> 
> Sounds good, but then this is blocked on bug 1058433... Marco, can you
> update the spreadsheet accordingly?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

One issue I've found with this approach is that the permission manager doesn't take a URL's scheme into account - it works just with hosts.  Eg:

Eg, the browser scratchpad session below shows adding a permission to http://example.com also has the permission applied to https://example.com
--8< --
let uri = Services.io.newURI("http://example.com", null, null);
Services.perms.add(uri, "test", Services.perms.ALLOW_ACTION);

Services.perms.testPermission(uri, "test");
/*
1
*/
let uri2 = Services.io.newURI("https://example.com", null, null);
Services.perms.testPermission(uri2, "test");

/*
1
*/
---8<---

However, the login manager does treat different schemes as different permissions and this is explicitly tested in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/unit/test_disabled_hosts.js#20

Assuming the current login manager implementation is a hard requirement, this seems a difficult incompatibility to resolve.  Any thoughts?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #4)
> One issue I've found with this approach is that the permission manager
> doesn't take a URL's scheme into account - it works just with hosts.  Eg:

FTR, the same basic issue also exists with ports.  The login manager treats URLs with different ports (and even the same port when the default port is explicitly specified) as different.

--8<--
Services.logins.setLoginSavingEnabled("http://example.com", false);
Services.logins.getLoginSavingEnabled("http://example.com");
/*
false
*/
Services.logins.getLoginSavingEnabled("http://example.com:80");
/*
true
*/
--8<--

contrast this with the permission manager:

--8<--
let uri = Services.io.newURI("http://example.com", null, null);
Services.perms.add(uri, "test2", Services.perms.ALLOW_ACTION);
Services.perms.testPermission(uri, "test2");
/*
1
*/
let uri2 = Services.io.newURI("http://example.com:999", null, null);
Services.perms.testPermission(uri2, "test2");
/*
1
*/
--8<--

So a simple work-around of using, eg, "login-http" and "login-https" as the permission name isn't going to be enough, and extending this to include the port number seems a hack-too-far...

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Mark Hammond [:markh] from comment #4)

> Assuming the current login manager implementation is a hard requirement,
> this seems a difficult incompatibility to resolve.  Any thoughts?

I don't think it's a requirement. Let's just drop the port and protocol when migrating to the permissions manager.

Comment 7

[Justin Dolske [:Dolske]]

Oh, I suppose that also means the new implementation of get/setLoginSavingEnabled (using permissions manager,to avoid API bustage) would also ignore protocol/port, but I think that's fine too.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: proof of concept

I'm not planning to work on this, but thought I should outline the current plan that dolske and I came up with for the poor soul who draws this short straw :)

* The password manager is also used by Fennec - but Fennec uses a different storage implementation - sqlite vs json used in the desktop.  We decided to try and avoid touching Fennec for now.

* The JSON storage implementation would, at startup, check to see if there are any disabledHosts in the JSON data, and if so, migrate them to the permission manager and delete them from the JSON.  Thus, this migration should only happen once and should handle the case when the user then uses the profile with an earlier Fx version, potentially causing items to reappear.

* The storage implementation would just delegate the 3 related methods (setLoginSavingEnabled, getLoginSavingEnabled and getAllDisabledHosts) to the permission manager.

* At some point in the future we would issue deprecation warnings when these methods are called, but we can't do that until all callers in the tree are updated to call the permission manager directly - and we probably can't do this until Fennec is addressed.

* Delegation to the permission manager has a number of implications for callers.  The current implementation treats, eg, http:// and https:// schemes for the same host as different entries, and similarly for hosts with a port explicitly specified.  However, the permission manager does not make this distinction.  This is of particular concern for getAllDisabledHosts() - currently that returns a full URL (ie, including scheme and optionally port) whereas with this model it would change to just returning the host.  As getAllDisabledHosts() often just passes the hosts back to setLoginSavingEnabled(), we'd want to change set/getLoginSavingEnabled to accept *either* a full URL or just a host.

* We then need to look at all callers in the tree of these 3 methods.  Ones that are only used by desktop should be changed to call the permission manager directly.  Those shared with Fennec will need to be tested to ensure they still work correctly in both cases.

* We open a bug for the fennec team to do their side of the migration.  The thinking here was that they could treat this as a "schema upgrade" and use the schema version as the trigger to perform the migration to the permission manager.

* We open a bug to issue the deprecation warnings.  These would primarily be for addon authors so they know to change to the new world order.

* We possibly open a bug to remove these methods completely at some point far in the future.

FTR, attaching a very rough proof-of-concept - it doesn't do much useful at all yet, but should give some idea...

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #8)
> * Delegation to the permission manager has a number of implications for
> callers.  The current implementation treats, eg, http:// and https://
> schemes for the same host as different entries, and similarly for hosts with
> a port explicitly specified.  However, the permission manager does not make
> this distinction.

Turns out this is a bug rather than a feature!  See bug 1066517.

Comment 10

[Matthew N. [:MattN]]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Mark Hammond [:markh] from comment #8)
> > * Delegation to the permission manager has a number of implications for
> > callers.  The current implementation treats, eg, http:// and https://
> > schemes for the same host as different entries, and similarly for hosts with
> > a port explicitly specified.  However, the permission manager does not make
> > this distinction.
> 
> Turns out this is a bug rather than a feature!  See bug 1066517.

That's changing in bug 1165263 now :)

Comment 11

[Kapeel Sable]

Hi, I would like to take on this bug. Could you outline the steps I should take? Thanks!

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kapeel Sable from comment #11)
> Hi, I would like to take on this bug. Could you outline the steps I should
> take? Thanks!

That's awesome! This bug may turn out to be alot of work, but if that doesn't deter you, the comments above have most of the information you need, particularly comment 8 and the patch attached. However, the good news is that the talk about the http/https/port distinction is no longer relevant and can be ignored - the permission manager now does correctly treat http/https/different-port-numbers as unique entries in the same way the login manager requires.

Let us know if you have any questions (and select the "need more information from" :MattN or myself so we don't miss it :)

Comment 13

[Kapeel Sable]

Attachment: 1058438_poc.diff

I recently started working on this. I used your patch as the starting point and modified it to work as expected (the MDN docs about nsIPermissionManager.remove() seem out of date!).

(In reply to Mark Hammond [:markh] from comment #8)

> * The JSON storage implementation would, at startup, check to see if there
> are any disabledHosts in the JSON data, and if so, migrate them to the
> permission manager and delete them from the JSON.  Thus, this migration
> should only happen once and should handle the case when the user then uses
> the profile with an earlier Fx version, potentially causing items to
> reappear.

I didn't touch this part of the patch, I am a little confused due inexperience with profiles within earlier Firefox version. Could you please guide as to what more needs to be done here?

> * The storage implementation would just delegate the 3 related methods
> (setLoginSavingEnabled, getLoginSavingEnabled and getAllDisabledHosts) to
> the permission manager.

I feel this is complete and it's working per my testing.

> * Delegation to the permission manager has a number of implications for
> callers.  The current implementation treats, eg, http:// and https://
> schemes for the same host as different entries, and similarly for hosts with
> a port explicitly specified.  However, the permission manager does not make
> this distinction.  This is of particular concern for getAllDisabledHosts() -
> currently that returns a full URL (ie, including scheme and optionally port)
> whereas with this model it would change to just returning the host.  As
> getAllDisabledHosts() often just passes the hosts back to
> setLoginSavingEnabled(), we'd want to change set/getLoginSavingEnabled to
> accept *either* a full URL or just a host.

I ignored this part like you said in the last comment.

> * We then need to look at all callers in the tree of these 3 methods.  Ones
> that are only used by desktop should be changed to call the permission
> manager directly.  Those shared with Fennec will need to be tested to ensure
> they still work correctly in both cases.

How do I distinguish callers used by desktop? Should I be looking in any specific directories?

I believe the next step in fixing this bug is changing the callers used by desktop Firefox.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8704613 [details] [diff] [review]
1058438_poc.diff

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

I'm very sorry for the delay here. I think this seems to be looking OK, but when it gets closer I'll be asking someone else to help with the reviews.

> How do I distinguish callers used by desktop? Should I be looking in any specific directories?

Probably just by searching for the function names in dxr.mozilla.org.

> I believe the next step in fixing this bug is changing the callers used by desktop Firefox.

Apart from the comments below, that sounds correct.

::: toolkit/components/passwordmgr/storage-json.js
@@ +88,5 @@
>          // We won't attempt import again on next startup.
>          Services.prefs.setBoolPref("signon.importedFromSqlite", true);
> +      }.bind(this)).catch(Cu.reportError)
> +      .then(Task.spawn(function () {
> +          // If the storage has a disabledHosts entry we migrate them

nit - this function is using 4-space indents - it should be 2.

@@ +408,5 @@
>      this._sendNotification("removeAllLogins", null);
>    },
>  
> +  // All disabledHost APIs are now delegated to the permission manager
> +  // after issuing a deprecation warning 

nit: whitespace at the end of line (and I guess it should end with a period)

@@ +414,2 @@
>    getAllDisabledHosts(count) {
> +    let enumerator = Services.perms.enumerator;

We need the deprecation warning, and I think we should make an attempt to find all the callers in the tree and update them.

@@ +414,5 @@
>    getAllDisabledHosts(count) {
> +    let enumerator = Services.perms.enumerator;
> +    let disabledHosts = [];
> +    while (enumerator.hasMoreElements()) {
> +        let perm = enumerator.getNext();

nit: 4 space indents in this block should be replaced with 2 spaces.

@@ +426,5 @@
>        count.value = disabledHosts.length; // needed for XPCOM
>      return disabledHosts;
>    },
>  
>    getLoginSavingEnabled(hostname) {

A quick look at the callers implies they are correctly passing a URL rather than just the hostname, so I think we should rename this param to something like origin.

We should probably issue the deprecation warning here too and change all callers in the tree where possible.

@@ +434,3 @@
>    },
>  
>    setLoginSavingEnabled(hostname, enabled) {

comments above apply here too

Comment 15

[Matthew N. [:MattN]]

Hey Kapeel, are you still interested in finishing this?

Comment 16

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Delegate disabledHost APIs to the permission manager.

Review commit: https://reviewboard.mozilla.org/r/62440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62440/

Comment 17

[Saad Quadri [:saadq]]

(In reply to Saad Quadri [:saadq] from comment #16)
> Created attachment 8768143 [details]
> Bug 1058438 - Delegate disabledHost APIs to the permission manager.
> 
> Review commit: https://reviewboard.mozilla.org/r/62440/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/62440/

^ Addressed nits.

Comment 18

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Add additional tests for disabledHost APIs in desktop with nonascii characters in URLs.

Review commit: https://reviewboard.mozilla.org/r/63772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63772/

Comment 19

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review commit: https://reviewboard.mozilla.org/r/63774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63774/

Comment 20

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Add test for disabledHost migration for sqlite v6 in Fennec.

Review commit: https://reviewboard.mozilla.org/r/63776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63776/

Comment 21

[Saad Quadri [:saadq]]

Comment on attachment 8768143 [details]
Bug 1058438 - Delegate disabledHost APIs to the permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62440/diff/1-2/

Comment 22

[Saad Quadri [:saadq]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63774/diff/1-2/

Comment 23

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Migrate disabledHosts from json storage to permission manager.

Review commit: https://reviewboard.mozilla.org/r/63788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63788/

Comment 24

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

Review commit: https://reviewboard.mozilla.org/r/63790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63790/

Comment 25

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

Review commit: https://reviewboard.mozilla.org/r/63792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63792/

Comment 26

[Saad Quadri [:saadq]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63774/diff/2-3/

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8770322 [details]
Bug 1058438 - Migrate disabledHosts from json storage to permission manager.

https://reviewboard.mozilla.org/r/63788/#review60836

::: toolkit/components/passwordmgr/storage-json.js:30
(Diff revision 1)
> +// The permission type we store in the permission manager.
> +const PERMISSION_TYPE_LOGIN = "login-saving";

Nit: PERMISSION_SAVE_LOGINS is more descriptive. I think this name was leftover from when it was "logins".

::: toolkit/components/passwordmgr/storage-json.js:105
(Diff revision 1)
>      } catch (e) {
> +      dump("Initialization failed: " + e + "\n");
>        this.log("Initialization failed:", e);
>        throw new Error("Initialization failed");

Please remove this line as the `this.log` should be sufficient.

::: toolkit/components/passwordmgr/storage-json.js:398
(Diff revision 1)
> +  // All disabledHost APIs are now delegated to the permission manager
> +  // after issuing a deprecation warning.
>    getAllDisabledHosts(count) {

This comment is stale as we aren't going to do a deprecation warning.

::: toolkit/components/passwordmgr/storage-json.js
(Diff revision 1)
> -    this.log("_getAllDisabledHosts: returning", disabledHosts.length, "disabled hosts.");
>      if (count)
>        count.value = disabledHosts.length; // needed for XPCOM
> +
> +    this.log("_getAllDisabledHosts: returning", disabledHosts.length, "disabled hosts.");

Nit: can you move this back to avoid changing blame unnecessarily.

Comment 28

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63788/#review60840

::: toolkit/components/passwordmgr/LoginStore.jsm:272
(Diff revision 1)
> +    // Stub needed for login imports before data has been migrated.
>      if (!this.data.disabledHosts) {
>        this.data.disabledHosts = [];
>      }

Please file a follow-up bug blocking this one to remove this backwards compatible code at some point.

::: toolkit/components/passwordmgr/storage-json.js:398
(Diff revision 1)
> +  // All disabledHost APIs are now delegated to the permission manager
> +  // after issuing a deprecation warning.
>    getAllDisabledHosts(count) {

Can you also remove the code at https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/toolkit/forgetaboutsite/ForgetAboutSite.jsm#147 since it's basically a duplicate of the permission code below it now.

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8770323 [details]
Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

https://reviewboard.mozilla.org/r/63790/#review60844

::: toolkit/components/passwordmgr/test/unit/test_disabled_hosts.js:170
(Diff revision 1)
> +  let hostname = "http://√.com";
> +  let encoding = "http://xn--19g.com";
> +
> +  Services.logins.setLoginSavingEnabled(hostname, false);

I had suggested testing setting of an ascii "xn--…" version as well. Can you please add that too?

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

https://reviewboard.mozilla.org/r/63774/#review60846

::: toolkit/components/passwordmgr/storage-mozStorage.js:7
(Diff revision 3)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> -const DB_VERSION = 5; // The database schema version
> +const DB_VERSION = 6; // The database schema version
> +const PERMISSION_TYPE_LOGIN = "login-saving";

Same name comment as JSON

::: toolkit/components/passwordmgr/storage-mozStorage.js:638
(Diff revision 3)
> +      if (perm.type == PERMISSION_TYPE_LOGIN
> +          && perm.capability == Services.perms.DENY_ACTION) {

This can fit on one line (< 100 characters). This same applies to JSON.

::: toolkit/components/passwordmgr/storage-mozStorage.js:1314
(Diff revision 3)
> +      query = "FLUSH TABLE moz_disabledHosts";
> +      this._dbConnection.executeSimpleSQL(query);

Did you test this? I don't see any evidence that FLUSH TABLE works on sqlite. StackOverflow is suggesting a `DELETE FROM` with no `WHERE`.

Comment 31

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63774/#review60852

::: toolkit/components/passwordmgr/storage-mozStorage.js:119
(Diff revision 3)
>        moz_disabledHosts:  "id                 INTEGER PRIMARY KEY," +
>                            "hostname           TEXT UNIQUE ON CONFLICT REPLACE",

We shouldn't be creating the table in new profiles. the `_queryDisableHosts` method conveniently returns an empty array if the query fails so this won't break a user who uses their profile first on a build with this patch but then downgrades to an old one.

::: toolkit/components/passwordmgr/storage-mozStorage.js:820
(Diff revision 3)
>     * _queryDisabledHosts
>     *
>     * Returns an array of hostnames from the database according to the
>     * criteria given in the argument. If the argument hostname is null, the
>     * result array contains all hostnames
>     */
>    _queryDisabledHosts : function (hostname) {
>      let disabledHosts = [];

I think this is unused now

::: toolkit/components/passwordmgr/storage-mozStorage.js:1356
(Diff revision 3)
>      query = "SELECT " +
>                 "id, " +
>                 "hostname " +
>              "FROM moz_disabledHosts";
>      try {
>        let stmt = this._dbConnection.createStatement(query);
>        // (no need to execute statement, if it compiled we're good)
>        stmt.finalize();
>      } catch (e) {
>        return false;
>      }

This should probably be removed too

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8770324 [details]
Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

https://reviewboard.mozilla.org/r/63792/#review60850

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:9
(Diff revision 1)
>   */
>  
>  
>  const ENCTYPE_BASE64 = 0;
>  const ENCTYPE_SDR = 1;
> +const PERMISSION_TYPE_LOGIN = "login-saved";

Same comment as the other two.

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:175
(Diff revision 1)
>  storage = reloadStorage(OUTDIR, "signons-v1.sqlite");
> +storage.setLoginSavingEnabled("https://disabled.net", false);
>  checkStorageData(storage, ["https://disabled.net"], [testuser1, testuser2]);
> +storage.setLoginSavingEnabled("https://disabled.net", true);

Why is the necessary for the upgrade cases? I thought the set would only be needed for downgrade.

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:389
(Diff revision 1)
>  do_check_true(dbConnection.tableExists("moz_deleted_logins"));
>  
> -
>  /* ========== 11 ========== */
>  testnum++;
> +testdesc = "Test upgrade from v4->v6 storage" // TODO: Should be v5->v6

Nit: you can remove the TODO

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:389
(Diff revision 1)
> +testdesc = "Test upgrade from v4->v6 storage" // TODO: Should be v5->v6
> +
> +yield* copyFile("signons-v4.sqlite");
> +
> +// Sanity check the test file.
> +dbConnection = openDB("signons-v4.sqlite");

When I said that you can use the v4 sqlite file I thought that it contained some disabled host entries already so it's actually testing the upgrade. Since it doesn't I think we should make a signons-v5v6.sqlite which has moz_disabledHosts records and test that they are migrated.

Comment 34

[Justin Dolske [:Dolske]]

Driveby comment: why not delete [gs]etLoginSavingEnabled and getAllDisabledHosts from nsILoginManagerStorage.idl (and the implementations, obviously), and have nsLoginManager.js's version of these APIs talk directly with the permission manager? (instead of delegating to the storage module, as they do now)

Also, it's worth noting that actually deleting the moz_disabledHosts table is will break forward-compatibility -- if an Android user ever runs an older version of Fennec, _dbAreExpectedColumnsPresent() will throw and we treat the DB as corrupted. The good news is that we'll recover, the bad news is that we do so by nuking the entire DB and starting over from scratch -- all your passwords are deleted.

If this was on desktop, I'd say that's unacceptable risk of dataloss, and that we should avoid it by keeping the (empty) moz_disabledHosts table around permanently (and continue to create it in new profiles)... OTOH, this is Android, and I'm not sure that we ever expect users to run older versions. (And AIUI the different release channels on Android effectively have different profiles, so you can't hit this by trying a Nightly and then switching back to Release.)

Still -- is that an explicit decision that was considered?

Comment 35

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #34)
> Driveby comment: why not delete [gs]etLoginSavingEnabled and
> getAllDisabledHosts from nsILoginManagerStorage.idl (and the
> implementations, obviously), and have nsLoginManager.js's version of these
> APIs talk directly with the permission manager? (instead of delegating to
> the storage module, as they do now)

Good idea. I forgot about the indirection here.

> Also, it's worth noting that actually deleting the moz_disabledHosts table
> is will break forward-compatibility -- if an Android user ever runs an older
> version of Fennec, _dbAreExpectedColumnsPresent() will throw and we treat
> the DB as corrupted. The good news is that we'll recover, the bad news is
> that we do so by nuking the entire DB and starting over from scratch -- all
> your passwords are deleted.
> 
> If this was on desktop, I'd say that's unacceptable risk of dataloss, and
> that we should avoid it by keeping the (empty) moz_disabledHosts table
> around permanently (and continue to create it in new profiles)... OTOH, this
> is Android, and I'm not sure that we ever expect users to run older
> versions. (And AIUI the different release channels on Android effectively
> have different profiles, so you can't hit this by trying a Nightly and then
> switching back to Release.)
> 
> Still -- is that an explicit decision that was considered?

Yes, I did think about it and that's why we're not deleting the table, only emptying it.

I was suggesting in comment 31 that we don't create the table for new profiles in new builds which means we would hit this problem if a mobile user first installs a version using perm-mgr, saves some passwords, and then downgrades. In my skimming of the code I didn't realize we would nuke the DB for that case. I guess we can continue to create the empty table until enough versions of Fx in the wild have the new code and we no longer care about downgrading to the older ones.

As you say though, because this is Android, maybe we don't need to worry about this case since it's uncommon/hard to do a downgrade with the same profile? I guess it may be possible via installing from FTP or restoring an old backup of an app somehow but those seem like edge cases.

Comment 36

[Saad Quadri [:saadq]]

Attachment: Bug 1058438 - Remove disabledHost APIs from storage and use permission manager directly in LoginManager.

Review commit: https://reviewboard.mozilla.org/r/65372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65372/

Comment 37

[Saad Quadri [:saadq]]

Comment on attachment 8770322 [details]
Bug 1058438 - Migrate disabledHosts from json storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63788/diff/1-2/

Comment 38

[Saad Quadri [:saadq]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63774/diff/3-4/

Comment 39

[Saad Quadri [:saadq]]

Comment on attachment 8770323 [details]
Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63790/diff/1-2/

Comment 40

[Saad Quadri [:saadq]]

Comment on attachment 8770324 [details]
Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63792/diff/1-2/

Comment 41

[Matthew N. [:MattN]]

Comment on attachment 8772616 [details]
Bug 1058438 - Remove disabledHost APIs from storage and use permission manager directly in LoginManager.

https://reviewboard.mozilla.org/r/65372/#review62382

::: toolkit/components/passwordmgr/LoginHelper.jsm:696
(Diff revision 1)
> +   * Send a notification when stored data is changed.
> +   */
> +  sendNotification(changeType, data) {

As I suggested when we were sitting together the other day, I think this method name should be named something like `notifyStorageChanged` since LoginHelper.sendNotification doesn't tell me what kind of notificatoin it will send and pwmgr sends more than one kind of notification.

::: toolkit/components/passwordmgr/nsLoginManager.js:357
(Diff revision 1)
>    getAllDisabledHosts(count) {
> -    log.debug("Getting a list of all disabled origins");
> +    let disabledHosts = [];

Can you leave this log line at the beginning please?

::: toolkit/components/passwordmgr/nsLoginManager.js:370
(Diff revision 1)
> +
> +    log.debug("_getAllDisabledHosts: returning", disabledHosts.length, "disabled hosts.");
> +    return disabledHosts;

Nit: remove the leading underscore on the log message so it matches the method name

::: toolkit/components/passwordmgr/nsLoginManager.js:456
(Diff revision 1)
> +    if (enabled)
> +      Services.perms.remove(uri, PERMISSION_SAVE_LOGINS);
> +    else
> +      Services.perms.add(uri, PERMISSION_SAVE_LOGINS, Services.perms.DENY_ACTION);
>  

The newer style is to include braces `if`/`else` even if their body is a single-line.

Comment 42

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63788/#review62388

Comment 43

[Matthew N. [:MattN]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

https://reviewboard.mozilla.org/r/63774/#review62392

::: toolkit/components/passwordmgr/storage-mozStorage.js:1198
(Diff revision 4)
> +      while (stmt.executeStep())
> +        disabledHosts.push(stmt.row.hostname);
> +

Nit: Can you include braces here too?

::: toolkit/components/passwordmgr/storage-mozStorage.js:1201
(Diff revision 4)
> +      for (let host of disabledHosts) {
> +        let uri = Services.io.newURI(host, null, null);
> +        Services.perms.add(uri, PERMISSION_SAVE_LOGINS, Services.perms.DENY_ACTION);

I'm thinking we should wrap the inside of the loop in `try…catch` as I worry about invalid URIs causing newURI to throw and I think we should continue migrating the others in that case. We can log with Cu.reportError if that happens.

Comment 44

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63788/#review62396

::: toolkit/components/passwordmgr/storage-json.js:97
(Diff revision 2)
> +        for (let host of this._store.data.disabledHosts) {
> +          let uri = Services.io.newURI(host, null, null);
> +          Services.perms.add(uri, PERMISSION_SAVE_LOGINS, Services.perms.DENY_ACTION);
> +        }

(same as mozstorage:) I'm thinking we should wrap the inside of the loop in `try…catch` as I worry about invalid URIs causing newURI to throw and I think we should continue migrating the others in that case. We can log with Cu.reportError if that happens.

Comment 45

[Matthew N. [:MattN]]

Comment on attachment 8770323 [details]
Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

https://reviewboard.mozilla.org/r/63790/#review62398

Comment 46

[Matthew N. [:MattN]]

Comment on attachment 8770324 [details]
Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

https://reviewboard.mozilla.org/r/63792/#review62402

Looking good. Just some small issues remaining.

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:62
(Diff revision 2)
> -function checkStorageData(storage, ref_disabledHosts, ref_logins)
> +function checkStorageData(storage, ref_logins)
>  {
>    LoginTestUtils.assertLoginListsEqual(storage.getAllLogins(), ref_logins);
> -  LoginTestUtils.assertDisabledHostsEqual(storage.getAllDisabledHosts(),
> -                                          ref_disabledHosts);
>  }

So my understanding when we discussed removing this on IRC was that it wasn't adding value over the test specifically for disabledHosts but in this case it actually is testing that the data is preserved across the various schema versions so I think we should keep it. Isn't the simple solution to change the line to get all the disabled hosts from the permission manager then pass them to assertDisabledHostsEqual?

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:91
(Diff revision 2)
>      var encType = stmt.row.encType;
>      stmt.finalize();
>      return encType;
>  }
>  
> +function getAllDisabledHosts(conn) {

`getAllDisabledHostsFromMozStorage`?

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:95
(Diff revision 2)
> +    while (stmt.executeStep())
> +      disabledHosts.push(stmt.row.hostname);
> +

Nit: braces please

::: toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js:407
(Diff revision 2)
> +];
> +
> +LoginTestUtils.assertDisabledHostsEqual(disabledHosts, getAllDisabledHosts(dbConnection));
> +
> +// Reload storage
> +storage = reloadStorage(OUTDIR, "signons-v5v6.sqlite");

Reminder to VACUUM this file if you didn't already

Comment 47

[Saad Quadri [:saadq]]

Comment on attachment 8772616 [details]
Bug 1058438 - Remove disabledHost APIs from storage and use permission manager directly in LoginManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65372/diff/1-2/

Comment 48

[Saad Quadri [:saadq]]

Comment on attachment 8770322 [details]
Bug 1058438 - Migrate disabledHosts from json storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63788/diff/2-3/

Comment 49

[Saad Quadri [:saadq]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63774/diff/4-5/

Comment 50

[Saad Quadri [:saadq]]

Comment on attachment 8770323 [details]
Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63790/diff/2-3/

Comment 51

[Saad Quadri [:saadq]]

Comment on attachment 8770324 [details]
Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63792/diff/2-3/

Comment 52

[Saad Quadri [:saadq]]

https://reviewboard.mozilla.org/r/63792/#review62402

> So my understanding when we discussed removing this on IRC was that it wasn't adding value over the test specifically for disabledHosts but in this case it actually is testing that the data is preserved across the various schema versions so I think we should keep it. Isn't the simple solution to change the line to get all the disabled hosts from the permission manager then pass them to assertDisabledHostsEqual?

I changed it to use the disabled hosts in the permission manager instead, which required me to use `setLoginSavingEnabled` in one place and to remove that entry in another. I used the disabledHost APIs on `Services.logins` instead of using the permission manager directly in order to minimize repetition, such as the code for the iteration when getting all the disabled hosts from the permission manager. Do you want me to keep it like this or do you just want me to still use `Services.perms` directly and maybe add a function to the top of the file like `getAllDisabledHostsFromPermissionManager` ?

Comment 53

[Saad Quadri [:saadq]]

Comment on attachment 8772616 [details]
Bug 1058438 - Remove disabledHost APIs from storage and use permission manager directly in LoginManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65372/diff/2-3/

Comment 54

[Saad Quadri [:saadq]]

Comment on attachment 8770322 [details]
Bug 1058438 - Migrate disabledHosts from json storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63788/diff/3-4/

Comment 55

[Saad Quadri [:saadq]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63774/diff/5-6/

Comment 56

[Saad Quadri [:saadq]]

Comment on attachment 8770323 [details]
Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63790/diff/3-4/

Comment 57

[Saad Quadri [:saadq]]

Comment on attachment 8770324 [details]
Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63792/diff/3-4/

Comment 58

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63792/#review62402

> I changed it to use the disabled hosts in the permission manager instead, which required me to use `setLoginSavingEnabled` in one place and to remove that entry in another. I used the disabledHost APIs on `Services.logins` instead of using the permission manager directly in order to minimize repetition, such as the code for the iteration when getting all the disabled hosts from the permission manager. Do you want me to keep it like this or do you just want me to still use `Services.perms` directly and maybe add a function to the top of the file like `getAllDisabledHostsFromPermissionManager` ?

I was initially going to suggest using Services.logins but I think that test may be trying to avoid initializing nsLoginManager as it may interfere with the test. I'm not sure of this though.

I think a helper like `getAllDisabledHostsFromPermissionManager` avoids the issue though and ensures that the actual data is migrated properly.

Comment 59

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/65372/#review62706

::: toolkit/components/passwordmgr/nsLoginManager.js
(Diff revisions 1 - 3)
>      }
>  
>      if (count)
>        count.value = disabledHosts.length; // needed for XPCOM
>  
> -    log.debug("_getAllDisabledHosts: returning", disabledHosts.length, "disabled hosts.");

I'm not sure why you got rid of this now? The only problem was the underscore at the beginning of the log message.

Comment 60

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63788/#review62708

Looks good :)

Comment 61

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63788/#review62710

Comment 62

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/63788/#review62712

Comment 63

[Saad Quadri [:saadq]]

https://reviewboard.mozilla.org/r/65372/#review62706

> I'm not sure why you got rid of this now? The only problem was the underscore at the beginning of the log message.

Oh, not sure what happened there, I'll get that fixed.

Comment 64

[Saad Quadri [:saadq]]

Comment on attachment 8772616 [details]
Bug 1058438 - Remove disabledHost APIs from storage and use permission manager directly in LoginManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65372/diff/3-4/

Comment 65

[Saad Quadri [:saadq]]

Comment on attachment 8770322 [details]
Bug 1058438 - Migrate disabledHosts from json storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63788/diff/4-5/

Comment 66

[Saad Quadri [:saadq]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63774/diff/6-7/

Comment 67

[Saad Quadri [:saadq]]

Comment on attachment 8770323 [details]
Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63790/diff/4-5/

Comment 68

[Saad Quadri [:saadq]]

Comment on attachment 8770324 [details]
Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63792/diff/4-5/

Comment 69

[Matthew N. [:MattN]]

Looks good! Can you do a try push and then set checkin-needed?

Comment 70

[Saad Quadri [:saadq]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eafd8dda437&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/13df5d72d373
Remove disabledHost APIs from storage and use permission manager directly in LoginManager. r=MattN
https://hg.mozilla.org/integration/fx-team/rev/d15abe37a931
Migrate disabledHosts from json storage to permission manager. r=MattN
https://hg.mozilla.org/integration/fx-team/rev/f0ef54ffcec1
Migrate disabledHosts from sqlite storage to permission manager. r=MattN
https://hg.mozilla.org/integration/fx-team/rev/5d761723a899
Add additional tests for disabledHost APIs with nonascii characters in URLs. r=MattN
https://hg.mozilla.org/integration/fx-team/rev/34e32a1c5cef
Add test for disabledHost migration to sqlite v6 in Fennec. r=MattN

Comment 72

[Carsten Book [:Tomcat]]

backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10733656&repo=fx-team

Comment 73

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b3a92e3d6ae3
Backed out changeset 34e32a1c5cef 
https://hg.mozilla.org/integration/fx-team/rev/cdca902ad554
Backed out changeset 5d761723a899 
https://hg.mozilla.org/integration/fx-team/rev/1c60e002f023
Backed out changeset f0ef54ffcec1 
https://hg.mozilla.org/integration/fx-team/rev/b93bc99d4c6e
Backed out changeset d15abe37a931 
https://hg.mozilla.org/integration/fx-team/rev/1bb05a19ff9d
Backed out changeset 13df5d72d373 for rc2 failures

Comment 74

[Matthew N. [:MattN]]

(In reply to Carsten Book [:Tomcat] from comment #72)
> backed out for failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=10733656&repo=fx-team

I guess that's referring to this line: https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testPasswordProvider.java#55

It's probably worth sanity-checking that the SQLite backend doesn't have an obvious mistake.

Comment 75

[Matthew N. [:MattN]]

On an unrelated note, I noticed that the migration code doesn't seem to work (at least for the JSON backend) because the migration code is running before the storage backend data is ready which means it always bails at https://hg.mozilla.org/mozilla-central/annotate/d15abe37a931/toolkit/components/passwordmgr/storage-json.js#l94

I don't think we want to force the data backend initialization with ensureDataReady so I think we'll need a different solution.

Comment 76

[Saad Quadri [:saadq]]

Ah yeah, I missed that since I didn't have an explicit test for the migration (for the JSON backend anyway). And I'll try to see if there's something glaringly wrong with the SQL side.

Comment 77

[Saad Quadri [:saadq]]

Comment on attachment 8772616 [details]
Bug 1058438 - Remove disabledHost APIs from storage and use permission manager directly in LoginManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65372/diff/4-5/

Comment 78

[Saad Quadri [:saadq]]

Comment on attachment 8770322 [details]
Bug 1058438 - Migrate disabledHosts from json storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63788/diff/5-6/

Comment 79

[Saad Quadri [:saadq]]

Comment on attachment 8770315 [details]
Bug 1058438 - Migrate disabledHosts from sqlite storage to permission manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63774/diff/7-8/

Comment 80

[Saad Quadri [:saadq]]

Comment on attachment 8770323 [details]
Bug 1058438 - Add additional tests for disabledHost APIs with nonascii characters in URLs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63790/diff/5-6/

Comment 81

[Saad Quadri [:saadq]]

Comment on attachment 8770324 [details]
Bug 1058438 - Add test for disabledHost migration to sqlite v6 in Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63792/diff/5-6/

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04e73415695b
Remove disabledHost APIs from storage and use permission manager directly in LoginManager. r=MattN
https://hg.mozilla.org/integration/autoland/rev/86825a2016c3
Migrate disabledHosts from json storage to permission manager. r=MattN
https://hg.mozilla.org/integration/autoland/rev/c99c19b2588a
Migrate disabledHosts from sqlite storage to permission manager. r=MattN
https://hg.mozilla.org/integration/autoland/rev/983a207d2df9
Add additional tests for disabledHost APIs with nonascii characters in URLs. r=MattN
https://hg.mozilla.org/integration/autoland/rev/da85a2ad0e70
Add test for disabledHost migration to sqlite v6 in Fennec. r=MattN

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/04e73415695b
https://hg.mozilla.org/mozilla-central/rev/86825a2016c3
https://hg.mozilla.org/mozilla-central/rev/c99c19b2588a
https://hg.mozilla.org/mozilla-central/rev/983a207d2df9
https://hg.mozilla.org/mozilla-central/rev/da85a2ad0e70