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

Support for wordlists in user configuration directory #6799

Merged

Conversation

snipfoo
Copy link
Contributor

@snipfoo snipfoo commented Aug 2, 2021

Hi!

This commit allows users to put alternative wordlists in a wordlists subdirectory below their KeePassXC directory (e.g., under Linux, ~/.config/keepassxc/wordlists). These wordlists will then appear in the dropdown menu in the Password Generator widget.

In order to differentiate between lists shipped with KeePassXC and user-provided lists, the latter appear with an absolute path. Relative paths (for KeePassXC-provided lists) are relative to resources()->dataPath().

Screenshots

keepassxc-wordlists-in-user-config

Testing strategy

Tested with zero, one and several files in the ~/.config/keepassxc/wordlists directory. No problem detected.

Type of change

  • ✅ New feature (change that adds functionality)

Thanks!
SnipFoo

@droidmonkey
Copy link
Member

Awesome!! Instead of prefixing with the absolute path can you just prefix the filename with "(CUSTOM)" or similar? You lose focus on the name of the list (seemingly important) with the path in there.

@snipfoo
Copy link
Contributor Author

snipfoo commented Aug 3, 2021

Hi!

Great, thanks for the feedback!

Using a (CUSTOM) prefix instead of the full path is a neat idea, thanks!

I'm however under the impression that we'd still better store the actual path (for user-provided wordlists) in the configuration file, under the PasswordGenerator/WordList key. Keeping the actual path there seems more robust to me, especially if the locale changes (and thus we have a translated (CUSTOM) tag instead) or if the user drops a file named (CUSTOM) wordlist.txt or the like in their /usr/share/keepassxc/wordlists directory (although I admit this is highly unlikely). It would also be able to handle the case where the (CUSTOM) tag is changed for something else in a later version of KeePassXC.

I've implemented this in a54bb8a by having the wordlist combo box still handle the actual paths internally (in the Qt::UserRole data of each item) but display shortened paths (i.e., with the (CUSTOM) tag—or translations thereof—for user-provided wordlists).

Here is a screenshot (using a French locale in order to test the translation as well):

keepassxc-wordlists-custom-tag

The configuration corresponding to the above screenshot still contains the following:

WordList=/home/[redacted]/.config/keepassxc/wordlists/wordlist_fr_5d.txt

Cheers!
SnipFoo

@droidmonkey
Copy link
Member

Yah that's perfect, thank you!

@snipfoo
Copy link
Contributor Author

snipfoo commented Aug 3, 2021

Great, thanks!

Since I was still working on this feature, the two extra commits above (25853c0 and 7bfbd31) add buttons for adding or deleting custom wordlists. This allows users to use custom wordlists without having to create the ~/.config/keepassxc/wordlists directory and to drop wordlist files in there themselves. Added wordlists are copied to the custom wordlist directory, so that they can be reused later on.

Here is a screenshot of the resulting GUI:

keepassxc-wordlists-add-delete-buttons

Behavior:

  • The "delete" button asks for confirmation before actually removing the selected wordlist. It is disabled for system-wide wordlists.
  • The "add" button opens a file dialog, and will ask for confirmation if there already is a custom wordlist with the same name.
  • The wordlist combo box is now always visible and enabled, since the "add" and "delete" buttons appear on the right of the combo box.

Cheers!
SnipFoo

@droidmonkey
Copy link
Member

Hot!

@droidmonkey droidmonkey added this to the v2.7.0 milestone Aug 8, 2021
@droidmonkey droidmonkey self-requested a review August 8, 2021 20:30
@phoerious
Copy link
Member

Can we add an automatic download feature for word lists? We could host them on our website. And I would drop the (Custom) prefix. No need for that. I would also prefer nicer display names than the raw filename.

@droidmonkey
Copy link
Member

droidmonkey commented Oct 17, 2021

I fixed up the code quite a bit. I also reversed the (CUSTOM) tag, instead it shows (SYSTEM) for the built-in wordlist. Adding a download feature can come later, perhaps a button that sends them to the download page?

Whatcha think @phoerious?

@droidmonkey droidmonkey force-pushed the feature/wordlists-in-user-config branch from 06e7d88 to 08ed9bf Compare October 17, 2021 02:44
This commit allows users to put alternative wordlists in a `wordlists` subdirectory below their KeePassXC directory (e.g., under Linux, `~/.config/keepassxc/wordlists`). These wordlists will then appear in the dropdown menu in the *Password Generator* widget.

In order to differentiate between lists shipped with KeePassXC and user-provided lists, the former appears with a (SYSTEM) prefix.
@droidmonkey droidmonkey force-pushed the feature/wordlists-in-user-config branch from 08ed9bf to 414bb0e Compare October 17, 2021 02:45
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2021

Codecov Report

Attention: Patch coverage is 29.03226% with 66 lines in your changes missing coverage. Please review.

Project coverage is 63.54%. Comparing base (b6716bd) to head (414bb0e).
Report is 590 commits behind head on develop.

Files with missing lines Patch % Lines
src/gui/PasswordGeneratorWidget.cpp 26.67% 66 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6799      +/-   ##
===========================================
- Coverage    63.61%   63.54%   -0.07%     
===========================================
  Files          330      330              
  Lines        41807    41887      +80     
===========================================
+ Hits         26595    26615      +20     
- Misses       15212    15272      +60     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

lgtm

@phoerious
Copy link
Member

Although I would perhaps remove the "SYSTEM" thing entirely. Just disable the delete button when a system list is selected.

@droidmonkey droidmonkey merged commit 7811f10 into keepassxreboot:develop Nov 5, 2021
@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
Hacktoberfest 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