1156752 - Too many fields (eg, profile information) stored in the login manager.

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

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

Attachment: 0001-explicit-secure-and-plain.patch

I think we are storing too much in the login manager.  Suspect things I see in mine include:

> {"customizeSync":false, "profile":{"email":"[e-mail]","uid":"3f57548965694918ad6d8c6493182ff5","avatar":null}}

The problem is we erred on the side of caution by having a "whitelist" of fields safe to store in plain. I think we need 2 lists, and throw when a new field is introduced forcing the introduction of that field to make an explicit decision. Something like the attachment.

Comment 1

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

Attachment: 0002-Bug-1156752-explicitly-list-where-each-FxA-field-is-.patch

This patch builds on the patch in bug 1156752.

There are now 3 categories of fields - those stored securely, those stored in plain-text, and those stored in memory (ie, not persisted anywhere). If a field is added that's not in any of these sets, we throw (but as per a comment in the patch, we should probably just ignore and warn)

This also moves the certificate and keypair functions from the accountState - they are only there because that's a convenient and safe place to store the (transient) certificate and keypair. These 2 fields are now flagged as being in the "memory" set - so they can be added and retried from storage as normal, but never actually get written anywhere.

This gets us much closer to having the accountState object be light-weight and not have a reference back to the FxA object - I haven't moved the profile code yet as bug 1171253 already moves the profile code back to the FxA object, and that seems likely to land before this.

Comment 2

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

Handling this in other bugs.

Comment 3

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

(In reply to Mark Hammond [:markh] from comment #2)
> Handling this in other bugs.

Apparently I was confused :/

Comment 4

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

Attachment: 0003-Bug-1156752-explicitly-list-where-each-FxA-field-is-.patch

This patch is in reasonably good shape. About the only outstanding thing is that there are a couple of fields that trigger the warning about an unknown field - customizeSync and sessionTokenContext. I think the former implies a different bug (about:accounts tries to remove that IIUC) and the latter doesn't seem to be used by the client, so theoretically could be explicitly flagged as a "memory" field, but it's also possible we will need it in the future (which would imply it should be flagged as a "json" field)

Comment 5

[Chris Karlof [:ckarlof]]

(In reply to Mark Hammond [:markh] from comment #4)
> Created attachment 8629223 [details] [diff] [review]
> 0003-Bug-1156752-explicitly-list-where-each-FxA-field-is-.patch
> 
> This patch is in reasonably good shape. About the only outstanding thing is
> that there are a couple of fields that trigger the warning about an unknown
> field - customizeSync and sessionTokenContext. I think the former implies a
> different bug (about:accounts tries to remove that IIUC) and the latter
> doesn't seem to be used by the client, so theoretically could be explicitly
> flagged as a "memory" field, but it's also possible we will need it in the
> future (which would imply it should be flagged as a "json" field)

I don't think we persist either of those. sessionTokenContext doesn't need to be sent at all and the customizeSync state is written to a pref in the about:accounts code.

Comment 6

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

Attachment: 0002-Bug-1156752-explicitly-list-where-each-FxA-field-is-.patch

(In reply to Chris Karlof [:ckarlof] from comment #5)
> > About the only outstanding thing is
> > that there are a couple of fields that trigger the warning about an unknown
> > field - customizeSync and sessionTokenContext.

> I don't think we persist either of those. sessionTokenContext doesn't need
> to be sent at all

We could look into having the server not send this field, but for now I just explicitly listed it as a "memory" field.

> and the customizeSync state is written to a pref in the
> about:accounts code.

Yeah, but about:accounts only removed that field if it was true - I guess there was previously an assumption this field would not exist rather than explicitly being set to false. I changed about:accounts to unconditionally remove this field so there's no need to list this field anywhere.

Here's a new version that I think is ready to roll. Chris, feel free to pass it to Shane if you think that makes sense.

Comment 7

[Chris Karlof [:ckarlof]]

Delegating to zaach.

Comment 8

[Zachary Carter [:zaach]]

Comment on attachment 8633098 [details] [diff] [review]
0002-Bug-1156752-explicitly-list-where-each-FxA-field-is-.patch

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

Half of this patch seemed like an unrelated refactor that could be split into it's own patch (for easier scanning :). It looks good though; only one concern really WRT sessionTokenContext.

::: services/fxaccounts/FxAccountsCommon.js
@@ +224,5 @@
> +  ["kA", "kB", "keyFetchToken", "unwrapBKey", "assertion"]);
> +
> +// Fields we keep in memory and don't persist anywhere.
> +exports.FXA_PWDMGR_MEMORY_FIELDS = new Set(
> +  ["cert", "keyPair", "sessionTokenContext"]);

We could safely exclude "sessionTokenContext", which seems to be mistakenly included in the data sent from the content-server. The value is only used on the content-server, and it's cached in localStorage there.

::: services/fxaccounts/FxAccountsStorage.jsm
@@ +206,5 @@
>      this._write();
>    }),
>  
>    _clearCachedData() {
> +    this.cachedMemory = {}

Missing semicolon.

Comment 9

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

Attachment: 0002-make-memory-fields-be-explicitly-requested-to-be-ret.patch

A problem with this patch is that it means these in-memory fields are now returned to all callers of getUserAccountData(). This wouldn't typically be a problem apart from the fact that a keyPair fails when JSON.stringify() is called on it - and many tests (and possibly even our own logging) will attempt to do just this. One test is failing due to this.

Chris and I discussed that in the medium-term, we'd live to move away from getUserAccountData() just blindly returning all data to all callers, and to make the callers explicitly nominate the fields they care about - see https://id.etherpad.mozilla.org/fxa-refactor.

So I felt a compromise was to start allowing the explicit fieldnames to be specified now and only return the in-memory fields when those fields are explicitly requested. Thus, a simple .getUserAccountData() will not return the keyPair, but .getUserAccountData("keyPair") will return *just* the keyPair (and .getUserAccountData(["keyPair", "cert"]) would return 2 fields. This means that all existing callers of getUserAccountData() don't see these new in-memory fields, but the FxAccounts code that needs them can still get at them.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6448c58b8693

(Note that for ease of reviewing, this patch is on-top of the previous patch you r+'d, but I'll roll them together for landing)

Comment 10

[Zachary Carter [:zaach]]

Comment on attachment 8636964 [details] [diff] [review]
0002-make-memory-fields-be-explicitly-requested-to-be-ret.patch

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

Good compromise for the time being.

::: services/fxaccounts/FxAccountsStorage.jsm
@@ +131,5 @@
>    // Get the account data by combining the plain and secure storage.
> +  // If fieldNames is specified, it may be a string or an array of strings,
> +  // and only those fields are returned. If not specified the entire account
> +  // data is returned except for "in memory" fields. Note that not specifying
> +  // field names will soon be deprecated/removed - we want all callers to

Perhaps we should go ahead and log a warning now.

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17591829e038

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2873123a1a32

Comment 13

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

(In reply to Zachary Carter [:zaach] from comment #10)
> ::: services/fxaccounts/FxAccountsStorage.jsm
> @@ +131,5 @@
> >    // Get the account data by combining the plain and secure storage.
> > +  // If fieldNames is specified, it may be a string or an array of strings,
> > +  // and only those fields are returned. If not specified the entire account
> > +  // data is returned except for "in memory" fields. Note that not specifying
> > +  // field names will soon be deprecated/removed - we want all callers to
> 
> Perhaps we should go ahead and log a warning now.

I landed without the warning - a warning doesn't seem worthwhile until we've at least changed all callers in services/fxaccounts to use the preferred technique - it will just create log noise for no benefit.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2873123a1a32