Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce mandatory session verification only for new logins #2811

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


@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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be moved to the api module, so that any module may contribute to migrate their internal state. But maybe keep this for later.

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
Loading