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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Dec 20, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5500f92
PM-16062 prevent account lock when new activity is created specifical…
dseverns-livefront Dec 19, 2024
1f583de
update docs
dseverns-livefront Dec 20, 2024
40dc0e9
parameter name
dseverns-livefront Dec 20, 2024
1373951
pr feedback: formatting, docs, and making sure timer is cancelled whe…
dseverns-livefront Dec 20, 2024
ea6f0d6
feedback, fmting, attempting to rightsize tests
dseverns-livefront Dec 20, 2024
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
10 changes: 10 additions & 0 deletions app/src/main/java/com/x8bit/bitwarden/data/autofill/util/AutofillIntentUtils.kt
View file
Open in desktop
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
dseverns-livefront marked this conversation as resolved.
Show resolved Hide resolved
get() = intent.getAutofillSelectionDataOrNull() != null ||
intent.getAutofillSaveItemOrNull() != null ||
intent.getAutofillAssistStructureOrNull() != null
10 changes: 7 additions & 3 deletions app/src/main/java/com/x8bit/bitwarden/data/platform/manager/AppStateManagerImpl.kt
View file
Open in desktop
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
7 changes: 4 additions & 3 deletions app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/AppCreationState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ 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.
dseverns-livefront marked this conversation as resolved.
Show resolved Hide resolved
*/
CREATED,
data class Created(val isAutoFill: Boolean) : AppCreationState()

/**
* Denotes that the app is currently destroyed.
*/
DESTROYED,
data object Destroyed : AppCreationState()
}
104 changes: 62 additions & 42 deletions app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,29 +305,38 @@ class VaultLockManagerImpl(
}

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Borderline but if I remember correctly after the first param we'd usually break these onto new lines so its easier to add a 3rd, 4th, etc. later.

val userId = activeUserId ?: return
userIdTimerJobMap[userId]?.cancel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little nervous about moving userIdTimerJobMap[userId]?.cancel() to handleOnCreated now that I see this. I wonder if we need to keep handleOnForeground just for this. I believe the intention of this line is to say "whenever the app is foregrounded, we shouldn't have a timer going for the current user". In which case moving this here is a behavior change.

checkForVaultTimeout(
userId = userId,
checkTimeoutReason = CheckTimeoutReason.AppCreated(
firstTimeCreation = isFirstCreated,
createdForAutofill = createdForAutofill,
),
)
}

private fun observeAppForegroundChanges() {
var isFirstForeground = true

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

AppForegroundState.FOREGROUNDED -> {
handleOnForeground(isFirstForeground = isFirstForeground)
isFirstForeground = false
}
AppForegroundState.FOREGROUNDED -> Unit
}
}
.launchIn(unconfinedScope)
Expand All @@ -349,21 +355,10 @@ class VaultLockManagerImpl(
val userId = activeUserId ?: return
checkForVaultTimeout(
userId = userId,
checkTimeoutReason = CheckTimeoutReason.APP_BACKGROUNDED,
checkTimeoutReason = CheckTimeoutReason.AppBackgrounded,
)
}

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

private fun observeUserSwitchingChanges() {
authDiskSource
.userSwitchingChangesFlow
Expand Down Expand Up @@ -461,7 +456,7 @@ class VaultLockManagerImpl(
// 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 +486,37 @@ class VaultLockManagerImpl(

VaultTimeout.OnAppRestart -> {
// If this is an app restart, trigger the timeout action; otherwise ignore.
if (checkTimeoutReason == CheckTimeoutReason.APP_RESTARTED) {
if (checkTimeoutReason is CheckTimeoutReason.AppCreated) {
// On restart the vault should be locked already but we may need to soft-logout
// the user.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doc likely needs updating because right now its only discussing the firstTimeCreation case.

handleTimeoutAction(userId = userId, vaultTimeoutAction = vaultTimeoutAction)
if (
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) {
dseverns-livefront marked this conversation as resolved.
Show resolved Hide resolved
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 +594,26 @@ class VaultLockManagerImpl(
}

/**
* 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) :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's update the formatting to

        data class AppCreated(
            val firstTimeCreation: Boolean,
            val createdForAutofill: Boolean
        ) :CheckTimeoutReason()

CheckTimeoutReason()

/**
* Indicates that the current user has changed.
*/
data object UserChanged : CheckTimeoutReason()
}
}
12 changes: 9 additions & 3 deletions app/src/test/java/com/x8bit/bitwarden/data/platform/manager/AppStateManagerTest.kt
View file
Open in desktop
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)
}
}
3 changes: 2 additions & 1 deletion app/src/test/java/com/x8bit/bitwarden/data/platform/manager/util/FakeAppStateManager.kt
View file
Open in desktop
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