Enforce mandatory session verification only for new logins (#2811)

* Enforce mandatory session verification only for new logins - Creates `AppMigration` base interface as a way to isolate migration logic, app migrations must implement this interface. - Creates `AppMigration01` with the existing logs removal migration and `AppMigration02` with the logic to allow existing sessions to skip verification. - Add `DefaultSessionPreferencesStoreFactory.remove(sessionId)` to allow a ephemeral session store access to exist outside the `SessionScope` for this new migration. * Fix tests * Add more tests. This also includes creating several abstractions. * Review changes. - Make `orderedMigrations` a class property, `migrations` just a constructor parameter to avoid incorrect usages. - Create `lastMigration` property too, use it instead of `MIGRATION_VERSION`.
2024-05-07 16:06:34 +02:00
parent 0359504fa9
commit 9aadec8435
30 changed files with 370 additions and 51 deletions
--- a/features/migration/impl/build.gradle.kts
+++ b/features/migration/impl/build.gradle.kts
@@ -27,9 +27,12 @@ android {
 dependencies {
    implementation(projects.features.migration.api)
    implementation(projects.libraries.architecture)
+    implementation(projects.libraries.preferences.impl)
    implementation(libs.androidx.datastore.preferences)
    implementation(projects.features.rageshake.api)
    implementation(projects.libraries.designsystem)
+    implementation(projects.libraries.matrix.api)
+    implementation(projects.libraries.sessionStorage.api)
    implementation(projects.libraries.uiStrings)

    ksp(libs.showkase.processor)
@@ -39,6 +42,9 @@ dependencies {
    testImplementation(libs.molecule.runtime)
    testImplementation(libs.test.truth)
    testImplementation(libs.test.turbine)
+    testImplementation(projects.libraries.sessionStorage.implMemory)
+    testImplementation(projects.libraries.sessionStorage.test)
+    testImplementation(projects.libraries.preferences.test)
    testImplementation(projects.tests.testutils)
    testImplementation(projects.features.rageshake.test)
 }
--- a/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/MigrationPresenter.kt
+++ b/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/MigrationPresenter.kt
@@ -24,50 +24,51 @@ import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.setValue
 import io.element.android.features.api.MigrationState
-import io.element.android.features.rageshake.api.logs.LogFilesRemover
+import io.element.android.features.migration.impl.migrations.AppMigration
 import io.element.android.libraries.architecture.AsyncData
 import io.element.android.libraries.architecture.Presenter
+import io.element.android.libraries.di.AppScope
+import io.element.android.libraries.di.SingleIn
+import timber.log.Timber
 import javax.inject.Inject

+@SingleIn(AppScope::class)
 class MigrationPresenter @Inject constructor(
    private val migrationStore: MigrationStore,
-    private val logFilesRemover: LogFilesRemover,
+    migrations: Set<@JvmSuppressWildcards AppMigration>,
 ) : Presenter<MigrationState> {
+    private val orderedMigrations = migrations.sortedBy { it.order }
+    private val lastMigration: Int = orderedMigrations.lastOrNull()?.order ?: 0
+
    @Composable
    override fun present(): MigrationState {
-        val migrationStoreVersion = migrationStore.applicationMigrationVersion().collectAsState(initial = null)
+        val migrationStoreVersion by migrationStore.applicationMigrationVersion().collectAsState(initial = null)
        var migrationAction: AsyncData<Unit> by remember { mutableStateOf(AsyncData.Uninitialized) }

-        /*
        // Uncomment this block to run the migration everytime
-        LaunchedEffect(Unit) {
-            migrationStore.setApplicationMigrationVersion(0)
-        }
-         */
+//        LaunchedEffect(Unit) {
+//            Timber.d("Resetting migration version to 0")
+//            migrationStore.setApplicationMigrationVersion(0)
+//        }

-        LaunchedEffect(migrationStoreVersion.value) {
-            val migrationValue = migrationStoreVersion.value ?: return@LaunchedEffect
-            if (migrationValue == MIGRATION_VERSION) {
+        LaunchedEffect(migrationStoreVersion) {
+            val migrationValue = migrationStoreVersion ?: return@LaunchedEffect
+            if (migrationValue == lastMigration) {
+                Timber.d("Current app migration version: $migrationValue. No migration needed.")
                migrationAction = AsyncData.Success(Unit)
                return@LaunchedEffect
            }
            migrationAction = AsyncData.Loading(Unit)
-            if (migrationValue < 1) {
-                logFilesRemover.perform()
+            val nextMigration = orderedMigrations.firstOrNull { it.order > migrationValue }
+            if (nextMigration != null) {
+                Timber.d("Current app migration version: $migrationValue. Applying migration: ${nextMigration.order}")
+                nextMigration.migrate()
+                migrationStore.setApplicationMigrationVersion(nextMigration.order)
            }
-            // Add new step here
-
-            migrationStore.setApplicationMigrationVersion(MIGRATION_VERSION)
        }

        return MigrationState(
            migrationAction = migrationAction,
        )
    }
-
-    companion object {
-        // Increment this value when you need to run the migration again, and
-        // add step in the LaunchedEffect above
-        const val MIGRATION_VERSION = 1
-    }
 }
--- a/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration.kt
+++ b/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration.kt
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+interface AppMigration {
+    val order: Int
+    suspend fun migrate()
+}
--- a/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration01.kt
+++ b/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration01.kt
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import com.squareup.anvil.annotations.ContributesMultibinding
+import io.element.android.features.rageshake.api.logs.LogFilesRemover
+import io.element.android.libraries.di.AppScope
+import javax.inject.Inject
+
+@ContributesMultibinding(AppScope::class)
+class AppMigration01 @Inject constructor(
+    private val logFilesRemover: LogFilesRemover,
+) : AppMigration {
+    override val order: Int = 1
+
+    override suspend fun migrate() {
+        logFilesRemover.perform()
+    }
+}
--- a/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration02.kt
+++ b/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration02.kt
@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import com.squareup.anvil.annotations.ContributesMultibinding
+import io.element.android.features.preferences.api.store.SessionPreferencesStoreFactory
+import io.element.android.libraries.di.AppScope
+import io.element.android.libraries.matrix.api.core.SessionId
+import io.element.android.libraries.sessionstorage.api.SessionStore
+import kotlinx.coroutines.coroutineScope
+import javax.inject.Inject
+
+@ContributesMultibinding(AppScope::class)
+class AppMigration02 @Inject constructor(
+    private val sessionStore: SessionStore,
+    private val sessionPreferenceStoreFactory: SessionPreferencesStoreFactory,
+) : AppMigration {
+    override val order: Int = 2
+
+    override suspend fun migrate() {
+        coroutineScope {
+            for (session in sessionStore.getAllSessions()) {
+                val sessionId = SessionId(session.userId)
+                val preferences = sessionPreferenceStoreFactory.get(sessionId, this)
+                preferences.setSkipSessionVerification(true)
+                // This session preference store must be ephemeral since it's not created with the right coroutine scope
+                sessionPreferenceStoreFactory.remove(sessionId)
+            }
+        }
+    }
+}
--- a/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/MigrationPresenterTest.kt
+++ b/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/MigrationPresenterTest.kt
@@ -20,10 +20,11 @@ import app.cash.molecule.RecompositionMode
 import app.cash.molecule.moleculeFlow
 import app.cash.turbine.test
 import com.google.common.truth.Truth.assertThat
-import io.element.android.features.rageshake.api.logs.LogFilesRemover
-import io.element.android.features.rageshake.test.logs.FakeLogFilesRemover
+import io.element.android.features.migration.impl.migrations.AppMigration
 import io.element.android.libraries.architecture.AsyncData
 import io.element.android.tests.testutils.WarmUpRule
+import io.element.android.tests.testutils.consumeItemsUntilPredicate
+import io.element.android.tests.testutils.lambda.LambdaNoParamRecorder
 import io.element.android.tests.testutils.lambda.lambdaRecorder
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.test.runTest
@@ -36,9 +37,11 @@ class MigrationPresenterTest {

    @Test
    fun `present - no migration should occurs if ApplicationMigrationVersion is the last one`() = runTest {
-        val store = InMemoryMigrationStore(MigrationPresenter.MIGRATION_VERSION)
+        val migrations = (1..10).map { FakeMigration(it) }
+        val store = InMemoryMigrationStore(migrations.maxOf { it.order })
        val presenter = createPresenter(
            migrationStore = store,
+            migrations = migrations.toSet(),
        )
        moleculeFlow(RecompositionMode.Immediate) {
            presenter.present()
@@ -54,10 +57,10 @@ class MigrationPresenterTest {
    @Test
    fun `present - testing all migrations`() = runTest {
        val store = InMemoryMigrationStore(0)
-        val logFilesRemoverLambda = lambdaRecorder { -> }
+        val migrations = (1..10).map { FakeMigration(it) }
        val presenter = createPresenter(
            migrationStore = store,
-            logFilesRemover = FakeLogFilesRemover(logFilesRemoverLambda),
+            migrations = migrations.toSet(),
        )
        moleculeFlow(RecompositionMode.Immediate) {
            presenter.present()
@@ -67,19 +70,28 @@ class MigrationPresenterTest {
            awaitItem().also { state ->
                assertThat(state.migrationAction).isEqualTo(AsyncData.Loading(Unit))
            }
-            awaitItem().also { state ->
-                assertThat(state.migrationAction).isEqualTo(AsyncData.Success(Unit))
+            consumeItemsUntilPredicate { it.migrationAction is AsyncData.Success }
+            assertThat(store.applicationMigrationVersion().first()).isEqualTo(migrations.maxOf { it.order })
+            for (migration in migrations) {
+                migration.migrateLambda.assertions().isCalledOnce()
            }
-            logFilesRemoverLambda.assertions().isCalledExactly(1)
-            assertThat(store.applicationMigrationVersion().first()).isEqualTo(MigrationPresenter.MIGRATION_VERSION)
        }
    }
 }

 private fun createPresenter(
    migrationStore: MigrationStore = InMemoryMigrationStore(0),
-    logFilesRemover: LogFilesRemover = FakeLogFilesRemover(lambdaRecorder(ensureNeverCalled = true) { -> }),
+    migrations: Set<AppMigration> = setOf(FakeMigration(1)),
 ) = MigrationPresenter(
    migrationStore = migrationStore,
-    logFilesRemover = logFilesRemover,
+    migrations = migrations,
 )
+
+private class FakeMigration(
+    override val order: Int,
+    var migrateLambda: LambdaNoParamRecorder<Unit> = lambdaRecorder { -> },
+) : AppMigration {
+    override suspend fun migrate() {
+        migrateLambda()
+    }
+}
--- a/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration01Test.kt
+++ b/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration01Test.kt
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import io.element.android.features.rageshake.test.logs.FakeLogFilesRemover
+import kotlinx.coroutines.test.runTest
+import org.junit.Test
+
+class AppMigration01Test {
+    @Test
+    fun `test migration`() = runTest {
+        val logsFileRemover = FakeLogFilesRemover()
+        val migration = AppMigration01(logsFileRemover)
+
+        migration.migrate()
+
+        logsFileRemover.performLambda.assertions().isCalledOnce()
+    }
+}
--- a/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration02Test.kt
+++ b/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration02Test.kt
@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import com.google.common.truth.Truth.assertThat
+import io.element.android.libraries.preferences.test.FakeSessionPreferenceStoreFactory
+import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
+import io.element.android.libraries.sessionstorage.impl.memory.InMemorySessionStore
+import io.element.android.libraries.sessionstorage.test.aSessionData
+import io.element.android.tests.testutils.lambda.lambdaRecorder
+import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.test.runTest
+import org.junit.Test
+
+class AppMigration02Test {
+    @Test
+    fun `test migration`() = runTest {
+        val sessionStore = InMemorySessionStore().apply {
+            updateData(aSessionData())
+        }
+        val sessionPreferencesStore = InMemorySessionPreferencesStore(isSessionVerificationSkipped = false)
+        val sessionPreferencesStoreFactory = FakeSessionPreferenceStoreFactory(
+            getLambda = lambdaRecorder { _, _, -> sessionPreferencesStore },
+        )
+        val migration = AppMigration02(sessionStore = sessionStore, sessionPreferenceStoreFactory = sessionPreferencesStoreFactory)
+
+        migration.migrate()
+
+        // We got the session preferences store
+        sessionPreferencesStoreFactory.getLambda.assertions().isCalledOnce()
+        // We changed the settings for the skipping the session verification
+        assertThat(sessionPreferencesStore.isSessionVerificationSkipped().first()).isTrue()
+        // We removed the session preferences store from cache
+        sessionPreferencesStoreFactory.removeLambda.assertions().isCalledOnce()
+    }
+}