Simplify password creation (#1646)

* Remove the password properties and directly access password fields from included fxml files
* wipe the password fields when closing a window

main/ui/src/main/java/org/cryptomator/ui/addvaultwizard/AddVaultModule.java

@@ -6,10 +6,10 @@ import dagger.Provides;
 import dagger.multibindings.IntoMap;
 import org.cryptomator.common.vaults.Vault;
 import org.cryptomator.ui.common.DefaultSceneFactory;
-import org.cryptomator.ui.common.FxmlLoaderFactory;
 import org.cryptomator.ui.common.FxController;
 import org.cryptomator.ui.common.FxControllerKey;
 import org.cryptomator.ui.common.FxmlFile;
+import org.cryptomator.ui.common.FxmlLoaderFactory;
 import org.cryptomator.ui.common.FxmlScene;
 import org.cryptomator.ui.common.NewPasswordController;
 import org.cryptomator.ui.common.PasswordStrengthUtil;
@@ -33,13 +33,6 @@ import java.util.ResourceBundle;
 @Module
 public abstract class AddVaultModule {
 
-	@Provides
-	@AddVaultWizardScoped
-	@Named("newPassword")
-	static ObjectProperty<CharSequence> provideNewPasswordProperty() {
-		return new SimpleObjectProperty<>("");
-	}
-
 	@Provides
 	@AddVaultWizardWindow
 	@AddVaultWizardScoped
@@ -167,8 +160,8 @@ public abstract class AddVaultModule {
 	@Provides
 	@IntoMap
 	@FxControllerKey(NewPasswordController.class)
-	static FxController provideNewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater, @Named("newPassword") ObjectProperty<CharSequence> password) {
-		return new NewPasswordController(resourceBundle, strengthRater, password);
+	static FxController provideNewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater) {
+		return new NewPasswordController(resourceBundle, strengthRater);
 	}
 
 	@Binds

main/ui/src/main/java/org/cryptomator/ui/changepassword/ChangePasswordController.java

@@ -10,21 +10,18 @@ import org.cryptomator.integrations.keychain.KeychainAccessException;
 import org.cryptomator.ui.common.Animations;
 import org.cryptomator.ui.common.ErrorComponent;
 import org.cryptomator.ui.common.FxController;
+import org.cryptomator.ui.common.NewPasswordController;
 import org.cryptomator.ui.controls.NiceSecurePasswordField;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.inject.Inject;
-import javax.inject.Named;
-import javafx.beans.binding.Bindings;
 import javafx.beans.binding.BooleanBinding;
-import javafx.beans.property.ObjectProperty;
 import javafx.fxml.FXML;
 import javafx.scene.control.Button;
 import javafx.scene.control.CheckBox;
 import javafx.stage.Stage;
 import java.io.IOException;
-import java.nio.CharBuffer;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
@@ -41,7 +38,6 @@ public class ChangePasswordController implements FxController {
 
 	private final Stage window;
 	private final Vault vault;
-	private final ObjectProperty<CharSequence> newPassword;
 	private final ErrorComponent.Builder errorComponent;
 	private final KeychainManager keychain;
 	private final SecureRandom csprng;
@@ -50,12 +46,12 @@ public class ChangePasswordController implements FxController {
 	public NiceSecurePasswordField oldPasswordField;
 	public CheckBox finalConfirmationCheckbox;
 	public Button finishButton;
+	public NewPasswordController newPasswordController;
 
 	@Inject
-	public ChangePasswordController(@ChangePasswordWindow Stage window, @ChangePasswordWindow Vault vault, @Named("newPassword") ObjectProperty<CharSequence> newPassword, ErrorComponent.Builder errorComponent, KeychainManager keychain, SecureRandom csprng, MasterkeyFileAccess masterkeyFileAccess) {
+	public ChangePasswordController(@ChangePasswordWindow Stage window, @ChangePasswordWindow Vault vault, ErrorComponent.Builder errorComponent, KeychainManager keychain, SecureRandom csprng, MasterkeyFileAccess masterkeyFileAccess) {
 		this.window = window;
 		this.vault = vault;
-		this.newPassword = newPassword;
 		this.errorComponent = errorComponent;
 		this.keychain = keychain;
 		this.csprng = csprng;
@@ -66,8 +62,12 @@ public class ChangePasswordController implements FxController {
 	public void initialize() {
 		BooleanBinding checkboxNotConfirmed = finalConfirmationCheckbox.selectedProperty().not();
 		BooleanBinding oldPasswordFieldEmpty = oldPasswordField.textProperty().isEmpty();
-		BooleanBinding newPasswordInvalid = Bindings.createBooleanBinding(() -> newPassword.get() == null || newPassword.get().length() == 0, newPassword);
-		finishButton.disableProperty().bind(checkboxNotConfirmed.or(oldPasswordFieldEmpty).or(newPasswordInvalid));
+		finishButton.disableProperty().bind(checkboxNotConfirmed.or(oldPasswordFieldEmpty).or(newPasswordController.passwordsMatchAndSufficientProperty().not()));
+		window.setOnHiding(event -> {
+			oldPasswordField.wipe();
+			newPasswordController.passwordField.wipe();
+			newPasswordController.reenterField.wipe();
+		});
 	}
 
 	@FXML
@@ -78,10 +78,8 @@ public class ChangePasswordController implements FxController {
 	@FXML
 	public void finish() {
 		try {
-			//String normalizedOldPassphrase = Normalizer.normalize(oldPasswordField.getCharacters(), Normalizer.Form.NFC);
-			//String normalizedNewPassphrase = Normalizer.normalize(newPassword.get(), Normalizer.Form.NFC);
-			CharSequence oldPassphrase = oldPasswordField.getCharacters(); // TODO verify: is this already NFC-normalized?
-			CharSequence newPassphrase = newPassword.get(); // TODO verify: is this already NFC-normalized?
+			CharSequence oldPassphrase = oldPasswordField.getCharacters();
+			CharSequence newPassphrase = newPasswordController.passwordField.getCharacters();
 			Path masterkeyPath = vault.getPath().resolve(MASTERKEY_FILENAME);
 			byte[] oldMasterkeyBytes = Files.readAllBytes(masterkeyPath);
 			byte[] newMasterkeyBytes = masterkeyFileAccess.changePassphrase(oldMasterkeyBytes, oldPassphrase, newPassphrase);
@@ -89,8 +87,8 @@ public class ChangePasswordController implements FxController {
 			Files.move(masterkeyPath, backupKeyPath, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
 			Files.write(masterkeyPath, newMasterkeyBytes, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE);
 			LOG.info("Successfully changed password for {}", vault.getDisplayName());
-			window.close();
 			updatePasswordInSystemkeychain();
+			window.close();
 		} catch (InvalidPassphraseException e) {
 			Animations.createShakeWindowAnimation(window).play();
 			oldPasswordField.selectAll();
@@ -104,7 +102,7 @@ public class ChangePasswordController implements FxController {
 	private void updatePasswordInSystemkeychain() {
 		if (keychain.isSupported()) {
 			try {
-				keychain.changePassphrase(vault.getId(), CharBuffer.wrap(newPassword.get()));
+				keychain.changePassphrase(vault.getId(), newPasswordController.passwordField.getCharacters());
 				LOG.info("Successfully updated password in system keychain for {}", vault.getDisplayName());
 			} catch (KeychainAccessException e) {
 				LOG.error("Failed to update password in system keychain.", e);

main/ui/src/main/java/org/cryptomator/ui/changepassword/ChangePasswordModule.java

@@ -5,10 +5,10 @@ import dagger.Module;
 import dagger.Provides;
 import dagger.multibindings.IntoMap;
 import org.cryptomator.ui.common.DefaultSceneFactory;
-import org.cryptomator.ui.common.FxmlLoaderFactory;
 import org.cryptomator.ui.common.FxController;
 import org.cryptomator.ui.common.FxControllerKey;
 import org.cryptomator.ui.common.FxmlFile;
+import org.cryptomator.ui.common.FxmlLoaderFactory;
 import org.cryptomator.ui.common.FxmlScene;
 import org.cryptomator.ui.common.NewPasswordController;
 import org.cryptomator.ui.common.PasswordStrengthUtil;
@@ -16,8 +16,6 @@ import org.cryptomator.ui.common.StageFactory;
 
 import javax.inject.Named;
 import javax.inject.Provider;
-import javafx.beans.property.ObjectProperty;
-import javafx.beans.property.SimpleObjectProperty;
 import javafx.scene.Scene;
 import javafx.stage.Modality;
 import javafx.stage.Stage;
@@ -27,13 +25,6 @@ import java.util.ResourceBundle;
 @Module
 abstract class ChangePasswordModule {
 
-	@Provides
-	@ChangePasswordScoped
-	@Named("newPassword")
-	static ObjectProperty<CharSequence> provideNewPasswordProperty() {
-		return new SimpleObjectProperty<>("");
-	}
-
 	@Provides
 	@ChangePasswordWindow
 	@ChangePasswordScoped
@@ -71,8 +62,8 @@ abstract class ChangePasswordModule {
 	@Provides
 	@IntoMap
 	@FxControllerKey(NewPasswordController.class)
-	static FxController provideNewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater, @Named("newPassword") ObjectProperty<CharSequence> password) {
-		return new NewPasswordController(resourceBundle, strengthRater, password);
+	static FxController provideNewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater) {
+		return new NewPasswordController(resourceBundle, strengthRater);
 	}
 
 }

main/ui/src/main/java/org/cryptomator/ui/common/NewPasswordController.java

@@ -8,7 +8,8 @@ import javafx.beans.Observable;
 import javafx.beans.binding.Bindings;
 import javafx.beans.binding.BooleanBinding;
 import javafx.beans.property.IntegerProperty;
-import javafx.beans.property.ObjectProperty;
+import javafx.beans.property.ReadOnlyBooleanProperty;
+import javafx.beans.property.ReadOnlyBooleanWrapper;
 import javafx.beans.property.SimpleIntegerProperty;
 import javafx.fxml.FXML;
 import javafx.scene.control.Label;
@@ -18,8 +19,8 @@ public class NewPasswordController implements FxController {
 
 	private final ResourceBundle resourceBundle;
 	private final PasswordStrengthUtil strengthRater;
-	private final ObjectProperty<CharSequence> password;
 	private final IntegerProperty passwordStrength = new SimpleIntegerProperty(-1);
+	private final ReadOnlyBooleanWrapper passwordsMatchAndSufficient = new ReadOnlyBooleanWrapper();
 
 	public NiceSecurePasswordField passwordField;
 	public NiceSecurePasswordField reenterField;
@@ -31,10 +32,9 @@ public class NewPasswordController implements FxController {
 	public FontAwesome5IconView passwordMatchCheckmark;
 	public FontAwesome5IconView passwordMatchCross;
 
-	public NewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater, ObjectProperty<CharSequence> password) {
+	public NewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater) {
 		this.resourceBundle = resourceBundle;
 		this.strengthRater = strengthRater;
-		this.password = password;
 	}
 
 	@FXML
@@ -44,7 +44,7 @@ public class NewPasswordController implements FxController {
 		passwordStrengthLabel.graphicProperty().bind(Bindings.createObjectBinding(this::getIconViewForPasswordStrengthLabel, passwordField.textProperty(), passwordStrength));
 		passwordStrengthLabel.textProperty().bind(EasyBind.map(passwordStrength, strengthRater::getStrengthDescription));
 
-		BooleanBinding passwordsMatch = Bindings.createBooleanBinding(this::hasSamePasswordInBothFields, passwordField.textProperty(), reenterField.textProperty());
+		BooleanBinding passwordsMatch = Bindings.createBooleanBinding(this::passwordFieldsMatch, passwordField.textProperty(), reenterField.textProperty());
 		BooleanBinding reenterFieldNotEmpty = reenterField.textProperty().isNotEmpty();
 		passwordMatchLabel.visibleProperty().bind(reenterFieldNotEmpty);
 		passwordMatchLabel.graphicProperty().bind(Bindings.when(passwordsMatch.and(reenterFieldNotEmpty)).then(passwordMatchCheckmark).otherwise(passwordMatchCross));
@@ -54,6 +54,7 @@ public class NewPasswordController implements FxController {
 		reenterField.textProperty().addListener(this::passwordsDidChange);
 	}
 
+
 	private FontAwesome5IconView getIconViewForPasswordStrengthLabel() {
 		if (passwordField.getCharacters().length() == 0) {
 			return null;
@@ -67,17 +68,19 @@ public class NewPasswordController implements FxController {
 	}
 
 	private void passwordsDidChange(@SuppressWarnings("unused") Observable observable) {
-		if (hasSamePasswordInBothFields() && strengthRater.fulfillsMinimumRequirements(passwordField.getCharacters())) {
-			password.set(passwordField.getCharacters());
-		} else {
-			password.set("");
+		if (passwordFieldsMatch() && strengthRater.fulfillsMinimumRequirements(passwordField.getCharacters())) {
+			passwordsMatchAndSufficient.setValue(true);
 		}
 	}
 
-	private boolean hasSamePasswordInBothFields() {
+	private boolean passwordFieldsMatch() {
 		return CharSequence.compare(passwordField.getCharacters(), reenterField.getCharacters()) == 0;
 	}
 
+	public ReadOnlyBooleanProperty passwordsMatchAndSufficientProperty() {
+		return passwordsMatchAndSufficient.getReadOnlyProperty();
+	}
+
 	/* Getter/Setter */
 
 	public IntegerProperty passwordStrengthProperty() {

main/ui/src/main/java/org/cryptomator/ui/controls/SecurePasswordField.java

@@ -123,7 +123,7 @@ public class SecurePasswordField extends TextField {
 	}
 
 	private void updateCapsLocked() {
-		// AWT code needed until https://bugs.openjdk.java.net/browse/JDK-8090882 is closed:
+		//TODO: fixed in JavaFX 17. AWT code needed until update (see https://bugs.openjdk.java.net/browse/JDK-8259680)
 		capsLocked.set(isFocused() && Toolkit.getDefaultToolkit().getLockingKeyState(java.awt.event.KeyEvent.VK_CAPS_LOCK));
 	}
 

main/ui/src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyModule.java

@@ -6,10 +6,10 @@ import dagger.Provides;
 import dagger.multibindings.IntoMap;
 import org.cryptomator.common.vaults.Vault;
 import org.cryptomator.ui.common.DefaultSceneFactory;
-import org.cryptomator.ui.common.FxmlLoaderFactory;
 import org.cryptomator.ui.common.FxController;
 import org.cryptomator.ui.common.FxControllerKey;
 import org.cryptomator.ui.common.FxmlFile;
+import org.cryptomator.ui.common.FxmlLoaderFactory;
 import org.cryptomator.ui.common.FxmlScene;
 import org.cryptomator.ui.common.NewPasswordController;
 import org.cryptomator.ui.common.PasswordStrengthUtil;
@@ -17,8 +17,6 @@ import org.cryptomator.ui.common.StageFactory;
 
 import javax.inject.Named;
 import javax.inject.Provider;
-import javafx.beans.property.ObjectProperty;
-import javafx.beans.property.SimpleObjectProperty;
 import javafx.beans.property.SimpleStringProperty;
 import javafx.beans.property.StringProperty;
 import javafx.scene.Scene;
@@ -56,14 +54,6 @@ abstract class RecoveryKeyModule {
 		return new SimpleStringProperty();
 	}
 
-	@Provides
-	@RecoveryKeyScoped
-	@Named("newPassword")
-	static ObjectProperty<CharSequence> provideNewPasswordProperty() {
-		return new SimpleObjectProperty<>("");
-	}
-
-
 	// ------------------
 
 	@Provides
@@ -126,8 +116,8 @@ abstract class RecoveryKeyModule {
 	@Provides
 	@IntoMap
 	@FxControllerKey(NewPasswordController.class)
-	static FxController provideNewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater, @Named("newPassword") ObjectProperty<CharSequence> password) {
-		return new NewPasswordController(resourceBundle, strengthRater, password);
+	static FxController provideNewPasswordController(ResourceBundle resourceBundle, PasswordStrengthUtil strengthRater) {
+		return new NewPasswordController(resourceBundle, strengthRater);
 	}
 
 }

main/ui/src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java

@@ -6,14 +6,12 @@ import org.cryptomator.ui.common.ErrorComponent;
 import org.cryptomator.ui.common.FxController;
 import org.cryptomator.ui.common.FxmlFile;
 import org.cryptomator.ui.common.FxmlScene;
+import org.cryptomator.ui.common.NewPasswordController;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.inject.Inject;
-import javax.inject.Named;
-import javafx.beans.binding.Bindings;
-import javafx.beans.binding.BooleanBinding;
-import javafx.beans.property.ObjectProperty;
+import javafx.beans.property.ReadOnlyBooleanProperty;
 import javafx.beans.property.StringProperty;
 import javafx.concurrent.Task;
 import javafx.fxml.FXML;
@@ -32,21 +30,19 @@ public class RecoveryKeyResetPasswordController implements FxController {
 	private final RecoveryKeyFactory recoveryKeyFactory;
 	private final ExecutorService executor;
 	private final StringProperty recoveryKey;
-	private final ObjectProperty<CharSequence> newPassword;
 	private final Lazy<Scene> recoverScene;
-	private final BooleanBinding invalidNewPassword;
 	private final ErrorComponent.Builder errorComponent;
 
+	public NewPasswordController newPasswordController;
+
 	@Inject
-	public RecoveryKeyResetPasswordController(@RecoveryKeyWindow Stage window, @RecoveryKeyWindow Vault vault, RecoveryKeyFactory recoveryKeyFactory, ExecutorService executor, @RecoveryKeyWindow StringProperty recoveryKey, @Named("newPassword") ObjectProperty<CharSequence> newPassword, @FxmlScene(FxmlFile.RECOVERYKEY_RECOVER) Lazy<Scene> recoverScene, ErrorComponent.Builder errorComponent) {
+	public RecoveryKeyResetPasswordController(@RecoveryKeyWindow Stage window, @RecoveryKeyWindow Vault vault, RecoveryKeyFactory recoveryKeyFactory, ExecutorService executor, @RecoveryKeyWindow StringProperty recoveryKey, @FxmlScene(FxmlFile.RECOVERYKEY_RECOVER) Lazy<Scene> recoverScene, ErrorComponent.Builder errorComponent) {
 		this.window = window;
 		this.vault = vault;
 		this.recoveryKeyFactory = recoveryKeyFactory;
 		this.executor = executor;
 		this.recoveryKey = recoveryKey;
-		this.newPassword = newPassword;
 		this.recoverScene = recoverScene;
-		this.invalidNewPassword = Bindings.createBooleanBinding(this::isInvalidNewPassword, newPassword);
 		this.errorComponent = errorComponent;
 	}
 
@@ -81,7 +77,7 @@ public class RecoveryKeyResetPasswordController implements FxController {
 
 		@Override
 		protected Void call() throws IOException, IllegalArgumentException {
-			recoveryKeyFactory.resetPasswordWithRecoveryKey(vault.getPath(), recoveryKey.get(), newPassword.get());
+			recoveryKeyFactory.resetPasswordWithRecoveryKey(vault.getPath(), recoveryKey.get(), newPasswordController.passwordField.getCharacters());
 			return null;
 		}
 
@@ -89,11 +85,12 @@ public class RecoveryKeyResetPasswordController implements FxController {
 
 	/* Getter/Setter */
 
-	public BooleanBinding invalidNewPasswordProperty() {
-		return invalidNewPassword;
+	public ReadOnlyBooleanProperty validPasswordProperty() {
+		return newPasswordController.passwordsMatchAndSufficientProperty();
 	}
 
-	public boolean isInvalidNewPassword() {
-		return newPassword.get() == null || newPassword.get().length() == 0;
+	public boolean isValidPassword() {
+		return newPasswordController.passwordsMatchAndSufficientProperty().get();
 	}
+
 }

main/ui/src/main/resources/fxml/addvault_new_password.fxml

@@ -23,7 +23,7 @@
 		<Insets topRightBottomLeft="24"/>
 	</padding>
 	<children>
-		<fx:include source="/fxml/new_password.fxml"/>
+		<fx:include fx:id="newPasswordScene" source="/fxml/new_password.fxml"/>
 
 		<Region VBox.vgrow="ALWAYS"/>
 

main/ui/src/main/resources/fxml/changepassword.fxml

@@ -25,7 +25,7 @@
 
 		<Region prefHeight="12" VBox.vgrow="NEVER"/>
 
-		<fx:include source="/fxml/new_password.fxml"/>
+		<fx:include fx:id="newPassword" source="/fxml/new_password.fxml"/>
 
 		<CheckBox fx:id="finalConfirmationCheckbox" text="%changepassword.finalConfirmation" wrapText="true"/>
 

main/ui/src/main/resources/fxml/recoverykey_reset_password.fxml

@@ -17,7 +17,7 @@
 		<Insets topRightBottomLeft="12"/>
 	</padding>
 	<children>
-		<fx:include source="/fxml/new_password.fxml"/>
+		<fx:include fx:id="newPassword" source="/fxml/new_password.fxml"/>
 
 		<Region VBox.vgrow="ALWAYS"/>
 
@@ -25,7 +25,7 @@
 			<ButtonBar buttonMinWidth="120" buttonOrder="B+I">
 				<buttons>
 					<Button text="%generic.button.back" ButtonBar.buttonData="BACK_PREVIOUS" cancelButton="true" onAction="#back"/>
-					<Button text="%generic.button.done" ButtonBar.buttonData="FINISH" defaultButton="true" onAction="#done" disable="${controller.invalidNewPassword}"/>
+					<Button text="%generic.button.done" ButtonBar.buttonData="FINISH" defaultButton="true" onAction="#done" disable="${!controller.validPassword}"/>
 				</buttons>
 			</ButtonBar>
 		</VBox>