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-16670] Force app to sync after 2FA notice #4525

Merged
merged 5 commits into from
Jan 8, 2025
+64 −1
Merged

[PM-16670] Force app to sync after 2FA notice #4525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
46c1995
[PM-16670] Force vault to sync by remove last sync date
andrebispo5 Jan 7, 2025
263fb9d
[PM-16670] Make sure the app syncs before launching the notice to the…
andrebispo5 Jan 7, 2025
609ca50
[PM-16670] Fix tests
andrebispo5 Jan 7, 2025
bd30ce6
[PM-16670] Update tests
andrebispo5 Jan 8, 2025
ab1f01c
Merge branch 'release/2024.12-rc13' into pm-16670/sync-2fa-notice
andrebispo5 Jan 8, 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
1 change: 1 addition & 0 deletions app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1350,6 +1350,7 @@ class AuthRepositoryImpl(
}

override fun checkUserNeedsNewDeviceTwoFactorNotice(): Boolean {
vaultRepository.syncIfNecessary()
return activeUserId?.let { userId ->
val temporaryFlag = featureFlagManager.getFeatureFlag(FlagKey.NewDeviceTemporaryDismiss)
val permanentFlag = featureFlagManager.getFeatureFlag(FlagKey.NewDevicePermanentDismiss)
Expand Down
6 changes: 6 additions & 0 deletions .../com/x8bit/bitwarden/ui/auth/feature/newdevicenotice/NewDeviceNoticeTwoFactorViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import com.x8bit.bitwarden.data.auth.repository.AuthRepository
import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager
import com.x8bit.bitwarden.data.platform.manager.model.FlagKey
import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository
import com.x8bit.bitwarden.data.platform.repository.SettingsRepository
import com.x8bit.bitwarden.data.platform.repository.util.baseWebVaultUrlOrDefault
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeTwoFactorAction.ChangeAccountEmailClick
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeTwoFactorAction.ContinueDialogClick
Expand All @@ -34,6 +35,7 @@ class NewDeviceNoticeTwoFactorViewModel @Inject constructor(
val authRepository: AuthRepository,
val environmentRepository: EnvironmentRepository,
val featureFlagManager: FeatureFlagManager,
val settingsRepository: SettingsRepository,
private val clock: Clock,
) : BaseViewModel<
NewDeviceNoticeTwoFactorState,
Expand Down Expand Up @@ -91,13 +93,17 @@ class NewDeviceNoticeTwoFactorViewModel @Inject constructor(
private fun handleContinueDialog() {
when (state.dialogState) {
is ChangeAccountEmailDialog -> {
// when the user leaves the app set sync date to null to force a sync on next unlock
settingsRepository.vaultLastSync = null
sendEvent(
NewDeviceNoticeTwoFactorEvent.NavigateToChangeAccountEmail(url = webAccountUrl),
)
updateDialogState(newState = null)
}

is TurnOnTwoFactorDialog -> {
// when the user leaves the app set sync date to null to force a sync on next unlock
settingsRepository.vaultLastSync = null
sendEvent(
NewDeviceNoticeTwoFactorEvent.NavigateToTurnOnTwoFactor(url = webTwoFactorUrl),
)
Expand Down
48 changes: 47 additions & 1 deletion app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ class AuthRepositoryTest {
private val vaultRepository: VaultRepository = mockk {
every { vaultUnlockDataStateFlow } returns mutableVaultUnlockDataStateFlow
every { deleteVaultData(any()) } just runs
every { syncIfNecessary() } just runs
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also be verifying that syncIfNecessary is being called in the correct spot.

}
private val fakeAuthDiskSource = FakeAuthDiskSource()
private val fakeEnvironmentRepository =
Expand Down Expand Up @@ -6542,6 +6543,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertTrue(shouldShowNewDeviceNotice)
}

Expand All @@ -6564,6 +6568,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertFalse(shouldShowNewDeviceNotice)
}

Expand All @@ -6585,6 +6592,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertTrue(shouldShowNewDeviceNotice)
}

Expand All @@ -6603,6 +6613,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertFalse(shouldShowNewDeviceNotice)
}

Expand All @@ -6623,6 +6636,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertFalse(shouldShowNewDeviceNotice)
}

Expand All @@ -6638,6 +6654,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertFalse(shouldShowNewDeviceNotice)
}

Expand All @@ -6663,6 +6682,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertFalse(shouldShowNewDeviceNotice)
}

Expand All @@ -6686,6 +6708,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertFalse(shouldShowNewDeviceNotice)
}

Expand All @@ -6709,6 +6734,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertTrue(shouldShowNewDeviceNotice)
}

Expand All @@ -6732,6 +6760,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertTrue(shouldShowNewDeviceNotice)
}

Expand All @@ -6755,6 +6786,9 @@ class AuthRepositoryTest {

val shouldShowNewDeviceNotice = repository.checkUserNeedsNewDeviceTwoFactorNotice()

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
assertFalse(shouldShowNewDeviceNotice)
}

Expand Down Expand Up @@ -6783,6 +6817,9 @@ class AuthRepositoryTest {
} returns false

assertFalse(repository.checkUserNeedsNewDeviceTwoFactorNotice())
verify(exactly = 2) {
vaultRepository.syncIfNecessary()
}
}

@Test
Expand All @@ -6796,6 +6833,9 @@ class AuthRepositoryTest {
fakeAuthDiskSource.userState = null

assertFalse(repository.checkUserNeedsNewDeviceTwoFactorNotice())
verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
}

@Test
Expand All @@ -6816,8 +6856,11 @@ class AuthRepositoryTest {
),
),
)

assertFalse(repository.checkUserNeedsNewDeviceTwoFactorNotice())

verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
}

@Test
Expand All @@ -6841,6 +6884,9 @@ class AuthRepositoryTest {
)

assertTrue(repository.checkUserNeedsNewDeviceTwoFactorNotice())
verify(exactly = 1) {
vaultRepository.syncIfNecessary()
}
}

companion object {
Expand Down
10 changes: 10 additions & 0 deletions .../x8bit/bitwarden/ui/auth/feature/newdevicenotice/NewDeviceNoticeTwoFactorViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import com.x8bit.bitwarden.data.auth.datasource.disk.model.NewDeviceNoticeState
import com.x8bit.bitwarden.data.auth.repository.AuthRepository
import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager
import com.x8bit.bitwarden.data.platform.manager.model.FlagKey
import com.x8bit.bitwarden.data.platform.repository.SettingsRepository
import com.x8bit.bitwarden.data.platform.repository.util.FakeEnvironmentRepository
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeTwoFactorDialogState.ChangeAccountEmailDialog
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeTwoFactorDialogState.TurnOnTwoFactorDialog
Expand All @@ -30,6 +31,8 @@ class NewDeviceNoticeTwoFactorViewModelTest : BaseViewModelTest() {
every { getFeatureFlag(FlagKey.NewDeviceTemporaryDismiss) } returns true
}

private val settingsRepository = mockk<SettingsRepository>(relaxed = true)

@Test
fun `initial state should be correct with NewDevicePermanentDismiss flag false`() = runTest {
val viewModel = createViewModel()
Expand Down Expand Up @@ -135,6 +138,9 @@ class NewDeviceNoticeTwoFactorViewModelTest : BaseViewModelTest() {
DEFAULT_STATE,
viewModel.stateFlow.value,
)
verify(exactly = 1) {
settingsRepository.vaultLastSync = null
}
}
}

Expand All @@ -156,6 +162,9 @@ class NewDeviceNoticeTwoFactorViewModelTest : BaseViewModelTest() {
DEFAULT_STATE,
viewModel.stateFlow.value,
)
verify(exactly = 1) {
settingsRepository.vaultLastSync = null
}
}
}

Expand All @@ -177,6 +186,7 @@ class NewDeviceNoticeTwoFactorViewModelTest : BaseViewModelTest() {
authRepository = authRepository,
environmentRepository = environmentRepository,
featureFlagManager = featureFlagManager,
settingsRepository = settingsRepository,
clock = FIXED_CLOCK,
)
}
Expand Down