From ce23135e4acced33db2c955a565975f6cc714980 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 23 May 2024 09:51:56 +0200 Subject: [PATCH 1/3] Do not fail un-registration if Firebase token is not known. Fixes #2895 --- .../pushproviders/firebase/FirebasePushProvider.kt | 11 +++++------ .../firebase/FirebasePushProviderTest.kt | 7 ++----- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt index b88d70b157..0228f8f74b 100644 --- a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt +++ b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt @@ -64,14 +64,13 @@ class FirebasePushProvider @Inject constructor( override suspend fun getCurrentDistributor(matrixClient: MatrixClient) = firebaseDistributor override suspend fun unregister(matrixClient: MatrixClient): Result { - val pushKey = firebaseStore.getFcmToken() ?: return Result.failure( - IllegalStateException( - "Unable to unregister pusher, Firebase token is not known." - ) - ).also { + val pushKey = firebaseStore.getFcmToken() + return if (pushKey == null) { Timber.tag(loggerTag.value).w("Unable to unregister pusher, Firebase token is not known.") + Result.success(Unit) + } else { + pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL) } - return pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL) } override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? { diff --git a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt index 3c82682767..880be2f053 100644 --- a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt +++ b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt @@ -134,17 +134,14 @@ class FirebasePushProviderTest { } @Test - fun `unregister ko no token`() = runTest { + fun `unregister no token - in this case, the error is ignored`() = runTest { val firebasePushProvider = createFirebasePushProvider( firebaseStore = InMemoryFirebaseStore( token = null ), - pusherSubscriber = FakePusherSubscriber( - unregisterPusherResult = { _, _, _ -> Result.success(Unit) } - ) ) val result = firebasePushProvider.unregister(FakeMatrixClient()) - assertThat(result.isFailure).isTrue() + assertThat(result.isSuccess).isTrue() } @Test From d066f03eabdbb030b7016a0de3e1d510655d400d Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 23 May 2024 09:58:29 +0200 Subject: [PATCH 2/3] Add logs on pusher registration --- .../android/libraries/push/impl/DefaultPushService.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt index bf2b1ed542..47e26fb920 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt @@ -51,13 +51,16 @@ class DefaultPushService @Inject constructor( pushProvider: PushProvider, distributor: Distributor, ): Result { + Timber.d("Registering with ${pushProvider.name}/${distributor.name}}") val userPushStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId) val currentPushProviderName = userPushStore.getPushProviderName() val currentPushProvider = pushProviders.find { it.name == currentPushProviderName } val currentDistributorValue = currentPushProvider?.getCurrentDistributor(matrixClient)?.value if (currentPushProviderName != pushProvider.name || currentDistributorValue != distributor.value) { // Unregister previous one if any - currentPushProvider?.unregister(matrixClient) + currentPushProvider + ?.also { Timber.d("Unregistering previous push provider $currentPushProviderName/$currentDistributorValue") } + ?.unregister(matrixClient) ?.onFailure { Timber.w(it, "Failed to unregister previous push provider") return Result.failure(it) From 3796def1c9b6304d96c24d805a1cfc22f1998d58 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 23 May 2024 10:29:07 +0200 Subject: [PATCH 3/3] Ignore local errors when unregistering UnifiedPush, to not block switching to another push provider. --- .../UnregisterUnifiedPushUseCase.kt | 7 ++++- ...DefaultUnregisterUnifiedPushUseCaseTest.kt | 28 ++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt index 65d924740d..769f6507d5 100644 --- a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt +++ b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt @@ -23,6 +23,7 @@ import io.element.android.libraries.di.ApplicationContext import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.pushproviders.api.PusherSubscriber import org.unifiedpush.android.connector.UnifiedPush +import timber.log.Timber import javax.inject.Inject interface UnregisterUnifiedPushUseCase { @@ -39,7 +40,11 @@ class DefaultUnregisterUnifiedPushUseCase @Inject constructor( val endpoint = unifiedPushStore.getEndpoint(clientSecret) val gateway = unifiedPushStore.getPushGateway(clientSecret) if (endpoint == null || gateway == null) { - return Result.failure(IllegalStateException("No endpoint or gateway found for client secret")) + Timber.w("No endpoint or gateway found for client secret") + // Ensure we don't have any remaining data, but ignore this error + unifiedPushStore.storeUpEndpoint(clientSecret, null) + unifiedPushStore.storePushGateway(clientSecret, null) + return Result.success(Unit) } return pusherSubscriber.unregisterPusher(matrixClient, endpoint, gateway) .onSuccess { diff --git a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnregisterUnifiedPushUseCaseTest.kt b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnregisterUnifiedPushUseCaseTest.kt index 1d04a07651..dfa03707cc 100644 --- a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnregisterUnifiedPushUseCaseTest.kt +++ b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnregisterUnifiedPushUseCaseTest.kt @@ -64,29 +64,49 @@ class DefaultUnregisterUnifiedPushUseCaseTest { } @Test - fun `test un registration error - no endpoint`() = runTest { + fun `test un registration error - no endpoint - will not unregister but return success`() = runTest { val matrixClient = FakeMatrixClient() + val storeUpEndpointResult = lambdaRecorder { _: String, _: String? -> } + val storePushGatewayResult = lambdaRecorder { _: String, _: String? -> } val useCase = createDefaultUnregisterUnifiedPushUseCase( unifiedPushStore = FakeUnifiedPushStore( getEndpointResult = { null }, getPushGatewayResult = { "aGateway" }, + storeUpEndpointResult = storeUpEndpointResult, + storePushGatewayResult = storePushGatewayResult, ), ) val result = useCase.execute(matrixClient, A_SECRET) - assertThat(result.isFailure).isTrue() + assertThat(result.isSuccess).isTrue() + storeUpEndpointResult.assertions() + .isCalledOnce() + .with(value(A_SECRET), value(null)) + storePushGatewayResult.assertions() + .isCalledOnce() + .with(value(A_SECRET), value(null)) } @Test - fun `test un registration error - no gateway`() = runTest { + fun `test un registration error - no gateway - will not unregister but return success`() = runTest { val matrixClient = FakeMatrixClient() + val storeUpEndpointResult = lambdaRecorder { _: String, _: String? -> } + val storePushGatewayResult = lambdaRecorder { _: String, _: String? -> } val useCase = createDefaultUnregisterUnifiedPushUseCase( unifiedPushStore = FakeUnifiedPushStore( getEndpointResult = { "aEndpoint" }, getPushGatewayResult = { null }, + storeUpEndpointResult = storeUpEndpointResult, + storePushGatewayResult = storePushGatewayResult, ), ) val result = useCase.execute(matrixClient, A_SECRET) - assertThat(result.isFailure).isTrue() + assertThat(result.isSuccess).isTrue() + storeUpEndpointResult.assertions() + .isCalledOnce() + .with(value(A_SECRET), value(null)) + storePushGatewayResult.assertions() + .isCalledOnce() + .with(value(A_SECRET), value(null)) } @Test