From 43fc976ad700e4b21bf7c6e8ad9e87d20a030d21 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Wed, 19 Feb 2025 11:49:59 +0100 Subject: [PATCH 1/6] reduce #3311 to touchID --- pom.xml | 2 +- src/main/java/org/cryptomator/JavaFXUtil.java | 20 ++++++++++++++ .../common/keychain/KeychainManager.java | 27 ++++++++++++------- .../MasterkeyFileLoadingStrategy.java | 8 +++--- .../VaultDetailLockedController.java | 13 ++++----- .../GeneralPreferencesController.java | 9 ++++++- .../common/keychain/KeychainManagerTest.java | 19 +++++++------ .../ui/controls/SecurePasswordFieldTest.java | 7 +++-- 8 files changed, 67 insertions(+), 38 deletions(-) create mode 100644 src/main/java/org/cryptomator/JavaFXUtil.java diff --git a/pom.xml b/pom.xml index 5aef0b15b..bed8a6597 100644 --- a/pom.xml +++ b/pom.xml @@ -36,7 +36,7 @@ 2.8.0 1.5.0 1.3.0 - 1.2.4 + 1.3.0 1.5.2 5.0.2 2.0.7 diff --git a/src/main/java/org/cryptomator/JavaFXUtil.java b/src/main/java/org/cryptomator/JavaFXUtil.java new file mode 100644 index 000000000..44372d58a --- /dev/null +++ b/src/main/java/org/cryptomator/JavaFXUtil.java @@ -0,0 +1,20 @@ +package org.cryptomator; + +import javafx.application.Platform; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +public class JavaFXUtil { + + public static boolean startPlatform() throws InterruptedException { + CountDownLatch latch = new CountDownLatch(1); + try { + Platform.startup(latch::countDown); + } catch (IllegalStateException e) { + //already initialized + latch.countDown(); + } + return latch.await(5, TimeUnit.SECONDS); + } + +} diff --git a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java index ac03e5ed6..b941147bc 100644 --- a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java +++ b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java @@ -24,7 +24,7 @@ public class KeychainManager implements KeychainAccessProvider { KeychainManager(ObjectExpression selectedKeychain) { this.keychain = selectedKeychain; this.passphraseStoredProperties = Caffeine.newBuilder() // - .weakValues() // + .softValues() // .build(this::createStoredPassphraseProperty); keychain.addListener(ignored -> passphraseStoredProperties.invalidateAll()); } @@ -43,8 +43,13 @@ public class KeychainManager implements KeychainAccessProvider { } @Override - public void storePassphrase(String key, String displayName, CharSequence passphrase, boolean ignored) throws KeychainAccessException { - getKeychainOrFail().storePassphrase(key, displayName, passphrase); + public void storePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException { + storePassphrase(key, displayName, passphrase, true); //TODO: currently only TouchID is using this parameter, so this is okayish + } + + @Override + public void storePassphrase(String key, String displayName, CharSequence passphrase, boolean requireOsAuthentication) throws KeychainAccessException { + getKeychainOrFail().storePassphrase(key, displayName, passphrase, requireOsAuthentication); setPassphraseStored(key, true); } @@ -101,13 +106,11 @@ public class KeychainManager implements KeychainAccessProvider { } private void setPassphraseStored(String key, boolean value) { - BooleanProperty property = passphraseStoredProperties.getIfPresent(key); - if (property != null) { - if (Platform.isFxApplicationThread()) { - property.set(value); - } else { - Platform.runLater(() -> property.set(value)); - } + BooleanProperty property = passphraseStoredProperties.get(key, _ -> new SimpleBooleanProperty(value)); + if (Platform.isFxApplicationThread()) { + property.set(value); + } else { + Platform.runLater(() -> property.set(value)); } } @@ -134,4 +137,8 @@ public class KeychainManager implements KeychainAccessProvider { } } + public ObjectExpression getKeychainImplementation() { + return this.keychain; + } + } diff --git a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java index 68877430a..3f9d5ba78 100644 --- a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java +++ b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java @@ -112,12 +112,12 @@ public class MasterkeyFileLoadingStrategy implements KeyLoadingStrategy { } private void savePasswordToSystemkeychain(Passphrase passphrase) { - if (keychain.isSupported()) { - try { + try { + if (keychain.isSupported() && !keychain.getPassphraseStoredProperty(vault.getId()).getValue()) { keychain.storePassphrase(vault.getId(), vault.getDisplayName(), passphrase); - } catch (KeychainAccessException e) { - LOG.error("Failed to store passphrase in system keychain.", e); } + } catch (KeychainAccessException e) { + LOG.error("Failed to store passphrase in system keychain.", e); } } diff --git a/src/main/java/org/cryptomator/ui/mainwindow/VaultDetailLockedController.java b/src/main/java/org/cryptomator/ui/mainwindow/VaultDetailLockedController.java index 8212f598f..6a7c046f2 100644 --- a/src/main/java/org/cryptomator/ui/mainwindow/VaultDetailLockedController.java +++ b/src/main/java/org/cryptomator/ui/mainwindow/VaultDetailLockedController.java @@ -8,9 +8,9 @@ import org.cryptomator.ui.vaultoptions.SelectedVaultOptionsTab; import org.cryptomator.ui.vaultoptions.VaultOptionsComponent; import javax.inject.Inject; +import javafx.beans.binding.Bindings; import javafx.beans.property.ObjectProperty; import javafx.beans.property.ReadOnlyObjectProperty; -import javafx.beans.property.SimpleBooleanProperty; import javafx.beans.value.ObservableValue; import javafx.fxml.FXML; import javafx.stage.Stage; @@ -21,7 +21,6 @@ public class VaultDetailLockedController implements FxController { private final ReadOnlyObjectProperty vault; private final FxApplicationWindows appWindows; private final VaultOptionsComponent.Factory vaultOptionsWindow; - private final KeychainManager keychain; private final Stage mainWindow; private final ObservableValue passwordSaved; @@ -30,13 +29,11 @@ public class VaultDetailLockedController implements FxController { this.vault = vault; this.appWindows = appWindows; this.vaultOptionsWindow = vaultOptionsWindow; - this.keychain = keychain; this.mainWindow = mainWindow; - if (keychain.isSupported() && !keychain.isLocked()) { - this.passwordSaved = vault.flatMap(v -> keychain.getPassphraseStoredProperty(v.getId())).orElse(false); - } else { - this.passwordSaved = new SimpleBooleanProperty(false); - } + this.passwordSaved = Bindings.createBooleanBinding(() -> { + var v = vault.get(); + return v != null && keychain.getPassphraseStoredProperty(v.getId()).getValue(); + }, vault, keychain.getKeychainImplementation()); } @FXML diff --git a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java index 800a292a9..3bbc30474 100644 --- a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java +++ b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java @@ -1,10 +1,14 @@ package org.cryptomator.ui.preferences; +import org.apache.commons.lang3.SystemUtils; import org.cryptomator.common.Environment; +import org.cryptomator.common.Passphrase; +import org.cryptomator.common.keychain.KeychainManager; import org.cryptomator.common.settings.Settings; import org.cryptomator.integrations.autostart.AutoStartProvider; import org.cryptomator.integrations.autostart.ToggleAutoStartFailedException; import org.cryptomator.integrations.common.NamedServiceProvider; +import org.cryptomator.integrations.keychain.KeychainAccessException; import org.cryptomator.integrations.keychain.KeychainAccessProvider; import org.cryptomator.integrations.quickaccess.QuickAccessService; import org.cryptomator.ui.common.FxController; @@ -14,6 +18,7 @@ import org.slf4j.LoggerFactory; import javax.inject.Inject; import javafx.application.Application; +import javafx.beans.Observable; import javafx.beans.binding.Bindings; import javafx.fxml.FXML; import javafx.scene.control.CheckBox; @@ -36,6 +41,7 @@ public class GeneralPreferencesController implements FxController { private final Application application; private final Environment environment; private final List keychainAccessProviders; + private final KeychainManager keychain; private final FxApplicationWindows appWindows; public CheckBox useKeychainCheckbox; public ChoiceBox keychainBackendChoiceBox; @@ -48,11 +54,12 @@ public class GeneralPreferencesController implements FxController { public ToggleGroup nodeOrientation; @Inject - GeneralPreferencesController(@PreferencesWindow Stage window, Settings settings, Optional autoStartProvider, List keychainAccessProviders, Application application, Environment environment, FxApplicationWindows appWindows) { + GeneralPreferencesController(@PreferencesWindow Stage window, Settings settings, Optional autoStartProvider, List keychainAccessProviders, KeychainManager keychain, Application application, Environment environment, FxApplicationWindows appWindows) { this.window = window; this.settings = settings; this.autoStartProvider = autoStartProvider; this.keychainAccessProviders = keychainAccessProviders; + this.keychain = keychain; this.quickAccessServices = QuickAccessService.get().toList(); this.application = application; this.environment = environment; diff --git a/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java b/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java index abf803e1e..6345faaf7 100644 --- a/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java +++ b/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java @@ -1,6 +1,7 @@ package org.cryptomator.common.keychain; +import org.cryptomator.JavaFXUtil; import org.cryptomator.integrations.keychain.KeychainAccessException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Assumptions; @@ -19,6 +20,12 @@ import java.util.concurrent.atomic.AtomicBoolean; public class KeychainManagerTest { + @BeforeAll + public static void startup() throws InterruptedException { + var isRunning = JavaFXUtil.startPlatform(); + Assumptions.assumeTrue(isRunning); + } + @Test public void testStoreAndLoad() throws KeychainAccessException { KeychainManager keychainManager = new KeychainManager(new SimpleObjectProperty<>(new MapKeychainAccess())); @@ -27,15 +34,7 @@ public class KeychainManagerTest { } @Nested - public static class WhenObservingProperties { - - @BeforeAll - public static void startup() throws InterruptedException { - CountDownLatch latch = new CountDownLatch(1); - Platform.startup(latch::countDown); - var javafxStarted = latch.await(5, TimeUnit.SECONDS); - Assumptions.assumeTrue(javafxStarted); - } + public class WhenObservingProperties { @Test public void testPropertyChangesWhenStoringPassword() throws KeychainAccessException, InterruptedException { @@ -43,7 +42,7 @@ public class KeychainManagerTest { ReadOnlyBooleanProperty property = keychainManager.getPassphraseStoredProperty("test"); Assertions.assertFalse(property.get()); - keychainManager.storePassphrase("test", null,"bar"); + keychainManager.storePassphrase("test", null, "bar"); AtomicBoolean result = new AtomicBoolean(false); CountDownLatch latch = new CountDownLatch(1); diff --git a/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java b/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java index bfe31816e..b6052361b 100644 --- a/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java +++ b/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java @@ -1,5 +1,6 @@ package org.cryptomator.ui.controls; +import org.cryptomator.JavaFXUtil; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Assumptions; @@ -18,10 +19,8 @@ public class SecurePasswordFieldTest { @BeforeAll public static void initJavaFx() throws InterruptedException { - CountDownLatch latch = new CountDownLatch(1); - Platform.startup(latch::countDown); - var javafxStarted = latch.await(5, TimeUnit.SECONDS); - Assumptions.assumeTrue(javafxStarted); + var isRunning = JavaFXUtil.startPlatform(); + Assumptions.assumeTrue(isRunning); } @Nested From 0bcbf9a13a472986cbeee08461035a688e86906e Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Wed, 19 Feb 2025 15:20:57 +0100 Subject: [PATCH 2/6] add migration code and synchronize keychain edits --- .../common/keychain/KeychainManager.java | 40 ++++++++++++++++--- .../GeneralPreferencesController.java | 40 ++++++++++++++++++- 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java index b941147bc..ab44a5003 100644 --- a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java +++ b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java @@ -13,12 +13,14 @@ import javafx.beans.property.BooleanProperty; import javafx.beans.property.ReadOnlyBooleanProperty; import javafx.beans.property.SimpleBooleanProperty; import java.util.Arrays; +import java.util.concurrent.locks.ReentrantReadWriteLock; @Singleton public class KeychainManager implements KeychainAccessProvider { private final ObjectExpression keychain; private final LoadingCache passphraseStoredProperties; + private final ReentrantReadWriteLock lock; @Inject KeychainManager(ObjectExpression selectedKeychain) { @@ -27,6 +29,7 @@ public class KeychainManager implements KeychainAccessProvider { .softValues() // .build(this::createStoredPassphraseProperty); keychain.addListener(ignored -> passphraseStoredProperties.invalidateAll()); + this.lock = new ReentrantReadWriteLock(false); } private KeychainAccessProvider getKeychainOrFail() throws KeychainAccessException { @@ -44,32 +47,57 @@ public class KeychainManager implements KeychainAccessProvider { @Override public void storePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException { - storePassphrase(key, displayName, passphrase, true); //TODO: currently only TouchID is using this parameter, so this is okayish + storePassphrase(key, displayName, passphrase, true); } + //TODO: remove ignored parameter once the API is fixed @Override - public void storePassphrase(String key, String displayName, CharSequence passphrase, boolean requireOsAuthentication) throws KeychainAccessException { - getKeychainOrFail().storePassphrase(key, displayName, passphrase, requireOsAuthentication); + public void storePassphrase(String key, String displayName, CharSequence passphrase, boolean ignored) throws KeychainAccessException { + try { + lock.writeLock().lock(); + var kc = getKeychainOrFail(); + //this is the only keychain actually using the parameter + var usesOSAuth = (kc.getClass().getName().equals("org.cryptomator.macos.keychain.TouchIdKeychainAccess")); + kc.storePassphrase(key, displayName, passphrase, usesOSAuth); + } finally { + lock.writeLock().unlock(); + } setPassphraseStored(key, true); } @Override public char[] loadPassphrase(String key) throws KeychainAccessException { - char[] passphrase = getKeychainOrFail().loadPassphrase(key); + char[] passphrase = null; + try { + lock.readLock().lock(); + passphrase = getKeychainOrFail().loadPassphrase(key); + } finally { + lock.readLock().unlock(); + } setPassphraseStored(key, passphrase != null); return passphrase; } @Override public void deletePassphrase(String key) throws KeychainAccessException { - getKeychainOrFail().deletePassphrase(key); + try { + lock.writeLock().lock(); + getKeychainOrFail().deletePassphrase(key); + } finally { + lock.writeLock().unlock(); + } setPassphraseStored(key, false); } @Override public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException { if (isPassphraseStored(key)) { - getKeychainOrFail().changePassphrase(key, displayName, passphrase); + try { + lock.writeLock().lock(); + getKeychainOrFail().changePassphrase(key, displayName, passphrase); + } finally { + lock.writeLock().unlock(); + } setPassphraseStored(key, true); } } diff --git a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java index 3bbc30474..f002d20fa 100644 --- a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java +++ b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java @@ -28,6 +28,9 @@ import javafx.stage.Stage; import javafx.util.StringConverter; import java.util.List; import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.ExecutorService; @PreferencesScoped public class GeneralPreferencesController implements FxController { @@ -42,6 +45,7 @@ public class GeneralPreferencesController implements FxController { private final Environment environment; private final List keychainAccessProviders; private final KeychainManager keychain; + private final ExecutorService backgroundExecutor; private final FxApplicationWindows appWindows; public CheckBox useKeychainCheckbox; public ChoiceBox keychainBackendChoiceBox; @@ -53,13 +57,18 @@ public class GeneralPreferencesController implements FxController { public CheckBox autoStartCheckbox; public ToggleGroup nodeOrientation; + private CompletionStage keychainMigrations = CompletableFuture.completedFuture(null); + @Inject - GeneralPreferencesController(@PreferencesWindow Stage window, Settings settings, Optional autoStartProvider, List keychainAccessProviders, KeychainManager keychain, Application application, Environment environment, FxApplicationWindows appWindows) { + GeneralPreferencesController(@PreferencesWindow Stage window, Settings settings, Optional autoStartProvider, // + List keychainAccessProviders, KeychainManager keychain, Application application, // + Environment environment, FxApplicationWindows appWindows, ExecutorService backgroundExecutor) { this.window = window; this.settings = settings; this.autoStartProvider = autoStartProvider; this.keychainAccessProviders = keychainAccessProviders; this.keychain = keychain; + this.backgroundExecutor = backgroundExecutor; this.quickAccessServices = QuickAccessService.get().toList(); this.application = application; this.environment = environment; @@ -80,6 +89,7 @@ public class GeneralPreferencesController implements FxController { Bindings.bindBidirectional(settings.keychainProvider, keychainBackendChoiceBox.valueProperty(), keychainSettingsConverter); useKeychainCheckbox.selectedProperty().bindBidirectional(settings.useKeychain); keychainBackendChoiceBox.disableProperty().bind(useKeychainCheckbox.selectedProperty().not()); + keychainBackendChoiceBox.valueProperty().addListener(this::migrateMacKeychainEntries); useQuickAccessCheckbox.selectedProperty().bindBidirectional(settings.useQuickAccess); var quickAccessSettingsConverter = new ServiceToSettingsConverter<>(quickAccessServices); @@ -90,6 +100,34 @@ public class GeneralPreferencesController implements FxController { quickAccessServiceChoiceBox.disableProperty().bind(useQuickAccessCheckbox.selectedProperty().not()); } + public void migrateMacKeychainEntries(Observable observable, KeychainAccessProvider oldProvider, KeychainAccessProvider newProvider) { + if (!SystemUtils.IS_OS_MAC) { + return; + } + + record VIdAndName(String id, String name) {} + var vaults = settings.directories.stream().map(vault -> new VIdAndName(vault.id, vault.displayName.getValue())).toList(); + + keychainMigrations = keychainMigrations.thenRunAsync(() -> { + if (!vaults.isEmpty()) { + LOG.info("Migrating keychain entries for vaults: {}", vaults.stream().map(VIdAndName::id)); + } + for (var v : vaults) { //TODO: migrate to pattern matching once supported + try { + var passphrase = oldProvider.loadPassphrase(v.id); + if (passphrase != null) { + var wrapper = new Passphrase(passphrase); + newProvider.storePassphrase(v.id, v.name, wrapper); + oldProvider.deletePassphrase(v.id); + wrapper.destroy(); + } + } catch (KeychainAccessException e) { + LOG.error("Failed to migrate keychain entry for vault {}.", v.id, e); + } + } + }, backgroundExecutor); + } + public boolean isAutoStartSupported() { return autoStartProvider.isPresent(); } From eb0e630a441d7258d2c20377698eb8b310acbc86 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Tue, 25 Feb 2025 17:33:14 +0100 Subject: [PATCH 3/6] move migration to KeychainManager --- .../common/keychain/KeychainManager.java | 26 +++++++++++++ .../GeneralPreferencesController.java | 37 +++++-------------- 2 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java index ab44a5003..8f589933d 100644 --- a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java +++ b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java @@ -2,8 +2,11 @@ package org.cryptomator.common.keychain; import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; +import org.cryptomator.common.Passphrase; import org.cryptomator.integrations.keychain.KeychainAccessException; import org.cryptomator.integrations.keychain.KeychainAccessProvider; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.inject.Inject; import javax.inject.Singleton; @@ -13,11 +16,14 @@ import javafx.beans.property.BooleanProperty; import javafx.beans.property.ReadOnlyBooleanProperty; import javafx.beans.property.SimpleBooleanProperty; import java.util.Arrays; +import java.util.Map; import java.util.concurrent.locks.ReentrantReadWriteLock; @Singleton public class KeychainManager implements KeychainAccessProvider { + private static final Logger LOG = LoggerFactory.getLogger(KeychainManager.class); + private final ObjectExpression keychain; private final LoadingCache passphraseStoredProperties; private final ReentrantReadWriteLock lock; @@ -169,4 +175,24 @@ public class KeychainManager implements KeychainAccessProvider { return this.keychain; } + public static void migrate(KeychainAccessProvider oldProvider, KeychainAccessProvider newProvider, Map idsAndNames) { + if (oldProvider instanceof KeychainManager || newProvider instanceof KeychainManager) { + throw new IllegalArgumentException("KeychainManger must not be the source or target of migration"); + } + + LOG.info("Migrating keychain entries {} from {} to {}", idsAndNames.keySet(), oldProvider.displayName(), newProvider.displayName()); + idsAndNames.forEach((id, name) -> { + try { + var passphrase = oldProvider.loadPassphrase(id); + if (passphrase != null) { + var wrapper = new Passphrase(passphrase); + newProvider.storePassphrase(id, name, wrapper); + oldProvider.deletePassphrase(id); + wrapper.destroy(); + } + } catch (KeychainAccessException e) { + LOG.error("Failed to migrate keychain entry for vault {}.", id, e); + } + }); + } } diff --git a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java index f002d20fa..361728816 100644 --- a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java +++ b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java @@ -2,13 +2,11 @@ package org.cryptomator.ui.preferences; import org.apache.commons.lang3.SystemUtils; import org.cryptomator.common.Environment; -import org.cryptomator.common.Passphrase; import org.cryptomator.common.keychain.KeychainManager; import org.cryptomator.common.settings.Settings; import org.cryptomator.integrations.autostart.AutoStartProvider; import org.cryptomator.integrations.autostart.ToggleAutoStartFailedException; import org.cryptomator.integrations.common.NamedServiceProvider; -import org.cryptomator.integrations.keychain.KeychainAccessException; import org.cryptomator.integrations.keychain.KeychainAccessProvider; import org.cryptomator.integrations.quickaccess.QuickAccessService; import org.cryptomator.ui.common.FxController; @@ -31,6 +29,7 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.ExecutorService; +import java.util.stream.Collectors; @PreferencesScoped public class GeneralPreferencesController implements FxController { @@ -89,7 +88,7 @@ public class GeneralPreferencesController implements FxController { Bindings.bindBidirectional(settings.keychainProvider, keychainBackendChoiceBox.valueProperty(), keychainSettingsConverter); useKeychainCheckbox.selectedProperty().bindBidirectional(settings.useKeychain); keychainBackendChoiceBox.disableProperty().bind(useKeychainCheckbox.selectedProperty().not()); - keychainBackendChoiceBox.valueProperty().addListener(this::migrateMacKeychainEntries); + keychainBackendChoiceBox.valueProperty().addListener(this::migrateKeychainEntries); useQuickAccessCheckbox.selectedProperty().bindBidirectional(settings.useQuickAccess); var quickAccessSettingsConverter = new ServiceToSettingsConverter<>(quickAccessServices); @@ -100,32 +99,14 @@ public class GeneralPreferencesController implements FxController { quickAccessServiceChoiceBox.disableProperty().bind(useQuickAccessCheckbox.selectedProperty().not()); } - public void migrateMacKeychainEntries(Observable observable, KeychainAccessProvider oldProvider, KeychainAccessProvider newProvider) { - if (!SystemUtils.IS_OS_MAC) { - return; + private void migrateKeychainEntries(Observable observable, KeychainAccessProvider oldProvider, KeychainAccessProvider newProvider) { + //currently, we only migrate on macOS (touchID vs regular keychain) + if (SystemUtils.IS_OS_MAC) { + var idsAndNames = settings.directories.stream().collect(Collectors.toMap(vs -> vs.id, vs -> vs.displayName.getValue())); + if (!idsAndNames.isEmpty()) { + keychainMigrations = keychainMigrations.thenRunAsync(() -> KeychainManager.migrate(oldProvider, newProvider, idsAndNames), backgroundExecutor); + } } - - record VIdAndName(String id, String name) {} - var vaults = settings.directories.stream().map(vault -> new VIdAndName(vault.id, vault.displayName.getValue())).toList(); - - keychainMigrations = keychainMigrations.thenRunAsync(() -> { - if (!vaults.isEmpty()) { - LOG.info("Migrating keychain entries for vaults: {}", vaults.stream().map(VIdAndName::id)); - } - for (var v : vaults) { //TODO: migrate to pattern matching once supported - try { - var passphrase = oldProvider.loadPassphrase(v.id); - if (passphrase != null) { - var wrapper = new Passphrase(passphrase); - newProvider.storePassphrase(v.id, v.name, wrapper); - oldProvider.deletePassphrase(v.id); - wrapper.destroy(); - } - } catch (KeychainAccessException e) { - LOG.error("Failed to migrate keychain entry for vault {}.", v.id, e); - } - } - }, backgroundExecutor); } public boolean isAutoStartSupported() { From fb56b61a758da56eb82580b728a89bd9433dde4f Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Tue, 11 Mar 2025 17:14:42 +0100 Subject: [PATCH 4/6] add exceptionally clause to keychain migration task --- .../ui/preferences/GeneralPreferencesController.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java index 361728816..d42dabc80 100644 --- a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java +++ b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java @@ -104,7 +104,11 @@ public class GeneralPreferencesController implements FxController { if (SystemUtils.IS_OS_MAC) { var idsAndNames = settings.directories.stream().collect(Collectors.toMap(vs -> vs.id, vs -> vs.displayName.getValue())); if (!idsAndNames.isEmpty()) { - keychainMigrations = keychainMigrations.thenRunAsync(() -> KeychainManager.migrate(oldProvider, newProvider, idsAndNames), backgroundExecutor); + keychainMigrations = keychainMigrations.thenRunAsync(() -> KeychainManager.migrate(oldProvider, newProvider, idsAndNames), backgroundExecutor) // + .exceptionally(e -> { + LOG.warn("Failed to migrate entries", e); + return null; + }); } } } From 7ec3c5d04ff20e61832b9c16f8ffa2dc981bfc07 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Wed, 12 Mar 2025 11:41:12 +0100 Subject: [PATCH 5/6] first delete, then write --- .../common/keychain/KeychainManager.java | 28 ++++++------------- .../GeneralPreferencesController.java | 14 ++++++---- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java index 8f589933d..04a46e742 100644 --- a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java +++ b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java @@ -5,8 +5,6 @@ import com.github.benmanes.caffeine.cache.LoadingCache; import org.cryptomator.common.Passphrase; import org.cryptomator.integrations.keychain.KeychainAccessException; import org.cryptomator.integrations.keychain.KeychainAccessProvider; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import javax.inject.Inject; import javax.inject.Singleton; @@ -22,8 +20,6 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; @Singleton public class KeychainManager implements KeychainAccessProvider { - private static final Logger LOG = LoggerFactory.getLogger(KeychainManager.class); - private final ObjectExpression keychain; private final LoadingCache passphraseStoredProperties; private final ReentrantReadWriteLock lock; @@ -175,24 +171,18 @@ public class KeychainManager implements KeychainAccessProvider { return this.keychain; } - public static void migrate(KeychainAccessProvider oldProvider, KeychainAccessProvider newProvider, Map idsAndNames) { + public static void migrate(KeychainAccessProvider oldProvider, KeychainAccessProvider newProvider, Map idsAndNames) throws KeychainAccessException { if (oldProvider instanceof KeychainManager || newProvider instanceof KeychainManager) { throw new IllegalArgumentException("KeychainManger must not be the source or target of migration"); } - - LOG.info("Migrating keychain entries {} from {} to {}", idsAndNames.keySet(), oldProvider.displayName(), newProvider.displayName()); - idsAndNames.forEach((id, name) -> { - try { - var passphrase = oldProvider.loadPassphrase(id); - if (passphrase != null) { - var wrapper = new Passphrase(passphrase); - newProvider.storePassphrase(id, name, wrapper); - oldProvider.deletePassphrase(id); - wrapper.destroy(); - } - } catch (KeychainAccessException e) { - LOG.error("Failed to migrate keychain entry for vault {}.", id, e); + for (var entry : idsAndNames.entrySet()) { + var passphrase = oldProvider.loadPassphrase(entry.getKey()); + if (passphrase != null) { + var wrapper = new Passphrase(passphrase); + oldProvider.deletePassphrase(entry.getKey()); //we cannot apply "first-write-then-delete" pattern here, since we can potentially write to the same passphrase store (e.g., touchID and regular keychain) + newProvider.storePassphrase(entry.getKey(), entry.getValue(), wrapper); + wrapper.destroy(); } - }); + } } } diff --git a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java index d42dabc80..ce286c9e7 100644 --- a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java +++ b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java @@ -7,6 +7,7 @@ import org.cryptomator.common.settings.Settings; import org.cryptomator.integrations.autostart.AutoStartProvider; import org.cryptomator.integrations.autostart.ToggleAutoStartFailedException; import org.cryptomator.integrations.common.NamedServiceProvider; +import org.cryptomator.integrations.keychain.KeychainAccessException; import org.cryptomator.integrations.keychain.KeychainAccessProvider; import org.cryptomator.integrations.quickaccess.QuickAccessService; import org.cryptomator.ui.common.FxController; @@ -104,11 +105,14 @@ public class GeneralPreferencesController implements FxController { if (SystemUtils.IS_OS_MAC) { var idsAndNames = settings.directories.stream().collect(Collectors.toMap(vs -> vs.id, vs -> vs.displayName.getValue())); if (!idsAndNames.isEmpty()) { - keychainMigrations = keychainMigrations.thenRunAsync(() -> KeychainManager.migrate(oldProvider, newProvider, idsAndNames), backgroundExecutor) // - .exceptionally(e -> { - LOG.warn("Failed to migrate entries", e); - return null; - }); + LOG.debug("Migrating keychain entries {} from {} to {}", idsAndNames.keySet(), oldProvider.displayName(), newProvider.displayName()); + keychainMigrations = keychainMigrations.thenRunAsync(() -> { + try { + KeychainManager.migrate(oldProvider, newProvider, idsAndNames); + } catch (KeychainAccessException e) { + LOG.warn("Failed to migrate all entries from {} to {}", oldProvider.displayName(), newProvider.displayName(), e); + } + }, backgroundExecutor); } } } From 42ec41b99103402a5df7714064e4f1ee4d338e2c Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Wed, 12 Mar 2025 12:19:27 +0100 Subject: [PATCH 6/6] clean up code --- src/main/java/org/cryptomator/JavaFXUtil.java | 2 ++ .../masterkeyfile/MasterkeyFileLoadingStrategy.java | 2 +- .../org/cryptomator/ui/mainwindow/MainWindowController.java | 2 +- .../ui/preferences/GeneralPreferencesController.java | 4 +++- .../org/cryptomator/common/keychain/KeychainManagerTest.java | 5 ++--- .../org/cryptomator/ui/controls/SecurePasswordFieldTest.java | 5 ----- 6 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/cryptomator/JavaFXUtil.java b/src/main/java/org/cryptomator/JavaFXUtil.java index 44372d58a..e1ec90587 100644 --- a/src/main/java/org/cryptomator/JavaFXUtil.java +++ b/src/main/java/org/cryptomator/JavaFXUtil.java @@ -6,6 +6,8 @@ import java.util.concurrent.TimeUnit; public class JavaFXUtil { + private JavaFXUtil() {} + public static boolean startPlatform() throws InterruptedException { CountDownLatch latch = new CountDownLatch(1); try { diff --git a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java index 3f9d5ba78..a13f3e223 100644 --- a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java +++ b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java @@ -113,7 +113,7 @@ public class MasterkeyFileLoadingStrategy implements KeyLoadingStrategy { private void savePasswordToSystemkeychain(Passphrase passphrase) { try { - if (keychain.isSupported() && !keychain.getPassphraseStoredProperty(vault.getId()).getValue()) { + if (keychain.isSupported() && !keychain.getPassphraseStoredProperty(vault.getId()).get()) { keychain.storePassphrase(vault.getId(), vault.getDisplayName(), passphrase); } } catch (KeychainAccessException e) { diff --git a/src/main/java/org/cryptomator/ui/mainwindow/MainWindowController.java b/src/main/java/org/cryptomator/ui/mainwindow/MainWindowController.java index 18db5e5ac..bdcfedaa8 100644 --- a/src/main/java/org/cryptomator/ui/mainwindow/MainWindowController.java +++ b/src/main/java/org/cryptomator/ui/mainwindow/MainWindowController.java @@ -176,7 +176,7 @@ public class MainWindowController implements FxController { return updateAvailable.get(); } - public BooleanBinding licenseValidProperty(){ + public BooleanBinding licenseValidProperty() { return licenseHolder.validLicenseProperty(); } diff --git a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java index ce286c9e7..4505f412d 100644 --- a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java +++ b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java @@ -105,7 +105,9 @@ public class GeneralPreferencesController implements FxController { if (SystemUtils.IS_OS_MAC) { var idsAndNames = settings.directories.stream().collect(Collectors.toMap(vs -> vs.id, vs -> vs.displayName.getValue())); if (!idsAndNames.isEmpty()) { - LOG.debug("Migrating keychain entries {} from {} to {}", idsAndNames.keySet(), oldProvider.displayName(), newProvider.displayName()); + if (LOG.isDebugEnabled()) { + LOG.debug("Migrating keychain entries {} from {} to {}", idsAndNames.keySet(), oldProvider.displayName(), newProvider.displayName()); + } keychainMigrations = keychainMigrations.thenRunAsync(() -> { try { KeychainManager.migrate(oldProvider, newProvider, idsAndNames); diff --git a/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java b/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java index 6345faaf7..438574b78 100644 --- a/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java +++ b/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java @@ -14,11 +14,10 @@ import javafx.beans.property.ReadOnlyBooleanProperty; import javafx.beans.property.SimpleObjectProperty; import java.time.Duration; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -public class KeychainManagerTest { +class KeychainManagerTest { @BeforeAll public static void startup() throws InterruptedException { @@ -34,7 +33,7 @@ public class KeychainManagerTest { } @Nested - public class WhenObservingProperties { + class WhenObservingProperties { @Test public void testPropertyChangesWhenStoringPassword() throws KeychainAccessException, InterruptedException { diff --git a/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java b/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java index b6052361b..da9d65117 100644 --- a/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java +++ b/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java @@ -1,7 +1,6 @@ package org.cryptomator.ui.controls; import org.cryptomator.JavaFXUtil; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeAll; @@ -9,10 +8,6 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import javafx.application.Platform; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; - public class SecurePasswordFieldTest { private SecurePasswordField pwField = new SecurePasswordField();