-
Notifications
You must be signed in to change notification settings - Fork 918
[PM-27150] React to device changes on device screen unlock method #6103
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
Changes from 6 commits
e55b8b7
3533af5
46c1cb3
436bcc1
793e798
3ce913d
e7ac063
85ff4e7
de0947a
12c28c2
3afe1b3
790ccd0
385caae
9e28ed5
8492014
4556e1a
9180c52
2de0197
df27f32
25f028c
d09486f
3018a6c
3c01f60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,10 @@ | ||
| package com.bitwarden.authenticator.data.auth.datasource.disk | ||
|
|
||
| import android.content.SharedPreferences | ||
| import com.bitwarden.core.data.repository.util.bufferedMutableSharedFlow | ||
| import com.bitwarden.data.datasource.disk.BaseEncryptedDiskSource | ||
| import kotlinx.coroutines.flow.Flow | ||
| import kotlinx.coroutines.flow.onSubscription | ||
| import java.util.UUID | ||
|
|
||
| private const val AUTHENTICATOR_SYNC_SYMMETRIC_KEY = "authenticatorSyncSymmetricKey" | ||
|
|
@@ -20,6 +23,8 @@ class AuthDiskSourceImpl( | |
| sharedPreferences = sharedPreferences, | ||
| ), | ||
| AuthDiskSource { | ||
| private val mutableUserBiometricUnlockKeyFlow = | ||
| bufferedMutableSharedFlow<String?>() | ||
|
|
||
| override val uniqueAppId: String | ||
| get() = getString(key = UNIQUE_APP_ID_KEY) ?: generateAndStoreUniqueAppId() | ||
|
|
@@ -39,13 +44,17 @@ class AuthDiskSourceImpl( | |
| override fun getUserBiometricUnlockKey(): String? = | ||
| getEncryptedString(key = BIOMETRICS_UNLOCK_KEY) | ||
|
|
||
| override fun getUserBiometricUnlockKeyFlow(): Flow<String?> = mutableUserBiometricUnlockKeyFlow | ||
|
||
| .onSubscription { emit(getUserBiometricUnlockKey()) } | ||
|
|
||
| override fun storeUserBiometricUnlockKey( | ||
| biometricsKey: String?, | ||
| ) { | ||
| putEncryptedString( | ||
| key = BIOMETRICS_UNLOCK_KEY, | ||
| value = biometricsKey, | ||
| ) | ||
| mutableUserBiometricUnlockKeyFlow.tryEmit(biometricsKey) | ||
| } | ||
|
|
||
| override var authenticatorBridgeSymmetricSyncKey: ByteArray? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,12 @@ class SettingsRepositoryImpl( | |
| override val isUnlockWithBiometricsEnabled: Boolean | ||
| get() = authDiskSource.getUserBiometricUnlockKey() != null | ||
|
|
||
| override val isUnlockWithBiometricsEnabledFlow: Flow<Boolean> | ||
| get() = | ||
| authDiskSource | ||
| .getUserBiometricUnlockKeyFlow() | ||
| .map { it != null } | ||
|
||
|
|
||
| override val appThemeStateFlow: StateFlow<AppTheme> | ||
| get() = settingsDiskSource | ||
| .appThemeFlow | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import androidx.compose.foundation.layout.padding | |
| import androidx.compose.foundation.layout.width | ||
| import androidx.compose.material3.ExperimentalMaterial3Api | ||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.runtime.LaunchedEffect | ||
| import androidx.compose.runtime.getValue | ||
| import androidx.compose.runtime.mutableStateOf | ||
| import androidx.compose.runtime.remember | ||
|
|
@@ -53,6 +54,12 @@ fun UnlockScreen( | |
| } | ||
| } | ||
|
|
||
| LaunchedEffect(biometricsManager.isBiometricsSupported) { | ||
|
||
| if (!biometricsManager.isBiometricsSupported) { | ||
| onUnlocked() | ||
| } | ||
| } | ||
|
||
|
|
||
| when (val dialog = state.dialog) { | ||
| is UnlockState.Dialog.Error -> BitwardenBasicDialog( | ||
| title = stringResource(id = BitwardenString.an_error_has_occurred), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import androidx.compose.runtime.collectAsState | |
| import androidx.compose.runtime.getValue | ||
| import androidx.compose.runtime.remember | ||
| import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel | ||
| import androidx.lifecycle.Lifecycle | ||
| import androidx.navigation.NavBackStackEntry | ||
| import androidx.navigation.NavDestination | ||
| import androidx.navigation.NavHostController | ||
|
|
@@ -19,13 +20,16 @@ import com.bitwarden.authenticator.ui.auth.unlock.unlockDestination | |
| import com.bitwarden.authenticator.ui.authenticator.feature.authenticator.AuthenticatorGraphRoute | ||
| import com.bitwarden.authenticator.ui.authenticator.feature.authenticator.authenticatorGraph | ||
| import com.bitwarden.authenticator.ui.authenticator.feature.authenticator.navigateToAuthenticatorGraph | ||
| import com.bitwarden.authenticator.ui.platform.composition.LocalBiometricsManager | ||
| import com.bitwarden.authenticator.ui.platform.feature.debugmenu.setupDebugMenuDestination | ||
| import com.bitwarden.authenticator.ui.platform.feature.splash.SplashRoute | ||
| import com.bitwarden.authenticator.ui.platform.feature.splash.navigateToSplash | ||
| import com.bitwarden.authenticator.ui.platform.feature.splash.splashDestination | ||
| import com.bitwarden.authenticator.ui.platform.feature.tutorial.TutorialRoute | ||
| import com.bitwarden.authenticator.ui.platform.feature.tutorial.navigateToTutorial | ||
| import com.bitwarden.authenticator.ui.platform.feature.tutorial.tutorialDestination | ||
| import com.bitwarden.authenticator.ui.platform.manager.biometrics.BiometricsManager | ||
| import com.bitwarden.ui.platform.base.util.LifecycleEventEffect | ||
| import com.bitwarden.ui.platform.theme.NonNullEnterTransitionProvider | ||
| import com.bitwarden.ui.platform.theme.NonNullExitTransitionProvider | ||
| import com.bitwarden.ui.platform.theme.RootTransitionProviders | ||
|
|
@@ -42,6 +46,7 @@ import java.util.concurrent.atomic.AtomicReference | |
| fun RootNavScreen( | ||
| viewModel: RootNavViewModel = hiltViewModel(), | ||
| navController: NavHostController = rememberNavController(), | ||
| biometricsManager: BiometricsManager = LocalBiometricsManager.current, | ||
| onSplashScreenRemoved: () -> Unit = {}, | ||
| onExitApplication: () -> Unit, | ||
| ) { | ||
|
|
@@ -63,6 +68,13 @@ fun RootNavScreen( | |
| .launchIn(this) | ||
| } | ||
|
|
||
| BiometricChanges( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ Finding 5: Well-structured biometric monitoring implementation DetailsThe โ Separated into a reusable composable function with comprehensive KDoc Architectural correctness: Placing this at the RootNav level is appropriate since:
This approach follows MVVM principles and maintains a single source of truth for biometric support status. |
||
| biometricsManager = biometricsManager, | ||
| onBiometricSupportChange = { | ||
| viewModel.trySendAction(RootNavAction.Internal.BiometricSupportChanged(it)) | ||
|
||
| }, | ||
david-livefront marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DetailsPer david-livefront's comment, the callback should be wrapped in onBiometricSupportChange = remember(viewModel) {
{ viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it)) }
},This prevents unnecessary recompositions of |
||
| ) | ||
|
|
||
| NavHost( | ||
| navController = navController, | ||
| startDestination = SplashRoute, | ||
|
|
@@ -187,3 +199,18 @@ private fun AnimatedContentTransitionScope<NavBackStackEntry>.toExitTransition() | |
| else -> RootTransitionProviders.Exit.fadeOut | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| private fun BiometricChanges( | ||
|
||
| biometricsManager: BiometricsManager, | ||
| onBiometricSupportChange: (isSupported: Boolean) -> Unit, | ||
| ) { | ||
| LifecycleEventEffect { _, event -> | ||
| when (event) { | ||
| Lifecycle.Event.ON_RESUME -> { | ||
| onBiometricSupportChange(biometricsManager.isBiometricsSupported) | ||
| } | ||
| else -> Unit | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,10 @@ class RootNavViewModel @Inject constructor( | |
| RootNavAction.Internal.AppUnlocked -> { | ||
| handleAppUnlocked() | ||
| } | ||
|
|
||
| is RootNavAction.Internal.BiometricSupportChanged -> { | ||
| handleBiometricSupportChanged(action.isBiometricsSupported) | ||
|
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -99,6 +103,12 @@ class RootNavViewModel @Inject constructor( | |
| it.copy(navState = RootNavState.NavState.Unlocked) | ||
| } | ||
| } | ||
|
|
||
| private fun handleBiometricSupportChanged(isBiometricsSupported: Boolean) { | ||
| if (!isBiometricsSupported) { | ||
| settingsRepository.clearBiometricsKey() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐จ Consider extracting navigation state check to improve readability DetailsThe current implementation checks the navigation state inline: if (mutableStateFlow.value.navState is RootNavState.NavState.Locked) {
mutableStateFlow.update { it.copy(navState = RootNavState.NavState.Unlocked) }
}Minor improvement: Extract to a local variable for clarity: private fun handleBiometricSupportChanged(
action: RootNavAction.BiometricSupportChanged,
) {
if (!action.isBiometricsSupported) {
settingsRepository.clearBiometricsKey()
// If currently locked, navigate to unlocked since biometrics are no longer available
val isCurrentlyLocked = mutableStateFlow.value.navState is RootNavState.NavState.Locked
if (isCurrentlyLocked) {
mutableStateFlow.update { it.copy(navState = RootNavState.NavState.Unlocked) }
}
}
}This makes the intent clearer and slightly improves readability, though the current implementation is acceptable. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -173,6 +183,11 @@ sealed class RootNavAction { | |
| */ | ||
| data object AppUnlocked : Internal() | ||
|
|
||
| /** | ||
| * Indicates an update on device biometrics support. | ||
| */ | ||
| data class BiometricSupportChanged(val isBiometricsSupported: Boolean) : Internal() | ||
|
|
||
| /** | ||
| * Indicates an update in the welcome guide being seen has been received. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,12 +41,14 @@ import androidx.compose.ui.tooling.preview.Preview | |
| import androidx.compose.ui.unit.dp | ||
| import androidx.core.net.toUri | ||
| import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel | ||
| import androidx.lifecycle.Lifecycle | ||
| import androidx.lifecycle.compose.collectAsStateWithLifecycle | ||
| import com.bitwarden.authenticator.ui.platform.composition.LocalBiometricsManager | ||
| import com.bitwarden.authenticator.ui.platform.feature.settings.data.model.DefaultSaveOption | ||
| import com.bitwarden.authenticator.ui.platform.manager.biometrics.BiometricsManager | ||
| import com.bitwarden.authenticator.ui.platform.util.displayLabel | ||
| import com.bitwarden.ui.platform.base.util.EventsEffect | ||
| import com.bitwarden.ui.platform.base.util.LifecycleEventEffect | ||
| import com.bitwarden.ui.platform.base.util.annotatedStringResource | ||
| import com.bitwarden.ui.platform.base.util.cardStyle | ||
| import com.bitwarden.ui.platform.base.util.mirrorIfRtl | ||
|
|
@@ -88,6 +90,14 @@ fun SettingsScreen( | |
| ) { | ||
| val state by viewModel.stateFlow.collectAsStateWithLifecycle() | ||
| val scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior() | ||
| var hasBiometrics by remember { mutableStateOf(biometricsManager.isBiometricsSupported) } | ||
|
||
|
|
||
| // Recheck biometrics support when app comes to foreground | ||
| LifecycleEventEffect { _, event -> | ||
| if (event == Lifecycle.Event.ON_RESUME) { | ||
| hasBiometrics = biometricsManager.isBiometricsSupported | ||
| } | ||
| } | ||
|
|
||
| EventsEffect(viewModel = viewModel) { event -> | ||
| when (event) { | ||
|
|
@@ -151,6 +161,7 @@ fun SettingsScreen( | |
| SecuritySettings( | ||
| state = state, | ||
| biometricsManager = biometricsManager, | ||
| hasBiometrics = hasBiometrics, | ||
| onBiometricToggle = remember(viewModel) { | ||
| { | ||
| viewModel.trySendAction( | ||
|
|
@@ -270,6 +281,7 @@ fun SettingsScreen( | |
| private fun SecuritySettings( | ||
| state: SettingsState, | ||
| biometricsManager: BiometricsManager = LocalBiometricsManager.current, | ||
| hasBiometrics: Boolean, | ||
| onBiometricToggle: (Boolean) -> Unit, | ||
| onScreenCaptureChange: (Boolean) -> Unit, | ||
| ) { | ||
|
|
@@ -282,7 +294,6 @@ private fun SecuritySettings( | |
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(8.dp)) | ||
| val hasBiometrics = biometricsManager.isBiometricsSupported | ||
| if (hasBiometrics) { | ||
| UnlockWithBiometricsRow( | ||
| modifier = Modifier | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,11 @@ class SettingsViewModel @Inject constructor( | |
| .map { SettingsAction.Internal.DefaultSaveOptionUpdated(it) } | ||
| .onEach(::sendAction) | ||
| .launchIn(viewModelScope) | ||
| settingsRepository | ||
| .isUnlockWithBiometricsEnabledFlow | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. โป๏ธ Finding 3: New Flow observation creates potential for state inconsistency DetailsThe ViewModel now observes
Concern: When Current implementation appears safe because:
However, this architectural pattern is more complex than necessary. Consider whether the Settings screen needs this flow observation or if it could rely on the repository's synchronous
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ญ Finding 3: Consider consolidating biometric state management DetailsThe ViewModel observes
While these represent different concerns (app setting vs device capability), consider whether the flow observation is necessary given that:
The current implementation works correctly, but you might simplify by relying on the synchronous Question: Are there other code paths that clear the biometric key besides
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ญ Finding 4: Consider the necessity of flow observation DetailsThe ViewModel observes Current architecture:
Consideration: This pattern is appropriate when multiple components can modify the same data. In this case:
The flow observation ensures Settings UI stays in sync with external changes, which is correct architecture. Verdict: This implementation is appropriate given that the biometric key can be cleared from multiple locations. The flow observation maintains consistency across the app.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ Finding 5: Appropriate flow observation pattern DetailsThe ViewModel observes Update paths:
Why this is correct:
This pattern follows the repository pattern correctly and ensures single source of truth from the data layer. |
||
| .map { SettingsAction.Internal.UnlockWithBiometricsUpdated(it) } | ||
| .onEach(::sendAction) | ||
| .launchIn(viewModelScope) | ||
| } | ||
|
|
||
| override fun handleAction(action: SettingsAction) { | ||
|
|
@@ -118,6 +123,20 @@ class SettingsViewModel @Inject constructor( | |
| } | ||
|
|
||
| is SettingsAction.Internal.DynamicColorsUpdated -> handleDynamicColorsUpdated(action) | ||
|
|
||
| is SettingsAction.Internal.UnlockWithBiometricsUpdated -> { | ||
| handleUnlockWithBiometricsUpdated(action) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun handleUnlockWithBiometricsUpdated( | ||
| action: SettingsAction.Internal.UnlockWithBiometricsUpdated, | ||
| ) { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| isUnlockWithBiometricsEnabled = action.isEnabled, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ |
||
| ) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -648,5 +667,12 @@ sealed class SettingsAction( | |
| data class DynamicColorsUpdated( | ||
| val isEnabled: Boolean, | ||
| ) : SettingsAction() | ||
|
|
||
| /** | ||
| * Indicates that the biometric state on disk was updated. | ||
| */ | ||
| data class UnlockWithBiometricsUpdated( | ||
| val isEnabled: Boolean, | ||
| ) : SettingsAction() | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,16 @@ | ||
| package com.bitwarden.authenticator.data.auth.datasource.disk.util | ||
|
|
||
| import com.bitwarden.authenticator.data.auth.datasource.disk.AuthDiskSource | ||
| import com.bitwarden.core.data.repository.util.bufferedMutableSharedFlow | ||
| import kotlinx.coroutines.flow.Flow | ||
| import kotlinx.coroutines.flow.onSubscription | ||
| import java.util.UUID | ||
|
|
||
| class FakeAuthDiskSource : AuthDiskSource { | ||
|
|
||
| private var lastActiveTimeMillis: Long? = null | ||
| private var userBiometricUnlockKey: String? = null | ||
| private val userBiometricUnlockKeyFlow = bufferedMutableSharedFlow<String?>() | ||
|
||
|
|
||
| override val uniqueAppId: String | ||
| get() = UUID.randomUUID().toString() | ||
|
|
@@ -19,7 +23,14 @@ class FakeAuthDiskSource : AuthDiskSource { | |
|
|
||
| override fun getUserBiometricUnlockKey(): String? = userBiometricUnlockKey | ||
|
|
||
| override fun getUserBiometricUnlockKeyFlow(): Flow<String?> = | ||
| userBiometricUnlockKeyFlow | ||
| .onSubscription { | ||
| emit(getUserBiometricUnlockKey()) | ||
| } | ||
|
|
||
| override fun storeUserBiometricUnlockKey(biometricsKey: String?) { | ||
| userBiometricUnlockKeyFlow.tryEmit(biometricsKey) | ||
| this@FakeAuthDiskSource.userBiometricUnlockKey = biometricsKey | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this have a
relayof 1?