Skip to content

PM-40295: bug: Add MP Policy checks on vault unlock #7165

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

Open
david-livefront wants to merge 1 commit into
base: main
Choose a base branch
from
Open

PM-40295: bug: Add MP Policy checks on vault unlock #7165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/UserLogoutManagerImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import com.x8bit.bitwarden.data.auth.repository.model.LogoutReason
import com.x8bit.bitwarden.data.platform.datasource.disk.PushDiskSource
import com.x8bit.bitwarden.data.platform.datasource.disk.SettingsDiskSource
import com.x8bit.bitwarden.data.platform.manager.CredentialExchangeRegistryManager
import com.x8bit.bitwarden.data.platform.manager.policy.PasswordPolicyManager
import com.x8bit.bitwarden.data.tools.generator.datasource.disk.GeneratorDiskSource
import com.x8bit.bitwarden.data.tools.generator.datasource.disk.PasswordHistoryDiskSource
import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource
Expand All @@ -36,6 +37,7 @@ class UserLogoutManagerImpl(
private val vaultDiskSource: VaultDiskSource,
private val vaultSdkSource: VaultSdkSource,
private val credentialExchangeRegistryManager: CredentialExchangeRegistryManager,
private val passwordPolicyManager: PasswordPolicyManager,
dispatcherManager: DispatcherManager,
) : UserLogoutManager {
private val unconfinedScope = CoroutineScope(dispatcherManager.unconfined)
Expand Down Expand Up @@ -116,6 +118,7 @@ class UserLogoutManagerImpl(
}

private fun clearData(userId: String) {
passwordPolicyManager.removePasswordToCheck(userId = userId)
vaultSdkSource.clearCrypto(userId = userId)
authDiskSource.clearData(userId = userId)
generatorDiskSource.clearData(userId = userId)
Expand Down
3 changes: 3 additions & 0 deletions app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/di/AuthManagerModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import com.x8bit.bitwarden.data.platform.datasource.disk.SettingsDiskSource
import com.x8bit.bitwarden.data.platform.manager.CredentialExchangeRegistryManager
import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager
import com.x8bit.bitwarden.data.platform.manager.PushManager
import com.x8bit.bitwarden.data.platform.manager.policy.PasswordPolicyManager
import com.x8bit.bitwarden.data.tools.generator.datasource.disk.GeneratorDiskSource
import com.x8bit.bitwarden.data.tools.generator.datasource.disk.PasswordHistoryDiskSource
import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource
Expand Down Expand Up @@ -124,6 +125,7 @@ object AuthManagerModule {
vaultSdkSource: VaultSdkSource,
dispatcherManager: DispatcherManager,
credentialExchangeRegistryManager: CredentialExchangeRegistryManager,
passwordPolicyManager: PasswordPolicyManager,
): UserLogoutManager =
UserLogoutManagerImpl(
authDiskSource = authDiskSource,
Expand All @@ -136,6 +138,7 @@ object AuthManagerModule {
vaultSdkSource = vaultSdkSource,
dispatcherManager = dispatcherManager,
credentialExchangeRegistryManager = credentialExchangeRegistryManager,
passwordPolicyManager = passwordPolicyManager,
)

@Provides
Expand Down
28 changes: 2 additions & 26 deletions app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.x8bit.bitwarden.data.auth.repository

import com.bitwarden.network.model.GetTokenResponseJson
import com.bitwarden.network.model.TwoFactorDataModel
import com.x8bit.bitwarden.data.auth.datasource.disk.model.ForcePasswordResetReason
import com.x8bit.bitwarden.data.auth.datasource.disk.model.OnboardingStatus
import com.x8bit.bitwarden.data.auth.manager.AuthRequestManager
import com.x8bit.bitwarden.data.auth.manager.KdfManager
Expand All @@ -19,8 +18,6 @@ import com.x8bit.bitwarden.data.auth.repository.model.LogoutReason
import com.x8bit.bitwarden.data.auth.repository.model.NewSsoUserResult
import com.x8bit.bitwarden.data.auth.repository.model.Organization
import com.x8bit.bitwarden.data.auth.repository.model.PasswordHintResult
import com.x8bit.bitwarden.data.auth.repository.model.PasswordStrengthResult
import com.x8bit.bitwarden.data.auth.repository.model.PolicyInformation
import com.x8bit.bitwarden.data.auth.repository.model.PrevalidateSsoResult
import com.x8bit.bitwarden.data.auth.repository.model.RegisterResult
import com.x8bit.bitwarden.data.auth.repository.model.RemovePasswordResult
Expand All @@ -42,6 +39,7 @@ import com.x8bit.bitwarden.data.auth.repository.util.WebAuthResult
import com.x8bit.bitwarden.data.auth.util.YubiKeyResult
import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.AuthenticatorProvider
import com.x8bit.bitwarden.data.platform.manager.BiometricsEncryptionManager
import com.x8bit.bitwarden.data.platform.manager.policy.PasswordPolicyManager
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.StateFlow

Expand All @@ -54,6 +52,7 @@ interface AuthRepository :
AuthRequestManager,
BiometricsEncryptionManager,
KdfManager,
PasswordPolicyManager,
UserStateManager {
/**
* Models the current auth state.
Expand Down Expand Up @@ -121,16 +120,6 @@ interface AuthRepository :
*/
var shouldTrustDevice: Boolean

/**
* Return the cached password policies for the current user.
*/
val passwordPolicies: List<PolicyInformation.MasterPassword>

/**
* The reason for resetting the password.
*/
val passwordResetReason: ForcePasswordResetReason?

/**
* The organization for the active user.
*/
Expand Down Expand Up @@ -373,13 +362,6 @@ interface AuthRepository :
*/
suspend fun getPasswordBreachCount(password: String): BreachCountResult

/**
* Get the password strength for the given [email] and [password] combo.
* If no value is passed for the [email] will use the active email of the current active
* account via the [userStateFlow].
*/
suspend fun getPasswordStrength(email: String? = null, password: String): PasswordStrengthResult

/**
* Validates the master password for the current logged-in user.
*/
Expand All @@ -390,12 +372,6 @@ interface AuthRepository :
*/
suspend fun validatePinUserKey(pin: String): ValidatePinResult

/**
* Validates the given [password] against the master password
* policies for the current user.
*/
suspend fun validatePasswordAgainstPolicies(password: String): Boolean

/**
* Send a verification email.
*/
Expand Down
140 changes: 4 additions & 136 deletions app/src/main/kotlin/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 @@ -51,8 +51,6 @@ import com.bitwarden.network.service.HaveIBeenPwnedService
import com.bitwarden.network.service.IdentityService
import com.bitwarden.network.service.OrganizationService
import com.bitwarden.network.util.isSslHandShakeError
import com.bitwarden.policies.PolicyType
import com.bitwarden.policies.PolicyView
import com.bitwarden.ui.platform.resource.BitwardenString
import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource
import com.x8bit.bitwarden.data.auth.datasource.disk.model.AccountJson
Expand All @@ -61,7 +59,6 @@ import com.x8bit.bitwarden.data.auth.datasource.disk.model.ForcePasswordResetRea
import com.x8bit.bitwarden.data.auth.datasource.disk.model.OnboardingStatus
import com.x8bit.bitwarden.data.auth.datasource.network.model.DeviceDataModel
import com.x8bit.bitwarden.data.auth.datasource.sdk.AuthSdkSource
import com.x8bit.bitwarden.data.auth.datasource.sdk.util.toInt
import com.x8bit.bitwarden.data.auth.datasource.sdk.util.toKdfTypeJson
import com.x8bit.bitwarden.data.auth.manager.AuthRequestManager
import com.x8bit.bitwarden.data.auth.manager.KdfManager
Expand All @@ -83,7 +80,6 @@ import com.x8bit.bitwarden.data.auth.repository.model.NewSsoUserResult
import com.x8bit.bitwarden.data.auth.repository.model.Organization
import com.x8bit.bitwarden.data.auth.repository.model.PasswordHintResult
import com.x8bit.bitwarden.data.auth.repository.model.PasswordStrengthResult
import com.x8bit.bitwarden.data.auth.repository.model.PolicyInformation
import com.x8bit.bitwarden.data.auth.repository.model.PrevalidateSsoResult
import com.x8bit.bitwarden.data.auth.repository.model.RegisterResult
import com.x8bit.bitwarden.data.auth.repository.model.RemovePasswordResult
Expand All @@ -105,15 +101,13 @@ 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.activeUserIdChangesFlow
import com.x8bit.bitwarden.data.auth.repository.util.policyInformation
import com.x8bit.bitwarden.data.auth.repository.util.toAccountCryptographicState
import com.x8bit.bitwarden.data.auth.repository.util.toDeviceInfo
import com.x8bit.bitwarden.data.auth.repository.util.toOrganizations
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.updateForcePasswordReset
import com.x8bit.bitwarden.data.auth.repository.util.updateMasterPasswordUnlock
import com.x8bit.bitwarden.data.auth.repository.util.userSwitchingChangesFlow
import com.x8bit.bitwarden.data.auth.util.KdfParamsConstants.DEFAULT_PBKDF2_ITERATIONS
import com.x8bit.bitwarden.data.auth.util.YubiKeyResult
import com.x8bit.bitwarden.data.auth.util.toSdkParams
Expand All @@ -122,9 +116,8 @@ import com.x8bit.bitwarden.data.platform.error.NoActiveUserException
import com.x8bit.bitwarden.data.platform.manager.BiometricsEncryptionManager
import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager
import com.x8bit.bitwarden.data.platform.manager.LogsManager
import com.x8bit.bitwarden.data.platform.manager.PolicyManager
import com.x8bit.bitwarden.data.platform.manager.PushManager
import com.x8bit.bitwarden.data.platform.manager.util.getActivePolicies
import com.x8bit.bitwarden.data.platform.manager.policy.PasswordPolicyManager
import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository
import com.x8bit.bitwarden.data.platform.repository.SettingsRepository
import com.x8bit.bitwarden.data.platform.util.appLinksScheme
Expand All @@ -143,13 +136,10 @@ import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asSharedFlow
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.filter
import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.mapNotNull
import kotlinx.coroutines.flow.merge
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.receiveAsFlow
import kotlinx.coroutines.flow.stateIn
Expand Down Expand Up @@ -182,18 +172,19 @@ class AuthRepositoryImpl(
private val keyConnectorManager: KeyConnectorManager,
private val trustedDeviceManager: TrustedDeviceManager,
private val userLogoutManager: UserLogoutManager,
private val policyManager: PolicyManager,
private val userStateManager: UserStateManager,
private val kdfManager: KdfManager,
private val toastManager: ToastManager,
private val featureFlagManager: FeatureFlagManager,
logsManager: LogsManager,
pushManager: PushManager,
passwordPolicyManager: PasswordPolicyManager,
dispatcherManager: DispatcherManager,
) : AuthRepository,
AuthRequestManager by authRequestManager,
BiometricsEncryptionManager by biometricsEncryptionManager,
KdfManager by kdfManager,
PasswordPolicyManager by passwordPolicyManager,
UserStateManager by userStateManager {
/**
* A scope intended for use when simply collecting multiple flows in order to combine them. The
Expand Down Expand Up @@ -231,12 +222,6 @@ class AuthRepositoryImpl(

private var organizationIdentifier: String? = null

/**
* The password that needs to be checked against any organization policies before
* the user can complete the login flow. This value is stored using the user ID.
*/
private var passwordsToCheckMap = mutableMapOf<String, String>()

private var keyConnectorResponse: GetTokenResponseJson.Success? = null

override var twoFactorResponse: GetTokenResponseJson.TwoFactorRequired? = null
Expand Down Expand Up @@ -299,16 +284,6 @@ class AuthRepositoryImpl(
}
}

override val passwordPolicies: List<PolicyInformation.MasterPassword>
get() = policyManager.getActivePolicies()

override val passwordResetReason: ForcePasswordResetReason?
get() = authDiskSource
.userState
?.activeAccount
?.profile
?.forcePasswordResetReason

override val organizations: List<Organization>
get() = activeUserId
?.let { authDiskSource.getOrganizations(it) }
Expand Down Expand Up @@ -364,52 +339,6 @@ class AuthRepositoryImpl(
.logoutFlow
.onEach { logout(userId = it.userId, reason = LogoutReason.Notification) }
.launchIn(unconfinedScope)

// When the policies for the user have been set, complete the login process.
policyManager
.getActivePoliciesFlow(type = PolicyType.MASTER_PASSWORD)
.onEach { policies ->
val userId = activeUserId ?: return@onEach

// If the user is logging on without a password, the check should complete.
val passwordToCheck = passwordsToCheckMap.remove(key = userId) ?: return@onEach

// If the password already has to be reset for some other reason, there's no
// need to check the password policies.
if (passwordResetReason != null) return@onEach

// Otherwise check the user's password against the policies and set or
// clear the force reset reason accordingly.
authDiskSource.userState = authDiskSource.userState?.updateForcePasswordReset(
userId = userId,
reason = ForcePasswordResetReason
.WEAK_MASTER_PASSWORD_ON_LOGIN
.takeIf {
!passwordPassesPolicies(
password = passwordToCheck,
policies = policies,
)
},
)
}
.launchIn(unconfinedScope)

// Clear the cached password whenever the user is no longer active
// or the vault is locked for that user.
merge(
authDiskSource
.userSwitchingChangesFlow
.mapNotNull { it.previousActiveUserId },
vaultRepository
.vaultUnlockDataStateFlow
.filter { vaultUnlockDataList ->
// Clear if the active user is not currently unlocking or unlocked
vaultUnlockDataList.none { it.userId == activeUserId }
}
.mapNotNull { activeUserId },
)
.onEach { userId -> passwordsToCheckMap.remove(key = userId) }
.launchIn(unconfinedScope)
}

override suspend fun deleteAccountWithMasterPassword(
Expand Down Expand Up @@ -1537,11 +1466,6 @@ class AuthRepositoryImpl(
)
}

override suspend fun validatePasswordAgainstPolicies(
password: String,
): Boolean = passwordPolicies
.all { validatePasswordAgainstPolicy(password, it) }

override suspend fun sendVerificationEmail(
email: String,
name: String,
Expand Down Expand Up @@ -1619,62 +1543,6 @@ class AuthRepositoryImpl(
onFailure = { RevokeFromOrganizationResult.Error(error = it) },
)

@Suppress("CyclomaticComplexMethod")
private suspend fun validatePasswordAgainstPolicy(
password: String,
policy: PolicyInformation.MasterPassword,
): Boolean {
// Check the password against all the enforced rules in the policy.
policy.minLength?.let { minLength ->
if (minLength > 0 && password.length < minLength) return false
}
policy.minComplexity?.let { minComplexity ->
// If there was a problem checking the complexity of the password, ignore
// the complexity checks and continue checking the other aspects of the policy.
val profile = authDiskSource.userState?.activeAccount?.profile ?: return@let
val passwordStrengthResult = getPasswordStrength(profile.email, password)
val passwordStrength = (passwordStrengthResult as? PasswordStrengthResult.Success)
?.passwordStrength
?.toInt()
?: return@let
if (minComplexity > 0 && passwordStrength < minComplexity) return false
}
policy.requireUpper?.let { requiresUpper ->
if (requiresUpper && !password.any { it.isUpperCase() }) return false
}
policy.requireLower?.let { requiresLower ->
if (requiresLower && !password.any { it.isLowerCase() }) return false
}
policy.requireNumbers?.let { requiresNumbers ->
if (requiresNumbers && !password.any { it.isDigit() }) return false
}
policy.requireSpecial?.let { requiresSpecial ->
if (requiresSpecial && !password.contains("^.*[!@#$%\\^&*].*$".toRegex())) return false
}

return true
}

/**
* Return true if there are any [PolicyInformation.MasterPassword] policies that the user's
* master password has failed to pass.
*/
private suspend fun passwordPassesPolicies(
password: String,
policies: List<PolicyView>,
): Boolean {
// If there are no master password policies that are enabled and should be
// enforced on login, the check should complete.
val passwordPolicies = policies
.mapNotNull { it.policyInformation as? PolicyInformation.MasterPassword }
.filter { it.enforceOnLogin == true }

// Check the password against all the policies.
return passwordPolicies.all { policy ->
validatePasswordAgainstPolicy(password, policy)
}
}

/**
* Enrolls the active user in password reset if their organization requires it.
*/
Expand Down Expand Up @@ -1895,7 +1763,7 @@ class AuthRepositoryImpl(
}

// Cache the password to verify against any password policies after the sync completes.
passwordsToCheckMap.put(userId, it)
storePasswordToCheck(userId = userId, password = it)

@vvolkgang vvolkgang Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's circle back on this later and research how we could restructure the sync + policy check process to remove this need.

}

settingsRepository.hasUserLoggedInOrCreatedAccount = true
Expand Down
Loading
Loading