Skip to content

PM-16062 Prevent account locks for ongoing autofill requests #4498

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

Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

package com.x8bit.bitwarden.data.autofill.util

import android.app.Activity
import android.app.PendingIntent
import android.app.assist.AssistStructure
import android.content.Context
Expand Down Expand Up @@ -147,3 +148,12 @@ fun Intent.getAutofillSelectionDataOrNull(): AutofillSelectionData? =
fun Intent.getTotpCopyIntentOrNull(): AutofillTotpCopyData? =
getBundleExtra(AUTOFILL_BUNDLE_KEY)
?.getSafeParcelableExtra(AUTOFILL_TOTP_COPY_DATA_KEY)

/**
* Checks if the given [Activity] was created for Autofill. This is useful to avoid locking the
* vault if one of the Autofill services starts the only only instance of the [MainActivity].
*/
val Activity.createdForAutofill: Boolean
get() = intent.getAutofillSelectionDataOrNull() != null ||
intent.getAutofillSaveItemOrNull() != null ||
intent.getAutofillAssistStructureOrNull() != null
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import android.os.Bundle
import androidx.lifecycle.DefaultLifecycleObserver
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.ProcessLifecycleOwner
import com.x8bit.bitwarden.data.autofill.util.createdForAutofill
import com.x8bit.bitwarden.data.platform.manager.model.AppCreationState
import com.x8bit.bitwarden.data.platform.manager.model.AppForegroundState
import kotlinx.coroutines.flow.MutableStateFlow
Expand All @@ -19,7 +20,8 @@ class AppStateManagerImpl(
application: Application,
processLifecycleOwner: LifecycleOwner = ProcessLifecycleOwner.get(),
) : AppStateManager {
private val mutableAppCreationStateFlow = MutableStateFlow(AppCreationState.DESTROYED)
private val mutableAppCreationStateFlow =
MutableStateFlow<AppCreationState>(AppCreationState.Destroyed)
private val mutableAppForegroundStateFlow = MutableStateFlow(AppForegroundState.BACKGROUNDED)

override val appCreatedStateFlow: StateFlow<AppCreationState>
Expand Down Expand Up @@ -49,13 +51,15 @@ class AppStateManagerImpl(
override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) {
activityCount++
// Always be in a created state if we have an activity
mutableAppCreationStateFlow.value = AppCreationState.CREATED
mutableAppCreationStateFlow.value = AppCreationState.Created(
isAutoFill = activity.createdForAutofill,
)
}

override fun onActivityDestroyed(activity: Activity) {
activityCount--
if (activityCount == 0 && !activity.isChangingConfigurations) {
mutableAppCreationStateFlow.value = AppCreationState.DESTROYED
mutableAppCreationStateFlow.value = AppCreationState.Destroyed
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@ package com.x8bit.bitwarden.data.platform.manager.model
/**
* Represents the creation state of the app.
*/
enum class AppCreationState {
sealed class AppCreationState {
/**
* Denotes that the app is currently created.
*
* @param isAutoFill Whether the app was created for autofill.
*/
CREATED,
data class Created(val isAutoFill: Boolean) : AppCreationState()

/**
* Denotes that the app is currently destroyed.
*/
DESTROYED,
data object Destroyed : AppCreationState()
}
Original file line number Diff line number Diff line change
Expand Up @@ -305,29 +305,40 @@
}

private fun observeAppCreationChanges() {
var isFirstCreated = true
appStateManager
.appCreatedStateFlow
.onEach { appCreationState ->
when (appCreationState) {
AppCreationState.CREATED -> Unit
AppCreationState.DESTROYED -> handleOnDestroyed()
is AppCreationState.Created -> {
handleOnCreated(
createdForAutofill = appCreationState.isAutoFill,
isFirstCreated = isFirstCreated,
)
isFirstCreated = false
}

AppCreationState.Destroyed -> Unit
}
}
.launchIn(unconfinedScope)
}

private fun handleOnDestroyed() {
activeUserId?.let { userId ->
checkForVaultTimeout(
userId = userId,
checkTimeoutReason = CheckTimeoutReason.APP_RESTARTED,
)
}
private fun handleOnCreated(
createdForAutofill: Boolean,
isFirstCreated: Boolean,
) {
val userId = activeUserId ?: return
checkForVaultTimeout(
userId = userId,
checkTimeoutReason = CheckTimeoutReason.AppCreated(
firstTimeCreation = isFirstCreated,
createdForAutofill = createdForAutofill,
),
)
}

private fun observeAppForegroundChanges() {
var isFirstForeground = true

appStateManager
.appForegroundStateFlow
.onEach { appForegroundState ->
Expand All @@ -336,10 +347,7 @@
handleOnBackground()
}

AppForegroundState.FOREGROUNDED -> {
handleOnForeground(isFirstForeground = isFirstForeground)
isFirstForeground = false
}
AppForegroundState.FOREGROUNDED -> handleOnForeground()
}
}
.launchIn(unconfinedScope)
Expand All @@ -349,19 +357,13 @@
val userId = activeUserId ?: return
checkForVaultTimeout(
userId = userId,
checkTimeoutReason = CheckTimeoutReason.APP_BACKGROUNDED,
checkTimeoutReason = CheckTimeoutReason.AppBackgrounded,
)
}

private fun handleOnForeground(isFirstForeground: Boolean) {
private fun handleOnForeground() {
val userId = activeUserId ?: return
userIdTimerJobMap[userId]?.cancel()
if (isFirstForeground) {
checkForVaultTimeout(
userId = userId,
checkTimeoutReason = CheckTimeoutReason.APP_RESTARTED,
)
}
}

private fun observeUserSwitchingChanges() {
Expand Down Expand Up @@ -461,7 +463,7 @@
// Check if the user's timeout action should be performed as we switch away.
checkForVaultTimeout(
userId = previousActiveUserId,
checkTimeoutReason = CheckTimeoutReason.USER_CHANGED,
checkTimeoutReason = CheckTimeoutReason.UserChanged,
)
}

Expand Down Expand Up @@ -491,27 +493,38 @@

VaultTimeout.OnAppRestart -> {
// If this is an app restart, trigger the timeout action; otherwise ignore.
if (checkTimeoutReason == CheckTimeoutReason.APP_RESTARTED) {
// On restart the vault should be locked already but we may need to soft-logout
// the user.
handleTimeoutAction(userId = userId, vaultTimeoutAction = vaultTimeoutAction)
if (checkTimeoutReason is CheckTimeoutReason.AppCreated) {
// We need to check the timeout action on the first time creation no matter what
// for all subsequent creations we should check if this is for autofill and
// and if it is we skip checking the timeout action.
if (

Check warning on line 500 in app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

View check run for this annotation

Codecov / codecov/patch

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt#L500

Added line #L500 was not covered by tests
checkTimeoutReason.firstTimeCreation ||
!checkTimeoutReason.createdForAutofill
) {
handleTimeoutAction(
userId = userId,
vaultTimeoutAction = vaultTimeoutAction,
)
}
}
}

else -> {
when (checkTimeoutReason) {
// Always preform the timeout action on app restart to ensure the user is
// in the correct state.
CheckTimeoutReason.APP_RESTARTED -> {
handleTimeoutAction(
userId = userId,
vaultTimeoutAction = vaultTimeoutAction,
)
is CheckTimeoutReason.AppCreated -> {
if (checkTimeoutReason.firstTimeCreation) {
handleTimeoutAction(
userId = userId,
vaultTimeoutAction = vaultTimeoutAction,
)
}
}

// User no longer active or engaging with the app.
CheckTimeoutReason.APP_BACKGROUNDED,
CheckTimeoutReason.USER_CHANGED,
CheckTimeoutReason.AppBackgrounded,
CheckTimeoutReason.UserChanged,
-> {
handleTimeoutActionWithDelay(
userId = userId,
Expand Down Expand Up @@ -589,11 +602,29 @@
}

/**
* Helper enum that indicates the reason we are checking for timeout.
* Helper sealed class which denotes the reason to check the vault timeout.
*/
private enum class CheckTimeoutReason {
APP_BACKGROUNDED,
APP_RESTARTED,
USER_CHANGED,
private sealed class CheckTimeoutReason {
/**
* Indicates the app has been backgrounded but is still running.
*/
data object AppBackgrounded : CheckTimeoutReason()

/**
* Indicates the app has entered a Created state.
*
* @param firstTimeCreation if this is the first time the process is being created.
* @param createdForAutofill if the the creation event is due to an activity being launched
* for autofill.
*/
data class AppCreated(
val firstTimeCreation: Boolean,
val createdForAutofill: Boolean,
) : CheckTimeoutReason()

/**
* Indicates that the current user has changed.
*/
data object UserChanged : CheckTimeoutReason()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@ package com.x8bit.bitwarden.data.platform.manager
import android.app.Activity
import android.app.Application
import app.cash.turbine.test
import com.x8bit.bitwarden.data.autofill.util.createdForAutofill
import com.x8bit.bitwarden.data.platform.manager.model.AppCreationState
import com.x8bit.bitwarden.data.platform.manager.model.AppForegroundState
import com.x8bit.bitwarden.data.util.FakeLifecycleOwner
import io.mockk.every
import io.mockk.just
import io.mockk.mockk
import io.mockk.mockkStatic
import io.mockk.runs
import io.mockk.slot
import io.mockk.unmockkStatic
import kotlinx.coroutines.test.runTest
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Test
Expand Down Expand Up @@ -59,15 +62,17 @@ class AppStateManagerTest {
@Test
fun `appCreatedStateFlow should emit whenever the underlying activities are all destroyed or a creation event occurs`() =
runTest {
mockkStatic(Activity::createdForAutofill)
val activity = mockk<Activity> {
every { isChangingConfigurations } returns false
every { createdForAutofill } returns false
}
appStateManager.appCreatedStateFlow.test {
// Initial state is DESTROYED
assertEquals(AppCreationState.DESTROYED, awaitItem())
assertEquals(AppCreationState.Destroyed, awaitItem())

activityLifecycleCallbacks.captured.onActivityCreated(activity, null)
assertEquals(AppCreationState.CREATED, awaitItem())
assertEquals(AppCreationState.Created(isAutoFill = false), awaitItem())

activityLifecycleCallbacks.captured.onActivityCreated(activity, null)
expectNoEvents()
Expand All @@ -76,7 +81,8 @@ class AppStateManagerTest {
expectNoEvents()

activityLifecycleCallbacks.captured.onActivityDestroyed(activity)
assertEquals(AppCreationState.DESTROYED, awaitItem())
assertEquals(AppCreationState.Destroyed, awaitItem())
}
unmockkStatic(Activity::createdForAutofill)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import kotlinx.coroutines.flow.asStateFlow
* A faked implementation of [AppStateManager]
*/
class FakeAppStateManager : AppStateManager {
private val mutableAppCreationStateFlow = MutableStateFlow(AppCreationState.DESTROYED)
private val mutableAppCreationStateFlow =
MutableStateFlow<AppCreationState>(AppCreationState.Destroyed)
private val mutableAppForegroundStateFlow = MutableStateFlow(AppForegroundState.BACKGROUNDED)

override val appCreatedStateFlow: StateFlow<AppCreationState>
Expand Down
Loading