check if sqlite folder exists

[stefan0xC]

Instead of creating the parent folders to a sqlite database vaultwarden should just exit.

This should fix most issues like #2835 where a wrongly configured DATABASE_URL falls back to using sqlite.

Fixes #2873

[BlackDex]

This will not fix that specific issue. Since the data folder should actually exist already, since that is also where the JWT Keys are stored, and the config.json etc..

While this might help in detecting those issues, it doesn't filter out wrongly configured DATABASE_URL strings i think.

[stefan0xC]

This change would only be for prevention of cases like #2835. It will not help with an existing misconfigured DATABASE_URL that is using a sqlite database, but if you e.g. try to switch from the default sqlite to postgres database, this change would prevent the creation of a new postgreql: folder (if you have a typo) or "postgresql: (in case of the second example with the quotes).

And in docker setups you won't notice the creation of this folders because you typically don't check / for wrongly created folders.

[stefan0xC]

The problem as I understood it was that if I specify a DATABASE_URL I most likely want to use a postgresql or mysql/mariadb database. If the URL does not start with exactly mysql: or exactly postgresql: Vaultwarden will fall back to using sqlite (since almost everything is a valid filename on Linux) instead of raising an error.

I'm not sure if we can catch all the ways a user might mistype/misconfigure the DATABASE_URL. But for the given examples I think it's a safe and easy fix to just not create the folders (since for Vaultwarden it's not an error but a valid name for the location of a SQLite database and maybe someone uses this name for their sqlite database, so we can't really fix that).

This change introduces a small inconvenience if you want your (new) sqlite database in a custom location, yes, but I think in most cases that is an acceptable tradeoff. Existing installations should not be affected by this, I think.

[BlackDex]

The only thing we can do in cases like this is, check if it starts with postgresql://, postgres:// or mysql://, if not we currently assume it's a sqlite database file path, but if i understand it correctly, we do not validate this.

The issue also is, that it could be that the environment variable isn't correctly visible, and i think that is what happens mostly, like when using quotes within a docker-compose file around only the value if i'm correct, but I'm not sure how those items actually work out. We can't detect an env variable spelling mistake for example, or a wrongly typed folder (especially if we create that folder manually hehe). So, you need to test several senarios with docker, docker-compose, podman maybe, baremetal binary etc... en all with different ways of defining the DATABASE_URL.

[jjlin]

It's probably very uncommon for an SQLite database path to contain a colon, so that would be an easy heuristic for detecting postgresql://... or mysql://... typos.

It would also be very uncommon for any DATABASE_URL value (and also the vast majority of other config values) to contain quote characters, so that could be another check. This is an especially common issue with start/end quote chars, where people use key="val" or key='val' with an env file reader that doesn't unquote values (like Docker's --env-file option or Docker Compose's env_file directive).

[BlackDex]

To keep a bit on topic of the PR, while the FP mentions it would solve an issue, which i think it doesn't directly, but there is an other PR for that. This does help people think about there directory structure, and if they configured it correctly.

I do think that instead of creating the directory, and warning the user, and abort further loading is good.
What might also be a good check here, is if the directory is writeable, i think that also would help some people who in the past had wrong access rights on the directory.

[stefan0xC]

What might also be a good check here, is if the directory is writeable [...]

Aside from actually creating a file and getting a permission denied error, it is not easily possible to check if a directory is writeable (because on FilePermissions there seems to be only the readonly() check which is insufficient).
If the error returned by SqliteConnection::establish() is not explicit enough I can add some code handling like I did with the write_file() function to make it more human readable but I am not sure if diesel will expose a permission denied error (or rather which specific ConnectionError will be returned if the directory is not writable, maybe CouldntSetupConfiguration). So I'll have to test what happens.

[stefan0xC]

Okay, diesel will just return a BadConnection and say that it's "Unable to open the database file".

[stefan0xC]

I've added a second commit which improves the initial connection handling and just exit if there was an error. I am not sure if it makes sense to keep trying in case of sqlite but maybe this will break if a file system is not available yet? So maybe we should just log the error instead of exiting to not break anything? (Then the second commit does not really change anything except that we could be more helpful by pointing to permission issues...)

[stefan0xC]

Also I've removed the commit with my rather clunky check for a BadConnection again because my error message

Could not establish a connection to `{url}`: {reason}

was barely an improvement to the current error message

[2022-11-22 05:05:02.563][vaultwarden::util][WARN] Can't connect to database, retrying: DieselCon.
[CAUSE] BadConnection(
    "Unable to open the database file",
)

we get with the simpler

let mut connection = diesel::sqlite::SqliteConnection::establish(&url)?;

[tessus]

@stefan0xC I think you might have to click on request review again. Right now the status is changes requested even though you have implemented the changes...