Merge #6572

6572: Close #6456: Allow empty body in AutoPush messages. r=rocketsroger a=jonalmeida

Allows nullable messages to be delivered to all observers. This was partly handled from the consumer-facing public API, but nullable messages were dropped when they were received from the push services.
Co-authored-by: Jonathan Almeida <jalmeida@mozilla.com>

components/concept/push/src/main/java/mozilla/components/concept/push/PushProcessor.kt

@@ -70,7 +70,7 @@ interface PushProcessor {
  */
 data class EncryptedPushMessage(
     val channelId: String,
-    val body: String,
+    val body: String?,
     val encoding: String,
     val salt: String = "",
     val cryptoKey: String = "" // diffie–hellman key
@@ -84,7 +84,7 @@ data class EncryptedPushMessage(
          */
         operator fun invoke(
             channelId: String,
-            body: String,
+            body: String?,
             encoding: String,
             salt: String? = null,
             cryptoKey: String? = null

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

@@ -7,8 +7,8 @@ package mozilla.components.feature.push
 import androidx.annotation.GuardedBy
 import androidx.annotation.VisibleForTesting
 import mozilla.appservices.push.BridgeType
+import mozilla.appservices.push.GeneralError
 import mozilla.appservices.push.PushAPI
-import mozilla.appservices.push.PushError
 import mozilla.appservices.push.PushManager
 import mozilla.appservices.push.PushSubscriptionChanged as SubscriptionChanged
 import mozilla.appservices.push.SubscriptionResponse
@@ -72,11 +72,11 @@ interface PushConnection : Closeable {
      */
     suspend fun decryptMessage(
         channelId: String,
-        body: String,
+        body: String?,
         encoding: String = "",
         salt: String = "",
         cryptoKey: String = ""
-    ): Pair<PushScope, ByteArray>?
+    ): DecryptedMessage?
 
     /**
      * Checks if the native Push API has already been initialized.
@@ -171,9 +171,7 @@ internal class RustPushConnection(
         // This call will fail if we haven't 'subscribed' yet.
         return try {
             pushApi.update(token)
-        } catch (e: PushError) {
-            // Once we get GeneralError, let's catch that instead:
-            // https://github.com/mozilla/application-services/issues/2541
+        } catch (e: GeneralError) {
             val fakeChannelId = "fake".toChannelId()
             // It's possible that we have a race (on a first run) between 'subscribing' and setting a token.
             // 'update' expects that we've called 'subscribe' (which would obtain a 'uaid' from an autopush
@@ -198,11 +196,11 @@ internal class RustPushConnection(
     @GuardedBy("this")
     override suspend fun decryptMessage(
         channelId: String,
-        body: String,
+        body: String?,
         encoding: String,
         salt: String,
         cryptoKey: String
-    ): Pair<PushScope, ByteArray>? = synchronized(this) {
+    ): DecryptedMessage? = synchronized(this) {
         val pushApi = api
         check(pushApi != null) { "Rust API is not initiated; updateToken hasn't been called yet." }
 
@@ -210,6 +208,11 @@ internal class RustPushConnection(
         val scope = pushApi.dispatchInfoForChid(channelId)?.scope
 
         scope?.let {
+
+            if (body == null) {
+                return DecryptedMessage(scope, null)
+            }
+
             val data = pushApi.decrypt(
                 channelID = channelId,
                 body = body,
@@ -218,7 +221,7 @@ internal class RustPushConnection(
                 dh = cryptoKey
             )
 
-            return Pair(scope, data)
+            return DecryptedMessage(scope, data)
         }
     }
 
@@ -277,3 +280,33 @@ internal fun SubscriptionChanged.toPushSubscriptionChanged() = AutoPushSubscript
     scope = scope,
     channelId = channelID
 )
+
+/**
+ * Represents a decrypted push message for notifying observers of the [scope].
+ */
+data class DecryptedMessage(val scope: PushScope, val message: ByteArray?) {
+
+    // Generated; do not edit.
+    override fun equals(other: Any?): Boolean {
+        if (this === other) return true
+        if (javaClass != other?.javaClass) return false
+
+        other as DecryptedMessage
+
+        if (scope != other.scope) return false
+        if (message != null) {
+            if (other.message == null) return false
+            if (!message.contentEquals(other.message)) return false
+        } else if (other.message != null) return false
+
+        return true
+    }
+
+    // Generated; do not edit.
+    @Suppress("MagicNumber")
+    override fun hashCode(): Int {
+        var result = scope.hashCode()
+        result = 31 * result + (message?.contentHashCode() ?: 0)
+        return result
+    }
+}

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

@@ -141,7 +141,7 @@ class AutoPushFeatureTest {
         whenever(encryptedMessage.channelId).thenReturn("992a0f0542383f1ea5ef51b7cf4ae6c4")
         whenever(connection.decryptMessage(any(), any(), any(), any(), any()))
             .thenReturn(null) // If we get null, we shouldn't notify observers.
-            .thenReturn("testScope" to "test".toByteArray())
+            .thenReturn(DecryptedMessage("testScope", "test".toByteArray()))
 
         val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection))
 
@@ -347,13 +347,11 @@ class AutoPushFeatureTest {
 
         override suspend fun decryptMessage(
             channelId: String,
-            body: String,
+            body: String?,
             encoding: String,
             salt: String,
             cryptoKey: String
-        ): Pair<PushScope, ByteArray>? {
-            TODO("not implemented")
-        }
+        ): DecryptedMessage? = null
 
         override fun isInitialized() = init
 

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

@@ -11,6 +11,7 @@ import mozilla.appservices.push.SubscriptionInfo
 import mozilla.appservices.push.SubscriptionResponse
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
 import org.junit.Test
 
 class ConnectionKtTest {
@@ -60,4 +61,40 @@ class ConnectionKtTest {
         assertEquals(response.channelID, sub.channelId)
         assertEquals(response.scope, sub.scope)
     }
+
+    @Test
+    fun `decrypted message equals`() {
+        val message1 = DecryptedMessage("test", "message1".toByteArray())
+        val message2 = DecryptedMessage("test", "message2".toByteArray())
+
+        assertTrue(message1 != message2)
+
+        val message3 = DecryptedMessage("test", "message".toByteArray())
+        val message4 = DecryptedMessage("test", null)
+
+        assertTrue(message3 != message4)
+
+        val message5 = DecryptedMessage("test", null)
+        val message6 = DecryptedMessage("test", "message".toByteArray())
+
+        assertTrue(message5 != message6)
+
+        val message7 = DecryptedMessage("test", "message".toByteArray())
+        val message8 = DecryptedMessage("test", "message".toByteArray())
+
+        assertTrue(message7 == message8)
+    }
+
+    @Test
+    fun `decrypted message hashcode`() {
+        val message1 = DecryptedMessage("test", "message1".toByteArray())
+        val message2 = DecryptedMessage("test", "message2".toByteArray())
+
+        assertTrue(message1.hashCode() != message2.hashCode())
+
+        val message3 = DecryptedMessage("test", "message".toByteArray())
+        val message4 = DecryptedMessage("test", "message".toByteArray())
+
+        assertTrue(message3.hashCode() == message4.hashCode())
+    }
 }

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

@@ -13,6 +13,7 @@ import mozilla.appservices.push.SubscriptionInfo
 import mozilla.appservices.push.SubscriptionResponse
 import mozilla.components.support.test.eq
 import mozilla.components.support.test.mock
+import mozilla.components.support.test.nullable
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertNotNull
@@ -20,7 +21,6 @@ import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Ignore
 import org.junit.Test
-import org.mockito.ArgumentMatchers.nullable
 import org.mockito.Mockito.`when`
 import org.mockito.Mockito.anyString
 import org.mockito.Mockito.never
@@ -83,7 +83,7 @@ class RustPushConnectionTest {
 
         connection.api = api
 
-        `when`(api.subscribe(anyString(), anyString(), nullable(String::class.java))).thenReturn(response)
+        `when`(api.subscribe(anyString(), anyString(), nullable())).thenReturn(response)
 
         runBlocking {
             val sub = connection.subscribe("123")
@@ -94,7 +94,7 @@ class RustPushConnectionTest {
             assertEquals("https://foo", sub.endpoint)
         }
 
-        verify(api).subscribe(anyString(), anyString(), nullable(String::class.java))
+        verify(api).subscribe(anyString(), anyString(), nullable())
     }
 
     @Test(expected = IllegalStateException::class)
@@ -201,6 +201,31 @@ class RustPushConnectionTest {
         verify(api).decrypt(anyString(), anyString(), eq("enc"), eq("salt"), eq("key"))
     }
 
+    @Test
+    fun `empty body decrypts nothing`() {
+        val connection = createConnection()
+        val api: PushAPI = mock()
+        val dispatchInfo: DispatchInfo = mock()
+        connection.api = api
+
+        runBlocking {
+            connection.decryptMessage("123", null)
+        }
+
+        verify(api, never()).decrypt(anyString(), anyString(), eq(""), eq(""), eq(""))
+
+        `when`(api.dispatchInfoForChid(anyString())).thenReturn(dispatchInfo)
+        `when`(dispatchInfo.scope).thenReturn("test")
+
+        runBlocking {
+            val (scope, message) = connection.decryptMessage("123", null)!!
+            assertEquals("test", scope)
+            assertNull(message)
+        }
+
+        verify(api, never()).decrypt(anyString(), nullable(), eq(""), eq(""), eq(""))
+    }
+
     @Test(expected = IllegalStateException::class)
     fun `close throws if API is not initialized first`() {
         val connection = createConnection()

components/lib/push-amazon/src/main/java/mozilla/components/lib/push/amazon/AbstractAmazonPushService.kt

@@ -59,8 +59,8 @@ abstract class AbstractAmazonPushService : ADMMessageHandlerBase("AbstractAmazon
             val message = try {
                 EncryptedPushMessage(
                     channelId = it.getStringWithException("chid"),
-                    body = it.getStringWithException("body"),
                     encoding = it.getStringWithException("con"),
+                    body = it.getString("body"),
                     salt = it.getString("enc"),
                     cryptoKey = it.getString("cryptokey")
                 )

components/lib/push-amazon/src/test/java/mozilla/components/lib/push/amazon/AbstractAmazonPushServiceTest.kt

@@ -102,7 +102,7 @@ class AbstractAmazonPushServiceTest {
     fun `malformed message exception handled with the processor`() {
         val messageIntent = Intent()
         val bundleExtra = Bundle()
-        bundleExtra.putString("body", "contents")
+        bundleExtra.putString("con", "qu2to2")
 
         val captor = argumentCaptor<PushError>()
         messageIntent.putExtras(bundleExtra)

components/lib/push-firebase/src/main/java/mozilla/components/lib/push/firebase/AbstractFirebasePushService.kt

@@ -53,8 +53,8 @@ abstract class AbstractFirebasePushService(
             val message = try {
                 EncryptedPushMessage(
                     channelId = it.data.getValue(MESSAGE_KEY_CHANNEL_ID),
-                    body = it.data.getValue(MESSAGE_KEY_BODY),
                     encoding = it.data.getValue(MESSAGE_KEY_ENCODING),
+                    body = it.data[MESSAGE_KEY_BODY],
                     salt = it.data[MESSAGE_KEY_SALT],
                     cryptoKey = it.data[MESSAGE_KEY_CRYPTO_KEY]
                 )

components/lib/push-firebase/src/test/java/mozilla/components/lib/push/firebase/AbstractFirebasePushServiceTest.kt

@@ -77,8 +77,8 @@ class AbstractFirebasePushServiceTest {
         val remoteMessage: RemoteMessage = mock()
         val data = mapOf(
             "chid" to "1234",
-            "con" to "encoding",
             "enc" to "salt",
+            "body" to "oo3j532j4",
             "cryptokey" to "dh256"
         )
         val captor = argumentCaptor<PushError>()

docs/changelog.md

@@ -104,6 +104,9 @@ permalink: /changelog/
 * **browser-session**
   * ⚠️ **This is a breaking change**: `SessionManager.runWithSessionIdOrSelected` now returns the result from the `block` it executes. This is consistent with `runWithSession`.
 
+* **feature-push**
+  * Allow nullable AutoPush messages to be delivered to observers.
+
 # 37.0.0
 
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v36.0.0...v37.0.0)