Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

WorkManagerSyncWorker requests a new OAuth token every sync #3357

Closed
rfk opened this issue Jun 11, 2019 · 3 comments
Closed

WorkManagerSyncWorker requests a new OAuth token every sync #3357

rfk opened this issue Jun 11, 2019 · 3 comments
Assignees
Labels
<sync> Component: sync-logins

Comments

@rfk
Copy link
Contributor

rfk commented Jun 11, 2019

From slack discussion with @grigoryk.

The WorkManagerSyncWorker class deserializes a fresh FirefoxAccount object out of preferences for every sync, as seen here.

This works fine from a functional perspective, but it defeats some important internal caching done by the FirefoxAccount object. The result is that every time Fenix syncs, it has to:

  1. Request a fresh OAuth access_token from the Firefox Accounts servers.
  2. Exchange that for a sync storage node token by passing it to the sync tokenserver.
  3. Use the storage node token to actually sync its data

The tokens from steps (1) and (2) live for several hours and should be cached to amortize the server load cost across multiple syncs. The app-services FirefoxAccount object caches the token from (1) for this purpose, but only in an in-memory cache on the object. Ideally the WorkManagerSyncWorker would be able to use the app's existing FirefoxAccount instance rather than creating a fresh one for each sync. (It may be that that's not feasible, and we have to think of some other approach, but I think it's the best place to start).

I'm concerned about the server load impact on FxA if the Fenix userbase grows too large with the current behaviour of fetching a fresh token on every sync.

┆Issue is synchronized with this Jira Task

@pocmo pocmo added the <sync> Component: sync-logins label Jun 11, 2019
@rfk
Copy link
Contributor Author

rfk commented Jun 11, 2019

Linking mozilla/application-services#1274 for completeness. We did an experiment today with trying to have WorkManagerSyncWorker persist the cache of access tokens back into the stored account data, but it didn't work out very well - we couldn't figure out a way to do it that would not risk racing with other writes to the pref store and potentially undoing important account actions (such as signing out).

@grigoryk
Copy link
Contributor

Talked this over with @rfk earlier today. A fairly straightforward path forward here is to explicitly manage access tokens at the a-c side, likely storing them in SharedPrefs (as opposed to in an in-memory instance of FirefoxAccount).
This will let us simply the sync worker (it will no longer need re-hydrate an account instance, just access to the token cache). Account manager will be responsible for making sure the a-c token storage layer has an up-to-date token(s) for the necessary scopes.

@grigoryk grigoryk self-assigned this Jun 11, 2019
@grigoryk
Copy link
Contributor

We've done this a while back. #3579

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
<sync> Component: sync-logins
Projects
None yet
Development

No branches or pull requests

3 participants