From 2c53afd70f32fd4d6dd3be6f07528552807d6a36 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Tue, 21 Jul 2026 17:06:17 +0200 Subject: [PATCH] simplify there is no intermediate directory in the zip. Also clean up tests Signed-off-by: Armin Schrenk --- .../ui/importtemplate/VaultTemplate.java | 52 +++++------- .../VaultTemplateExtractor.java | 43 ++++------ .../VaultTemplateExtractorTest.java | 63 ++++++++++---- .../ui/importtemplate/VaultTemplateTest.java | 84 +++++-------------- 4 files changed, 104 insertions(+), 138 deletions(-) diff --git a/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplate.java b/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplate.java index 5fea636d8..1126d7919 100644 --- a/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplate.java +++ b/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplate.java @@ -13,39 +13,31 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CompletionException; -import java.util.concurrent.CompletionStage; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Executor; /** * A vault template that has been unpacked to a temporary directory and found sound. *

- * Holding an instance means the template can be imported: it was a readable ZIP within the entry and size - * limits, held exactly one decodable {@value Constants#VAULTCONFIG_FILENAME}, and no entry escaped the extraction - * directory. Unpacking therefore happens once, when the deeplink arrives, so a template that could never be imported is - * rejected before the user is asked to choose a storage location - and {@link #moveTo(Path)} is left with nothing to - * validate but the destination. + * Holding an instance means the template is valid: it was a readable ZIP within the entry and size + * limits, held a decodable {@value Constants#VAULTCONFIG_FILENAME} directly in its root and no entry escaped + * the extraction directory. *

- * The instance owns a temporary directory and must be {@link #close() closed}, whether or not the + * Because the vault lies in the archive root, the temporary directory is the vault directory: importing moves + * it wholesale to its destination. + *

+ * The instance owns that temporary directory and must be {@link #close() closed}, whether or not the * import completes. *

- * {@link #hubUrl()} is read from the unverified vault config: its signature is keyed on the masterkey, which is - * only obtained by completing the Hub flow. It is shown so the user can recognize an unexpected Hub; the authoritative - * trust decision remains with {@code CheckHostTrustController} at unlock time. + * {@link #hubUrl()} is read from the unverified vault config. */ public final class VaultTemplate implements AutoCloseable { private static final Logger LOG = LoggerFactory.getLogger(VaultTemplate.class); - private final Path tempDir; - private final Path vaultRoot; + private final Path vaultDir; private final @Nullable String hubUrl; - private VaultTemplate(Path tempDir, Path vaultRoot, @Nullable String hubUrl) { - this.tempDir = tempDir; - this.vaultRoot = vaultRoot; + private VaultTemplate(Path vaultDir, @Nullable String hubUrl) { + this.vaultDir = vaultDir; this.hubUrl = hubUrl; } @@ -65,21 +57,21 @@ public final class VaultTemplate implements AutoCloseable { @VisibleForTesting static VaultTemplate extract(byte[] archive, @Nullable Path tempParent) throws IOException { - Path tempDir = tempParent == null // + Path vaultDir = tempParent == null // ? Files.createTempDirectory("vault-template-") // : Files.createTempDirectory(tempParent, "vault-template-"); try { - var vaultRoot = VaultTemplateExtractor.extractTo(archive, tempDir); - var hubURL = readHubUrl(vaultRoot); - return new VaultTemplate(tempDir, vaultRoot, hubURL); + VaultTemplateExtractor.extractTo(archive, vaultDir); + var hubUrl = readHubUrl(vaultDir); + return new VaultTemplate(vaultDir, hubUrl); } catch (IOException | RuntimeException e) { - VaultTemplateExtractor.deleteQuietly(tempDir); + VaultTemplateExtractor.deleteQuietly(vaultDir); throw e; } } - private static @Nullable String readHubUrl(Path vaultRoot) throws IOException { - var token = Files.readString(vaultRoot.resolve(Constants.VAULTCONFIG_FILENAME), StandardCharsets.US_ASCII).trim(); + private static @Nullable String readHubUrl(Path vaultDir) throws IOException { + var token = Files.readString(vaultDir.resolve(Constants.VAULTCONFIG_FILENAME), StandardCharsets.US_ASCII).trim(); HubConfig hubConfig; try { hubConfig = VaultConfig.decode(token).getHeader("hub", HubConfig.class); @@ -115,15 +107,15 @@ public final class VaultTemplate implements AutoCloseable { * @throws IOException if the vault cannot be moved */ public void moveTo(Path destination) throws IOException { - VaultTemplateExtractor.moveToDestination(vaultRoot, destination); + VaultTemplateExtractor.moveToDestination(vaultDir, destination); } /** - * Deletes the temporary directory. Safe to call after a successful {@link #moveTo(Path)}, which only moves the - * vault directory out of it. + * Discards the unpacked vault. A no-op after a successful {@link #moveTo(Path)}, which relocates the directory + * this would otherwise delete. */ @Override public void close() { - VaultTemplateExtractor.deleteQuietly(tempDir); + VaultTemplateExtractor.deleteQuietly(vaultDir); } } diff --git a/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractor.java b/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractor.java index df4741af1..77bf46565 100644 --- a/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractor.java +++ b/src/main/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractor.java @@ -22,9 +22,10 @@ import java.util.zip.ZipException; /** * Low-level mechanics of unpacking a vault template (a ZIP archive holding a ready-made vault). *

- * The archive is expanded into a directory using the Java ZIP {@link FileSystem}, and the vault directory is located by - * finding the {@value Constants#VAULTCONFIG_FILENAME} it contains. Moving that directory to its final location is a - * separate step, so the vault only ever appears complete at its destination. + * Per the template spec the vault lies directly in the archive root, i.e. the archive holds + * {@value Constants#VAULTCONFIG_FILENAME} at its top level rather than inside an enclosing folder. The extraction + * directory therefore is the vault directory. Moving it to its final location is a separate step, so the vault + * only ever appears complete at its destination. *

* Lifecycle of the temporary directory is owned by {@link VaultTemplate}, not by this class. */ @@ -41,39 +42,35 @@ final class VaultTemplateExtractor { } /** - * Unpacks the given template into {@code targetDir} and locates the vault it contains. + * Unpacks the given template into {@code targetDir}, which then holds the vault itself. * * @param template the ZIP archive bytes * @param targetDir an existing, empty directory to unpack into - * @return the unpacked vault directory below {@code targetDir} * @throws MalformedTemplateException if the template is not a readable ZIP, exceeds the entry or size limits, or - * does not contain exactly one {@value Constants#VAULTCONFIG_FILENAME} + * does not hold {@value Constants#VAULTCONFIG_FILENAME} in its root * @throws IOException if the files cannot be written */ - static Path extractTo(byte[] template, Path targetDir) throws IOException { + static void extractTo(byte[] template, Path targetDir) throws IOException { Path tmpZip = Files.createTempFile("vault-template-", ".zip"); try { Files.write(tmpZip, template); - return unzip(tmpZip, targetDir); + unzip(tmpZip, targetDir); } finally { deleteQuietly(tmpZip); } } - private static Path unzip(Path zipFile, Path targetDir) throws IOException { + private static void unzip(Path zipFile, Path targetDir) throws IOException { Path normalizedTarget = targetDir.normalize(); try (FileSystem zipFs = FileSystems.newFileSystem(zipFile)) { Path zipRoot = zipFs.getRootDirectories().iterator().next(); //we take the first available root and ignore others - var templateExtractor = new TemplateExtractionVisitor(zipRoot, normalizedTarget, MAX_ENTRIES, MAX_TOTAL_BYTES); - Files.walkFileTree(zipRoot, templateExtractor); - var vaultConfig = templateExtractor.getVaultConfig(); - if (vaultConfig == null) { - throw new MalformedTemplateException("Template does not contain a vault (no " + Constants.VAULTCONFIG_FILENAME + " found)."); - } - return vaultConfig.getParent(); + Files.walkFileTree(zipRoot, new TemplateExtractionVisitor(zipRoot, normalizedTarget, MAX_ENTRIES, MAX_TOTAL_BYTES)); } catch (ZipException e) { throw new MalformedTemplateException("Template is not a readable ZIP archive.", e); } + if (!Files.isRegularFile(normalizedTarget.resolve(Constants.VAULTCONFIG_FILENAME))) { + throw new MalformedTemplateException("Template does not hold " + Constants.VAULTCONFIG_FILENAME + " in its root directory."); + } } static class TemplateExtractionVisitor extends SimpleFileVisitor { @@ -85,7 +82,6 @@ final class VaultTemplateExtractor { private int totalEntries = 0; private long totalBytes = 0; - private Path vaultConfig = null; TemplateExtractionVisitor(Path zipRoot, Path target, int maxEntries, long maxSize) { this.zipRoot = zipRoot; @@ -111,14 +107,7 @@ final class VaultTemplateExtractor { throw new MalformedTemplateException("Vault template exceeds the maximum allowed size of " + maxSize + " bytes."); } - var actualTarget = Files.copy(file, resolveSafely(target, zipRoot, file), StandardCopyOption.REPLACE_EXISTING); - - if (Constants.VAULTCONFIG_FILENAME.equals(file.getFileName().toString())) { - if (vaultConfig != null) { - throw new MalformedTemplateException("Vault template contains more than one " + Constants.VAULTCONFIG_FILENAME); - } - vaultConfig = actualTarget; - } + Files.copy(file, resolveSafely(target, zipRoot, file), StandardCopyOption.REPLACE_EXISTING); return FileVisitResult.CONTINUE; } @@ -128,10 +117,6 @@ final class VaultTemplateExtractor { throw new MalformedTemplateException("Vault template contains more than the maximum allowed " + maxEntries + " entries."); } } - - Path getVaultConfig() { - return vaultConfig; - } } private static Path resolveSafely(Path targetDir, Path zipRoot, Path entry) throws IOException { diff --git a/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractorTest.java b/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractorTest.java index 173c5567d..f3523091f 100644 --- a/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractorTest.java +++ b/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateExtractorTest.java @@ -8,6 +8,7 @@ import org.junit.jupiter.api.io.TempDir; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; import java.util.LinkedHashMap; @@ -21,31 +22,29 @@ public class VaultTemplateExtractorTest { private static final byte[] CIPHERTEXT = "some-ciphertext".getBytes(StandardCharsets.UTF_8); @Test - @DisplayName("a vault at the archive root is unpacked and located") + @DisplayName("a vault at the archive root is unpacked into the target directory") public void testVaultAtArchiveRoot(@TempDir Path tmp) throws IOException { var zip = zip(Map.of( // "vault.cryptomator", CONFIG, // "d/AB/CDEF/0.c9r", CIPHERTEXT // )); - var vaultRoot = VaultTemplateExtractor.extractTo(zip, tmp); + VaultTemplateExtractor.extractTo(zip, tmp); - Assertions.assertArrayEquals(CONFIG, Files.readAllBytes(vaultRoot.resolve("vault.cryptomator"))); - Assertions.assertArrayEquals(CIPHERTEXT, Files.readAllBytes(vaultRoot.resolve("d/AB/CDEF/0.c9r"))); + Assertions.assertArrayEquals(CONFIG, Files.readAllBytes(tmp.resolve("vault.cryptomator"))); + Assertions.assertArrayEquals(CIPHERTEXT, Files.readAllBytes(tmp.resolve("d/AB/CDEF/0.c9r"))); } @Test - @DisplayName("a vault nested in a single top-level folder is located") + @DisplayName("a vault nested in a top-level folder fails as malformed") public void testVaultInSubfolder(@TempDir Path tmp) throws IOException { + // requirement: the vault starts in the archive root, an enclosing folder is not a supported template var zip = zip(Map.of( // "TemplateVault/vault.cryptomator", CONFIG, // "TemplateVault/d/AB/CDEF/0.c9r", CIPHERTEXT // )); - var vaultRoot = VaultTemplateExtractor.extractTo(zip, tmp); - - Assertions.assertEquals("TemplateVault", vaultRoot.getFileName().toString()); - Assertions.assertArrayEquals(CONFIG, Files.readAllBytes(vaultRoot.resolve("vault.cryptomator"))); + Assertions.assertThrows(MalformedTemplateException.class, () -> VaultTemplateExtractor.extractTo(zip, tmp)); } @Test @@ -65,14 +64,19 @@ public class VaultTemplateExtractorTest { } @Test - @DisplayName("an archive with more than one vault config fails as malformed") - public void testMultipleVaultConfigs(@TempDir Path tmp) throws IOException { + @DisplayName("a further vault config below the root is ordinary content, the root one identifies the vault") + public void testFurtherVaultConfigBelowRoot(@TempDir Path tmp) throws IOException { + // with the vault fixed at the archive root a nested config is simply extracted along with everything + // else and counts against the entry limit var zip = zip(Map.of( // "vault.cryptomator", CONFIG, // - "nested/vault.cryptomator", CONFIG // + "nested/vault.cryptomator", CIPHERTEXT // )); - Assertions.assertThrows(MalformedTemplateException.class, () -> VaultTemplateExtractor.extractTo(zip, tmp)); + VaultTemplateExtractor.extractTo(zip, tmp); + + Assertions.assertArrayEquals(CONFIG, Files.readAllBytes(tmp.resolve("vault.cryptomator"))); + Assertions.assertArrayEquals(CIPHERTEXT, Files.readAllBytes(tmp.resolve("nested/vault.cryptomator"))); } @Test @@ -92,9 +96,9 @@ public class VaultTemplateExtractorTest { public void testAtEntryLimit(@TempDir Path tmp) throws IOException { var zip = zip(flatEntries(VaultTemplateExtractor.MAX_ENTRIES)); - var vaultRoot = VaultTemplateExtractor.extractTo(zip, tmp); + VaultTemplateExtractor.extractTo(zip, tmp); - Assertions.assertArrayEquals(CONFIG, Files.readAllBytes(vaultRoot.resolve("vault.cryptomator"))); + Assertions.assertArrayEquals(CONFIG, Files.readAllBytes(tmp.resolve("vault.cryptomator"))); } @Test @@ -124,6 +128,35 @@ public class VaultTemplateExtractorTest { Assertions.assertTrue(Files.notExists(tmp.resolve("escaped.txt")), "entry escaped the extraction directory"); } + @Test + @DisplayName("an unpacked vault is moved to the destination, creating missing parents") + public void testMoveToDestination(@TempDir Path tmp) throws IOException { + var source = unpackedVault(tmp.resolve("unpacked")); + var destination = tmp.resolve("parent").resolve("MyVault"); // parent does not exist yet + + VaultTemplateExtractor.moveToDestination(source, destination); + + Assertions.assertArrayEquals(CONFIG, Files.readAllBytes(destination.resolve("vault.cryptomator"))); + Assertions.assertArrayEquals(CIPHERTEXT, Files.readAllBytes(destination.resolve("d/AB/0.c9r"))); + Assertions.assertTrue(Files.notExists(source), "the vault should no longer be at its old location"); + } + + @Test + @DisplayName("an existing destination is not overwritten") + public void testMoveToExistingDestination(@TempDir Path tmp) throws IOException { + var source = unpackedVault(tmp.resolve("unpacked")); + var destination = Files.createDirectory(tmp.resolve("MyVault")); + + Assertions.assertThrows(FileAlreadyExistsException.class, () -> VaultTemplateExtractor.moveToDestination(source, destination)); + } + + private static Path unpackedVault(Path dir) throws IOException { + Files.createDirectories(dir.resolve("d/AB")); + Files.write(dir.resolve("vault.cryptomator"), CONFIG); + Files.write(dir.resolve("d/AB/0.c9r"), CIPHERTEXT); + return dir; + } + /** * @param count total number of entries below the zip root, the vault config included */ diff --git a/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateTest.java b/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateTest.java index 7f3ece039..4640e0848 100644 --- a/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateTest.java +++ b/src/test/java/org/cryptomator/ui/importtemplate/VaultTemplateTest.java @@ -8,7 +8,6 @@ import org.junit.jupiter.api.io.TempDir; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; import java.util.Base64; @@ -50,43 +49,6 @@ public class VaultTemplateTest { Assertions.assertThrows(MalformedTemplateException.class, () -> VaultTemplate.extract(zip)); } - @Test - @DisplayName("data that is not a zip is rejected") - public void testNotAZip() { - var notAZip = "this is not a zip archive".getBytes(StandardCharsets.UTF_8); - - Assertions.assertThrows(MalformedTemplateException.class, () -> VaultTemplate.extract(notAZip)); - } - - @Test - @DisplayName("the unpacked vault is moved to the destination") - public void testMoveTo(@TempDir Path tmp) throws IOException { - var zip = zip(Map.of( // - "vault.cryptomator", hubVaultConfig(API_BASE_URL), // - "d/AB/CDEF/0.c9r", CIPHERTEXT // - )); - var destination = tmp.resolve("MyVault"); - - try (var inTest = VaultTemplate.extract(zip)) { - inTest.moveTo(destination); - } - - Assertions.assertArrayEquals(CIPHERTEXT, Files.readAllBytes(destination.resolve("d/AB/CDEF/0.c9r"))); - Assertions.assertTrue(Files.exists(destination.resolve("vault.cryptomator"))); - } - - @Test - @DisplayName("an existing destination is not overwritten") - public void testDestinationExists(@TempDir Path tmp) throws IOException { - var destination = tmp.resolve("MyVault"); - Files.createDirectory(destination); - var zip = zip(Map.of("vault.cryptomator", hubVaultConfig(API_BASE_URL))); - - try (var inTest = VaultTemplate.extract(zip)) { - Assertions.assertThrows(FileAlreadyExistsException.class, () -> inTest.moveTo(destination)); - } - } - @Test @DisplayName("closing discards the temporary directory, also after a completed import") public void testCloseCleansUpAfterMove(@TempDir Path tmp) throws IOException { @@ -107,24 +69,6 @@ public class VaultTemplateTest { Assertions.assertArrayEquals(CIPHERTEXT, Files.readAllBytes(destination.resolve("d/AB/CDEF/0.c9r"))); } - @Test - @DisplayName("closing discards the temporary directory a nested vault leaves behind") - public void testCloseCleansUpNestedRemainder(@TempDir Path tmp) throws IOException { - // a vault at the archive root IS the temporary directory, so moveTo relocates it wholesale; a nested vault - // leaves its enclosing temporary directory behind, and only close() removes that - var zip = zip(Map.of("TemplateVault/vault.cryptomator", hubVaultConfig(API_BASE_URL))); - var workDir = Files.createDirectory(tmp.resolve("work")); - var destination = tmp.resolve("MyVault"); - - try (var inTest = VaultTemplate.extract(zip, workDir)) { - inTest.moveTo(destination); - Assertions.assertFalse(isEmpty(workDir), "the enclosing temporary directory should outlive the move"); - } - - Assertions.assertTrue(isEmpty(workDir), "temporary extraction directory was not cleaned up"); - Assertions.assertTrue(Files.exists(destination.resolve("vault.cryptomator"))); - } - @Test @DisplayName("closing without importing discards the temporary directory") public void testCloseCleansUpWithoutMove(@TempDir Path tmp) throws IOException { @@ -157,11 +101,15 @@ public class VaultTemplateTest { private static byte[] hubVaultConfig(String apiBaseUrl) { return vaultConfig(""" - ,"hub":{"clientId":"cryptomator","authEndpoint":"https://login.example.com/auth",\ - "tokenEndpoint":"https://login.example.com/token",\ - "authSuccessUrl":"https://hub.example.com/app/unlock-success",\ - "authErrorUrl":"https://hub.example.com/app/unlock-error",\ - "apiBaseUrl":"%s"}""".formatted(apiBaseUrl)); + , + "hub": { + "clientId":"cryptomator",\ + "authEndpoint":"https://login.example.com/auth",\ + "tokenEndpoint":"https://login.example.com/token",\ + "authSuccessUrl":"https://hub.example.com/app/unlock-success",\ + "authErrorUrl":"https://hub.example.com/app/unlock-error",\ + "apiBaseUrl":"%s" + }""".formatted(apiBaseUrl)); } /** @@ -169,9 +117,17 @@ public class VaultTemplateTest { * the import dialog operates in - so a dummy signature is sufficient here. */ private static byte[] vaultConfig(String extraHeaderFields) { - var header = "{\"kid\":\"hub+https://hub.example.com/api/vaults/123\",\"typ\":\"JWT\",\"alg\":\"HS256\"%s}" // - .formatted(extraHeaderFields == null ? "" : extraHeaderFields); - var payload = "{\"format\":8,\"cipherCombo\":\"SIV_GCM\",\"shorteningThreshold\":220}"; + var header = """ + { "kid":"hub+https://hub.example.com/api/vaults/123",\ + "typ":"JWT",\ + "alg":"HS256"\ + %s + }""".formatted(extraHeaderFields == null ? "" : extraHeaderFields); + var payload = """ + { "format":8,\ + "cipherCombo":"SIV_GCM",\ + "shorteningThreshold":220\ + }"""; var encoder = Base64.getUrlEncoder().withoutPadding(); var token = encoder.encodeToString(header.getBytes(StandardCharsets.UTF_8)) // + "." + encoder.encodeToString(payload.getBytes(StandardCharsets.UTF_8)) //