Merge #5103

5103: Closes #5050: Improve handling of authentication problems r=csadilek a=grigoryk

This PR ensures that account doesn't disappear on app restart after hitting authentication problems.

Also:
- there's some FxaDeviceConstellation cleanup/simplification
- profile cache is now used; this way, we don't need to fetch profile every time account manager is instantiated
Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FxaDeviceConstellation.kt

@@ -5,7 +5,6 @@
 package mozilla.components.service.fxa
 
 import android.content.Context
-import androidx.annotation.GuardedBy
 import androidx.lifecycle.LifecycleOwner
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Deferred
@@ -39,12 +38,10 @@ class FxaDeviceConstellation(
 
     private val deviceObserverRegistry = ObserverRegistry<DeviceConstellationObserver>()
 
-    @GuardedBy("this")
+    @Volatile
     private var constellationState: ConstellationState? = null
 
-    override fun state(): ConstellationState? = synchronized(this) {
-        return constellationState
-    }
+    override fun state(): ConstellationState? = constellationState
 
     override fun initDeviceAsync(
         name: String,
@@ -137,37 +134,35 @@ class FxaDeviceConstellation(
         notifyObservers { onEvents(events) }
     }
 
-    override fun refreshDevicesAsync(): Deferred<Boolean> = synchronized(this) {
-        return scope.async {
-            logger.info("Refreshing device list...")
+    override fun refreshDevicesAsync(): Deferred<Boolean> = scope.async {
+        logger.info("Refreshing device list...")
 
-            // Attempt to fetch devices, or bail out on failure.
-            val allDevices = fetchAllDevicesAsync().await() ?: return@async false
+        // Attempt to fetch devices, or bail out on failure.
+        val allDevices = fetchAllDevicesAsync().await() ?: return@async false
 
-            // Find the current device.
-            val currentDevice = allDevices.find { it.isCurrentDevice }
-            // Filter out the current devices.
-            val otherDevices = allDevices.filter { !it.isCurrentDevice }
+        // Find the current device.
+        val currentDevice = allDevices.find { it.isCurrentDevice }?.also {
+            // Check if our current device's push subscription needs to be renewed.
+            if (it.subscriptionExpired) {
+                logger.info("Current device needs push endpoint registration")
+            }
+        }
 
-            val newState = ConstellationState(currentDevice, otherDevices)
-            constellationState = newState
+        // Filter out the current devices.
+        val otherDevices = allDevices.filter { !it.isCurrentDevice }
 
-            CoroutineScope(Dispatchers.Main).launch {
-                // NB: at this point, 'constellationState' might have changed.
-                // Notify with an immutable, local 'newState' instead.
-                deviceObserverRegistry.notifyObservers { onDevicesUpdate(newState) }
-            }
+        val newState = ConstellationState(currentDevice, otherDevices)
+        constellationState = newState
 
-            // Check if our current device's push subscription needs to be renewed.
-            constellationState?.currentDevice?.let {
-                if (it.subscriptionExpired) {
-                    logger.info("Current device needs push endpoint registration")
-                }
-            }
+        logger.info("Refreshed device list; saw ${allDevices.size} device(s).")
 
-            logger.info("Refreshed device list; saw ${allDevices.size} device(s).")
-            true
+        CoroutineScope(Dispatchers.Main).launch {
+            // NB: at this point, 'constellationState' might have changed.
+            // Notify with an immutable, local 'newState' instead.
+            deviceObserverRegistry.notifyObservers { onDevicesUpdate(newState) }
         }
+
+        true
     }
 
     /**

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt

@@ -233,6 +233,10 @@ open class FxaAccountManager(
     // list, although that's probably an overkill.
     @Volatile private lateinit var account: OAuthAccount
     @Volatile private var profile: Profile? = null
+
+    // We'd like to persist this state, so that we can short-circuit transition to AuthenticationProblem on
+    // initialization, instead of triggering the full state machine knowing in advance we'll hit auth problems.
+    // See https://github.com/mozilla-mobile/android-components/issues/5102
     @Volatile private var state = AccountState.Start
     private val eventQueue = ConcurrentLinkedQueue<Event>()
 
@@ -683,14 +687,13 @@ open class FxaAccountManager(
                         // If this is the first time ensuring our capabilities,
                         logger.info("Ensuring device capabilities...")
                         if (account.deviceConstellation().ensureCapabilitiesAsync(deviceConfig.capabilities).await()) {
-                            logger.info("Successfully ensured device capabilities.")
+                            logger.info("Successfully ensured device capabilities. Continuing...")
+                            postAuthenticated(AuthType.Existing)
+                            Event.FetchProfile
                         } else {
-                            logger.warn("Failed to ensure device capabilities.")
+                            logger.warn("Failed to ensure device capabilities. Stopping.")
+                            null
                         }
-
-                        postAuthenticated(AuthType.Existing)
-
-                        Event.FetchProfile
                     }
                     Event.SignedInShareableAccount -> {
                         // Note that we are not registering an account persistence callback here like
@@ -734,7 +737,8 @@ open class FxaAccountManager(
                         // https://github.com/mozilla/application-services/issues/483
                         logger.info("Fetching profile...")
 
-                        profile = account.getProfileAsync(true).await()
+                        // `account` provides us with intelligent profile caching, so make sure to use it.
+                        profile = account.getProfileAsync(ignoreCache = false).await()
                         if (profile == null) {
                             return Event.FailedToFetchProfile
                         }
@@ -793,20 +797,9 @@ open class FxaAccountManager(
                                 // we will disconnect users that hit transient network errors during
                                 // an authorization check.
                                 // See https://github.com/mozilla-mobile/android-components/issues/3347
-                                logger.info("Unable to recover from an auth problem, clearing state.")
-
-                                // We perform similar actions to what we do on logout, except for destroying
-                                // the device. We don't have valid access tokens at this point to do that!
-
-                                // Clean up resources.
-                                profile = null
-                                account.close()
-                                // Delete persisted state.
-                                getAccountStorage().clear()
-                                // Re-initialize account.
-                                account = createAccount(serverConfig)
+                                logger.info("Unable to recover from an auth problem, notifying observers.")
 
-                                // Finally, tell our listeners we're in a bad auth state.
+                                // Tell our listeners we're in a bad auth state.
                                 notifyObservers { onAuthenticationProblems() }
                             }
                         }

components/service/firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaAccountManagerTest.kt

@@ -917,7 +917,7 @@ class FxaAccountManagerTest {
         val constellation: DeviceConstellation = mock()
         val profile = Profile(
             "testUid", "test@example.com", null, "Test Profile")
-        `when`(mockAccount.getProfileAsync(anyBoolean())).thenReturn(CompletableDeferred(profile))
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).thenReturn(CompletableDeferred(profile))
         // We have an account at the start.
         `when`(accountStorage.read()).thenReturn(mockAccount)
         `when`(mockAccount.getCurrentDeviceId()).thenReturn("testDeviceId")
@@ -1046,7 +1046,7 @@ class FxaAccountManagerTest {
         val mockAccount: OAuthAccount = mock()
         val profile = Profile(uid = "testUID", avatar = null, email = "test@example.com", displayName = "test profile")
         val accountStorage = mock<AccountStorage>()
-        `when`(mockAccount.getProfileAsync(anyBoolean())).thenReturn(CompletableDeferred(profile))
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).thenReturn(CompletableDeferred(profile))
 
         val fxaPanic = CompletableDeferred<AuthFlowUrl>()
         fxaPanic.completeExceptionally(FxaPanicException("panic!"))
@@ -1232,7 +1232,9 @@ class FxaAccountManagerTest {
         verify(accountObserver, times(1)).onAuthenticationProblems()
         assertTrue(manager.accountNeedsReauth())
         assertEquals(mockAccount, manager.authenticatedAccount())
-        assertNull(manager.accountProfile())
+
+        // Make sure profile is still available.
+        assertEquals(profile, manager.accountProfile())
 
         // Able to re-authenticate.
         reset(accountObserver)
@@ -1295,7 +1297,7 @@ class FxaAccountManagerTest {
         `when`(mockAccount.deviceConstellation()).thenReturn(constellation)
         `when`(mockAccount.getCurrentDeviceId()).thenReturn("testDeviceId")
         `when`(constellation.initDeviceAsync(any(), any(), any())).thenReturn(CompletableDeferred(true))
-        `when`(mockAccount.getProfileAsync(anyBoolean())).thenReturn(CompletableDeferred(value = null))
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).thenReturn(CompletableDeferred(value = null))
         `when`(mockAccount.beginOAuthFlowAsync(any())).thenReturn(CompletableDeferred(AuthFlowUrl(EXPECTED_AUTH_STATE, "auth://url")))
         `when`(mockAccount.completeOAuthFlowAsync(anyString(), anyString())).thenReturn(CompletableDeferred(true))
         // There's no account at the start.
@@ -1340,7 +1342,7 @@ class FxaAccountManagerTest {
         val profile = Profile(
             uid = "testUID", avatar = null, email = "test@example.com", displayName = "test profile")
 
-        `when`(mockAccount.getProfileAsync(anyBoolean())).thenReturn(CompletableDeferred(profile))
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).thenReturn(CompletableDeferred(profile))
 
         manager.updateProfileAsync().await()
 
@@ -1376,7 +1378,7 @@ class FxaAccountManagerTest {
             mockAccount
         }
 
-        `when`(mockAccount.getProfileAsync(anyBoolean())).then {
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).then {
             // Hit an auth error.
             CoroutineScope(coroutineContext).launch { manager.encounteredAuthError() }
             CompletableDeferred(value = null)
@@ -1433,7 +1435,7 @@ class FxaAccountManagerTest {
             mockAccount
         }
 
-        `when`(mockAccount.getProfileAsync(anyBoolean())).then {
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).then {
             // Hit an auth error.
             CoroutineScope(coroutineContext).launch { manager.encounteredAuthError() }
             CompletableDeferred(value = null)
@@ -1495,7 +1497,7 @@ class FxaAccountManagerTest {
         }
 
         var didFailProfileFetch = false
-        `when`(mockAccount.getProfileAsync(anyBoolean())).then {
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).then {
             // Hit an auth error, but only once. As we recover from it, we'll attempt to fetch a profile
             // again. At that point, we'd like to succeed.
             if (!didFailProfileFetch) {
@@ -1552,7 +1554,7 @@ class FxaAccountManagerTest {
         `when`(mockAccount.getCurrentDeviceId()).thenReturn("testDeviceId")
         `when`(mockAccount.deviceConstellation()).thenReturn(constellation)
         `when`(constellation.initDeviceAsync(any(), any(), any())).thenReturn(CompletableDeferred(true))
-        `when`(mockAccount.getProfileAsync(anyBoolean())).thenReturn(exceptionalProfile)
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).thenReturn(exceptionalProfile)
         `when`(mockAccount.beginOAuthFlowAsync(any())).thenReturn(CompletableDeferred(AuthFlowUrl(EXPECTED_AUTH_STATE, "auth://url")))
         `when`(mockAccount.completeOAuthFlowAsync(anyString(), anyString())).thenReturn(CompletableDeferred(true))
         // There's no account at the start.
@@ -1630,7 +1632,7 @@ class FxaAccountManagerTest {
         coroutineContext: CoroutineContext
     ): FxaAccountManager {
 
-        `when`(mockAccount.getProfileAsync(anyBoolean())).thenReturn(CompletableDeferred(profile))
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).thenReturn(CompletableDeferred(profile))
         `when`(mockAccount.beginOAuthFlowAsync(any())).thenReturn(CompletableDeferred(AuthFlowUrl(EXPECTED_AUTH_STATE, "auth://url")))
         `when`(mockAccount.beginPairingFlowAsync(anyString(), any())).thenReturn(CompletableDeferred(AuthFlowUrl(EXPECTED_AUTH_STATE, "auth://url")))
         `when`(mockAccount.completeOAuthFlowAsync(anyString(), anyString())).thenReturn(CompletableDeferred(true))
@@ -1661,7 +1663,7 @@ class FxaAccountManagerTest {
         accountObserver: AccountObserver,
         coroutineContext: CoroutineContext
     ): FxaAccountManager {
-        `when`(mockAccount.getProfileAsync(anyBoolean())).thenReturn(CompletableDeferred(profile))
+        `when`(mockAccount.getProfileAsync(ignoreCache = false)).thenReturn(CompletableDeferred(profile))
 
         `when`(mockAccount.beginOAuthFlowAsync(any())).thenReturn(CompletableDeferred(value = null))
         `when`(mockAccount.beginPairingFlowAsync(anyString(), any())).thenReturn(CompletableDeferred(value = null))

docs/changelog.md

@@ -18,6 +18,10 @@ permalink: /changelog/
 * **feature-prompts** and **feature-downloads**
   * Fix [issue #6439](https://github.com/mozilla-mobile/fenix/issues/6439) "Crash when downloading Image"
 
+* **service-firefox-accounts**
+  * Account profile cache is now used, removing a network call from most instances of account manager instantiation.
+  * Fixed a bug where account would disappear after restarting an app which hit authentication problems.
+
 # 22.0.0
 
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v22.0.0...master)

samples/sync/src/main/java/org/mozilla/samples/sync/MainActivity.kt

@@ -42,6 +42,7 @@ import mozilla.components.service.fxa.FxaAuthData
 import mozilla.components.service.fxa.SyncEngine
 import mozilla.components.service.fxa.sync.SyncReason
 import mozilla.components.service.fxa.toAuthType
+import mozilla.components.support.base.log.logger.Logger
 import mozilla.components.support.base.log.sink.AndroidLogSink
 import mozilla.components.support.rusthttp.RustHttpConfig
 import mozilla.components.support.rustlog.RustLog
@@ -89,6 +90,8 @@ class MainActivity :
         const val REDIRECT_URL = "https://accounts.firefox.com/oauth/success/$CLIENT_ID"
     }
 
+    private val logger = Logger("SampleSync")
+
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
         RustLog.enable()
@@ -237,6 +240,8 @@ class MainActivity :
         lateinit var lastAuthType: AuthType
 
         override fun onLoggedOut() {
+            logger.info("onLoggedOut")
+
             launch {
                 val txtView: TextView = findViewById(R.id.fxaStatusView)
                 txtView.text = getString(R.string.logged_out)
@@ -262,6 +267,8 @@ class MainActivity :
         }
 
         override fun onAuthenticationProblems() {
+            logger.info("onAuthenticationProblems")
+
             launch {
                 val txtView: TextView = findViewById(R.id.fxaStatusView)
                 txtView.text = getString(R.string.need_reauth)
@@ -271,6 +278,8 @@ class MainActivity :
         }
 
         override fun onAuthenticated(account: OAuthAccount, authType: AuthType) {
+            logger.info("onAuthenticated")
+
             launch {
                 lastAuthType = authType
 
@@ -292,6 +301,8 @@ class MainActivity :
         }
 
         override fun onProfileUpdated(profile: Profile) {
+            logger.info("onProfileUpdated")
+
             launch {
                 val txtView: TextView = findViewById(R.id.fxaStatusView)
                 txtView.text = getString(
@@ -305,12 +316,14 @@ class MainActivity :
 
     private val syncObserver = object : SyncStatusObserver {
         override fun onStarted() {
+            logger.info("onSyncStarted")
             CoroutineScope(Dispatchers.Main).launch {
                 syncStatus?.text = getString(R.string.syncing)
             }
         }
 
         override fun onIdle() {
+            logger.info("onSyncIdle")
             CoroutineScope(Dispatchers.Main).launch {
                 syncStatus?.text = getString(R.string.sync_idle)
 
@@ -344,6 +357,7 @@ class MainActivity :
         }
 
         override fun onError(error: Exception?) {
+            logger.error("onSyncError", error)
             CoroutineScope(Dispatchers.Main).launch {
                 syncStatus?.text = getString(R.string.sync_error, error)
             }