CryptoFileSystemProvider.containsVault(path) returns true for invalid vault

[infeo]

If a version 8 vault contains the masterkey file and the d directory, but the config file is missing, CryptoFileSystemProvider.containsVault(...) returns true:

https://github.com/cryptomator/cryptofs/blob/fe410fdef30bf59bc007fb56828501a6439423fc/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProvider.java#L171-L176

From my point of view, this function should check the given path only with the criteria of the most recent vault version. At the time of opening this ticket, this would be vault format 8 and the actual check would a existence test of the vault config file and the data directory.

A consequence of this would be, that testing a vault of a lower version would return false. But that's ok, for this we have the migration package.

[overheadhunter]

It must not return false, otherwise you can't run migration in the main app. But maybe we can change it to return an enum.

[infeo]

It must not return false, otherwise you can't run migration in the main app.

From my point of view this question should not be answered from a single,existing downstream perspective. We rather should aim for a good, clean and clear api usage and than adapt our consumer libraries.

Maybe we should create for each vault version a method, i.e. containsVersion5Vault(...), containsVersion6Vault(...), etc. Otherwise this method will only increase in complexity for every structural change in the future. (This is more a theoretical argument, but still).

[overheadhunter]

Maybe we should create for each vault version a method, i.e. containsVersion5Vault(...), containsVersion6Vault(...), etc.

This would just move complexity from one project to another. I don't want to call containsV5 || containsV6 || containsV7 || ... from downstream.

The mere purpose of this method is to check if a directory resembles a vault directory. Neither the version nor any structural integrity is guaranteed.

Otherwise this method will only increase in complexity for every structural change in the future. (This is more a theoretical argument, but still).

The change from "masterkey file" to "vault config" is the first major change in years. The vault config file is designed with future extensions in mind, so YAGNI.

Since the current implementation works as intended, but is admittedly unclear regarding current vs. legacy vaults, I agree we should clarify the signature as long as we're not 2.0.0-final.

I propose to change it to:

enum DirStructure {

/**
 * Dir contains a <code>d</code> dir as well as a vault config file.
 */
VAULT,

/**
 * Dir contains a <code>d</code> dir and a masterkey file, but misses a vault config file.
 * Probably needs migration to a newer format.
 */
LEGACY,

/**
 * Dir does not qualify as vault.
 */
UNRELATED

}

public static DirStructure checkDirStructure(Path pathToVault, String vaultConfigFilename, String masterkeyFilename) {...}