Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config variable for specifying a default file name for the database #8042

Conversation

snipfoo
Copy link
Contributor

@snipfoo snipfoo commented May 11, 2022

Hi!

Currently, the default file name when saving a new database is localized (e.g., Passwords.kdbx in English, Mots de passe.kdbx in French, and so on), as per src/gui/DatabaseWidget.cpp:1984.

However, in some cases, it might be useful to have the default file name to be locale-independent.

For instance, this is the case in Tails, which is a live system where the KeePassXC configuration is never stored: at boot time, the user's home directory is populated using fixed files in /etc/skel, which, among other things, include a configuration file for KeePassXC (see here). This file expects that the user's database file name is Passwords.kdbx, so that KeePassXC will automatically open it.

However, this will not work for users using a non-English language locale: upon creating a new database, the default file name for saving it will be localized and will not be Passwords.kdbx. Also, Tails users may change their locale every time they log in, which makes it extremely impractical to have a file name depend on the current language locale. (This issue was reported as #18966 on the Tails tracker.)

The patch in this PR proposes to address this by introducing a new configuration variable, DefaultDatabaseFileName, which allows one to specify a default file name for new databases. This variable isn't accessible or modifiable from the GUI, as most users wouldn't need to change it anyway. If this variable is unset, the behaviour remains unchanged: a localized version of Passwords.kdbx is used as the default file name. But if this variable is set to a string, this string will be used as the default file name.

This change was made to be as unobtrusive as possible: regular users will never have to use or set this variable. However, live systems such as Tails operating with fixed configuration files will be able to set this configuration variable to a fixed string, so that the default database file name is always the same, regardless of the current language locale.

An alternative to this change would be to introduce a simple boolean variable (LocalizeDefaultDatabaseFileName, for instance) instead, which could be set to false in order to disable the localization of the default file name and to always use Passwords.kdbx.

I felt that the DefaultDatabaseFileName variable allowed for more flexibility (such as choosing a totally different file name, if need be), but I'll be happy to look into this alternative option and modify my PR accordingly if you'd rather go with the boolean setting.

Screenshots

Below is a screenshot of the KeePassXC window while saving a new database, localized in French and with the following line in the configuration file:

DefaultDatabaseFileName=MyCustomName.kdbx

default-database-file-name

Testing strategy

I've tested this change by creating new databases and checking which default file name was proposed by KeePassXC when saving it. I've checked that the behaviour was still the same when the variable was not set; and that, when set, the specified file name was proposed. I ran these tests under two different language locales (English and French).

Type of change

  • ✅ New feature (change that adds functionality)

Thank you very much in advance!

snipfoo

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #8042 (018e270) into develop (a4d4adb) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #8042      +/-   ##
===========================================
- Coverage    64.29%   64.28%   -0.01%     
===========================================
  Files          339      339              
  Lines        43431    43435       +4     
===========================================
- Hits         27923    27922       -1     
- Misses       15508    15513       +5     
Impacted Files Coverage Δ
src/core/Config.cpp 89.70% <ø> (ø)
src/core/Config.h 100.00% <ø> (ø)
src/gui/DatabaseWidget.cpp 61.62% <0.00%> (-0.16%) ⬇️
src/core/FileWatcher.cpp 85.54% <0.00%> (-1.20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4d4adb...018e270. Read the comment docs.

@droidmonkey
Copy link
Member

I have no idea why this is required and your description details issues with other platforms doing silly things that they need to fix.

@snipfoo
Copy link
Contributor Author

snipfoo commented May 12, 2022

Hello and thank you for your reply!

You're right that this is not required by KeePassXC users who use it on a regular (i.e., non-live) system, and I completely understand that this is what looks like a marginal use case.

Actually, this is the reason why the change proposed in this PR was written to be as small and as low-impact as possible, so as not to change anything for those users, and so as to modify the code base as minimally as possible.

However (and even though I'm merely a Tails user and not a Tails dev, and, as such, maybe not the best person to write about this), I would like to argue that the fact that systems such as Tails use hard-coded and locale-independent configuration files for the applications shipped with the system is not a “silly thing” but actually a very useful feature upon which Tails users strongly rely: Tails is designed and built to be an “amnesic” live system, meaning that it strives to leave as few traces as possible of one's use of the system (where it be on the computer's internal disk or on the flash drive where Tails is stored), and only offers to store what's strictly necessary onto an encrypted partition of the flash drive. In the case of KeePassXC (which is one of the featured apps in Tails), this means that only the password database can be written to the encrypted partition, but not the configuration files, which therefore have to be generated anew at each boot.

This amnesic feature actually makes Tails extremely popular among users for which privacy is a crucial—and sometimes vital—concern (whistleblowers, journalists, survivors of domestic violence, activists, and so on). As such, these users also tend to heavily rely on KeePassXC for generating and storing strong passwords, and this patch would actually make their experience with KeePassXC even more seamless and intuitive (which is another key feature of the Tails design rationale).

On the Tails side, we've already discussed alternative solutions to this issue which would not entail modifying KeePassXC, but unfortunately we couldn't come up with any which would properly cover all reasonable use cases :( This is why we're humbly submitting this PR.

I totally understand your position regarding this change, which is indeed unnecessary for most of the KeePassXC user base. I however hope the fact that the proposed modifications are so small and that they shouldn't have any repercussions for regular KeePassXC users will mitigate the somewhat extraneous nature of this request.

In any case, thank you very much for taking the time to read this!

Kind regards.

snipfoo

@droidmonkey
Copy link
Member

Ohhh interesting, I actually didn't know we used the translation of "Passwords" to make a new database file name. This change works for me.

QDir::toNativeSeparators(config()->get(Config::LastDir).toString() + "/" + tr("Passwords").append(".kdbx"));
QVariant defaultFileName = config()->get(Config::DefaultDatabaseFileName);
oldFilePath = QDir::toNativeSeparators(config()->get(Config::LastDir).toString() + "/"
+ (defaultFileName.isNull() ? tr("Passwords").append(".kdbx")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use isEmpty() instead, you want the config variable to contain at least one character.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, thanks! I've just pushed the suggested changes (as 018e270).

@snipfoo
Copy link
Contributor Author

snipfoo commented May 12, 2022

Ohhh interesting, I actually didn't know we used the translation of "Passwords" to make a new database file name. This change works for me.

Great, thank you so much! :) Sorry my initial description of the PR wasn't clear enough about its actual use case.

snipfoo

@droidmonkey droidmonkey added this to the v2.7.2 milestone May 28, 2022
@droidmonkey droidmonkey added pr: backport pending Pull request yet to be backported to a previous release new feature labels May 28, 2022
@droidmonkey droidmonkey force-pushed the feature/default-database-file-name branch from 018e270 to 6e27136 Compare June 4, 2022 14:59
@droidmonkey droidmonkey merged commit 806b8b0 into keepassxreboot:develop Jun 4, 2022
@droidmonkey droidmonkey added pr: backported Pull request backported to previous release and removed pr: backport pending Pull request yet to be backported to a previous release labels Jun 27, 2022
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: backported Pull request backported to previous release pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants