From 78e83e90d424a7e48125fe82c6f18cbbef6592f7 Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Tue, 3 Nov 2020 12:33:15 -0500 Subject: [PATCH] Close #8846: Re-subscribe FxA push subscription on subscriptionExpired This is a workaround fix to re-enable Send Tab when we are notified about the subscriptionExpired flag. Until #7143 is fixed, we need to avoid calling `registrationRenewal` to avoid going into an unfixable state. In the context of the FxaPushSupportFeature, we know that our end goal is to call `push.subscribe(fxaPushScope)`, so we can by pass the `verifyConnection` call **assuming** that our native push client is always has the latest push token and knows how to invalidate the endpoint with the Autopush server. --- .../accounts/push/FxaPushSupportFeature.kt | 77 +++++++++++++++---- .../accounts/push/AccountObserverTest.kt | 40 ++++++++++ .../push/ConstellationObserverTest.kt | 75 ++++++++++++++++-- docs/changelog.md | 3 + 4 files changed, 176 insertions(+), 19 deletions(-) diff --git a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt index 712d9538dab..4c2312ccabc 100644 --- a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt +++ b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt @@ -2,6 +2,8 @@ * 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/. */ +@file:Suppress("LongParameterList") + package mozilla.components.feature.accounts.push import android.content.Context @@ -11,7 +13,8 @@ import androidx.lifecycle.ProcessLifecycleOwner import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch -import mozilla.components.concept.push.PushProcessor +import mozilla.components.concept.base.crash.Breadcrumb +import mozilla.components.concept.base.crash.CrashReporting import mozilla.components.concept.sync.AuthType import mozilla.components.concept.sync.ConstellationState import mozilla.components.concept.sync.Device @@ -42,6 +45,7 @@ internal const val PREF_FXA_SCOPE = "fxa_push_scope" * @param context The application Android context. * @param accountManager The FxaAccountManager. * @param pushFeature The [AutoPushFeature] if that is setup for observing push events. + * @param crashReporter Instance of `CrashReporting` to record unexpected caught exceptions. * @param owner the lifecycle owner for the observer. Defaults to [ProcessLifecycleOwner]. * @param autoPause whether to stop notifying the observer during onPause lifecycle events. * Defaults to false so that observers are always notified. @@ -50,6 +54,7 @@ class FxaPushSupportFeature( private val context: Context, accountManager: FxaAccountManager, pushFeature: AutoPushFeature, + private val crashReporter: CrashReporting? = null, owner: LifecycleOwner = ProcessLifecycleOwner.get(), autoPause: Boolean = false ) { @@ -85,6 +90,7 @@ class FxaPushSupportFeature( context, pushFeature, fxaPushScope, + crashReporter, owner, autoPause ) @@ -107,25 +113,42 @@ internal class AccountObserver( private val context: Context, private val push: AutoPushFeature, private val fxaPushScope: String, + private val crashReporter: CrashReporting?, private val lifecycleOwner: LifecycleOwner, private val autoPause: Boolean ) : SyncAccountObserver { private val logger = Logger(AccountObserver::class.java.simpleName) private val verificationDelegate = VerificationDelegate(context, push.config.disableRateLimit) - private val constellationObserver = ConstellationObserver(push, verificationDelegate) override fun onAuthenticated(account: OAuthAccount, authType: AuthType) { + + val constellationObserver = ConstellationObserver( + context = context, + push = push, + scope = fxaPushScope, + account = account, + verifier = verificationDelegate, + crashReporter = crashReporter + ) + // We need a new subscription only when we have a new account. // The subscription is removed when an account logs out. if (authType != AuthType.Existing && authType != AuthType.Recovered) { logger.debug("Subscribing for FxaPushScope ($fxaPushScope) events.") - push.subscribe(fxaPushScope) { subscription -> - CoroutineScope(Dispatchers.Main).launch { - account.deviceConstellation().setDevicePushSubscription(subscription.into()) - } - } + push.subscribe( + scope = fxaPushScope, + onSubscribeError = { e -> + crashReporter?.recordCrashBreadcrumb(Breadcrumb("Subscribing to FxA push failed at login.")) + logger.info("Subscribing to FxA push failed at login.", e) + }, + onSubscribe = { subscription -> + logger.info("Created a new subscription: $subscription") + CoroutineScope(Dispatchers.Main).launch { + account.deviceConstellation().setDevicePushSubscription(subscription.into()) + } + }) } // NB: can we just expose registerDeviceObserver on account manager? @@ -152,8 +175,12 @@ internal class AccountObserver( * when notified by the FxA server. See [Device.subscriptionExpired]. */ internal class ConstellationObserver( - private val push: PushProcessor, - private val verifier: VerificationDelegate + context: Context, + private val push: AutoPushFeature, + private val scope: String, + private val account: OAuthAccount, + private val verifier: VerificationDelegate = VerificationDelegate(context), + private val crashReporter: CrashReporting? ) : DeviceConstellationObserver { private val logger = Logger(ConstellationObserver::class.java.simpleName) @@ -166,21 +193,44 @@ internal class ConstellationObserver( // If our last check was recent (see: PERIODIC_INTERVAL_MILLISECONDS), we do nothing. val allowedToRenew = verifier.allowedToRenew() if (!updateSubscription || !allowedToRenew) { - logger.info("Short-circuiting onDevicesUpdate: " + - "updateSubscription($updateSubscription), allowedToRenew($allowedToRenew)") + logger.info( + "Short-circuiting onDevicesUpdate: " + + "updateSubscription($updateSubscription), allowedToRenew($allowedToRenew)" + ) return } else { logger.info("Proceeding to renew registration") } - logger.info("Renewing registration") - push.renewRegistration() + logger.info("We have been notified that our push subscription has expired; re-subscribing.") + push.subscribe(scope) { onSubscribe(constellation, it) } logger.info("Incrementing verifier") logger.info("Verifier state before: timestamp=${verifier.innerTimestamp}, count=${verifier.innerCount}") verifier.increment() logger.info("Verifier state after: timestamp=${verifier.innerTimestamp}, count=${verifier.innerCount}") } + + internal fun onSubscribe(constellation: ConstellationState, subscription: AutoPushSubscription) { + + logger.info("Created a new subscription: $subscription") + + val oldEndpoint = constellation.currentDevice?.subscription?.endpoint + if (subscription.endpoint == oldEndpoint) { + val exception = IllegalStateException( + "New push endpoint matches existing one", + Throwable("New endpoint: ${subscription.endpoint}\nOld endpoint: $oldEndpoint") + ) + + crashReporter?.submitCaughtException(exception) + + logger.warn("Push endpoints match!", exception) + } + + CoroutineScope(Dispatchers.Main).launch { + account.deviceConstellation().setDevicePushSubscription(subscription.into()) + } + } } /** @@ -261,6 +311,7 @@ internal class VerificationDelegate( @VisibleForTesting internal var innerCount: Int = 0 + @VisibleForTesting internal var innerTimestamp: Long = System.currentTimeMillis() diff --git a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt index c82c1347bef..3b01d76b78e 100644 --- a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt +++ b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt @@ -7,6 +7,7 @@ package mozilla.components.feature.accounts.push import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner import kotlinx.coroutines.runBlocking +import mozilla.components.concept.base.crash.CrashReporting import mozilla.components.concept.sync.AuthType import mozilla.components.concept.sync.DeviceConstellation import mozilla.components.concept.sync.OAuthAccount @@ -42,6 +43,7 @@ class AccountObserverTest { private val account: OAuthAccount = mock() private val constellation: DeviceConstellation = mock() private val config: PushConfig = mock() + private val crashReporter: CrashReporting = mock() @Before fun setup() { @@ -58,6 +60,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, lifecycleOwner, false ) @@ -81,6 +84,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -104,6 +108,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -123,6 +128,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -140,6 +146,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -161,6 +168,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -173,12 +181,32 @@ class AccountObserverTest { Unit } + @Test + fun `notify crash reporter on subscription error`() = runBlocking { + val observer = AccountObserver( + testContext, + pushFeature, + pushScope, + crashReporter, + mock(), + false + ) + + whenSubscribeError() + + observer.onAuthenticated(account, AuthType.Signin) + + verify(crashReporter).recordCrashBreadcrumb(any()) + Unit + } + @Test fun `feature and service invoked on logout`() { val observer = AccountObserver( testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -194,6 +222,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -221,4 +250,15 @@ class AccountObserverTest { ) } } + + @Suppress("UNCHECKED_CAST") + private fun whenSubscribeError(): OngoingStubbing? { + return `when`(pushFeature.subscribe(any(), nullable(), any(), any())).thenAnswer { + + // Invoke the `onSubscribe` lambda with a fake subscription. + (it.arguments[2] as ((Exception) -> Unit)).invoke( + IllegalStateException("test") + ) + } + } } \ No newline at end of file diff --git a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt index 7cefcef13e0..c11845861d1 100644 --- a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt +++ b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt @@ -7,10 +7,24 @@ package mozilla.components.feature.accounts.push import android.content.Context -import mozilla.components.concept.push.PushProcessor +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.test.TestCoroutineDispatcher +import kotlinx.coroutines.test.setMain +import mozilla.components.concept.base.crash.CrashReporting import mozilla.components.concept.sync.ConstellationState import mozilla.components.concept.sync.Device +import mozilla.components.concept.sync.DeviceConstellation +import mozilla.components.concept.sync.DevicePushSubscription +import mozilla.components.concept.sync.OAuthAccount +import mozilla.components.feature.push.AutoPushFeature +import mozilla.components.feature.push.AutoPushSubscription +import mozilla.components.support.test.any +import mozilla.components.support.test.eq import mozilla.components.support.test.mock +import mozilla.components.support.test.rule.MainCoroutineRule +import org.junit.Before +import org.junit.Ignore +import org.junit.Rule import org.junit.Test import org.mockito.Mockito.`when` import org.mockito.Mockito.verify @@ -18,15 +32,26 @@ import org.mockito.Mockito.verifyZeroInteractions class ConstellationObserverTest { - private val push: PushProcessor = mock() + private val push: AutoPushFeature = mock() private val verifier: VerificationDelegate = mock() private val state: ConstellationState = mock() private val device: Device = mock() private val context: Context = mock() + private val account: OAuthAccount = mock() + private val crashReporter: CrashReporting = mock() + private val testDispatcher = TestCoroutineDispatcher() + + @get:Rule + val coroutinesTestRule = MainCoroutineRule(testDispatcher) + + @Before + fun setup() { + Dispatchers.setMain(testDispatcher) + } @Test fun `do nothing if subscription has not expired`() { - val observer = ConstellationObserver(push, verifier) + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) observer.onDevicesUpdate(state) @@ -42,7 +67,7 @@ class ConstellationObserverTest { @Test fun `do nothing if verifier is false`() { - val observer = ConstellationObserver(push, verifier) + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) observer.onDevicesUpdate(state) @@ -62,8 +87,9 @@ class ConstellationObserverTest { } @Test + @Ignore("Disabling the test until we revert the changes from #8846 and fix #7143") fun `invoke registration renewal`() { - val observer = ConstellationObserver(push, verifier) + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) `when`(state.currentDevice).thenReturn(device) `when`(device.subscriptionExpired).thenReturn(true) @@ -74,4 +100,41 @@ class ConstellationObserverTest { verify(push).renewRegistration() verify(verifier).increment() } -} \ No newline at end of file + + /** + * Remove this test in the future. See [invoke registration renewal] test. + */ + @Test + fun `re-subscribe for push in onDevicesUpdate`() { + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) + + `when`(state.currentDevice).thenReturn(device) + `when`(device.subscriptionExpired).thenReturn(true) + `when`(verifier.allowedToRenew()).thenReturn(true) + + observer.onDevicesUpdate(state) + + verify(push).subscribe(eq("testScope"), any(), any(), any()) + verify(verifier).increment() + } + + @Test + fun `notify crash reporter if old and new subscription matches`() { + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) + val constellation: DeviceConstellation = mock() + val state: ConstellationState = mock() + val device: Device = mock() + val subscription: DevicePushSubscription = mock() + + `when`(account.deviceConstellation()).thenReturn(constellation) + `when`(state.currentDevice).thenReturn(device) + `when`(device.subscription).thenReturn(subscription) + `when`(subscription.endpoint).thenReturn("https://example.com") + + observer.onSubscribe(state, testSubscription()) + + verify(crashReporter).submitCaughtException(any()) + } + + private fun testSubscription() = AutoPushSubscription(scope = "testScope", endpoint = "https://example.com", publicKey = "", authKey = "", appServerKey = null) +} diff --git a/docs/changelog.md b/docs/changelog.md index 304c3ea491a..8c4a3c1a66f 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -15,6 +15,9 @@ permalink: /changelog/ * **feature-sitepermissions** * 🚒 Bug fixed [issue #8943](https://github.com/mozilla-mobile/android-components/issues/8943) Refactor `SwipeRefreshFeature` to not use `EngineSession.Observer`, this result in multiple site permissions bugs getting fixed see [fenix#8987](https://github.com/mozilla-mobile/fenix/issues/8987) and [fenix#16411](https://github.com/mozilla-mobile/fenix/issues/16411). +* **feature-accounts-push** + * ⚠️ `FxaPushSupportFeature` now re-subscribes to push instead of triggering the registration renewal process - this is a temporary workaround and will be removed in the future. + # 66.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v65.0.0...v66.0.0)