From cd5c55aad72123d34ace4c3b9a669830d5003cf2 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Tue, 13 Apr 2021 11:22:42 +0200 Subject: [PATCH] Refactor lock/unlock workflows: * don't set vault state on successful lock workflow * improved error handling --- .../org/cryptomator/ui/lock/LockWorkflow.java | 23 +++---- .../cryptomator/ui/unlock/UnlockWorkflow.java | 60 ++++++++----------- 2 files changed, 39 insertions(+), 44 deletions(-) diff --git a/main/ui/src/main/java/org/cryptomator/ui/lock/LockWorkflow.java b/main/ui/src/main/java/org/cryptomator/ui/lock/LockWorkflow.java index 673b74641..fecbe42c4 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/lock/LockWorkflow.java +++ b/main/ui/src/main/java/org/cryptomator/ui/lock/LockWorkflow.java @@ -4,6 +4,7 @@ import dagger.Lazy; import org.cryptomator.common.vaults.Vault; import org.cryptomator.common.vaults.VaultState; import org.cryptomator.common.vaults.Volume; +import org.cryptomator.ui.common.ErrorComponent; import org.cryptomator.ui.common.FxmlFile; import org.cryptomator.ui.common.FxmlScene; import org.cryptomator.ui.common.UserInteractionLock; @@ -35,14 +36,16 @@ public class LockWorkflow extends Task { private final UserInteractionLock forceLockDecisionLock; private final Lazy lockForcedScene; private final Lazy lockFailedScene; + private final ErrorComponent.Builder errorComponent; @Inject - public LockWorkflow(@LockWindow Stage lockWindow, @LockWindow Vault vault, UserInteractionLock forceLockDecisionLock, @FxmlScene(FxmlFile.LOCK_FORCED) Lazy lockForcedScene, @FxmlScene(FxmlFile.LOCK_FAILED) Lazy lockFailedScene) { + public LockWorkflow(@LockWindow Stage lockWindow, @LockWindow Vault vault, UserInteractionLock forceLockDecisionLock, @FxmlScene(FxmlFile.LOCK_FORCED) Lazy lockForcedScene, @FxmlScene(FxmlFile.LOCK_FAILED) Lazy lockFailedScene, ErrorComponent.Builder errorComponent) { this.lockWindow = lockWindow; this.vault = vault; this.forceLockDecisionLock = forceLockDecisionLock; this.lockForcedScene = lockForcedScene; this.lockFailedScene = lockFailedScene; + this.errorComponent = errorComponent; } @Override @@ -77,23 +80,23 @@ public class LockWorkflow extends Task { return forceLockDecisionLock.awaitInteraction(); } - @Override - protected void scheduled() { - vault.stateProperty().transition(VaultState.Value.UNLOCKED, VaultState.Value.PROCESSING); - } - @Override protected void succeeded() { LOG.info("Lock of {} succeeded.", vault.getDisplayName()); - vault.stateProperty().transition(VaultState.Value.PROCESSING, VaultState.Value.LOCKED); + //DO NOT SET VAULT STATE HERE, this is done by the vault internally } @Override protected void failed() { - LOG.warn("Failed to lock {}.", vault.getDisplayName()); + final var throwable = super.getException(); + LOG.warn("Lock of {} failed.", vault.getDisplayName(), throwable); vault.stateProperty().transition(VaultState.Value.PROCESSING, VaultState.Value.UNLOCKED); - lockWindow.setScene(lockFailedScene.get()); - lockWindow.show(); + if (throwable instanceof Volume.VolumeException) { + lockWindow.setScene(lockFailedScene.get()); + lockWindow.show(); + } else { + errorComponent.cause(throwable).window(lockWindow).build().showErrorScene(); + } } @Override diff --git a/main/ui/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java b/main/ui/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java index 08ae1fd4c..2815ad3c4 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java +++ b/main/ui/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java @@ -73,22 +73,15 @@ public class UnlockWorkflow extends Task { this.successScene = successScene; this.invalidMountPointScene = invalidMountPointScene; this.errorComponent = errorComponent; - - setOnFailed(event -> { - Throwable throwable = event.getSource().getException(); - if (throwable instanceof InvalidMountPointException e) { - handleInvalidMountPoint(e); - } else { - handleGenericError(throwable); - } - }); } @Override protected Boolean call() throws InterruptedException, IOException, VolumeException, InvalidMountPointException { try { if (attemptUnlock()) { - handleSuccess(); + if (savePassword.get()) { + savePasswordToSystemkeychain(); //savePassword will be wiped on method return, so it must be set here + } return true; } else { cancel(false); // set Tasks state to cancelled @@ -131,24 +124,6 @@ public class UnlockWorkflow extends Task { return passwordEntryLock.awaitInteraction(); } - private void handleSuccess() { - LOG.info("Unlock of '{}' succeeded.", vault.getDisplayName()); - if (savePassword.get()) { - savePasswordToSystemkeychain(); - } - switch (vault.getVaultSettings().actionAfterUnlock().get()) { - case ASK -> Platform.runLater(() -> { - window.setScene(successScene.get()); - window.show(); - }); - case REVEAL -> { - Platform.runLater(window::close); - vaultService.reveal(vault); - } - case IGNORE -> Platform.runLater(window::close); - } - } - private void savePasswordToSystemkeychain() { if (keychain.isSupported()) { try { @@ -193,7 +168,7 @@ public class UnlockWorkflow extends Task { private void handleGenericError(Throwable e) { LOG.error("Unlock failed for technical reasons.", e); - errorComponent.cause(e).window(window).returnToScene(window.getScene()).build().showErrorScene(); + errorComponent.cause(e).window(window).build().showErrorScene(); } private void wipePassword(char[] pw) { @@ -202,23 +177,40 @@ public class UnlockWorkflow extends Task { } } - @Override - protected void scheduled() { - vault.stateProperty().transition(VaultState.Value.LOCKED, VaultState.Value.PROCESSING); - } - @Override protected void succeeded() { + LOG.info("Unlock of '{}' succeeded.", vault.getDisplayName()); + + switch (vault.getVaultSettings().actionAfterUnlock().get()) { + case ASK -> Platform.runLater(() -> { + window.setScene(successScene.get()); + window.show(); + }); + case REVEAL -> { + Platform.runLater(window::close); + vaultService.reveal(vault); + } + case IGNORE -> Platform.runLater(window::close); + } + vault.stateProperty().transition(VaultState.Value.PROCESSING, VaultState.Value.UNLOCKED); } @Override protected void failed() { + LOG.info("Unlock of '{}' failed.", vault.getDisplayName()); + Throwable throwable = super.getException(); + if (throwable instanceof InvalidMountPointException e) { + handleInvalidMountPoint(e); + } else { + handleGenericError(throwable); + } vault.stateProperty().transition(VaultState.Value.PROCESSING, VaultState.Value.LOCKED); } @Override protected void cancelled() { + LOG.debug("Unlock of '{}' canceled.", vault.getDisplayName()); vault.stateProperty().transition(VaultState.Value.PROCESSING, VaultState.Value.LOCKED); }