mirror of
https://github.com/bitwarden/android.git
synced 2025-02-17 12:30:00 +03:00
BIT-1259: Fix for Increasing Length Creates a Password in History for Each Adjustment (#427)
This commit is contained in:
parent
3fd93b2589
commit
f9667d0390
8 changed files with 101 additions and 20 deletions
|
@ -20,10 +20,13 @@ interface GeneratorRepository {
|
|||
val passwordHistoryStateFlow: StateFlow<LocalDataState<List<PasswordHistoryView>>>
|
||||
|
||||
/**
|
||||
* Attempt to generate a password.
|
||||
* Attempt to generate a password based on specifications in [passwordGeneratorRequest].
|
||||
* The [shouldSave] flag determines if the password is saved for future reference
|
||||
* or generated for temporary use.
|
||||
*/
|
||||
suspend fun generatePassword(
|
||||
passwordGeneratorRequest: PasswordGeneratorRequest,
|
||||
shouldSave: Boolean,
|
||||
): GeneratedPasswordResult
|
||||
|
||||
/**
|
||||
|
|
|
@ -25,6 +25,7 @@ import kotlinx.coroutines.flow.launchIn
|
|||
import kotlinx.coroutines.flow.map
|
||||
import kotlinx.coroutines.flow.onEach
|
||||
import kotlinx.coroutines.flow.onStart
|
||||
import kotlinx.coroutines.launch
|
||||
import java.time.Instant
|
||||
import javax.inject.Singleton
|
||||
|
||||
|
@ -79,6 +80,7 @@ class GeneratorRepositoryImpl(
|
|||
|
||||
override suspend fun generatePassword(
|
||||
passwordGeneratorRequest: PasswordGeneratorRequest,
|
||||
shouldSave: Boolean,
|
||||
): GeneratedPasswordResult =
|
||||
generatorSdkSource
|
||||
.generatePassword(passwordGeneratorRequest)
|
||||
|
@ -88,7 +90,12 @@ class GeneratorRepositoryImpl(
|
|||
password = generatedPassword,
|
||||
lastUsedDate = Instant.now(),
|
||||
)
|
||||
storePasswordHistory(passwordHistoryView)
|
||||
|
||||
if (shouldSave) {
|
||||
scope.launch {
|
||||
storePasswordHistory(passwordHistoryView)
|
||||
}
|
||||
}
|
||||
GeneratedPasswordResult.Success(generatedPassword)
|
||||
},
|
||||
onFailure = { GeneratedPasswordResult.InvalidRequest },
|
||||
|
@ -105,7 +112,9 @@ class GeneratorRepositoryImpl(
|
|||
password = generatedPassphrase,
|
||||
lastUsedDate = Instant.now(),
|
||||
)
|
||||
storePasswordHistory(passwordHistoryView)
|
||||
scope.launch {
|
||||
storePasswordHistory(passwordHistoryView)
|
||||
}
|
||||
GeneratedPassphraseResult.Success(generatedPassphrase)
|
||||
},
|
||||
onFailure = { GeneratedPassphraseResult.InvalidRequest },
|
||||
|
|
|
@ -47,6 +47,7 @@ import androidx.compose.ui.unit.Dp
|
|||
import androidx.compose.ui.unit.dp
|
||||
import androidx.hilt.navigation.compose.hiltViewModel
|
||||
import androidx.lifecycle.compose.collectAsStateWithLifecycle
|
||||
import androidx.lifecycle.viewmodel.compose.viewModel
|
||||
import com.x8bit.bitwarden.R
|
||||
import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect
|
||||
import com.x8bit.bitwarden.ui.platform.base.util.toDp
|
||||
|
@ -404,8 +405,7 @@ private fun ColumnScope.PasswordTypeContent(
|
|||
|
||||
PasswordLengthSliderItem(
|
||||
length = passwordTypeState.length,
|
||||
onPasswordSliderLengthChange =
|
||||
passwordHandlers.onPasswordSliderLengthChange,
|
||||
onPasswordSliderLengthChange = passwordHandlers.onPasswordSliderLengthChange,
|
||||
)
|
||||
|
||||
Spacer(modifier = Modifier.height(8.dp))
|
||||
|
@ -464,8 +464,9 @@ private fun ColumnScope.PasswordTypeContent(
|
|||
@Composable
|
||||
private fun PasswordLengthSliderItem(
|
||||
length: Int,
|
||||
onPasswordSliderLengthChange: (Int) -> Unit,
|
||||
onPasswordSliderLengthChange: (value: Int, isUserInteracting: Boolean) -> Unit,
|
||||
) {
|
||||
var sliderValue by remember { mutableStateOf(length) }
|
||||
var labelTextWidth by remember { mutableStateOf(Dp.Unspecified) }
|
||||
|
||||
val density = LocalDensity.current
|
||||
|
@ -479,11 +480,7 @@ private fun PasswordLengthSliderItem(
|
|||
OutlinedTextField(
|
||||
value = length.toString(),
|
||||
readOnly = true,
|
||||
onValueChange = { newText ->
|
||||
newText.toIntOrNull()?.let { newValue ->
|
||||
onPasswordSliderLengthChange(newValue)
|
||||
}
|
||||
},
|
||||
onValueChange = { },
|
||||
label = {
|
||||
Text(
|
||||
text = stringResource(id = R.string.length),
|
||||
|
@ -502,9 +499,13 @@ private fun PasswordLengthSliderItem(
|
|||
)
|
||||
|
||||
Slider(
|
||||
value = length.toFloat(),
|
||||
value = sliderValue.toFloat(),
|
||||
onValueChange = { newValue ->
|
||||
onPasswordSliderLengthChange(newValue.toInt())
|
||||
sliderValue = newValue.toInt()
|
||||
onPasswordSliderLengthChange(sliderValue, true)
|
||||
},
|
||||
onValueChangeFinished = {
|
||||
onPasswordSliderLengthChange(sliderValue, false)
|
||||
},
|
||||
valueRange =
|
||||
PASSWORD_LENGTH_SLIDER_MIN.toFloat()..PASSWORD_LENGTH_SLIDER_MAX.toFloat(),
|
||||
|
@ -938,7 +939,7 @@ private fun GeneratorPreview() {
|
|||
*/
|
||||
@Suppress("LongParameterList")
|
||||
private class PasswordHandlers(
|
||||
val onPasswordSliderLengthChange: (Int) -> Unit,
|
||||
val onPasswordSliderLengthChange: (Int, Boolean) -> Unit,
|
||||
val onPasswordToggleCapitalLettersChange: (Boolean) -> Unit,
|
||||
val onPasswordToggleLowercaseLettersChange: (Boolean) -> Unit,
|
||||
val onPasswordToggleNumbersChange: (Boolean) -> Unit,
|
||||
|
@ -951,11 +952,12 @@ private class PasswordHandlers(
|
|||
@Suppress("LongMethod")
|
||||
fun create(viewModel: GeneratorViewModel): PasswordHandlers {
|
||||
return PasswordHandlers(
|
||||
onPasswordSliderLengthChange = { newLength ->
|
||||
onPasswordSliderLengthChange = { newLength, isUserInteracting ->
|
||||
viewModel.trySendAction(
|
||||
GeneratorAction.MainType.Passcode.PasscodeType.Password
|
||||
.SliderLengthChange(
|
||||
length = newLength,
|
||||
isUserInteracting = isUserInteracting,
|
||||
),
|
||||
)
|
||||
},
|
||||
|
|
|
@ -236,8 +236,9 @@ class GeneratorViewModel @Inject constructor(
|
|||
minNumber = null,
|
||||
minSpecial = null,
|
||||
)
|
||||
val shouldSave = !password.isUserInteracting
|
||||
|
||||
val result = generatorRepository.generatePassword(request)
|
||||
val result = generatorRepository.generatePassword(request, shouldSave)
|
||||
sendAction(GeneratorAction.Internal.UpdateGeneratedPasswordResult(result))
|
||||
}
|
||||
|
||||
|
@ -386,7 +387,10 @@ class GeneratorViewModel @Inject constructor(
|
|||
val adjustedLength = action.length
|
||||
|
||||
updatePasswordType { currentPasswordType ->
|
||||
currentPasswordType.copy(length = adjustedLength)
|
||||
currentPasswordType.copy(
|
||||
length = adjustedLength,
|
||||
isUserInteracting = action.isUserInteracting,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -839,6 +843,9 @@ data class GeneratorState(
|
|||
* @property minNumbers The minimum number of numeric characters.
|
||||
* @property minSpecial The minimum number of special characters.
|
||||
* @property avoidAmbiguousChars Whether to avoid characters that look similar.
|
||||
* @property isUserInteracting Indicates whether the user is currently interacting
|
||||
* with a control. This flag can be used to prevent unnecessary updates or
|
||||
* processing during continuous interaction.
|
||||
*/
|
||||
@Parcelize
|
||||
data class Password(
|
||||
|
@ -850,6 +857,7 @@ data class GeneratorState(
|
|||
val minNumbers: Int = MIN_NUMBERS,
|
||||
val minSpecial: Int = MIN_SPECIAL,
|
||||
val avoidAmbiguousChars: Boolean = false,
|
||||
val isUserInteracting: Boolean = false,
|
||||
) : PasscodeType(), Parcelable {
|
||||
override val displayStringResId: Int
|
||||
get() = PasscodeTypeOption.PASSWORD.labelRes
|
||||
|
@ -1182,6 +1190,7 @@ sealed class GeneratorAction {
|
|||
*/
|
||||
data class SliderLengthChange(
|
||||
val length: Int,
|
||||
val isUserInteracting: Boolean,
|
||||
) : Password()
|
||||
|
||||
/**
|
||||
|
|
|
@ -71,8 +71,9 @@ class GeneratorRepositoryTest {
|
|||
unmockkStatic(Instant::class)
|
||||
}
|
||||
|
||||
@Suppress("MaxLineLength")
|
||||
@Test
|
||||
fun `generatePassword should emit Success result and store the generated password`() = runTest {
|
||||
fun `generatePassword should emit Success result and store the generated password when shouldSave is true`() = runTest {
|
||||
val fixedInstant = Instant.parse("2021-01-01T00:00:00Z")
|
||||
|
||||
mockkStatic(Instant::class)
|
||||
|
@ -105,7 +106,7 @@ class GeneratorRepositoryTest {
|
|||
|
||||
coEvery { passwordHistoryDiskSource.insertPasswordHistory(any()) } just runs
|
||||
|
||||
val result = repository.generatePassword(request)
|
||||
val result = repository.generatePassword(request, true)
|
||||
|
||||
assertEquals(generatedPassword, (result as GeneratedPasswordResult.Success).generatedString)
|
||||
coVerify { generatorSdkSource.generatePassword(request) }
|
||||
|
@ -117,6 +118,51 @@ class GeneratorRepositoryTest {
|
|||
}
|
||||
}
|
||||
|
||||
@Suppress("MaxLineLength")
|
||||
@Test
|
||||
fun `generatePassword should emit Success result but not store the generated password when shouldSave is false`() = runTest {
|
||||
val fixedInstant = Instant.parse("2021-01-01T00:00:00Z")
|
||||
|
||||
mockkStatic(Instant::class)
|
||||
every { Instant.now() } returns fixedInstant
|
||||
|
||||
val userId = "testUserId"
|
||||
val request = PasswordGeneratorRequest(
|
||||
lowercase = true,
|
||||
uppercase = true,
|
||||
numbers = true,
|
||||
special = true,
|
||||
length = 12.toUByte(),
|
||||
avoidAmbiguous = false,
|
||||
minLowercase = null,
|
||||
minUppercase = null,
|
||||
minNumber = null,
|
||||
minSpecial = null,
|
||||
)
|
||||
val generatedPassword = "GeneratedPassword123!"
|
||||
val encryptedPasswordHistory =
|
||||
PasswordHistory(password = generatedPassword, lastUsedDate = Instant.now())
|
||||
|
||||
coEvery { authDiskSource.userState?.activeUserId } returns userId
|
||||
|
||||
coEvery { generatorSdkSource.generatePassword(request) } returns
|
||||
Result.success(generatedPassword)
|
||||
|
||||
coEvery { vaultSdkSource.encryptPasswordHistory(any()) } returns
|
||||
Result.success(encryptedPasswordHistory)
|
||||
|
||||
coEvery { passwordHistoryDiskSource.insertPasswordHistory(any()) } just runs
|
||||
|
||||
val result = repository.generatePassword(request, false)
|
||||
|
||||
assertEquals(generatedPassword, (result as GeneratedPasswordResult.Success).generatedString)
|
||||
coVerify { generatorSdkSource.generatePassword(request) }
|
||||
|
||||
coVerify(exactly = 0) {
|
||||
passwordHistoryDiskSource.insertPasswordHistory(any())
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `generatePassword should emit InvalidRequest result when SDK throws exception`() = runTest {
|
||||
val request = PasswordGeneratorRequest(
|
||||
|
@ -134,7 +180,7 @@ class GeneratorRepositoryTest {
|
|||
val exception = RuntimeException("An error occurred")
|
||||
coEvery { generatorSdkSource.generatePassword(request) } returns Result.failure(exception)
|
||||
|
||||
val result = repository.generatePassword(request)
|
||||
val result = repository.generatePassword(request, true)
|
||||
|
||||
assertTrue(result is GeneratedPasswordResult.InvalidRequest)
|
||||
coVerify { generatorSdkSource.generatePassword(request) }
|
||||
|
|
|
@ -33,6 +33,7 @@ class FakeGeneratorRepository : GeneratorRepository {
|
|||
|
||||
override suspend fun generatePassword(
|
||||
passwordGeneratorRequest: PasswordGeneratorRequest,
|
||||
shouldSave: Boolean,
|
||||
): GeneratedPasswordResult {
|
||||
return generatePasswordResult
|
||||
}
|
||||
|
|
|
@ -288,6 +288,16 @@ class GeneratorScreenTest : BaseComposeTest() {
|
|||
viewModel.trySendAction(
|
||||
GeneratorAction.MainType.Passcode.PasscodeType.Password.SliderLengthChange(
|
||||
length = 128,
|
||||
isUserInteracting = true,
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
verify {
|
||||
viewModel.trySendAction(
|
||||
GeneratorAction.MainType.Passcode.PasscodeType.Password.SliderLengthChange(
|
||||
length = 128,
|
||||
isUserInteracting = false,
|
||||
),
|
||||
)
|
||||
}
|
||||
|
|
|
@ -409,6 +409,7 @@ class GeneratorViewModelTest : BaseViewModelTest() {
|
|||
viewModel.actionChannel.trySend(
|
||||
GeneratorAction.MainType.Passcode.PasscodeType.Password.SliderLengthChange(
|
||||
length = newLength,
|
||||
isUserInteracting = false,
|
||||
),
|
||||
)
|
||||
|
||||
|
|
Loading…
Add table
Reference in a new issue