From b9df8f969a6f788a1d203d8e3fa69160fa7f7f75 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 18 Sep 2025 10:48:39 +0200 Subject: [PATCH] Make PushData.clientSecret mandatory. Also do not restore the last session as a fallback, it can lead to error in a multi account context, or even when a ghost pusher send a Push. --- .../push/impl/push/DefaultPushHandler.kt | 27 ++------ .../push/impl/push/DefaultPushHandlerTest.kt | 62 +------------------ .../libraries/pushproviders/api/PushData.kt | 2 +- .../firebase/PushDataFirebase.kt | 3 +- .../firebase/FirebasePushParserTest.kt | 6 ++ 5 files changed, 16 insertions(+), 84 deletions(-) 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 index d251948992..3a4cf4d6aa 100644 --- 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 index 91c1293220..e482385b3d 100644 --- 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, Result>>> { _, _ -> - 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, Unit> {} - val incrementPushCounterResult = lambdaRecorder {} - val aPushData = PushData( - eventId = AN_EVENT_ID, - roomId = A_ROOM_ID, - unread = 0, - clientSecret = A_SECRET, - ) - val onPushReceivedResult = lambdaRecorder { _, _, _, _, _, _, _ -> } - 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 { _, _, _, _, 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 index e08ab1c18f..6000f74a95 100644 --- 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 index 7bc1515332..fb33ab9c1f 100644 --- 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 index 71848fc4df..49ed5bc6d4 100644 --- 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()