diff --git a/CHANGELOG.md b/CHANGELOG.md index 06840472b..bfec9bf6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ Changes to prior versions can be found on the [Github release page](https://gith * Fixed background threads piling up with every unlock and lock of a vault * Fixed vaults showing as unlocked while still locking when locking several vaults at once, for example on quit, and one of them failed to lock * Fixed a vault showing as locked while unlocked when the tray menu was opened during its unlock +* Fixed an error window ("Vault not locked." or "Vault not unlocked.") appearing when a vault was unlocked or locked twice in quick succession, for example from a stale tray menu ### Changed * Improved usability of the trust on first use dialog ([#4295](https://github.com/cryptomator/cryptomator/pull/4295)) diff --git a/src/main/java/org/cryptomator/ui/fxapp/FxApplicationWindows.java b/src/main/java/org/cryptomator/ui/fxapp/FxApplicationWindows.java index f80b6de0e..0cf6e80c5 100644 --- a/src/main/java/org/cryptomator/ui/fxapp/FxApplicationWindows.java +++ b/src/main/java/org/cryptomator/ui/fxapp/FxApplicationWindows.java @@ -10,6 +10,7 @@ import org.cryptomator.ui.dialogs.SimpleDialog; import org.cryptomator.ui.error.ErrorComponent; import org.cryptomator.ui.eventview.EventViewComponent; import org.cryptomator.ui.lock.LockComponent; +import org.cryptomator.ui.lock.LockWorkflow; import org.cryptomator.ui.mainwindow.MainWindowComponent; import org.cryptomator.ui.notification.NotificationComponent; import org.cryptomator.ui.preferences.PreferencesComponent; @@ -170,11 +171,14 @@ public class FxApplicationWindows { public CompletionStage startUnlockWorkflow(Vault vault, @Nullable Stage owner) { return CompletableFuture.supplyAsync(() -> { - Preconditions.checkState(vault.stateProperty().transition(VaultState.Value.LOCKED, VaultState.Value.PROCESSING), "Vault not locked."); + if (!vault.stateProperty().transition(VaultState.Value.LOCKED, VaultState.Value.PROCESSING)) { + LOG.warn("Vault '{}' is {} instead of LOCKED. Skipping unlock.", vault.getDisplayName(), vault.stateProperty().get()); + return Optional.empty(); + } LOG.debug("Start unlock workflow for {}", vault.getDisplayName()); - return unlockWorkflowFactory.create(vault, owner).unlockWorkflow(); + return Optional.of(unlockWorkflowFactory.create(vault, owner).unlockWorkflow()); }, Platform::runLater) // - .thenAcceptAsync(UnlockWorkflow::run, executor).exceptionally(e -> { + .thenAcceptAsync(unlockWorkflow -> unlockWorkflow.ifPresent(UnlockWorkflow::run), executor).exceptionally(e -> { showErrorWindow(e, owner == null ? primaryStage : owner, null); return null; }); @@ -182,12 +186,14 @@ public class FxApplicationWindows { public CompletionStage startLockWorkflow(Vault vault, @Nullable Stage owner) { return CompletableFuture.supplyAsync(() -> { - Preconditions.checkState(vault.stateProperty().transition(VaultState.Value.UNLOCKED, VaultState.Value.PROCESSING), "Vault not unlocked."); + if (!vault.stateProperty().transition(VaultState.Value.UNLOCKED, VaultState.Value.PROCESSING)) { + LOG.warn("Vault '{}' is {} instead of UNLOCKED. Skipping lock.", vault.getDisplayName(), vault.stateProperty().get()); + return Optional.empty(); + } LOG.debug("Start lock workflow for {}", vault.getDisplayName()); - return lockWorkflowFactory.create(vault, owner).lockWorkflow(); + return Optional.of(lockWorkflowFactory.create(vault, owner).lockWorkflow()); }, Platform::runLater) // - .thenCompose(lockWorkflow -> CompletableFuture.runAsync(lockWorkflow, executor)) // - .exceptionally(e -> { + .thenAcceptAsync(lockWorkflow -> lockWorkflow.ifPresent(LockWorkflow::run), executor).exceptionally(e -> { showErrorWindow(e, owner == null ? primaryStage : owner, null); return null; }); diff --git a/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java b/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java index 5214d5629..ecacf0490 100644 --- a/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java +++ b/src/main/java/org/cryptomator/ui/unlock/UnlockWorkflow.java @@ -170,7 +170,7 @@ public class UnlockWorkflow extends Task { var readOnlyDialog = dialogs.prepareRetryIfReadonlyDialog(mainWindow, stage -> { stage.close(); vault.getVaultSettings().usesReadOnlyMode.set(true); - restartUnlock(); + appWindows.startUnlockWorkflow(vault, owner); }).build(); Platform.runLater(readOnlyDialog::showAndWait); @@ -179,7 +179,7 @@ public class UnlockWorkflow extends Task { private void handleExtensionSwitchedOff() { var extensionSwitchedOffDialog = dialogs.prepareFSKitExtensionSwitchedOffDialog(mainWindow, vault, stage -> { stage.close(); - restartUnlock(); + appWindows.startUnlockWorkflow(vault, owner); }).build(); Platform.runLater(() -> { @@ -188,14 +188,6 @@ public class UnlockWorkflow extends Task { }); } - private void restartUnlock() { - if (!vault.isLocked()) { - LOG.warn("Vault '{}' is {} instead of LOCKED. Aborting unlock retry.", vault.getDisplayName(), vault.getState()); - return; - } - appWindows.startUnlockWorkflow(vault, owner); - } - @Override protected void cancelled() { LOG.debug("Unlock of '{}' canceled.", vault.getDisplayName()); diff --git a/src/test/java/org/cryptomator/ui/fxapp/FxApplicationWindowsTest.java b/src/test/java/org/cryptomator/ui/fxapp/FxApplicationWindowsTest.java new file mode 100644 index 000000000..946a52603 --- /dev/null +++ b/src/test/java/org/cryptomator/ui/fxapp/FxApplicationWindowsTest.java @@ -0,0 +1,191 @@ +package org.cryptomator.ui.fxapp; + +import org.cryptomator.common.vaults.Vault; +import org.cryptomator.common.vaults.VaultState; +import org.cryptomator.ui.dialogs.Dialogs; +import org.cryptomator.ui.error.ErrorComponent; +import org.cryptomator.ui.eventview.EventViewComponent; +import org.cryptomator.ui.lock.LockComponent; +import org.cryptomator.ui.lock.LockWorkflow; +import org.cryptomator.ui.mainwindow.MainWindowComponent; +import org.cryptomator.ui.notification.NotificationComponent; +import org.cryptomator.ui.preferences.PreferencesComponent; +import org.cryptomator.ui.quit.QuitComponent; +import org.cryptomator.ui.sharevault.ShareVaultComponent; +import org.cryptomator.ui.unlock.UnlockComponent; +import org.cryptomator.ui.unlock.UnlockWorkflow; +import org.cryptomator.ui.updatereminder.UpdateReminderComponent; +import org.cryptomator.ui.vaultoptions.VaultOptionsComponent; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; +import org.mockito.Mockito; + +import javafx.application.Platform; +import javafx.stage.Stage; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.ExecutorService; + +class FxApplicationWindowsTest { + + private final Stage primaryStage = Mockito.mock(Stage.class); + private final Stage owner = Mockito.mock(Stage.class); + private final Vault vault = Mockito.mock(Vault.class); + private final UnlockComponent.Factory unlockWorkflowFactory = Mockito.mock(UnlockComponent.Factory.class); + private final UnlockWorkflow unlockWorkflow = Mockito.mock(UnlockWorkflow.class); + private final LockComponent.Factory lockWorkflowFactory = Mockito.mock(LockComponent.Factory.class); + private final LockWorkflow lockWorkflow = Mockito.mock(LockWorkflow.class); + private final ErrorComponent.Factory errorWindowFactory = Mockito.mock(ErrorComponent.Factory.class); + private final ExecutorService executor = Mockito.mock(ExecutorService.class); + private final List queuedTasks = new ArrayList<>(); + private MockedStatic platform; + private FxApplicationWindows appWindows; + + @BeforeEach + public void setup() { + platform = Mockito.mockStatic(Platform.class); + platform.when(() -> Platform.runLater(Mockito.any())).thenAnswer(invocation -> { + invocation.getArgument(0, Runnable.class).run(); + return null; + }); + Mockito.doAnswer(invocation -> queuedTasks.add(invocation.getArgument(0))).when(executor).execute(Mockito.any()); + Mockito.when(vault.getDisplayName()).thenReturn("vault"); + var unlockComponent = Mockito.mock(UnlockComponent.class); + Mockito.when(unlockComponent.unlockWorkflow()).thenReturn(unlockWorkflow); + Mockito.when(unlockWorkflowFactory.create(Mockito.eq(vault), Mockito.any())).thenReturn(unlockComponent); + var lockComponent = Mockito.mock(LockComponent.class); + Mockito.when(lockComponent.lockWorkflow()).thenReturn(lockWorkflow); + Mockito.when(lockWorkflowFactory.create(Mockito.eq(vault), Mockito.any())).thenReturn(lockComponent); + Mockito.when(errorWindowFactory.create(Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(Mockito.mock(ErrorComponent.class)); + appWindows = new FxApplicationWindows(primaryStage, Optional.empty(), Mockito.mock(MainWindowComponent.Builder.class), Mockito.mock(PreferencesComponent.Builder.class), Mockito.mock(QuitComponent.Builder.class), unlockWorkflowFactory, Mockito.mock(UpdateReminderComponent.Factory.class), lockWorkflowFactory, errorWindowFactory, Mockito.mock(VaultOptionsComponent.Factory.class), Mockito.mock(ShareVaultComponent.Factory.class), Mockito.mock(EventViewComponent.Factory.class), Mockito.mock(NotificationComponent.Factory.class), executor, Mockito.mock(Dialogs.class)); + } + + @AfterEach + public void tearDown() { + platform.close(); + } + + private VaultState givenVaultState(VaultState.Value value) { + var state = new VaultState(value); + Mockito.when(vault.stateProperty()).thenReturn(state); + return state; + } + + private void runQueuedTasks() { + var tasks = List.copyOf(queuedTasks); + queuedTasks.clear(); + tasks.forEach(Runnable::run); + } + + private static void assertCompletedNormally(CompletionStage stage) { + var future = stage.toCompletableFuture(); + Assertions.assertTrue(future.isDone()); + Assertions.assertFalse(future.isCompletedExceptionally()); + } + + @Test + public void testSecondUnlockStartWhileProcessingSkips() { + var state = givenVaultState(VaultState.Value.LOCKED); + + var firstUnlock = appWindows.startUnlockWorkflow(vault, null); + var secondUnlock = appWindows.startUnlockWorkflow(vault, null); + runQueuedTasks(); + + Mockito.verify(unlockWorkflowFactory).create(vault, null); + Mockito.verify(unlockWorkflow).run(); + Mockito.verifyNoInteractions(errorWindowFactory); + assertCompletedNormally(firstUnlock); + assertCompletedNormally(secondUnlock); + Assertions.assertEquals(VaultState.Value.PROCESSING, state.get()); + } + + @Test + public void testStaleLockStartSkips() { + var state = givenVaultState(VaultState.Value.LOCKED); + + var lock = appWindows.startLockWorkflow(vault, null); + runQueuedTasks(); + + Mockito.verifyNoInteractions(lockWorkflowFactory, errorWindowFactory); + assertCompletedNormally(lock); + Assertions.assertEquals(VaultState.Value.LOCKED, state.get()); + } + + @Test + public void testUnlockCompletesAfterWorkflowRan() { + givenVaultState(VaultState.Value.LOCKED); + + var unlock = appWindows.startUnlockWorkflow(vault, owner); + + Mockito.verify(unlockWorkflowFactory).create(vault, owner); + Assertions.assertFalse(unlock.toCompletableFuture().isDone()); + Mockito.verify(unlockWorkflow, Mockito.never()).run(); + runQueuedTasks(); + Mockito.verify(unlockWorkflow).run(); + assertCompletedNormally(unlock); + } + + @Test + public void testLockCompletesAfterWorkflowRan() { + givenVaultState(VaultState.Value.UNLOCKED); + + var lock = appWindows.startLockWorkflow(vault, owner); + + Mockito.verify(lockWorkflowFactory).create(vault, owner); + Assertions.assertFalse(lock.toCompletableFuture().isDone()); + Mockito.verify(lockWorkflow, Mockito.never()).run(); + runQueuedTasks(); + Mockito.verify(lockWorkflow).run(); + assertCompletedNormally(lock); + } + + @Test + public void testUnlockFactoryFailureShowsErrorInOwner() { + givenVaultState(VaultState.Value.LOCKED); + Mockito.when(unlockWorkflowFactory.create(vault, owner)).thenThrow(new RuntimeException("factory failed")); + + appWindows.startUnlockWorkflow(vault, owner); + runQueuedTasks(); + + Mockito.verify(errorWindowFactory).create(Mockito.any(), Mockito.eq(owner), Mockito.isNull()); + } + + @Test + public void testUnlockFactoryFailureWithoutOwnerShowsErrorInPrimaryStage() { + givenVaultState(VaultState.Value.LOCKED); + Mockito.when(unlockWorkflowFactory.create(vault, null)).thenThrow(new RuntimeException("factory failed")); + + appWindows.startUnlockWorkflow(vault, null); + runQueuedTasks(); + + Mockito.verify(errorWindowFactory).create(Mockito.any(), Mockito.eq(primaryStage), Mockito.isNull()); + } + + @Test + public void testLockFactoryFailureShowsErrorInOwner() { + givenVaultState(VaultState.Value.UNLOCKED); + Mockito.when(lockWorkflowFactory.create(vault, owner)).thenThrow(new RuntimeException("factory failed")); + + appWindows.startLockWorkflow(vault, owner); + runQueuedTasks(); + + Mockito.verify(errorWindowFactory).create(Mockito.any(), Mockito.eq(owner), Mockito.isNull()); + } + + @Test + public void testLockFactoryFailureWithoutOwnerShowsErrorInPrimaryStage() { + givenVaultState(VaultState.Value.UNLOCKED); + Mockito.when(lockWorkflowFactory.create(vault, null)).thenThrow(new RuntimeException("factory failed")); + + appWindows.startLockWorkflow(vault, null); + runQueuedTasks(); + + Mockito.verify(errorWindowFactory).create(Mockito.any(), Mockito.eq(primaryStage), Mockito.isNull()); + } + +} diff --git a/src/test/java/org/cryptomator/ui/unlock/UnlockWorkflowTest.java b/src/test/java/org/cryptomator/ui/unlock/UnlockWorkflowTest.java index 129d2dca8..3603577bd 100644 --- a/src/test/java/org/cryptomator/ui/unlock/UnlockWorkflowTest.java +++ b/src/test/java/org/cryptomator/ui/unlock/UnlockWorkflowTest.java @@ -140,16 +140,6 @@ class UnlockWorkflowTest { Assertions.assertFalse(vaultSettings.usesReadOnlyMode.get()); } - @Test - public void testExtensionSwitchedOffRetryAbortsWhenVaultIsNotLocked() throws Exception { - var okAction = failWithExtensionSwitchedOffAndCaptureRetry(); - Mockito.when(vault.isLocked()).thenReturn(false); // e.g. an unlock from the tray menu started while the dialog was open - - okAction.accept(Mockito.mock(Stage.class)); - - Mockito.verify(appWindows, Mockito.never()).startUnlockWorkflow(Mockito.any(), Mockito.any()); - } - private Consumer failWithExtensionSwitchedOffAndCaptureRetry() throws Exception { Mockito.when(dialogs.prepareFSKitExtensionSwitchedOffDialog(Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(dialogBuilder); runFailingWith(new FSKitExtensionSwitchedOffException(new MountFailedException("switched off"))); @@ -179,7 +169,6 @@ class UnlockWorkflowTest { invocation.getArgument(0, Runnable.class).run(); return null; }); - Mockito.when(vault.isLocked()).thenReturn(true); Mockito.doThrow(exception).when(keyLoadingStrategy).use(Mockito.any()); workflow.run();