From 01306f87c3de19bd91179f8a7ad6a4bcfdeda943 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Fri, 25 Sep 2020 00:59:18 -0700 Subject: [PATCH] Closes #8492: Gracefully handle network failures while obtaining token server url --- .../components/concept/sync/OAuthAccount.kt | 4 +-- .../components/service/fxa/FirefoxAccount.kt | 6 +++-- .../service/fxa/manager/FxaAccountManager.kt | 22 +++++++++++----- .../service/fxa/FxaAccountManagerTest.kt | 26 ++++++++++++++++--- 4 files changed, 45 insertions(+), 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 index 0826c0f3b44..3e955c34610 100644 --- 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/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt b/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt index 04a24af249e..c2965639cec 100644 --- a/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt +++ b/components/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/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt b/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt index de517c437fa..fd3ab09aa21 100644 --- a/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt +++ b/components/service/firefox-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/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt b/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt index a01e1c02d8a..f5c7761fd38 100644 --- a/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt +++ b/components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt @@ -35,6 +35,7 @@ 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.support.base.observer.Observable import mozilla.components.support.base.observer.ObserverRegistry import mozilla.components.support.test.any @@ -91,6 +92,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 +746,7 @@ class FxaAccountManagerTest { return ableToRecoverFromAuthError } - override fun getTokenServerEndpointURL(): String { + override suspend fun getTokenServerEndpointURL(): String? { if (tokenServerEndpointUrl != null) return tokenServerEndpointUrl fail() @@ -946,6 +949,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 +1690,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 }