From 70baa4d09f9b5ba1e5b0dc75212098a7de35eb1d Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Tue, 27 Apr 2021 17:41:28 +0200 Subject: [PATCH] Rework sanitizer workflow: * select set of performed HealthChecks prior to loading masterkey * two predefined healthcheck sets and custom one * start sequential execution of checks after successful masterkey loading * removed vault config info * destroy masterkey on window close, on cancellation, on check finish --- .../ui/health/CheckController.java | 33 +++---- .../ui/health/HealthCheckModule.java | 45 +++++---- .../ui/health/HealthCheckTask.java | 9 +- .../ui/health/StartController.java | 94 ++++++++++++++++--- .../src/main/resources/fxml/health_check.fxml | 8 +- .../src/main/resources/fxml/health_start.fxml | 23 ++++- 6 files changed, 144 insertions(+), 68 deletions(-) diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java b/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java index 0b4b4a4f0..40287a6ca 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java @@ -1,38 +1,33 @@ package org.cryptomator.ui.health; import com.tobiasdiez.easybind.EasyBind; -import com.tobiasdiez.easybind.EasyBinding; import com.tobiasdiez.easybind.optional.OptionalBinding; import dagger.Lazy; import org.cryptomator.cryptofs.VaultConfig; import org.cryptomator.cryptofs.health.api.DiagnosticResult; -import org.cryptomator.cryptofs.health.api.HealthCheck; import org.cryptomator.ui.common.FxController; import javax.inject.Inject; -import javafx.beans.binding.Binding; import javafx.beans.binding.Bindings; -import javafx.beans.binding.BooleanBinding; import javafx.beans.binding.BooleanExpression; import javafx.beans.binding.ObjectBinding; import javafx.beans.property.ObjectProperty; -import javafx.beans.property.ReadOnlyObjectProperty; import javafx.collections.FXCollections; import javafx.collections.ObservableList; import javafx.concurrent.Worker; import javafx.fxml.FXML; import javafx.scene.control.ListView; -import javafx.stage.Stage; import java.util.Collection; import java.util.Objects; import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicReference; @HealthCheckScoped public class CheckController implements FxController { - private final Stage window; private final VaultConfig vaultConfig; + private final Runnable masterkeyDestructor; private final ExecutorService executor; private final ObjectProperty selectedTask; private final ObservableList tasks; @@ -46,10 +41,10 @@ public class CheckController implements FxController { @Inject - public CheckController(@HealthCheckWindow Stage window, AtomicReference vaultConfigRef, Lazy> tasks, ExecutorService executor, ObjectProperty selectedTask) { - this.window = window; + public CheckController(AtomicReference vaultConfigRef, Runnable masterkeyDestructor, Lazy> tasks, ObjectProperty selectedTask) { this.vaultConfig = Objects.requireNonNull(vaultConfigRef.get()); - this.executor = executor; + this.masterkeyDestructor = masterkeyDestructor; + this.executor = Executors.newSingleThreadExecutor(); this.selectedTask = selectedTask; this.selectedResults = Bindings.createObjectBinding(this::getSelectedResults, selectedTask); this.selectedTaskState = EasyBind.wrapNullable(selectedTask).mapObservable(HealthCheckTask::stateProperty); @@ -65,19 +60,21 @@ public class CheckController implements FxController { resultsListView.itemsProperty().bind(selectedResults); resultsListView.setCellFactory(ignored -> new ResultListCell()); selectedTask.bind(checksListView.getSelectionModel().selectedItemProperty()); + checksListView.getSelectionModel().select(0); + + tasks.forEach(task -> executor.execute(task)); + + //ensure that at the end the masterkey is destroyed + executor.submit(masterkeyDestructor); } - @FXML - public void runCheck() { - assert selectedTask.get() != null; - executor.execute(selectedTask.get()); - } @FXML public void cancelCheck() { - assert selectedTask.get() != null; - assert selectedTask.get().isRunning(); - selectedTask.get().cancel(); + //assert selectedTask.get().isRunning(); Replace with something like executor.isRunning() + executor.shutdownNow(); + masterkeyDestructor.run(); } + /* Getter/Setter */ public VaultConfig getVaultConfig() { diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java index 37915edd6..886ec3d03 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java @@ -3,7 +3,6 @@ package org.cryptomator.ui.health; import dagger.Binds; import dagger.Module; import dagger.Provides; -import dagger.multibindings.ElementsIntoSet; import dagger.multibindings.IntoMap; import org.cryptomator.common.vaults.Vault; import org.cryptomator.cryptofs.VaultConfig; @@ -23,16 +22,15 @@ import org.cryptomator.ui.mainwindow.MainWindow; import javax.inject.Provider; import javafx.beans.property.ObjectProperty; import javafx.beans.property.SimpleObjectProperty; -import javafx.beans.value.ChangeListener; import javafx.scene.Scene; import javafx.stage.Modality; import javafx.stage.Stage; import java.security.SecureRandom; +import java.util.ArrayList; import java.util.Collection; import java.util.Map; import java.util.Optional; import java.util.ResourceBundle; -import java.util.Set; import java.util.concurrent.atomic.AtomicReference; @Module(subcomponents = {KeyLoadingComponent.class}) @@ -50,12 +48,31 @@ abstract class HealthCheckModule { return new AtomicReference<>(); } + @Provides + @HealthCheckScoped + static Runnable provideMasterkeyDestructor(AtomicReference masterkeyRef) { + return () -> Optional.ofNullable(masterkeyRef.getAndSet(null)).ifPresent(Masterkey::destroy); + } + @Provides @HealthCheckScoped static AtomicReference provideVaultConfigRef() { return new AtomicReference<>(); } + @Provides + @HealthCheckScoped + static Collection provideSelectedHealthChecks() { + return new ArrayList<>(); + } + + /* Only inject with Lazy-Wrapper!*/ + @Provides + @HealthCheckScoped + static Collection provideSelectedHealthCheckTasks(Collection selectedHealthChecks, @HealthCheckWindow Vault vault, AtomicReference masterkeyRef, AtomicReference vaultConfigRef, SecureRandom csprng) { + return selectedHealthChecks.stream().map(check -> new HealthCheckTask(vault.getPath(), vaultConfigRef.get(), masterkeyRef.get(), csprng, check)).toList(); //TODO: for every task clone the masterkey and destroy it afterwards + } + @Provides @HealthCheckWindow @HealthCheckScoped @@ -63,12 +80,6 @@ abstract class HealthCheckModule { return compBuilder.vault(vault).window(window).build().keyloadingStrategy(); } - @Provides - @HealthCheckScoped - static Collection provideHealthCheckTasks(@HealthCheckWindow Vault vault, AtomicReference masterkeyRef, AtomicReference vaultConfigRef, SecureRandom csprng) { - return HealthCheck.allChecks().stream().map(check -> new HealthCheckTask(vault.getPath(), vaultConfigRef.get(), masterkeyRef.get(), csprng, check)).toList(); - } - @Provides @HealthCheckWindow @HealthCheckScoped @@ -79,24 +90,18 @@ abstract class HealthCheckModule { @Provides @HealthCheckWindow @HealthCheckScoped - static Stage provideStage(StageFactory factory, @MainWindow Stage owner, ResourceBundle resourceBundle, ChangeListener showingListener) { + static Stage provideStage(StageFactory factory, @MainWindow Stage owner, ResourceBundle resourceBundle, Runnable masterkeyDestructor) { Stage stage = factory.create(); stage.setTitle(resourceBundle.getString("health.title")); stage.setResizable(true); stage.initModality(Modality.WINDOW_MODAL); stage.initOwner(owner); - stage.showingProperty().addListener(showingListener); - return stage; - } - - @Provides - @HealthCheckScoped - static ChangeListener provideWindowShowingChangeListener(AtomicReference masterkey) { - return (observable, wasShowing, isShowing) -> { + stage.showingProperty().addListener((observable, wasShowing, isShowing) -> { //TODO: should we use showingProperty or onCloseRequest if (!isShowing) { - Optional.ofNullable(masterkey.getAndSet(null)).ifPresent(Masterkey::destroy); + masterkeyDestructor.run(); } - }; + }); + return stage; } @Provides diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java index ba3b5a85d..b345352ab 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java @@ -8,17 +8,13 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javafx.application.Platform; -import javafx.beans.binding.BooleanBinding; -import javafx.beans.property.ReadOnlyBooleanProperty; import javafx.collections.FXCollections; import javafx.collections.ObservableList; import javafx.concurrent.Task; -import javafx.concurrent.Worker; import java.nio.file.Path; import java.security.SecureRandom; import java.util.Objects; import java.util.concurrent.CancellationException; -import java.util.function.Consumer; class HealthCheckTask extends Task { @@ -42,8 +38,9 @@ class HealthCheckTask extends Task { @Override protected Void call() { - try (var cryptor = vaultConfig.getCipherCombo().getCryptorProvider(csprng).withKey(masterkey)) { - check.check(vaultPath, vaultConfig, masterkey, cryptor, result -> { + try (var masterkeyClone = masterkey.clone(); // + var cryptor = vaultConfig.getCipherCombo().getCryptorProvider(csprng).withKey(masterkeyClone)) { + check.check(vaultPath, vaultConfig, masterkeyClone, cryptor, result -> { //TODO: why using both masterkey and Cryptor ?? if (isCancelled()) { throw new CancellationException(); } diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/StartController.java b/main/ui/src/main/java/org/cryptomator/ui/health/StartController.java index 848be8a9d..170493c97 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/StartController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/StartController.java @@ -5,12 +5,13 @@ import org.cryptomator.common.vaults.Vault; import org.cryptomator.cryptofs.VaultConfig; import org.cryptomator.cryptofs.VaultConfigLoadException; import org.cryptomator.cryptofs.VaultKeyInvalidException; +import org.cryptomator.cryptofs.health.api.HealthCheck; +import org.cryptomator.cryptofs.health.dirid.DirIdCheck; import org.cryptomator.cryptolib.api.Masterkey; import org.cryptomator.cryptolib.api.MasterkeyLoadingFailedException; import org.cryptomator.ui.common.FxController; import org.cryptomator.ui.common.FxmlFile; import org.cryptomator.ui.common.FxmlScene; -import org.cryptomator.ui.fxapp.FxApplication; import org.cryptomator.ui.keyloading.KeyLoadingStrategy; import org.cryptomator.ui.unlock.UnlockCancelledException; import org.slf4j.Logger; @@ -18,11 +19,22 @@ import org.slf4j.LoggerFactory; import javax.inject.Inject; import javafx.application.Platform; +import javafx.beans.binding.BooleanBinding; +import javafx.beans.property.BooleanProperty; +import javafx.beans.property.SimpleBooleanProperty; +import javafx.collections.FXCollections; import javafx.fxml.FXML; import javafx.scene.Scene; +import javafx.scene.control.ListView; +import javafx.scene.control.RadioButton; +import javafx.scene.control.ToggleGroup; +import javafx.scene.control.cell.CheckBoxListCell; import javafx.stage.Stage; -import java.net.URI; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.atomic.AtomicReference; @@ -32,22 +44,58 @@ public class StartController implements FxController { private static final Logger LOG = LoggerFactory.getLogger(StartController.class); private final Stage window; + private final Collection availableChecks; + private final Map availableCheckListSelectProperties; private final Optional unverifiedVaultConfig; private final KeyLoadingStrategy keyLoadingStrategy; private final ExecutorService executor; private final AtomicReference masterkeyRef; private final AtomicReference vaultConfigRef; + private final Collection selectedChecks; private final Lazy checkScene; + /* FXML */ + public ListView availableChecksList; + public RadioButton customCheckSetButton; + public RadioButton quickCheckSetButton; + public RadioButton fullCheckSetButton; + public ToggleGroup checksSetToggleGroup; + @Inject - public StartController(@HealthCheckWindow Vault vault, @HealthCheckWindow Stage window, @HealthCheckWindow KeyLoadingStrategy keyLoadingStrategy, ExecutorService executor, AtomicReference masterkeyRef, AtomicReference vaultConfigRef, @FxmlScene(FxmlFile.HEALTH_CHECK) Lazy checkScene) { + public StartController(@HealthCheckWindow Vault vault, @HealthCheckWindow Stage window, @HealthCheckWindow KeyLoadingStrategy keyLoadingStrategy, ExecutorService executor, AtomicReference masterkeyRef, AtomicReference vaultConfigRef, Collection selectedChecks, @FxmlScene(FxmlFile.HEALTH_CHECK) Lazy checkScene) { this.window = window; - this.unverifiedVaultConfig = vault.getUnverifiedVaultConfig(); + this.unverifiedVaultConfig = vault.getUnverifiedVaultConfig(); //TODO: prevent workflow at all, if the vault config is emtpy this.keyLoadingStrategy = keyLoadingStrategy; this.executor = executor; this.masterkeyRef = masterkeyRef; this.vaultConfigRef = vaultConfigRef; + this.selectedChecks = selectedChecks; this.checkScene = checkScene; + this.availableChecks = HealthCheck.allChecks(); + this.availableCheckListSelectProperties = new HashMap<>(); + + availableChecks.forEach(check -> availableCheckListSelectProperties.put(check, new SimpleBooleanProperty(false))); + } + + public void initialize() { + availableChecksList.setItems(FXCollections.observableList(availableChecks.stream().toList())); + availableChecksList.setCellFactory(CheckBoxListCell.forListView(availableCheckListSelectProperties::get)); + availableChecksList.setEditable(false); + + var availableCheckIds = availableChecks.stream().map(HealthCheck::identifier).toList(); + + if (PredefinedCheckSet.QUICK.getCheckIds().stream().anyMatch(checkId -> !availableCheckIds.contains(checkId))) { + quickCheckSetButton.setVisible(false); + quickCheckSetButton.setManaged(false); + } + if (PredefinedCheckSet.FULL.getCheckIds().stream().anyMatch(checkId -> !availableCheckIds.contains(checkId))) { + fullCheckSetButton.setVisible(false); + fullCheckSetButton.setManaged(false); + } + + quickCheckSetButton.setUserData(PredefinedCheckSet.QUICK); + fullCheckSetButton.setUserData(PredefinedCheckSet.FULL); + customCheckSetButton.setUserData(PredefinedCheckSet.CUSTOM); } @FXML @@ -58,6 +106,15 @@ public class StartController implements FxController { @FXML public void next() { + switch ((PredefinedCheckSet) checksSetToggleGroup.getSelectedToggle().getUserData()) { + case FULL -> selectedChecks.addAll(availableChecks); + case QUICK -> selectedChecks.addAll(availableChecks.stream().filter(check -> PredefinedCheckSet.QUICK.getCheckIds().contains(check.identifier())).toList()); + case CUSTOM -> availableCheckListSelectProperties.forEach((check, selected) -> { + if (selected.get()) { + selectedChecks.add(check); + } + }); + } LOG.trace("StartController.next()"); executor.submit(this::loadKey); } @@ -109,16 +166,27 @@ public class StartController implements FxController { return unverifiedVaultConfig.isEmpty(); } - public int getVaultVersion() { - return unverifiedVaultConfig - .map(VaultConfig.UnverifiedVaultConfig::allegedVaultVersion) - .orElse(-1); + public BooleanBinding anyCheckSetSelectedProperty() { + return checksSetToggleGroup.selectedToggleProperty().isNotNull(); } - public String getKeyId() { - return unverifiedVaultConfig - .map(VaultConfig.UnverifiedVaultConfig::getKeyId) - .map(URI::toString) - .orElse(null); + public boolean isAnyCheckSetSelected() { + return anyCheckSetSelectedProperty().get(); + } + + enum PredefinedCheckSet { + QUICK(DirIdCheck.class.getCanonicalName()), //TODO: get identifier via static method? + FULL(DirIdCheck.class.getCanonicalName()), + CUSTOM(); + + private Collection checkIds; + + PredefinedCheckSet(String... checkIds) { + this.checkIds = Set.of(checkIds); + } + + Collection getCheckIds() { + return checkIds; + } } } diff --git a/main/ui/src/main/resources/fxml/health_check.fxml b/main/ui/src/main/resources/fxml/health_check.fxml index d67f9e1a9..786df27e7 100644 --- a/main/ui/src/main/resources/fxml/health_check.fxml +++ b/main/ui/src/main/resources/fxml/health_check.fxml @@ -3,10 +3,9 @@ - - - + + -