From 92e1ff61556439745f348440973a5c1fe1f3169f Mon Sep 17 00:00:00 2001 From: Vladimir Krivosheev Date: Tue, 2 Oct 2018 20:08:32 +0200 Subject: [PATCH] prepare to fix IDEA-165257 KeePass OVERWRITES existing database instead of appending or prompting --- .../src/PasswordSafeConfigurable.kt | 93 +++++++++---------- .../credential-store/src/PasswordSafeImpl.kt | 10 +- 2 files changed, 51 insertions(+), 52 deletions(-) diff --git a/platform/credential-store/src/PasswordSafeConfigurable.kt b/platform/credential-store/src/PasswordSafeConfigurable.kt index f0f94797d37f..b25cadd80692 100644 --- a/platform/credential-store/src/PasswordSafeConfigurable.kt +++ b/platform/credential-store/src/PasswordSafeConfigurable.kt @@ -15,11 +15,11 @@ import com.intellij.openapi.options.ConfigurationException import com.intellij.openapi.project.DumbAwareAction import com.intellij.openapi.ui.MessageDialogBuilder import com.intellij.openapi.util.SystemInfo -import com.intellij.openapi.vfs.VirtualFile import com.intellij.ui.TextFieldWithHistoryWithBrowseButton import com.intellij.ui.components.RadioButton import com.intellij.ui.layout.* import com.intellij.util.io.exists +import com.intellij.util.io.isDirectory import com.intellij.util.text.nullize import gnu.trove.THashMap import java.io.File @@ -84,41 +84,39 @@ internal class PasswordSafeConfigurableUi : ConfigurableUi passwordSafe.currentProvider = createPersistentCredentialStore()!! } - ProviderType.KEEPASS -> { - runAndHandleIncorrectMasterPasswordException { - if (!changeExistingKeepassStoreIfPossible(settings, passwordSafe, isMemoryOnly = false)) { - passwordSafe.currentProvider = createKeePassCredentialStoreUsingNewOptions() - } - } - } - + ProviderType.KEEPASS -> createAndSaveKeePassDatabaseWithNewOptions(settings) else -> throw IllegalStateException("Unknown provider type: $providerType") } } else if (isKeepassFileLocationChanged(settings)) { - val newDbFile = getNewDbFile() - if (newDbFile != null) { - val currentProviderIfComputed = passwordSafe.currentProviderIfComputed as? KeePassCredentialStore - if (currentProviderIfComputed == null) { - runAndHandleIncorrectMasterPasswordException { - passwordSafe.currentProvider = createKeePassCredentialStoreUsingNewOptions() - } - } - else { - currentProviderIfComputed.dbFile = newDbFile - } - settings.keepassDb = newDbFile.toString() - } + createAndSaveKeePassDatabaseWithNewOptions(settings) } settings.providerType = providerType } - private fun createKeePassCredentialStoreUsingNewOptions(): KeePassCredentialStore { - return KeePassCredentialStore(dbFile = getNewDbFileOrError(), masterKeyFile = getDefaultMasterPasswordFile()) - } + private fun createAndSaveKeePassDatabaseWithNewOptions(settings: PasswordSafeSettings) { + // todo should we respect existing in-memory KeePass database or not + closeCurrentStoreIfKeePass() - private fun getNewDbFileOrError() = getNewDbFile() ?: throw ConfigurationException("KeePass database path is empty") + val newDbFile = getNewDbFile() ?: throw ConfigurationException("KeePass database path is empty.") + if (newDbFile.isDirectory()) { + // we do not normalize as we do on file choose because if user decoded to type path manually, + // it should be valid path and better to avoid any magic here + throw ConfigurationException("KeePass database file is directory.") + } + if (!newDbFile.fileName.toString().endsWith(".kdbx")) { + throw ConfigurationException("KeePass database file should ends with \".kdbx\".") + } + + settings.keepassDb = newDbFile.toString() + try { + KeePassCredentialStore(dbFile = newDbFile, masterKeyFile = getDefaultMasterPasswordFile()).save() + } + catch (e: IncorrectMasterPasswordException) { + throw ConfigurationException("Master password for KeePass database is not correct (\"Clear\" can be used to reset database).") + } + } private fun changeExistingKeepassStoreIfPossible(settings: PasswordSafeSettings, passwordSafe: PasswordSafeImpl, isMemoryOnly: Boolean): Boolean { if (settings.providerType != ProviderType.MEMORY_ONLY || settings.providerType != ProviderType.KEEPASS) { @@ -164,17 +162,20 @@ internal class PasswordSafeConfigurableUi : ConfigurableUi inKeePass() row("Database:") { val fileChooserDescriptor = FileChooserDescriptorFactory.createSingleLocalFileDescriptor().withFileFilter { - it.name.endsWith(".kdbx") + it.isDirectory || it.name.endsWith(".kdbx") } keePassDbFile = textFieldWithBrowseButton("KeePass Database File", fileChooserDescriptor = fileChooserDescriptor, fileChosen = { - normalizeSelectedFile(it) + when { + it.isDirectory -> "${it.path}${File.separator}$DB_FILE_NAME" + else -> it.path + } }, comment = if (SystemInfo.isWindows) null else "Stored using weak encryption. It is recommended to store on encrypted volume for additional security.") gearButton( ClearKeePassDatabaseAction(), - ImportKeePassDatabaseAction(passwordSafe), + ImportKeePassDatabaseAction(), ChangeKeePassDatabaseMasterPasswordAction() ) } @@ -209,9 +210,10 @@ internal class PasswordSafeConfigurableUi : ConfigurableUi return } + closeCurrentStoreIfKeePass() + LOG.info("Passwords cleared", Error()) - createKeePassFileManager()?.clear() ?: return - (PasswordSafe.instance as PasswordSafeImpl).closeCurrentStoreIfKeePass() + createKeePassFileManager()?.clear() } override fun update(e: AnActionEvent) { @@ -219,24 +221,26 @@ internal class PasswordSafeConfigurableUi : ConfigurableUi } } - private inner class ImportKeePassDatabaseAction(private val passwordSafe: PasswordSafeImpl) : DumbAwareAction("Import") { + private inner class ImportKeePassDatabaseAction : DumbAwareAction("Import") { override fun actionPerformed(event: AnActionEvent) { + closeCurrentStoreIfKeePass() + FileChooserDescriptorFactory.createSingleLocalFileDescriptor() .withFileFilter { - it.nameSequence.endsWith(".kdbx") + !it.isDirectory && it.nameSequence.endsWith(".kdbx") } .chooseFile(event) { - createKeePassFileManager()?.import(Paths.get(normalizeSelectedFile(it)), event) - passwordSafe.closeCurrentStoreIfKeePass() + createKeePassFileManager()?.import(Paths.get(it.path), event) } } } private inner class ChangeKeePassDatabaseMasterPasswordAction : DumbAwareAction("${if (MasterKeyFileStorage(getDefaultMasterPasswordFile()).isAutoGenerated()) "Set" else "Change"} Master Password") { override fun actionPerformed(event: AnActionEvent) { + closeCurrentStoreIfKeePass() + // even if current provider is not KEEPASS, all actions for db file must be applied immediately (show error if new master password not applicable for existing db file) if (createKeePassFileManager()?.askAndSetMasterKey(event) == true) { - (PasswordSafe.instance as PasswordSafeImpl).closeCurrentStoreIfKeePass() templatePresentation.text = "Change Master Password" } } @@ -247,11 +251,9 @@ internal class PasswordSafeConfigurableUi : ConfigurableUi } } -private fun normalizeSelectedFile(file: VirtualFile): String { - return when { - file.isDirectory -> file.path + File.separator + DB_FILE_NAME - else -> file.path - } +// we must save and close opened KeePass database before any action that can modify KeePass database files +private fun closeCurrentStoreIfKeePass() { + (PasswordSafe.instance as PasswordSafeImpl).closeCurrentStoreIfKeePass() } enum class ProviderType { @@ -260,13 +262,4 @@ enum class ProviderType { // unused, but we cannot remove it because enum value maybe stored in the config and we must correctly deserialize it @Deprecated("") DO_NOT_STORE -} - -private inline fun runAndHandleIncorrectMasterPasswordException(handler: () -> Unit) { - try { - handler() - } - catch (e: IncorrectMasterPasswordException) { - throw ConfigurationException("Master password for KeePass database is not correct (\"Clear\" can be used to reset database).") - } } \ No newline at end of file diff --git a/platform/credential-store/src/PasswordSafeImpl.kt b/platform/credential-store/src/PasswordSafeImpl.kt index 5dc1c36e25d2..c089c7143a90 100644 --- a/platform/credential-store/src/PasswordSafeImpl.kt +++ b/platform/credential-store/src/PasswordSafeImpl.kt @@ -83,9 +83,15 @@ class PasswordSafeImpl @JvmOverloads constructor(val settings: PasswordSafeSetti // force reload KeePass Store if settings changed internal fun closeCurrentStoreIfKeePass() { - val store = currentProviderIfComputed - if (store is KeePassCredentialStore && !store.isMemoryOnly) { + val store = currentProviderIfComputed as? KeePassCredentialStore ?: return + if (!store.isMemoryOnly) { (_currentProvider as SynchronizedClearableLazy).drop() + try { + store.save() + } + catch (e: Exception) { + LOG.warn(e) + } } }