Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e55b8b7
[PM-27150] React to device changes on device screen unlock method.
andrebispo5 Oct 30, 2025
3533af5
[PM-27150] Add BiometricUnlockKey flow to AuthDiskSouce
andrebispo5 Nov 6, 2025
46c1cb3
[PM-27150] Add isUnlockWithBiometricsEnabledFlow to SettingsRepository
andrebispo5 Nov 6, 2025
436bcc1
[PM-27150] Remove saved biometric key if device no longer supports it.
andrebispo5 Nov 6, 2025
793e798
[PM-27150] Bypass UnlockScreen if device no longer supports it.
andrebispo5 Nov 6, 2025
3ce913d
[PM-27150] Update AppLock row visibility and toggle based on device bโ€ฆ
andrebispo5 Nov 6, 2025
e7ac063
[PM-27150] Add PR suggestions.
andrebispo5 Nov 7, 2025
85ff4e7
[PM-27150] Fix typo
andrebispo5 Nov 7, 2025
de0947a
[PM-27150] Add replay 1
andrebispo5 Nov 7, 2025
12c28c2
Merge branch 'main' into pm-27150/authenticator-unlock-enabled-bug
andrebispo5 Nov 7, 2025
3afe1b3
[PM-27150] Fix pre existing typo
andrebispo5 Nov 12, 2025
790ccd0
[PM-27150] Make isUnlockWithBiometricsEnabledFlow state flow
andrebispo5 Nov 12, 2025
385caae
[PM-27150] Extract BiometricChanges composable to be reused.
andrebispo5 Nov 12, 2025
9e28ed5
[PM-27150] Change BiometricSupportChanged to be external.
andrebispo5 Nov 12, 2025
8492014
[PM-27150] Track device biometric changes in Settings UI and ViewModel.
andrebispo5 Nov 12, 2025
4556e1a
[PM-27150] Add Kdoc
andrebispo5 Nov 12, 2025
9180c52
[PM-27150] Pass callback value
andrebispo5 Nov 12, 2025
2de0197
[PM-27150] Add missing test classes for RootNav in Authenticator.
andrebispo5 Nov 13, 2025
df27f32
[PM-27150] Init biometric support as true
andrebispo5 Nov 13, 2025
25f028c
[PM-27150] Move unlock screen navigation to RootNavViewModel
andrebispo5 Nov 13, 2025
d09486f
[PM-27150] Add launch effect to BiometricChanges composable
andrebispo5 Nov 13, 2025
3018a6c
[PM-27150] Add remember viewmodel to avoid recreation on recomposition.
andrebispo5 Nov 13, 2025
3c01f60
[PM-27150] Change assert to assertTrue or assertFalse
andrebispo5 Nov 13, 2025
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 @@ -711,6 +711,25 @@ class AuthDiskSourceTest {
assertEquals(biometricsKey, actual)
}

@Test
fun `getUserBiometricUnlockKeyFlow should react to changes in getUserBiometricUnlockKey`() =
runTest {
val mockUserId = "mockUserId"
val biometricsKey = "1234"
authDiskSource.getUserBiometicUnlockKeyFlow(userId = mockUserId).test {
// The initial values of the Flow and the property are in sync
assertNull(authDiskSource.getUserBiometricUnlockKey(userId = mockUserId))
assertNull(awaitItem())

// Updating the disk source updates shared preferences
authDiskSource.storeUserBiometricUnlockKey(
userId = mockUserId,
biometricsKey = biometricsKey,
)
assertEquals(biometricsKey, awaitItem())
}
}

@Test
fun `storeUserBiometricInitVector for non-null values should update SharedPreferences`() {
val biometricsInitVectorBaseKey = "bwSecureStorage:biometricInitializationVector"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.bitwarden.authenticator.data.auth.datasource.disk

import com.bitwarden.network.provider.AppIdProvider
import kotlinx.coroutines.flow.Flow

/**
* Primary access point for disk information.
Expand Down Expand Up @@ -28,6 +29,11 @@ interface AuthDiskSource : AppIdProvider {
*/
fun getUserBiometricUnlockKey(): String?

/**
* Tracks the biometrics key.
*/
fun getUserBiometricUnlockKeyFlow(): Flow<String?>

/**
* Stores the biometrics key.
*/
Expand Down
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"
Expand All @@ -20,6 +23,8 @@ class AuthDiskSourceImpl(
sharedPreferences = sharedPreferences,
),
AuthDiskSource {
private val mutableUserBiometricUnlockKeyFlow =
bufferedMutableSharedFlow<String?>()
Copy link
Collaborator

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 relay of 1?


override val uniqueAppId: String
get() = getString(key = UNIQUE_APP_ID_KEY) ?: generateAndStoreUniqueAppId()
Expand All @@ -39,13 +44,17 @@ class AuthDiskSourceImpl(
override fun getUserBiometricUnlockKey(): String? =
getEncryptedString(key = BIOMETRICS_UNLOCK_KEY)

override fun getUserBiometricUnlockKeyFlow(): Flow<String?> = mutableUserBiometricUnlockKeyFlow
Copy link
Collaborator

@david-livefront david-livefront Nov 6, 2025

Choose a reason for hiding this comment

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

Can we make this a val with a get() here?

.onSubscription { emit(getUserBiometricUnlockKey()) }

override fun storeUserBiometricUnlockKey(
biometricsKey: String?,
) {
putEncryptedString(
key = BIOMETRICS_UNLOCK_KEY,
value = biometricsKey,
)
mutableUserBiometricUnlockKeyFlow.tryEmit(biometricsKey)
}

override var authenticatorBridgeSymmetricSyncKey: ByteArray?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ interface SettingsRepository {
*/
val isUnlockWithBiometricsEnabled: Boolean

/**
* Tracks whether or not biometric unlocking is enabled for the current user.
*/
val isUnlockWithBiometricsEnabledFlow: Flow<Boolean>

/**
* Tracks changes to the expiration alert threshold.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you run the foirmatter here, this looks overly indented. You can also move the authDiskSource up a line


override val appThemeStateFlow: StateFlow<AppTheme>
get() = settingsDiskSource
.appThemeFlow
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -53,6 +54,12 @@ fun UnlockScreen(
}
}

LaunchedEffect(biometricsManager.isBiometricsSupported) {
Copy link
Collaborator

@david-livefront david-livefront Nov 12, 2025

Choose a reason for hiding this comment

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

Hmmm, isn't the UnlockScreen a top-level screen? I believe this should be state driven in the RootNavViewModel, right?

if (!biometricsManager.isBiometricsSupported) {
onUnlocked()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

โŒ Navigation bypass violates MVVM architecture

Details

This LaunchedEffect directly calls onUnlocked() when biometrics are unsupported, bypassing the ViewModel and creating screen-level navigation logic.

Issue: Per docs/ARCHITECTURE.md:298:

Any state that will eventually need to be used by the VM should be hoisted to the VM... These state changes are communicated via the "actions" that the Compose layer sends.

Navigation decisions should be state-driven from RootNavViewModel, not made locally in screens.

Why this is problematic:

  1. Inconsistent navigation logic: Some navigation happens via ViewModel state, some happens directly in the screen
  2. Harder to test: This navigation logic is not unit-testable through the ViewModel
  3. Violates separation of concerns: The screen should only render state, not make navigation decisions

Recommendation: Remove this LaunchedEffect entirely. The correct flow already exists:

  1. BiometricChanges in RootNavScreen detects biometric support loss on resume
  2. Sends BiometricSupportChanged(false) to RootNavViewModel
  3. RootNavViewModel.handleBiometricSupportChanged clears the biometric key
  4. State flow updates cause RootNavViewModel.handleHasSeenWelcomeTutorialChange to navigate to Unlocked state (lines 79-85 already check isUnlockWithBiometricsEnabled)

This ensures all navigation logic is centralized, state-driven, and testable.


when (val dialog = state.dialog) {
is UnlockState.Dialog.Error -> BitwardenBasicDialog(
title = stringResource(id = BitwardenString.an_error_has_occurred),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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,
) {
Expand All @@ -63,6 +68,13 @@ fun RootNavScreen(
.launchIn(this)
}

BiometricChanges(
Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿ‘ Finding 5: Well-structured biometric monitoring implementation

Details

The BiometricChanges composable is properly integrated at the root navigation level:

โœ“ Separated into a reusable composable function with comprehensive KDoc
โœ“ Uses LifecycleEventEffect to monitor app lifecycle
โœ“ Clear callback interface that sends actions to ViewModel
โœ“ Checks biometric support on ON_RESUME to detect changes

Architectural correctness: Placing this at the RootNav level is appropriate since:

  1. Biometric state affects navigation decisions (whether to show unlock screen)
  2. Centralized monitoring ensures consistent behavior across the app
  3. Both RootNavViewModel and SettingsViewModel receive updates about biometric support changes

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

Choose a reason for hiding this comment

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

This is an internal event but you are sending it externally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I was moving things around and didn't noticed. ty

},
Copy link
Contributor

Choose a reason for hiding this comment

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

โš ๏ธ Finding 1: Callback creates unnecessary lambda allocation on each recomposition

Details

Per david-livefront's comment, the callback should be wrapped in remember(viewModel) for stability:

onBiometricSupportChange = remember(viewModel) {
    { viewModel.trySendAction(RootNavAction.BiometricSupportChanged(it)) }
},

This prevents unnecessary recompositions of BiometricChanges and follows the pattern established elsewhere in this file (lines 166-172).

)

NavHost(
navController = navController,
startDestination = SplashRoute,
Expand Down Expand Up @@ -187,3 +199,18 @@ private fun AnimatedContentTransitionScope<NavBackStackEntry>.toExitTransition()
else -> RootTransitionProviders.Exit.fadeOut
}
}

@Composable
private fun BiometricChanges(
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Finding 5: Well-structured biometric monitoring composable

Details

The BiometricChanges composable follows good practices:

โœ“ Separated into its own composable function
โœ“ Uses LifecycleEventEffect to monitor lifecycle events
โœ“ Clear callback interface with onBiometricSupportChange
โœ“ Properly checks biometric support on ON_RESUME

This pattern is clean and reusable. The only consideration is whether this logic belongs at the RootNav level or could be more appropriately handled in individual screens that need biometric support detection.

Current approach is acceptable given that biometric state affects navigation decisions at the root level (determining whether to show the unlock screen).

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
Expand Up @@ -60,6 +60,10 @@ class RootNavViewModel @Inject constructor(
RootNavAction.Internal.AppUnlocked -> {
handleAppUnlocked()
}

is RootNavAction.Internal.BiometricSupportChanged -> {
handleBiometricSupportChanged(action.isBiometricsSupported)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be passing the entire action through

}
}
}

Expand Down Expand Up @@ -99,6 +103,12 @@ class RootNavViewModel @Inject constructor(
it.copy(navState = RootNavState.NavState.Unlocked)
}
}

private fun handleBiometricSupportChanged(isBiometricsSupported: Boolean) {
if (!isBiometricsSupported) {
settingsRepository.clearBiometricsKey()
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Consider extracting navigation state check to improve readability

Details

The 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.

}
}
}

/**
Expand Down Expand Up @@ -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.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -88,6 +90,14 @@ fun SettingsScreen(
) {
val state by viewModel.stateFlow.collectAsStateWithLifecycle()
val scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior()
var hasBiometrics by remember { mutableStateOf(biometricsManager.isBiometricsSupported) }
Copy link
Contributor

Choose a reason for hiding this comment

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

โš ๏ธ Finding 4: Biometric state management bypasses ViewModel architecture

Details

The screen maintains local hasBiometrics state that's updated on ON_RESUME, but this violates MVVM principles:

var hasBiometrics by remember { mutableStateOf(biometricsManager.isBiometricsSupported) }

LifecycleEventEffect { _, event ->
    if (event == Lifecycle.Event.ON_RESUME) {
        hasBiometrics = biometricsManager.isBiometricsSupported
    }
}

Issue: This UI-layer state should be hoisted to the ViewModel per architecture guidelines:

Any state that will eventually need to be used by the VM should be hoisted to the VM... These state changes are communicated via the "actions" that the Compose layer sends. (ARCHITECTURE.md:298)

Recommended approach:

  1. Add an action to notify the ViewModel of lifecycle events:
sealed class SettingsAction {
    data object OnResume : SettingsAction()
    // ... existing actions
}
  1. Handle in ViewModel:
private fun handleOnResume() {
    mutableStateFlow.update {
        it.copy(hasBiometricsSupport = biometricsManager.isBiometricsSupported)
    }
    // Clear key if biometrics are no longer supported
    if (!biometricsManager.isBiometricsSupported) {
        settingsRepository.clearBiometricsKey()
    }
}
  1. Use state from ViewModel in the screen instead of local state

This ensures:

  • Single source of truth in the ViewModel
  • Testable logic
  • Consistent with architecture patterns

Copy link
Collaborator

Choose a reason for hiding this comment

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

What Claude said ๐Ÿ˜„

Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not need the remembered value here, just call it directly.

The ViewModel will be updated when the biometric support is cleared. That will cause a recomposition and everything should just update.

Copy link
Contributor

Choose a reason for hiding this comment

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

โš ๏ธ Finding 2: UI-layer biometric state bypasses MVVM architecture

Details

The hasBiometrics state maintained in the UI layer violates MVVM principles per docs/ARCHITECTURE.md:298:

Any state that will eventually need to be used by the VM should be hoisted to the VM... These state changes are communicated via the "actions" that the Compose layer sends.

Current approach:

  • UI maintains local state
  • Updates on lifecycle events
  • No single source of truth

Recommended approach:
Since the ViewModel already observes isUnlockWithBiometricsEnabledFlow (which updates when the key is cleared), you can rely on this flow to trigger recomposition. Remove the remembered hasBiometrics variable and pass biometricsManager.isBiometricsSupported directly to SecuritySettings:

SecuritySettings(
    state = state,
    biometricsManager = biometricsManager,
    hasBiometrics = biometricsManager.isBiometricsSupported,
    // ...
)

When RootNavViewModel clears the biometric key (when biometrics become unsupported), the flow will emit, SettingsViewModel will update its state, triggering recomposition, and the UI will re-evaluate 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) {
Expand Down Expand Up @@ -151,6 +161,7 @@ fun SettingsScreen(
SecuritySettings(
state = state,
biometricsManager = biometricsManager,
hasBiometrics = hasBiometrics,
onBiometricToggle = remember(viewModel) {
{
viewModel.trySendAction(
Expand Down Expand Up @@ -270,6 +281,7 @@ fun SettingsScreen(
private fun SecuritySettings(
state: SettingsState,
biometricsManager: BiometricsManager = LocalBiometricsManager.current,
hasBiometrics: Boolean,
onBiometricToggle: (Boolean) -> Unit,
onScreenCaptureChange: (Boolean) -> Unit,
) {
Expand All @@ -282,7 +294,6 @@ private fun SecuritySettings(
)

Spacer(modifier = Modifier.height(8.dp))
val hasBiometrics = biometricsManager.isBiometricsSupported
if (hasBiometrics) {
UnlockWithBiometricsRow(
modifier = Modifier
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ class SettingsViewModel @Inject constructor(
.map { SettingsAction.Internal.DefaultSaveOptionUpdated(it) }
.onEach(::sendAction)
.launchIn(viewModelScope)
settingsRepository
.isUnlockWithBiometricsEnabledFlow
Copy link
Contributor

Choose a reason for hiding this comment

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

โ™ป๏ธ Finding 3: New Flow observation creates potential for state inconsistency

Details

The ViewModel now observes isUnlockWithBiometricsEnabledFlow to update state, but this creates a dual path for state updates:

  1. Direct path: User toggles biometrics โ†’ handleBiometricsSetupClick โ†’ updates state directly
  2. Flow path: Disk source changes โ†’ Flow emits โ†’ handleUnlockWithBiometricsUpdated โ†’ updates state

Concern: When RootNavViewModel clears the biometric key (when biometrics become unsupported), the flow will emit and update SettingsViewModel's state. However, this happens asynchronously and could race with user interactions.

Current implementation appears safe because:

  • The biometric toggle in settings already checks biometricsManager.isBiometricsSupported before showing (SettingsScreen.kt:302)
  • The repository clears the key immediately, and the flow propagates the change
  • State updates are synchronized through the action queue

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 isUnlockWithBiometricsEnabled property.

Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿ’ญ Finding 3: Consider consolidating biometric state management

Details

The ViewModel observes isUnlockWithBiometricsEnabledFlow and also handles BiometricSupportChanged action separately. This creates two distinct state update paths:

  1. Flow observation: Updates isUnlockWithBiometricsEnabled when the biometric key changes (cleared by RootNavViewModel)
  2. Action handling: Updates hasBiometricsSupport based on device capability

While these represent different concerns (app setting vs device capability), consider whether the flow observation is necessary given that:

  • RootNavViewModel clears the key when biometrics become unsupported
  • The Settings screen checks biometricsManager.isBiometricsSupported before showing the toggle (line 300)

The current implementation works correctly, but you might simplify by relying on the synchronous settingsRepository.isUnlockWithBiometricsEnabled property in state initialization rather than observing the flow. The flow observation would still be useful if the key could be cleared from other locations in the app.

Question: Are there other code paths that clear the biometric key besides RootNavViewModel.handleBiometricSupportChanged?

Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿ’ญ Finding 4: Consider the necessity of flow observation

Details

The ViewModel observes isUnlockWithBiometricsEnabledFlow to reactively update state when the biometric key is cleared externally (by RootNavViewModel). This creates a flow-based update path in addition to the direct state updates.

Current architecture:

  • Direct path: User toggles โ†’ handleBiometricsSetupClick โ†’ updates state
  • Flow path: External clear โ†’ Disk source emits โ†’ Flow observer โ†’ updates state

Consideration: This pattern is appropriate when multiple components can modify the same data. In this case:

  • RootNavViewModel can clear the key when biometrics become unsupported
  • SettingsViewModel can enable/disable the key via user action

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿ‘ Finding 5: Appropriate flow observation pattern

Details

The ViewModel observes isUnlockWithBiometricsEnabledFlow to reactively update state when the biometric key is cleared externally. This creates a proper reactive architecture:

Update paths:

  • Direct path: User toggles โ†’ handleBiometricsSetupClick โ†’ updates state
  • Flow path: External clear (from RootNavViewModel) โ†’ Disk source emits โ†’ Flow observer โ†’ updates state

Why this is correct:

  • The biometric key can be cleared from multiple locations (RootNavViewModel when biometrics become unsupported, SettingsViewModel when user disables)
  • Flow observation ensures Settings UI stays in sync with external changes
  • Maintains consistency across the app without tight coupling

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) {
Expand Down Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

๐Ÿ‘

)
}
}

Expand Down Expand Up @@ -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?>()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here, shouldn't this have a replay of 1?


override val uniqueAppId: String
get() = UUID.randomUUID().toString()
Expand All @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,19 @@ class SettingsRepositoryTest {
settingsRepository.previouslySyncedBitwardenAccountIds = setOf("1", "2", "3")
verify { settingsDiskSource.previouslySyncedBitwardenAccountIds = setOf("1", "2", "3") }
}

@Test
fun `isUnlockWithBiometricsEnabledFlow should react to changes in AuthDiskSource`() = runTest {
settingsRepository.isUnlockWithBiometricsEnabledFlow.test {
assertFalse(awaitItem())
authDiskSource.storeUserBiometricUnlockKey(
biometricsKey = "biometricsKey",
)
assertTrue(awaitItem())
authDiskSource.storeUserBiometricUnlockKey(
biometricsKey = null,
)
assertFalse(awaitItem())
}
}
}
Loading