From 5ec0c1010104b6411b39d4badebbe1b8f59ee2e8 Mon Sep 17 00:00:00 2001 From: romain Date: Tue, 17 Dec 2024 22:04:46 +0100 Subject: [PATCH 1/5] #4416 add possibility to select a client certificate for mTLS --- .../java/com/x8bit/bitwarden/MainActivity.kt | 8 ++ .../disk/model/EnvironmentUrlDataJson.kt | 6 ++ .../network/di/PlatformNetworkModule.kt | 21 ++++++ .../network/retrofit/RetrofitsImpl.kt | 4 +- .../datasource/network/util/TLSHelper.kt | 75 +++++++++++++++++++ .../ChoosePrivateKeyAliasCallback.kt | 7 ++ .../ChoosePrivateKeyAliasCallbackImpl.kt | 12 +++ .../platform/repository/KeyChainRepository.kt | 13 ++++ .../repository/KeyChainRepositoryImpl.kt | 58 ++++++++++++++ .../ui/auth/feature/auth/AuthNavigation.kt | 3 + .../environment/EnvironmentNavigation.kt | 4 +- .../feature/environment/EnvironmentScreen.kt | 34 +++++++++ .../environment/EnvironmentViewModel.kt | 21 ++++++ .../platform/feature/rootnav/RootNavScreen.kt | 4 +- app/src/main/res/values-de-rDE/strings.xml | 3 + app/src/main/res/values-fr-rFR/strings.xml | 3 + app/src/main/res/values/strings.xml | 3 + .../network/retrofit/RetrofitsTest.kt | 7 ++ .../environment/EnvironmentScreenTest.kt | 46 ++++++++++++ .../environment/EnvironmentViewModelTest.kt | 3 + 20 files changed, 332 insertions(+), 3 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt b/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt index 175487965b2..e6abd152627 100644 --- a/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt +++ b/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt @@ -19,6 +19,8 @@ import com.x8bit.bitwarden.data.autofill.accessibility.manager.AccessibilityComp import com.x8bit.bitwarden.data.autofill.manager.AutofillActivityManager import com.x8bit.bitwarden.data.autofill.manager.AutofillCompletionManager import com.x8bit.bitwarden.data.platform.annotation.OmitFromCoverage +import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback +import com.x8bit.bitwarden.data.platform.repository.KeyChainRepository import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect import com.x8bit.bitwarden.ui.platform.composition.LocalManagerProvider @@ -53,6 +55,9 @@ class MainActivity : AppCompatActivity() { @Inject lateinit var debugLaunchManager: DebugMenuLaunchManager + @Inject + lateinit var keyChainRepository: KeyChainRepository + override fun onCreate(savedInstanceState: Bundle?) { var shouldShowSplashScreen = true installSplashScreen().setKeepOnScreenCondition { shouldShowSplashScreen } @@ -102,6 +107,9 @@ class MainActivity : AppCompatActivity() { RootNavScreen( onSplashScreenRemoved = { shouldShowSplashScreen = false }, navController = navController, + choosePrivateKeyAlias = { callback: ChoosePrivateKeyAliasCallback -> + keyChainRepository.choosePrivateKeyAlias(this@MainActivity, callback) + }, ) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/model/EnvironmentUrlDataJson.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/model/EnvironmentUrlDataJson.kt index 738091f49bd..d6ddaeeab51 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/model/EnvironmentUrlDataJson.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/model/EnvironmentUrlDataJson.kt @@ -7,6 +7,7 @@ import kotlinx.serialization.Serializable * Represents URLs for various Bitwarden domains. * * @property base The overall base URL. + * @property keyAlias A key alias to use for connections with the server. * @property api Separate base URL for the "/api" domain (if applicable). * @property identity Separate base URL for the "/identity" domain (if applicable). * @property icon Separate base URL for the icon domain (if applicable). @@ -19,6 +20,9 @@ data class EnvironmentUrlDataJson( @SerialName("base") val base: String, + @SerialName("keyAlias") + val keyAlias: String? = null, + @SerialName("api") val api: String? = null, @@ -51,6 +55,7 @@ data class EnvironmentUrlDataJson( */ val DEFAULT_LEGACY_US: EnvironmentUrlDataJson = EnvironmentUrlDataJson( base = "https://vault.bitwarden.com", + keyAlias = null, api = "https://api.bitwarden.com", identity = "https://identity.bitwarden.com", icon = "https://icons.bitwarden.net", @@ -71,6 +76,7 @@ data class EnvironmentUrlDataJson( */ val DEFAULT_LEGACY_EU: EnvironmentUrlDataJson = EnvironmentUrlDataJson( base = "https://vault.bitwarden.eu", + keyAlias = null, api = "https://api.bitwarden.eu", identity = "https://identity.bitwarden.eu", icon = "https://icons.bitwarden.eu", diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt index ff476177341..6b155219194 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt @@ -1,6 +1,8 @@ package com.x8bit.bitwarden.data.platform.datasource.network.di +import android.content.Context import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource +import com.x8bit.bitwarden.data.platform.datasource.disk.EnvironmentDiskSource import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.RefreshAuthenticator import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.AuthTokenInterceptor import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.BaseUrlInterceptors @@ -14,9 +16,13 @@ import com.x8bit.bitwarden.data.platform.datasource.network.service.EventService import com.x8bit.bitwarden.data.platform.datasource.network.service.EventServiceImpl import com.x8bit.bitwarden.data.platform.datasource.network.service.PushService import com.x8bit.bitwarden.data.platform.datasource.network.service.PushServiceImpl +import com.x8bit.bitwarden.data.platform.datasource.network.util.TLSHelper +import com.x8bit.bitwarden.data.platform.repository.KeyChainRepository +import com.x8bit.bitwarden.data.platform.repository.KeyChainRepositoryImpl import dagger.Module import dagger.Provides import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.components.SingletonComponent import kotlinx.serialization.json.Json import kotlinx.serialization.modules.SerializersModule @@ -70,6 +76,19 @@ object PlatformNetworkModule { @Singleton fun providesRefreshAuthenticator(): RefreshAuthenticator = RefreshAuthenticator() + @Provides + @Singleton + fun providesKeyChainRepository( + environmentDiskSource: EnvironmentDiskSource, + @ApplicationContext context: Context, + ): KeyChainRepository = + KeyChainRepositoryImpl(environmentDiskSource = environmentDiskSource, context = context) + + @Provides + @Singleton + fun providesTlsHelper(keyChainRepository: KeyChainRepository): TLSHelper = + TLSHelper(keyChainRepository = keyChainRepository) + @Provides @Singleton fun provideRetrofits( @@ -78,6 +97,7 @@ object PlatformNetworkModule { headersInterceptor: HeadersInterceptor, refreshAuthenticator: RefreshAuthenticator, json: Json, + tlsHelper: TLSHelper, ): Retrofits = RetrofitsImpl( authTokenInterceptor = authTokenInterceptor, @@ -85,6 +105,7 @@ object PlatformNetworkModule { headersInterceptor = headersInterceptor, refreshAuthenticator = refreshAuthenticator, json = json, + tlsHelper = tlsHelper ) @Provides diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt index 7ee544457c6..268c015ccdd 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt @@ -7,6 +7,7 @@ import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.BaseUrlI import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.BaseUrlInterceptors import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.HeadersInterceptor import com.x8bit.bitwarden.data.platform.datasource.network.util.HEADER_KEY_AUTHORIZATION +import com.x8bit.bitwarden.data.platform.datasource.network.util.TLSHelper import kotlinx.serialization.json.Json import okhttp3.MediaType.Companion.toMediaType import okhttp3.OkHttpClient @@ -24,6 +25,7 @@ class RetrofitsImpl( headersInterceptor: HeadersInterceptor, refreshAuthenticator: RefreshAuthenticator, json: Json, + tlsHelper: TLSHelper ) : Retrofits { //region Authenticated Retrofits @@ -84,7 +86,7 @@ class RetrofitsImpl( } private val baseOkHttpClient: OkHttpClient = - OkHttpClient.Builder() + tlsHelper.setupOkHttpClientSSLSocketFactory(OkHttpClient.Builder()) .addInterceptor(headersInterceptor) .build() diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt new file mode 100644 index 00000000000..e3426ba45e4 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt @@ -0,0 +1,75 @@ +package com.x8bit.bitwarden.data.platform.datasource.network.util + +import com.x8bit.bitwarden.data.platform.repository.KeyChainRepository +import java.net.Socket +import java.security.KeyStore +import java.security.Principal +import java.security.PrivateKey +import java.security.cert.X509Certificate +import javax.inject.Inject +import javax.inject.Named +import javax.net.ssl.SSLContext +import javax.net.ssl.TrustManagerFactory +import javax.net.ssl.X509ExtendedKeyManager +import javax.net.ssl.X509TrustManager +import okhttp3.OkHttpClient + +class TLSHelper @Inject constructor( + @Named("keyChainRepository") private val keyChainRepository: KeyChainRepository, +) { + fun setupOkHttpClientSSLSocketFactory(builder: OkHttpClient.Builder): OkHttpClient.Builder { + val trustManagerFactory = + TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()) + trustManagerFactory.init(null as KeyStore?) + val trustManagers = trustManagerFactory.trustManagers + + val sslContext = SSLContext.getInstance("TLS") + sslContext.init(arrayOf(getMTLSKeyManagerForOKHTTP()), trustManagers, null) + + builder.sslSocketFactory(sslContext.socketFactory, trustManagers[0] as X509TrustManager) + + return builder + } + + private fun getMTLSKeyManagerForOKHTTP(): X509ExtendedKeyManager { + return object : X509ExtendedKeyManager() { + override fun getClientAliases( + p0: String?, + p1: Array?, + ): Array { + return emptyArray() + } + + override fun chooseClientAlias( + p0: Array?, + p1: Array?, + p2: Socket?, + ): String { + return "" + } + + override fun getServerAliases( + p0: String?, + p1: Array?, + ): Array { + return arrayOf() + } + + override fun chooseServerAlias( + p0: String?, + p1: Array?, + p2: Socket?, + ): String { + return "" + } + + override fun getCertificateChain(p0: String?): Array? { + return keyChainRepository.getCertificateChain() + } + + override fun getPrivateKey(p0: String?): PrivateKey? { + return keyChainRepository.getPrivateKey() + } + } + } +} diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt new file mode 100644 index 00000000000..1d2654d3e02 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt @@ -0,0 +1,7 @@ +package com.x8bit.bitwarden.data.platform.repository + +import android.security.KeyChainAliasCallback + +interface ChoosePrivateKeyAliasCallback { + fun getCallback(): KeyChainAliasCallback +} \ No newline at end of file diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt new file mode 100644 index 00000000000..8ceefc74982 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt @@ -0,0 +1,12 @@ +package com.x8bit.bitwarden.data.platform.repository + +import android.security.KeyChainAliasCallback + +class ChoosePrivateKeyAliasCallbackImpl constructor(val callback: (String?) -> Unit) : + ChoosePrivateKeyAliasCallback { + override fun getCallback(): KeyChainAliasCallback { + return KeyChainAliasCallback { + callback(it) + } + } +} \ No newline at end of file diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt new file mode 100644 index 00000000000..c01da4df3eb --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt @@ -0,0 +1,13 @@ +package com.x8bit.bitwarden.data.platform.repository + +import android.app.Activity +import java.security.PrivateKey +import java.security.cert.X509Certificate + +interface KeyChainRepository { + fun choosePrivateKeyAlias(activity: Activity, callback: ChoosePrivateKeyAliasCallback) + + fun getPrivateKey(): PrivateKey? + + fun getCertificateChain(): Array? +} \ No newline at end of file diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt new file mode 100644 index 00000000000..e578ddb1d4a --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt @@ -0,0 +1,58 @@ +package com.x8bit.bitwarden.data.platform.repository + +import android.app.Activity +import android.content.Context +import android.security.KeyChain +import com.x8bit.bitwarden.data.platform.datasource.disk.EnvironmentDiskSource +import com.x8bit.bitwarden.data.platform.repository.util.toEnvironmentUrlsOrDefault +import java.security.PrivateKey +import java.security.cert.X509Certificate +import javax.inject.Inject + +class KeyChainRepositoryImpl @Inject constructor( + environmentDiskSource: EnvironmentDiskSource, + val context: Context, +) : KeyChainRepository { + private var alias: String? = null + private var key: PrivateKey? = null + private var chain: Array? = null + + init { + alias = + environmentDiskSource.preAuthEnvironmentUrlData.toEnvironmentUrlsOrDefault().environmentUrlData.keyAlias + } + + override fun choosePrivateKeyAlias( + activity: Activity, + callback: ChoosePrivateKeyAliasCallback, + ) { + KeyChain.choosePrivateKeyAlias(activity, { a -> + callback.getCallback().alias(a) + alias = a + }, null, null, null, alias) + } + + override fun getPrivateKey(): PrivateKey? { + if (key == null && !alias.isNullOrEmpty()) { + key = try { + KeyChain.getPrivateKey(context, alias!!) + } catch (e: Exception) { + null + } + } + + return key + } + + override fun getCertificateChain(): Array? { + if (chain == null && !alias.isNullOrEmpty()) { + chain = try { + KeyChain.getCertificateChain(context, alias!!) + } catch (e: Exception) { + null + } + } + + return chain + } +} \ No newline at end of file diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt index 7df1ecfa3e7..d8a4ae9ceed 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt @@ -6,6 +6,7 @@ import androidx.navigation.NavHostController import androidx.navigation.NavOptions import androidx.navigation.navOptions import androidx.navigation.navigation +import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.ui.auth.feature.checkemail.checkEmailDestination import com.x8bit.bitwarden.ui.auth.feature.checkemail.navigateToCheckEmail import com.x8bit.bitwarden.ui.auth.feature.completeregistration.completeRegistrationDestination @@ -50,6 +51,7 @@ const val AUTH_GRAPH_ROUTE: String = "auth_graph" @Suppress("LongMethod") fun NavGraphBuilder.authGraph( navController: NavHostController, + choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit, ) { navigation( startDestination = LANDING_ROUTE, @@ -173,6 +175,7 @@ fun NavGraphBuilder.authGraph( ) environmentDestination( onNavigateBack = { navController.popBackStack() }, + choosePrivateKeyAlias = choosePrivateKeyAlias, ) masterPasswordHintDestination( onNavigateBack = { navController.popBackStack() }, diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt index abd0e27fa36..e3c6b6ef443 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt @@ -3,6 +3,7 @@ package com.x8bit.bitwarden.ui.auth.feature.environment import androidx.navigation.NavController import androidx.navigation.NavGraphBuilder import androidx.navigation.NavOptions +import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.ui.platform.base.util.composableWithSlideTransitions private const val ENVIRONMENT_ROUTE = "environment" @@ -12,11 +13,12 @@ private const val ENVIRONMENT_ROUTE = "environment" */ fun NavGraphBuilder.environmentDestination( onNavigateBack: () -> Unit, + choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit, ) { composableWithSlideTransitions( route = ENVIRONMENT_ROUTE, ) { - EnvironmentScreen(onNavigateBack = onNavigateBack) + EnvironmentScreen(onNavigateBack = onNavigateBack, choosePrivateKeyAlias = choosePrivateKeyAlias) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt index 7dad832e0a9..02c03ff60ab 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt @@ -28,6 +28,8 @@ import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.x8bit.bitwarden.BuildConfig import com.x8bit.bitwarden.R +import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback +import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallbackImpl import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect import com.x8bit.bitwarden.ui.platform.components.appbar.BitwardenTopAppBar import com.x8bit.bitwarden.ui.platform.components.button.BitwardenTextButton @@ -47,6 +49,7 @@ import kotlinx.collections.immutable.persistentListOf fun EnvironmentScreen( onNavigateBack: () -> Unit, viewModel: EnvironmentViewModel = hiltViewModel(), + choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit, ) { val state by viewModel.stateFlow.collectAsStateWithLifecycle() val context = LocalContext.current @@ -133,6 +136,37 @@ fun EnvironmentScreen( textFieldTestTag = "ServerUrlEntry", ) + Spacer(modifier = Modifier.height(16.dp)) + + BitwardenListHeaderText( + label = stringResource(id = R.string.client_certificate), + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = 16.dp), + ) + + BitwardenTextField( + label = stringResource(id = R.string.client_certificate), + value = state.keyAlias, + hint = stringResource(id = R.string.client_certificate_footer), + onValueChange = { + }, + readOnly = true, + modifier = Modifier + .fillMaxWidth() + .testTag("KeyAliasEntry") + .padding(horizontal = 16.dp), + ) + + BitwardenTextButton( + label = stringResource(id = R.string.choose_client_certificate), + onClick = { + choosePrivateKeyAlias(ChoosePrivateKeyAliasCallbackImpl { + viewModel.trySendAction(EnvironmentAction.KeyAliasChange(it.orEmpty())) + }) + } + ) + Spacer(modifier = Modifier.height(24.dp)) BitwardenListHeaderText( diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt index 4c93113f898..03faaa9b49f 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt @@ -6,6 +6,7 @@ import androidx.lifecycle.viewModelScope import com.x8bit.bitwarden.R import com.x8bit.bitwarden.data.auth.datasource.disk.model.EnvironmentUrlDataJson import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository +import com.x8bit.bitwarden.data.platform.repository.KeyChainRepository import com.x8bit.bitwarden.data.platform.repository.model.Environment import com.x8bit.bitwarden.ui.platform.base.BaseViewModel import com.x8bit.bitwarden.ui.platform.base.util.Text @@ -39,6 +40,7 @@ class EnvironmentViewModel @Inject constructor( } EnvironmentState( serverUrl = environmentUrlData.base, + keyAlias = environmentUrlData.keyAlias.orEmpty(), webVaultServerUrl = environmentUrlData.webVault.orEmpty(), apiServerUrl = environmentUrlData.api.orEmpty(), identityServerUrl = environmentUrlData.identity.orEmpty(), @@ -61,6 +63,7 @@ class EnvironmentViewModel @Inject constructor( is EnvironmentAction.SaveClick -> handleSaveClickAction() is EnvironmentAction.ErrorDialogDismiss -> handleErrorDialogDismiss() is EnvironmentAction.ServerUrlChange -> handleServerUrlChangeAction(action) + is EnvironmentAction.KeyAliasChange -> handleKeyAliasChangeAction(action) is EnvironmentAction.WebVaultServerUrlChange -> handleWebVaultServerUrlChangeAction(action) is EnvironmentAction.ApiServerUrlChange -> handleApiServerUrlChangeAction(action) is EnvironmentAction.IdentityServerUrlChange -> handleIdentityServerUrlChangeAction(action) @@ -91,6 +94,7 @@ class EnvironmentViewModel @Inject constructor( // Ensure all non-null/non-empty values have "http(s)://" prefixed. val updatedServerUrl = state.serverUrl.prefixHttpsIfNecessaryOrNull() ?: "" + val updatedKeyAlias = state.keyAlias val updatedWebVaultServerUrl = state.webVaultServerUrl.prefixHttpsIfNecessaryOrNull() val updatedApiServerUrl = state.apiServerUrl.prefixHttpsIfNecessaryOrNull() val updatedIdentityServerUrl = state.identityServerUrl.prefixHttpsIfNecessaryOrNull() @@ -99,6 +103,7 @@ class EnvironmentViewModel @Inject constructor( environmentRepository.environment = Environment.SelfHosted( environmentUrlData = EnvironmentUrlDataJson( base = updatedServerUrl, + keyAlias = updatedKeyAlias, api = updatedApiServerUrl, identity = updatedIdentityServerUrl, icon = updatedIconsServerUrl, @@ -122,6 +127,14 @@ class EnvironmentViewModel @Inject constructor( } } + private fun handleKeyAliasChangeAction( + action: EnvironmentAction.KeyAliasChange, + ) { + mutableStateFlow.update { + it.copy(keyAlias = action.keyAlias) + } + } + private fun handleWebVaultServerUrlChangeAction( action: EnvironmentAction.WebVaultServerUrlChange, ) { @@ -161,6 +174,7 @@ class EnvironmentViewModel @Inject constructor( @Parcelize data class EnvironmentState( val serverUrl: String, + val keyAlias: String, val webVaultServerUrl: String, val apiServerUrl: String, val identityServerUrl: String, @@ -211,6 +225,13 @@ sealed class EnvironmentAction { val serverUrl: String, ) : EnvironmentAction() + /** + * Indicates that the key alias has changed. + */ + data class KeyAliasChange( + val keyAlias: String, + ) : EnvironmentAction() + /** * Indicates that the web vault server URL has changed. */ diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt index bde02656cea..9ceec094d0f 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt @@ -15,6 +15,7 @@ import androidx.navigation.NavHostController import androidx.navigation.compose.NavHost import androidx.navigation.compose.rememberNavController import androidx.navigation.navOptions +import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.ui.auth.feature.accountsetup.SETUP_AUTO_FILL_AS_ROOT_ROUTE import com.x8bit.bitwarden.ui.auth.feature.accountsetup.SETUP_COMPLETE_ROUTE import com.x8bit.bitwarden.ui.auth.feature.accountsetup.SETUP_UNLOCK_AS_ROOT_ROUTE @@ -75,6 +76,7 @@ fun RootNavScreen( viewModel: RootNavViewModel = hiltViewModel(), navController: NavHostController = rememberNavController(), onSplashScreenRemoved: () -> Unit = {}, + choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit = { _ -> }, ) { val state by viewModel.stateFlow.collectAsStateWithLifecycle() val previousStateReference = remember { AtomicReference(state) } @@ -93,7 +95,7 @@ fun RootNavScreen( popExitTransition = { toExitTransition()(this) }, ) { splashDestination() - authGraph(navController) + authGraph(navController = navController, choosePrivateKeyAlias = choosePrivateKeyAlias) removePasswordDestination() resetPasswordDestination() trustedDeviceGraph(navController) diff --git a/app/src/main/res/values-de-rDE/strings.xml b/app/src/main/res/values-de-rDE/strings.xml index 2fc5ad57ab0..cd99463172f 100644 --- a/app/src/main/res/values-de-rDE/strings.xml +++ b/app/src/main/res/values-de-rDE/strings.xml @@ -262,6 +262,9 @@ Das Scannen erfolgt automatisch. Verschlüsselungscode-Migration erforderlich. Bitte melde dich über den Web-Tresor an, um deinen Verschlüsselungscode zu aktualisieren. Mehr erfahren API Server-URL + Clientzertifikat + Clientzertifikat, welches benutzt wird, um sich mit dem Server zu verbinden. Falls kein Clientzertifikat installiert ist, passiert beim Klicken auf „Auswählen“ nichts. + Auswählen Benutzerdefinierte Umgebung Für fortgeschrittene Benutzer. Du kannst die Basis-URL der jeweiligen Dienste unabhängig voneinander festlegen. Die URLs der Umgebung wurden gespeichert. diff --git a/app/src/main/res/values-fr-rFR/strings.xml b/app/src/main/res/values-fr-rFR/strings.xml index 0c876ea6e7e..f60d12f5e6a 100644 --- a/app/src/main/res/values-fr-rFR/strings.xml +++ b/app/src/main/res/values-fr-rFR/strings.xml @@ -262,6 +262,9 @@ La numérisation se fera automatiquement. Migration de la clé de chiffrement nécessaire. Veuillez vous connecter sur le coffre web pour mettre à jour votre clé de chiffrement. En savoir plus URL du serveur de l\'API + Certificat client + Certificat client qui sera envoyé au serveur. Si aucun certificat client n\'est installé, cliquer sur le bouton « Choisir » n\'aura aucun effet. + Choisir Environnement personnalisé Pour les utilisateurs avancés. Vous pouvez spécifier l\'URL de base de chaque service indépendamment. Les URLs d\'environnement ont été enregistrées. diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index f3b88d3b215..84ed1ff902d 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -262,6 +262,9 @@ Scanning will happen automatically. Encryption key migration required. Please login through the web vault to update your encryption key. Learn more API server URL + Client certificate + Client certificate that will be used for connections to the server. In case no user certificate is installed, clicking "Choose" will have no effect. + Choose Custom environment For advanced users. You can specify the base URL of each service independently. The environment URLs have been saved. diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt index fe9cbb4b077..53481defc7a 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt @@ -5,6 +5,7 @@ import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.AuthToke import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.BaseUrlInterceptors import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.HeadersInterceptor import com.x8bit.bitwarden.data.platform.datasource.network.model.NetworkResult +import com.x8bit.bitwarden.data.platform.datasource.network.util.TLSHelper import io.mockk.every import io.mockk.mockk import io.mockk.slot @@ -13,6 +14,7 @@ import kotlinx.serialization.json.Json import kotlinx.serialization.json.JsonObject import okhttp3.Authenticator import okhttp3.Interceptor +import okhttp3.OkHttpClient import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import org.junit.After @@ -47,6 +49,10 @@ class RetrofitsTest { } private val json = Json private val server = MockWebServer() + private val tlsHelper = mockk { + val builderSlot = slot() + every { setupOkHttpClientSSLSocketFactory(capture(builderSlot)) } answers { builderSlot.captured } + } private val retrofits = RetrofitsImpl( authTokenInterceptor = authTokenInterceptor, @@ -54,6 +60,7 @@ class RetrofitsTest { headersInterceptor = headersInterceptors, refreshAuthenticator = refreshAuthenticator, json = json, + tlsHelper = tlsHelper, ) private var isAuthInterceptorCalled = false diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt index 9e9ddd3c824..d755990cd59 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt @@ -9,8 +9,11 @@ import androidx.compose.ui.test.isDialog import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithContentDescription import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.onParent import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performTextInput +import androidx.compose.ui.test.requestFocus +import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow import com.x8bit.bitwarden.ui.platform.base.BaseComposeTest import io.mockk.every @@ -30,6 +33,9 @@ class EnvironmentScreenTest : BaseComposeTest() { every { eventFlow } returns mutableEventFlow every { stateFlow } returns mutableStateFlow } + private val choosePrivateKeyAlias = { callback: ChoosePrivateKeyAliasCallback -> + callback.getCallback().alias("alias") + } @Before fun setUp() { @@ -37,6 +43,7 @@ class EnvironmentScreenTest : BaseComposeTest() { EnvironmentScreen( onNavigateBack = { onNavigateBackCalled = true }, viewModel = viewModel, + choosePrivateKeyAlias = choosePrivateKeyAlias, ) } } @@ -140,6 +147,44 @@ class EnvironmentScreenTest : BaseComposeTest() { } } + @Test + fun `key alias should change according to the state`() { + composeTestRule + .onNodeWithText("Client certificate") + // Click to focus to see placeholder + .performClick() + .assertTextEquals( + "Client certificate", + "Client certificate that will be used for connections to the server", + "", + "", + ) + + mutableStateFlow.update { it.copy(keyAlias = "alias") } + + composeTestRule + .onNodeWithText("Client certificate") + .assertTextEquals( + "Client certificate", + "Client certificate that will be used for connections to the server", + "alias", + ) + } + + @Test + fun `key alias change should send KeyAliasChange`() { + composeTestRule + .onNodeWithText("Choose", useUnmergedTree = true) + .onParent() + .requestFocus() + .performClick() + verify { + viewModel.trySendAction( + EnvironmentAction.KeyAliasChange(keyAlias = "alias"), + ) + } + } + @Test fun `web vault URL should change according to the state`() { composeTestRule @@ -255,6 +300,7 @@ class EnvironmentScreenTest : BaseComposeTest() { companion object { val DEFAULT_STATE = EnvironmentState( serverUrl = "", + keyAlias = "", webVaultServerUrl = "", apiServerUrl = "", identityServerUrl = "", diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModelTest.kt index 8d7c3a3ed06..f018109e82a 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModelTest.kt @@ -173,6 +173,7 @@ class EnvironmentViewModelTest : BaseViewModelTest() { Environment.SelfHosted( environmentUrlData = EnvironmentUrlDataJson( base = "https://server-url", + keyAlias = "", api = "https://api-url", identity = "https://identity-url", icon = "https://icons-url", @@ -220,6 +221,7 @@ class EnvironmentViewModelTest : BaseViewModelTest() { Environment.SelfHosted( environmentUrlData = EnvironmentUrlDataJson( base = "", + keyAlias = "", api = null, identity = null, icon = null, @@ -308,6 +310,7 @@ class EnvironmentViewModelTest : BaseViewModelTest() { companion object { private val DEFAULT_STATE = EnvironmentState( serverUrl = "", + keyAlias = "", webVaultServerUrl = "", apiServerUrl = "", identityServerUrl = "", From 4bd2fef67f312b2009885c787433185c854c5db3 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 22 Jan 2025 13:54:05 -0500 Subject: [PATCH 2/5] Fix detekt issues These errors are preventing me from committing locally because detekt is run as a pre-commit hook. We use `detekt` to enforce code style. It can be run from command-line (`./gradlew detekt`) or from the Gradle window in Android Studio. --- .../network/di/PlatformNetworkModule.kt | 2 +- .../network/retrofit/RetrofitsImpl.kt | 2 +- .../datasource/network/util/TLSHelper.kt | 13 ++++++++++++- .../repository/ChoosePrivateKeyAliasCallback.kt | 8 +++++++- .../ChoosePrivateKeyAliasCallbackImpl.kt | 8 +++++++- .../platform/repository/KeyChainRepository.kt | 14 +++++++++++++- .../repository/KeyChainRepositoryImpl.kt | 17 ++++++++++++----- .../environment/EnvironmentNavigation.kt | 5 ++++- .../feature/environment/EnvironmentScreen.kt | 2 +- .../feature/environment/EnvironmentViewModel.kt | 1 - .../network/retrofit/RetrofitsTest.kt | 4 +++- 11 files changed, 61 insertions(+), 15 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt index 6b155219194..6ee9cb1ff7e 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt @@ -105,7 +105,7 @@ object PlatformNetworkModule { headersInterceptor = headersInterceptor, refreshAuthenticator = refreshAuthenticator, json = json, - tlsHelper = tlsHelper + tlsHelper = tlsHelper, ) @Provides diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt index 268c015ccdd..3e67e944561 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsImpl.kt @@ -25,7 +25,7 @@ class RetrofitsImpl( headersInterceptor: HeadersInterceptor, refreshAuthenticator: RefreshAuthenticator, json: Json, - tlsHelper: TLSHelper + tlsHelper: TLSHelper, ) : Retrofits { //region Authenticated Retrofits diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt index e3426ba45e4..fe9e9b764aa 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/util/TLSHelper.kt @@ -1,6 +1,7 @@ package com.x8bit.bitwarden.data.platform.datasource.network.util import com.x8bit.bitwarden.data.platform.repository.KeyChainRepository +import okhttp3.OkHttpClient import java.net.Socket import java.security.KeyStore import java.security.Principal @@ -12,11 +13,21 @@ import javax.net.ssl.SSLContext import javax.net.ssl.TrustManagerFactory import javax.net.ssl.X509ExtendedKeyManager import javax.net.ssl.X509TrustManager -import okhttp3.OkHttpClient +/** + * Helper class for setting up TLS (Transport Layer Security) for OkHttp client. + * It provides functionality to setup a SSL Socket factory using KeyChainRepository certificate and + * key. + * + * @param keyChainRepository repository for access to certificate and private key. + */ class TLSHelper @Inject constructor( @Named("keyChainRepository") private val keyChainRepository: KeyChainRepository, ) { + + /** + * Sets up a SSL Socket factory using KeyChainRepository certificate and key. + */ fun setupOkHttpClientSSLSocketFactory(builder: OkHttpClient.Builder): OkHttpClient.Builder { val trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt index 1d2654d3e02..37b541b5ba6 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt @@ -2,6 +2,12 @@ package com.x8bit.bitwarden.data.platform.repository import android.security.KeyChainAliasCallback +/** + * Callback for [KeyChainRepositoryImpl.choosePrivateKeyAlias]. + */ interface ChoosePrivateKeyAliasCallback { + /** + * Returns the callback. + */ fun getCallback(): KeyChainAliasCallback -} \ No newline at end of file +} diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt index 8ceefc74982..38292ec7223 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt @@ -2,11 +2,17 @@ package com.x8bit.bitwarden.data.platform.repository import android.security.KeyChainAliasCallback +/** + * Callback for [KeyChainRepositoryImpl.choosePrivateKeyAlias]. + */ class ChoosePrivateKeyAliasCallbackImpl constructor(val callback: (String?) -> Unit) : ChoosePrivateKeyAliasCallback { + /** + * Returns the callback. + */ override fun getCallback(): KeyChainAliasCallback { return KeyChainAliasCallback { callback(it) } } -} \ No newline at end of file +} diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt index c01da4df3eb..ff784edd7af 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt @@ -4,10 +4,22 @@ import android.app.Activity import java.security.PrivateKey import java.security.cert.X509Certificate +/** + * Repository for accessing the KeyChain. + */ interface KeyChainRepository { + /** + * Chooses a private key alias. + */ fun choosePrivateKeyAlias(activity: Activity, callback: ChoosePrivateKeyAliasCallback) + /** + * Returns the private key. + */ fun getPrivateKey(): PrivateKey? + /** + * Returns the certificate chain. + */ fun getCertificateChain(): Array? -} \ No newline at end of file +} diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt index e578ddb1d4a..95203b93e71 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt @@ -3,12 +3,16 @@ package com.x8bit.bitwarden.data.platform.repository import android.app.Activity import android.content.Context import android.security.KeyChain +import android.security.KeyChainException import com.x8bit.bitwarden.data.platform.datasource.disk.EnvironmentDiskSource import com.x8bit.bitwarden.data.platform.repository.util.toEnvironmentUrlsOrDefault import java.security.PrivateKey import java.security.cert.X509Certificate import javax.inject.Inject +/** + * Default implementation of [KeyChainRepository]. + */ class KeyChainRepositoryImpl @Inject constructor( environmentDiskSource: EnvironmentDiskSource, val context: Context, @@ -18,8 +22,11 @@ class KeyChainRepositoryImpl @Inject constructor( private var chain: Array? = null init { - alias = - environmentDiskSource.preAuthEnvironmentUrlData.toEnvironmentUrlsOrDefault().environmentUrlData.keyAlias + alias = environmentDiskSource + .preAuthEnvironmentUrlData + .toEnvironmentUrlsOrDefault() + .environmentUrlData + .keyAlias } override fun choosePrivateKeyAlias( @@ -36,7 +43,7 @@ class KeyChainRepositoryImpl @Inject constructor( if (key == null && !alias.isNullOrEmpty()) { key = try { KeyChain.getPrivateKey(context, alias!!) - } catch (e: Exception) { + } catch (_: KeyChainException) { null } } @@ -48,11 +55,11 @@ class KeyChainRepositoryImpl @Inject constructor( if (chain == null && !alias.isNullOrEmpty()) { chain = try { KeyChain.getCertificateChain(context, alias!!) - } catch (e: Exception) { + } catch (_: KeyChainException) { null } } return chain } -} \ No newline at end of file +} diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt index e3c6b6ef443..6cb908ef371 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt @@ -18,7 +18,10 @@ fun NavGraphBuilder.environmentDestination( composableWithSlideTransitions( route = ENVIRONMENT_ROUTE, ) { - EnvironmentScreen(onNavigateBack = onNavigateBack, choosePrivateKeyAlias = choosePrivateKeyAlias) + EnvironmentScreen( + onNavigateBack = onNavigateBack, + choosePrivateKeyAlias = choosePrivateKeyAlias, + ) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt index 02c03ff60ab..36c54e27e1a 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt @@ -164,7 +164,7 @@ fun EnvironmentScreen( choosePrivateKeyAlias(ChoosePrivateKeyAliasCallbackImpl { viewModel.trySendAction(EnvironmentAction.KeyAliasChange(it.orEmpty())) }) - } + }, ) Spacer(modifier = Modifier.height(24.dp)) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt index 03faaa9b49f..de575beb44e 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt @@ -6,7 +6,6 @@ import androidx.lifecycle.viewModelScope import com.x8bit.bitwarden.R import com.x8bit.bitwarden.data.auth.datasource.disk.model.EnvironmentUrlDataJson import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository -import com.x8bit.bitwarden.data.platform.repository.KeyChainRepository import com.x8bit.bitwarden.data.platform.repository.model.Environment import com.x8bit.bitwarden.ui.platform.base.BaseViewModel import com.x8bit.bitwarden.ui.platform.base.util.Text diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt index 53481defc7a..77797bba518 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/retrofit/RetrofitsTest.kt @@ -51,7 +51,9 @@ class RetrofitsTest { private val server = MockWebServer() private val tlsHelper = mockk { val builderSlot = slot() - every { setupOkHttpClientSSLSocketFactory(capture(builderSlot)) } answers { builderSlot.captured } + every { + setupOkHttpClientSSLSocketFactory(capture(builderSlot)) + } answers { builderSlot.captured } } private val retrofits = RetrofitsImpl( From d5a24180bb52090fb1077e2a2a784f3bac9b9630 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 22 Jan 2025 17:16:01 -0500 Subject: [PATCH 3/5] Migrate system certificate alias selection to KeyChainManager Because `KeyChain.choosePrivateKeyAlias` is reliant on `Activity` we extract the private key alias selection logic from `KeyChainRepository` to `KeyChainManager`. `KeyChainManager` is made available to Composables via `LocalManagerProvider` so that `Activity` context can be safely injected. `ChoosePrivateKeyAliasCallback` and its implementation are no longer needed and have been removed. --- .../java/com/x8bit/bitwarden/MainActivity.kt | 8 -- .../ChoosePrivateKeyAliasCallback.kt | 13 -- .../ChoosePrivateKeyAliasCallbackImpl.kt | 18 --- .../platform/repository/KeyChainRepository.kt | 5 - .../repository/KeyChainRepositoryImpl.kt | 11 -- .../ui/auth/feature/auth/AuthNavigation.kt | 3 - .../environment/EnvironmentNavigation.kt | 7 +- .../feature/environment/EnvironmentScreen.kt | 32 +++-- .../environment/EnvironmentViewModel.kt | 63 ++++++--- .../composition/LocalManagerProvider.kt | 7 + .../platform/feature/rootnav/RootNavScreen.kt | 4 +- .../manager/keychain/KeyChainManager.kt | 18 +++ .../manager/keychain/KeyChainManagerImpl.kt | 41 ++++++ .../model/PrivateKeyAliasSelectionResult.kt | 17 +++ app/src/main/res/values-de-rDE/strings.xml | 6 +- app/src/main/res/values-fr-rFR/strings.xml | 6 +- app/src/main/res/values/strings.xml | 8 +- .../environment/EnvironmentScreenTest.kt | 79 +++++++---- .../manager/keychain/KeyChainManagerTest.kt | 133 ++++++++++++++++++ 19 files changed, 345 insertions(+), 134 deletions(-) delete mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt delete mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt create mode 100644 app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManager.kt create mode 100644 app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerImpl.kt create mode 100644 app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/model/PrivateKeyAliasSelectionResult.kt create mode 100644 app/src/test/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerTest.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt b/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt index e6abd152627..175487965b2 100644 --- a/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt +++ b/app/src/main/java/com/x8bit/bitwarden/MainActivity.kt @@ -19,8 +19,6 @@ import com.x8bit.bitwarden.data.autofill.accessibility.manager.AccessibilityComp import com.x8bit.bitwarden.data.autofill.manager.AutofillActivityManager import com.x8bit.bitwarden.data.autofill.manager.AutofillCompletionManager import com.x8bit.bitwarden.data.platform.annotation.OmitFromCoverage -import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback -import com.x8bit.bitwarden.data.platform.repository.KeyChainRepository import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect import com.x8bit.bitwarden.ui.platform.composition.LocalManagerProvider @@ -55,9 +53,6 @@ class MainActivity : AppCompatActivity() { @Inject lateinit var debugLaunchManager: DebugMenuLaunchManager - @Inject - lateinit var keyChainRepository: KeyChainRepository - override fun onCreate(savedInstanceState: Bundle?) { var shouldShowSplashScreen = true installSplashScreen().setKeepOnScreenCondition { shouldShowSplashScreen } @@ -107,9 +102,6 @@ class MainActivity : AppCompatActivity() { RootNavScreen( onSplashScreenRemoved = { shouldShowSplashScreen = false }, navController = navController, - choosePrivateKeyAlias = { callback: ChoosePrivateKeyAliasCallback -> - keyChainRepository.choosePrivateKeyAlias(this@MainActivity, callback) - }, ) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt deleted file mode 100644 index 37b541b5ba6..00000000000 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallback.kt +++ /dev/null @@ -1,13 +0,0 @@ -package com.x8bit.bitwarden.data.platform.repository - -import android.security.KeyChainAliasCallback - -/** - * Callback for [KeyChainRepositoryImpl.choosePrivateKeyAlias]. - */ -interface ChoosePrivateKeyAliasCallback { - /** - * Returns the callback. - */ - fun getCallback(): KeyChainAliasCallback -} diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt deleted file mode 100644 index 38292ec7223..00000000000 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ChoosePrivateKeyAliasCallbackImpl.kt +++ /dev/null @@ -1,18 +0,0 @@ -package com.x8bit.bitwarden.data.platform.repository - -import android.security.KeyChainAliasCallback - -/** - * Callback for [KeyChainRepositoryImpl.choosePrivateKeyAlias]. - */ -class ChoosePrivateKeyAliasCallbackImpl constructor(val callback: (String?) -> Unit) : - ChoosePrivateKeyAliasCallback { - /** - * Returns the callback. - */ - override fun getCallback(): KeyChainAliasCallback { - return KeyChainAliasCallback { - callback(it) - } - } -} diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt index ff784edd7af..4fa6cd3267e 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepository.kt @@ -1,6 +1,5 @@ package com.x8bit.bitwarden.data.platform.repository -import android.app.Activity import java.security.PrivateKey import java.security.cert.X509Certificate @@ -8,10 +7,6 @@ import java.security.cert.X509Certificate * Repository for accessing the KeyChain. */ interface KeyChainRepository { - /** - * Chooses a private key alias. - */ - fun choosePrivateKeyAlias(activity: Activity, callback: ChoosePrivateKeyAliasCallback) /** * Returns the private key. diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt index 95203b93e71..e5b80c81919 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/KeyChainRepositoryImpl.kt @@ -1,6 +1,5 @@ package com.x8bit.bitwarden.data.platform.repository -import android.app.Activity import android.content.Context import android.security.KeyChain import android.security.KeyChainException @@ -29,16 +28,6 @@ class KeyChainRepositoryImpl @Inject constructor( .keyAlias } - override fun choosePrivateKeyAlias( - activity: Activity, - callback: ChoosePrivateKeyAliasCallback, - ) { - KeyChain.choosePrivateKeyAlias(activity, { a -> - callback.getCallback().alias(a) - alias = a - }, null, null, null, alias) - } - override fun getPrivateKey(): PrivateKey? { if (key == null && !alias.isNullOrEmpty()) { key = try { diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt index d8a4ae9ceed..7df1ecfa3e7 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/auth/AuthNavigation.kt @@ -6,7 +6,6 @@ import androidx.navigation.NavHostController import androidx.navigation.NavOptions import androidx.navigation.navOptions import androidx.navigation.navigation -import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.ui.auth.feature.checkemail.checkEmailDestination import com.x8bit.bitwarden.ui.auth.feature.checkemail.navigateToCheckEmail import com.x8bit.bitwarden.ui.auth.feature.completeregistration.completeRegistrationDestination @@ -51,7 +50,6 @@ const val AUTH_GRAPH_ROUTE: String = "auth_graph" @Suppress("LongMethod") fun NavGraphBuilder.authGraph( navController: NavHostController, - choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit, ) { navigation( startDestination = LANDING_ROUTE, @@ -175,7 +173,6 @@ fun NavGraphBuilder.authGraph( ) environmentDestination( onNavigateBack = { navController.popBackStack() }, - choosePrivateKeyAlias = choosePrivateKeyAlias, ) masterPasswordHintDestination( onNavigateBack = { navController.popBackStack() }, diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt index 6cb908ef371..abd0e27fa36 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentNavigation.kt @@ -3,7 +3,6 @@ package com.x8bit.bitwarden.ui.auth.feature.environment import androidx.navigation.NavController import androidx.navigation.NavGraphBuilder import androidx.navigation.NavOptions -import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.ui.platform.base.util.composableWithSlideTransitions private const val ENVIRONMENT_ROUTE = "environment" @@ -13,15 +12,11 @@ private const val ENVIRONMENT_ROUTE = "environment" */ fun NavGraphBuilder.environmentDestination( onNavigateBack: () -> Unit, - choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit, ) { composableWithSlideTransitions( route = ENVIRONMENT_ROUTE, ) { - EnvironmentScreen( - onNavigateBack = onNavigateBack, - choosePrivateKeyAlias = choosePrivateKeyAlias, - ) + EnvironmentScreen(onNavigateBack = onNavigateBack) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt index f15c99d93a6..3b662c0cf6f 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreen.kt @@ -28,8 +28,6 @@ import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.x8bit.bitwarden.BuildConfig import com.x8bit.bitwarden.R -import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback -import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallbackImpl import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect import com.x8bit.bitwarden.ui.platform.base.util.standardHorizontalMargin import com.x8bit.bitwarden.ui.platform.components.appbar.BitwardenTopAppBar @@ -40,6 +38,8 @@ import com.x8bit.bitwarden.ui.platform.components.header.BitwardenListHeaderText import com.x8bit.bitwarden.ui.platform.components.model.CardStyle import com.x8bit.bitwarden.ui.platform.components.scaffold.BitwardenScaffold import com.x8bit.bitwarden.ui.platform.components.util.rememberVectorPainter +import com.x8bit.bitwarden.ui.platform.composition.LocalKeyChainManager +import com.x8bit.bitwarden.ui.platform.manager.keychain.KeyChainManager import kotlinx.collections.immutable.persistentListOf /** @@ -50,8 +50,8 @@ import kotlinx.collections.immutable.persistentListOf @Composable fun EnvironmentScreen( onNavigateBack: () -> Unit, + keyChainManager: KeyChainManager = LocalKeyChainManager.current, viewModel: EnvironmentViewModel = hiltViewModel(), - choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit, ) { val state by viewModel.stateFlow.collectAsStateWithLifecycle() val context = LocalContext.current @@ -61,6 +61,14 @@ fun EnvironmentScreen( is EnvironmentEvent.ShowToast -> { Toast.makeText(context, event.message(context.resources), Toast.LENGTH_SHORT).show() } + + is EnvironmentEvent.ShowSystemCertificateSelectionDialog -> { + viewModel.trySendAction( + EnvironmentAction.SystemCertificateSelectionResultReceive( + result = keyChainManager.choosePrivateKeyAlias(state.serverUrl), + ), + ) + } } } @@ -144,18 +152,21 @@ fun EnvironmentScreen( Spacer(modifier = Modifier.height(16.dp)) BitwardenListHeaderText( - label = stringResource(id = R.string.client_certificate), + label = stringResource(id = R.string.client_certificate_mtls), modifier = Modifier .fillMaxWidth() .padding(horizontal = 16.dp), ) BitwardenTextField( - label = stringResource(id = R.string.client_certificate), + label = stringResource(id = R.string.certificate_alias), value = state.keyAlias, - supportingText = stringResource(id = R.string.client_certificate_footer), + supportingText = stringResource( + id = R.string.certificate_used_for_client_authentication, + ), onValueChange = {}, readOnly = true, + cardStyle = CardStyle.Full, modifier = Modifier .fillMaxWidth() .testTag("KeyAliasEntry") @@ -163,12 +174,11 @@ fun EnvironmentScreen( ) BitwardenTextButton( - label = stringResource(id = R.string.choose_client_certificate), - onClick = { - choosePrivateKeyAlias(ChoosePrivateKeyAliasCallbackImpl { - viewModel.trySendAction(EnvironmentAction.KeyAliasChange(it.orEmpty())) - }) + label = stringResource(id = R.string.use_system_certificate), + onClick = remember(viewModel) { + { viewModel.trySendAction(EnvironmentAction.UseSystemCertificateClick) } }, + modifier = Modifier.standardHorizontalMargin(), ) Spacer(modifier = Modifier.height(height = 16.dp)) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt index de575beb44e..ee0147b3ac4 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentViewModel.kt @@ -12,6 +12,7 @@ import com.x8bit.bitwarden.ui.platform.base.util.Text import com.x8bit.bitwarden.ui.platform.base.util.asText import com.x8bit.bitwarden.ui.platform.base.util.isValidUri import com.x8bit.bitwarden.ui.platform.base.util.orNullIfBlank +import com.x8bit.bitwarden.ui.platform.manager.keychain.model.PrivateKeyAliasSelectionResult import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach @@ -24,6 +25,7 @@ private const val KEY_STATE = "state" /** * View model for the self-hosted/custom environment screen. */ +@Suppress("TooManyFunctions") @HiltViewModel class EnvironmentViewModel @Inject constructor( private val environmentRepository: EnvironmentRepository, @@ -62,11 +64,14 @@ class EnvironmentViewModel @Inject constructor( is EnvironmentAction.SaveClick -> handleSaveClickAction() is EnvironmentAction.ErrorDialogDismiss -> handleErrorDialogDismiss() is EnvironmentAction.ServerUrlChange -> handleServerUrlChangeAction(action) - is EnvironmentAction.KeyAliasChange -> handleKeyAliasChangeAction(action) is EnvironmentAction.WebVaultServerUrlChange -> handleWebVaultServerUrlChangeAction(action) is EnvironmentAction.ApiServerUrlChange -> handleApiServerUrlChangeAction(action) is EnvironmentAction.IdentityServerUrlChange -> handleIdentityServerUrlChangeAction(action) is EnvironmentAction.IconsServerUrlChange -> handleIconsServerUrlChangeAction(action) + is EnvironmentAction.UseSystemCertificateClick -> handleUseSystemCertificateClickAction() + is EnvironmentAction.SystemCertificateSelectionResultReceive -> { + handleSystemCertificateSelectionResultReceive(action) + } } private fun handleCloseClickAction() { @@ -126,14 +131,6 @@ class EnvironmentViewModel @Inject constructor( } } - private fun handleKeyAliasChangeAction( - action: EnvironmentAction.KeyAliasChange, - ) { - mutableStateFlow.update { - it.copy(keyAlias = action.keyAlias) - } - } - private fun handleWebVaultServerUrlChangeAction( action: EnvironmentAction.WebVaultServerUrlChange, ) { @@ -165,6 +162,30 @@ class EnvironmentViewModel @Inject constructor( it.copy(iconsServerUrl = action.iconsServerUrl) } } + + private fun handleUseSystemCertificateClickAction() { + sendEvent(EnvironmentEvent.ShowSystemCertificateSelectionDialog) + } + + private fun handleSystemCertificateSelectionResultReceive( + action: EnvironmentAction.SystemCertificateSelectionResultReceive, + ) { + when (val result = action.result) { + is PrivateKeyAliasSelectionResult.Success -> { + mutableStateFlow.update { + it.copy(keyAlias = result.alias.orEmpty()) + } + } + + is PrivateKeyAliasSelectionResult.Error -> { + sendEvent( + EnvironmentEvent.ShowToast( + message = R.string.error_loading_certificate.asText(), + ), + ) + } + } + } } /** @@ -190,6 +211,11 @@ sealed class EnvironmentEvent { */ data object NavigateBack : EnvironmentEvent() + /** + * Show the system certificate selection dialog. + */ + data object ShowSystemCertificateSelectionDialog : EnvironmentEvent() + /** * Show a toast with the given message. */ @@ -218,17 +244,15 @@ sealed class EnvironmentAction { data object ErrorDialogDismiss : EnvironmentAction() /** - * Indicates that the overall server URL has changed. + * User clicked the use system certificate button. */ - data class ServerUrlChange( - val serverUrl: String, - ) : EnvironmentAction() + data object UseSystemCertificateClick : EnvironmentAction() /** - * Indicates that the key alias has changed. + * Indicates that the overall server URL has changed. */ - data class KeyAliasChange( - val keyAlias: String, + data class ServerUrlChange( + val serverUrl: String, ) : EnvironmentAction() /** @@ -258,6 +282,13 @@ sealed class EnvironmentAction { data class IconsServerUrlChange( val iconsServerUrl: String, ) : EnvironmentAction() + + /** + * Indicates the result of system certificate selection has been received. + */ + data class SystemCertificateSelectionResultReceive( + val result: PrivateKeyAliasSelectionResult, + ) : EnvironmentAction() } /** diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt index 0c7edbf0e8d..df63f899561 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt @@ -20,6 +20,8 @@ import com.x8bit.bitwarden.ui.platform.manager.exit.ExitManager import com.x8bit.bitwarden.ui.platform.manager.exit.ExitManagerImpl import com.x8bit.bitwarden.ui.platform.manager.intent.IntentManager import com.x8bit.bitwarden.ui.platform.manager.intent.IntentManagerImpl +import com.x8bit.bitwarden.ui.platform.manager.keychain.KeyChainManager +import com.x8bit.bitwarden.ui.platform.manager.keychain.KeyChainManagerImpl import com.x8bit.bitwarden.ui.platform.manager.nfc.NfcManager import com.x8bit.bitwarden.ui.platform.manager.nfc.NfcManagerImpl import com.x8bit.bitwarden.ui.platform.manager.permissions.PermissionsManager @@ -49,6 +51,7 @@ fun LocalManagerProvider( LocalNfcManager provides NfcManagerImpl(activity), LocalFido2CompletionManager provides fido2CompletionManager, LocalAppReviewManager provides AppReviewManagerImpl(activity), + LocalKeyChainManager provides KeyChainManagerImpl(activity), ) { content() } @@ -103,3 +106,7 @@ val LocalFido2CompletionManager: ProvidableCompositionLocal = compositionLocalOf { error("CompositionLocal AppReviewManager not present") } + +val LocalKeyChainManager: ProvidableCompositionLocal = compositionLocalOf { + error("CompositionLocal KeyChainManager not present") +} diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt index 36b8225a8f1..9a1c8706d63 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt @@ -14,7 +14,6 @@ import androidx.navigation.NavHostController import androidx.navigation.compose.NavHost import androidx.navigation.compose.rememberNavController import androidx.navigation.navOptions -import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.ui.auth.feature.accountsetup.SETUP_AUTO_FILL_AS_ROOT_ROUTE import com.x8bit.bitwarden.ui.auth.feature.accountsetup.SETUP_COMPLETE_ROUTE import com.x8bit.bitwarden.ui.auth.feature.accountsetup.SETUP_UNLOCK_AS_ROOT_ROUTE @@ -75,7 +74,6 @@ fun RootNavScreen( viewModel: RootNavViewModel = hiltViewModel(), navController: NavHostController = rememberNavController(), onSplashScreenRemoved: () -> Unit = {}, - choosePrivateKeyAlias: (ChoosePrivateKeyAliasCallback) -> Unit = { _ -> }, ) { val state by viewModel.stateFlow.collectAsStateWithLifecycle() val previousStateReference = remember { AtomicReference(state) } @@ -94,7 +92,7 @@ fun RootNavScreen( popExitTransition = { toExitTransition()(this) }, ) { splashDestination() - authGraph(navController = navController, choosePrivateKeyAlias = choosePrivateKeyAlias) + authGraph(navController = navController) removePasswordDestination() resetPasswordDestination() trustedDeviceGraph(navController) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManager.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManager.kt new file mode 100644 index 00000000000..099e9a89a56 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManager.kt @@ -0,0 +1,18 @@ +package com.x8bit.bitwarden.ui.platform.manager.keychain + +import com.x8bit.bitwarden.ui.platform.manager.keychain.model.PrivateKeyAliasSelectionResult + +/** + * Responsible for managing keys stored in the system KeyChain. + */ +interface KeyChainManager { + + /** + * Display the system private key alias selection dialog. + * + * @param currentServerUrl The currently selected server URL. + */ + suspend fun choosePrivateKeyAlias( + currentServerUrl: String?, + ): PrivateKeyAliasSelectionResult +} diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerImpl.kt new file mode 100644 index 00000000000..7bdd61fc940 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerImpl.kt @@ -0,0 +1,41 @@ +package com.x8bit.bitwarden.ui.platform.manager.keychain + +import android.app.Activity +import android.security.KeyChain +import androidx.core.net.toUri +import com.x8bit.bitwarden.ui.platform.manager.keychain.model.PrivateKeyAliasSelectionResult +import kotlinx.coroutines.channels.awaitClose +import kotlinx.coroutines.flow.callbackFlow +import kotlinx.coroutines.flow.first + +/** + * Default implementation of [KeyChainManager]. + */ +class KeyChainManagerImpl( + private val activity: Activity, +) : KeyChainManager { + + override suspend fun choosePrivateKeyAlias( + currentServerUrl: String?, + ) = callbackFlow { + try { + KeyChain.choosePrivateKeyAlias( + activity, + { alias -> + trySend(PrivateKeyAliasSelectionResult.Success(alias)) + close() + }, + null, + null, + currentServerUrl?.toUri(), + null, + ) + } catch (_: IllegalArgumentException) { + trySend(PrivateKeyAliasSelectionResult.Error) + close() + } + + awaitClose() + } + .first() +} diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/model/PrivateKeyAliasSelectionResult.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/model/PrivateKeyAliasSelectionResult.kt new file mode 100644 index 00000000000..7cf93b5034c --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/keychain/model/PrivateKeyAliasSelectionResult.kt @@ -0,0 +1,17 @@ +package com.x8bit.bitwarden.ui.platform.manager.keychain.model + +/** + * Represents the result of an operation to select a private key alias from the system KeyChain. + */ +sealed class PrivateKeyAliasSelectionResult { + /** + * Indicates that the operation was successful and an alias was selected (or null if none was + * selected). + */ + data class Success(val alias: String?) : PrivateKeyAliasSelectionResult() + + /** + * Indicates that an error occurred during the operation. + */ + object Error : PrivateKeyAliasSelectionResult() +} diff --git a/app/src/main/res/values-de-rDE/strings.xml b/app/src/main/res/values-de-rDE/strings.xml index 02bed934dda..2291ae6da44 100644 --- a/app/src/main/res/values-de-rDE/strings.xml +++ b/app/src/main/res/values-de-rDE/strings.xml @@ -262,9 +262,9 @@ Das Scannen erfolgt automatisch. Verschlüsselungscode-Migration erforderlich. Bitte melde dich über den Web-Tresor an, um deinen Verschlüsselungscode zu aktualisieren. Mehr erfahren API Server-URL - Clientzertifikat - Clientzertifikat, welches benutzt wird, um sich mit dem Server zu verbinden. Falls kein Clientzertifikat installiert ist, passiert beim Klicken auf „Auswählen“ nichts. - Auswählen + Clientzertifikat + Clientzertifikat, welches benutzt wird, um sich mit dem Server zu verbinden. Falls kein Clientzertifikat installiert ist, passiert beim Klicken auf „Auswählen“ nichts. + Auswählen Benutzerdefinierte Umgebung Für fortgeschrittene Benutzer. Du kannst die Basis-URL der jeweiligen Dienste unabhängig voneinander festlegen. Die URLs der Umgebung wurden gespeichert. diff --git a/app/src/main/res/values-fr-rFR/strings.xml b/app/src/main/res/values-fr-rFR/strings.xml index 200d91cf67a..635223df580 100644 --- a/app/src/main/res/values-fr-rFR/strings.xml +++ b/app/src/main/res/values-fr-rFR/strings.xml @@ -262,9 +262,9 @@ La numérisation se fera automatiquement. Migration de la clé de chiffrement nécessaire. Veuillez vous connecter sur le coffre web pour mettre à jour votre clé de chiffrement. En savoir plus URL du serveur de l\'API - Certificat client - Certificat client qui sera envoyé au serveur. Si aucun certificat client n\'est installé, cliquer sur le bouton « Choisir » n\'aura aucun effet. - Choisir + Certificat client + Certificat client qui sera envoyé au serveur. Si aucun certificat client n\'est installé, cliquer sur le bouton « Choisir » n\'aura aucun effet. + Choisir Environnement personnalisé Pour les utilisateurs avancés. Vous pouvez spécifier l\'URL de base de chaque service indépendamment. Les URLs d\'environnement ont été enregistrées. diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index ff2aba9d1ba..2fa62abc131 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -262,9 +262,9 @@ Scanning will happen automatically. Encryption key migration required. Please login through the web vault to update your encryption key. Learn more API server URL - Client certificate - Client certificate that will be used for connections to the server. In case no user certificate is installed, clicking "Choose" will have no effect. - Choose + Client certificate (mTLS) + Certificate used for client authentication. + Use system certificate Custom environment For advanced users. You can specify the base URL of each service independently. The environment URLs have been saved. @@ -1124,4 +1124,6 @@ Do you want to switch to this account? 3 of 3 You must add a web address to use autofill to access this account. Mutual TLS + Error loading certificate + Certificate alias diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt index bdb4105fab2..bba0a963c62 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/environment/EnvironmentScreenTest.kt @@ -9,13 +9,15 @@ import androidx.compose.ui.test.isDialog import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithContentDescription import androidx.compose.ui.test.onNodeWithText -import androidx.compose.ui.test.onParent import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.performScrollTo import androidx.compose.ui.test.performTextInput -import androidx.compose.ui.test.requestFocus -import com.x8bit.bitwarden.data.platform.repository.ChoosePrivateKeyAliasCallback import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow import com.x8bit.bitwarden.ui.platform.base.BaseComposeTest +import com.x8bit.bitwarden.ui.platform.manager.keychain.KeyChainManager +import com.x8bit.bitwarden.ui.platform.manager.keychain.model.PrivateKeyAliasSelectionResult +import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.verify @@ -29,21 +31,23 @@ class EnvironmentScreenTest : BaseComposeTest() { private var onNavigateBackCalled = false private val mutableEventFlow = bufferedMutableSharedFlow() private val mutableStateFlow = MutableStateFlow(DEFAULT_STATE) + private val mockKeyChainManager = mockk { + coEvery { + choosePrivateKeyAlias(any()) + } returns PrivateKeyAliasSelectionResult.Success("mockAlias") + } private val viewModel = mockk(relaxed = true) { every { eventFlow } returns mutableEventFlow every { stateFlow } returns mutableStateFlow } - private val choosePrivateKeyAlias = { callback: ChoosePrivateKeyAliasCallback -> - callback.getCallback().alias("alias") - } @Before fun setUp() { composeTestRule.setContent { EnvironmentScreen( onNavigateBack = { onNavigateBackCalled = true }, + keyChainManager = mockKeyChainManager, viewModel = viewModel, - choosePrivateKeyAlias = choosePrivateKeyAlias, ) } } @@ -139,43 +143,56 @@ class EnvironmentScreenTest : BaseComposeTest() { } @Test - fun `key alias should change according to the state`() { + fun `use system certificate click should send UseSystemKeyCertificateClick`() { composeTestRule - .onNodeWithText("Client certificate") - // Click to focus to see placeholder + .onNodeWithText("Use system certificate") + .performScrollTo() + .assertIsDisplayed() .performClick() - .assertTextEquals( - "Client certificate", - "Client certificate that will be used for connections to the server", - "", - "", - ) - mutableStateFlow.update { it.copy(keyAlias = "alias") } + verify { + viewModel.trySendAction(EnvironmentAction.UseSystemCertificateClick) + } + } - composeTestRule - .onNodeWithText("Client certificate") - .assertTextEquals( - "Client certificate", - "Client certificate that will be used for connections to the server", - "alias", - ) + @Test + fun `ShowSystemCertificateSelection event should show system certificate selection dialog`() { + mutableEventFlow.tryEmit(EnvironmentEvent.ShowSystemCertificateSelectionDialog) + coVerify { mockKeyChainManager.choosePrivateKeyAlias("") } } + @Suppress("MaxLineLength") @Test - fun `key alias change should send KeyAliasChange`() { - composeTestRule - .onNodeWithText("Choose", useUnmergedTree = true) - .onParent() - .requestFocus() - .performClick() + fun `system certificate selection should send SystemCertificateSelectionResultReceive action`() { + coEvery { + mockKeyChainManager.choosePrivateKeyAlias("") + } returns PrivateKeyAliasSelectionResult.Success("alias") + + mutableEventFlow.tryEmit(EnvironmentEvent.ShowSystemCertificateSelectionDialog) + verify { viewModel.trySendAction( - EnvironmentAction.KeyAliasChange(keyAlias = "alias"), + EnvironmentAction.SystemCertificateSelectionResultReceive( + result = PrivateKeyAliasSelectionResult.Success("alias"), + ), ) } } + @Suppress("MaxLineLength") + @Test + fun `key alias should change according to the state`() { + composeTestRule + .onNodeWithText("Certificate alias") + .assertTextEquals("Certificate alias", "") + + mutableStateFlow.update { it.copy(keyAlias = "mock-alias") } + + composeTestRule + .onNodeWithText("Certificate alias") + .assertTextEquals("Certificate alias", "mock-alias") + } + @Test fun `web vault URL should change according to the state`() { composeTestRule diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerTest.kt new file mode 100644 index 00000000000..7e52bb3ee91 --- /dev/null +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/manager/keychain/KeyChainManagerTest.kt @@ -0,0 +1,133 @@ +package com.x8bit.bitwarden.ui.platform.manager.keychain + +import android.app.Activity +import android.net.Uri +import android.security.KeyChain +import android.security.KeyChainAliasCallback +import androidx.core.net.toUri +import com.x8bit.bitwarden.ui.platform.manager.keychain.model.PrivateKeyAliasSelectionResult +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.slot +import io.mockk.unmockkStatic +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertInstanceOf + +class KeyChainManagerTest { + + private val mockActivity = mockk() + private val keyChainManager = KeyChainManagerImpl(activity = mockActivity) + + @BeforeEach + fun setUp() { + mockkStatic(KeyChain::class) + } + + @AfterEach + fun tearDown() { + unmockkStatic(KeyChain::class) + } + + @Test + fun `choosePrivateKeyAlias should return Success with alias when key is selected`() = runTest { + setupMockUri() + val systemCallbackCaptor = slot() + every { + KeyChain.choosePrivateKeyAlias( + /* activity = */ mockActivity, + /* response = */ capture(systemCallbackCaptor), + /* keyTypes = */ null, + /* issuers = */ null, + /* uri = */ null, + /* alias = */ null, + ) + } answers { + systemCallbackCaptor.captured.alias("mockAlias") + } + + val result = keyChainManager.choosePrivateKeyAlias(currentServerUrl = null) + + assertInstanceOf(result) + .also { assertEquals("mockAlias", it.alias) } + } + + @Test + fun `choosePrivateKeyAlias should return Error when IllegalArgumentException is thrown`() = + runTest { + setupMockUri() + every { + KeyChain.choosePrivateKeyAlias( + /* activity = */ mockActivity, + /* response = */ any(), + /* keyTypes = */ null, + /* issuers = */ null, + /* uri = */ null, + /* alias = */ null, + ) + } throws IllegalArgumentException() + + val result = keyChainManager.choosePrivateKeyAlias(currentServerUrl = null) + + assertInstanceOf(result) + } + + @Test + fun `choosePrivateKeyAlias should pass currentServerUrl to system KeyChain`() = runTest { + setupMockUri() + val systemCallbackCaptor = slot() + every { + KeyChain.choosePrivateKeyAlias( + /* activity = */ mockActivity, + /* response = */ capture(systemCallbackCaptor), + /* keyTypes = */ null, + /* issuers = */ null, + /* uri = */ "www.mockuri.com".toUri(), + /* alias = */ null, + ) + } answers { + systemCallbackCaptor.captured.alias("mockAlias") + } + + val result = keyChainManager.choosePrivateKeyAlias(currentServerUrl = "www.mockuri.com") + + assertInstanceOf(result) + .also { assertEquals("mockAlias", it.alias) } + } + + @Test + fun `choosePrivateKeyAlias should return Success with null alias when no key is selected`() = + runTest { + setupMockUri() + val systemCallbackCaptor = slot() + every { + KeyChain.choosePrivateKeyAlias( + /* activity = */ mockActivity, + /* response = */ capture(systemCallbackCaptor), + /* keyTypes = */ null, + /* issuers = */ null, + /* uri = */ null, + /* alias = */ null, + ) + } answers { + systemCallbackCaptor.captured.alias(null) + } + + val result = keyChainManager.choosePrivateKeyAlias(currentServerUrl = null) + + assertInstanceOf(result) + .also { assertNull(it.alias) } + } + + private fun setupMockUri() { + mockkStatic(Uri::class) + val uriMock = mockk() + every { Uri.parse(any()) } returns uriMock + every { uriMock.host } returns "www.mockuri.com" + } +} From 968289134bfec8b6f6f794b632baaf7a4e26e16e Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 22 Jan 2025 17:18:54 -0500 Subject: [PATCH 4/5] Remove manually added translations All translations are managed through Crowdin. The configuration we uses will overwrite any translations manually added to the source files. Translations can be added after the changes are merged into our default branch (`main`). --- app/src/main/res/values-de-rDE/strings.xml | 3 --- app/src/main/res/values-fr-rFR/strings.xml | 3 --- 2 files changed, 6 deletions(-) diff --git a/app/src/main/res/values-de-rDE/strings.xml b/app/src/main/res/values-de-rDE/strings.xml index 2291ae6da44..b8c97cd5e97 100644 --- a/app/src/main/res/values-de-rDE/strings.xml +++ b/app/src/main/res/values-de-rDE/strings.xml @@ -262,9 +262,6 @@ Das Scannen erfolgt automatisch. Verschlüsselungscode-Migration erforderlich. Bitte melde dich über den Web-Tresor an, um deinen Verschlüsselungscode zu aktualisieren. Mehr erfahren API Server-URL - Clientzertifikat - Clientzertifikat, welches benutzt wird, um sich mit dem Server zu verbinden. Falls kein Clientzertifikat installiert ist, passiert beim Klicken auf „Auswählen“ nichts. - Auswählen Benutzerdefinierte Umgebung Für fortgeschrittene Benutzer. Du kannst die Basis-URL der jeweiligen Dienste unabhängig voneinander festlegen. Die URLs der Umgebung wurden gespeichert. diff --git a/app/src/main/res/values-fr-rFR/strings.xml b/app/src/main/res/values-fr-rFR/strings.xml index 635223df580..82c2d3c5727 100644 --- a/app/src/main/res/values-fr-rFR/strings.xml +++ b/app/src/main/res/values-fr-rFR/strings.xml @@ -262,9 +262,6 @@ La numérisation se fera automatiquement. Migration de la clé de chiffrement nécessaire. Veuillez vous connecter sur le coffre web pour mettre à jour votre clé de chiffrement. En savoir plus URL du serveur de l\'API - Certificat client - Certificat client qui sera envoyé au serveur. Si aucun certificat client n\'est installé, cliquer sur le bouton « Choisir » n\'aura aucun effet. - Choisir Environnement personnalisé Pour les utilisateurs avancés. Vous pouvez spécifier l\'URL de base de chaque service indépendamment. Les URLs d\'environnement ont été enregistrées. From 30f167ae503313b2838591bec59439f263e87db7 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Wed, 22 Jan 2025 18:00:47 -0500 Subject: [PATCH 5/5] Revert unnecessary change to RootNavScreen.kt --- .../bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt index 9a1c8706d63..a6bcb21b447 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavScreen.kt @@ -92,7 +92,7 @@ fun RootNavScreen( popExitTransition = { toExitTransition()(this) }, ) { splashDestination() - authGraph(navController = navController) + authGraph(navController) removePasswordDestination() resetPasswordDestination() trustedDeviceGraph(navController)