From a discussion on IRC

premature send :) From a discussion on IRC, our oauth tokens live forever and it is a little bad if we re-request these tokens each startup. In an ideal world we'd also revoke all such tokens when the user signs out. Therefore, we feel that FxAccounts should manage the storage of these tokens. We'd store all oauth tokens keyed by a "client service id" (eg, readinglist, loop) and the scope (so a single service can request tokens with different scopes). When the service requests a token, FxA will be free to return a previously cached one. On logout we'd make a "best effort" attempt to revoke them all. We will store the tokens in signedInUser.json - there's no need to store them in the "secure" (hah!) login manager. We'd store them as something like: > "oauthTokens": { > "readinglist": { > "scope1": { > "access_token": "the actual token value", > "server": "https://something", // The oauth server we got the token from, which is the server we'll use to revoke it > "expires": 12345, // later when we get token expiry, this is where we will stash that. null == never expires. > } > } >}

This patch converts a couple of functions to use tasks rather than promises, but doesn't change any semantics. It's really only done to make part2 easier.

This patch attempts to use a cached token, and stores tokens in the cache when it can't. There's no attempt yet to revoke the tokens on logout (mainly as I have no idea how to do that), and I think a little more testing would be nice, but this is the general idea.

Oh, and we will also need some way of explicitly discarding cached values, so a client seeing a 401 can request a new token and not get the cached one. This could either be done with a new entry in the options param, or with a new method specifically for that purpose. Zaaaaaach, what do you think?

This one is getting quite a big bigger. It has code to destroy a previously fetched token plus basic support for destroying them on logout. It also has the start of a test that stands-up a local http server for serving tokens :) Still has nothing around comment 4 though :(

Comment on attachment 8573022 [details] [diff] [review] 0001-Bug-1139743-part-1-Use-Task.jsm-for-fetching-OAuth-t.patch Review of attachment 8573022 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +936,3 @@ > log.debug("getOAuthToken enter"); > > if (!options.scope) { As you brought up Mark, we should consider moving to an array of scopes here. Eventually that needs to be turned into a string of space separated scopes, but there's no good reason to expose that implementation detail in this API.

Comment on attachment 8573089 [details] [diff] [review] 0002-Bug-1139743-part-2-Cache-OAuth-tokens.-r-zaach.patch Review of attachment 8573089 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +942,5 @@ > * > * @param options > * { > * scope: (string) the oauth scope being requested > + * service: (string, optional) the name of the client service Thinking about this more, I'm not sure about introducing this "service" string. What should it be? A URL? A name? I'm currently chewing on the idea of just indexing them on some canonicalization of the requested scopes, e.g., sort, lower case, and join(" "). What do you think Mark and Zach? Simpler, I think.

(In reply to Mark Hammond [:markh] from comment #4) > Oh, and we will also need some way of explicitly discarding cached values, > so a client seeing a 401 can request a new token and not get the cached one. > This could either be done with a new entry in the options param, or with a > new method specifically for that purpose. Zaaaaaach, what do you think? https://developer.chrome.com/apps/identity#method-removeCachedAuthToken Something like that should work, although it's little ugly.

(In reply to Chris Karlof [:ckarlof] from comment #7) > Thinking about this more, I'm not sure about introducing this "service" > string. What should it be? A URL? A name? I was thinking anything at all, so long as it was consistently used. IOW, I was thinking "readinglist", "loop" etc. > I'm currently chewing on the idea of just indexing them on some > canonicalization of the requested scopes, e.g., sort, lower case, and join(" > "). What do you think Mark and Zach? Simpler, I think. I'm fine with that, although I guess we will want the server as part of that key (which isn't a problem as we also need that server URL to be kept so we can revoke it)

> I'm fine with that, although I guess we will want the server as part of that key (which isn't a problem as we also need that server URL to be kept so we can revoke it) We could, but I think we can just use the scope as the key. Worse case if the environment changes, the service will get a 401, which will trigger the cache eviction and refetching anyway.

This version: * Allows getOAuthToken()'s 'scope' option to be an array or string, and passes them as a space-sep'd string to FxAccountsOAuthGrantClient. * Uses a sorted, lower-cased, joined by '|' scope string as the key for the token cache. * Adds removeCachedOAuthToken() - like getOAuthToken, it takes an "options" param that expects a single item, 'token'. As suggested by Ryan, we also attempt to destroy the token on the server, but we do this in the background (and thus don't fail it it fails, which seems likely) * moar tests.

Comment on attachment 8573700 [details] [diff] [review] 0002-Bug-1139743-part-2-Cache-OAuth-tokens.-r-zaach.patch Review of attachment 8573700 [details] [diff] [review]: ----------------------------------------------------------------- Looks good Mark. My biggest concerns are around the potential race conditions. If we view the cached tokens as "best effort", meaning that callers shouldn't rely on us caching tokens for them, then it's more tolerable I suppose. ::: services/fxaccounts/FxAccounts.jsm @@ +975,3 @@ > } > + // This is the string we pass to the server for the scope. > + let scopeString = scope.join(" "); // XXX - correct? yes. @@ +1013,5 @@ > + let result = yield client.getTokenFromAssertion(assertion, scopeString); > + let token = result.access_token; > + // If we have a service we can cache it. > + if (token) { > + let data = yield currentState.getUserAccountData(); There's likely a race condition in here if two callers are requesting an OAuth token at the same time and the second writer overwrites the cached token written by the first writer. @@ +1056,5 @@ > + return; > + } > + for (let [key, tokenValue] in Iterator(data.oauthTokens)) { > + if (tokenValue.access_token == options.token) { > + delete data.oauthTokens[key]; Doesn't the data need to be re-written to storage after the token is removed? This also seems like a place for a race condition. ::: services/fxaccounts/tests/xpcshell/test_accounts.js @@ +741,5 @@ > + let result = yield fxa.getOAuthToken({ scope: "profile", client: client, service: "test-service" }); > + do_check_eq(numTokenFromAssertionCalls, 1); > + do_check_eq(result, "token"); > + > + // requesting it again should not re-fetch the assertion. This comment should say "should not re-fetch the token". ::: services/fxaccounts/tests/xpcshell/test_oauth_tokens.js @@ +134,5 @@ > + equal(numTokenFetches, 1); > + equal(activeTokens.size, 1); > + ok(token1, "got a token"); > + > + // re-request the same token - it should not hit the server and should This test seems to be about revoking. Don't we have coverage for caching elsewhere? I just prefer tests to have focused concerns. :) @@ +148,5 @@ > + // explicitly drop the new token from our cache. > + yield fxa.removeCachedOAuthToken({token: token2}); > + > + // FxA fires an observer when the "background" revoke is complete. > + yield promiseNotification("testhelper-fxa-revoke-complete"); Do we need to check more? After this notification fires, how about checking that we have no active tokens and then when we make a make a subsequent fetch, it reaches back out to the server? @@ +219,5 @@ > + > + // Check the token cache was written to signedInUser.json. > + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); > + let data = yield CommonUtils.readJSON(path); > + ok(data.accountData.oauthTokens, "the data is in the json"); This could probably use a little more validation. @@ +224,5 @@ > + // And that it's not in the login manager data. > + let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); > + Assert.equal(logins.length, 1, "only 1 login available"); > + let storedData = JSON.parse(logins[0].password); > + ok(!storedData.accountData.oauthTokens, "and not in the login manager") The test coverage concerning caching around l. 138 seems more at home down here.

So, races... I believe Chris's main concern in the previous patch is that the setting of each token causes a read/write cycle on the login data, and thus, if a second token fetch starts before the previous read/write cycle is complete the data will be lost from the json. The way the code works, this is 1/2 true, and other potential races do exist. So https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccounts.jsm#99 is setup such that only 1 such read ever happens - calls to getUserAccountData() will cache the previous value and return that without hitting disk. Further, setUserAccountData saves the user data back into the cached value as well as saving it to disk - the end result being that so long as the first read is complete, that's the only read that happens, so we should not be vulnerable to multiple read/write cycles (although setUserAccountData is a little problematic - see below) However, there are still some subtleties in the code that makes it potentially incorrect: * getUserAccountData doesn't protect against this "initial read" happening multiple times - IOW, if getUserAccountData() is called a second time before the first is complete, we will attempt to read again. * setUserAccountData *does* do a read, *then* updates the cached value, then issues a write. Thus, it's possible that multiple setUserAccountData calls will lose the data from one of them while we wait for the first read to complete. Note also that while the OAuth cache seems more likely to hit this problem, the problem exists independently of the token cache - there is a risk that any attribute sets will fall foul of this. This patch is an attempt to harden against these problems (and is independent of the cache): * getUserAccountData stores a promise for the initial read. This means that multiple getUserAccountData calls in-flight before the first is finished all use the same promise. Once this initial promise is complete we will return the in-memory value as before. * setUserAccountData now *immediately* stores the new accountData before starting a write. It no longer reads at all (which it did only to get the "version" field). It also throws if the initialRead promise is in-flight (as it seems insane that someone would want to write before it's been read) I think this solves the problem in practice due to us not cloning the data - so code such as: let data = yield getUserAccountData(...); data.foo = "bar" yield setUserAccountData(data); is actually using the same 'data' object in all cases, as the first call is almost certainly returning the object cached in the accountState - so even if the code above executes multiple times concurrently, the same "data" object is used in both cases. However, this is still non-obvious - a better answer might be to make that explicit (ie, not have setUserAccountData take the data to write, but instead use the in-memory object). For hysterical raisins this is not trivial and probably beyond the scope of this bug. We should consider another bug to make a better (but far more intrusive) scheme here. Chris, I'm wondering what you think of this patch? It should not change any semantics (the tests all pass) but should harden all the code against such races. Note that all other patches for this bug *also* still work without this patch - it's basically guarding against potential races that we haven't managed to see in practice yet. IOW, this patch is optional, but without it there are risks the token cache might somehow hit these races, and it seems quite difficult for the token cache code to itself guard against the races as they are implicit in the accountState object.

This patch addresses all previous review comments, and also handles multiple concurrent requests for the same token scope - which it handles by immediately revoking one of the tokens and returning a previously cached one (ie, we also check *after* we've got a token if someone managed to cache one while we were fetching it, and if it exists we revoke the new one and return that cached one). This also has explicit tests. It doesn't make any attempt to deal with the inherent racey behaviour of the storage - see previous patch.

Comment on attachment 8575698 [details] [diff] [review] 0002-remove-a-few-potential-race-conditions.patch Review of attachment 8575698 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +147,5 @@ > + // Set our signedInUser before we start the write, so any updates to the > + // data while the write completes are still captured. > + this.signedInUser = {version: DATA_FORMAT_VERSION, accountData: accountData}; > + return this.fxaInternal.signedInUserStorage.set(this.signedInUser) > + .then(() => this.resolve(this.signedInUser)); FWIW, this line is wrong - it needs to .resolve(accountData)

Comment on attachment 8575698 [details] [diff] [review] 0002-remove-a-few-potential-race-conditions.patch Review of attachment 8575698 [details] [diff] [review]: ----------------------------------------------------------------- The "not cloning" part is a little scary not that you point it out. :) Do you think it's worth cloning the data in both get and set to be defensive about whatever the callers decide to do with the parameter/return value subsequently? ::: services/fxaccounts/FxAccounts.jsm @@ +142,5 @@ > + return this.reject(new Error("Another user has signed in")); > + } > + if (this.promiseInitialAccountData) { > + throw new Error("Can't set account data before it's been read."); > + } I think this comment is more along the lines of "while it's being initially read" vs. "before it's read". This code path seems to allow setting before reading. Does this create a new throw condition that didn't exist before? It seems unlikely this would ever get triggered, though.

I meant to say 'The not "cloning" part is a little scary now that you point it out.'

Mark I have to catch a bus, but I'll try to review the 2nd patch when I get home tonight.

(In reply to Chris Karlof [:ckarlof] from comment #16) > The "not cloning" part is a little scary not that you point it out. :) Do > you think it's worth cloning the data in both get and set to be defensive > about whatever the callers decide to do with the parameter/return value > subsequently? From an engineering POV, that's certainly the safest thing to do. However, given how the code is implemented, I don't think we want getUserAccountData() to clone on each call or we end up in the race situation we were talking about - ie: promise1: getUserAccountData().then(data => data.foo = "bar"); promise2: getUserAccountData().then(data => data.bar = "foo"); could do the wrong thing, but currently works correctly. But yeah, this is all scary and I think it's worth taking a step back and reevaluating the model we expose here. > Does this create a new throw condition that didn't exist before? It seems > unlikely this would ever get triggered, though. It does - it's defensive rather than addressing a real problem so I've no problem removing it (but OTOH, this entire patch is simply defensive, and I can't provoke a test/runtime failure without it being applied at all). Note however that browserid_identity already has defense against a different user signing in, so it would be reasonable to decide to take this patch without that additional check - but I'm inclined to say we might as well take this too.

> promise1: getUserAccountData().then(data => data.foo = "bar"); > promise2: getUserAccountData().then(data => data.bar = "foo"); Right. I'm happy with the first patch.

Comment on attachment 8575703 [details] [diff] [review] 0004-Bug-1139743-part-2-Cache-OAuth-tokens.-r-zaach.patch Review of attachment 8575703 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +978,5 @@ > */ > getOAuthToken: Task.async(function* (options = {}) { > log.debug("getOAuthToken enter"); > + let scope = options.scope; > + if (typeof scope === "string") { I didn't see a test for when scope is an array. Did I miss it? @@ +1012,3 @@ > yield this._getVerifiedAccountOrReject(); > + // See if we already have one > + let data = yield this.currentAccountState.getUserAccountData(); After pondering all those race issues, it seems that we could improve matters a bit by adding methods on the account state object to manage the cached OAuth tokens. That would have a couple of benefits: * reduce the size of this function * avoid manipulating the actual data representation directly, e.g., data.oauthTokens[scopeKey].access_token looks :-( I'm thinking we should add: hasCachedToken(scope), getCachedToken(scope), setCachedToken(scope, tokenData), deleteCachedToken(scope). We could also consider adding a method on state to persist the state explicitly to disk, so all these could be synchronous in-memory operations. ::: services/fxaccounts/tests/xpcshell/test_oauth_tokens.js @@ +229,5 @@ > + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); > + let data = yield CommonUtils.readJSON(path); > + ok(data.accountData.oauthTokens, "the data is in the json"); > + > + // Check it's all in the json. Thanks for the test improvements!

Comment on attachment 8573022 [details] [diff] [review] 0001-Bug-1139743-part-1-Use-Task.jsm-for-fetching-OAuth-t.patch So this one seems non-controversial...

Comment on attachment 8575698 [details] [diff] [review] 0002-remove-a-few-potential-race-conditions.patch And I think everyone's happy with this (although I didn't address: > I think this comment is more along the lines of "while it's being initially > read" vs. "before it's read". This code path seems to allow setting before > reading. yet - it's late here, but if you can be specific I'll change it :)

(In reply to Chris Karlof [:ckarlof] from comment #21) > I didn't see a test for when scope is an array. Did I miss it? A nice catch well caught - it was broken :) > I'm thinking we should add: hasCachedToken(scope), getCachedToken(scope), > setCachedToken(scope, tokenData), deleteCachedToken(scope). Agreed - I think this captures what you are suggesting?

Mark, Zach is on PTO in Japan until the end of next week, so I'll probably need to review that first patch as well.

Comment on attachment 8575698 [details] [diff] [review] 0002-remove-a-few-potential-race-conditions.patch Review of attachment 8575698 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +147,5 @@ > + // Set our signedInUser before we start the write, so any updates to the > + // data while the write completes are still captured. > + this.signedInUser = {version: DATA_FORMAT_VERSION, accountData: accountData}; > + return this.fxaInternal.signedInUserStorage.set(this.signedInUser) > + .then(() => this.resolve(this.signedInUser)); I just r+'ed, but did this get fixed?

Comment on attachment 8576595 [details] [diff] [review] 0005-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch Review of attachment 8576595 [details] [diff] [review]: ----------------------------------------------------------------- I like the refactor! I had couple of questions about the tests. ::: services/fxaccounts/FxAccounts.jsm @@ +260,5 @@ > + > + // Set a cached token. |options| must have a 'token' element, but may also > + // have additional fields (eg, it probably specifies the server to revoke > + // from). The 'get' functions below return the entire |options| value. > + setCachedToken(scope, options) { Given that that scope is actually an array here, maybe scopes or scopesArray would be a better param name for each of these. I also am a little way of using "options" here. It isn't really optional. :) tokenData maybe? @@ +261,5 @@ > + // Set a cached token. |options| must have a 'token' element, but may also > + // have additional fields (eg, it probably specifies the server to revoke > + // from). The 'get' functions below return the entire |options| value. > + setCachedToken(scope, options) { > + this._cachePreamble(options); _cachePreamble doesn't take options. @@ +290,5 @@ > + } > + return null; > + }, > + > + // Get an array all data for cached tokens. This sentence doesn't parse for me. ::: services/fxaccounts/tests/xpcshell/test_oauth_tokens.js @@ +206,5 @@ > + }); > + } > +}); > + > +add_task(function testCacheStorage() { Are there any tests for the new methods on AccountState? These tests could be refactored into two sets of tests 1) Tests on the new methods on AccountState to make sure they properly write stuff out to disk. 2) (A refactor of the below) tests on fxa.getOAuthToken to make sure it properly invokes the new methods on AccountState. With the refactor, I feel that the below test is now more of a single end-to-end integration test from getOAuthToken all the way to the JSON file on disk. That has value, but I find that unit tests that focus on local input and outputs are less brittle and easier to maintain. What are your thoughts?

oh dear - it seems the existing code that attempts to reuse the cached data is broken: https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccounts.jsm#112 this.version is undefined there. If I fix that, the tests around the login manager being locked fail - things assume we re-read when we only have partial data, so we get the missing fields when the MP is unlocked. Not sure what to do about that :( Chris, any thoughts?

If I understand this correctly, the fact that we never set this.signedInUser was masking a bug around how master password interacts with this.signedInUser. The result is that we read the signed in user from disk every time. For master password users, this may mean that they need to re-enter their master password on every Sync, right? (Looking through the LoginManagerStorage code, there are some code paths there, particularly around this version field that results in the user's LoginManager Sync creds being blown away. I wonder if one of these is causing those bugs where people report their keys missing from the login manager?) > Not sure what to do about that :( Chris, any thoughts? This does look delicate. The least risky thing perhaps is: 1) Fix the this.version bug 2) Only cache the user data after that if it's complete, meaning the MP is unlocked. In 2), we'll need to be careful of what happens if the MP is unlocked and someone requests an OAuth token. We probably want to try to avoid that. :) If you plan on working on this before you arrive in SF, let me know and we can discuss. Otherwise, we can tackle this on Monday.

(In reply to Chris Karlof [:ckarlof] from comment #29) > If I understand this correctly, the fact that we never set this.signedInUser > was masking a bug around how master password interacts with > this.signedInUser. The result is that we read the signed in user from disk > every time. For master password users, this may mean that they need to > re-enter their master password on every Sync, right? I don't think that's quite correct - once the master-password is unlocked, it stays unlocked, so each getSignedInUserData() after the unlock returns everything. The subtlety is that the code *looks* like it is caching the logged in user data, but really isn't. This is why it works correctly once MP is unlocked - that cached data isn't used.Each getSignedInUserData hits the storage, so before MP unlock it returns partial data, but after it returns full data as that cache isn't actually used. > (Looking through the LoginManagerStorage code, there are some code paths > there, particularly around this version field that results in the user's > LoginManager Sync creds being blown away. I wonder if one of these is > causing those bugs where people report their keys missing from the login > manager?) Yeah, another subtlety is that setSignedInUserData *did* cache the data. So if we ended up with setSignedInUserData called while the MP was locked we then would have cached the partial data and getSignedInUserData would no longer read storage - so even after the MP was unlocked we'd be stuck with partial data. It certainly is possible that this explains those bugs. Another potential erroneous flow might be: * getSignedInUserData called with MP locked - partial data returned. * user unlocks MP for reasons external to this (Eg, to use a saved password or something) * We call setSignedInUserData, and because we only have the partial data in memory we overwrite the data that did exist in the login manager that we haven't yet read this session. * End result is login manager data is lost. I'm not sure how we could actually do that, but it's a possibility. > 1) Fix the this.version bug > 2) Only cache the user data after that if it's complete, meaning the MP is > unlocked. It would be easy to check if the MP is locked. Checking for "partial data" sounds a little more scary as it's not clear exactly what "partial" means. > In 2), we'll need to be careful of what happens if the MP is unlocked and > someone requests an OAuth token. We probably want to try to avoid that. :) Yeah. > If you plan on working on this before you arrive in SF, let me know and we > can discuss. Otherwise, we can tackle this on Monday. I might have more of a play on the plane...

Mark, Track me down in SF and we'll drive this to conclusion ASAP.

For those watching at home, this bug currently isn't blocking anything. RL already has the capability to get an OAuth token. This bug is about caching and revocation, which something we'll want to address before release, but isn't a high priority right now. Mark and I plan to discuss this in person while he's in SF this week.

Re-uploading just as this commit message now has r=ckarlof instead of r=zaach

Current version of part 2 with nits fixed, r=ckarlof

This version now stores OAuth tokens in a separate .json file in the profile directory. All tests pass, although I haven't self-reviewed it that closely yet, but getting it up for early feedback - Chris, let's go over this tomorrow if we can find time.

Yep, let's meet up tomorrow.

Comment on attachment 8579754 [details] [diff] [review] 0003-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch Review of attachment 8579754 [details] [diff] [review]: ----------------------------------------------------------------- Looks good Mark! ::: services/fxaccounts/FxAccounts.jsm @@ +112,5 @@ > return this.resolve(this.signedInUser.accountData); > } > > + // We try and fetch the tokens first. > + return this.promiseInitialAccountData = this.fxaInternal.signedInUserStorage.getOAuthTokens().then( Swap these to read the signedInUser first, then the cachedTokens, and match the uids in each. @@ +113,5 @@ > } > > + // We try and fetch the tokens first. > + return this.promiseInitialAccountData = this.fxaInternal.signedInUserStorage.getOAuthTokens().then( > + record => { Do the uid check here, and the "save" code needs to write the uid, too. @@ +335,5 @@ > + // except tests. > + _persistCachedTokens() { > + this._cachePreamble(); > + let record = {version: this.fxaInternal.version, tokens: this.oauthTokens}; > + return this.fxaInternal.signedInUserStorage.setOAuthTokens(record).catch( As per our discussion: 1) persistCachedTokens should perhaps be a public method 2) longer term, we probably also want a persist method for the basic account state (or we have one method that persists both?) 3) the signedInUserStorage should perhaps be passed in the constructor 4) the AccountState object should not be dependent on fxaInternal Not clear addressing all of these is part of this patch though. @@ +586,5 @@ > let currentState = this.currentAccountState; > // Cache a clone of the credentials object. > currentState.signedInUser = JSON.parse(JSON.stringify(record)); > + // The new user can't have any OAuth tokens yet. > + currentState.oauthTokens = {}; Maybe clean this leaky abstraction a bit, e.g., something like this.currentAccountState = new AccountState(record). @@ +591,5 @@ > > // This promise waits for storage, but not for verification. > // We're telling the caller that this is durable now. > return this.signedInUserStorage.set(record).then(() => { > + return this.signedInUserStorage.setOAuthTokens(null); Not sure this is part of this patch, but it would good to think more about what object is responsible for coordinating the persistence of user data: is the AccountState object or is it this object (FxAccountsInternal). Currently, it seems that the concerns are mixed between the two. @@ +728,5 @@ > this.abortExistingFlow(); > this.currentAccountState.signedInUser = null; // clear in-memory cache > + this.currentAccountState.oauthTokens = null; // clear in-memory tokens too > + return this.signedInUserStorage.set(null).then(() => { > + return this.signedInUserStorage.setOAuthTokens(null); 1) Swap the order of set(null), and setOAuthTokens(null) 2) Change promise chain to handle failure of first "set(null)" to continue to log error and the continue with the second "set(null)". ::: services/fxaccounts/FxAccountsCommon.js @@ +65,5 @@ > exports.FXACCOUNTS_PERMISSION = "firefox-accounts"; > > exports.DATA_FORMAT_VERSION = 1; > exports.DEFAULT_STORAGE_FILENAME = "signedInUser.json"; > +exports.DEFAULT_OAUTH_TOKENS_FILENAME = "signedInUserOAuth.json"; OAuthTokens? @@ +215,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", and the > +// same logic applies for oauthTokens. This comment should probably be moved to the persist method in the AccountState to explain why we're just writing it to a JSON file instead of the PW manager. @@ +220,3 @@ > exports.FXA_PWDMGR_PLAINTEXT_FIELDS = ["email", "verified", "authAt", > + "sessionToken", "uid", > + "oauthTokens"]; Stale, since we're no longer writing oauthTokens to signedInUser.json. ::: services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js @@ +87,5 @@ > + // wait for background write to complete. > + yield promiseWritten; > + > + // Check the token cache was written to signedInUserOAuth.json. > + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUserOAuth.json"); Get the filename from FxACommon? @@ +89,5 @@ > + > + // Check the token cache was written to signedInUserOAuth.json. > + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUserOAuth.json"); > + let data = yield CommonUtils.readJSON(path); > + ok(data.tokens, "the data is in the json"); 1) Rather than checking the end results on disk, we should refactor tests here to just make sure that the appropriate methods were called on the Storage object. 2) If we do this, then the Storage object needs its own suite of tests to make sure it properly reads/writes from/to disk. ::: services/fxaccounts/tests/xpcshell/test_oauth_tokens.js @@ +118,5 @@ > + let server = startServer(); > + try { > + let fxa = yield createMockFxA(); > + > + let client = new FxAccountsOAuthGrantClient({ Similar to the comment in the storage tests, we could refactor this to use a mocked FxAccountsOAuthGrantClient instead of mocking the server it interacts with.

This is on-top of the previous patches. It addresses all your feedback comments apart from the test refactoring (which I'll do, but might as well get early feedback). > Swap these to read the signedInUser first, then the cachedTokens, and match > the uids in each. Done, and the uid checks are done and tested. > As per our discussion: > > 1) persistCachedTokens should perhaps be a public method > 2) longer term, we probably also want a persist method for the basic account > state (or we have one method that persists both?) _persistCachedTokens remains public, but there's a new persistUserData() function that writes both. As we discussed, the AccountState() object is now the only thing that references the storage object. (Sadly, I had to keep the storage object somewhat public as tests try and use it, and having it directly on the AccountState object defeated some of the tests due to how frequently we throw AccountState objects away. > 3) the signedInUserStorage should perhaps be passed in the constructor Done. > 4) the AccountState object should not be dependent on fxaInternal Much closer to done - we do reference .version and still reference it in isCurrent, but it's much cleaner now. The FxAccounts object has no storage, so we don't reach in for that. I think we can fix those 2 abstraction-leakages in another refactor later. > Maybe clean this leaky abstraction a bit, e.g., something like > this.currentAccountState = new AccountState(record). Done - it's now new AccountState(this, record); > Not sure this is part of this patch, but it would good to think more about > what object is responsible for coordinating the persistence of user data: is > the AccountState object or is it this object (FxAccountsInternal). > Currently, it seems that the concerns are mixed between the two. Indeed - it's now exclusively the AccountState. > 1) Swap the order of set(null), and setOAuthTokens(null) > 2) Change promise chain to handle failure of first "set(null)" to continue > to log error and the continue with the second "set(null)". Done. > > +exports.DEFAULT_OAUTH_TOKENS_FILENAME = "signedInUserOAuth.json"; > > OAuthTokens? Done. > > +// See bug 1013064 comments 23-25 for why the sessionToken is "safe", and the > > +// same logic applies for oauthTokens. > > This comment should probably be moved to the persist method in the > AccountState to explain why we're just writing it to a JSON file instead of > the PW manager. Done. > Stale, since we're no longer writing oauthTokens to signedInUser.json. Done. > Get the filename from FxACommon? Done. > 1) Rather than checking the end results on disk, we should refactor tests > here to just make sure that the appropriate methods were called on the > Storage object. > 2) If we do this, then the Storage object needs its own suite of tests to > make sure it properly reads/writes from/to disk. I'll do the tests next.

(In reply to Chris Karlof [:ckarlof] from comment #37) > > + > > + // Check the token cache was written to signedInUserOAuth.json. > > + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUserOAuth.json"); > > + let data = yield CommonUtils.readJSON(path); > > + ok(data.tokens, "the data is in the json"); > > 1) Rather than checking the end results on disk, we should refactor tests > here to just make sure that the appropriate methods were called on the > Storage object. > 2) If we do this, then the Storage object needs its own suite of tests to > make sure it properly reads/writes from/to disk. I didn't end up doing this as I'm running out of time, and I think the test covers what we need, just in a sub-optimal way - I hope you're OK with that for now. > Similar to the comment in the storage tests, we could refactor this to use a > mocked FxAccountsOAuthGrantClient instead of mocking the server it interacts > with. Did that one - new test for the grant client that hits a real server, and the token cache test uses a mock. This is the entire patch rolled up.

This new test file was missing from the patch. I've fixed the commit locally but attaching it here for reference.

Comment on attachment 8584186 [details] test_oauth_grant_client_server.js *sigh* - my brain is struggling today...

Hi Mark, I was on the phone all day today, and I'll make it a priority to get to this tomorrow.

Comment on attachment 8583529 [details] [diff] [review] 0005-fixup-ckarlof-review-comments-sans-tests.patch Review of attachment 8583529 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +157,5 @@ > + }).then(tokenData => { > + if (tokenData && tokenData.tokens && > + tokenData.version == this.fxaInternal.version && > + tokenData.uid == accountData.uid ) { > + oauthTokens = tokenData.tokens; If I understand this correctly, if somehow the cached token file and signedInUser file get out of sync wrt uid, then we just ignore the cached tokens and not fail, correct? @@ +362,5 @@ > this._cachePreamble(); > + let record; > + if (this.signedInUser) { > + record = { > + version: this.fxaInternal.version, version seems like something we can pass in to the AccountState constructor. Or, why is this different than DATA_FORMAT_VERSION? @@ +369,5 @@ > + }; > + } else { > + record = null; > + } > + return signedInUserStorage.setOAuthTokens(record).catch( Feels funny signedInUserStorage is like a global here.

Comment on attachment 8583590 [details] [diff] [review] 0004-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch Review of attachment 8583590 [details] [diff] [review]: ----------------------------------------------------------------- Looks good so far, Mark. I'll continue to take a look over the weekend. ::: services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js @@ +16,5 @@ > Cu.import("resource://services-common/utils.js"); > Cu.import("resource://gre/modules/FxAccountsCommon.js"); > > +// A "back-stage pass" allows us to get the private storage object. > +let {signedInUserStorage} = Cu.import("resource://gre/modules/FxAccounts.jsm", {}); This all still seems less than ideal, but no worse than what we had before. :)

This part 1 had some issues with error throwing/rejections. I was confused, so opened bug 1148301, which is invalid - the only reason it was necessary was due to the errors in this patch. I'll close that as invalid, but Zaach already r+'d the missing bits in that bug, so I'm going to carry the r+ forward here - this patch is now a combination of the earlier version of this patch plus the patch in bug 1148301.

Part 2, unchanged but rebased.

Reflecting on the old patch, I think I went a little too far :) This patch is different from the previous one in the following ways: * signedInUserStorage is no longer global. Instead it is still created by the FxAccounts object, but it only uses it to pass to AccountState objects as they are created. Thus, FxAccountsInternal creates, but does not use, the storage object. AccountState is still responsible for all storage operations. This means the tests did not need as many hacks and we no longer need a backstage-pass to test the storage. Even though the FxAccountsInternal creates storage but doesn't directly use it isn't ideal, I think it's a better pragmatic solution at this point and helps make this patch easier to reason about. There's also a patch to browser/components/readinglist/ServerClient.jsm, which guarded against removeCachedOAuthToken() not existing. As this patch lands it the guard has been removed. (In reply to Chris Karlof [:ckarlof] from comment #43) > Comment on attachment 8583529 [details] [diff] [review] > 0005-fixup-ckarlof-review-comments-sans-tests.patch > > If I understand this correctly, if somehow the cached token file and > signedInUser file get out of sync wrt uid, then we just ignore the cached > tokens and not fail, correct? Correct. > version seems like something we can pass in to the AccountState constructor. > Or, why is this different than DATA_FORMAT_VERSION? Yeah, I now just use DATA_FORMAT_VERSION. > @@ +369,5 @@ > > + }; > > + } else { > > + record = null; > > + } > > + return signedInUserStorage.setOAuthTokens(record).catch( > > Feels funny signedInUserStorage is like a global here. Yeah - see comments above - it is no longer a global. (In reply to Chris Karlof [:ckarlof] from comment #44) > Comment on attachment 8583590 [details] [diff] [review] > 0004-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch > > Review of attachment 8583590 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good so far, Mark. I'll continue to take a look over the weekend. > > ::: services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js > @@ +16,5 @@ > > Cu.import("resource://services-common/utils.js"); > > Cu.import("resource://gre/modules/FxAccountsCommon.js"); > > > > +// A "back-stage pass" allows us to get the private storage object. > > +let {signedInUserStorage} = Cu.import("resource://gre/modules/FxAccounts.jsm", {}); > > This all still seems less than ideal, but no worse than what we had before. Yeah, but this no longer exists either :) I'm requesting review as I think this is pretty good and we are running out of time to sanely land on 38. Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=29541669296b

Comment on attachment 8584935 [details] [diff] [review] 0003-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch Review of attachment 8584935 [details] [diff] [review]: ----------------------------------------------------------------- I'm not going to require it for this patch, but now that we're shifting more concerns on the AccountState object, we should eventually add more unit tests for that object, with a mocked storage object. test_oauth_token_storage.js does test some methods on AccountState, but those tests are written more as an integration test between AccountState and the storage object. In particular, the tests in test_oauth_token_storage.js require us to mock out nearly everything in this module to run those tests. AccountState seems is become a more narrowly concerned object that we could probably unit test just by mocking out it's dependencies (i.e., storage). isCurrent() does depend on the FxA internal object, but maybe we could probable provide a mock internal object that handles that code path. Likewise, some unit tests that tested JSONStorage.setOAuthTokens and JSONStorage.getOAuthTokens would also be valuable. I'm going to r+ pending an answer/solution to my question about whether tokens get cached if this.signedInUser isn't set on startup. ::: services/fxaccounts/FxAccounts.jsm @@ +205,5 @@ > throw new Error("Can't set account data before it's been read."); > } > // Set our signedInUser before we start the write, so any updates to the > // data while the write completes are still captured. > this.signedInUser = {version: DATA_FORMAT_VERSION, accountData: accountData}; Kind of weird this.signedInUser is set in setUserAccountData, but never in getUserAccountData. This seems to be for master password interactions, and as you mention, I think a refactor later would be prudent to make these more consistent. @@ +404,5 @@ > + // set of user data.) > + _persistCachedTokens() { > + this._cachePreamble(); > + let record; > + if (this.signedInUser) { Given this this.signedInUser is never set if we're just reading the user info from disk, I concerned that this means if we have a situation where: 1) User is already logged into FxA. 2) Starts browser. 3) RL requests a token. Then it won't be persisted because this.signedInUser isn't set in this case.

(In reply to Chris Karlof [:ckarlof] from comment #48) > I'm not going to require it for this patch, but now that we're shifting more > concerns on the AccountState object... Agree completely with all these comments, but also agree we need not make that part of this. > Given this this.signedInUser is never set if we're just reading the user > info from disk, I concerned that this means if we have a situation where: > > 1) User is already logged into FxA. > 2) Starts browser. > 3) RL requests a token. > > Then it won't be persisted because this.signedInUser isn't set in this case. Yep, you are correct :( This patch fixes this and adds a new test for the scenario. Now we *do* save the user's UID - I first tried just doing getSignedInUserData() to get the UID, but (a) that's more racey and (b) also reads the token file that we are trying to save to. Also, assuming you like this, how far should we uplift it? I'm thinking we should start at 39 and 40, then request 38 in a week or so?

Comment on attachment 8587140 [details] [diff] [review] Fix, on top of previous patch Review of attachment 8587140 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +197,2 @@ > this.oauthTokens = oauthTokens; > + this.uid = accountData ? accountData.uid : null; I think that setUserAccountData() needs a line similar to this. Otherwise, the uid won't be updated. While looking at this, it also made be question why setUserAccountData takes the user data object, but the AccountState constructor takes a wrapped version of that called a signedInUser which looks like {version: DATA_FORMAT_VERSION, accountData: accountData}. Seems dangerous and a likely source of confusion. I think that the constructor is only called with the third parameter in setSignedInUser. It constructs the signedInUser "record" first, but now that seems something that should be done in AccountState just prior to writing out the record to disk. I know we're getting late here, and we could clean this up later, but I am concerned this mismatch in how AccountState initializes and mutates might be hiding a bug.

Comment on attachment 8587140 [details] [diff] [review] Fix, on top of previous patch Review of attachment 8587140 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +197,2 @@ > this.oauthTokens = oauthTokens; > + this.uid = accountData ? accountData.uid : null; We discussed to throw an exception in setUserAccountData() if the uid in the accountData doesn't match what is currently in the AccountState object. This is because setUserAccountData should only be used to update the data for the current user, not to change it to another user. We also discussed changing the constructor for AccountState so it takes "accountData" rather than a "signedInUser" (i.e., not something like {version: DATA_FORMAT_VERSION, accountData: accountData}).

Comment on attachment 8587140 [details] [diff] [review] Fix, on top of previous patch Review of attachment 8587140 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js @@ +167,5 @@ > + cas.setCachedToken(scopeArray, tokenData); > + > + yield promiseWritten; > + > + yield cas.getUserAccountData(); What do you think about something more direct here, like calling cas.signedInUserStorage.getOAuthTokens() and seeing if the token is in here?

(In reply to Mark Hammond [:markh] from comment #49) > Also, assuming you like this, how far should we uplift it? I'm thinking we > should start at 39 and 40, then request 38 in a week or so? Sounds good to me.

Fixups from our discussion today.

Final part 3 with fixups applied.

Comment on attachment 8587140 [details] [diff] [review] Fix, on top of previous patch Review of attachment 8587140 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with the additional fixup in fixup-2.

Comment on attachment 8587775 [details] [diff] [review] 0003-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch Review of attachment 8587775 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Mark!

\o/ https://hg.mozilla.org/integration/fx-team/rev/d674c20be9f6 https://hg.mozilla.org/integration/fx-team/rev/46415f2b0f35 https://hg.mozilla.org/integration/fx-team/rev/a450bb0b9d62

Comment on attachment 8587775 [details] [diff] [review] 0003-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch Approval request for all 3 patches. Note that we intend requesting approval for beta once it has baked on central and aurora for a week or so. Approval Request Comment [Feature/regressing bug #]: readinglist [User impact if declined]: Reading List sync will attempt to fetch a new token each sync, increasing network usage. Services team will get upset as many new tokens are created and thrown away instead of being reused and revoked. [Describe test coverage new/current, TreeHerder]: Has tests. [Risks and why]: Low risk limited to FxA (ie, in a worst-case scenario it would impact Sync, ReadingList and Loop, but nothing else) [String/UUID change made/needed]: None

https://hg.mozilla.org/mozilla-central/rev/d674c20be9f6 https://hg.mozilla.org/mozilla-central/rev/46415f2b0f35 https://hg.mozilla.org/mozilla-central/rev/a450bb0b9d62

Let's let this stay on Nightly and verify it first before uplift. Stephen can your team verify this bug on Nightly? This looks like a big patch, and possibly affecting everyone using Firefox Accounts seems like a moderately serious risk to me. Why would the services team get upset? Is it a cost or a performance issue or both?

> > Services team will get upset as many new tokens are created and thrown away instead of being reused and revoked. > Why would the services team get upset? Is it a cost or a performance issue or both? Whenever a new token is "created and thrown away", it will sit around forever in the server-side database because we have no way of knowing whether it's safe to revoke it. It's a small but real hit on server storage costs and performance. This patch ensures they're re-used and explicitly revoked, and hence is kinder to sever resources.

Clearing NI? as it is covered by comment 62

Comment on attachment 8587775 [details] [diff] [review] 0003-Bug-1139743-part-3-Cache-OAuth-tokens.-r-ckarlof.patch Approving this for uplift to aurora. We can focus verification effort on aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/bbdecb2586cb https://hg.mozilla.org/releases/mozilla-aurora/rev/e367fafd28d7 https://hg.mozilla.org/releases/mozilla-aurora/rev/5174d1db7188

(In reply to Liz Henry (:lizzard) from comment #61) > Let's let this stay on Nightly and verify it first before uplift. Stephen > can your team verify this bug on Nightly? > > This looks like a big patch, and possibly affecting everyone using Firefox > Accounts seems like a moderately serious risk to me. Sorry for the delay, Liz (was on vacation and neglected to update Bugzilla); Stuart would know better who'd be best to verify this bug (willing to help if need-be).

This sounds like a job for, jrgm

Can we receive some guidance on how to manually verify this bug?

To be honest I don't see how we can sanely verify this manually. The patch landed with good test coverage, so I think this should be fine.