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.
mozilla-mobile · Jul 10, 2019 · 8308693 · 8308693
1 parent fa8e3d6
commit 8308693
Show file tree

Hide file tree

Showing 36 changed files with 972 additions and 1,072 deletions.
diff --git a/.buildconfig.yml b/.buildconfig.yml
@@ -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.'

diff --git a/.../browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt b/.../browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt
@@ -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)
     }
 

diff --git a/...rage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt b/...rage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt
@@ -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)
             }
         }
     }

diff --git a/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt b/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt
@@ -13,16 +13,14 @@ 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
 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)
             }
         }
     }

diff --git a/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt b/...nents/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt
@@ -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,

diff --git a/...-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt b/...-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt
@@ -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)

diff --git a/...ge-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt b/...ge-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt
@@ -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
@@ -518,11 +518,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)
     }
 
@@ -549,7 +550,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)
     }
 
@@ -581,7 +582,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)
 
@@ -724,7 +725,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)
@@ -901,7 +902,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)
@@ -970,7 +971,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)
@@ -1003,7 +1004,7 @@ class PlacesHistoryStorageTest {
             }
         }
         val storage = MockingPlacesHistoryStorage(conn)
-        storage.sync(AuthInfo("kid", "token", "key", "serverUrl"))
+        storage.sync(SyncAuthInfo("kid", "token", 123L, "key", "serverUrl"))
         fail()
     }
 }
diff --git a/components/concept/sync/src/main/java/mozilla/components/concept/sync/Devices.kt b/components/concept/sync/src/main/java/mozilla/components/concept/sync/Devices.kt
@@ -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.

diff --git a/components/concept/sync/src/main/java/mozilla/components/concept/sync/OAuthAccount.kt b/components/concept/sync/src/main/java/mozilla/components/concept/sync/OAuthAccount.kt
@@ -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(