Merge #5232

5232: Closes #5134 - Defensive PushType lookup from the incoming channelId r=jonalmeida a=grigoryk

`channelId` comes to us from the "outside world", so to speak - as part of an
incoming push message. Don't crash in case we couldn't map it to a PushType.

@jonalmeida what do you think? I reckon we need to do something like this regardless of migration crashes - but it's particularly annoying with those :)



Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt

@@ -155,8 +155,15 @@ class AutoPushFeature(
      * New encrypted messages received from a supported push messaging service.
      */
     override fun onMessageReceived(message: EncryptedPushMessage) {
+        val type = DeliveryManager.serviceForChannelId(message.channelId)
+        // We may have received a message which specifies `channelId` that we don't know about.
+        // Most likely it was directed at an older version of Firefox running on this device, which still has a valid
+        // FxA device record. Shouldn't be any harm in dropping the message on the floor.
+        if (type == null) {
+            logger.error("Unknown channelID: ${message.channelId}")
+            return
+        }
         job = scope.launchAndTry {
-            val type = DeliveryManager.serviceForChannelId(message.channelId)
             DeliveryManager.with(connection) {
                 logger.info("New push message decrypted.")
                 val decrypted = decrypt(
@@ -378,8 +385,11 @@ interface PushSubscriptionObserver {
  * Application Service implementation.
  */
 internal object DeliveryManager {
-    fun serviceForChannelId(channelId: String): PushType {
-        return PushType.values().first { it.toChannelId() == channelId }
+    /**
+     * @return A [PushType] for the provided [channelId], if one exists.
+     */
+    fun serviceForChannelId(channelId: String): PushType? {
+        return PushType.values().firstOrNull { it.toChannelId() == channelId }
     }
 
     /**
@@ -437,7 +447,7 @@ data class PushConfig(
  * A helper to convert the internal data class.
  */
 internal fun SubscriptionResponse.toPushSubscription(): AutoPushSubscription {
-    val type = DeliveryManager.serviceForChannelId(channelID)
+    val type = DeliveryManager.serviceForChannelId(channelID)!!
     return AutoPushSubscription(
         type = type,
         endpoint = subscriptionInfo.endpoint,

components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt

@@ -39,6 +39,7 @@ import org.mockito.Mockito.spy
 import org.mockito.Mockito.times
 import org.mockito.Mockito.verify
 import org.mockito.Mockito.verifyNoMoreInteractions
+import org.mockito.Mockito.verifyZeroInteractions
 
 @ExperimentalCoroutinesApi
 @RunWith(AndroidJUnit4::class)
@@ -153,6 +154,33 @@ class AutoPushFeatureTest {
         feature.onMessageReceived(encryptedMessage)
     }
 
+    @Test
+    fun `onMessageReceived handles unknown channelId`() = runBlockingTest {
+        val connection: PushConnection = mock()
+        val encryptedMessage: EncryptedPushMessage = mock()
+        val owner: LifecycleOwner = mock()
+        val lifecycle: Lifecycle = mock()
+        whenever(owner.lifecycle).thenReturn(lifecycle)
+        whenever(lifecycle.currentState).thenReturn(Lifecycle.State.STARTED)
+        whenever(connection.isInitialized()).thenReturn(true)
+        whenever(encryptedMessage.channelId).thenReturn("whatisachannelidanyway")
+        whenever(connection.decrypt(any(), any(), any(), any(), any())).thenReturn("test".toByteArray())
+
+        val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection))
+
+        val pushServiceObserver: Bus.Observer<PushType, String> = mock()
+        val webpushServiceObserver: Bus.Observer<PushType, String> = mock()
+        feature.registerForPushMessages(PushType.Services, pushServiceObserver)
+        feature.registerForPushMessages(PushType.WebPush, webpushServiceObserver)
+
+        // Shouldn't crash!
+        feature.onMessageReceived(encryptedMessage)
+
+        // Observers don't need to know about garbage we received.
+        verifyZeroInteractions(pushServiceObserver)
+        verifyZeroInteractions(webpushServiceObserver)
+    }
+
     @Test
     fun `subscribeForType notifies observers`() = runBlockingTest {
         val connection: PushConnection = spy(TestPushConnection(true))

components/feature/push/src/test/java/mozilla/components/feature/push/DeliveryManagerTest.kt

@@ -7,6 +7,7 @@ package mozilla.components.feature.push
 import mozilla.components.support.test.mock
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.mockito.Mockito.`when`
@@ -19,9 +20,9 @@ class DeliveryManagerTest {
         assertEquals(PushType.Services, pushType)
     }
 
-    @Test(expected = NoSuchElementException::class)
-    fun `exception thrown if serviceType not found`() {
-        DeliveryManager.serviceForChannelId("992a0f0542383f1ea5ef51b7cf4ea6c4")
+    @Test
+    fun `unknown channelIDs handled gracefully`() {
+        assertNull(DeliveryManager.serviceForChannelId("992a0f0542383f1ea5ef51b7cf4ea6c4"))
     }
 
     @Test