973749 - browserid_identity doesn't clear enough prefs as a new user logs in.

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #972070 +++

Bug 972070 has morphed into addressing a problem in FxAccounts, whereby it doesn't drop some of the "current account" state as a new user logs in.

This bug is about a similar issue in browserid_identity (and to a lesser extent, sync itself).  When a new user starts to log in, the .username attribute and some other credentials (eg, the token), and the clusterURL attribute on the Service will still reflect the previous user.  This causes the first request for the new user to hit the previous user's cluster URL.  Although the 401 resulting from this is correctly handled, it's bad and should be fixed.

Comment 1

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

I hoped to have a patch to put up today; although I've a patch that works, I'm bashing my head against the tests (which, in general, seem to dislike having the clusterURL change on logout or login).

Hopefully I'll be able to bash things into submission tomorrow...

Comment 2

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

Attachment: 0001-Bug-973749-Have-bid_identity-drop-more-credentials-a.patch

(In reply to Mark Hammond [:markh] from comment #1)
> I'm bashing my head against the tests

Problem here was simply that a couple of tests now need to have the cluster URL set after credentials are setup, not before.

This patch attempts to fix the described problem by:

* Ensuring the .username (and thus all credentials) is dropped as a new user logs in and as a user logs out.

* Resets both ._token and ._account when the credentials are dropped.

* Changes Service._updateCachedURLs() to not update the URLs when the username is null or an empty string (currently it only checks for an empty string).  Service.js itself sets the initial username to null, causing the cluster URL to occasionally embed 'null' directly in the URL (which itself is probably a regression since FxA landed).  This required changing a couple of the tests as I described above.

It has a couple of strictly unrelated changes that seem worthwhile, but not significant enough for a new bug:

* Logging fix:
        this._log.error("Background fetch for key bundle failed: " + err.message);
would sometime log 'undefined' for err.message - now uses (err.message || err)

* The BrowserIDClusterManager's _findCluster() now has a sanity check that we really are authenticated.  In addition, it attempts to use the token we already have for the user.  If it can't use the token it fetches a new one, and replaces the identity's token with the newly fetched on.  This should avoid a number of redundant round-trips to the token server.

* Promise error handlers: ensures all the .then(null, err ...) calls are at the very end of the promise chain.

All tests pass and manual testing looks good, so I think this is ready - but I won't push my luck by asking for review before I've done the feedback dance :)

Comment 3

[Chris Karlof [:ckarlof]]

Comment on attachment 8377896 [details] [diff] [review]
0001-Bug-973749-Have-bid_identity-drop-more-credentials-a.patch

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

How much would this be addressed by the proposed fix for bug 967015 in https://bugzilla.mozilla.org/show_bug.cgi?id=967015#c5 Maybe entirely?

Comment 4

[Chris Karlof [:ckarlof]]

Comment on attachment 8377896 [details] [diff] [review]
0001-Bug-973749-Have-bid_identity-drop-more-credentials-a.patch

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

f+ but I think a lot of this is covered by the proposed fix here: https://bugzilla.mozilla.org/show_bug.cgi?id=967015#c5

Comment 5

[Richard Newman [:rnewman]]

You'd need to make sure that startOver clears everything in question…

Comment 6

[Chris Karlof [:ckarlof]]

(In reply to Richard Newman [:rnewman] from comment #5)
> You'd need to make sure that startOver clears everything in question…

I agree.

Comment 7

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

This bug is subsumed by bug 967015, so marking as a dependency pending feedback/review on that bug.

Comment 8

[Chris Karlof [:ckarlof]]

Fixed in Bug 967015.