From eb2ba8e5984486417abd27a1fb96acf829446d91 Mon Sep 17 00:00:00 2001 From: David Perez Date: Thu, 22 Aug 2024 13:48:26 -0500 Subject: [PATCH] PM-11264: Ensure user has valid timeout action after migrating to Key Connector (#3807) --- .../auth/repository/AuthRepositoryImpl.kt | 9 +- .../util/UserStateJsonExtensions.kt | 26 ++++ .../repository/SettingsRepositoryImpl.kt | 23 ++-- .../auth/repository/AuthRepositoryTest.kt | 17 ++- .../util/UserStateJsonExtensionsTest.kt | 111 ++++++++++++++++++ .../repository/SettingsRepositoryTest.kt | 44 ++++++- 6 files changed, 219 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt index aad19ca46..2d6a5bb26 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt @@ -73,6 +73,7 @@ import com.x8bit.bitwarden.data.auth.repository.util.SsoCallbackResult import com.x8bit.bitwarden.data.auth.repository.util.WebAuthResult import com.x8bit.bitwarden.data.auth.repository.util.activeUserIdChangesFlow import com.x8bit.bitwarden.data.auth.repository.util.policyInformation +import com.x8bit.bitwarden.data.auth.repository.util.toRemovedPasswordUserStateJson import com.x8bit.bitwarden.data.auth.repository.util.toSdkParams import com.x8bit.bitwarden.data.auth.repository.util.toUserState import com.x8bit.bitwarden.data.auth.repository.util.toUserStateJsonWithPassword @@ -873,7 +874,13 @@ class AuthRepositoryImpl( masterPassword = masterPassword, kdf = profile.toSdkParams(), ) - .onSuccess { vaultRepository.sync() } + .onSuccess { + authDiskSource.userState = authDiskSource + .userState + ?.toRemovedPasswordUserStateJson(userId = userId) + vaultRepository.sync() + settingsRepository.setDefaultsIfNecessary(userId = userId) + } .fold( onFailure = { RemovePasswordResult.Error }, onSuccess = { RemovePasswordResult.Success }, diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensions.kt index a57a77cd3..861273824 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensions.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensions.kt @@ -13,6 +13,32 @@ import com.x8bit.bitwarden.data.vault.repository.model.VaultUnlockData import com.x8bit.bitwarden.data.vault.repository.util.statusFor import com.x8bit.bitwarden.ui.platform.base.util.toHexColorRepresentation +/** + * Updates the given [UserStateJson] with the data to indicate that the password has been removed. + * The original will be returned if the [userId] does not match any accounts in the [UserStateJson]. + */ +fun UserStateJson.toRemovedPasswordUserStateJson( + userId: String, +): UserStateJson { + val account = this.accounts[userId] ?: return this + val profile = account.profile + val updatedUserDecryptionOptions = profile + .userDecryptionOptions + ?.copy(hasMasterPassword = false) + ?: UserDecryptionOptionsJson( + hasMasterPassword = false, + trustedDeviceUserDecryptionOptions = null, + keyConnectorUserDecryptionOptions = null, + ) + val updatedProfile = profile.copy(userDecryptionOptions = updatedUserDecryptionOptions) + val updatedAccount = account.copy(profile = updatedProfile) + return this.copy( + accounts = accounts + .toMutableMap() + .apply { replace(userId, updatedAccount) }, + ) +} + /** * Updates the given [UserStateJson] with the data from the [syncResponse] to return a new * [UserStateJson]. The original will be returned if the sync response does not match any accounts diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt index d25b9dfa5..363f65fe8 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt @@ -325,14 +325,23 @@ class SettingsRepositoryImpl( override fun setDefaultsIfNecessary(userId: String) { // Set Vault Settings defaults - if (!isVaultTimeoutActionSet(userId = userId)) { + val hasMasterPassword = authDiskSource + .userState + ?.activeAccount + ?.profile + ?.userDecryptionOptions + ?.hasMasterPassword != false + val timeoutAction = settingsDiskSource.getVaultTimeoutAction(userId = userId) + val hasPin = authDiskSource.getPinProtectedUserKey(userId = userId) != null + val hasBiometrics = authDiskSource.getUserBiometricUnlockKey(userId = userId) != null + // The timeout action cannot be "lock" if you do not have master password, pin, or + // biometrics unlock enabled. + val hasInvalidTimeoutAction = timeoutAction == VaultTimeoutAction.LOCK && + !hasPin && + !hasBiometrics && + !hasMasterPassword + if (!isVaultTimeoutActionSet(userId = userId) || hasInvalidTimeoutAction) { storeVaultTimeout(userId, VaultTimeout.FifteenMinutes) - val hasMasterPassword = authDiskSource - .userState - ?.activeAccount - ?.profile - ?.userDecryptionOptions - ?.hasMasterPassword != false storeVaultTimeoutAction( userId = userId, vaultTimeoutAction = if (!hasMasterPassword) { diff --git a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt index 5f6ef158e..5eab95640 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt @@ -87,6 +87,7 @@ import com.x8bit.bitwarden.data.auth.repository.util.DuoCallbackTokenResult import com.x8bit.bitwarden.data.auth.repository.util.SsoCallbackResult import com.x8bit.bitwarden.data.auth.repository.util.WebAuthResult import com.x8bit.bitwarden.data.auth.repository.util.toOrganizations +import com.x8bit.bitwarden.data.auth.repository.util.toRemovedPasswordUserStateJson import com.x8bit.bitwarden.data.auth.repository.util.toSdkParams import com.x8bit.bitwarden.data.auth.repository.util.toUserState import com.x8bit.bitwarden.data.auth.util.YubiKeyResult @@ -255,12 +256,18 @@ class AuthRepositoryTest { @BeforeEach fun beforeEach() { - mockkStatic(GetTokenResponseJson.Success::toUserState) + mockkStatic( + GetTokenResponseJson.Success::toUserState, + UserStateJson::toRemovedPasswordUserStateJson, + ) } @AfterEach fun tearDown() { - unmockkStatic(GetTokenResponseJson.Success::toUserState) + unmockkStatic( + GetTokenResponseJson.Success::toUserState, + UserStateJson::toRemovedPasswordUserStateJson, + ) } @Test @@ -4341,13 +4348,19 @@ class AuthRepositoryTest { kdf = PROFILE_1.toSdkParams(), ) } returns Unit.asSuccess() + every { + SINGLE_USER_STATE_1.toRemovedPasswordUserStateJson(userId = USER_ID_1) + } returns SINGLE_USER_STATE_1 every { vaultRepository.sync() } just runs + every { settingsRepository.setDefaultsIfNecessary(userId = USER_ID_1) } just runs val result = repository.removePassword(masterPassword = PASSWORD) assertEquals(RemovePasswordResult.Success, result) verify(exactly = 1) { + SINGLE_USER_STATE_1.toRemovedPasswordUserStateJson(userId = USER_ID_1) vaultRepository.sync() + settingsRepository.setDefaultsIfNecessary(userId = USER_ID_1) } } diff --git a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensionsTest.kt index 81446042b..d79368fb9 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensionsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensionsTest.kt @@ -24,6 +24,117 @@ import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test class UserStateJsonExtensionsTest { + @Test + fun `toUpdatedUserStateJn should do nothing for a non-matching account`() { + val originalUserState = UserStateJson( + activeUserId = "activeUserId", + accounts = mapOf("activeUserId" to mockk()), + ) + assertEquals( + originalUserState, + originalUserState.toRemovedPasswordUserStateJson(userId = "nonActiveUserId"), + ) + } + + @Suppress("MaxLineLength") + @Test + fun `toUpdatedUserStateJn should create user decryption options without a password if not present`() { + val originalProfile = AccountJson.Profile( + userId = "activeUserId", + email = "email", + isEmailVerified = true, + name = "name", + stamp = null, + organizationId = null, + avatarColorHex = null, + hasPremium = true, + forcePasswordResetReason = null, + kdfType = KdfTypeJson.ARGON2_ID, + kdfIterations = 600000, + kdfMemory = 16, + kdfParallelism = 4, + userDecryptionOptions = null, + ) + val originalAccount = AccountJson( + profile = originalProfile, + tokens = null, + settings = AccountJson.Settings(environmentUrlData = null), + ) + val originalUserState = UserStateJson( + activeUserId = "activeUserId", + accounts = mapOf("activeUserId" to originalAccount), + ) + + assertEquals( + UserStateJson( + activeUserId = "activeUserId", + accounts = mapOf( + "activeUserId" to originalAccount.copy( + profile = originalProfile.copy( + userDecryptionOptions = UserDecryptionOptionsJson( + hasMasterPassword = false, + trustedDeviceUserDecryptionOptions = null, + keyConnectorUserDecryptionOptions = null, + ), + ), + ), + ), + ), + originalUserState.toRemovedPasswordUserStateJson(userId = "activeUserId"), + ) + } + + @Test + fun `toUpdatedUserStateJn should update user decryption options to not have a password`() { + val originalProfile = AccountJson.Profile( + userId = "activeUserId", + email = "email", + isEmailVerified = true, + name = "name", + stamp = null, + organizationId = null, + avatarColorHex = null, + hasPremium = true, + forcePasswordResetReason = null, + kdfType = KdfTypeJson.ARGON2_ID, + kdfIterations = 600000, + kdfMemory = 16, + kdfParallelism = 4, + userDecryptionOptions = UserDecryptionOptionsJson( + hasMasterPassword = true, + trustedDeviceUserDecryptionOptions = null, + keyConnectorUserDecryptionOptions = null, + ), + ) + val originalAccount = AccountJson( + profile = originalProfile, + tokens = null, + settings = AccountJson.Settings(environmentUrlData = null), + ) + val originalUserState = UserStateJson( + activeUserId = "activeUserId", + accounts = mapOf("activeUserId" to originalAccount), + ) + + assertEquals( + UserStateJson( + activeUserId = "activeUserId", + accounts = mapOf( + "activeUserId" to originalAccount.copy( + profile = originalProfile.copy( + userDecryptionOptions = UserDecryptionOptionsJson( + hasMasterPassword = false, + trustedDeviceUserDecryptionOptions = null, + keyConnectorUserDecryptionOptions = null, + ), + ), + ), + ), + ), + originalUserState.toRemovedPasswordUserStateJson(userId = "activeUserId"), + ) + } + @Test fun `toUpdatedUserStateJson should do nothing for a non-matching account`() { val originalUserState = UserStateJson( diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt index 03c052c67..1fa18d281 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt @@ -176,7 +176,11 @@ class SettingsRepositoryTest { ) // Updating the Vault settings values and calling setDefaultsIfNecessary again has no - // effect on the currently stored values. + // effect on the currently stored values since we have a way to unlock the vault. + fakeAuthDiskSource.storePinProtectedUserKey( + userId = USER_ID, + pinProtectedUserKey = "pinProtectedKey", + ) fakeSettingsDiskSource.apply { storeVaultTimeoutInMinutes( userId = USER_ID, @@ -195,6 +199,44 @@ class SettingsRepositoryTest { ) } + @Suppress("MaxLineLength") + @Test + fun `setDefaultsIfNecessary should reset default values to LOGOUT for the given user without a password if necessary`() { + fakeAuthDiskSource.userState = MOCK_USER_STATE + assertNull(fakeSettingsDiskSource.getVaultTimeoutInMinutes(userId = USER_ID)) + assertNull(fakeSettingsDiskSource.getVaultTimeoutAction(userId = USER_ID)) + + settingsRepository.setDefaultsIfNecessary(userId = USER_ID) + + // Calling once sets values + assertEquals(15, fakeSettingsDiskSource.getVaultTimeoutInMinutes(userId = USER_ID)) + assertEquals( + VaultTimeoutAction.LOGOUT, + fakeSettingsDiskSource.getVaultTimeoutAction(userId = USER_ID), + ) + + // Updating the Vault settings values and calling setDefaultsIfNecessary again has no + // effect on the currently stored values. + fakeSettingsDiskSource.apply { + storeVaultTimeoutInMinutes( + userId = USER_ID, + vaultTimeoutInMinutes = 240, + ) + storeVaultTimeoutAction( + userId = USER_ID, + vaultTimeoutAction = VaultTimeoutAction.LOCK, + ) + } + // This will reset the setting because the user does not have a method to unlock the vault + // so you cannot use the "lock" timeout action, it must be "logout". + settingsRepository.setDefaultsIfNecessary(userId = USER_ID) + assertEquals(15, fakeSettingsDiskSource.getVaultTimeoutInMinutes(userId = USER_ID)) + assertEquals( + VaultTimeoutAction.LOGOUT, + fakeSettingsDiskSource.getVaultTimeoutAction(userId = USER_ID), + ) + } + @Test fun `appLanguage should pull from and update SettingsDiskSource`() { assertEquals(