Properly unregister from the ntfy app when the user logs out.

This commit is contained in:
Benoit Marty 2024-11-15 12:31:23 +01:00 committed by Benoit Marty
parent 022cd93653
commit 7a7b5d2dd0
15 changed files with 110 additions and 77 deletions

View file

@ -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)

View file

@ -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 }
@ -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()
}
}

View file

@ -22,8 +22,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
@ -215,12 +219,16 @@ class DefaultPushServiceTest {
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,
)
}
}

View file

@ -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.
@ -43,6 +44,11 @@ interface PushProvider {
*/
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

View file

@ -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
@ -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(

View file

@ -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 {
@ -38,6 +40,10 @@ class FakePushProvider(
return unregisterWithResult(matrixClient)
}
override suspend fun onSessionDeleted(sessionId: SessionId) {
onSessionDeletedLambda(sessionId)
}
override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {
return currentUserPushConfig
}

View file

@ -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
@ -47,7 +48,12 @@ class UnifiedPushProvider @Inject constructor(
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? {

View file

@ -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,7 +35,7 @@ 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) {
@ -42,7 +50,7 @@ class DefaultUnregisterUnifiedPushUseCase @Inject constructor(
}
}
private fun cleanup(clientSecret: String) {
override fun cleanup(clientSecret: String) {
unifiedPushStore.storeUpEndpoint(clientSecret, null)
unifiedPushStore.storePushGateway(clientSecret, null)
UnifiedPush.unregisterApp(context, clientSecret)

View file

@ -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()
}

View file

@ -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> {
return result(matrixClient, clientSecret)
override suspend fun unregister(matrixClient: MatrixClient, clientSecret: String): Result<Unit> {
return unregisterLambda(matrixClient, clientSecret)
}
override fun cleanup(clientSecret: String) {
cleanupLambda(clientSecret)
}
}

View file

@ -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))
}

View file

@ -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)

View file

@ -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, SessionListener {
init {
observeSessions()
}
) : UserPushStoreFactory {
// 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()
}
}

View file

@ -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, boundType = PushClientSecret::class)
@ContributesBinding(AppScope::class)
class DefaultPushClientSecret @Inject constructor(
private val pushClientSecretFactory: PushClientSecretFactory,
private val pushClientSecretStore: PushClientSecretStore,
private val sessionObserver: SessionObserver,
) : PushClientSecret, SessionListener {
init {
observeSessions()
}
) : PushClientSecret {
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))
}
}

View file

@ -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,
)
)