Closes mozilla-mobile#8492: Gracefully handle network failures while …

…obtaining token server url
grigoryk · Sep 25, 2020 · 6f1147c · 6f1147c
1 parent 40f30c2
commit 6f1147c
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 13 deletions.
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
@@ -169,9 +169,9 @@ interface OAuthAccount : AutoCloseable {
     /**
      * Fetches the token server endpoint, for authentication using the SAML bearer flow.
      *
-     * @return Token server endpoint URL string
+     * @return Token server endpoint URL string, `null` if it couldn't be obtained.
      */
-    fun getTokenServerEndpointURL(): String
+    suspend fun getTokenServerEndpointURL(): String?
 
     /**
      * Get the pairing URL to navigate to on the Authority side (typically a computer).

diff --git a/...s/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt b/...s/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt
@@ -184,8 +184,10 @@ class FirefoxAccount internal constructor(
         }
     }
 
-    override fun getTokenServerEndpointURL(): String {
-        return inner.getTokenServerEndpointURL()
+    override suspend fun getTokenServerEndpointURL() = withContext(scope.coroutineContext) {
+        handleFxaExceptions(logger, "getTokenServerEndpointURL", { null }) {
+            inner.getTokenServerEndpointURL()
+        }
     }
 
     override fun getPairingAuthorityURL(): String {

diff --git a/...irefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt b/...irefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt
@@ -837,12 +837,22 @@ open class FxaAccountManager(
             return
         }
 
-        // NB: this call will call into GlobalAccountManager.authError if necessary.
-        // Network failures are ignored.
-        account.getAccessToken(SCOPE_SYNC)?.let {
-            SyncAuthInfoCache(context).setToCache(
-                it.asSyncAuthInfo(account.getTokenServerEndpointURL())
-            )
+        val accessToken = account.getAccessToken(SCOPE_SYNC)
+        val tokenServerUrl = if (accessToken != null) {
+            // Only try to get the endpoint if we have an access token.
+            account.getTokenServerEndpointURL()
+        } else {
+            null
+        }
+
+        if (accessToken != null && tokenServerUrl != null) {
+            SyncAuthInfoCache(context).setToCache(accessToken.asSyncAuthInfo(tokenServerUrl))
+        } else {
+            // At this point, SyncAuthInfoCache may be entirely empty. In this case, we won't be
+            // able to sync via the background worker. We will attempt to populate SyncAuthInfoCache
+            // again in `syncNow` (in response to a direct user action) and after application restarts.
+            logger.warn("Couldn't populate SyncAuthInfoCache. Sync may not work.")
+            logger.warn("Is null? - accessToken: ${accessToken == null}, tokenServerUrl: ${tokenServerUrl == null}")
         }
     }
 

diff --git a/...ce/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt b/...ce/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt
@@ -35,6 +35,8 @@ import mozilla.components.service.fxa.sync.SyncDispatcher
 import mozilla.components.service.fxa.sync.SyncReason
 import mozilla.components.service.fxa.sync.SyncStatusObserver
 import mozilla.components.concept.base.crash.CrashReporting
+import mozilla.components.concept.sync.OAuthScopedKey
+import mozilla.components.concept.sync.SyncAuthInfo
 import mozilla.components.support.base.observer.Observable
 import mozilla.components.support.base.observer.ObserverRegistry
 import mozilla.components.support.test.any
@@ -53,6 +55,7 @@ import org.junit.Assert.fail
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.ArgumentMatchers.anyBoolean
+import org.mockito.ArgumentMatchers.anyLong
 import org.mockito.ArgumentMatchers.anyString
 import org.mockito.Mockito.`when`
 import org.mockito.Mockito.doAnswer
@@ -91,6 +94,8 @@ internal open class TestableFxaAccountManager(
     override fun getAccountStorage(): AccountStorage {
         return storage
     }
+
+    override fun createSyncManager(config: SyncConfig): SyncManager = mock()
 }
 
 const val EXPECTED_AUTH_STATE = "goodAuthState"
@@ -743,7 +748,7 @@ class FxaAccountManagerTest {
             return ableToRecoverFromAuthError
         }
 
-        override fun getTokenServerEndpointURL(): String {
+        override suspend fun getTokenServerEndpointURL(): String? {
             if (tokenServerEndpointUrl != null) return tokenServerEndpointUrl
 
             fail()
@@ -946,6 +951,14 @@ class FxaAccountManagerTest {
 
         assertEquals(mockAccount, manager.authenticatedAccount())
         assertEquals(profile, manager.accountProfile())
+
+        val cachedAuthInfo = SyncAuthInfoCache(testContext).getCached()
+        assertNotNull(cachedAuthInfo)
+        assertEquals("kid", cachedAuthInfo!!.kid)
+        assertEquals("someToken", cachedAuthInfo.fxaAccessToken)
+        assertEquals("k", cachedAuthInfo.syncKey)
+        assertEquals("some://url", cachedAuthInfo.tokenServerUrl)
+        assertTrue(cachedAuthInfo.fxaAccessTokenExpiresAt > 0)
     }
 
     @Test(expected = FxaPanicException::class)
@@ -1679,17 +1692,26 @@ class FxaAccountManagerTest {
         crashReporter: CrashReporting? = null
     ): FxaAccountManager {
 
+        val accessTokenInfo = AccessTokenInfo(
+            "testSc0pe", "someToken", OAuthScopedKey("kty", "testSc0pe", "kid", "k"), System.currentTimeMillis() + 1000 * 10
+        )
+
         `when`(mockAccount.getProfile(ignoreCache = false)).thenReturn(profile)
         `when`(mockAccount.beginOAuthFlow(any(), anyString())).thenReturn(AuthFlowUrl(EXPECTED_AUTH_STATE, "auth://url"))
         `when`(mockAccount.beginPairingFlow(anyString(), any(), anyString())).thenReturn(AuthFlowUrl(EXPECTED_AUTH_STATE, "auth://url"))
         `when`(mockAccount.completeOAuthFlow(anyString(), anyString())).thenReturn(true)
+        `when`(mockAccount.getAccessToken(anyString())).thenReturn(accessTokenInfo)
+        `when`(mockAccount.getTokenServerEndpointURL()).thenReturn("some://url")
+
         // There's no account at the start.
         `when`(accountStorage.read()).thenReturn(null)
 
         val manager = TestableFxaAccountManager(
             testContext,
-                ServerConfig(Server.RELEASE, "dummyId", "bad://url"),
-                accountStorage, capabilities, coroutineContext = coroutineContext, crashReporter = crashReporter
+            ServerConfig(Server.RELEASE, "dummyId", "bad://url"),
+            accountStorage, capabilities,
+            SyncConfig(setOf(SyncEngine.History, SyncEngine.Bookmarks), PeriodicSyncConfig()),
+            coroutineContext = coroutineContext, crashReporter = crashReporter
         ) {
             mockAccount
         }