Skip to content

PM-27597: Update Yubikey illustration to match the rest of the app #6087

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

Merged
merged 2 commits into from
Nov 4, 2025
Merged

PM-27597: Update Yubikey illustration to match the rest of the app #6087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6be233f
PM-27817: Consolidate totp parsing with TotpUriUtils
david-livefront Nov 4, 2025
d9a1f8a
PM-27597: Update Yubikey illustration and copy
david-livefront Nov 4, 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
91 changes: 37 additions & 54 deletions ...rc/main/kotlin/com/x8bit/bitwarden/ui/auth/feature/twofactorlogin/TwoFactorLoginScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,19 @@ import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.navigationBarsPadding
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.foundation.text.KeyboardActions
import androidx.compose.foundation.verticalScroll
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.Text
import androidx.compose.material3.TopAppBarDefaults
import androidx.compose.material3.rememberTopAppBarState
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.input.nestedscroll.nestedScroll
import androidx.compose.ui.layout.ContentScale
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.text.input.ImeAction
Expand Down Expand Up @@ -58,7 +53,6 @@ import com.bitwarden.ui.platform.manager.IntentManager
import com.bitwarden.ui.platform.resource.BitwardenDrawable
import com.bitwarden.ui.platform.resource.BitwardenString
import com.bitwarden.ui.platform.theme.BitwardenTheme
import com.bitwarden.ui.util.asText
import com.x8bit.bitwarden.ui.auth.feature.twofactorlogin.util.description
import com.x8bit.bitwarden.ui.auth.feature.twofactorlogin.util.title
import com.x8bit.bitwarden.ui.platform.composition.LocalAuthTabLaunchers
Expand Down Expand Up @@ -126,19 +120,18 @@ fun TwoFactorLoginScreen(
},
)

val scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior(rememberTopAppBarState())
val title = if (state.isNewDeviceVerification) {
BitwardenString.verify_your_identity.asText()
} else {
state.authMethod.title
}
val scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior()
BitwardenScaffold(
modifier = Modifier
.fillMaxSize()
.nestedScroll(scrollBehavior.nestedScrollConnection),
topBar = {
BitwardenTopAppBar(
title = title(),
title = if (state.isNewDeviceVerification) {
stringResource(id = BitwardenString.verify_your_identity)
} else {
state.authMethod.title()
},
scrollBehavior = scrollBehavior,
navigationIcon = rememberVectorPainter(id = BitwardenDrawable.ic_close),
navigationIconContentDescription = stringResource(id = BitwardenString.close),
Expand Down Expand Up @@ -230,55 +223,45 @@ private fun TwoFactorLoginScreenContent(
modifier = modifier
.verticalScroll(rememberScrollState()),
) {
if (state.authMethod != TwoFactorAuthMethod.YUBI_KEY) {
state.imageRes?.let {
Spacer(modifier = Modifier.height(12.dp))
Image(
painter = painterResource(id = it),
contentDescription = null,
modifier = Modifier
.standardHorizontalMargin()
.size(124.dp),
)
Spacer(modifier = Modifier.height(12.dp))
}
state.imageRes?.let {
Spacer(modifier = Modifier.height(12.dp))
Image(
painter = painterResource(id = it),
contentDescription = null,
modifier = Modifier
.standardHorizontalMargin()
.size(124.dp),
Copy link
Contributor

@claude claude bot Nov 4, 2025

Choose a reason for hiding this comment

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

👍 Good Improvement

The migration from painterResource (used for raster images) to using it with the new vector drawable is correct. The image is now properly sized at 124.dp, which matches the vector drawable's viewport dimensions and provides consistent sizing with other 2FA method illustrations like the authenticator app icon.

The removal of unnecessary layout complexity (previous version had more nested layouts) also improves performance.

Copy link
Contributor

@claude claude bot Nov 4, 2025

Choose a reason for hiding this comment

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

👍 Improved Image Rendering

Good migration from raster to vector drawable:

  • ✅ painterResource now used with vector drawable
  • ✅ Proper sizing at 124.dp matching viewport dimensions
  • ✅ Consistent with other 2FA illustrations (authenticator app icon)
  • ✅ Simplified layout complexity (removed unnecessary nesting)

The streamlined implementation improves both performance and maintainability.

)
Spacer(modifier = Modifier.height(12.dp))
}
Spacer(modifier = Modifier.height(height = 12.dp))
Text(
text = if (state.isNewDeviceVerification) {
stringResource(BitwardenString.enter_verification_code_new_device)
} else {
state.authMethod.description(
state.displayEmail,
).invoke()
},
textAlign = TextAlign.Center,
style = BitwardenTheme.typography.bodyMedium,
color = BitwardenTheme.colorScheme.text.primary,
modifier = Modifier
.standardHorizontalMargin()
.fillMaxWidth(),
)

Spacer(modifier = Modifier.height(12.dp))

if (state.authMethod == TwoFactorAuthMethod.YUBI_KEY) {
state.imageRes?.let {
Spacer(modifier = Modifier.height(12.dp))
Image(
painter = painterResource(id = it),
contentDescription = null,
alignment = Alignment.Center,
contentScale = ContentScale.FillWidth,
if (state.isNewDeviceVerification) {
Spacer(modifier = Modifier.height(height = 12.dp))
Text(
text = stringResource(id = BitwardenString.enter_verification_code_new_device),
textAlign = TextAlign.Center,
style = BitwardenTheme.typography.bodyMedium,
color = BitwardenTheme.colorScheme.text.primary,
modifier = Modifier
.standardHorizontalMargin()
.fillMaxWidth(),
)
} else {
state.authMethod.description(email = state.displayEmail)?.let { text ->
Spacer(modifier = Modifier.height(height = 12.dp))
Text(
text = text(),
textAlign = TextAlign.Center,
style = BitwardenTheme.typography.bodyMedium,
color = BitwardenTheme.colorScheme.text.primary,
modifier = Modifier
.padding(horizontal = 24.dp)
.clip(RoundedCornerShape(4.dp))
.standardHorizontalMargin()
.fillMaxWidth(),
)
Spacer(modifier = Modifier.height(24.dp))
}
}

Spacer(modifier = Modifier.height(12.dp))
if (state.shouldShowCodeInput) {
BitwardenPasswordField(
value = state.codeInput,
Expand Down
11 changes: 8 additions & 3 deletions .../com/x8bit/bitwarden/ui/auth/feature/twofactorlogin/util/TwoFactorAuthMethodExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ val TwoFactorAuthMethod.title: Text
/**
* Get the description for the given auth method.
*/
fun TwoFactorAuthMethod.description(email: String): Text = when (this) {
fun TwoFactorAuthMethod.description(email: String): Text? = when (this) {
TwoFactorAuthMethod.AUTHENTICATOR_APP -> BitwardenString.enter_verification_code_app.asText()
TwoFactorAuthMethod.DUO -> {
BitwardenString.follow_the_steps_from_duo_to_finish_logging_in.asText()
}

TwoFactorAuthMethod.DUO_ORGANIZATION -> {
BitwardenString.duo_two_step_login_is_required_for_your_account
.asText()
Expand All @@ -45,8 +46,12 @@ fun TwoFactorAuthMethod.description(email: String): Text = when (this) {
TwoFactorAuthMethod.WEB_AUTH -> {
BitwardenString.continue_to_complete_web_authn_verification.asText()
}

TwoFactorAuthMethod.YUBI_KEY -> BitwardenString.yubi_key_instruction.asText()
else -> "".asText()
TwoFactorAuthMethod.U2F,
TwoFactorAuthMethod.REMEMBER,
TwoFactorAuthMethod.RECOVERY_CODE,
-> null
}

/**
Expand Down Expand Up @@ -122,7 +127,7 @@ val TwoFactorAuthMethod.shouldUseNfc: Boolean
@get:DrawableRes
val TwoFactorAuthMethod.imageRes: Int?
get() = when (this) {
TwoFactorAuthMethod.YUBI_KEY -> BitwardenDrawable.img_yubi_key
TwoFactorAuthMethod.YUBI_KEY -> BitwardenDrawable.ill_yubi_key
Copy link
Contributor

@claude claude bot Nov 4, 2025

Choose a reason for hiding this comment

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

👍 Correct Resource Update

The reference has been properly updated from the raster image resources (img_yubi_key) to the new vector drawable (ill_yubi_key). This follows the established naming convention in the codebase:

  • img_* prefix for raster images
  • ill_* prefix for illustration vector drawables

The vector drawable provides:

  • Better scalability across different screen densities
  • Smaller binary size (single XML file vs 5 PNG files)
  • Consistency with other 2FA illustrations

Copy link
Contributor

@claude claude bot Nov 4, 2025

Choose a reason for hiding this comment

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

👍 Correct Resource Update

Properly updated from raster (img_yubi_key) to vector drawable (ill_yubi_key). Follows established naming conventions:

  • img_* prefix for raster images
  • ill_* prefix for illustration vector drawables

The vector approach provides better scalability and smaller binary size.

TwoFactorAuthMethod.EMAIL -> BitwardenDrawable.ill_new_device_verification
TwoFactorAuthMethod.AUTHENTICATOR_APP -> BitwardenDrawable.ill_authenticator
else -> null
Expand Down
93 changes: 10 additions & 83 deletions app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/qrcodescan/QrCodeScanViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,14 @@
package com.x8bit.bitwarden.ui.vault.feature.qrcodescan

import android.net.Uri
import com.bitwarden.ui.platform.base.BaseViewModel
import com.bitwarden.ui.platform.util.getTotpDataOrNull
import com.x8bit.bitwarden.data.vault.repository.VaultRepository
import com.x8bit.bitwarden.data.vault.repository.model.TotpCodeResult
import dagger.hilt.android.lifecycle.HiltViewModel
import javax.inject.Inject

private const val ALGORITHM = "algorithm"
private const val DIGITS = "digits"
private const val PERIOD = "period"
private const val SECRET = "secret"
private const val TOTP_CODE_PREFIX = "otpauth://totp"

/**
* Handles [QrCodeScanAction],
* and launches [QrCodeScanEvent] for the [QrCodeScanScreen].
* Handles [QrCodeScanAction] and launches [QrCodeScanEvent] for the [QrCodeScanScreen].
*/
@HiltViewModel
class QrCodeScanViewModel @Inject constructor(
Expand All @@ -33,83 +26,25 @@ class QrCodeScanViewModel @Inject constructor(
}

private fun handleCloseClick() {
sendEvent(
QrCodeScanEvent.NavigateBack,
)
sendEvent(QrCodeScanEvent.NavigateBack)
}

private fun handleManualEntryTextClick() {
sendEvent(
QrCodeScanEvent.NavigateToManualCodeEntry,
)
sendEvent(QrCodeScanEvent.NavigateToManualCodeEntry)
}

// For more information: https://bitwarden.com/help/authenticator-keys/#support-for-more-parameters
private fun handleQrCodeScanReceive(action: QrCodeScanAction.QrCodeScanReceive) {
var result: TotpCodeResult = TotpCodeResult.Success(action.qrCode)
val scannedCode = action.qrCode

if (scannedCode.isBlank() || !scannedCode.startsWith(TOTP_CODE_PREFIX)) {
vaultRepository.emitTotpCodeResult(TotpCodeResult.CodeScanningError())
sendEvent(QrCodeScanEvent.NavigateBack)
return
}

val scannedCodeUri = Uri.parse(scannedCode)
val secretValue = scannedCodeUri.getQueryParameter(SECRET)
if (secretValue == null || !secretValue.isBase32()) {
vaultRepository.emitTotpCodeResult(TotpCodeResult.CodeScanningError())
sendEvent(QrCodeScanEvent.NavigateBack)
return
}

val values = scannedCodeUri.queryParameterNames
if (!areParametersValid(scannedCode, values)) {
result = TotpCodeResult.CodeScanningError()
}

vaultRepository.emitTotpCodeResult(result)
val qrCode = action.qrCode
qrCode
.getTotpDataOrNull()
Copy link
Contributor

@claude claude bot Nov 4, 2025

Choose a reason for hiding this comment

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

👍 Simplified ViewModel Logic

Excellent refactoring using the new TotpUriUtils.getTotpDataOrNull() extension:

Before: ~80 lines of URI parsing logic duplicated in this ViewModel
After: Single line delegation to shared utility

This aligns with MVVM architecture guidelines:

  • ViewModels should focus on UI logic, not parsing utilities
  • Shared parsing logic belongs in utility modules (:ui)
  • Reduces cyclomatic complexity and improves testability

The error handling remains clear with the elvis operator handling invalid TOTP data.

?.let { vaultRepository.emitTotpCodeResult(TotpCodeResult.Success(code = qrCode)) }
?: run { vaultRepository.emitTotpCodeResult(TotpCodeResult.CodeScanningError()) }
sendEvent(QrCodeScanEvent.NavigateBack)
}

private fun handleCameraErrorReceive() {
sendEvent(
QrCodeScanEvent.NavigateToManualCodeEntry,
)
}

@Suppress("NestedBlockDepth", "MagicNumber")
private fun areParametersValid(scannedCode: String, parameters: Set<String>): Boolean {
parameters.forEach { parameter ->
Uri.parse(scannedCode).getQueryParameter(parameter)?.let { value ->
when (parameter) {
DIGITS -> {
val digit = value.toInt()
if (digit > 10 || digit < 1) {
return false
}
}

PERIOD -> {
val period = value.toInt()
if (period < 1) {
return false
}
}

ALGORITHM -> {
val lowercaseAlgo = value.lowercase()
if (lowercaseAlgo != "sha1" &&
lowercaseAlgo != "sha256" &&
lowercaseAlgo != "sha512"
) {
return false
}
}
}
}
}
return true
sendEvent(QrCodeScanEvent.NavigateToManualCodeEntry)
}
}

Expand Down Expand Up @@ -154,11 +89,3 @@ sealed class QrCodeScanAction {
*/
data object CameraSetupErrorReceive : QrCodeScanAction()
}

/**
* Checks if a string is using base32 digits.
*/
private fun String.isBase32(): Boolean {
val regex = ("^[A-Za-z2-7]+=*$").toRegex()
return regex.matches(this)
}
8 changes: 4 additions & 4 deletions ...m/x8bit/bitwarden/ui/auth/feature/twofactorlogin/util/TwoFactorAuthMethodExtensionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ class TwoFactorAuthMethodExtensionTest {
TwoFactorAuthMethod.DUO to
BitwardenString.follow_the_steps_from_duo_to_finish_logging_in.asText(),
TwoFactorAuthMethod.YUBI_KEY to BitwardenString.yubi_key_instruction.asText(),
TwoFactorAuthMethod.U2F to "".asText(),
TwoFactorAuthMethod.REMEMBER to "".asText(),
TwoFactorAuthMethod.U2F to null,
TwoFactorAuthMethod.REMEMBER to null,
TwoFactorAuthMethod.DUO_ORGANIZATION to
BitwardenString.duo_two_step_login_is_required_for_your_account
.asText()
Expand All @@ -54,7 +54,7 @@ class TwoFactorAuthMethodExtensionTest {
),
TwoFactorAuthMethod.WEB_AUTH to
BitwardenString.continue_to_complete_web_authn_verification.asText(),
TwoFactorAuthMethod.RECOVERY_CODE to "".asText(),
TwoFactorAuthMethod.RECOVERY_CODE to null,
)
.forEach { (type, title) ->
assertEquals(
Expand Down Expand Up @@ -142,7 +142,7 @@ class TwoFactorAuthMethodExtensionTest {
TwoFactorAuthMethod.AUTHENTICATOR_APP to BitwardenDrawable.ill_authenticator,
TwoFactorAuthMethod.EMAIL to BitwardenDrawable.ill_new_device_verification,
TwoFactorAuthMethod.DUO to null,
TwoFactorAuthMethod.YUBI_KEY to BitwardenDrawable.img_yubi_key,
TwoFactorAuthMethod.YUBI_KEY to BitwardenDrawable.ill_yubi_key,
Copy link
Contributor

@claude claude bot Nov 4, 2025

Choose a reason for hiding this comment

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

✅ Test Coverage Updated

The test has been correctly updated to verify that TwoFactorAuthMethod.YUBI_KEY returns BitwardenDrawable.ill_yubi_key instead of the old raster image resource. This ensures:

  1. The extension property returns the correct drawable resource
  2. Test coverage is maintained at the same level
  3. Future refactoring won't break the image association

All tests are passing according to CI, confirming the changes work as expected.

Copy link
Contributor

@claude claude bot Nov 4, 2025

Choose a reason for hiding this comment

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

✅ Test Coverage Maintained

Test correctly updated to verify TwoFactorAuthMethod.YUBI_KEY returns the new ill_yubi_key drawable resource. This ensures:

  • ✅ Extension property returns correct drawable
  • ✅ Test coverage maintained at same level
  • ✅ Future refactoring won't break the image association

All tests passing per CI confirms the changes work as expected.

TwoFactorAuthMethod.U2F to null,
TwoFactorAuthMethod.REMEMBER to null,
TwoFactorAuthMethod.DUO_ORGANIZATION to null,
Expand Down
Loading
Loading