Merge pull request #3877 from element-hq/feature/bma/fixUnifiedPushUnregister

Fix unified push unregister

appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt

@@ -142,12 +142,12 @@ class LoggedInPresenter @Inject constructor(
                     .also { Timber.tag(pusherTag.value).w("No distributors available") }
                     .also {
                         // In this case, consider the push provider is chosen.
-                        pushService.selectPushProvider(matrixClient, pushProvider)
+                        pushService.selectPushProvider(matrixClient.sessionId, pushProvider)
                     }
                     .also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable()) }
             pushService.registerWith(matrixClient, pushProvider, distributor)
         } else {
-            val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient)
+            val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient.sessionId)
             if (currentPushDistributor == null) {
                 Timber.tag(pusherTag.value).d("Register with the first available distributor")
                 val distributor = currentPushProvider.getDistributors().firstOrNull()

appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt

@@ -378,7 +378,7 @@ class LoggedInPresenterTest {
         val lambda = lambdaRecorder<MatrixClient, PushProvider, Distributor, Result<Unit>> { _, _, _ ->
             Result.success(Unit)
         }
-        val selectPushProviderLambda = lambdaRecorder<MatrixClient, PushProvider, Unit> { _, _ -> }
+        val selectPushProviderLambda = lambdaRecorder<SessionId, PushProvider, Unit> { _, _ -> }
         val sessionVerificationService = FakeSessionVerificationService(
             initialSessionVerifiedStatus = SessionVerifiedStatus.Verified
         )
@@ -408,8 +408,8 @@ class LoggedInPresenterTest {
             selectPushProviderLambda.assertions()
                 .isCalledOnce()
                 .with(
-                    // MatrixClient
+                    // SessionId
-                    any(),
+                    value(A_SESSION_ID),
                     // PushProvider
                     value(pushProvider),
                 )
@@ -481,7 +481,7 @@ class LoggedInPresenterTest {
         registerWithLambda: (MatrixClient, PushProvider, Distributor) -> Result<Unit> = { _, _, _ ->
             Result.success(Unit)
         },
-        selectPushProviderLambda: (MatrixClient, PushProvider) -> Unit = { _, _ -> lambdaError() },
+        selectPushProviderLambda: (SessionId, PushProvider) -> Unit = { _, _ -> lambdaError() },
         currentPushProvider: () -> PushProvider? = { null },
         setIgnoreRegistrationErrorLambda: (SessionId, Boolean) -> Unit = { _, _ -> lambdaError() },
     ): PushService {

features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt

@@ -93,7 +93,7 @@ class NotificationSettingsPresenter @Inject constructor(
 
         LaunchedEffect(refreshPushProvider) {
             val p = pushService.getCurrentPushProvider()
-            val name = p?.getCurrentDistributor(matrixClient)?.name
+            val name = p?.getCurrentDistributor(matrixClient.sessionId)?.name
             currentDistributorName = if (name != null) {
                 AsyncData.Success(name)
             } else {

libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt

@@ -40,7 +40,7 @@ interface PushService {
      * To be used when there is no distributor available.
      */
     suspend fun selectPushProvider(
-        matrixClient: MatrixClient,
+        sessionId: SessionId,
         pushProvider: PushProvider,
     )
 

libraries/push/impl/build.gradle.kts

@@ -43,6 +43,7 @@ dependencies {
     implementation(projects.libraries.matrix.api)
     implementation(projects.libraries.matrixui)
     implementation(projects.libraries.preferences.api)
+    implementation(projects.libraries.sessionStorage.api)
     implementation(projects.libraries.uiStrings)
     implementation(projects.libraries.troubleshoot.api)
     implementation(projects.features.call.api)
@@ -64,6 +65,7 @@ dependencies {
     testImplementation(libs.coroutines.test)
     testImplementation(projects.libraries.matrix.test)
     testImplementation(projects.libraries.preferences.test)
+    testImplementation(projects.libraries.sessionStorage.test)
     testImplementation(projects.libraries.push.test)
     testImplementation(projects.libraries.pushproviders.test)
     testImplementation(projects.libraries.pushstore.test)

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt

@@ -9,6 +9,7 @@ package io.element.android.libraries.push.impl
 
 import com.squareup.anvil.annotations.ContributesBinding
 import io.element.android.libraries.di.AppScope
+import io.element.android.libraries.di.SingleIn
 import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.push.api.GetCurrentPushProvider
@@ -17,17 +18,27 @@ import io.element.android.libraries.push.impl.test.TestPush
 import io.element.android.libraries.pushproviders.api.Distributor
 import io.element.android.libraries.pushproviders.api.PushProvider
 import io.element.android.libraries.pushstore.api.UserPushStoreFactory
+import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecretStore
+import io.element.android.libraries.sessionstorage.api.observer.SessionListener
+import io.element.android.libraries.sessionstorage.api.observer.SessionObserver
 import kotlinx.coroutines.flow.Flow
 import timber.log.Timber
 import javax.inject.Inject
 
-@ContributesBinding(AppScope::class)
+@ContributesBinding(AppScope::class, boundType = PushService::class)
+@SingleIn(AppScope::class)
 class DefaultPushService @Inject constructor(
     private val testPush: TestPush,
     private val userPushStoreFactory: UserPushStoreFactory,
     private val pushProviders: Set<@JvmSuppressWildcards PushProvider>,
     private val getCurrentPushProvider: GetCurrentPushProvider,
-) : PushService {
+    private val sessionObserver: SessionObserver,
+    private val pushClientSecretStore: PushClientSecretStore,
+) : PushService, SessionListener {
+    init {
+        observeSessions()
+    }
+
     override suspend fun getCurrentPushProvider(): PushProvider? {
         val currentPushProvider = getCurrentPushProvider.getCurrentPushProvider()
         return pushProviders.find { it.name == currentPushProvider }
@@ -47,7 +58,7 @@ class DefaultPushService @Inject constructor(
         val userPushStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId)
         val currentPushProviderName = userPushStore.getPushProviderName()
         val currentPushProvider = pushProviders.find { it.name == currentPushProviderName }
-        val currentDistributorValue = currentPushProvider?.getCurrentDistributor(matrixClient)?.value
+        val currentDistributorValue = currentPushProvider?.getCurrentDistributor(matrixClient.sessionId)?.value
         if (currentPushProviderName != pushProvider.name || currentDistributorValue != distributor.value) {
             // Unregister previous one if any
             currentPushProvider
@@ -65,11 +76,11 @@ class DefaultPushService @Inject constructor(
     }
 
     override suspend fun selectPushProvider(
-        matrixClient: MatrixClient,
+        sessionId: SessionId,
         pushProvider: PushProvider,
     ) {
         Timber.d("Select ${pushProvider.name}")
-        val userPushStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId)
+        val userPushStore = userPushStoreFactory.getOrCreate(sessionId)
         userPushStore.setPushProviderName(pushProvider.name)
     }
 
@@ -87,4 +98,31 @@ class DefaultPushService @Inject constructor(
         testPush.execute(config)
         return true
     }
+
+    private fun observeSessions() {
+        sessionObserver.addListener(this)
+    }
+
+    override suspend fun onSessionCreated(userId: String) {
+        // Nothing to do
+    }
+
+    /**
+     * The session has been deleted.
+     * In this case, this is not necessary to unregister the pusher from the homeserver,
+     * but we need to do some cleanup locally.
+     * The current push provider may want to take action, and we need to
+     * cleanup the stores.
+     */
+    override suspend fun onSessionDeleted(userId: String) {
+        val sessionId = SessionId(userId)
+        val userPushStore = userPushStoreFactory.getOrCreate(sessionId)
+        val currentPushProviderName = userPushStore.getPushProviderName()
+        val currentPushProvider = pushProviders.find { it.name == currentPushProviderName }
+        // Cleanup the current push provider. They may need the client secret, so delete the secret after.
+        currentPushProvider?.onSessionDeleted(sessionId)
+        // Now we can safely reset the stores.
+        pushClientSecretStore.resetSecret(sessionId)
+        userPushStore.reset()
+    }
 }

libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/DefaultPushServiceTest.kt

@@ -9,6 +9,7 @@ package io.element.android.libraries.push.impl
 
 import com.google.common.truth.Truth.assertThat
 import io.element.android.libraries.matrix.api.MatrixClient
+import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.matrix.test.AN_EXCEPTION
 import io.element.android.libraries.matrix.test.A_SESSION_ID
 import io.element.android.libraries.matrix.test.FakeMatrixClient
@@ -22,8 +23,12 @@ import io.element.android.libraries.pushproviders.api.PushProvider
 import io.element.android.libraries.pushproviders.test.FakePushProvider
 import io.element.android.libraries.pushproviders.test.aCurrentUserPushConfig
 import io.element.android.libraries.pushstore.api.UserPushStoreFactory
+import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecretStore
 import io.element.android.libraries.pushstore.test.userpushstore.FakeUserPushStore
 import io.element.android.libraries.pushstore.test.userpushstore.FakeUserPushStoreFactory
+import io.element.android.libraries.pushstore.test.userpushstore.clientsecret.InMemoryPushClientSecretStore
+import io.element.android.libraries.sessionstorage.api.observer.SessionObserver
+import io.element.android.libraries.sessionstorage.test.observer.NoOpSessionObserver
 import io.element.android.tests.testutils.lambda.lambdaRecorder
 import io.element.android.tests.testutils.lambda.value
 import kotlinx.coroutines.flow.first
@@ -210,17 +215,87 @@ class DefaultPushServiceTest {
         assertThat(defaultPushService.ignoreRegistrationError(A_SESSION_ID).first()).isTrue()
     }
 
+    @Test
+    fun `onSessionCreated is noop`() = runTest {
+        val defaultPushService = createDefaultPushService()
+        defaultPushService.onSessionCreated(A_SESSION_ID.value)
+    }
+
+    @Test
+    fun `onSessionDeleted should transmit the info to the current push provider and cleanup the stores`() = runTest {
+        val onSessionDeletedLambda = lambdaRecorder<SessionId, Unit> { }
+        val aCurrentPushProvider = FakePushProvider(
+            name = "aCurrentPushProvider",
+            onSessionDeletedLambda = onSessionDeletedLambda,
+        )
+        val userPushStore = FakeUserPushStore(
+            pushProviderName = aCurrentPushProvider.name,
+        )
+        val pushClientSecretStore = InMemoryPushClientSecretStore()
+        val defaultPushService = createDefaultPushService(
+            pushProviders = setOf(aCurrentPushProvider),
+            getCurrentPushProvider = FakeGetCurrentPushProvider(currentPushProvider = aCurrentPushProvider.name),
+            userPushStoreFactory = FakeUserPushStoreFactory(
+                userPushStore = { userPushStore },
+            ),
+            pushClientSecretStore = pushClientSecretStore,
+        )
+        defaultPushService.onSessionDeleted(A_SESSION_ID.value)
+        assertThat(userPushStore.getPushProviderName()).isNull()
+        assertThat(pushClientSecretStore.getSecret(A_SESSION_ID)).isNull()
+        onSessionDeletedLambda.assertions().isCalledOnce().with(value(A_SESSION_ID))
+    }
+
+    @Test
+    fun `onSessionDeleted when there is no push provider should just cleanup the stores`() = runTest {
+        val userPushStore = FakeUserPushStore(
+            pushProviderName = null,
+        )
+        val pushClientSecretStore = InMemoryPushClientSecretStore()
+        val defaultPushService = createDefaultPushService(
+            pushProviders = emptySet(),
+            getCurrentPushProvider = FakeGetCurrentPushProvider(currentPushProvider = null),
+            userPushStoreFactory = FakeUserPushStoreFactory(
+                userPushStore = { userPushStore },
+            ),
+            pushClientSecretStore = pushClientSecretStore,
+        )
+        defaultPushService.onSessionDeleted(A_SESSION_ID.value)
+        assertThat(userPushStore.getPushProviderName()).isNull()
+        assertThat(pushClientSecretStore.getSecret(A_SESSION_ID)).isNull()
+    }
+
+    @Test
+    fun `selectPushProvider should store the data in the store`() = runTest {
+        val userPushStore = FakeUserPushStore()
+        val defaultPushService = createDefaultPushService(
+            userPushStoreFactory = FakeUserPushStoreFactory(
+                userPushStore = { userPushStore },
+            ),
+        )
+        val aPushProvider = FakePushProvider(
+            name = "aCurrentPushProvider",
+        )
+        assertThat(userPushStore.getPushProviderName()).isNull()
+        defaultPushService.selectPushProvider(A_SESSION_ID, aPushProvider)
+        assertThat(userPushStore.getPushProviderName()).isEqualTo(aPushProvider.name)
+    }
+
     private fun createDefaultPushService(
         testPush: TestPush = FakeTestPush(),
         userPushStoreFactory: UserPushStoreFactory = FakeUserPushStoreFactory(),
         pushProviders: Set<@JvmSuppressWildcards PushProvider> = emptySet(),
         getCurrentPushProvider: GetCurrentPushProvider = FakeGetCurrentPushProvider(currentPushProvider = null),
+        sessionObserver: SessionObserver = NoOpSessionObserver(),
+        pushClientSecretStore: PushClientSecretStore = InMemoryPushClientSecretStore(),
     ): DefaultPushService {
         return DefaultPushService(
             testPush = testPush,
             userPushStoreFactory = userPushStoreFactory,
             pushProviders = pushProviders,
             getCurrentPushProvider = getCurrentPushProvider,
+            sessionObserver = sessionObserver,
+            pushClientSecretStore = pushClientSecretStore,
         )
     }
 }

libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt

@@ -24,7 +24,7 @@ class FakePushService(
         Result.success(Unit)
     },
     private val currentPushProvider: () -> PushProvider? = { availablePushProviders.firstOrNull() },
-    private val selectPushProviderLambda: suspend (MatrixClient, PushProvider) -> Unit = { _, _ -> lambdaError() },
+    private val selectPushProviderLambda: suspend (SessionId, PushProvider) -> Unit = { _, _ -> lambdaError() },
     private val setIgnoreRegistrationErrorLambda: (SessionId, Boolean) -> Unit = { _, _ -> lambdaError() },
 ) : PushService {
     override suspend fun getCurrentPushProvider(): PushProvider? {
@@ -50,8 +50,8 @@ class FakePushService(
             }
     }
 
-    override suspend fun selectPushProvider(matrixClient: MatrixClient, pushProvider: PushProvider) {
+    override suspend fun selectPushProvider(sessionId: SessionId, pushProvider: PushProvider) {
-        selectPushProviderLambda(matrixClient, pushProvider)
+        selectPushProviderLambda(sessionId, pushProvider)
     }
 
     private val ignoreRegistrationError = MutableStateFlow(false)

libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushProvider.kt

@@ -8,6 +8,7 @@
 package io.element.android.libraries.pushproviders.api
 
 import io.element.android.libraries.matrix.api.MatrixClient
+import io.element.android.libraries.matrix.api.core.SessionId
 
 /**
  * This is the main API for this module.
@@ -36,13 +37,18 @@ interface PushProvider {
     /**
      * Return the current distributor, or null if none.
      */
-    suspend fun getCurrentDistributor(matrixClient: MatrixClient): Distributor?
+    suspend fun getCurrentDistributor(sessionId: SessionId): Distributor?
 
     /**
      * Unregister the pusher.
      */
     suspend fun unregister(matrixClient: MatrixClient): Result<Unit>
 
+    /**
+     * To invoke when the session is deleted.
+     */
+    suspend fun onSessionDeleted(sessionId: SessionId)
+
     suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig?
 
     fun canRotateToken(): Boolean

libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt

@@ -11,6 +11,7 @@ import com.squareup.anvil.annotations.ContributesMultibinding
 import io.element.android.libraries.core.log.logger.LoggerTag
 import io.element.android.libraries.di.AppScope
 import io.element.android.libraries.matrix.api.MatrixClient
+import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.pushproviders.api.CurrentUserPushConfig
 import io.element.android.libraries.pushproviders.api.Distributor
 import io.element.android.libraries.pushproviders.api.PushProvider
@@ -51,7 +52,7 @@ class FirebasePushProvider @Inject constructor(
         )
     }
 
-    override suspend fun getCurrentDistributor(matrixClient: MatrixClient) = firebaseDistributor
+    override suspend fun getCurrentDistributor(sessionId: SessionId) = firebaseDistributor
 
     override suspend fun unregister(matrixClient: MatrixClient): Result<Unit> {
         val pushKey = firebaseStore.getFcmToken()
@@ -63,6 +64,11 @@ class FirebasePushProvider @Inject constructor(
         }
     }
 
+    /**
+     * Nothing to clean up here.
+     */
+    override suspend fun onSessionDeleted(sessionId: SessionId) = Unit
+
     override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {
         return firebaseStore.getFcmToken()?.let { fcmToken ->
             CurrentUserPushConfig(

libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt

@@ -10,6 +10,7 @@ package io.element.android.libraries.pushproviders.firebase
 import com.google.common.truth.Truth.assertThat
 import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.libraries.matrix.test.AN_EXCEPTION
+import io.element.android.libraries.matrix.test.A_SESSION_ID
 import io.element.android.libraries.matrix.test.FakeMatrixClient
 import io.element.android.libraries.push.test.FakePusherSubscriber
 import io.element.android.libraries.pushproviders.api.CurrentUserPushConfig
@@ -47,9 +48,9 @@ class FirebasePushProviderTest {
     }
 
     @Test
-    fun `getCurrentDistributor always return the unique distributor`() = runTest {
+    fun `getCurrentDistributor always returns the unique distributor`() = runTest {
         val firebasePushProvider = createFirebasePushProvider()
-        val result = firebasePushProvider.getCurrentDistributor(FakeMatrixClient())
+        val result = firebasePushProvider.getCurrentDistributor(A_SESSION_ID)
         assertThat(result).isEqualTo(Distributor("Firebase", "Firebase"))
     }
 
@@ -176,6 +177,18 @@ class FirebasePushProviderTest {
         lambda.assertions().isCalledOnce()
     }
 
+    @Test
+    fun `canRotateToken should return true`() = runTest {
+        val firebasePushProvider = createFirebasePushProvider()
+        assertThat(firebasePushProvider.canRotateToken()).isTrue()
+    }
+
+    @Test
+    fun `onSessionDeleted should be noop`() = runTest {
+        val firebasePushProvider = createFirebasePushProvider()
+        firebasePushProvider.onSessionDeleted(A_SESSION_ID)
+    }
+
     private fun createFirebasePushProvider(
         firebaseStore: FirebaseStore = InMemoryFirebaseStore(),
         pusherSubscriber: PusherSubscriber = FakePusherSubscriber(),

libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt

@@ -8,6 +8,7 @@
 package io.element.android.libraries.pushproviders.test
 
 import io.element.android.libraries.matrix.api.MatrixClient
+import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.pushproviders.api.CurrentUserPushConfig
 import io.element.android.libraries.pushproviders.api.Distributor
 import io.element.android.libraries.pushproviders.api.PushProvider
@@ -21,6 +22,7 @@ class FakePushProvider(
     private val currentUserPushConfig: CurrentUserPushConfig? = null,
     private val registerWithResult: (MatrixClient, Distributor) -> Result<Unit> = { _, _ -> lambdaError() },
     private val unregisterWithResult: (MatrixClient) -> Result<Unit> = { lambdaError() },
+    private val onSessionDeletedLambda: (SessionId) -> Unit = { lambdaError() },
     private val canRotateTokenResult: () -> Boolean = { lambdaError() },
     private val rotateTokenLambda: () -> Result<Unit> = { lambdaError() },
 ) : PushProvider {
@@ -30,7 +32,7 @@ class FakePushProvider(
         return registerWithResult(matrixClient, distributor)
     }
 
-    override suspend fun getCurrentDistributor(matrixClient: MatrixClient): Distributor? {
+    override suspend fun getCurrentDistributor(sessionId: SessionId): Distributor? {
         return currentDistributor()
     }
 
@@ -38,6 +40,10 @@ class FakePushProvider(
         return unregisterWithResult(matrixClient)
     }
 
+    override suspend fun onSessionDeleted(sessionId: SessionId) {
+        onSessionDeletedLambda(sessionId)
+    }
+
     override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {
         return currentUserPushConfig
     }

libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt

@@ -10,6 +10,7 @@ package io.element.android.libraries.pushproviders.unifiedpush
 import com.squareup.anvil.annotations.ContributesMultibinding
 import io.element.android.libraries.di.AppScope
 import io.element.android.libraries.matrix.api.MatrixClient
+import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.pushproviders.api.CurrentUserPushConfig
 import io.element.android.libraries.pushproviders.api.Distributor
 import io.element.android.libraries.pushproviders.api.PushProvider
@@ -40,14 +41,19 @@ class UnifiedPushProvider @Inject constructor(
             }
     }
 
-    override suspend fun getCurrentDistributor(matrixClient: MatrixClient): Distributor? {
+    override suspend fun getCurrentDistributor(sessionId: SessionId): Distributor? {
-        val distributorValue = unifiedPushStore.getDistributorValue(matrixClient.sessionId)
+        val distributorValue = unifiedPushStore.getDistributorValue(sessionId)
         return getDistributors().find { it.value == distributorValue }
     }
 
     override suspend fun unregister(matrixClient: MatrixClient): Result<Unit> {
         val clientSecret = pushClientSecret.getSecretForUser(matrixClient.sessionId)
-        return unRegisterUnifiedPushUseCase.execute(matrixClient, clientSecret)
+        return unRegisterUnifiedPushUseCase.unregister(matrixClient, clientSecret)
+    }
+
+    override suspend fun onSessionDeleted(sessionId: SessionId) {
+        val clientSecret = pushClientSecret.getSecretForUser(sessionId)
+        unRegisterUnifiedPushUseCase.cleanup(clientSecret)
     }
 
     override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {

libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt

@@ -18,7 +18,15 @@ import timber.log.Timber
 import javax.inject.Inject
 
 interface UnregisterUnifiedPushUseCase {
-    suspend fun execute(matrixClient: MatrixClient, clientSecret: String): Result<Unit>
+    /**
+     * Unregister the app from the homeserver, then from UnifiedPush.
+     */
+    suspend fun unregister(matrixClient: MatrixClient, clientSecret: String): Result<Unit>
+
+    /**
+     * Cleanup any remaining data for the given client secret and unregister the app from UnifiedPush.
+     */
+    fun cleanup(clientSecret: String)
 }
 
 @ContributesBinding(AppScope::class)
@@ -27,21 +35,24 @@ class DefaultUnregisterUnifiedPushUseCase @Inject constructor(
     private val unifiedPushStore: UnifiedPushStore,
     private val pusherSubscriber: PusherSubscriber,
 ) : UnregisterUnifiedPushUseCase {
-    override suspend fun execute(matrixClient: MatrixClient, clientSecret: String): Result<Unit> {
+    override suspend fun unregister(matrixClient: MatrixClient, clientSecret: String): Result<Unit> {
         val endpoint = unifiedPushStore.getEndpoint(clientSecret)
         val gateway = unifiedPushStore.getPushGateway(clientSecret)
         if (endpoint == null || gateway == null) {
             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)
+            cleanup(clientSecret)
-            unifiedPushStore.storePushGateway(clientSecret, null)
             return Result.success(Unit)
         }
         return pusherSubscriber.unregisterPusher(matrixClient, endpoint, gateway)
             .onSuccess {
-                unifiedPushStore.storeUpEndpoint(clientSecret, null)
+                cleanup(clientSecret)
-                unifiedPushStore.storePushGateway(clientSecret, null)
-                UnifiedPush.unregisterApp(context)
             }
     }
+
+    override fun cleanup(clientSecret: String) {
+        unifiedPushStore.storeUpEndpoint(clientSecret, null)
+        unifiedPushStore.storePushGateway(clientSecret, null)
+        UnifiedPush.unregisterApp(context, clientSecret)
+    }
 }

libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnregisterUnifiedPushUseCaseTest.kt

@@ -41,7 +41,7 @@ class DefaultUnregisterUnifiedPushUseCaseTest {
                 unregisterPusherResult = lambda
             )
         )
-        val result = useCase.execute(matrixClient, A_SECRET)
+        val result = useCase.unregister(matrixClient, A_SECRET)
         assertThat(result.isSuccess).isTrue()
         lambda.assertions()
             .isCalledOnce()
@@ -67,7 +67,7 @@ class DefaultUnregisterUnifiedPushUseCaseTest {
                 storePushGatewayResult = storePushGatewayResult,
             ),
         )
-        val result = useCase.execute(matrixClient, A_SECRET)
+        val result = useCase.unregister(matrixClient, A_SECRET)
         assertThat(result.isSuccess).isTrue()
         storeUpEndpointResult.assertions()
             .isCalledOnce()
@@ -90,7 +90,7 @@ class DefaultUnregisterUnifiedPushUseCaseTest {
                 storePushGatewayResult = storePushGatewayResult,
             ),
         )
-        val result = useCase.execute(matrixClient, A_SECRET)
+        val result = useCase.unregister(matrixClient, A_SECRET)
         assertThat(result.isSuccess).isTrue()
         storeUpEndpointResult.assertions()
             .isCalledOnce()
@@ -112,7 +112,7 @@ class DefaultUnregisterUnifiedPushUseCaseTest {
                 unregisterPusherResult = { _, _, _ -> Result.failure(AN_EXCEPTION) }
             )
         )
-        val result = useCase.execute(matrixClient, A_SECRET)
+        val result = useCase.unregister(matrixClient, A_SECRET)
         assertThat(result.isFailure).isTrue()
     }
 

libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/FakeUnregisterUnifiedPushUseCase.kt

@@ -11,9 +11,14 @@ import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.tests.testutils.lambda.lambdaError
 
 class FakeUnregisterUnifiedPushUseCase(
-    private val result: (MatrixClient, String) -> Result<Unit> = { _, _ -> lambdaError() }
+    private val unregisterLambda: (MatrixClient, String) -> Result<Unit> = { _, _ -> lambdaError() },
+    private val cleanupLambda: (String) -> Unit = { lambdaError() },
 ) : UnregisterUnifiedPushUseCase {
-    override suspend fun execute(matrixClient: MatrixClient, clientSecret: String): Result<Unit> {
+    override suspend fun unregister(matrixClient: MatrixClient, clientSecret: String): Result<Unit> {
-        return result(matrixClient, clientSecret)
+        return unregisterLambda(matrixClient, clientSecret)
+    }
+
+    override fun cleanup(clientSecret: String) {
+        cleanupLambda(clientSecret)
     }
 }

libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProviderTest.kt

@@ -117,13 +117,13 @@ class UnifiedPushProviderTest {
     fun `unregister ok`() = runTest {
         val matrixClient = FakeMatrixClient()
         val getSecretForUserResultLambda = lambdaRecorder<SessionId, String> { A_SECRET }
-        val executeLambda = lambdaRecorder<MatrixClient, String, Result<Unit>> { _, _ -> Result.success(Unit) }
+        val unregisterLambda = lambdaRecorder<MatrixClient, String, Result<Unit>> { _, _ -> Result.success(Unit) }
         val unifiedPushProvider = createUnifiedPushProvider(
             pushClientSecret = FakePushClientSecret(
                 getSecretForUserResult = getSecretForUserResultLambda,
             ),
             unRegisterUnifiedPushUseCase = FakeUnregisterUnifiedPushUseCase(
-                result = executeLambda,
+                unregisterLambda = unregisterLambda,
             ),
         )
         val result = unifiedPushProvider.unregister(matrixClient)
@@ -131,7 +131,7 @@ class UnifiedPushProviderTest {
         getSecretForUserResultLambda.assertions()
             .isCalledOnce()
             .with(value(A_SESSION_ID))
-        executeLambda.assertions()
+        unregisterLambda.assertions()
             .isCalledOnce()
             .with(value(matrixClient), value(A_SECRET))
     }
@@ -140,13 +140,13 @@ class UnifiedPushProviderTest {
     fun `unregister ko`() = runTest {
         val matrixClient = FakeMatrixClient()
         val getSecretForUserResultLambda = lambdaRecorder<SessionId, String> { A_SECRET }
-        val executeLambda = lambdaRecorder<MatrixClient, String, Result<Unit>> { _, _ -> Result.failure(AN_EXCEPTION) }
+        val unregisterLambda = lambdaRecorder<MatrixClient, String, Result<Unit>> { _, _ -> Result.failure(AN_EXCEPTION) }
         val unifiedPushProvider = createUnifiedPushProvider(
             pushClientSecret = FakePushClientSecret(
                 getSecretForUserResult = getSecretForUserResultLambda,
             ),
             unRegisterUnifiedPushUseCase = FakeUnregisterUnifiedPushUseCase(
-                result = executeLambda,
+                unregisterLambda = unregisterLambda,
             ),
         )
         val result = unifiedPushProvider.unregister(matrixClient)
@@ -154,7 +154,7 @@ class UnifiedPushProviderTest {
         getSecretForUserResultLambda.assertions()
             .isCalledOnce()
             .with(value(A_SESSION_ID))
-        executeLambda.assertions()
+        unregisterLambda.assertions()
             .isCalledOnce()
             .with(value(matrixClient), value(A_SECRET))
     }
@@ -162,7 +162,6 @@ class UnifiedPushProviderTest {
     @Test
     fun `getCurrentDistributor ok`() = runTest {
         val distributor = Distributor("value", "Name")
-        val matrixClient = FakeMatrixClient()
         val unifiedPushProvider = createUnifiedPushProvider(
             unifiedPushStore = FakeUnifiedPushStore(
                 getDistributorValueResult = { distributor.value }
@@ -174,14 +173,13 @@ class UnifiedPushProviderTest {
                 )
             )
         )
-        val result = unifiedPushProvider.getCurrentDistributor(matrixClient)
+        val result = unifiedPushProvider.getCurrentDistributor(A_SESSION_ID)
         assertThat(result).isEqualTo(distributor)
     }
 
     @Test
     fun `getCurrentDistributor not know`() = runTest {
         val distributor = Distributor("value", "Name")
-        val matrixClient = FakeMatrixClient()
         val unifiedPushProvider = createUnifiedPushProvider(
             unifiedPushStore = FakeUnifiedPushStore(
                 getDistributorValueResult = { "unknown" }
@@ -192,14 +190,13 @@ class UnifiedPushProviderTest {
                 )
             )
         )
-        val result = unifiedPushProvider.getCurrentDistributor(matrixClient)
+        val result = unifiedPushProvider.getCurrentDistributor(A_SESSION_ID)
         assertThat(result).isNull()
     }
 
     @Test
     fun `getCurrentDistributor not found`() = runTest {
         val distributor = Distributor("value", "Name")
-        val matrixClient = FakeMatrixClient()
         val unifiedPushProvider = createUnifiedPushProvider(
             unifiedPushStore = FakeUnifiedPushStore(
                 getDistributorValueResult = { distributor.value }
@@ -208,7 +205,7 @@ class UnifiedPushProviderTest {
                 getDistributorsResult = emptyList()
             )
         )
-        val result = unifiedPushProvider.getCurrentDistributor(matrixClient)
+        val result = unifiedPushProvider.getCurrentDistributor(A_SESSION_ID)
         assertThat(result).isNull()
     }
 
@@ -224,6 +221,27 @@ class UnifiedPushProviderTest {
         assertThat(result).isEqualTo(currentUserPushConfig)
     }
 
+    @Test
+    fun `canRotateToken should return false`() = runTest {
+        val unifiedPushProvider = createUnifiedPushProvider()
+        assertThat(unifiedPushProvider.canRotateToken()).isFalse()
+    }
+
+    @Test
+    fun `onSessionDeleted should do the cleanup`() = runTest {
+        val cleanupLambda = lambdaRecorder<String, Unit> { }
+        val unifiedPushProvider = createUnifiedPushProvider(
+            pushClientSecret = FakePushClientSecret(
+                getSecretForUserResult = { A_SECRET }
+            ),
+            unRegisterUnifiedPushUseCase = FakeUnregisterUnifiedPushUseCase(
+                cleanupLambda = cleanupLambda,
+            ),
+        )
+        unifiedPushProvider.onSessionDeleted(A_SESSION_ID)
+        cleanupLambda.assertions().isCalledOnce().with(value(A_SECRET))
+    }
+
     private fun createUnifiedPushProvider(
         unifiedPushDistributorProvider: UnifiedPushDistributorProvider = FakeUnifiedPushDistributorProvider(),
         registerUnifiedPushUseCase: RegisterUnifiedPushUseCase = FakeRegisterUnifiedPushUseCase(),

libraries/pushstore/impl/build.gradle.kts

@@ -28,7 +28,6 @@ dependencies {
     implementation(projects.libraries.core)
     implementation(projects.libraries.matrix.api)
     implementation(projects.libraries.pushstore.api)
-    implementation(projects.libraries.sessionStorage.api)
     implementation(libs.androidx.corektx)
     implementation(libs.androidx.datastore.preferences)
 
@@ -41,7 +40,6 @@ dependencies {
     testImplementation(projects.libraries.matrix.test)
     testImplementation(projects.services.appnavstate.test)
     testImplementation(projects.libraries.pushstore.test)
-    testImplementation(projects.libraries.sessionStorage.test)
 
     androidTestImplementation(libs.coroutines.test)
     androidTestImplementation(libs.test.core)

libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt

@@ -15,21 +15,14 @@ import io.element.android.libraries.di.SingleIn
 import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.pushstore.api.UserPushStore
 import io.element.android.libraries.pushstore.api.UserPushStoreFactory
-import io.element.android.libraries.sessionstorage.api.observer.SessionListener
-import io.element.android.libraries.sessionstorage.api.observer.SessionObserver
 import java.util.concurrent.ConcurrentHashMap
 import javax.inject.Inject
 
 @SingleIn(AppScope::class)
-@ContributesBinding(AppScope::class, boundType = UserPushStoreFactory::class)
+@ContributesBinding(AppScope::class)
 class DefaultUserPushStoreFactory @Inject constructor(
     @ApplicationContext private val context: Context,
-    private val sessionObserver: SessionObserver,
+) : UserPushStoreFactory {
-) : UserPushStoreFactory, SessionListener {
-    init {
-        observeSessions()
-    }
-
     // We can have only one class accessing a single data store, so keep a cache of them.
     private val cache = ConcurrentHashMap<SessionId, UserPushStore>()
     override fun getOrCreate(userId: SessionId): UserPushStore {
@@ -40,17 +33,4 @@ class DefaultUserPushStoreFactory @Inject constructor(
             )
         }
     }
-
-    private fun observeSessions() {
-        sessionObserver.addListener(this)
-    }
-
-    override suspend fun onSessionCreated(userId: String) {
-        // Nothing to do
-    }
-
-    override suspend fun onSessionDeleted(userId: String) {
-        // Delete the store
-        getOrCreate(SessionId(userId)).reset()
-    }
 }

libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/DefaultPushClientSecret.kt

@@ -9,26 +9,17 @@ package io.element.android.libraries.pushstore.impl.clientsecret
 
 import com.squareup.anvil.annotations.ContributesBinding
 import io.element.android.libraries.di.AppScope
-import io.element.android.libraries.di.SingleIn
 import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecret
 import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecretFactory
 import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecretStore
-import io.element.android.libraries.sessionstorage.api.observer.SessionListener
-import io.element.android.libraries.sessionstorage.api.observer.SessionObserver
 import javax.inject.Inject
 
-@SingleIn(AppScope::class)
+@ContributesBinding(AppScope::class)
-@ContributesBinding(AppScope::class, boundType = PushClientSecret::class)
 class DefaultPushClientSecret @Inject constructor(
     private val pushClientSecretFactory: PushClientSecretFactory,
     private val pushClientSecretStore: PushClientSecretStore,
-    private val sessionObserver: SessionObserver,
+) : PushClientSecret {
-) : PushClientSecret, SessionListener {
-    init {
-        observeSessions()
-    }
-
     override suspend fun getSecretForUser(userId: SessionId): String {
         val existingSecret = pushClientSecretStore.getSecret(userId)
         if (existingSecret != null) {
@@ -42,17 +33,4 @@ class DefaultPushClientSecret @Inject constructor(
     override suspend fun getUserIdFromSecret(clientSecret: String): SessionId? {
         return pushClientSecretStore.getUserIdFromSecret(clientSecret)
     }
-
-    private fun observeSessions() {
-        sessionObserver.addListener(this)
-    }
-
-    override suspend fun onSessionCreated(userId: String) {
-        // Nothing to do
-    }
-
-    override suspend fun onSessionDeleted(userId: String) {
-        // Delete the secret
-        pushClientSecretStore.resetSecret(SessionId(userId))
-    }
 }

libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/DefaultPushClientSecretTest.kt

@@ -10,7 +10,6 @@ package io.element.android.libraries.pushstore.impl.clientsecret
 import com.google.common.truth.Truth.assertThat
 import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.pushstore.test.userpushstore.clientsecret.InMemoryPushClientSecretStore
-import io.element.android.libraries.sessionstorage.test.observer.NoOpSessionObserver
 import kotlinx.coroutines.test.runTest
 import org.junit.Test
 
@@ -24,11 +23,10 @@ internal class DefaultPushClientSecretTest {
     fun test() = runTest {
         val factory = FakePushClientSecretFactory()
         val store = InMemoryPushClientSecretStore()
-        val sut = DefaultPushClientSecret(factory, store, NoOpSessionObserver())
+        val sut = DefaultPushClientSecret(factory, store)
 
         val secret0 = factory.getSecretForUser(0)
         val secret1 = factory.getSecretForUser(1)
-        val secret2 = factory.getSecretForUser(2)
 
         assertThat(store.getSecrets()).isEmpty()
         assertThat(sut.getUserIdFromSecret(secret0)).isNull()
@@ -48,16 +46,10 @@ internal class DefaultPushClientSecretTest {
         // Unknown secret
         assertThat(sut.getUserIdFromSecret(A_UNKNOWN_SECRET)).isNull()
 
-        // User signs out
-        sut.onSessionDeleted(A_USER_ID_0.value)
-        assertThat(store.getSecrets()).hasSize(1)
-        // Create a new secret after reset
-        assertThat(sut.getSecretForUser(A_USER_ID_0)).isEqualTo(secret2)
-
         // Check the store content
         assertThat(store.getSecrets()).isEqualTo(
             mapOf(
-                A_USER_ID_0 to secret2,
+                A_USER_ID_0 to secret0,
                 A_USER_ID_1 to secret1,
             )
         )

libraries/pushstore/test/src/main/kotlin/io/element/android/libraries/pushstore/test/userpushstore/FakeUserPushStore.kt

@@ -54,5 +54,6 @@ class FakeUserPushStore(
     }
 
     override suspend fun reset() {
+        pushProviderName = null
     }
 }