From ee12bd9da5aaf7db68ba742211a3ca673c8b4a5b Mon Sep 17 00:00:00 2001 From: Caleb Derosier <125901828+caleb-livefront@users.noreply.github.com> Date: Mon, 8 Apr 2024 14:16:28 -0600 Subject: [PATCH] BIT-2136: Update minimum length handling in Generator (#1238) --- .../feature/generator/GeneratorScreen.kt | 7 ++-- .../feature/generator/GeneratorViewModel.kt | 32 ++++++++++++------- .../feature/generator/GeneratorScreenTest.kt | 6 +++- .../generator/GeneratorViewModelTest.kt | 30 ++++++++--------- 4 files changed, 43 insertions(+), 32 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreen.kt index 285c9892f8..2c44dde1d8 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreen.kt @@ -28,9 +28,9 @@ import androidx.compose.material3.TopAppBarScrollBehavior import androidx.compose.material3.rememberTopAppBarState import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -580,7 +580,7 @@ private fun PasswordLengthSliderItem( minValue: Int, maxValue: Int, ) { - var sliderValue by remember { mutableIntStateOf(length.coerceIn(minValue, maxValue)) } + val sliderValue by rememberUpdatedState(newValue = length.coerceIn(minValue, maxValue)) var labelTextWidth by remember { mutableStateOf(Dp.Unspecified) } val density = LocalDensity.current @@ -617,8 +617,7 @@ private fun PasswordLengthSliderItem( Slider( value = sliderValue.toFloat(), onValueChange = { newValue -> - sliderValue = newValue.toInt().coerceIn(minValue, maxValue) - onPasswordSliderLengthChange(sliderValue, true) + onPasswordSliderLengthChange(newValue.toInt(), true) }, onValueChangeFinished = { onPasswordSliderLengthChange(sliderValue, false) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModel.kt index ead2d0b7ed..47e0cc257e 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModel.kt @@ -300,7 +300,7 @@ class GeneratorViewModel @Inject constructor( is Password -> { val minLength = policy.minLength ?: Password.PASSWORD_LENGTH_SLIDER_MIN val password = Password( - length = max(options.length, minLength), + rawLength = max(options.length, minLength), minLength = minLength, useCapitals = options.hasUppercase || policy.useUpper == true, capitalsEnabled = policy.useUpper != true, @@ -372,7 +372,7 @@ class GeneratorViewModel @Inject constructor( val options = generatorRepository .getPasscodeGenerationOptions() ?: generatePasscodeDefaultOptions() val newOptions = options.copy( - length = password.minimumLength, + length = password.length, allowAmbiguousChar = password.avoidAmbiguousChars, hasNumbers = password.useNumbers, minNumber = password.minNumbers, @@ -477,7 +477,7 @@ class GeneratorViewModel @Inject constructor( val defaultPassphrase = Passphrase() return PasscodeGenerationOptions( - length = defaultPassword.minimumLength, + length = defaultPassword.length, allowAmbiguousChar = defaultPassword.avoidAmbiguousChars, hasNumbers = defaultPassword.useNumbers, minNumber = defaultPassword.minNumbers, @@ -518,7 +518,7 @@ class GeneratorViewModel @Inject constructor( uppercase = password.useCapitals, numbers = password.useNumbers, special = password.useSpecialChars, - length = password.minimumLength.toUByte(), + length = password.length.toUByte(), avoidAmbiguous = password.avoidAmbiguousChars, minLowercase = null, minUppercase = null, @@ -764,10 +764,14 @@ class GeneratorViewModel @Inject constructor( updatePasswordType { currentPasswordType -> currentPasswordType.copy( - length = adjustedLength, + rawLength = adjustedLength.coerceIn( + minimumValue = currentPasswordType.minLength, + maximumValue = currentPasswordType.maxLength, + ), isUserInteracting = action.isUserInteracting, ) } + updatePasswordLength() } private fun handleToggleCapitalLetters( @@ -1412,7 +1416,7 @@ class GeneratorViewModel @Inject constructor( private fun updatePasswordLength() { updatePasswordType { currentPasswordType -> currentPasswordType.copy( - length = currentPasswordType.minimumLength, + rawLength = currentPasswordType.length, ) } } @@ -1737,7 +1741,7 @@ data class GeneratorState( * Represents a standard PASSWORD type, with configurable options for * length, character types, and requirements. * - * @property length The length of the generated password. + * @property rawLength The length of the generated password. * @property minLength The number available on the low end of the slider. * @property maxLength The number available on the high end of the slider. * @property useCapitals Whether to include capital letters. @@ -1762,7 +1766,7 @@ data class GeneratorState( */ @Parcelize data class Password( - val length: Int = DEFAULT_PASSWORD_LENGTH, + private val rawLength: Int = DEFAULT_PASSWORD_LENGTH, val minLength: Int = PASSWORD_LENGTH_SLIDER_MIN, val maxLength: Int = PASSWORD_LENGTH_SLIDER_MAX, val useCapitals: Boolean = true, @@ -1786,6 +1790,13 @@ data class GeneratorState( override val displayStringResId: Int get() = PasscodeTypeOption.PASSWORD.labelRes + /** + * The larger of the user-set length and the minimum length this password can be + * (as determined by which characters are enabled, and how many). + */ + val length: Int + get() = max(rawLength, minimumLength) + companion object { private const val DEFAULT_PASSWORD_LENGTH: Int = 14 private const val MIN_NUMBERS: Int = 1 @@ -2560,10 +2571,7 @@ private val Password.minimumLength: Int val minUppercase = if (useCapitals) 1 else 0 val minimumNumbers = if (useNumbers) max(1, minNumbers) else 0 val minimumSpecial = if (useSpecialChars) max(1, minSpecial) else 0 - return max( - minLowercase + minUppercase + minimumNumbers + minimumSpecial, - length, - ) + return minLowercase + minUppercase + minimumNumbers + minimumSpecial } private fun UsernameGenerationOptions.ForwardedEmailServiceType?.toServiceType( diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreenTest.kt index 1c7f6475d5..c45126c60d 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreenTest.kt @@ -374,10 +374,14 @@ class GeneratorScreenTest : BaseComposeTest() { ) } + // This value would be 128 in a real scenario, because length passed here depends on the + // `sliderValue` which is indirectly updated via the call verified above. However, because + // the view model is a mock, the `length` value that `sliderValue` depends on will not + // actually get updated from its original value, and thus will be its original value of 14. verify { viewModel.trySendAction( GeneratorAction.MainType.Passcode.PasscodeType.Password.SliderLengthChange( - length = 128, + length = 14, isUserInteracting = false, ), ) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModelTest.kt index 580c01fb36..a99587e110 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModelTest.kt @@ -148,7 +148,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { initialPasscodeState.copy( selectedType = GeneratorState.MainType.Passcode( selectedType = GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 14, + rawLength = 14, minLength = 10, maxLength = 128, useCapitals = true, @@ -182,7 +182,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { initialPasscodeState.copy( selectedType = GeneratorState.MainType.Passcode( selectedType = GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 14, + rawLength = 14, minLength = 5, maxLength = 128, useCapitals = true, @@ -499,7 +499,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { initialPasscodeState.copy( selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 14, + rawLength = 14, minLength = 10, useCapitals = true, capitalsEnabled = false, @@ -821,7 +821,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = newLength, + rawLength = newLength, ), ), ) @@ -863,7 +863,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 5, + rawLength = 5, useCapitals = false, ), ), @@ -882,7 +882,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 5, // 0 uppercase + 1 lowercase + 4 numbers + 0 special chars + rawLength = 5, // 0 uppercase + 1 lowercase + 4 numbers + 0 special chars minNumbers = 4, useCapitals = false, useNumbers = true, @@ -908,7 +908,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 6, // 1 uppercase + 1 lowercase + 4 numbers + 0 special chars + rawLength = 6, // 1 uppercase + 1 lowercase + 4 numbers + 0 special chars minNumbers = 4, useCapitals = true, ), @@ -951,7 +951,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 5, + rawLength = 5, useLowercase = false, ), ), @@ -970,7 +970,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 5, // 1 uppercase + 0 lowercase + 4 numbers + 0 special chars + rawLength = 5, // 1 uppercase + 0 lowercase + 4 numbers + 0 special chars minNumbers = 4, useLowercase = false, useNumbers = true, @@ -996,7 +996,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 6, // 1 uppercase + 1 lowercase + 4 numbers + 0 special chars + rawLength = 6, // 1 uppercase + 1 lowercase + 4 numbers + 0 special chars minNumbers = 4, useLowercase = true, ), @@ -1105,7 +1105,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 5, // the current slider value, which the min does not exceed + rawLength = 5, // the current slider value, which the min doesn't exceed minNumbers = minNumbers, useNumbers = false, ), @@ -1129,7 +1129,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 7, // 1 uppercase + 1 lowercase + 5 numbers + 0 special chars + rawLength = 7, // 1 uppercase + 1 lowercase + 5 numbers + 0 special minNumbers = minNumbers, useNumbers = true, ), @@ -1173,7 +1173,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 5, + rawLength = 5, minSpecial = minSpecial, useSpecialChars = false, ), @@ -1197,7 +1197,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = updatedGeneratedPassword, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = 8, // 1 uppercase + 1 lowercase + 1 number + 5 special chars + rawLength = 8, // 1 uppercase + 1 lowercase + 1 number + 5 special chars minSpecial = minSpecial, useSpecialChars = true, ), @@ -2098,7 +2098,7 @@ class GeneratorViewModelTest : BaseViewModelTest() { generatedText = generatedText, selectedType = GeneratorState.MainType.Passcode( GeneratorState.MainType.Passcode.PasscodeType.Password( - length = length, + rawLength = length, useCapitals = useCapitals, useLowercase = useLowercase, useNumbers = useNumbers,