1017433 - Update and upload metadata sentinel as part of migration

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

As part of migrating to FxA sync, we will need to update the client metadata for the current client to reflect the FxA account name, and sync the client engine so it is pushed to the legacy sync servers.  As mentioned in bug 1014406 comment 2, we probably do *not* want to force a full sync of all engines or there will be a higher probability of striking a user for which the full sync never completes.

The complication is that this must be performed *after* and FxA login (so we know what username to write in the client record), but *before* we reset Sync and re-initialize it with the FxA account (so we still have the legacy sync credentials.)

We also need to handle failures here - if there is some transient error performing the client engine sync, we probably want to retry - but at the same time we don't want to prevent the upgrade process from proceeding for a significant amount of time.  However, the failure case here probably isn't too bad - it just means subsequent devices will not know the FxA account name - the user probably does though, and if they enter it manually and log in, the subsequent devices will still be correctly attached to the new account.

Comment 1

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

To flesh this out a little more, we need:

1) A way so the migration process can ask "have we already written (ie, synced) an FxA account name for this device?"

2) Write the specified FxA account name to this client's record (ie, update the record locally and force the local clients engine to sync so it is written)  Before this step, (1) would be false, but after it would be true.

3) Have any other devices had an FxA account name written for them, and what is the FxA account name?  (This may require us to force-sync the clients engine to ensure we are up-to-date)  ie, are there *any* devices for which (1) would be true?

4) Is the migration complete (ie, have all devices, including this, written an FxA account name)?  This may also require us to force-sync the clients engine.  (ie, would *all* devices return true for (1))

Comment 2

[Richard Newman [:rnewman]]

Is there a reason why you want to write a client record, rather than a new meta sentinel a la Bug 956444?

Client records are a pretty bad choice for a migration sentinel:

* They are TTLed, so without exceptional handling they won't help users who span release channels or take more than 3 weeks to switch all their devices.
* They are only stored in memcache, IIRC, so they're not durable.
* Other clients write them in some circumstances (e.g., sending a tab), which will lose your custom fields.

Each of these will cause the sentinel to be lost.

The only reason I see to write a client record is if you specifically care about which clients have migrated, and allow them to migrate to different FxAs.

The approach I was expecting for this:

1. The first migrating client writes the full account credentials to meta/fxa on the old account.
2. Clients can check for meta/fxa and offer seamless upgrade.
3. When a client migrates, it deletes its client record from the old account.

Accounts with zero client records are fully migrated, or haven't been touched by any client at all within 3 weeks (and we can distinguish just that state if we're lazy about pruning TTLed records).

Comment 3

[Ryan Kelly [:rfkelly]]

> Client records are a pretty bad choice for a migration sentinel:
> * They are only stored in memcache, IIRC, so they're not durable.

The others are all good reasons, but FYI this one is inaccurate - they are write-through cached, so we use memcache for fast lookup but still put them in db for proper persistence.

Comment 4

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

(In reply to Richard Newman [:rnewman] from comment #2)
> Is there a reason why you want to write a client record, rather than a new
> meta sentinel a la Bug 956444?

Nope, just my lack of understanding!

Comment 5

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

On reflection, I think p=8 is more appropriate as this will require taming the sync xpcshell test infrastructure.

Also, for completion:

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

> 3. When a client migrates, it deletes its client record from the old account.

IIUC, this will be handled automatically due to the fact we will need to call .startOver() as part of the migration.

Comment 6

[Nick Alexander :nalexander [he/him]]

In the Sync migration meeting, we (nalexander, mhammond, rfkelly, ckarlof) discussed writing a JSON blob into /meta/global to act as sentinel.  Said JSON blob would contain simply

{"email":email, "uid":uid, "verified":verified}

with email the utf8 string of an existing Firefox Account, uid the corresponding utf8 string, and verified a boolean.  (uid and verified just make things simpler on Android but are not strictly required.  uid is really nice to have, though.)  Both auth and token server URIs would default to the production service endpoints.

A second device encountering such a sentinel will require the user to enter their password to connect.

In Bug 956444, rnewman implemented a sentinel of this type but written into the meta namespace away from /meta/global (for example, /meta/credentials).  It's my understanding that everything under /meta is not encrypted.  It seems marginally easier to write to /meta/credentials and a whole lot easier to delete /meta/credentials, because it can be written without writing a valid (or even well-formed) /meta/global record.  So in the absence of other opinions, I suggest we write the record format above to /meta/credentials or similar.

rnewman, markh: concur?

Comment 7

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

(In reply to Nick Alexander :nalexander from comment #6)
> rnewman, markh: concur?

SGTM, and seems to agree with what rnewman said in bug 1017433 comment 2.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #6)

> rnewman, markh: concur?

There's no reason why meta/ can't contain encrypted data; it's just "records that aren't browser data, and thus shouldn't be wiped without good reason".

(There are reasons why it might be inconvenient for it to be encrypted, such as "maybe your keys changed and now you can't decrypt this", but that's probably a good thing.)

I strongly recommend not writing into meta/global, because an old dumb client (< 36, at this point) will overwrite whatever you put there.


I see value in uploading the user's full FxA credentials -- whatever the user would have to type in somewhere else -- because it makes migration smoother, and will drive user delight. It's magic!

I gently oppose having cleartext email addresses uploaded to your old Sync server. We take some steps to not do that in any case (that's why the username is a hash), but this obviously would leak a cleartext link from old to new, where the old isn't well-protected.

Given that (a) doing it the cleartext way requires the user to enter their password, and (b) that's pretty close to the failure case for encrypted migration sentinel, I suggest dumping everything into an encrypted record, and leaving no cleartext credentials at all.

If the client can decrypt it (99% of users), great -- one-click migration.

If they can't, then they know they've been migrated, and they can ask the user to just sign in with email+pass. Barely worse than not writing any data at all, no?

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #8)
> (In reply to Nick Alexander :nalexander from comment #6)
> 
> > rnewman, markh: concur?
> 
> There's no reason why meta/ can't contain encrypted data; it's just "records
> that aren't browser data, and thus shouldn't be wiped without good reason".
> 
> (There are reasons why it might be inconvenient for it to be encrypted, such
> as "maybe your keys changed and now you can't decrypt this", but that's
> probably a good thing.)
> 
> I strongly recommend not writing into meta/global, because an old dumb
> client (< 36, at this point) will overwrite whatever you put there.

Ah yes, our old friend.  I'm convinced.
 
> I see value in uploading the user's full FxA credentials -- whatever the
> user would have to type in somewhere else -- because it makes migration
> smoother, and will drive user delight. It's magic!
> 
> I gently oppose having cleartext email addresses uploaded to your old Sync
> server. We take some steps to not do that in any case (that's why the
> username is a hash), but this obviously would leak a cleartext link from old
> to new, where the old isn't well-protected.

Don't you provide user/password to basic auth (over HTTPS) anyway?  Is that user the hash?  I made two assumptions based on my memory:

1) the user for basic auth is bare;
2) new clients have user names that are valid email addresses.

With these two things together, I concluded that the new email will be already known to whomever reads meta/global.  Either or both of these assumptions could be wrong.

> Given that (a) doing it the cleartext way requires the user to enter their
> password, and (b) that's pretty close to the failure case for encrypted
> migration sentinel, I suggest dumping everything into an encrypted record,
> and leaving no cleartext credentials at all.

This is possible but a little awkward because the most natural way to connect an Android device seamlessly includes access tokens that shouldn't be shared across devices.  So the first client needs to write everything needed for a /login call.  On Android, that doesn't correspond to a State -- it's before Engaged.  It is, however, do-able, and we've discussed it before: we'd write what the Android client calls "quickStretchedPW" and some other things to the sentinel.  If we go this route I can spec this.

> If the client can decrypt it (99% of users), great -- one-click migration.
> 
> If they can't, then they know they've been migrated, and they can ask the
> user to just sign in with email+pass. Barely worse than not writing any data
> at all, no?

Not being able to fill email sucks but that's the 1%.

In conclusion: rfeeley has proposed a flow that expects users to sign in on each device.  It's less complicated than the flow you propose.  And it will make sense to every Android user: they'll get a notification asking them to sign in on each device, which is onerous but really puts them in control of their Sync constellation.  We could also give a notification saying they've been upgraded and need to sign in.  And we could make it easier to not connect a device by doing something like we're doing in Bug 1002888.

rfeeley: weigh in.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #9)

> With these two things together, I concluded that the new email will be
> already known to whomever reads meta/global.  Either or both of these
> assumptions could be wrong.

For 1.1 clients, the username in both paths and auth headers is 'normalized' (hashed for an email, not for a non-email).

The email address (the "account" in client parlance) is only used to derive the normalized value (the "username").

So email addresses don't go over the wire unmolested.

New clients indeed have email addresses, but they're not necessarily valid (we don't email you to check; it just needs to have an "@" in there), and they're not necessarily the same as your FxA email, which is what we'd be leaking.

Even if the FxA's email address is the same as the old account, an attacker on the old account will not know it unless they've compromised SHA-1, can guess (there's no salt), or we give it to them in cleartext in this sentinel.

(Don't forget that we need to bundle your FxA server and token server settings in here, too; email address alone isn't enough. We could refuse to write migration sentinels for non-Mozilla servers, but arguably users on custom servers -- lots of typing! -- are the ones who would most benefit from automatic migration.)



> Not being able to fill email sucks but that's the 1%.

The 1% of the 1%, really. We could even pre-fill your old Sync email (which the client on the other end knows), if we do a ping to the server and find that an FxA exists with that email address, or if we write a flag to the sentinel envelope saying "it's the same email!".

 
> In conclusion: rfeeley has proposed a flow that expects users to sign in on
> each device.  It's less complicated than the flow you propose.

It's a little simpler, but does have some costs.

It doesn't require the client to decrypt, at the cost of leaking cleartext FxA email address correlations to the Sync server, and to anyone MITMing the connection (like financial employers).

The client still needs to process this record and do something with it. That's unchanged.

In one plan we throw up a notification that feeds you into the setup process with a pre-filled email, ultimately deleting the sync account at the end.

In the other we do the same thing, but with your password effectively pre-filled.


> sign in on each device, which is onerous but really puts them in control of
> their Sync constellation.

Can you explain what extra control we add with the password sign-in?

Do you mean that we have a requirement to let users opt-out -- "I was syncing these three devices, but now I'd like to only sync these two and disconnect the other" -- or to reject potential 'wiretaps' on the old account?

For opt-out, wouldn't this just be a "would you like to migrate this device to your Firefox Account, or disconnect it?" notification rather than a "would you like to sign in with this Firefox Account, or disconnect?"

Comment 11

[Nick Alexander :nalexander [he/him]]

At the sync migration meeting yesterday:

* rfeeley was strongly in favour of automatic migration (welcome to the party instead of join the party) if it was possible.  Clearing NI.

* ckarlof was mildly against due to the additional time required, engineering complexity, and additional failure modes.  Leaving the NI because he had additional ideas about server-side signalling to clients about what migration routes they should take.

Comment 12

[Nick Alexander :nalexander [he/him]]

markh and I discussed uploading a sentinel with enough data for automatic migration.

tl;dr: it's a good deal of work on Desktop.

Details: Desktop chrome JS never sees the password or even the stretched password.  The contents of the "signedInUser.json" bundle are presented to the chrome JS by in-content (about:accounts) JS.  Those contents are:

{"version":1,"accountData":{"email":"[e-mail]","uid":"XXX","sessionToken":"XXX","verified":true}}

As explained above, sessionToken cannot be shared.  Further, there is no way to leverage a sessionToken to get more credentials.  (By design.)

Now, it could be done.  We could modify the chrome and content JS to exchange the password or sufficiently powerful derivatives of the password.  We could modify the auth server to add ways for connected clients to provision new clients.  Or...  In any case, it's work, co-ordination, and complexity.

Comment 13

[Nick Alexander :nalexander [he/him]]

rnewman: do you have a comment on https://bugzilla.mozilla.org/show_bug.cgi?id=1017433#c12?

Comment 14

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #12)

> Now, it could be done.  We could modify the chrome and content JS to
> exchange the password or sufficiently powerful derivatives of the password. 
> We could modify the auth server to add ways for connected clients to
> provision new clients.  Or...  In any case, it's work, co-ordination, and
> complexity.

I was expecting (on desktop, at least) for the migration bundle to be computed by about:accounts at the point of setup.

That is: it knows everything it needs to configure the browser it's running in (including your password, from which it derives other things), so that's a perfect moment for it to say "oh, and if you have a Sync 1.1 account, encrypt and stick this JSON on that server".

Yes, that'll be an extra message between chrome and content, but perhaps not a difficult one.

(Writing the sentinel at this point is the only place that makes sense: after this you've probably thrown away all the context you need to do a write to the old server, and you're about to set up the new one.)

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #14)
> (In reply to Nick Alexander :nalexander from comment #12)
> 
> > Now, it could be done.  We could modify the chrome and content JS to
> > exchange the password or sufficiently powerful derivatives of the password. 
> > We could modify the auth server to add ways for connected clients to
> > provision new clients.  Or...  In any case, it's work, co-ordination, and
> > complexity.
> 
> I was expecting (on desktop, at least) for the migration bundle to be
> computed by about:accounts at the point of setup.

This is a significant change to the information architecture of the existing Desktop system.  Right now, Desktop clients never see these high value secrets, and they never cross the wire from server to client.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Nick Alexander :nalexander from comment #12)
> markh and I discussed uploading a sentinel with enough data for automatic
> migration.
> 
> tl;dr: it's a good deal of work on Desktop.

Let's not do it. Signing in again is not that hard.

Comment 17

[Chris Karlof [:ckarlof]]

Clearing NI. I think my concerns have been represented here.

Comment 18

[Nick Alexander :nalexander [he/him]]

OK, rnewman and I discussed this at length on Friday.  Here's where we at:

Desktop uploads /meta/fxa_credentials encrypted with the "meta" collection key.

The fxa_credentials record will look like:

{
  auth: "",
  services: {
    sync: "",
  },
  email: "",
  uid: "",
  verified: true, [optional]
  password: "", [optional]
}

Notes:

The record is encrypted so we don't expose the Fx Account email (or password).

We want to include the auth and sync servers up front so that we have future flexibility and possibly can help migrate custom server users in client code.

The auth/services.sync choice matches the format we discussed and ended up using in the fxa-custom-server-addon.  I see no reason to change.

Including the Fx Account uid and (optional) verified allows Firefox for Android to enter directly into an already defined "NeedsPassword" state.

Including the (optional) Fx Account password allows both a future version of Desktop to include the password (thereby helping other clients out), and allows Android to re-upload the sentinel with the password once an Android device sees it.  This would not be a required feature.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #18)
> OK, rnewman and I discussed this at length on Friday.  Here's where we at:
> 
> Desktop uploads /meta/fxa_credentials encrypted with the "meta" collection
> key.

For simplicity, let's do this with the main Sync key bundle.  It's possible for clients to arrange for different "meta" keys as part of the "crypto/keys" bundle.  By using the Sync key bundle, we could have an empty server with just a /meta/fxa_credentials record.  That avoids having to download and/or upload "crypto/keys" at all.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #19)

> For simplicity, let's do this with the main Sync key bundle.

Makes perfect sense to me. (Indeed, it's consistent with my instincts about the role of the meta 'collection'; it's a closer sibling to 'crypto' than 'bookmarks'.)

Comment 21

[Marco Mucci [:MarcoM]]

Hi Mark, can you mark this bug for QE verification.

Comment 22

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

I think we can just verify the entire flow

Comment 23

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

Attachment: 0011-Bug-1017433-allow-for-upload-and-download-of-encrypt.patch

Something like this?

Comment 24

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

(In reply to Nick Alexander :nalexander from comment #18)
> OK, rnewman and I discussed this at length on Friday.  Here's where we at:
> 
> Desktop uploads /meta/fxa_credentials encrypted with the "meta" collection
> key.
> 
> The fxa_credentials record will look like:
> 
> {
>   auth: "",
>   services: {
>     sync: "",
>   },
>   email: "",
>   uid: "",
>   verified: true, [optional]
>   password: "", [optional]
> }

Can I suggest a small change to this?  Something like:
{
  fxa: {...},
  sync: {...},
}

(where inside those top-level objects the layout is similar to what you suggested?  The advantage is that we can potentially decouple internal knowledge.  What I was thinking was a function in Eg, sync:

  setFxAMigrationSentinel(fxaPortion) {
    let sentinel = {fxa: fxaPortion};
    // compute the sync portion
    sentinel.sync = sync_portion;
    // send to to meta/fxa_credentials
  },

  getFxAMigrationSentinel() {
    ... grab the entire sentinel
    return sentinel.fxa;
  },

the intent being that there is a clear distinction between the 2 sets of data, and both FxA and sync itself can be oblivious to the other's data.

Or something like that :)  The idea is that sync doesn't need to know how to look into nor interpret fxa-specific stuff, and the reverse for the sync engine specific stuff.

Comment 25

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Hammond [:markh] from comment #24)
> (In reply to Nick Alexander :nalexander from comment #18)
> > OK, rnewman and I discussed this at length on Friday.  Here's where we at:
> > 
> > Desktop uploads /meta/fxa_credentials encrypted with the "meta" collection
> > key.
> > 
> > The fxa_credentials record will look like:
> > 
> > {
> >   auth: "",
> >   services: {
> >     sync: "",
> >   },
> >   email: "",
> >   uid: "",
> >   verified: true, [optional]
> >   password: "", [optional]
> > }
> 
> Can I suggest a small change to this?  Something like:
> {
>   fxa: {...},
>   sync: {...},
> }
> 
> (where inside those top-level objects the layout is similar to what you
> suggested?  The advantage is that we can potentially decouple internal
> knowledge.  What I was thinking was a function in Eg, sync:
> 
>   setFxAMigrationSentinel(fxaPortion) {
>     let sentinel = {fxa: fxaPortion};
>     // compute the sync portion
>     sentinel.sync = sync_portion;
>     // send to to meta/fxa_credentials
>   },
> 
>   getFxAMigrationSentinel() {
>     ... grab the entire sentinel
>     return sentinel.fxa;
>   },
> 
> the intent being that there is a clear distinction between the 2 sets of
> data, and both FxA and sync itself can be oblivious to the other's data.
> 
> Or something like that :)  The idea is that sync doesn't need to know how to
> look into nor interpret fxa-specific stuff, and the reverse for the sync
> engine specific stuff.

Sure, wfm.  Document the details here as they become clear.

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 8522006 [details] [diff] [review]
0011-Bug-1017433-allow-for-upload-and-download-of-encrypt.patch

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

I think a new iteration is on its way, judging by IRC!

Comment 27

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

Attachment: 0004-Bug-1017433-part-1-Have-the-sync-RecordManager-retur.patch

rnewman and I discussed on IRC that a cleaner approach would be to use the RecordManager as the cache.  However, this means changing the record manager to return CryptoWrappers instead of a WBORecord.  Sadly, this means the CryptoWrapper object needs to be defined before the RecordManager.  All tests pass with this patch.

Comment 28

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

Attachment: 0005-Bug-1017433-part-2-allow-for-upload-and-download-of-.patch

This is part 2 where the work is done.  When a sync completes successfully we fetch (but don't process) meta/fxa_credentials.  This has the side-effect of caching it in the record manager, so when getFxaMigrationSentinel() is called there's no need to hit the server.

Note however that at no point is there an optimization made for "has the meta collection changed" as I don't see how that would actually work here - the record manager doesn't seem to track this (which is what I would expect) so the record is fetched every sync.

get/setFxaMigrationSentinel() both return promises - that's not strictly necessary in this pseudo-blocking sync, but will help migrate to a promise-based sync should we ever get around to that.

Note that get/setFxaMigrationSentinel() also forces a login, as there will be edge cases when this is called before a sync (specifically for the case when "sync never completes" - the migration process will have "blocked" sync from starting so the next startup can complete the migration)

Finally, note that these functions don't define what the sentinel looks like at all - that is all up to the consumers.  The current WIP uses:

// A list of preference names we write to the migration sentinel.  We only
// write ones that have a user-set value.
const FXA_SENTINEL_PREFS = [
  "identity.fxaccounts.auth.uri",
  "services.sync.tokenServerURI",
];

  /* Return an object with the preferences we care about */
  _getSentinelPrefs() {
    let result = {};
    for (let pref of FXA_SENTINEL_PREFS) {
      if (Services.prefs.prefHasUserValue(pref)) {
        result[pref] = Services.prefs.getCharPref(pref);
      }
    }
    return result;
  },

  _setSyncMigrationSentinel: Task.async(function* () {
    yield WeaveService.whenLoaded();
    let signedInUser = yield fxAccounts.getSignedInUser();
    let sentinel = {
      email: signedInUser.email,
      uid: signedInUser.uid,
      verified: signedInUser.verified,
      prefs: this._getSentinelPrefs(),
    };
    yield Weave.Service.setFxaMigrationSentinel(sentinel);
  }),

feedback requested on Nick for this code that's not actually part of this patch, but shows how it will be used by desktop and gives some idea of what Android can expect.

Comment 29

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

Attachment: 0006-Bug-1017433-part-2-allow-for-upload-and-download-of-.patch

It turns out that the CryptoWrapper deletes the ciphertext after decrypting.  This means that the *second* attempt to get the sentinel fails as it has no ciphertext nor the clear-text payload.

I worked around this by putting the decrypted version back into the record manager, and adjusted the test to check for this case.

Comment 30

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

Comment on attachment 8528141 [details] [diff] [review]
0006-Bug-1017433-part-2-allow-for-upload-and-download-of-.patch

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

::: services/sync/modules/service.js
@@ +1336,5 @@
> +    if (this._shouldLogin()) {
> +      this._log.debug("In getFxaMigrationSentinel: should login.");
> +      if (!this.login()) {
> +        this._log.debug("Can't get migration sentinel: login returned false.");
> +        return null;

this should return a promise - I fixed this locally but didn't think it worth uploading a new patch.

Comment 31

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

Attachment: 0004-Bug-1017433-part-2-allow-for-upload-and-download-of-.patch

This now removes the ability for the migration module to function without this patch.

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 8526462 [details] [diff] [review]
0004-Bug-1017433-part-1-Have-the-sync-RecordManager-retur.patch

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

::: services/sync/modules/record.js
@@ +256,5 @@
> +
> +  contains: function RecordMgr_contains(url) {
> +    if ((url.spec || url) in this._records)
> +      return true;
> +    return false;

Urgh this code needs a rewrite!

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 8532309 [details] [diff] [review]
0004-Bug-1017433-part-2-allow-for-upload-and-download-of-.patch

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

Fine by me -- I assume you've manually tested this, too?

::: services/sync/modules/service.js
@@ +1331,5 @@
> +   * data.
> +   *
> +   * Returns a promise that resolves with the sentinel, or null.
> +   */
> +  getFxaMigrationSentinel: function() {

FxA

@@ +1341,5 @@
> +      }
> +    }
> +    try {
> +      let collectionURL = this.storageURL + "meta/fxa_credentials";
> +      let cryptoWrapper = this.recordManager.get(collectionURL);

You should probably check that we have this.identity.syncKeyBundle first, no? I think it's technically possible for it to be missing or changed.

@@ +1352,5 @@
> +      // decrypted version last time we were called.
> +      if (cryptoWrapper.payload.sentinel) {
> +        return Promise.resolve(cryptoWrapper.payload.sentinel);
> +      }
> +      let payload = cryptoWrapper.decrypt(this.identity.syncKeyBundle);

If this fails, it's probably because the key is wrong, right?

@@ +1353,5 @@
> +      if (cryptoWrapper.payload.sentinel) {
> +        return Promise.resolve(cryptoWrapper.payload.sentinel);
> +      }
> +      let payload = cryptoWrapper.decrypt(this.identity.syncKeyBundle);
> +      // After decrypting the ciphertext is lost, so we kust "put" the

kust

@@ +1356,5 @@
> +      let payload = cryptoWrapper.decrypt(this.identity.syncKeyBundle);
> +      // After decrypting the ciphertext is lost, so we kust "put" the
> +      // decrypted version back.
> +      cryptoWrapper.payload = payload;
> +      this.recordManager.set(collectionURL, cryptoWrapper);

I don't think you need to do this set() -- it's the same object you got on line 1345, and you're mutating the object directly.

@@ +1367,5 @@
> +  },
> +
> +  /**
> +   * Set a migration sentinel for the Firefox Accounts migration.
> +   * Accepts  a JSON blob - it is up to callers of this to make sense of the

s/  / /

@@ +1373,5 @@
> +   *
> +   * Returns a promise that resolves with a boolean which indicates if the
> +   * sentinel was successfully written.
> +   */
> +  setFxaMigrationSentinel: function(sentinel) {

FxA

@@ +1396,5 @@
> +        throw response;
> +      }
> +      this.recordManager.set(collectionURL, cryptoWrapper);
> +    } catch (ex) {
> +      this._log.error("Failed to set the migration sentinel: " + ex);

IIRC Log.jsm now allows:

  .error("...", ex);

Comment 34

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

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/961e53b507fe
https://hg.mozilla.org/integration/fx-team/rev/d90fa41d351c

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/961e53b507fe
https://hg.mozilla.org/mozilla-central/rev/d90fa41d351c

Comment 36

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8532309 [details] [diff] [review]
0004-Bug-1017433-part-2-allow-for-upload-and-download-of-.patch

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

As of Bug 956444, Firefox for Android is fetching these sentinels.  Thanks, markh!