Closed Bug 1013064 Opened 10 years ago Closed 10 years ago

Enable password sync with FxA and master password

Categories

(Firefox :: Sync, defect)

Product:
Firefox
Component:
Sync
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla34
Iteration:
34.2
Tracking Flags:
Tracking Status
firefox34 --- verified

People

(Reporter: markh, Assigned: enndeakin)

References

Details

Attachments

(5 files, 15 obsolete files)

0001-Bug-1013064-part-1-prompt-for-master-password-unlock.patch
2.06 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
0002-Bug-1013064-part-2-Store-sensitive-FxA-credentials-i.patch
21.03 KB, patch
ckarlof
: review+
Details | Diff | Splinter Review
0003-Bug-1013064-part-3-only-migrate-data-into-the-loginm.patch
8.20 KB, patch
ckarlof
: review+
Details | Diff | Splinter Review
0004-Bug-1013064-part-4b-browserid_identity-and-sync-chang.patch
12.04 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
0005-Bug-1013064-part-5-stop-disabling-the-password-engin.patch
20.64 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review 
This bug is to decide what strategy to take when master-password is enabled.  It's probably not particularly useful to have that discussion in bug 995268.

This stuff (obviously) isn't my strong point, but IIUC, there are basically 3 options on the table:

1) Store the FxA credentials in the password store instead of on disk.  This should give us parity with legacy-sync - including the somewhat poor existing UX, plus some new poor UX specifically around FxA - eg, we'd need the MP to be unlocked before doing anything FxA-specific.

2) Use local OS encryption facilities for the FxA credentials.  However, on Windows at least, this doesn't seem to offer the facilities users want - eg, any app running as the same user has access to this data.  IIUC, unlike the OSX keychain, we can't configure it such that the user must be forced to enter a passphrase before the data is unlocked (and even if it could, it just leaves us with (1) above)

3) Sync the encrypted versions of the passwords.  This would force the user to use the same MP on all devices, but that might be a reasonable tradeoff.  I'm sure there will be devil in this detail though - eg, what happens if the user has used a different MP on a remote device - an "encrypted" password comes in, but when the user finally enters their MP on this second device, the password can't be decrypted.

Other ideas, thoughts, people who should be CCd for the discussion, etc?
(In reply to Mark Hammond [:markh] from comment #0)
> 3) Sync the encrypted versions of the passwords.  This would force the user
> to use the same MP on all devices, but that might be a reasonable tradeoff. 
> I'm sure there will be devil in this detail though - eg, what happens if the
> user has used a different MP on a remote device - an "encrypted" password
> comes in, but when the user finally enters their MP on this second device,
> the password can't be decrypted.

If we go there, we could go and just force the MP to be identical to the FxA password, or even replace the MP mechanism with the FxA mechanism in some way, i.e. also replacing the current MP-based encryption mechanism with the same FxA-derived-key-based encryption that Sync uses. There would be a number of corner cases to be figured out around password switches and around not both MP and FxA being activated and switching between those states - but I think that could be figured out.
Oh, and the original decision for the current exclusion state was apparently made in bug 970167 which unfortunately is hidden from the public.
(In reply to Mark Hammond [:markh] from comment #0)

> 1) Store the FxA credentials in the password store instead of on disk.  This
> should give us parity with legacy-sync - including the somewhat poor
> existing UX, plus some new poor UX specifically around FxA - eg, we'd need
> the MP to be unlocked before doing anything FxA-specific.

This is probably the easiest thing to do. Store all of the sensitive FxA stuff in password manager. We can figure out if you're logged in by reading JSON, but everything else requires entering your MP.

When you clear private data, you have to re-enter your FxA credentials. Sucks to be you.


> 3) Sync the encrypted versions of the passwords.  This would force the user
> to use the same MP on all devices, but that might be a reasonable tradeoff. 
> I'm sure there will be devil in this detail though - eg, what happens if the
> user has used a different MP on a remote device - an "encrypted" password
> comes in, but when the user finally enters their MP on this second device,
> the password can't be decrypted.

Indeed, this is difficult: you're forcing every device to know your MP, and you need to prevent any device from changing the MP without reuploading every password and forcing other clients to change their MP. There's a lot of UX around that, lots of ways to go wrong (when I connect a device with MP already turned on, with a different password, what happens?), and it ties Sync to the specific implementation details of the login manager.

(For example, what do we do for the even smaller subset of users who have a hardware smartcard? Or the users who have a Keychain-backed login manager, like <https://addons.mozilla.org/en-us/firefox/addon/keychain-services-integration/>?)

 
> Other ideas, thoughts, people who should be CCd for the discussion, etc?

I think there are two problems here:

* Who's building SITB? That seems like the obvious long game here: your Firefox Account is what you use to sign in to a profile, not normal in-profile data. All of these suggestions about "just encrypt stuff with your FxA password!" basically boil down to this.

* In the short term, how do we make unhappy people happy?


The latter can be solved by a whole spectrum, ranging from "just let them turn password sync on, warning appropriately", to one of the alternative schemes you outline. But we should be thinking about what the proper fix is.
Created Bug 1013448 to help us be more evidence driven in this and future decisions regarding master password.
> In the short term, how do we make unhappy people happy?

IMO, the short term fix is to do the minimal work that allows MP users to safely sync their passwords with FxA Sync. The lack of password syncing when MP is enabled motivated Bug 995268, and that seems to be the loudest concern echoed in the comments. 

Unfortunately, the correct thing to do here is that if the user has MP and Sync enabled, then we must protect the user's FxA creds with the MP as well. Otherwise the attacker could just download and decrypt the user's password from the server using the stored FxA creds. An alternative solution is "login to profile", but that seems like a longer term solution to me.

Storing the FxA Sync creds in the password manager has additional consequences over old Sync because we anticipate adding non-Sync services to the browser in the future. E.g., "Why do I need to enter my MP to use Fx Messaging?" and "Why when I clear my saved passwords do I get logged out of Fx Messaging?"

Since old Sync+MP users (at least those in Bug 995268) seem to accept the UX consequences of the "store the Sync creds in the password manager", it would be nice to limit the UX consequences of a similar approach in FxA Sync solely to MP users. This may mean detecting if MP is enabled and if it is, we save the FxA creds in the password store, and if it's not, we continue to store them on disk.

Until we have "login to profile", I think an orthogonal worthwhile improvement is to look into storing the FxA creds in whatever local crypto store is available (e.g., Keychain) instead of the storing then plainly on local disk (for non-MP users).
Whiteboard: [qa+]
(In reply to Richard Newman [:rnewman] from comment #3)
> When you clear private data, you have to re-enter your FxA credentials.

That's actually an interesting case.
1) Do we have evidence that this caused unexpected problems for Sync 1.1 users?
2) Why would one expect to stay logged into FxA and have the credentials still saved when wiping all private data? (What is the use case or are the use cases for clearing private data? Why should the FxA credentials not be cleared in those use cases?)
3) Should we have a separate entry in the dialogs for clearing private data so that website passwords and service/account/internal passwords like FxA can be treated differently?

> This is probably the easiest thing to do. Store all of the sensitive FxA
> stuff in password manager. We can figure out if you're logged in by reading
> JSON, but everything else requires entering your MP.

I think if I would use MP, I would expect that (similar with the Fx Messaging example brought forward by Chris) - though explaining it in the UI that asks for the MP might be helpful.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #6)

> 1) Do we have evidence that this caused unexpected problems for Sync 1.1
> users?

Yeah, we've had a steady low-frequency stream of confusion and complaints from users over the past four years. From the user's perspective it basically comes down to "this sucks, Sync won't stay signed in; every time I quit, I have to set it up again. something is broken.".

See Bug 553400 and all of its dupes. (And the dependencies are interesting.)


> 2) Why would one expect to stay logged into FxA and have the credentials
> still saved when wiping all private data?

Some users seem to: they don't think of their special Firefox credentials as being in the same bucket as data that Firefox saves on their behalf as they browse the web. They want to clear their browsing history (including passwords that Firefox has saved), not get signed out.

Evidence suggests that users don't understand Private Browsing -- that is, they don't decide *in advance* to not record data. Easier "Clear History" is pretty much our #1 request on Android.

(IOW: this is a leaky abstraction. The user doesn't know or even expect that Sync 1.1 credentials are basically website passwords, and the leak hits them in the face.)


> use cases for clearing private data? Why should the FxA credentials not be
> cleared in those use cases?)

... and other users *do* expect that they should be cleared.

We sometimes see users who, to paraphrase, think that Clear Private Data is equivalent to creating a new profile (obviously they don't think of it in those terms) -- that it's the same thing as a "factory reset" on their cellphone.

That is, before you sell your laptop to a friend, delete all of your saved documents and clear your Firefox profile. All done!


There might well not be a single answer here.
(In reply to Chris Karlof [:ckarlof] from comment #5)

> IMO, the short term fix is to do the minimal work that allows MP users to
> safely sync their passwords with FxA Sync. The lack of password syncing when
> MP is enabled motivated Bug 995268, and that seems to be the loudest concern
> echoed in the comments. 

Right - but the keyword there is "safely", and ISTM this depends on your viewpoint.

> Until we have "login to profile", I think an orthogonal worthwhile
> improvement is to look into storing the FxA creds in whatever local crypto
> store is available (e.g., Keychain) instead of the storing then plainly on
> local disk (for non-MP users).

My investigations for Windows implies there's no reasonable way to protect this from other applications running under the same user account.  So it will protect against "local file read" vulnerabilities, but that isn't going to keep everyone in bug 995268 happy.

I wonder if it is worth enumerating the kinds of attacks we consider "in-scope" here?  Eg, I'm not particularly sympathetic to an argument of "malware on my machine could get my passwords from the windows secure store" when the same malware could grab the encrypted versions of the passwords and crack it relatively easily (or install keyloggers, or ...) - but my sympathy, or lack thereof, isn't particularly relevant :)
As a matter of fact, a malware could even ask firefox to give it out decrypted passwords after the master password was asked.
The comments above seem to be agreeing that the long-game fix isn't going to be quick, but a short-term fix might be.  Given this is emotional to some users, I propose we focus on this short-term fix (with an aim to uplift) and address that long-game in another bug/forum.

Richard and I agree (IIUC) that the storing the signin data as a combination of json+nsILoginManager is the quickest fix and brings us to parity with the old sync.  It means an additional UX-hit, but as long as we don't attempt to fix *existing* UX issues around master-password+sync, that should be OK.

Off the top of my head and expanding on Richard's comment 3, this would mean:

* Have FxA store only the email address (and maybe verified state etc) as plain-text JSON and the rest in the login manager.  If the MP is locked, the data returned would only have the plain-text info plus a marker (eg, .isLocked) to reflect that state.

* The core sync code (mainly the FxA identity code) would just "do the existing thing" if MP is locked - ie, in most cases just silently fail to sync.  Sync already handles MP being locked, so as long as the FxA code ensures Sync is in that state, things should "just work".

(I've actually got an experiment that does some of the above and it does work - ie, really bad UX when locked, sync starts working magically when unlocked ;)

* For signin/signup etc flows, we need to "encourage" an unlocked password.  We could add a new iframe to about:accounts which *all* MP-enabled users see at the start of the signin/signup process explaining "the issue".  If the MP is currently locked it would also offer a button which unlocks via the normal mechanisms.  This means that even users who are MP-enabled but unlocked still get shown information about the limitations - each and every time they login - but that should be rare.

* Mop-up around the edge cases (eg, "user refuses to unlock when signing in", "user clears all credentials on shutdown") ensuring we always enter the existing "needs re-login" state as required, even if that's alot.

Does that sound reasonable and doable in this mythical "short-term"?
(In reply to Richard Newman [:rnewman] from comment #7)
> There might well not be a single answer here.

Sounds like it. And it may really make sense to split password cleaning into two buckets, one for websites and one for FxA (or internal service in general, not sure if there would be any other). Also, thinking about that sounds mid- to long-term, for the short-term solution we probably will have to swallow what's there.
Flags: sec-review?(dveditz)
Summary: Decide on strategy to enable password sync with FxA and master password → Breakdown - Enable password sync with FxA and master password
Whiteboard: [qa+] → p=0 [qa+]
Whiteboard: p=0 [qa+] → p=0 [qa-]
I'm uploading 4 attachments which constitutes a "proof of concept" for this.  This first patch though is uninteresting and not related specifically to this effort - it just cleans up about:accounts to make the show() and hide() methods sane - so feel free to ignore this for now.
This patch creates a new "section" in about:accounts, specific to users with a master-password set.  It uses place-holder text and styling.  Whenever about:accounts would show the remote iframe (eg, for signin/signup), this new master-password section is shown first.  It has a button which allows the user to unlock their MP (only shown if not already unlocked) and a button so the user can say "ok" and see the actual login/signin page.

As mentioned above, this is shown *every time* about:accounts is loaded and even when the user has already unlocked the MP.
This is the meat of the patch.  It stores a couple of fields in the plain-text JSON file, and most fields in the login manager.  Note that before it attempts to use the login manager it always checks it is unlocked - so this code should never cause a prompt to unlock the MP - it will just return a subset of the data in this case.  This has some implications for Sync - the legacy sync *will* cause an unsolicited MP-unlock prompt in some cases - see "part 4" for my work-around for this.

It hacks the existing JSONStorage class, which will either need to be renamed, or these changes broken out into a (say) LoginManagerStorage object.

browserid_identity is also changed to handle the fact a subset of data may only be available, and causes sync to enter either the "needs reauth" or "needs MP unlocked" states.
This patch will prompt for the MP to be unlocked when the user explicitly says "Sync Now".  However, the implication of this is that unless this is selected, Sync will just silently and continuously fail to Sync - there is no UI exposed to tell the user they aren't syncing due to the MP being unlocked.  However, we might decide this is actually OK given the small number of users we expect to be impacted, but if it's not OK, we will need to invent some way of reflecting this state or causing an automatic unlock as necessary.
Justin, I'm hoping for some feedback on how I'm using nsILoginManager in these patches.  An example:

    let login = new loginInfo("chrome://fxa",
                              null, // aFormSubmitURL,
                              "FxA", // aHttpRealm,
                              contents.accountData.email, // aUsername
                              JSON.stringify(contents.accountData), // aPassword
                              "", // aUsernameField
                              "");// aPasswordField

So what I'm doing is (a) using a dummy URL (chrome://fxa), a dummy realm, and storing a JSON blob as the password.  A quick look over the implementation of the login manager doesn't show any other obvious techniques I could use.  Do you have any thoughts on what the correct way to do this is?  Obviously, feel free to pass the needinfo? on to anyone else you feel appropriate.
Flags: needinfo?(dolske)
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Whiteboard: p=0 [qa-] → p=5 s=it-32c-31a-30b.3 [qa-]
Neil, one thing I haven't mentioned is that, assuming nsILoginManager is used, we will want to tell Sync to *not* sync these credentials.  In theory this will be as simple as adding whatever host we use to getSyncCredentialsHosts() - see mxr/dxr for where and how this is referenced.
If we start putting the FxA credentials in the password manager, it would be nice to have some treatment for when the user does something (other than the normal logout flow) that would cause them to be deleted and logged out (e.g., clear all data). For example, we could display a warning that this action will cause the user to be logged out of the browser. 

I also think it would be a good idea to add some text explaining how the MP+Sync combination works that we can show to users that are using Sync in combination with Master Password.
I don't think I can do this.
Assignee: enndeakin → nobody
Removed from Iteration 32.3
Status: ASSIGNED → NEW
Whiteboard: p=5 s=it-32c-31a-30b.3 [qa-] → p=5 [qa-]
(In reply to Mark Hammond [:markh] from comment #16)

> So what I'm doing is (a) using a dummy URL (chrome://fxa), a dummy realm,
> and storing a JSON blob as the password.  A quick look over the
> implementation of the login manager doesn't show any other obvious
> techniques I could use.  Do you have any thoughts on what the correct way to
> do this is?

That seems fine, there isn't really a better way for storing arbitrary data along with a login. This does mean that if someone views their password in the management UI they might be confused why it's not their actual password. Oh, and as of bug 998081 we show the "last changed" field in the UI, so if you're frequently updating some field in your JSON "password" that could look odd. But none of this seems like a huge deal.

What all is in the contents.accountData "password"? Are there non-sensitive parts that could just be stored separately?
Flags: needinfo?(dolske)
Thanks Dolske!

(In reply to Justin Dolske [:Dolske] from comment #21)

> What all is in the contents.accountData "password"? Are there non-sensitive
> parts that could just be stored separately?

That's a good question - the code is setup such that the choice of fields can easily be specified, but I'm not sure which of them are truly sensitive.  Chris, can you help here?  Precisely which of the fields currently stored in signedInUser.json should be moved, and which can remain?
Flags: needinfo?(ckarlof)
> Chris, can you help here?  Precisely which of the fields currently stored in signedInUser.json should be moved, and which can remain?

This is a little complicated. The user can be verified or unverified.

If she is unverified, we have three secrets in signedInUser.json:

1) unwrapKb: your key encryption key (note: this should be deleted after we calculate kB. currently, it's not)
2) keyFetchToken: credential which lets you fetch the server portion of the key material (useful after you verify your email)
3) sessionToken: session token to talking to the authenticated portion of the FxA API

If she is verified, we have three secrets in signedInUser.json:
1) kB: your kB encryption key
2) kA: your kA encryption key (currently not used)
3) sessionToken: session token to talking to the authenticated portion of the FxA API

Other data, e.g., the user's email and uid, I don't consider sufficiently sensitive to put in the password manager. 

It's worth discussing whether put the session token in the password manager or not. If we do, master password will prevent the user from using other FxA attached services until she enters her MP (because the system will likely need the sessionToken to do so). Other attached services will also have auth credentials (e.g., Oauth tokens), and we should consider our policy for storing those.
Flags: needinfo?(ckarlof)
(In reply to Chris Karlof [:ckarlof] from comment #23)
> This is a little complicated. The user can be verified or unverified.

Thanks.  I just had a chat with Chris, and he believes that storing the sessionToken in the .json file isn't an issue for this specific case - the sessionToken *would* allow you to pull down the encrypted sync data, but without kB you can't decrypt it.  So in effect this gives you data which has better encryption than if you have the encrypted loginmanager data (which such an attacker would also have access to)

Given we just want to solve this master-password-and-sync issue, that sounds fine to me, and it doesn't cause future problems for other FxA-related services.  It doesn't *solve* similar problems for those future services, but this bug isn't trying to do that.

Brian, can you please confirm the above - that an attacker having access to your sessionToken but not the keys will be unable to access unencrypted password data via sync?  And that in general, this approach sounds fine?

If so, I think we should continue with the approach in the attached patches, but add the sessionToken to the "whitelist" of fields we store in the clear-text .json file.
Flags: needinfo?(warner-bugzilla)
Yeah, I agree. Leaving sessionToken outside the MP-protected box, and putting (kA, kB, unwrapKb, keyFetchToken) inside the MP-protected box, seems like a good split.

An attacker who has sessionToken, but not those other values, has slightly less power than the fxa-auth-server (the server also knows kA and wrap(kB)). They can spoof logins, but they don't learn the keys. We designed sessionToken and keyFetchToken to be peers (instead of having one giving access to the other) specifically to enable this split, so clients could deliberately forget one or the other when they didn't need it any longer.
Flags: needinfo?(warner-bugzilla)
Depends on: 1020851
To re-summarize this patch:

* nsILoginManager is used to save *some* of the user credentials, whether you use a MP or not.  If you don't, then this patch should be transparent and works just like before.

* We migrate data into the login manager and re-save the sanitised JSON when we notice stuff is in the JSON that shouldn't be (which should only be the first time this is run)

* The user is never prompted for a master-password as a result of this.  Another patch tweaks sync so a "Sync Now" command *does* prompt for the MP.  This is a change in behaviour from legacy sync where the user would be prompted as a sync was normally scheduled (good) with the cost being that a locked MP means sync is silently not doing anything (bad-ish)

This version has the following main changes from the earlier version:

* Creates a new LoginManagerStorage object so other users of FxAccounts.jsm can continue using the simple JSONStorage.  The patch unconditionally uses the new manager though - Jed, I'm assuming other consumers of this module will want to keep using JSONStorage, right?  If so, do you think a simple Services.appinfo check for desktop Fx would be OK?

* Stores the session token in the plain JSON.  browserid_identity now checks if there are none of keyfetchToken, kA and kB to see if a reauth is necessary.  I *think* we don't need kA in this check though (or was it kB? ;) - Chris?

* The "host" stored in the login manager is now "chrome://FirefoxAccounts" and the realm "Firefox Accounts credentials".  This makes it look alot like the legacy sync credentials.

* Tests, including that we "migrate" the contents of signedInUser.json when first loaded.
Attachment #8428514 - Attachment is obsolete: true
Attachment #8434803 - Flags: feedback?(jparsons)
Attachment #8434803 - Flags: feedback?(ckarlof)
I'd use:

  if (kA and kB):
    return "no reauth needed" # have the keys!
  if keyFetchToken:
    return "no reauth needed" # can get the keys!
  return "reauth needed" # need help

We should always be in one of two states: having both kA and kB, or having neither. Although I can imagine us changing that in the future.. the method that returns a kA or kB derivative checks to see if the necessary master key is available, and if not, triggers a reauth request (but if you don't need e.g. kA, it gets dropped).

Does this patch have the bit which prevents kA/kB/keyFetchToken from being synchronized like the other passwords? That won't matter much right now, but if we add a new key type in the future ("kC", for pairing), and decide to allow some devices to be more empowered than others, then it'll be important to avoid leaking all keys through the PW store.
As discussed with Gavin, changing this from a breakdown into an engineering bug.  Marco, can you please add it to the next iteration where I'll be picking it?
Flags: needinfo?(mmucci)
Summary: Breakdown - Enable password sync with FxA and master password → Enable password sync with FxA and master password
Thanks Mark.  I've added to the 33.1 priority sheet.
Flags: needinfo?(mmucci)
Comment on attachment 8434803 [details] [diff] [review]
0003-store-sensitive-FxA-data-in-the-login-manager.patch

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

Wow, this is quite a pretty little issue you've just pulled me into!  :)  I've carefully read this bug, bug 970167, and especially bug 995268, to try to understand the issues involved.  I hope I've done so successfully.

As I understand the goals for a short-term fix, this patch looks to me like it's doing the right thing.

And as you say in Comment 10, the UX is hard.  Has anyone from UX been involved yet?

I need to find out more from b2g people about the LoginManager question you pose here and in Comment 26.  How nice it would be if it just worked on all platforms.  I will do some asking around, but didn't want my feedback to have to wait on that answer.

::: services/fxaccounts/FxAccounts.jsm
@@ +870,5 @@
>   *                  baseDir: directory where the file resides
>   *                }
>   * @return instance
>   */
> + // XXX - is this hybrid of loginmanager+JSON able to be the default on b2g?

I think it should be ok based on this:

https://mxr.mozilla.org/mozilla-central/source/b2g/installer/package-manifest.in#383

I'll double-check with b2g people.  I can't find any instance of it in b2g.

@@ +917,5 @@
> +
> +  // The realm we use in the login manager.
> +  PWDMGR_REALM: "Firefox Accounts credentials",
> +  // The pseudo-host we use in the login manager
> +  PWDMGR_HOST: "chrome://FirefoxAccounts",

Since you also set these in the tests, maybe add them to Common?

@@ +1011,5 @@
> +    }
> +
> +    // if we have encryption keys it must have been saved before we
> +    // used the login manager, so re-save it.
> +    if (data.accountData.kA || data.accountData.kB || data.keyFetchToken) {

(Future generations might benefit from a link to the bug here)

::: services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js
@@ +188,5 @@
> +
> +  Assert.ok(!("kA" in data.accountData), "kA not stored in clear text");
> +  Assert.ok(!("kB" in data.accountData), "kB not stored in clear text");
> +
> +  // and it should magically be in the login manager.

Nice.  This is a pretty test.
Attachment #8434803 - Flags: feedback?(jparsons) → feedback+
Speaking of LoginManager: be aware that attempting to grab an instance can fail, because there are implementations of nsILoginManagerStorage (RoboForm, Keychain Services Integration) that don't work correctly. We don't have to support their bugs, but we should be defensive.
Sorry for the delay Mark. I'll get to this tomorrow.
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Whiteboard: p=5 [qa-] → p=5 s=33.1 [qa-]
We decided to not show any additional messages when a master-password is locked, but instead to just prompt for it to be unlocked.
Attachment #8428511 - Attachment is obsolete: true
Attachment #8428512 - Attachment is obsolete: true
Attachment #8438255 - Flags: feedback?(ttaubert)
Changes from last-time.

* Implemented the check suggested by warner, and he also reminded me to ensure the credentials themselves aren't synced.

* Wrapped all interaction with the login manager in try/catch blocks as implied by rnewman.  This even uses the new fancy logging feature to format the exception.

* The login-manager is only used on desktop by way of #ifdefs.

* As Jed suggested, the constants are directly in FxAccountsCommon.

All feedback welcome, especially from those I nominated ;)
Attachment #8434803 - Attachment is obsolete: true
Attachment #8434803 - Flags: feedback?(ckarlof)
Attachment #8438256 - Flags: feedback?(warner-bugzilla)
Attachment #8438256 - Flags: feedback?(rnewman)
Attachment #8438256 - Flags: feedback?(ckarlof)
Given the implementation explicitly avoids master-password prompts, this patch makes "sync now" prompt for an unlock.
Attachment #8428517 - Attachment is obsolete: true
Attachment #8438258 - Flags: feedback?(rnewman)
Sorry, I made a trivial fixup.
Attachment #8438256 - Attachment is obsolete: true
Attachment #8438256 - Flags: feedback?(warner-bugzilla)
Attachment #8438256 - Flags: feedback?(rnewman)
Attachment #8438256 - Flags: feedback?(ckarlof)
Attachment #8438266 - Flags: feedback?(warner-bugzilla)
Attachment #8438266 - Flags: feedback?(rnewman)
Attachment #8438266 - Flags: feedback?(ckarlof)
per comment #28 breakdown -> engineering bug. As such, this is qa+
Whiteboard: p=5 s=33.1 [qa-] → p=5 s=33.1 [qa+]
Flags: firefox-backlog+
Comment on attachment 8438266 [details] [diff] [review]
0002-Bug-1013064-part-2-store-sensitive-FxA-data-in-the-l.patch

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

::: services/fxaccounts/FxAccounts.jsm
@@ +877,5 @@
>   *                  baseDir: directory where the file resides
>   *                }
>   * @return instance
>   */
> + // XXX - is this hybrid of loginmanager+JSON able to be the default on b2g?

Sam, Jed, or ferjm can help answer this.

@@ +977,5 @@
> +      // and the stuff into the login manager.
> +      yield Services.logins.initializationPromise;
> +      // If MP is locked we silently fail - the user may need to re-auth
> +      // next startup.
> +      if (!this._isLoggedIn) {

I appreciate the simplicity here, but does this mean if the user logs in to Sync when her master password is locked, she get's no feedback why her FxA creds weren't persisted?
Attachment #8438266 - Flags: feedback?(ckarlof) → feedback+
Figuring out whether LoginManager works on b2g
(In reply to Chris Karlof [:ckarlof] from comment #38)

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +877,5 @@
> >   *                  baseDir: directory where the file resides
> >   *                }
> >   * @return instance
> >   */
> > + // XXX - is this hybrid of loginmanager+JSON able to be the default on b2g?
> 
> Sam, Jed, or ferjm can help answer this.

The LoginManager is supposed to work on b2g, but as :gavin has explained to :markh, who told it to me on irc, since there's no concept of a master password on b2g, there's not really any point on storing anything there on b2g.  So it looks like the patch should stay as it is (with the preprocessor check for b2g).
Comment on attachment 8438266 [details] [diff] [review]
0002-Bug-1013064-part-2-store-sensitive-FxA-data-in-the-l.patch

Daniel,
  This patch is probably the one most in need of the sec review, and is largely complete, notwithstanding some other review comments which I expect to be quite minimal.  comment 14 and comment 26 might add more context.

If we can get a prelim sec-review on this somewhat soon I'd appreciate it.
Attachment #8438266 - Flags: review?(dveditz)
Comment on attachment 8438258 [details] [diff] [review]
Prompt for master-password on "Sync Now"

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

I'm not sure I understand what's going on with this patch.

Current Sync only needs your MP *once*, because it caches your Sync credentials in the identity object. It prompts once by attempting to access your credentials, catches the exception if you cancel or fail to unlock, and in that case enters the Status.login == MASTER_PASSWORD_LOCKED state.

You're implying here that FxA needs to read from the login manager every time, otherwise this patch wouldn't exist.

But reading from a locked login manager *already* prompts for you to unlock it -- that's why syncIfMPUnlocked exists, so we don't spam you every time. Simply choosing Sync Now should call policies.js syncAndReportErrors, which calls Service.sync(), which will try to log in, which will try to read from the password manager, which will prompt you to unlock.

So what gives?

(And sidenote: you're changing the behavior for Old Sync, here, so you need to guard against that.)
Attachment #8438258 - Flags: feedback?(rnewman) → feedback-
This is an update to the patch which stores some of the credentials in the login manager.  It no longer has any sync/browserid_identity changes - these are now all in part 3.

This patch is, IMO, the only one that is security sensitive, so asking dveditz for review.  Chris, if you would prefer to punt this review to someone else (Jed, Warner?) feel free to do so.
Attachment #8438266 - Attachment is obsolete: true
Attachment #8438266 - Flags: review?(dveditz)
Attachment #8438266 - Flags: feedback?(warner-bugzilla)
Attachment #8438266 - Flags: feedback?(rnewman)
Attachment #8440238 - Flags: review?(dveditz)
Attachment #8440238 - Flags: review?(ckarlof)
Thanks Richard - these comments are very helpful.

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

> But reading from a locked login manager *already* prompts for you to unlock
> it -- that's why syncIfMPUnlocked exists, so we don't spam you every time.

Note that the "part 2" patch which uses the login manager never actually prompts for an MP unlock - it just avoids attempt to read if it is locked.  This is mainly as the code asks for the FxA credentials so often it would indeed spam the user or require complex state management to mitigate this.

This new patch works much closer with how sync expects this to work.  Specifically:

* If we are missing the credentials we still return STATUS_OK for currentAuthState - this is what the legacy identity also does, and IIUC, it happens because we query for the auth state before we've attempted to unlock.

* In verifyLogin, instead of assuming a reference to |this.identity.syncKey| unlocks the key, I've added a new identity method unlockAndVerifyAuthState().  The legacy provider implementation of this still just references .syncKey, but the bid_identity one checks to see if we have the keys, and if not, explicitly prompts for an unlock.  I believe this causes browserid_identity to have identical semantics to the legacy provider and avoids sync itself explicitly requesting an unlock.

So unlike my comments on previous patches, for better or worse, this patch causes Sync to prompt for an unlock in exactly the same way as legacy sync - ie, you will generally be prompted to unlock 10 seconds after a startup.  If you decline, sync backs off in the same way but still continues to nag you in the same way.

If you do unlock and the keys aren't there, sync goes into a "needs reauth" state, and after re-authenticating sync starts as normal.
Attachment #8438258 - Attachment is obsolete: true
Attachment #8440241 - Flags: feedback?(rnewman)
This patch removes all the cruft which forced the password engine to be disabled if we have a master-password - ie, it is this patch which will actually allow such users to re-enable password syncing.

Note we make no attempt to automagically re-enable the passwords engine - FxA sync users with a MP enabled will need to manually go to the sync prefs pane and enable the engine.

I wonder if we need a bug on file to remove the support article which describes the fact the password engine can't be enabled?
Attachment #8440243 - Flags: review?(ttaubert)
(In reply to Chris Karlof [:ckarlof] from comment #38)
> I appreciate the simplicity here, but does this mean if the user logs in to
> Sync when her master password is locked, she get's no feedback why her FxA
> creds weren't persisted?

Well, the act of logging in to sync will prompt for the MP to be unlocked.  If she declines to unlock I imagine she wouldn't be *too* surprised.  Next time the browser starts she will be forced to re-auth, and this too will prompt for an unlock.  So while I don't see this as ideal, I think it's acceptable.

If you think it really does need improving (which is fairly easy to argue), would you be happy to do that in a followup bug?
While testing this I found an edge-case.  Consider an existing MP user - what should happen is:

* On startup we see all the fields in the JSON, so we migrate.  We drop kA etc and re-write the JSON.  We try and save the keys in the login manager - but it's locked.  This happens immediately on startup, so we don't want to prompt for MP unlock as the user will have no clue why they are being asked.

* Those keys *are* still in memory though, so things *should* still work OK, and next time they do unlock we will write the keys back.

However, what actually happens is that fxAccounts.getUserAccountData() is called multiple times - these are promise-based, so multiple calls to read the JSON etc are in-flight at the same time.

The second such call to getUserAccountData doesn't see the cached values - it's not cached yet.  Thus it goes back to reading the disk.  At this point we have done the migration, so the keys aren't in the JSON.  The MP is still locked, so we don't get those keys.

This second call then caches its result - so the cached result, and the value returned by all subsequent calls, does not include those keys.

This patch fixes this by ensuring there is only one call to getUserAccountData() in-flight at once (by reusing the promise from the first call).  Thus, these users all see the full data, including the keys, and thus they do get re-saved once the user does unlock.
Attachment #8440557 - Flags: feedback?(jparsons)
Attachment #8440557 - Flags: feedback?(ckarlof)
There's still a one-off problem here for MP users.  Second run after these patches are applied, a user with a master-password will end up in an unauthorized state, and have to do a one-off reauth.  Assuming they unlock for that reauth, it all works fine after that.

The solution to this is a little painful - it probably involves remember what we want to save in the login manager and an observer for an MP unlock to update it.  And even that wouldn't guarantee things - if the user terminates without ever unlocking we would be in the same position.

So can we live with this one-off reauth for users with an MP enabled?
I didn't follow every case this patch applies to, but would any of those users be affected in any way?

- MP users who don't use FxA.
- MP users who don't use FxA AND still use the old sync.
(In reply to Mark Hammond [:markh] from comment #47)
> * Those keys *are* still in memory though, so things *should* still work OK,
> and next time they do unlock we will write the keys back.

What happens if we crash or exit before we can write them to the login manager? Why not only remove them from the JSON when we have successfully written them to login manager?
Comment on attachment 8440238 [details] [diff] [review]
0002-Bug-1013064-part-2-Store-sensitive-FxA-credentials-i.patch

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

I think this could use significant QA on the "unlikely" flows discussed in the below comment. I'd also like to know why that one function doesn't return true even thought it looks like it should.

::: services/fxaccounts/FxAccounts.jsm
@@ +938,5 @@
> +        Services.logins.removeLogin(login);
> +      }
> +    } catch (ex) {
> +      log.error("Failed to clear login data: ${}", ex);
> +    }

I don't see where this function returns true when the login manager is unlocked.

@@ +1045,5 @@
> +          // Merge the login manager data
> +          copyObjectProperties(lmData.accountData, data.accountData);
> +        } else {
> +          log.info("version field in the login manager doesn't match - ignoring it");
> +          yield this._clearLoginMgrData();

If we hit this line or the one below it, what should happen with the data in JSON storage? Should the user be forcibly logged out of the browser? Should we force a re-auth flow? How does the user recover in this situation? I'm not sure these cases (e.g., version mismatch) would likely come up, but the most likely case to this situation is 1) log in with user 1 pwd manager locked, 2) logout with it still locked, 3) login with user 2, and unlock pwd manager. It would be interesting to know what happens in this case.

::: services/fxaccounts/FxAccountsCommon.js
@@ +180,5 @@
> +// These constants relate to that.
> +
> +// The fields we save in the plaintext JSON.
> +// See bug 1013064 comments 23-25 for why the sessionToken is "safe"
> +this.FXA_PWDMGR_PLAINTEXT_FIELDS = ["email", "verified", "authAt", "sessionToken"];

We should include the "uid" in this list as well.
Attachment #8440238 - Flags: review?(ckarlof) → review+
Comment on attachment 8440557 [details] [diff] [review]
0005-Part-prevent-multiple-getUserAccountData-calls-from-.patch

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

I couldn't fully understand this patch without reading the comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1013064#c47

I would probably benefit from including some of the text of the above comment in the code to explain why those code paths are the way they are.
Attachment #8440557 - Flags: feedback?(ckarlof) → feedback+
(In reply to Avi Halachmi (:avih) from comment #49)
> I didn't follow every case this patch applies to, but would any of those
> users be affected in any way?
> 
> - MP users who don't use FxA.
> - MP users who don't use FxA AND still use the old sync.

Neither of these users are affected.

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #50)
> (In reply to Mark Hammond [:markh] from comment #47)
> > * Those keys *are* still in memory though, so things *should* still work OK,
> > and next time they do unlock we will write the keys back.
> 
> What happens if we crash or exit before we can write them to the login
> manager?

Sync will enter a "needs reauth" state.  This re-auth process will prompt to unlock, and if they decline, the user will remain in that "needs reauth" state next startup.

IOW, crashing before we wrote them is the same as the user declining to unlock.

> Why not only remove them from the JSON when we have successfully
> written them to login manager?

Because this would leave a period of time where the user is able to successfully sync their passwords, but the credentials are still in the plain-test JSON.
"I think this could use significant QA on the "unlikely" flows discussed in the
below comment. I'd also like to know why that one function doesn't return true
even thought it looks like it should."

+1000
Comment on attachment 8440557 [details] [diff] [review]
0005-Part-prevent-multiple-getUserAccountData-calls-from-.patch

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

Thanks, Mark.  I agree with Chris that the explanation in Comment 47 is important.  Could you add some or all of it in the form of comments?

::: services/fxaccounts/FxAccounts.jsm
@@ +127,5 @@
>            log.debug("setting signed in user");
>            this.signedInUser = user;
> +          // kill our "in-flight" promise - we'll always resolve with
> +          // this.signedInUser for future requests.
> +          this.whenUserDataReadyDeferred = null;

The double assignment in line 110 made it look to me as though `deferred` and `whenUserDataReadyDeferred` were supposed to be aliases for the same object, so it took me a moment to see that nulling the latter here and resolving the former (line 133 below) was ok.
Attachment #8440557 - Flags: feedback?(jparsons) → feedback+
(In reply to Chris Karlof [:ckarlof] from comment #51)

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +938,5 @@
> > +        Services.logins.removeLogin(login);
> > +      }
> > +    } catch (ex) {
> > +      log.error("Failed to clear login data: ${}", ex);
> > +    }
> 
> I don't see where this function returns true when the login manager is
> unlocked.

Good catch - it doesn't :(  However, the result of this function is only used in one place, and only to log a message indicating we couldn't drop it - so that doesn't affect the functionality of the patch (but obviously I'll fix it!)


> @@ +1045,5 @@
> > +          // Merge the login manager data
> > +          copyObjectProperties(lmData.accountData, data.accountData);
> > +        } else {
> > +          log.info("version field in the login manager doesn't match - ignoring it");
> > +          yield this._clearLoginMgrData();
> 
> If we hit this line or the one below it, what should happen with the data in
> JSON storage? Should the user be forcibly logged out of the browser? Should
> we force a re-auth flow? How does the user recover in this situation?

In this case we simply decline to copy the data from the login manager, but we *do* still return the JSON data - including the (presumably mis-matched) version field there.  So what should happen is whatever would currently happen on a mis-matched version string.  And sadly, I believe "whatever would currently happen" is broken - see bug 1026836.

So in effect the code is saying "whatever version is in the JSON wins, and if the login manager doesn't match, we simply decline to return it.  The caller remains responsible for validating the version".

> not sure these cases (e.g., version mismatch) would likely come up, but the
> most likely case to this situation is:
> 1) log in with user 1 pwd manager locked

At this point we have JSON and nothing in the storage manager (or if we do, the username doesn't match user1, so we decline to use it)

> 2) logout with it still locked

JSON on disk has "null", LoginMger is in the same state it was.  Note that on reading the creds, if the JSON is null we ignore the loginmgr data (and even attempt to remove it)

> 3) login with user 2, and unlock pwd manager.

JSON on disk has user 2, pwdmgr has user 2.

> It would be interesting to know what happens in this case.

I might be missing something, but I don't see any problem here.

(Note I do have the start of test-plan that I'm nearly ready to paste here)

> We should include the "uid" in this list as well.

Will do.
Same patch as before, just with a real commit message etc.
Attachment #8438255 - Attachment is obsolete: true
Attachment #8438255 - Flags: feedback?(ttaubert)
Attachment #8441869 - Flags: review?(ttaubert)
This is almost identical to the previous patch, but (a) includes the user's uid in the plain-text and (b) fixes the _clearLoginMgrData() function to always return a boolean.

re-requesting review from ckarlof only as I couldn't work out how to "carry the r+ forward" will still adding a request on dveditz.
Attachment #8440238 - Attachment is obsolete: true
Attachment #8440238 - Flags: review?(dveditz)
Attachment #8441877 - Flags: review?(dveditz)
Attachment #8441877 - Flags: review?(ckarlof)
This patch fixes the issue I mentioned in comment 48 and avoids the need for 0005-Part-prevent-multiple-getUserAccountData-calls-from-.patch

This patch is against "part 2", so it could be rolled up into that patch - however, I've kept it separate to make review of this change easier.

What we now do for migration is:

* Read the JSON from disk and find it hasn't been migrated.

* See if the master-password is unlocked.  A user without a MP enabled will see it as unlocked, so it will still migrate as normal.

* For a user with MP, the MP will *not* be unlocked the first time this is run - it happens near startup so the user couldn't have unlocked even if they wanted to.  So we decline to migrate at this point.

* Even though we haven't migrated (and thus have *all* keys etc available), we only return the JSON-safe fields - so the returned data *looks* like any other normal startup with the MP locked.

* Later, a sync wants to start and finds it doesn't have the keys (as we didn't return them, even though we had them).  So it prompts for unlock, and if granted, it re-requests the signedInUserData.

* At this point we re-read the JSON from disk, and find the keys etc are still there, so again check to see if we can migrate - but now the MP *is* unlocked - so we migrate and return all keys.

* Sync works

The end result here is that we will migrate as soon as Sync prompts to unlock - and thus we should still never be able to sync without the data migrated.

Requesting review from dveditz as this patch falls into the "security sensitive" parts of this patch set.
Attachment #8440557 - Attachment is obsolete: true
Attachment #8441888 - Flags: review?(dveditz)
Attachment #8441888 - Flags: review?(ckarlof)
Same patch as last one, but it's now in a different position in the set.
Attachment #8440241 - Attachment is obsolete: true
Attachment #8440241 - Flags: feedback?(rnewman)
Attachment #8441890 - Flags: review?(rnewman)
Same patch as last one, but it's now in a different position in the set.
Attachment #8440243 - Attachment is obsolete: true
Attachment #8440243 - Flags: review?(ttaubert)
Attachment #8441893 - Flags: review?(ttaubert)
Green-looking try at https://tbpl.mozilla.org/?tree=Try&rev=4db0492c11a1

Below are some notes I made re a test-plan, which I hope helps QA:

Migration of existing data (non-master-password) into password manager:
* On a build *without* this patch enabled, *without* a master-password enabled, but with a user signed in and configured for sync:
** Open signedInUser.json from the profile directory in a text editor, and verify it has fields keyFetchToken, kA and kB.
** Verify the password manager does *not* have a "Firefox Accounts" entry.
* Run a build *with* this patch enabled and after startup:
** Verify signedInUser no longer has kA or KB (ie, that it has been silently and automatically updated)
** Verify there is an entry in the password manager for Firefox Accounts.

Migration of existing data (with master-password) into password manager:
* On a build *without* this patch enabled, *with* a master-password enabled and with a user signed in and configured for sync:
** Open signedInUser.json from the profile directory in a text editor, and verify it has fields keyFetchToken, kA and kB.
** Verify the password manager does *not* have a "Firefox Accounts" entry.
* Run a build *with* this patch enabled:
** Verify no master-password prompt is shown at startup (note however that ~10 seconds after startup a prompt *is* expected as sync will attempt to start)
** Verify signedInUser.json *still does* have the fields mentioned above (we haven't migrated these users yet)
** Wait 10 seconds for sync to start, and verify MP unlock is requested.
** Unlock
** Verify sync works correctly.
** Verify signedInUser.json no longer has fields mentioned above (we migrated after sync attempted to start and prompted for unlock)
** Verify the pwdmgr has an FxA entry.

Verify no regressions:
* In a new profile, verify all sync functionality *without* a master-password enabled (ie, verify no regressions for users without a master-password)

Other verifications:
In a new profile:
* create a master-password.
* Signup for sync - verify about:accounts prompts for an unlock.
* Unlock the master-password.
* Sync should function normally without additional prompts until restart.
* Restart

* 10 seconds after restart sync should try and start - verify you are prompted for a master-password.
* Decline to unlock
* Verify no error states are shown - master-password being locked is not a reportable error state.
* Do tools->sync now - verify master-password prompt is shown.  Unlock and verify sync works correctly.

* restart
* 10 seconds after restart sync should try and start - verify you are prompted for a master-password.
* Unlock
* Verify sync works correctly.
* Tools -> Sync now, verify no unlock request is made (it is already unlocked)
* Wait a few minutes, verify syncs continue to happen normally without additional prompts.

In a new profile:
* Create master-password.
* Signup for sync - verify you are prompted to unlock
* decline to unlock.
* Verify sync *does* still start normally and continues to sync until a restart.

* restart and wait 10 seconds - verify you are prompted to unlock.
* Unlock.
* Verify that sync now enters a "you must reconnect" state (ie, the same state as if you had changed your FxA password on a different device) - note we enter this state due to declining to unlock when setting up sync, so we could not save the creds in the pwd-mgr
* Click to reconnect sync - verify you are prompted to unlock
* Unlock
* re-enter your current FxA password as part of the reconnect flow.
* verify sync works correctly for this session.
    
* restart, wait 10 seconds, verify you are prompted to unlock.
* unlock.
* verify sync works correctly (we now *do* have the credentials)

Verify another user's credentials aren't used:
In a new profile:
* Create master-password.
* Signup for a sync account, unlocking when prompted, verify sync works.
* Restart.
* Go to "sync options" and disconnect the account - but *decline* to unlock when asked in this session.
(Note at this point, the user's credentials are still stored in the password manager even though they are disconnected - as the user declined to unlock we can't remove them)
* Restart
* Start a new sync signup process with a different FxA account name, but always declining to unlock.
* Restart
* In 10 seconds, sync should prompt for a master-password unlock.
* Unlock
* Verify we enter the "you must reconnect" state for the correct new user - sync should *not* automatically start working using the previous user.

Verify the UI allows you to sync passwords:
* In a build *without* this patch, configure a MP, then configure sync and verify password syncing can't be enabled.
* Run a build *with* this patch.
* Go to sync->options and verify you can now enable the password engine.
* Enable the engine and verify passwords are synced.
The QA script looks fantastic. markh, you're right, I can't figure out what I was thinking in the above hypothetical situation I asked about with multiple users logging in with MP enabled.
Attachment #8441877 - Flags: review?(ckarlof) → review+
The QA script is so damn good, I sent a message to QA staff (subset) about it.
Comment on attachment 8441888 [details] [diff] [review]
0003-Bug-1013064-part-3-only-migrate-data-into-the-loginm.patch

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

Good catch.
Attachment #8441888 - Flags: review?(ckarlof) → review+
Comment on attachment 8441890 [details] [diff] [review]
0004-Bug-1013064-part-4-browserid_identity-and-sync-chang.patch

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

r+ with nits (assuming there's someone available to apply them with markh on vacation).

::: services/sync/modules/browserid_identity.js
@@ +422,5 @@
> +    // If bundle creation started, but failed due to any reason other than
> +    // the MP being locked...
> +    if (this._shouldHaveSyncKeyBundle && !this.syncKeyBundle && !Utils.mpLocked()) {
> +      // Return a state that says a re-auth is necessary so we can get keys.
> +      return LOGIN_FAILED_LOGIN_REJECTED;

Minor red flag: NO_PASSPHRASE is now never returned from browserid_identity. Perhaps fine...

@@ +434,5 @@
> +  _canFetchKeys: function() {
> +    let userData = this._signedInUser;
> +    // a keyFetchToken means we can almost certainly grab them.
> +    // kA and kB means we already have them.
> +    return userData.keyFetchToken || (userData.kA && userData.kB);

Worth checking that userData isn't falsy before we go grubbing around in `undefined`.
Attachment #8441890 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #66)
> r+ with nits (assuming there's someone available to apply them with markh on
> vacation).

Thanks!

> Minor red flag: NO_PASSPHRASE is now never returned from browserid_identity.
> Perhaps fine...

Yeah, I think that is fine - that state doesn't really make sense for us, and I don't think we ever actually saw it (ie, that was from the very early days when we did a copy-pasta from the legacy identity provider).

> Worth checking that userData isn't falsy before we go grubbing around in
> `undefined`.

In theory it will not be null, but yeah, it makes sense to be safe.
Assignee: mhammond → enndeakin
Iteration backlog updated with new assignee
Attachment #8441869 - Flags: review?(ttaubert) → review+
Comment on attachment 8441893 [details] [diff] [review]
0005-Bug-1013064-part-5-stop-disabling-the-password-engin.patch

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

gSyncUtils.openMPInfoPage() in browser/base/content/sync/utils.js doesn't seem to be needed anymore.

::: browser/base/content/sync/customize.js
@@ +7,5 @@
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  let service = Cc["@mozilla.org/weave/service;1"]

This can be removed as well then.

::: browser/components/preferences/in-content/sync.xul
@@ -290,5 @@
> -                      preference="engine.passwords"/>
> -
> -            <vbox id="fxa-pweng-help">
> -              <spacer flex="1"/>
> -              <hbox id="fxa-pweng-help-link">

There's some leftover CSS for #fxa-pweng-help-link that wants to be removed as well.
Attachment #8441893 - Flags: review?(ttaubert) → review+
Iteration: --- → 33.2
Points: --- → 5
Whiteboard: p=5 s=33.1 [qa+] → [qa+]

        Attachment #8441893 -
        Attachment is obsolete: true

        Attachment #8446066 -
        Flags: review+

    
  

        Attachment #8446065 -
        Flags: review+

    
  

        Attachment #8441877 -
        Flags: review?(dveditz)

    
  

        Attachment #8441888 -
        Flags: review?(dveditz)

    
    

    
  
So this bug is just waiting for a security review.
Iteration: 33.2 → 33.3

    
    

    
  
Can't wait this long on security review.

Neil: please go ahead and land this.

    
    

    
  
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)

    
    

    
  
If this is focused on desktop, the probably :tracy
Work with him to get proper QA coverage.
Flags: needinfo?(florin.mezei) → needinfo?(twalker)

    
    

    
  
Yes, QA for this is mine. A long read here to understand the details of the final resolution. Then, obviously, much to test around. I should be able to complete by eod today, if not, tomorrow.
QA Contact: twalker
Flags: needinfo?(twalker)

    
    

    
  
in signedInUser.json, I'm assuming "keyFetchToken" stated in STR's in comment #62 is the "unwrapBkey" shown here:

{"version":1,"accountData":{"email":"<snip>","uid":"2a72a57d765d4bc497224a6e09419ecd","unwrapBKey":"9433be64f1d64a3efd727171d1a1d730387b4f57a8624f0260465e03ce40cfa3","sessionToken":"69dfe3095c54d21cac82b8e67e4b9a5bbaf715bd69e670f95c36f89fba64f81a","sessionTokenContext":"fx_desktop_v1","verified":true,"kA":"58eb2a29160c97f8779d1f0366abe7657bbaec5ae7a35abf3e3b0b1015d70699","kB":"cd7d08e07982c1bbb5eb7f3359241e267ad5731fde56c29783043ea59c8f4667"}}

This is what is left on MP supported builds:

{"version":1,"accountData":{"email":"<snip>","uid":"2a72a57d765d4bc497224a6e09419ecd","sessionToken":"69dfe3095c54d21cac82b8e67e4b9a5bbaf715bd69e670f95c36f89fba64f81a","verified":true}}


I'm no sure how it happened, sync seemed to be working ok with MP on latest build. But, after a restart, in the profile with MP enabled, the browser does not prompt for MP, Password sync got turned off and every 90 seconds I get the follow in the logs:

1405696542534	Sync.ErrorHandler	DEBUG	Flushing file log.
1405696542534	Sync.Service	DEBUG	Exception: No keyFetchToken No traceback available
1405696542535	Sync.Service	DEBUG	Not syncing: login returned false.
1405696542535	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696542535	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696542535	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696542536	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696542536	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696542536	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696542537	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696542537	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696542537	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696542539	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696542539	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696542539	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696542540	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696542540	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696542540	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696542540	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696542540	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696542540	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696542541	Sync.ErrorHandler	DEBUG	Log cleanup threshold time: 1404832542541
1405696542542	Sync.ErrorHandler	DEBUG	No logs to clean up.
1405696542542	Sync.ErrorHandler	DEBUG	Log cleanup threshold time: 1404832542542
1405696542542	Sync.ErrorHandler	DEBUG	No logs to clean up.
1405696574269	Sync.Tracker.History	DEBUG	Saving changed IDs to history
1405696604529	Sync.Tracker.History	DEBUG	Saving changed IDs to history
1405696620734	Sync.Tracker.History	DEBUG	Saving changed IDs to history
1405696632532	Sync.Service	DEBUG	User-Agent: Firefox/33.0a1 FxSync/1.35.0.20140714030201.
1405696632532	Sync.Service	INFO	Starting sync at 2014-07-18 10:17:12
1405696632533	Sync.Service	DEBUG	In sync: should login.
1405696632533	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696632533	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696632533	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696632533	Sync.BrowserIDManager	INFO	currentAuthState returning error.login.reason.network due to previous failure
1405696632533	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696632533	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696632536	Sync.BrowserIDManager	INFO	Waiting for user to be verified.
1405696632536	FirefoxAccounts	DEBUG	already verified
1405696632537	Sync.BrowserIDManager	INFO	Starting fetch for key bundle.
1405696632537	Sync.BrowserIDManager	INFO	Fetching assertion and token from: https://token.services.mozilla.com/1.0/sync/1.5
1405696632537	FirefoxAccounts	DEBUG	already verified
1405696632538	Sync.BrowserIDManager	ERROR	Non-authentication error in _fetchTokenForUser: undefined
1405696632538	Sync.Status	DEBUG	Status.login: error.login.reason.network => error.login.reason.network
1405696632538	Sync.Status	DEBUG	Status.service: error.login.failed => error.login.failed
1405696632538	Sync.SyncScheduler	DEBUG	Clearing sync triggers and the global score.
1405696632538	Sync.SyncScheduler	DEBUG	Next sync in 90000 ms.

last successful sync prior to the failures contained:

1405696083380	Sync.Declined	DEBUG	Handling remote declined: ["passwords"]

I then turned off MP, restart and sync is still broken with the above error.login.failed logs  

investigating more (trying to reproduce with new profiles), but thought I'd post current testing status.

    
    

    
  
I can't reproduce the above failure with clean profiles and a brand new Fx account. One client has MP enabled and a second client is not using MP.

In the above failure, I was using an oldish account, also with one client MP enabled and a second client not using MP.

    
    

    
  
In a new profile:
* Create master-password.
* Signup for sync - verify you are prompted to unlock
* decline to unlock.
* Verify sync *does* still start normally and continues to sync until a restart.

Tested results: Sync does not work

proceed on with:

* restart and wait 10 seconds - verify you are prompted to unlock.
* Unlock.
* Verify that sync now enters a "you must reconnect" state (ie, the same state as if you had changed your FxA password on a different device) - note we enter this state due to declining to unlock when setting up sync, so we could not save the creds in the pwd-mgr
* Click to reconnect sync - verify you are prompted to unlock
* Unlock
* re-enter your current FxA password as part of the reconnect flow.
* verify sync works correctly for this session.

Tested results: Sync still does not work.

Based on this and comment #27 failure (both put the browser in a state where Sync doesn't work) I am reopening this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
I think the patch(es) may have to be backed out.  The failures I reported above put sync into a broken state that can't be recovered from.

    
    

    
  
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #81)

> Based on this and comment #27 failure (both put the browser in a state where
> Sync doesn't work) I am reopening this bug.

Make that comment #79

    
    

    
  
Neil, can you investigate ASAP.
Flags: needinfo?(enndeakin)

    
    

    
  
I backed the patches out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f09caa4ea7c5
Flags: needinfo?(enndeakin)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/f09caa4ea7c5
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Iteration: 33.3 → 34.1

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Are there known steps how to "fix" an installation/profile that experiences these errors? Interestingly I never used master password, but still have the same errors:

1406539728710	Sync.BrowserIDManager	ERROR	Non-authentication error in _fetchTokenForUser: undefined
1406539728710	Sync.Status	DEBUG	Status.login: success.login => error.login.reason.network
1406539728710	Sync.Status	DEBUG	Status.service: success.status_ok => error.login.failed

or

1406539791382	Sync.ErrorHandler	DEBUG	Flushing file log.
1406539791383	Sync.BrowserIDManager	ERROR	Background fetch for key bundle failed: No keyFetchToken
1406539791383	Sync.BrowserIDManager	ERROR	Could not authenticate: No keyFetchToken
1406539791383	Sync.SyncScheduler	DEBUG	Clearing sync triggers and the global score.
1406539791384	Sync.SyncScheduler	DEBUG	Next sync in 90000 ms.

    
    

    
  
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #81)
> In a new profile:
> * Create master-password.
> * Signup for sync - verify you are prompted to unlock
> * decline to unlock.
> * Verify sync *does* still start normally and continues to sync until a
> restart.
> 
> Tested results: Sync does not work

When I try these steps, sync seems to work. The sync password is not listed in the passwords dialog.

> * restart and wait 10 seconds - verify you are prompted to unlock.
> * Unlock.
> * Verify that sync now enters a "you must reconnect" state (ie, the same
> state as if you had changed your FxA password on a different device) - note
> we enter this state due to declining to unlock when setting up sync, so we
> could not save the creds in the pwd-mgr
> * Click to reconnect sync - verify you are prompted to unlock

However, here, I am not prompted to unlock again. I enter the password and continue. Sync continues to work. However, now the sync password is listed in the passwords dialog.

    
    

    
  
I can't reproduce this failure described in comment 81 or 79.

Tracy, what do you mean by 'sync does not work'? For me, I get a black error bar along the bottom of the browser if I decline to unlock the master password during the sync that says a failure occurs, but the log shows that a sync occurs anyway except for the passwords which don't sync.

I posted a new try build with these patches at https://tbpl.mozilla.org/?tree=Try&rev=386198af9636

Could you retest and provide more details/logs/etc?
Flags: needinfo?(twalker)

    
    

    
  
Neil,  I'm in QA workweek today and traveling back home tomorrow.  I'll give the try build a go on Monday.
Flags: needinfo?(twalker)

    
    

    
  
With the new try build, I'm unable to reproduce the failures I previously saw.  I have had some local network flakiness since replacing lightning destroyed routers, perhaps that us what was driving sync failures in earlier testing.  

Please re-land and I'm sorry about the delay.
Iteration: 34.1 → 34.2

    
    

    
  
Hey all. I created bug 995268 but have only just seen this thread.

It seems like you've all been really hard at work on the problem which is great to see. :-)

As far as I can tell (I'm not a C/C++ programmer so some of the patches are above my head), you've opted to store the sensitive FxA details within the password manager but mark them so that they are never uploaded to the Sync server, just like legacy Sync.  This is great news for me.

You talk quite a bit about the problems of UI and sync being locked due to an unknown master password.  I would recommend the following:

- When a user enables Firefox Sync for the first time on a device, the Sync button (two arrows in a circle) is automatically placed on their toolbar (they can obviously remove this in the usual way if they want.)
- When Firefox Sync attempts to sync but encounters an error, the button changes appearance to make it really obvious that something has gone wrong (e.g. it turns red).  The mouse over text would explain what went wrong.
- Every time a Sync fails, a message could also be displayed in the same manner to when a pop-up window is blocked (instead of "Firefox prevented this site from opening 2 pop-up windows" it could say "Firefox Sync cannot sync your data as the Master Password is locked.  Click here to unlock"

It would also be nice to have a bit more description for these events:

User enables Firefox Sync when Master Passwords are currently enabled...
"You currently have Master Password enabled. Firefox Sync will only be able to sync your info when your Master Password is unlocked. Please remember this and check the Firefox Sync toolbar button for Sync status"

User enables Master Password when Firefox Sync is currently in use...
"You are currently syncing with Firefox Sync.  Firefox Sync will only be able to sync your info when your Master Password is unlocked. Please remember this and check the Firefox Sync toolbar button for Sync status"

Thanks again, Ben

    
    

    
  
Not sure I agree with bug 995268 being marked as a duplicate of this bug because that's where I initially made the bug report and this one was created as an offshoot of that.  Also as an aside; whilst not a C/C++ developer (I mainly deal with LAMP dev), I'd like to get involved more with technical/design decisions w.r.t. Firefox.  How do I go about registering this interest?

    
    

    
  
What aspect of 995268 do you think has not been addressed?

    
    

    
  
I wasn't saying that bug 995268 hasn't been addressed - just that it is the original bug report and not a duplicate of this one :)

    
    

    
  
> Also as an aside; whilst not a C/C++ developer (I mainly deal with LAMP dev), I'd like to get involved more with technical/design decisions w.r.t. Firefox.  How do I go about registering this interest?

Most of the relevant dev for this issue (and many others) is in JS. The most direct way to act on your interest is to contribute code for open bugs that have been prioritized highly.

    
    

    
  
Do you know in which FF version it will be included?

    
    

    
  
Can we push it to Aurora too?

    
    

    
  
(In reply to Melendro from comment #100)
> Do you know in which FF version it will be included?

See the Target Milestone field for that information. It says that it will be Firefox 34.

(In reply to Ben from comment #101)
> Can we push it to Aurora too?

Given that it still needs security review and is a 5-part patch set, I doubt it will be uplifted, even though it is obviously high value. The whole point about the "rapid release" process is to not do such high risk changes during the stabilization process (aurora/beta) but instead have it "ride the trains" and go through the proper testing process but still go out to users in a good time frame.

    
    

    
  
(In reply to [:markh away until August 18] Mark Hammond from comment #45)
> I wonder if we need a bug on file to remove the support article which
> describes the fact the password engine can't be enabled?

Thanks to you all for your hard work at resolving this bug!

Yes. Removing the article when the fix hits the release channel would be a tremendous way to reduce confusion. In the mean time, I sent feedback today to request changes to the article
https://support.mozilla.org/en-US/kb/why-cant-i-sync-my-passwords . 

I requested addition of a date, versions affected, a snipped indicating work is ongoing, and a recommended course of action for users of Sync/MP. 

A final post to bug 995268 (and this bug as well), briefly explaining the same information would also be more helpful than marking that bug as a duplicate of this bug (or complete), thus leaving typical users to read and interpret complex security dev comments in an attempt to extract advice on how to proceed.

From my read of this thread, I suggest the update/recommendation might contain the advice that users avoid update from their current version until a fix in version 34 releases toward the end of 2014 which is expected to return functionality similar to Sync 1.1, but under the new web login scheme.
Agreed?

    
    

    
  
Tracy is this something you can give a shot at verifying this week? Thanks!
Flags: needinfo?(twalker)

    
    

    
  
> From my read of this thread, I suggest the update/recommendation might
> contain the advice that users avoid update from their current version until
> a fix in version 34 releases toward the end of 2014 which is expected to
> return functionality similar to Sync 1.1, but under the new web login scheme.
> Agreed?

One user's input.  I had tried the earlier incarnations in nightlies, and found that I had to blow away my sync account and start from scratch once 34 was fixed.  If this is going to be common, you certainly want people to wait.  But possibly, my difficulties were with previous nightly attempts, not with the released code.

    
    

    
  
Looks good on latest Nightly.

Will need to add these test cases to moztrap and figure out which can be done in TPS.
Status: RESOLVED → VERIFIED
Flags: needinfo?(twalker)
Flags: in-qa-testsuite?(hskupin)
Flags: in-moztrap?(twalker)

          status-firefox34:
          --- → verified
Keywords: verifyme
Whiteboard: [qa+]

    
    

    
  
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #106)
> Will need to add these test cases to moztrap and figure out which can be
> done in TPS.

For TPS best is to talk with Daniel and Mihaela, so that they can be added to our list of tests to be added.
Flags: qe-verify+

    
    

    
  
Removing my name from in-qa-testsuite flag for a better query.
Flags: in-qa-testsuite?(hskupin) → in-qa-testsuite?

    
    

    
  
I created bug 1063820, and I just read half of this thread in reverse order. So if I understand this patch correctly, it will allow the master password to lock the Firefox Account credentials, but the credentials will never be synced while the rest of the passwords are?

    
    

    
  
(In reply to Patrick Seiter from comment #110)
> I created bug 1063820, and I just read half of this thread in reverse order.
> So if I understand this patch correctly, it will allow the master password
> to lock the Firefox Account credentials, but the credentials will never be
> synced while the rest of the passwords are?

Yes - the FxA credentials are stored in the login manager (so aren't available until the MP has been unlocked) and these credentials are never synced.

    
    

    
  
How do I STORE the sync password in my password manager? there is no prompt and it doesn't seem to add it automatically. I switched from the old to the new sync and all it did was delete my "weave" credentials from password manager. I would expect it to store in password manager under "about:accounts" or somewhere. 

I just added Sync (Signing on to existing account) to Fx aurora 34.0a2 after setting a master password for Password Manager. Am I missing a step?

    
    

    
  
(In reply to Axel Grude [:realRaven] from comment #113)
> How do I STORE the sync password in my password manager?

Your "credentials" should be stored there automatically when you log in - there should be a firefox account entry there with a json string instead of a password.  The password itself is never stored.

    
    

    
  
Hi

(yes bugs reports are not fors discussions, and so on... but these scenario haven't been designed in https://wiki.mozilla.org/User_Services/Sync/Relaunch#Desktop_MVP but they happen in bugzilla)

I'm UPSET !
Mozilla Sync was a GREAT GREAT feature that kept me using Firefox instead of any web browser since it was introduced, because password sync is VITAL, to me and probably hundred of thousand of users.

Since Fx 32, I'm being forced:
- to drop usage of the old Sync
- to upgrade to Fx 32 (every tentative to downgrade to 29, 31 and disable auto update has failed, every 2 days I'm asked to reboot my computer to complete upgrade) (couldn't find a bug report about this actually)
- to drop usage of Mozilla SeaMonkey since they didn't implement FxA before it became mandatory in Firefox see bug 1022319
- to drop usage of master password, see bug 970167, which is VITAL requirement if storing passwords in my browser profile, as documented by Mozilla itself
- thus to drop password sync, which was my first motivation to setup Firefox on Android (and my primary reason to use Firefox at all)

And I now have to wait Fx 34 !! (more than 2 months) to recover usage of password sync ?!

This isn't a migration plan as described at https://wiki.mozilla.org/User_Services/Sync/Relaunch, this is a user frustration plan
You should display votes on support pages like https://support.mozilla.org/en-US/kb/why-cant-i-sync-my-passwords to understand how users feel with this product

How can you force to upgrade to a system that isn't 100% equivalent to the previous featureset ?

    
    

    
  
(In reply to Olivier Vit (just a reporter) from comment #115)
> ...
> Since Fx 32, I'm being forced:
> - to drop usage of the old Sync
> ...

AFAIK you're not forced to drop the old sync. AFAIK the old sync servers still work and operational, and the code within Firefox to use an old sync server (either Mozilla's or your own) still exists and works.

I still use the old sync with current Firefox nightly and release (with my own server - see http://avih.github.io/blog/2013/05/31/private-firefox-sync-server-in-5-mins/ or many other guides).

Newer Firefox indeed doesn't let you setup old sync server, but you can run an older Firefox (with your current profile) which does let you setup an old sync server, and once it's set up, switch back to newer Firefox and old sync will still work. You'd need this procedure (old Firefox -> setup old sync -> back to use new Firefox) on each system where you'd like to setup old sync.

Don't know how long Mozilla's old-sync servers will keep working or how long the old-sync code in Firefox will keep working (if you use your own old sync server), but for now, Firefox nightly still works with the old sync, at least if you have your own server, but AFAIK also if you use Mozilla's server.

    
    

    
  
(In reply to Avi Halachmi (:avih) from comment #116)
> I still use the old sync with current Firefox nightly and release 

I don't know how you can perform this since Fx 32
Each time I'm ugraded to Fx 32, my existing old Sync setup is lost, I need to setup Fx 28 again, launch it once, and setup 31 again.

To stick to auto-update disable, I'm now trying this configuration in a general.config.filename (I'm running multiple profiles, through Profile Manager https://developer.mozilla.org/en-US/docs/Profile_Manager )

lockPref("app.update.auto", false);
lockPref("app.update.enabled", false);
lockPref("app.update.silent", false);
lockPref("app.update.service.enabled", false);
lockPref("app.update.staging.enabled", false);
lockPref("services.sync.prefs.sync.app.update.mode", false);

I'm also trying this option (which lacks awareness)
lockPref("services.sync.fxaccounts.enabled",false);

    
    

    
  
Hi guys,

Is it possible to backport this patch into 'esr' branch?
Many people uses 'esr'. We all need this fix :)

    
    

    
  
(In reply to Dmitriy Popkov from comment #118)
> Hi guys,
> 
> Is it possible to backport this patch into 'esr' branch?
> Many people uses 'esr'. We all need this fix :)

I don't think that's possible as it would conflict with our policy of only shipping bug fixes, not features between major ESR versions. However, I've flagged Release Management to review this request.
Flags: needinfo?(release-mgmt)

    
    

    
  
I had to try Firefox Aurora on my Android device to see if an unrelated bug was fixed and noticed Firefox Sync has been removed while Firefox Accounts is still not feature complete. I think this is a major mistake and would like to make sure Firefox Sync will still be present in Firefox 35 until Firefox Accounts is at least able to support the Master Password.

It is already painful enough that I need to install and old Firefox when I want to synchronize a newly installed device, if I need to use the Long Term Support version just to keep my passwords encrypted, that will be a bit too much for me.

I don't want to be forced to use another tool or extension to save passwords.

    
    

    
  
(In reply to Jérôme Poulin from comment #120)
> … and noticed Firefox Sync has been removed

Why do you say that?

> while Firefox Accounts is still not feature complete.

Why do you say that?

This bug in particular is fixed in 34 (Aurora) on desktop, but doesn't affect Android -- Firefox for Android has *never* supported syncing with a master password enabled. This has been the case since Firefox 14, when Sync first shipped in Native Fennec.

    
    

    
  
I say that Firefox Sync has been removed in Aurora since it won't use my Firefox Sync Android account that is currently working with Firefox and Firefox Beta, it only asks to setup a new Firefox Account.

Firefox Accounts is not feature complete since it is incompatible with the master password.

I don't think sending screenshots would be very productive but my current Firefox setup is version 31.0 on Android using "Firefox Sync (deprecated)" and with Master Password enabled which pops when a web site with a password box is shown after Firefox has been killed.

    
    

    
  
(In reply to Jérôme Poulin from comment #122)
> I say that Firefox Sync has been removed in Aurora since it won't use my
> Firefox Sync Android account that is currently working with Firefox and
> Firefox Beta, it only asks to setup a new Firefox Account.

Assuming you're talking about Android, again, that's not true: go look in Android Settings > Accounts & sync. You can create a "Firefox Sync (deprecated)" account through to current Nightly 36.


> Firefox Accounts is not feature complete since it is incompatible with the
> master password.

As the VERIFIED FIXED status of this bug shows, that's not true as of Firefox 34.

 
> I don't think sending screenshots would be very productive but my current
> Firefox setup is version 31.0 on Android using "Firefox Sync (deprecated)"
> and with Master Password enabled which pops when a web site with a password
> box is shown after Firefox has been killed.

I would be surprised (but pleased) if this is syncing passwords. Nothing has changed in that regard on Android; literally the exact same code is running to sync passwords with an old Sync account or a new FxA Sync account.

    
    

    
  
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #119)
> (In reply to Dmitriy Popkov from comment #118)
> > Hi guys,
> > 
> > Is it possible to backport this patch into 'esr' branch?
> > Many people uses 'esr'. We all need this fix :)
> 
> I don't think that's possible as it would conflict with our policy of only
> shipping bug fixes, not features between major ESR versions. However, I've
> flagged Release Management to review this request.

As Anthony said, our policy is to focus on stability and security fixes for Firefox ESR. This is both what the consumers of this channel have told us they want and the level of support that we are willing to provide. If you are interested in the latest features, I suggest moving to the mainline Firefox releases (if this is possible).
Flags: needinfo?(release-mgmt)

    
    

    
  
(In reply to Olivier Vit (just a reporter) from comment #117)
> (In reply to Avi Halachmi (:avih) from comment #116)
> > I still use the old sync with current Firefox nightly and release 
> 
> I don't know how you can perform this since Fx 32
> Each time I'm ugraded to Fx 32, my existing old Sync setup is lost, I need
> to setup Fx 28 again, launch it once, and setup 31 again.
> 
> To stick to auto-update disable, I'm now trying this configuration in a
> general.config.filename (I'm running multiple profiles, through Profile
> Manager https://developer.mozilla.org/en-US/docs/Profile_Manager )
> 
> lockPref("app.update.auto", false);
> lockPref("app.update.enabled", false);
> lockPref("app.update.silent", false);
> lockPref("app.update.service.enabled", false);
> lockPref("app.update.staging.enabled", false);
> lockPref("services.sync.prefs.sync.app.update.mode", false);
> 
> I'm also trying this option (which lacks awareness)
> lockPref("services.sync.fxaccounts.enabled",false);

Couldn't prevent auto update to run with these settings
I ended with running the ESR version and that's a working solution for the moment
https://www.mozilla.org/en-US/firefox/organizations/

    
    

    
  
(In reply to Richard Newman [:rnewman] from comment #121)
> Firefox for Android has *never* supported syncing with a
> master password enabled. This has been the case since Firefox 14, when Sync
> first shipped in Native Fennec.

I have been running sucessfully Firefox Sync with master password enabled on desktop AND Android from January to April 2014
I have been using https://addons.mozilla.org/fr/android/addon/mobile-password-manager/ along (accessing passwords through that extension would prompt for master password input)

Since Firefox 29, I couldn't use Firefox Sync again on Android until today: following your instructions going through Android / Accounts / Firefox Sync, it works !
But the Firefox GUI doesn't offer that option and since Fx 29 it didn't import my existing Sync configuration from Fx 28

User experience is terrible, as well as user documentation on this topic.

    
    

    
  
Ok fellas need some help with the workaround presented above to use old-type sync on Android devices: at the time it was instructed to install mozilla/fennec 28.0, configure its (old-type) sync and THEN let it upgrade to the latest firefox version. This way it kept the old-type sync.

I have upgraded my nexus 7 tablet to Android L and on this platform although installation of fennec 28 is possible, trying to actually run it throws an error.

Does anyone know a workaround? That is install *and* run an old-type sync firefox client on Android L, without having to root the device (if rooted I could backup my working recent ff with its old-type sync and restore it in its entirety with titanium backup)?

Any help will be appreciated. And apologies for this bug-report partial hijacking...

    
    

    
  
(In reply to Michail Pappas from comment #127)
> Ok fellas need some help with the workaround presented above to use old-type
> sync on Android devices: at the time it was instructed to install
> mozilla/fennec 28.0, configure its (old-type) sync and THEN let it upgrade
> to the latest firefox version. This way it kept the old-type sync.

There is no need to do this. Firefox for Android up to 37 continues to support creating old Sync accounts.

Settings > Accounts & sync > Add > "Firefox Sync (deprecated)".
Flags: in-moztrap?(twalker)

    
    

    
  
Could it be possible that this bug has returned? I was able to sync passwords between my Ubuntu desktop and Android phone (both with master passwords set) up to about a month ago.

Now I see that the option for syncing passwords is available and selected in both platforms, but the information is not being synced up.

Also, at least on my Asus Zenfone 2, I don't see the option to add a "Firefox Sync (deprecated)" account.

    
    

    
  
The old Sync account type was removed on Android. You can no longer use it or sign in. 

Please file a new bug with any error logs in your about:sync-log from desktop.
Flags: in-qa-testsuite?
Flags: sec-review?(dveditz)
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
See Also:  → 1479749

    
    

    
  
With this bug marked as "VERIFIED FIXED", and still plenty of resources claiming the opposite: is there a bug still left open that will actually fix what the title of this bug claimed it was set out to do? I couldn't find it, but I'd hate to open a duplicate to get this encouragement for not using a password, in order to have sync fulfil its purpose, addressed.



    
    

    
  
To clarify, are you talking specifically about Firefox for Android not syncing passwords when MP is enabled, or a more general issues with password syncing on, say, Desktop Firefox with MP enabled?


Flags: needinfo?(age.bosma)

    
    

    
  
I'm talking about sync between Android and desktop with MP enabled on both sides.


The three pages I referenced are not clear about what is supposed to work. No sync between different Android devices with MP at all or only no sync between Android and desktop with MP.


Flags: needinfo?(age.bosma)

    
    

    
  
(+Matt for context on password syncing behaviour on our various platforms)


IIUC, password sync should work correctly with master-password enabled on Desktop (which was the scope of this bug) but it's still the case that Android will not sync passwords when master-password enabled.  Nick, you seem likely to have the most context on that restriction, is my interpretation accurate?


I'm not aware of any active work to change this behavior on Android.


Flags: needinfo?(nalexander)
See Also:  → 711636

    
    

    
  
Thanks Mark! Clearing Nick's ni? because I think Bug 711636 has all the right context.


Flags: needinfo?(nalexander)

    
    

    
  
( i commented in #995268 by mistake, duplicating the comment here )


Hi, i'm getting this error. Sync works as i can see the updated bookmarks, but for the love of Password Managers gods, i can't sync passwords. I use master password.


Ubuntu Linux 18.04

Firefox 67.0


The other system is Archlinux, with the updated firefox.


As a developer, I travel around and i switch from working from the pc to the laptop a lot.


What i been doing is keeping chrome around because it can sync properly, and i just have to open it to get the old passwords.



    
    

    
  
(In reply to leite.carvalho from comment #137)




( i commented in #995268 by mistake, duplicating the comment here )


Hi, i'm getting this error. Sync works as i can see the updated bookmarks, but for the love of Password Managers gods, i can't sync passwords. I use master password.




What error are you getting exactly? There are no known issues syncing passwords with desktop Firefox, so it would be great if you could please clarify.



    
    

    
  
Hi, thanks for your help.


I don't know how to find what errors it was throwing, all i could see is that the passwords didn't sync. But! its all in the past, the current versions of firefox ( 69.0 ) on both machine started syncing passwords. thanks everyone!



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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: