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..e1ec90587 --- /dev/null +++ b/src/main/java/org/cryptomator/JavaFXUtil.java @@ -0,0 +1,22 @@ +package org.cryptomator; + +import javafx.application.Platform; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +public class JavaFXUtil { + + private 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..04a46e742 100644 --- a/src/main/java/org/cryptomator/common/keychain/KeychainManager.java +++ b/src/main/java/org/cryptomator/common/keychain/KeychainManager.java @@ -2,6 +2,7 @@ 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; @@ -13,20 +14,24 @@ 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 final ObjectExpression keychain; private final LoadingCache passphraseStoredProperties; + private final ReentrantReadWriteLock lock; @Inject KeychainManager(ObjectExpression selectedKeychain) { this.keychain = selectedKeychain; this.passphraseStoredProperties = Caffeine.newBuilder() // - .weakValues() // + .softValues() // .build(this::createStoredPassphraseProperty); keychain.addListener(ignored -> passphraseStoredProperties.invalidateAll()); + this.lock = new ReentrantReadWriteLock(false); } private KeychainAccessProvider getKeychainOrFail() throws KeychainAccessException { @@ -42,29 +47,59 @@ public class KeychainManager implements KeychainAccessProvider { return getClass().getName(); } + @Override + public void storePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException { + 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 ignored) throws KeychainAccessException { - getKeychainOrFail().storePassphrase(key, displayName, passphrase); + 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); } } @@ -101,13 +136,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 +167,22 @@ public class KeychainManager implements KeychainAccessProvider { } } + public ObjectExpression getKeychainImplementation() { + return this.keychain; + } + + 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"); + } + 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/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java index 68877430a..a13f3e223 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()).get()) { 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/MainWindowController.java b/src/main/java/org/cryptomator/ui/mainwindow/MainWindowController.java index 203f50085..c6e084518 100644 --- a/src/main/java/org/cryptomator/ui/mainwindow/MainWindowController.java +++ b/src/main/java/org/cryptomator/ui/mainwindow/MainWindowController.java @@ -166,7 +166,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/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..4505f412d 100644 --- a/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java +++ b/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java @@ -1,10 +1,13 @@ package org.cryptomator.ui.preferences; +import org.apache.commons.lang3.SystemUtils; import org.cryptomator.common.Environment; +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 +17,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; @@ -23,6 +27,10 @@ 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; +import java.util.stream.Collectors; @PreferencesScoped public class GeneralPreferencesController implements FxController { @@ -36,6 +44,8 @@ public class GeneralPreferencesController implements FxController { private final Application application; 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; @@ -47,12 +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, 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; @@ -73,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::migrateKeychainEntries); useQuickAccessCheckbox.selectedProperty().bindBidirectional(settings.useQuickAccess); var quickAccessSettingsConverter = new ServiceToSettingsConverter<>(quickAccessServices); @@ -83,6 +100,25 @@ public class GeneralPreferencesController implements FxController { quickAccessServiceChoiceBox.disableProperty().bind(useQuickAccessCheckbox.selectedProperty().not()); } + 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()) { + 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); + } catch (KeychainAccessException e) { + LOG.warn("Failed to migrate all entries from {} to {}", oldProvider.displayName(), newProvider.displayName(), e); + } + }, backgroundExecutor); + } + } + } + public boolean isAutoStartSupported() { return autoStartProvider.isPresent(); } diff --git a/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java b/src/test/java/org/cryptomator/common/keychain/KeychainManagerTest.java index abf803e1e..438574b78 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; @@ -13,11 +14,16 @@ 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 { + var isRunning = JavaFXUtil.startPlatform(); + Assumptions.assumeTrue(isRunning); + } @Test public void testStoreAndLoad() throws KeychainAccessException { @@ -27,15 +33,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); - } + class WhenObservingProperties { @Test public void testPropertyChangesWhenStoringPassword() throws KeychainAccessException, InterruptedException { @@ -43,7 +41,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..da9d65117 100644 --- a/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java +++ b/src/test/java/org/cryptomator/ui/controls/SecurePasswordFieldTest.java @@ -1,6 +1,6 @@ package org.cryptomator.ui.controls; -import org.junit.jupiter.api.AfterAll; +import org.cryptomator.JavaFXUtil; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeAll; @@ -8,20 +8,14 @@ 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(); @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