Commit 7afec7d4 authored by Grisha Kruglov's avatar Grisha Kruglov Committed by Grisha Kruglov
Browse files

Move sync scope ownership into account manager; API simplification

I've started pulling on one little thread, and ended up with a few more changes than initially anticipated.

Raison d'être for this PR - introducing access token caching for Sync.
- Some background on the issue: Rust FirefoxAccount object maintains an in-memory cache of access tokens, keyed by 'scope'. During every sync, we "rehydrate" an instance of FirefoxAccount, starting with a fresh cache. We then obtain an access token from it to sync; this performs a network request (since the internal cache is empty), which is quite costly at scale for our services. This creates a situation when we may overwhelm our own servers with a large enough, actively syncing user base.
- This PR adds a caching layer for sync authInfo objects. Sync workers no longer interact with the account directly, and instead look into the cache to obtain authentication info necessary for syncing. No more "talk to the FxA server before every sync".
Account manager is responsible for keeping the cache up-to-date, and resetting it when necessary. Cache is currently updated: on startup (but only if access token has expired), on authentication, and when we recover from auth problems.

And this is where the "thread pulling" begins! In order to "own" the access token for sync, account manager needs to be aware of the "sync scope".
Before, we just relied on the application to specify that scope. Instead, I've changed account manager's constructor to take a SyncConfig object which allows consuming application to configure how sync should behave (enabled at all?, periodic syncing enabled? how often to sync? which stores should be synced?).
Ownership of the "sync manager" moved down the stack, from the application layer into the account manager.

Application is now expected to interact with sync only via AccountManager's `sync` method, which exposes an internal SyncManager instance (if sync is enabled).

Above changes were a good reason to move support classes from feature-sync and into services-firefox-account. Note that since "sync" is part of our "storage" modules, this change doesn't mean that you need to take an extra native dependency on your classpath simply if you need to use FxA. Thanks to concept-sync, actual "Firefox Sync" machinery (within libplaces) is still fully decoupled from FxA. `feature-sync` has been removed entirely.

Since we're churning the public API anyway, I took the chance to introduce a few more simplifications at the API layer:
- 'SyncManager' interface was removed, since we're not expecting to have multiple implementations of it
- 'Config' was renamed to 'ServerConfig'
- 'DeviceTuple' was renamed to 'DeviceConfig'
- account manager grew a new public API, 'setSyncConfig', which allows application to re-configure how it wants sync to behave
- 'AuthInfo' was renamed to 'SyncAuthInfo', and a bunch of cleanup happened in that area
- 'AccountObservable'@'onError' method was removed. The only error that could have been passed into it (unable to restore account) wasn't actionable by the application anyway, and none of the integrations did anything with that call

Documentation of public APIs and classes was improved.
parent 29c14f8d
......@@ -72,10 +72,6 @@ projects:
path: components/feature/session
description: 'Feature implementation connecting an engine implementation with the session module.'
publish: true
feature-sync:
path: components/feature/sync
description: 'Feature implementation that enables syncing of syncable components.'
publish: true
feature-tabs:
path: components/feature/tabs
description: 'Feature implementation connecting a tabs tray implementation with the session and toolbar modules.'
......
......@@ -14,6 +14,7 @@ import mozilla.appservices.sync15.SyncTelemetryPing
import mozilla.components.browser.storage.sync.GleanMetrics.BookmarksSync
import mozilla.components.browser.storage.sync.GleanMetrics.HistorySync
import mozilla.components.browser.storage.sync.GleanMetrics.Pings
import mozilla.components.concept.sync.SyncAuthInfo
import java.io.Closeable
import java.io.File
......@@ -184,7 +185,7 @@ internal object RustPlacesConnection : Connection {
override fun syncHistory(syncInfo: SyncAuthInfo) {
check(api != null) { "must call init first" }
val ping = api!!.syncHistory(syncInfo)
val ping = api!!.syncHistory(syncInfo.into())
assembleHistoryPing(ping)
}
......@@ -194,7 +195,7 @@ internal object RustPlacesConnection : Connection {
override fun syncBookmarks(syncInfo: SyncAuthInfo) {
check(api != null) { "must call init first" }
val ping = api!!.syncBookmarks(syncInfo)
val ping = api!!.syncBookmarks(syncInfo.into())
assembleBookmarksPing(ping)
}
......
......@@ -16,7 +16,7 @@ import mozilla.components.concept.storage.BookmarkInfo
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.concept.storage.BookmarkNodeType
import mozilla.components.concept.storage.BookmarksStorage
import mozilla.components.concept.sync.AuthInfo
import mozilla.components.concept.sync.SyncAuthInfo
import mozilla.components.concept.sync.SyncStatus
import mozilla.components.concept.sync.SyncableStore
import mozilla.components.support.base.log.logger.Logger
......@@ -177,10 +177,10 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
* @param authInfo The authentication information to sync with.
* @return Sync status of OK or Error
*/
override suspend fun sync(authInfo: AuthInfo): SyncStatus {
override suspend fun sync(authInfo: SyncAuthInfo): SyncStatus {
return withContext(scope.coroutineContext) {
syncAndHandleExceptions {
places.syncBookmarks(authInfo.into())
places.syncBookmarks(authInfo)
}
}
}
......
......@@ -13,7 +13,7 @@ import mozilla.components.concept.storage.PageObservation
import mozilla.components.concept.storage.SearchResult
import mozilla.components.concept.storage.VisitInfo
import mozilla.components.concept.storage.VisitType
import mozilla.components.concept.sync.AuthInfo
import mozilla.components.concept.sync.SyncAuthInfo
import mozilla.components.concept.sync.SyncStatus
import mozilla.components.concept.sync.SyncableStore
import mozilla.components.support.base.log.logger.Logger
......@@ -21,8 +21,6 @@ import mozilla.components.support.utils.segmentAwareDomainMatch
const val AUTOCOMPLETE_SOURCE_NAME = "placesHistory"
typealias SyncAuthInfo = mozilla.appservices.places.SyncAuthInfo
/**
* Implementation of the [HistoryStorage] which is backed by a Rust Places lib via [PlacesApi].
*/
......@@ -172,10 +170,10 @@ open class PlacesHistoryStorage(context: Context) : PlacesStorage(context), Hist
* @param authInfo The authentication information to sync with.
* @return Sync status of OK or Error
*/
override suspend fun sync(authInfo: AuthInfo): SyncStatus {
override suspend fun sync(authInfo: SyncAuthInfo): SyncStatus {
return withContext(scope.coroutineContext) {
syncAndHandleExceptions {
places.syncHistory(authInfo.into())
places.syncHistory(authInfo)
}
}
}
......
......@@ -5,20 +5,20 @@
@file:Suppress("MatchingDeclarationName")
package mozilla.components.browser.storage.sync
import mozilla.appservices.places.SyncAuthInfo
import java.util.Date
import mozilla.appservices.sync15.EngineInfo
import mozilla.appservices.sync15.FailureReason
import mozilla.components.concept.storage.VisitInfo
import mozilla.components.concept.storage.VisitType
import mozilla.components.concept.sync.AuthInfo
// We have type definitions at the concept level, and "external" types defined within Places.
// In practice these two types are largely the same, and this file is the conversion point.
/**
* Conversion from a generic AuthInfo type into a type 'places' lib uses at the interface boundary.
* Conversion from our SyncAuthInfo into its "native" version used at the interface boundary.
*/
internal fun AuthInfo.into(): SyncAuthInfo {
internal fun mozilla.components.concept.sync.SyncAuthInfo.into(): SyncAuthInfo {
return SyncAuthInfo(
kid = this.kid,
fxaAccessToken = this.fxaAccessToken,
......
......@@ -19,7 +19,7 @@ import mozilla.components.browser.storage.sync.GleanMetrics.Pings
import mozilla.components.concept.storage.BookmarkInfo
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.concept.storage.BookmarkNodeType
import mozilla.components.concept.sync.AuthInfo
import mozilla.components.concept.sync.SyncAuthInfo
import mozilla.components.concept.sync.SyncStatus
import mozilla.components.support.test.mock
import mozilla.components.support.test.robolectric.testContext
......@@ -374,7 +374,7 @@ class PlacesBookmarksStorageTest {
}
val storage = TestablePlacesBookmarksStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
Assert.assertTrue(result is SyncStatus.Ok)
Assert.assertEquals(conn.pingCount, 1)
......@@ -542,7 +542,7 @@ class PlacesBookmarksStorageTest {
}
val storage = TestablePlacesBookmarksStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
Assert.assertTrue(result is SyncStatus.Ok)
Assert.assertEquals(6, conn.pingCount)
......@@ -611,7 +611,7 @@ class PlacesBookmarksStorageTest {
}
val storage = TestablePlacesBookmarksStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
Assert.assertTrue(result is SyncStatus.Ok)
Assert.assertEquals(1, conn.pingCount)
......
......@@ -21,7 +21,7 @@ import mozilla.components.browser.storage.sync.GleanMetrics.HistorySync
import mozilla.components.browser.storage.sync.GleanMetrics.Pings
import mozilla.components.concept.storage.PageObservation
import mozilla.components.concept.storage.VisitType
import mozilla.components.concept.sync.AuthInfo
import mozilla.components.concept.sync.SyncAuthInfo
import mozilla.components.concept.sync.SyncStatus
import mozilla.components.support.test.mock
import mozilla.components.support.test.robolectric.testContext
......@@ -519,11 +519,12 @@ class PlacesHistoryStorageTest {
}
val storage = MockingPlacesHistoryStorage(conn)
storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
assertEquals("kid", passedAuthInfo!!.kid)
assertEquals("serverUrl", passedAuthInfo!!.tokenserverURL)
assertEquals("serverUrl", passedAuthInfo!!.tokenServerUrl)
assertEquals("token", passedAuthInfo!!.fxaAccessToken)
assertEquals(123L, passedAuthInfo!!.fxaAccessTokenExpiresAt)
assertEquals("key", passedAuthInfo!!.syncKey)
}
......@@ -550,7 +551,7 @@ class PlacesHistoryStorageTest {
}
val storage = MockingPlacesHistoryStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
assertEquals(SyncStatus.Ok, result)
}
......@@ -582,7 +583,7 @@ class PlacesHistoryStorageTest {
}
val storage = MockingPlacesHistoryStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
assertTrue(result is SyncStatus.Error)
......@@ -725,7 +726,7 @@ class PlacesHistoryStorageTest {
}
val storage = MockingPlacesHistoryStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
assertTrue(result is SyncStatus.Ok)
assertEquals(2, conn.pingCount)
......@@ -902,7 +903,7 @@ class PlacesHistoryStorageTest {
}
val storage = MockingPlacesHistoryStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
assertTrue(result is SyncStatus.Ok)
assertEquals(6, conn.pingCount)
......@@ -971,7 +972,7 @@ class PlacesHistoryStorageTest {
}
val storage = MockingPlacesHistoryStorage(conn)
val result = storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
val result = storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
assertTrue(result is SyncStatus.Ok)
assertEquals(1, conn.pingCount)
......@@ -1004,7 +1005,7 @@ class PlacesHistoryStorageTest {
}
}
val storage = MockingPlacesHistoryStorage(conn)
storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
fail()
}
}
......@@ -23,7 +23,7 @@ interface DeviceConstellation : Observable<DeviceEventsObserver> {
fun initDeviceAsync(
name: String,
type: DeviceType = DeviceType.MOBILE,
capabilities: List<DeviceCapability>
capabilities: Set<DeviceCapability>
): Deferred<Boolean>
/**
......@@ -43,7 +43,7 @@ interface DeviceConstellation : Observable<DeviceEventsObserver> {
* not supported.
* @return A [Deferred] that will be resolved with a success flag once operation is complete.
*/
fun ensureCapabilitiesAsync(capabilities: List<DeviceCapability>): Deferred<Boolean>
fun ensureCapabilitiesAsync(capabilities: Set<DeviceCapability>): Deferred<Boolean>
/**
* Current state of the constellation. May be missing if state was never queried.
......
......@@ -36,27 +36,6 @@ interface OAuthAccount : AutoCloseable {
fun registerPersistenceCallback(callback: StatePersistenceCallback)
fun deviceConstellation(): DeviceConstellation
fun toJSONString(): String
/**
* Returns an [AuthInfo] instance which may be used for data synchronization.
*
* @return An [AuthInfo] which is guaranteed to have a sync key.
* @throws AuthException if account needs to restart the OAuth flow.
*/
suspend fun authInfo(singleScope: String): AuthInfo {
val tokenServerURL = this.getTokenServerEndpointURL()
val tokenInfo = this.getAccessTokenAsync(singleScope).await()
?: throw AuthException(AuthExceptionType.NO_TOKEN)
val keyInfo = tokenInfo.key
?: throw AuthException(AuthExceptionType.KEY_INFO)
return AuthInfo(
kid = keyInfo.kid,
fxaAccessToken = tokenInfo.token,
syncKey = keyInfo.k,
tokenServerUrl = tokenServerURL
)
}
}
/**
......@@ -69,16 +48,6 @@ interface StatePersistenceCallback {
fun persist(data: String)
}
/**
* A Firefox Sync friendly auth object which can be obtained from [OAuthAccount].
*/
data class AuthInfo(
val kid: String,
val fxaAccessToken: String,
val syncKey: String,
val tokenServerUrl: String
)
/**
* Observer interface which lets its users monitor account state changes and major events.
*/
......@@ -104,12 +73,6 @@ interface AccountObserver {
* Account needs to be re-authenticated (e.g. due to a password change).
*/
fun onAuthenticationProblems()
/**
* Account manager encountered an error. Inspect [error] for details.
* @param error A specific error encountered.
*/
fun onError(error: Exception)
}
data class Avatar(
......
......@@ -4,9 +4,6 @@
package mozilla.components.concept.sync
import mozilla.components.support.base.observer.Observable
import java.lang.Exception
/**
* Results of running a sync via [SyncableStore.sync].
*/
......@@ -22,6 +19,29 @@ sealed class SyncStatus {
data class Error(val exception: Throwable) : SyncStatus()
}
/**
* A Firefox Sync friendly auth object which can be obtained from [OAuthAccount].
*
* Why is there a Firefox Sync-shaped authentication object at the concept level, you ask?
* Mainly because this is what the [SyncableStore] consumes in order to actually perform
* synchronization, which is in turn implemented by `places`-backed storage layer.
* If this class lived in `services-firefox-accounts`, we'd end up with an ugly dependency situation
* between services and storage components.
*
* Turns out that building a generic description of an authentication/synchronization layer is not
* quite the way to go when you only have a single, legacy implementation.
*
* However, this may actually improve once we retire the tokenserver from the architecture.
* We could also consider a heavier use of generics, as well.
*/
data class SyncAuthInfo(
val kid: String,
val fxaAccessToken: String,
val fxaAccessTokenExpiresAt: Long,
val syncKey: String,
val tokenServerUrl: String
)
/**
* Describes a "sync" entry point for a storage layer.
*/
......@@ -32,67 +52,7 @@ interface SyncableStore {
* @param authInfo Auth information necessary for syncing this store.
* @return [SyncStatus] A status object describing how sync went.
*/
suspend fun sync(authInfo: AuthInfo): SyncStatus
}
/**
* Describes a "sync" entry point for an application.
*/
interface SyncManager : Observable<SyncStatusObserver> {
/**
* An authenticated account is now available.
*/
fun authenticated(account: OAuthAccount)
/**
* Previously authenticated account has been logged-out.
*/
fun loggedOut()
/**
* Add a store, by [name], to a set of synchronization stores.
* Implementation is expected to be able to either access or instantiate concrete
* [SyncableStore] instances given this name.
*/
fun addStore(name: String)
/**
* Remove a store previously specified via [addStore] from a set of synchronization stores.
*/
fun removeStore(name: String)
/**
* Request an immediate synchronization of all configured stores.
*
* @param startup Boolean flag indicating if sync is being requested in a startup situation.
*/
fun syncNow(startup: Boolean = false)
/**
* Indicates if synchronization is currently active.
*/
fun isSyncRunning(): Boolean
}
/**
* An interface for consumers that wish to observer "sync lifecycle" events.
*/
interface SyncStatusObserver {
/**
* Gets called at the start of a sync, before any configured syncable is synchronized.
*/
fun onStarted()
/**
* Gets called at the end of a sync, after every configured syncable has been synchronized.
*/
fun onIdle()
/**
* Gets called if sync encounters an error that's worthy of processing by status observers.
* @param error Optional relevant exception.
*/
fun onError(error: Exception?)
suspend fun sync(authInfo: SyncAuthInfo): SyncStatus
}
/**
......
......@@ -11,8 +11,7 @@ import mozilla.components.concept.sync.DeviceType
import mozilla.components.concept.sync.OAuthAccount
import mozilla.components.concept.sync.Profile
import mozilla.components.feature.tabs.TabsUseCases
import mozilla.components.service.fxa.Config
import mozilla.components.service.fxa.manager.DeviceTuple
import mozilla.components.service.fxa.ServerConfig
import mozilla.components.service.fxa.manager.FxaAccountManager
import mozilla.components.support.test.any
import mozilla.components.support.test.mock
......@@ -25,6 +24,7 @@ import org.mockito.Mockito.`when`
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.service.fxa.DeviceConfig
// Same as the actual account manager, except we get to control how FirefoxAccountShaped instances
// are created. This is necessary because due to some build issues (native dependencies not available
......@@ -32,12 +32,12 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
// Instead, we express all of our account-related operations over an interface.
class TestableFxaAccountManager(
context: Context,
config: Config,
scopes: Array<String>,
config: ServerConfig,
scopes: Set<String>,
val block: () -> OAuthAccount = { mock() }
) : FxaAccountManager(context, config, scopes, DeviceTuple("test", DeviceType.MOBILE, listOf()), null) {
) : FxaAccountManager(context, config, DeviceConfig("test", DeviceType.MOBILE, setOf()), null, scopes) {
override fun createAccount(config: Config): OAuthAccount {
override fun createAccount(config: ServerConfig): OAuthAccount {
return block()
}
}
......@@ -118,8 +118,8 @@ class FirefoxAccountsAuthFeatureTest {
val manager = TestableFxaAccountManager(
testContext,
Config.release("dummyId", "bad://url"),
arrayOf("profile", "test-scope")
ServerConfig.release("dummyId", "bad://url"),
setOf("test-scope")
) {
mockAccount
}
......@@ -143,8 +143,8 @@ class FirefoxAccountsAuthFeatureTest {
val manager = TestableFxaAccountManager(
testContext,
Config.release("dummyId", "bad://url"),
arrayOf("profile", "test-scope")
ServerConfig.release("dummyId", "bad://url"),
setOf("test-scope")
) {
mockAccount
}
......
# [Android Components](../../../README.md) > Feature > Sync
A component which provides facilities for managing data synchronization.
Provided is an implementation of concept-sync's SyncManager, allowing
background execution of synchronization and easily observing synchronization
state.
Currently, synchronization runs periodically in the background.
## Usage
First, set everything up:
```kotlin
// A SyncableStore instance that we want to be able to synchronize.
private val historyStorage = PlacesHistoryStorage(this)
// Add your SyncableStore instances to the global registry.
GlobalSyncableStoreProvider.configureStore("history" to historyStorage)
// Configure SyncManager with sync scope and stores to synchronize.
private val syncManager = BackgroundSyncManager("https://identity.mozilla.com/apps/oldsync").also {
// Enable synchronization of store called "history".
// Internally, it will be accessed via GlobalSyncableStoreProvider.
it.addStore("history")
}
// Configure the account manager with an instance of a SyncManager.
// This will ensure that sync manager is made aware of auth changes.
private val accountManager by lazy {
FxaAccountManager(
this,
Config.release(CLIENT_ID, REDIRECT_URL),
arrayOf("profile", "https://identity.mozilla.com/apps/oldsync"),
syncManager
)
}
```
Once account manager is in an authenticated state, sync manager will immediately
synchronize, as well as schedule periodic syncing.
It's possible to re-configure which stores are to be synchronized via `addStore` and
`removeStore` calls.
It's also possible to observe sync states and request immediate sync of configured stores:
```kotlin
// Define what we want to do in response to sync status changes.
private val syncObserver = object : SyncStatusObserver {
override fun onStarted() {
CoroutineScope(Dispatchers.Main).launch {
syncStatus?.text = getString(R.string.syncing)
}
}
override fun onIdle() {
CoroutineScope(Dispatchers.Main).launch {
syncStatus?.text = getString(R.string.sync_idle)
val resultTextView: TextView = findViewById(R.id.syncResult)
// Can access synchronized data now.
}
}
override fun onError(error: Exception?) {
CoroutineScope(Dispatchers.Main).launch {
syncStatus?.text = getString(R.string.sync_error, error)
}
}
}
// Register a sync status observer with this sync manager.
syncManager.register(syncObserver, owner = this, autoPause = true)
// Kick off a sync in response to some user action. Sync will run in the
// background until it completes, surviving application shutdown if needed.
findViewById<View>(R.id.buttonSyncNow).setOnClickListener {
syncManager.syncNow()
}
```
### Setting up the dependency
Use Gradle to download the library from [maven.mozilla.org](https://maven.mozilla.org/) ([Setup repository](../../../README.md#maven-repository)):
```Groovy
implementation "org.mozilla.components:feature-sync:{latest-version}"
```
## License
This Source Code Form is subject to the terms of the Mozilla Public
License, v. 2.0. If a copy of the MPL was not distributed with this
file, You can obtain one at http://mozilla.org/MPL/2.0/
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
apply plugin: 'com.android.library'
apply plugin: 'kotlin-android'
android {
compileSdkVersion config.compileSdkVersion