967047 - Excluding FxA credentials from password sync

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

The passwords engine excludes Sync 1.1's credentials from the uploaded data. You should make sure to do the same with FxA credentials, unless you want other clients to sync them back down and apply them…

Comment 1

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

Attachment: 0004-Bug-967047-give-FxA-credentials-the-same-special-tre.patch

We have already started syncing the credentials for the FxA hosts, so I kept the existing logic for the one-time deletion of already synced credentials, but added the FxA hosts to this (and also tweaked it so no "delete" request was made if none were found.)  I also "renamed" the pref so it will be hit once for everyone.

The code now skips these hosts too when syncing passwords.

This also got me looking at all the uses of PWDMGR_HOST, and I found deleteSyncCredentials in identity.js, and figured this should also delete the FxA hosts.  Seeing multiple modules use the list of hosts, I added a helper Utils.getSyncCredentialsHosts() - this returns a set of all hosts that should be skipped/deleted.  It still includes PWDMGR_HOST as I see no harm in always including that.

There were other uses of PWDMGR_HOST in identity.js, but I believe none of these are relevant in an Fxa world - eg, persistCredentials, _getLogins and _setLogin should either never be called or never hit the conditions that causes it to be used.  In this spirit, I've also removed a comment from browserid_identity that says "Keep in mind that persistCredentials() will need to be called to flush the changes to disk." as this is not true.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8372074 [details] [diff] [review]
0004-Bug-967047-give-FxA-credentials-the-same-special-tre.patch

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

::: services/sync/modules/engines/passwords.js
@@ +45,5 @@
> +        let allIds = [];
> +        for (let host of Utils.getSyncCredentialsHosts()) {
> +          let ids = Services.logins.findLogins({}, host, "", "")
> +                            .map(function(info) {
> +            return info.QueryInterface(Components.interfaces.nsILoginMetaInfo).guid;

Could just do 

  for (let id of (Services.logins.findLogins(…))) {
    allIds.push(info.QI…);
  }

and avoid the concat and mapping into a new array.

@@ +53,5 @@
> +        if (allIds.length) {
> +          let coll = new Collection(this.engineURL, null, this.service);
> +          coll.ids = allIds;
> +          let ret = coll.delete();
> +          this._log.debug("Delete result: " + ret);

Maybe for a little extra thoroughness here, because FxA will increase the rate of 401s: only write the success pref here on a 400 or 2xx. If we get a 401 or a 503, let's try again later.

@@ +57,5 @@
> +          this._log.debug("Delete result: " + ret);
> +        } else {
> +          this._log.debug("Didn't find any passwords to delete");
> +        }
> +        Svc.Prefs.set("deletePwdFxA", true);

Also delete the "deletePwd" pref?

@@ +295,5 @@
>        case "addLogin":
>        case "removeLogin":
>          // Skip over Weave password/passphrase changes.
>          subject.QueryInterface(Ci.nsILoginMetaInfo).QueryInterface(Ci.nsILoginInfo);
> +        if (Utils.getSyncCredentialsHosts().has(subject.hostname)) {

Define Utils.isSyncCredentialsHost()?

Or, even, Utils.isSyncCredential(subject)?

Comment 3

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

Attachment: 0001-Bug-967047-give-FxA-credentials-the-same-special-tre.patch

(In reply to Richard Newman [:rnewman] from comment #2)

Thanks! Addressed all comments, except:

> Define Utils.isSyncCredentialsHost()?
> 
> Or, even, Utils.isSyncCredential(subject)?

I didn't think this was necessary given we already return a set - so a new Utils method just to wrap a .has() call doesn't seem worthwhile to me.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8373066 [details] [diff] [review]
0001-Bug-967047-give-FxA-credentials-the-same-special-tre.patch

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

::: services/sync/modules/engines/passwords.js
@@ +54,5 @@
> +          coll.ids = ids;
> +          let ret = coll.delete();
> +          this._log.debug("Delete result: " + ret);
> +          // Anything other than success or 400 means we'll try again later.
> +          clearPref = ret.success || ret.status == 400;

if (ids.length) {
  ...
  if (!ret.success && ret.status != 400) {
    // A non-400 failure means try again next time.
    return;
  }
}
// If there were no ids to delete, or we succeeded, or got a 400,
// record success and be done.
Svc.Prefs.set("deletePwdFxA", true);
Svc.Prefs.reset("deletePwd");

Now you don't need clearPref at all; just fall through.

@@ +167,1 @@
>          continue;

Only compute getSyncCredentialsHosts once. Here you're doing three prefs lookups and three URI parses *per login*!

Comment 5

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

https://hg.mozilla.org/integration/fx-team/rev/05d01f97ed0c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05d01f97ed0c

Comment 7

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

Attachment: As landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Sync.Next
User impact if declined: User's Firefox Account password may be synched via Firefox Accounts!
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Low, limited to sync
String or IDL/UUID changes made by this patch: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b2cc1963145