From 9f4164a287cc9521a4dc1a4d928b3d20cfbebc03 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Jan 2024 14:02:40 +0100 Subject: [PATCH 1/5] Migrate preferencesDataStoreFile to a file using a hash, to fix a crash if the userId is too long. --- libraries/pushstore/impl/build.gradle.kts | 1 + .../pushstore/impl/UserPushStoreDataStore.kt | 24 ++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/libraries/pushstore/impl/build.gradle.kts b/libraries/pushstore/impl/build.gradle.kts index 17e0268af1..9b01588090 100644 --- a/libraries/pushstore/impl/build.gradle.kts +++ b/libraries/pushstore/impl/build.gradle.kts @@ -34,6 +34,7 @@ anvil { dependencies { implementation(libs.dagger) implementation(projects.libraries.architecture) + implementation(projects.libraries.androidutils) implementation(projects.libraries.core) implementation(projects.libraries.matrix.api) implementation(projects.libraries.pushstore.api) diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt index a79eafbbfd..506652f98f 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt @@ -23,12 +23,15 @@ import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.stringPreferencesKey import androidx.datastore.preferences.preferencesDataStore +import androidx.datastore.preferences.preferencesDataStoreFile +import io.element.android.libraries.androidutils.hash.hash import io.element.android.libraries.core.bool.orTrue import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.pushstore.api.UserPushStore import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map +import timber.log.Timber /** * Store data related to push about a user. @@ -37,7 +40,24 @@ class UserPushStoreDataStore( private val context: Context, userId: SessionId, ) : UserPushStore { - private val Context.dataStore: DataStore by preferencesDataStore(name = "push_store_$userId") + // Hash the sessionId to get ride of exotic chars and take only the first 16 chars. + // The risk of collision is not high. + private val preferenceName = "push_store_${userId.value.hash().take(16)}" + + init { + // Migrate legacy data. Previous file can be too long if the userId is too long. The userId can be up to 255 chars. + // Example of long file path, with `averylonguserid` replacing a very longer name + // /data/user/0/io.element.android.x.debug/files/datastore/push_store_@averylonguserid:example.org.preferences_pb + val legacyFile = context.preferencesDataStoreFile("push_store_$userId") + if (legacyFile.exists()) { + Timber.d("Migrating legacy push data store for $userId") + if (!legacyFile.renameTo(context.preferencesDataStoreFile(preferenceName))) { + Timber.w("Failed to migrate legacy push data store for $userId") + } + } + } + + private val Context.dataStore: DataStore by preferencesDataStore(name = preferenceName) private val pushProviderName = stringPreferencesKey("pushProviderName") private val currentPushKey = stringPreferencesKey("currentPushKey") private val notificationEnabled = booleanPreferencesKey("notificationEnabled") @@ -81,4 +101,6 @@ class UserPushStoreDataStore( it.clear() } } + + } } From 504ff46f1b119408ee027afd287cc8203bdcbc22 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Jan 2024 14:14:04 +0100 Subject: [PATCH 2/5] Also delete the preference file when the store is reset. --- .../libraries/pushstore/impl/UserPushStoreDataStore.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt index 506652f98f..8fe59b36c9 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt @@ -100,7 +100,7 @@ class UserPushStoreDataStore( context.dataStore.edit { it.clear() } - } - + // Also delete the file + context.preferencesDataStoreFile(preferenceName).delete() } } From fcb84016ca660959cadaf7f75ba2ddecb211fcfe Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Jan 2024 14:34:06 +0100 Subject: [PATCH 3/5] Ensure client secret is deleted when the user signs out. --- .../api/clientsecret/PushClientSecret.kt | 5 ---- libraries/pushstore/impl/build.gradle.kts | 1 + .../impl/clientsecret/PushClientSecretImpl.kt | 24 +++++++++++++++---- .../clientsecret/PushClientSecretImplTest.kt | 5 ++-- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/clientsecret/PushClientSecret.kt b/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/clientsecret/PushClientSecret.kt index dbdd22ce07..1700d41181 100644 --- a/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/clientsecret/PushClientSecret.kt +++ b/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/clientsecret/PushClientSecret.kt @@ -29,9 +29,4 @@ interface PushClientSecret { * Return null if not found. */ suspend fun getUserIdFromSecret(clientSecret: String): SessionId? - - /** - * To call when the user signs out. - */ - suspend fun resetSecretForUser(userId: SessionId) } diff --git a/libraries/pushstore/impl/build.gradle.kts b/libraries/pushstore/impl/build.gradle.kts index 9b01588090..28e53e011c 100644 --- a/libraries/pushstore/impl/build.gradle.kts +++ b/libraries/pushstore/impl/build.gradle.kts @@ -49,6 +49,7 @@ dependencies { testImplementation(libs.coroutines.test) testImplementation(projects.libraries.matrix.test) testImplementation(projects.services.appnavstate.test) + testImplementation(projects.libraries.sessionStorage.test) androidTestImplementation(libs.coroutines.test) androidTestImplementation(libs.test.core) diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt index ca0ed14e33..25c0332b43 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt @@ -22,13 +22,20 @@ 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 -@ContributesBinding(AppScope::class) +@ContributesBinding(AppScope::class, boundType = PushClientSecret::class) class PushClientSecretImpl @Inject constructor( private val pushClientSecretFactory: PushClientSecretFactory, private val pushClientSecretStore: PushClientSecretStore, -) : PushClientSecret { + private val sessionObserver: SessionObserver, +) : PushClientSecret, SessionListener { + init { + observeSessions() + } + override suspend fun getSecretForUser(userId: SessionId): String { val existingSecret = pushClientSecretStore.getSecret(userId) if (existingSecret != null) { @@ -43,7 +50,16 @@ class PushClientSecretImpl @Inject constructor( return pushClientSecretStore.getUserIdFromSecret(clientSecret) } - override suspend fun resetSecretForUser(userId: SessionId) { - pushClientSecretStore.resetSecret(userId) + 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)) } } diff --git a/libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImplTest.kt b/libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImplTest.kt index 5af4877ed3..dc0e5b3651 100644 --- a/libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImplTest.kt +++ b/libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImplTest.kt @@ -18,6 +18,7 @@ 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.sessionstorage.test.observer.NoOpSessionObserver import kotlinx.coroutines.test.runTest import org.junit.Test @@ -31,7 +32,7 @@ internal class PushClientSecretImplTest { fun test() = runTest { val factory = FakePushClientSecretFactory() val store = InMemoryPushClientSecretStore() - val sut = PushClientSecretImpl(factory, store) + val sut = PushClientSecretImpl(factory, store, NoOpSessionObserver()) val secret0 = factory.getSecretForUser(0) val secret1 = factory.getSecretForUser(1) @@ -56,7 +57,7 @@ internal class PushClientSecretImplTest { assertThat(sut.getUserIdFromSecret(A_UNKNOWN_SECRET)).isNull() // User signs out - sut.resetSecretForUser(A_USER_ID_0) + 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) From 22a3b64f5f42ce5ce82b9d28ae346b4264cf9f32 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Jan 2024 17:31:26 +0100 Subject: [PATCH 4/5] Fix typo --- .../android/libraries/pushstore/impl/UserPushStoreDataStore.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt index 8fe59b36c9..f7a159f6c3 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt @@ -40,7 +40,7 @@ class UserPushStoreDataStore( private val context: Context, userId: SessionId, ) : UserPushStore { - // Hash the sessionId to get ride of exotic chars and take only the first 16 chars. + // Hash the sessionId to get rid of exotic chars and take only the first 16 chars. // The risk of collision is not high. private val preferenceName = "push_store_${userId.value.hash().take(16)}" From 48a7573884fa8ba650744afac70bdfeb28ad9d47 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Jan 2024 17:33:36 +0100 Subject: [PATCH 5/5] PushClientSecretImpl needs to be a singleton now that it observe the sessions. --- .../pushstore/impl/clientsecret/PushClientSecretImpl.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt index 25c0332b43..5f0c83b6d7 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/clientsecret/PushClientSecretImpl.kt @@ -18,6 +18,7 @@ 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 @@ -26,6 +27,7 @@ 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) class PushClientSecretImpl @Inject constructor( private val pushClientSecretFactory: PushClientSecretFactory,