For #6490 - track abnormal FxA behaviour via Sentry
This commit is contained in:
parent
b1623cbba6
commit
40047315c2
|
@ -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<Unit>
|
||||||
|
): Deferred<Unit> {
|
||||||
|
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())
|
||||||
|
}
|
||||||
|
}
|
|
@ -25,6 +25,7 @@ import mozilla.components.feature.push.AutoPushSubscription
|
||||||
import mozilla.components.feature.push.PushConfig
|
import mozilla.components.feature.push.PushConfig
|
||||||
import mozilla.components.feature.push.PushSubscriptionObserver
|
import mozilla.components.feature.push.PushSubscriptionObserver
|
||||||
import mozilla.components.feature.push.PushType
|
import mozilla.components.feature.push.PushType
|
||||||
|
import mozilla.components.lib.crash.CrashReporter
|
||||||
import mozilla.components.service.fxa.DeviceConfig
|
import mozilla.components.service.fxa.DeviceConfig
|
||||||
import mozilla.components.service.fxa.ServerConfig
|
import mozilla.components.service.fxa.ServerConfig
|
||||||
import mozilla.components.service.fxa.SyncConfig
|
import mozilla.components.service.fxa.SyncConfig
|
||||||
|
@ -50,6 +51,7 @@ import java.util.FormatFlagsConversionMismatchException
|
||||||
@Mockable
|
@Mockable
|
||||||
class BackgroundServices(
|
class BackgroundServices(
|
||||||
private val context: Context,
|
private val context: Context,
|
||||||
|
crashReporter: CrashReporter,
|
||||||
historyStorage: PlacesHistoryStorage,
|
historyStorage: PlacesHistoryStorage,
|
||||||
bookmarkStorage: PlacesBookmarksStorage
|
bookmarkStorage: PlacesBookmarksStorage
|
||||||
) {
|
) {
|
||||||
|
@ -116,6 +118,8 @@ class BackgroundServices(
|
||||||
context.components.analytics.metrics
|
context.components.analytics.metrics
|
||||||
)
|
)
|
||||||
|
|
||||||
|
val accountAbnormalities = AccountAbnormalities(context, crashReporter)
|
||||||
|
|
||||||
private val pushAccountObserver by lazy { push?.let { PushAccountObserver(it) } }
|
private val pushAccountObserver by lazy { push?.let { PushAccountObserver(it) } }
|
||||||
|
|
||||||
val accountManager = makeAccountManager(context, serverConfig, deviceConfig, syncConfig)
|
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.
|
// Register a telemetry account observer to keep track of FxA auth metrics.
|
||||||
accountManager.register(telemetryAccountObserver)
|
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.
|
// Enable push if it's configured.
|
||||||
push?.let { autoPushFeature ->
|
push?.let { autoPushFeature ->
|
||||||
// Register the push account observer so we know how to update our push subscriptions.
|
// 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()
|
||||||
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
|
@ -15,7 +15,7 @@ import org.mozilla.fenix.utils.ClipboardHandler
|
||||||
@Mockable
|
@Mockable
|
||||||
class Components(private val context: Context) {
|
class Components(private val context: Context) {
|
||||||
val backgroundServices by lazy {
|
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 services by lazy { Services(context, backgroundServices.accountManager) }
|
||||||
val core by lazy { Core(context) }
|
val core by lazy { Core(context) }
|
||||||
|
|
|
@ -57,6 +57,8 @@ class SignOutFragment : BottomSheetDialogFragment() {
|
||||||
|
|
||||||
view.signOutDisconnect.setOnClickListener {
|
view.signOutDisconnect.setOnClickListener {
|
||||||
lifecycleScope.launch {
|
lifecycleScope.launch {
|
||||||
|
requireComponents
|
||||||
|
.backgroundServices.accountAbnormalities.userRequestedLogout()
|
||||||
accountManager.logoutAsync().await()
|
accountManager.logoutAsync().await()
|
||||||
}.invokeOnCompletion {
|
}.invokeOnCompletion {
|
||||||
if (!findNavController().popBackStack(R.id.settingsFragment, false)) {
|
if (!findNavController().popBackStack(R.id.settingsFragment, false)) {
|
||||||
|
|
|
@ -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 <T : AbnormalFxaEvent> assertCaughtException(crashReporter: CrashReporter, type: KClass<T>) {
|
||||||
|
val captor = argumentCaptor<AbnormalFxaEvent>()
|
||||||
|
verify(crashReporter).submitCaughtException(captor.capture())
|
||||||
|
assertEquals(type, captor.value::class)
|
||||||
|
}
|
||||||
|
}
|
|
@ -33,7 +33,7 @@ import org.mozilla.fenix.isInExperiment
|
||||||
class BackgroundServicesTest {
|
class BackgroundServicesTest {
|
||||||
class TestableBackgroundServices(
|
class TestableBackgroundServices(
|
||||||
val context: Context
|
val context: Context
|
||||||
) : BackgroundServices(context, mockk(), mockk()) {
|
) : BackgroundServices(context, mockk(), mockk(), mockk()) {
|
||||||
override fun makeAccountManager(
|
override fun makeAccountManager(
|
||||||
context: Context,
|
context: Context,
|
||||||
serverConfig: ServerConfig,
|
serverConfig: ServerConfig,
|
||||||
|
|
Loading…
Reference in New Issue
Block a user