Skip to content

Commit

Permalink
Enforce mandatory session verification only for new logins (#2811)
Browse files Browse the repository at this point in the history
* 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`.
  • Loading branch information
jmartinesp committed May 7, 2024
1 parent 2007752 commit 5c59f6c
Show file tree
Hide file tree
Showing 30 changed files with 370 additions and 51 deletions.
1 change: 1 addition & 0 deletions changelog.d/2810.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Enforce mandatory session verification only for new logins.
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package io.element.android.features.call.utils

import com.google.common.truth.Truth.assertThat
import io.element.android.features.preferences.api.store.AppPreferencesStore
import io.element.android.libraries.featureflag.test.InMemoryAppPreferencesStore
import io.element.android.libraries.matrix.api.MatrixClientProvider
import io.element.android.libraries.matrix.api.widget.CallWidgetSettingsProvider
import io.element.android.libraries.matrix.test.A_ROOM_ID
Expand All @@ -28,6 +27,7 @@ import io.element.android.libraries.matrix.test.FakeMatrixClientProvider
import io.element.android.libraries.matrix.test.room.FakeMatrixRoom
import io.element.android.libraries.matrix.test.widget.FakeCallWidgetSettingsProvider
import io.element.android.libraries.matrix.test.widget.FakeWidgetDriver
import io.element.android.libraries.preferences.test.InMemoryAppPreferencesStore
import kotlinx.coroutines.test.runTest
import org.junit.Test

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ import io.element.android.features.ftue.impl.state.DefaultFtueService
import io.element.android.features.ftue.impl.state.FtueStep
import io.element.android.features.lockscreen.api.LockScreenService
import io.element.android.features.lockscreen.test.FakeLockScreenService
import io.element.android.libraries.featureflag.test.InMemorySessionPreferencesStore
import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus
import io.element.android.libraries.matrix.test.verification.FakeSessionVerificationService
import io.element.android.libraries.permissions.impl.FakePermissionStateProvider
import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
import io.element.android.services.analytics.api.AnalyticsService
import io.element.android.services.analytics.test.FakeAnalyticsService
import io.element.android.services.toolbox.test.sdk.FakeBuildVersionSdkIntProvider
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,6 @@ import io.element.android.libraries.designsystem.components.avatar.AvatarSize
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher
import io.element.android.libraries.featureflag.api.FeatureFlags
import io.element.android.libraries.featureflag.test.FakeFeatureFlagService
import io.element.android.libraries.featureflag.test.InMemoryAppPreferencesStore
import io.element.android.libraries.featureflag.test.InMemorySessionPreferencesStore
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.api.room.MatrixRoom
Expand Down Expand Up @@ -93,6 +91,8 @@ import io.element.android.libraries.mediaviewer.test.FakeLocalMediaFactory
import io.element.android.libraries.permissions.api.PermissionsPresenter
import io.element.android.libraries.permissions.test.FakePermissionsPresenter
import io.element.android.libraries.permissions.test.FakePermissionsPresenterFactory
import io.element.android.libraries.preferences.test.InMemoryAppPreferencesStore
import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
import io.element.android.libraries.textcomposer.model.MessageComposerMode
import io.element.android.libraries.voicerecorder.test.FakeVoiceRecorder
import io.element.android.services.analytics.test.FakeAnalyticsService
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ import io.element.android.features.messages.impl.timeline.model.event.aTimelineI
import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemStateEventContent
import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemVoiceContent
import io.element.android.features.poll.api.pollcontent.aPollAnswerItemList
import io.element.android.libraries.featureflag.test.InMemoryAppPreferencesStore
import io.element.android.libraries.matrix.test.A_MESSAGE
import io.element.android.libraries.preferences.test.InMemoryAppPreferencesStore
import io.element.android.tests.testutils.WarmUpRule
import kotlinx.collections.immutable.persistentListOf
import kotlinx.coroutines.test.runTest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatch
import io.element.android.libraries.featureflag.api.FeatureFlagService
import io.element.android.libraries.featureflag.api.FeatureFlags
import io.element.android.libraries.featureflag.test.FakeFeatureFlagService
import io.element.android.libraries.featureflag.test.InMemorySessionPreferencesStore
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.core.TransactionId
import io.element.android.libraries.matrix.api.media.ImageInfo
Expand Down Expand Up @@ -77,6 +76,7 @@ import io.element.android.libraries.mediaviewer.test.FakeLocalMediaFactory
import io.element.android.libraries.permissions.api.PermissionsPresenter
import io.element.android.libraries.permissions.test.FakePermissionsPresenter
import io.element.android.libraries.permissions.test.FakePermissionsPresenterFactory
import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
import io.element.android.libraries.textcomposer.model.Message
import io.element.android.libraries.textcomposer.model.MessageComposerMode
import io.element.android.libraries.textcomposer.model.Suggestion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import io.element.android.features.poll.api.actions.EndPollAction
import io.element.android.features.poll.api.actions.SendPollResponseAction
import io.element.android.features.poll.test.actions.FakeEndPollAction
import io.element.android.features.poll.test.actions.FakeSendPollResponseAction
import io.element.android.libraries.featureflag.test.InMemorySessionPreferencesStore
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState
import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem
Expand All @@ -53,6 +52,7 @@ import io.element.android.libraries.matrix.test.timeline.FakeTimeline
import io.element.android.libraries.matrix.test.timeline.aMessageContent
import io.element.android.libraries.matrix.test.timeline.anEventTimelineItem
import io.element.android.libraries.matrix.ui.components.aMatrixUserList
import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
import io.element.android.tests.testutils.WarmUpRule
import io.element.android.tests.testutils.awaitLastSequentialItem
import io.element.android.tests.testutils.consumeItemsUntilPredicate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import app.cash.turbine.Event
import app.cash.turbine.test
import com.google.common.truth.Truth.assertThat
import io.element.android.features.preferences.api.store.SessionPreferencesStore
import io.element.android.libraries.featureflag.test.InMemorySessionPreferencesStore
import io.element.android.libraries.matrix.api.core.UserId
import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState
Expand All @@ -32,6 +31,7 @@ import io.element.android.libraries.matrix.test.A_USER_ID_3
import io.element.android.libraries.matrix.test.A_USER_ID_4
import io.element.android.libraries.matrix.test.room.FakeMatrixRoom
import io.element.android.libraries.matrix.test.room.aRoomInfo
import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
import io.element.android.tests.testutils.WarmUpRule
import kotlinx.collections.immutable.toImmutableList
import kotlinx.coroutines.test.runTest
Expand Down
6 changes: 6 additions & 0 deletions features/migration/impl/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Original file line number Diff line number Diff line change
@@ -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()
}
Original file line number Diff line number Diff line change
@@ -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()
}
}
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
}
}
Loading

0 comments on commit 5c59f6c

Please sign in to comment.