diff --git a/app/src/main/java/org/mozilla/fenix/components/AccountAbnormalities.kt b/app/src/main/java/org/mozilla/fenix/components/AccountAbnormalities.kt new file mode 100644 index 000000000..e52718539 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/components/AccountAbnormalities.kt @@ -0,0 +1,182 @@ +/* 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/. */ + +package org.mozilla.fenix.components + +import android.content.Context +import androidx.annotation.GuardedBy +import androidx.annotation.VisibleForTesting +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import mozilla.components.concept.sync.AccountObserver +import mozilla.components.concept.sync.AuthType +import mozilla.components.concept.sync.OAuthAccount +import mozilla.components.lib.crash.CrashReporter +import mozilla.components.service.fxa.manager.FxaAccountManager +import mozilla.components.support.base.log.logger.Logger +import java.lang.Exception +import kotlin.coroutines.CoroutineContext + +/** + * Miscellaneous FxA-related abnormalities. + */ +@VisibleForTesting +internal abstract class AbnormalFxaEvent : Exception() { + /** + * Indicates an overlapping sign-out request. + */ + class OverlappingFxaLogoutRequest : AbnormalFxaEvent() + + /** + * Indicates an onLogout callback which was received without a preceding onAuthenticated callback. + */ + class LogoutWithoutAuth : AbnormalFxaEvent() + + /** + * Indicates an unexpected sign-out event. All events must be user-triggered; this exception is + * logged when a sign-out event was detected without a corresponding user action. + */ + class UnexpectedFxaLogout : AbnormalFxaEvent() + + /** + * Indicates an account that's missing after startup, while it was expected to be present. + */ + class MissingExpectedAccountAfterStartup : AbnormalFxaEvent() +} + +/** + * Observes account-related events, and reports any detected abnormalities via [crashReporter]. + * + * See [AbnormalFxaEvent] for types of abnormal events this class detects. + * + * @param crashReporter An instance of [CrashReporter] used for reporting detected abnormalities. + * @param coroutineContext A [CoroutineContext] used for executing async tasks. Defaults to [Dispatchers.IO]. + */ +class AccountAbnormalities( + context: Context, + private val crashReporter: CrashReporter, + private val coroutineContext: CoroutineContext = Dispatchers.IO +) : AccountObserver { + companion object { + private const val PREF_FXA_ABNORMALITIES = "fxa_abnormalities" + private const val KEY_HAS_ACCOUNT = "has_account" + } + + @GuardedBy("this") + private var isLoggingOut = false + + @Volatile + private var accountManagerConfigured = false + + @Volatile + private var onAuthenticatedCalled = false + + private val logger = Logger("AccountAbnormalities") + + private val prefs = context.getSharedPreferences(PREF_FXA_ABNORMALITIES, Context.MODE_PRIVATE) + + /** + * Once [accountManager] is initialized, queries it to detect abnormal account states. + * Call this right after running [FxaAccountManager.initAsync]. + * + * @param accountManager An instance of [FxaAccountManager]. + * @param initResult A deferred result of initializing [accountManager]. + * @return A [Unit] deferred, resolved once [initResult] is resolved and state is processed for abnormalities. + */ + fun accountManagerInitializedAsync( + accountManager: FxaAccountManager, + initResult: Deferred + ): Deferred { + accountManagerConfigured = true + + return CoroutineScope(coroutineContext).async { + // Wait for the account manager to finish initializing. If it's queried before the + // "init" deferred returns, we'll get inaccurate results. + initResult.await() + + // Account manager finished initialization, we can now query it for the account state + // and see if it doesn't match our expectations. + // Behaviour considered abnormal: + // - we had an account before, and it's no longer present during startup + + // We use a flag in prefs to keep track of the fact that we have an authenticated + // account. This works because our account state is persisted in the application's + // directory, same as SharedPreferences. If user clears application data, both the + // fxa state and our flag will be removed. + val hadAccountBefore = prefs.getBoolean(KEY_HAS_ACCOUNT, false) + val hasAccountNow = accountManager.authenticatedAccount() != null + if (hadAccountBefore && !hasAccountNow) { + prefs.edit().putBoolean(KEY_HAS_ACCOUNT, false).apply() + + logger.warn("Missing expected account on startup") + + crashReporter.submitCaughtException( + AbnormalFxaEvent.MissingExpectedAccountAfterStartup() + ) + } + } + } + + /** + * Keeps track of user requests to logout. + */ + fun userRequestedLogout() { + check(accountManagerConfigured) { + "userRequestedLogout before account manager was configured" + } + + // Expecting to have seen an onAuthenticated callback before a logout can be triggered. + if (!onAuthenticatedCalled) { + crashReporter.submitCaughtException(AbnormalFxaEvent.LogoutWithoutAuth()) + } + + // If we're not already in the process of logging out, do nothing. + synchronized(this) { + if (!isLoggingOut) { + isLoggingOut = true + return + } + } + + logger.warn("Overlapping logout request") + + // Otherwise, this is an unexpected logout request - there shouldn't be a legitimate way for + // the user to request multiple overlapping logouts. Log an exception. + crashReporter.submitCaughtException(AbnormalFxaEvent.OverlappingFxaLogoutRequest()) + } + + override fun onAuthenticated(account: OAuthAccount, authType: AuthType) { + check(accountManagerConfigured) { "onAuthenticated before account manager was configured" } + + onAuthenticatedCalled = true + + // We don't check if KEY_HAS_ACCOUNT was already true: we will see onAuthenticated on every + // startup, so any combination of "new value" and "previous value" for this flag is normal. + prefs.edit().putBoolean(KEY_HAS_ACCOUNT, true).apply() + } + + override fun onLoggedOut() { + check(accountManagerConfigured) { "onLoggedOut before account manager was configured" } + + onAuthenticatedCalled = false + + prefs.edit().putBoolean(KEY_HAS_ACCOUNT, false).apply() + + // If we're in the process of logging out (via userRequestedLogout), do nothing. + synchronized(this) { + if (isLoggingOut) { + isLoggingOut = false + return + } + } + + logger.warn("Unexpected sign-out") + + // Otherwise, this is an unexpected logout event - all logout events are expected to be + // user-triggered. Log an exception. + crashReporter.submitCaughtException(AbnormalFxaEvent.UnexpectedFxaLogout()) + } +} diff --git a/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt b/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt index cede0b2f8..042c6081b 100644 --- a/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt +++ b/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt @@ -25,6 +25,7 @@ import mozilla.components.feature.push.AutoPushSubscription import mozilla.components.feature.push.PushConfig import mozilla.components.feature.push.PushSubscriptionObserver import mozilla.components.feature.push.PushType +import mozilla.components.lib.crash.CrashReporter import mozilla.components.service.fxa.DeviceConfig import mozilla.components.service.fxa.ServerConfig import mozilla.components.service.fxa.SyncConfig @@ -50,6 +51,7 @@ import java.util.FormatFlagsConversionMismatchException @Mockable class BackgroundServices( private val context: Context, + crashReporter: CrashReporter, historyStorage: PlacesHistoryStorage, bookmarkStorage: PlacesBookmarksStorage ) { @@ -116,6 +118,8 @@ class BackgroundServices( context.components.analytics.metrics ) + val accountAbnormalities = AccountAbnormalities(context, crashReporter) + private val pushAccountObserver by lazy { push?.let { PushAccountObserver(it) } } val accountManager = makeAccountManager(context, serverConfig, deviceConfig, syncConfig) @@ -171,6 +175,10 @@ class BackgroundServices( // Register a telemetry account observer to keep track of FxA auth metrics. accountManager.register(telemetryAccountObserver) + // Register an "abnormal fxa behaviour" middleware to keep track of events such as + // unexpected logouts. + accountManager.register(accountAbnormalities) + // Enable push if it's configured. push?.let { autoPushFeature -> // Register the push account observer so we know how to update our push subscriptions. @@ -206,7 +214,10 @@ class BackgroundServices( } }) } - accountManager.initAsync() + accountAbnormalities.accountManagerInitializedAsync( + accountManager, + accountManager.initAsync() + ) } /** diff --git a/app/src/main/java/org/mozilla/fenix/components/Components.kt b/app/src/main/java/org/mozilla/fenix/components/Components.kt index 1764710d3..e222f736e 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Components.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Components.kt @@ -15,7 +15,7 @@ import org.mozilla.fenix.utils.ClipboardHandler @Mockable class Components(private val context: Context) { val backgroundServices by lazy { - BackgroundServices(context, core.historyStorage, core.bookmarksStorage) + BackgroundServices(context, analytics.crashReporter, core.historyStorage, core.bookmarksStorage) } val services by lazy { Services(context, backgroundServices.accountManager) } val core by lazy { Core(context) } diff --git a/app/src/main/java/org/mozilla/fenix/settings/account/SignOutFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/account/SignOutFragment.kt index ae990de9a..d1cec43a2 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/account/SignOutFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/account/SignOutFragment.kt @@ -57,6 +57,8 @@ class SignOutFragment : BottomSheetDialogFragment() { view.signOutDisconnect.setOnClickListener { lifecycleScope.launch { + requireComponents + .backgroundServices.accountAbnormalities.userRequestedLogout() accountManager.logoutAsync().await() }.invokeOnCompletion { if (!findNavController().popBackStack(R.id.settingsFragment, false)) { diff --git a/app/src/test/java/org/mozilla/fenix/components/AccountAbnormalitiesTest.kt b/app/src/test/java/org/mozilla/fenix/components/AccountAbnormalitiesTest.kt new file mode 100644 index 000000000..7b707e781 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/components/AccountAbnormalitiesTest.kt @@ -0,0 +1,160 @@ +/* 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/. */ + +package org.mozilla.fenix.components + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.runBlocking +import mozilla.components.lib.crash.CrashReporter +import mozilla.components.service.fxa.manager.FxaAccountManager +import mozilla.components.support.test.argumentCaptor +import mozilla.components.support.test.mock +import mozilla.components.support.test.robolectric.testContext +import org.junit.Assert.assertEquals +import org.junit.Assert.fail +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.verifyZeroInteractions +import org.mockito.Mockito.verify +import org.mozilla.fenix.TestApplication +import org.robolectric.annotation.Config +import kotlin.reflect.KClass + +@RunWith(AndroidJUnit4::class) +@Config(application = TestApplication::class) +class AccountAbnormalitiesTest { + @Test + fun `account manager must be configured`() { + val crashReporter: CrashReporter = mock() + + // no account present + val accountAbnormalities = AccountAbnormalities(testContext, crashReporter) + + try { + accountAbnormalities.userRequestedLogout() + fail() + } catch (e: IllegalStateException) { + assertEquals("userRequestedLogout before account manager was configured", e.message) + } + + try { + accountAbnormalities.onAuthenticated(mock(), mock()) + fail() + } catch (e: IllegalStateException) { + assertEquals("onAuthenticated before account manager was configured", e.message) + } + + try { + accountAbnormalities.onLoggedOut() + fail() + } catch (e: IllegalStateException) { + assertEquals("onLoggedOut before account manager was configured", e.message) + } + + verifyZeroInteractions(crashReporter) + } + + @Test + fun `LogoutWithoutAuth detected`() = runBlocking { + val crashReporter: CrashReporter = mock() + val accountManager: FxaAccountManager = mock() + + val accountAbnormalities = AccountAbnormalities(testContext, crashReporter, this.coroutineContext) + accountAbnormalities.accountManagerInitializedAsync( + accountManager, + CompletableDeferred(Unit).also { it.complete(Unit) } + ).await() + + // Logout action must be preceded by auth. + accountAbnormalities.userRequestedLogout() + assertCaughtException(crashReporter, AbnormalFxaEvent.LogoutWithoutAuth::class) + } + + @Test + fun `OverlappingFxaLogoutRequest detected`() = runBlocking { + val crashReporter: CrashReporter = mock() + val accountManager: FxaAccountManager = mock() + + val accountAbnormalities = AccountAbnormalities(testContext, crashReporter, this.coroutineContext) + accountAbnormalities.accountManagerInitializedAsync( + accountManager, + CompletableDeferred(Unit).also { it.complete(Unit) } + ).await() + + accountAbnormalities.onAuthenticated(mock(), mock()) + // So far, so good. A regular logout request while being authenticated. + accountAbnormalities.userRequestedLogout() + verifyZeroInteractions(crashReporter) + + // We never saw a logout callback after previous logout request, so this is an overlapping request. + accountAbnormalities.userRequestedLogout() + assertCaughtException(crashReporter, AbnormalFxaEvent.OverlappingFxaLogoutRequest::class) + } + + @Test + fun `callback logout abnormalities detected`() = runBlocking { + val crashReporter: CrashReporter = mock() + val accountManager: FxaAccountManager = mock() + + val accountAbnormalities = AccountAbnormalities(testContext, crashReporter, this.coroutineContext) + accountAbnormalities.accountManagerInitializedAsync( + accountManager, + CompletableDeferred(Unit).also { it.complete(Unit) } + ).await() + + // User didn't request this logout. + accountAbnormalities.onLoggedOut() + assertCaughtException(crashReporter, AbnormalFxaEvent.UnexpectedFxaLogout::class) + } + + @Test + fun `login happy case + disappearing account detected`() = runBlocking { + val crashReporter: CrashReporter = mock() + val accountManager: FxaAccountManager = mock() + + val accountAbnormalities = AccountAbnormalities(testContext, crashReporter, this.coroutineContext) + accountAbnormalities.accountManagerInitializedAsync( + accountManager, + CompletableDeferred(Unit).also { it.complete(Unit) } + ).await() + + accountAbnormalities.onAuthenticated(mock(), mock()) + verifyZeroInteractions(crashReporter) + + // Pretend we restart, and instantiate a new middleware instance. + val accountAbnormalities2 = AccountAbnormalities(testContext, crashReporter, this.coroutineContext) + // mock accountManager doesn't have an account, but we expect it to have one since we + // were authenticated before our "restart". + accountAbnormalities2.accountManagerInitializedAsync( + accountManager, + CompletableDeferred(Unit).also { it.complete(Unit) } + ).await() + + assertCaughtException(crashReporter, AbnormalFxaEvent.MissingExpectedAccountAfterStartup::class) + } + + @Test + fun `logout happy case`() = runBlocking { + val crashReporter: CrashReporter = mock() + val accountManager: FxaAccountManager = mock() + + val accountAbnormalities = AccountAbnormalities(testContext, crashReporter, this.coroutineContext) + accountAbnormalities.accountManagerInitializedAsync( + accountManager, + CompletableDeferred(Unit).also { it.complete(Unit) } + ).await() + + // We saw an auth event, then user requested a logout. + accountAbnormalities.onAuthenticated(mock(), mock()) + accountAbnormalities.userRequestedLogout() + verifyZeroInteractions(crashReporter) + } + + private fun assertCaughtException(crashReporter: CrashReporter, type: KClass) { + val captor = argumentCaptor() + verify(crashReporter).submitCaughtException(captor.capture()) + assertEquals(type, captor.value::class) + } +} diff --git a/app/src/test/java/org/mozilla/fenix/components/BackgroundServicesTest.kt b/app/src/test/java/org/mozilla/fenix/components/BackgroundServicesTest.kt index b1262b5b2..31b277430 100644 --- a/app/src/test/java/org/mozilla/fenix/components/BackgroundServicesTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/BackgroundServicesTest.kt @@ -33,7 +33,7 @@ import org.mozilla.fenix.isInExperiment class BackgroundServicesTest { class TestableBackgroundServices( val context: Context - ) : BackgroundServices(context, mockk(), mockk()) { + ) : BackgroundServices(context, mockk(), mockk(), mockk()) { override fun makeAccountManager( context: Context, serverConfig: ServerConfig,