Merge pull request #5369 from element-hq/feature/bma/requireClientSecret

bmarty · web-flow · commit b5e535029f5d · 2025-09-19T09:40:25.000+02:00
Make PushData.clientSecret mandatory.
diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt
@@ -16,7 +16,6 @@ import io.element.android.features.call.api.ElementCallEntryPoint
 import io.element.android.libraries.core.log.logger.LoggerTag
 import io.element.android.libraries.core.meta.BuildMeta
 import io.element.android.libraries.di.annotations.AppCoroutineScope
-import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService
 import io.element.android.libraries.matrix.api.exception.NotificationResolverException
 import io.element.android.libraries.push.impl.history.PushHistoryService
 import io.element.android.libraries.push.impl.history.onDiagnosticPush
@@ -58,7 +57,6 @@ class DefaultPushHandler(
     private val userPushStoreFactory: UserPushStoreFactory,
     private val pushClientSecret: PushClientSecret,
     private val buildMeta: BuildMeta,
-    private val matrixAuthenticationService: MatrixAuthenticationService,
     private val diagnosticPushHandler: DiagnosticPushHandler,
     private val elementCallEntryPoint: ElementCallEntryPoint,
     private val notificationChannels: NotificationChannels,
@@ -241,32 +239,15 @@ class DefaultPushHandler(
             } else {
                 Timber.tag(loggerTag.value).d("## handleInternal()")
             }
-            val clientSecret = pushData.clientSecret
-            // clientSecret should not be null. If this happens, restore default session
-            var reason = if (clientSecret == null) "No client secret" else ""
-            val userId = clientSecret?.let {
-                // Get userId from client secret
-                pushClientSecret.getUserIdFromSecret(clientSecret).also {
-                    if (it == null) {
-                        reason = "Unable to get userId from client secret"
-                    }
-                }
-            }
-                ?: run {
-                    matrixAuthenticationService.getLatestSessionId().also {
-                        if (it == null) {
-                            if (reason.isNotEmpty()) reason += " - "
-                            reason += "Unable to get latest sessionId"
-                        }
-                    }
-                }
+            // Get userId from client secret
+            val userId = pushClientSecret.getUserIdFromSecret(pushData.clientSecret)
             if (userId == null) {
-                Timber.w("Unable to get a session")
+                Timber.w("Unable to get userId from client secret")
                 pushHistoryService.onUnableToRetrieveSession(
                     providerInfo = providerInfo,
                     eventId = pushData.eventId,
                     roomId = pushData.roomId,
-                    reason = reason,
+                    reason = "Unable to get userId from client secret",
                 )
                 return
             }
diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandlerTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandlerTest.kt
@@ -14,7 +14,6 @@ import com.google.common.truth.Truth.assertThat
 import io.element.android.features.call.api.CallType
 import io.element.android.features.call.test.FakeElementCallEntryPoint
 import io.element.android.libraries.core.meta.BuildMeta
-import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService
 import io.element.android.libraries.matrix.api.core.EventId
 import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.api.core.SessionId
@@ -28,7 +27,6 @@ import io.element.android.libraries.matrix.test.A_ROOM_ID
 import io.element.android.libraries.matrix.test.A_SECRET
 import io.element.android.libraries.matrix.test.A_SESSION_ID
 import io.element.android.libraries.matrix.test.A_USER_ID
-import io.element.android.libraries.matrix.test.auth.FakeMatrixAuthenticationService
 import io.element.android.libraries.matrix.test.core.aBuildMeta
 import io.element.android.libraries.push.impl.history.FakePushHistoryService
 import io.element.android.libraries.push.impl.history.PushHistoryService
@@ -181,7 +179,7 @@ class DefaultPushHandlerTest {
         }
 
     @Test
-    fun `when PushData is received, but client secret is not known, fallback the latest session`() =
+    fun `when PushData is received, but client secret is not known, nothing happen`() =
         runTest {
             val aNotifiableMessageEvent = aNotifiableMessageEvent()
             val notifiableEventResult =
@@ -207,58 +205,6 @@ class DefaultPushHandlerTest {
                 pushClientSecret = FakePushClientSecret(
                     getUserIdFromSecretResult = { null }
                 ),
-                matrixAuthenticationService = FakeMatrixAuthenticationService().apply {
-                    getLatestSessionIdLambda = { A_USER_ID }
-                },
-                incrementPushCounterResult = incrementPushCounterResult,
-                pushHistoryService = pushHistoryService,
-            )
-            defaultPushHandler.handle(aPushData, A_PUSHER_INFO)
-
-            advanceTimeBy(300.milliseconds)
-
-            incrementPushCounterResult.assertions()
-                .isCalledOnce()
-            notifiableEventResult.assertions()
-                .isCalledOnce()
-                .with(value(A_USER_ID), any())
-            onNotifiableEventsReceived.assertions()
-                .isCalledOnce()
-                .with(value(listOf(aNotifiableMessageEvent)))
-            onPushReceivedResult.assertions()
-                .isCalledOnce()
-        }
-
-    @Test
-    fun `when PushData is received, but client secret is not known, and there is no latest session, nothing happen`() =
-        runTest {
-            val aNotifiableMessageEvent = aNotifiableMessageEvent()
-            val notifiableEventResult =
-                lambdaRecorder<SessionId, List<NotificationEventRequest>, Result<Map<NotificationEventRequest, Result<ResolvedPushEvent>>>> { _, _ ->
-                    val request = NotificationEventRequest(A_SESSION_ID, A_ROOM_ID, AN_EVENT_ID, A_PUSHER_INFO)
-                    Result.success(mapOf(request to Result.success(ResolvedPushEvent.Event(aNotifiableMessageEvent))))
-                }
-            val onNotifiableEventsReceived = lambdaRecorder<List<NotifiableEvent>, Unit> {}
-            val incrementPushCounterResult = lambdaRecorder<Unit> {}
-            val aPushData = PushData(
-                eventId = AN_EVENT_ID,
-                roomId = A_ROOM_ID,
-                unread = 0,
-                clientSecret = A_SECRET,
-            )
-            val onPushReceivedResult = lambdaRecorder<String, EventId?, RoomId?, SessionId?, Boolean, Boolean, String?, Unit> { _, _, _, _, _, _, _ -> }
-            val pushHistoryService = FakePushHistoryService(
-                onPushReceivedResult = onPushReceivedResult,
-            )
-            val defaultPushHandler = createDefaultPushHandler(
-                onNotifiableEventsReceived = onNotifiableEventsReceived,
-                notifiableEventsResult = notifiableEventResult,
-                pushClientSecret = FakePushClientSecret(
-                    getUserIdFromSecretResult = { null }
-                ),
-                matrixAuthenticationService = FakeMatrixAuthenticationService().apply {
-                    getLatestSessionIdLambda = { null }
-                },
                 incrementPushCounterResult = incrementPushCounterResult,
                 pushHistoryService = pushHistoryService,
             )
@@ -655,8 +601,8 @@ class DefaultPushHandlerTest {
         var receivedFallbackEvent = false
         val onPushReceivedResult =
             lambdaRecorder<String, EventId?, RoomId?, SessionId?, Boolean, Boolean, String?, Unit> { _, _, _, _, isResolved, _, comment ->
-            receivedFallbackEvent = !isResolved && comment == "Unable to resolve event: ${aNotifiableFallbackEvent.cause}"
-        }
+                receivedFallbackEvent = !isResolved && comment == "Unable to resolve event: ${aNotifiableFallbackEvent.cause}"
+            }
         val pushHistoryService = FakePushHistoryService(
             onPushReceivedResult = onPushReceivedResult,
         )
@@ -694,7 +640,6 @@ class DefaultPushHandlerTest {
         userPushStore: UserPushStore = FakeUserPushStore(),
         pushClientSecret: PushClientSecret = FakePushClientSecret(),
         buildMeta: BuildMeta = aBuildMeta(),
-        matrixAuthenticationService: MatrixAuthenticationService = FakeMatrixAuthenticationService(),
         diagnosticPushHandler: DiagnosticPushHandler = DiagnosticPushHandler(),
         elementCallEntryPoint: FakeElementCallEntryPoint = FakeElementCallEntryPoint(),
         notificationChannels: FakeNotificationChannels = FakeNotificationChannels(),
@@ -712,7 +657,6 @@ class DefaultPushHandlerTest {
             userPushStoreFactory = FakeUserPushStoreFactory { userPushStore },
             pushClientSecret = pushClientSecret,
             buildMeta = buildMeta,
-            matrixAuthenticationService = matrixAuthenticationService,
             diagnosticPushHandler = diagnosticPushHandler,
             elementCallEntryPoint = elementCallEntryPoint,
             notificationChannels = notificationChannels,
diff --git a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushData.kt b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushData.kt
@@ -22,5 +22,5 @@ data class PushData(
     val eventId: EventId,
     val roomId: RoomId,
     val unread: Int?,
-    val clientSecret: String?,
+    val clientSecret: String,
 )
diff --git a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/PushDataFirebase.kt b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/PushDataFirebase.kt
@@ -34,10 +34,11 @@ data class PushDataFirebase(
 fun PushDataFirebase.toPushData(): PushData? {
     val safeEventId = eventId?.let(::EventId) ?: return null
     val safeRoomId = roomId?.let(::RoomId) ?: return null
+    val safeClientSecret = clientSecret ?: return null
     return PushData(
         eventId = safeEventId,
         roomId = safeRoomId,
         unread = unread,
-        clientSecret = clientSecret,
+        clientSecret = safeClientSecret,
     )
 }
diff --git a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushParserTest.kt b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushParserTest.kt
@@ -59,6 +59,12 @@ class FirebasePushParserTest {
         assertThrowsInDebug { pushParser.parse(FIREBASE_PUSH_DATA.mutate("event_id", "")) }
     }
 
+    @Test
+    fun `test empty client secret`() {
+        val pushParser = FirebasePushParser()
+        assertThat(pushParser.parse(FIREBASE_PUSH_DATA.mutate("cs", null))).isNull()
+    }
+
     @Test
     fun `test invalid eventId`() {
         val pushParser = FirebasePushParser()

Original file line number	Diff line number	Diff line change
`@@ -22,5 +22,5 @@ data class PushData(`
`22`	`22`	`val eventId: EventId,`
`23`	`23`	`val roomId: RoomId,`
`24`	`24`	`val unread: Int?,`
`25`		`- val clientSecret: String?,`
	`25`	`+ val clientSecret: String,`
`26`	`26`	`)`
Original file line number	Diff line number	Diff line change
`@@ -34,10 +34,11 @@ data class PushDataFirebase(`
`34`	`34`	`fun PushDataFirebase.toPushData(): PushData? {`
`35`	`35`	`val safeEventId = eventId?.let(::EventId) ?: return null`
`36`	`36`	`val safeRoomId = roomId?.let(::RoomId) ?: return null`
	`37`	`+ val safeClientSecret = clientSecret ?: return null`
`37`	`38`	`return PushData(`
`38`	`39`	`eventId = safeEventId,`
`39`	`40`	`roomId = safeRoomId,`
`40`	`41`	`unread = unread,`
`41`		`- clientSecret = clientSecret,`
	`42`	`+ clientSecret = safeClientSecret,`
`42`	`43`	`)`
`43`	`44`	`}`