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

Make password generator length use existing length if available #2318

Conversation

Projects
None yet
4 participants
@kneitinger
Copy link
Contributor

kneitinger commented Sep 24, 2018

Description

Adds optional length parameter to PasswordGeneratorWidget::reset() (default=0), which when >0 sets the length SpinBox's value to the value passed in. The EditEntryWidget already called reset() when the password at a time when was known, so the current length was just added to this call (accounts for empty passwords correctly)

This seems like a clean approach to me, as the use of the default parameter keeps all previous usage the same, except when a value is passed in. Another approach could be to add a public PasswordGeneratorWidget::setLength() function. If that is more desired, I would be happy to do that instead.

Edit: Additionally, I made the change discussed in my comment below. Now, if the entry edit is closed without saving, any changes to the password generator length will not carry over into new entries.

Motivation and context

Fixes #2180
Fixes undesirable behavior discussed in comments of #2318

How has this been tested?

  1. Changed the value of [generator]->Length to something recognizable
    $ grep ^Length ~/.config/keepassxc/keepassxc.ini
    Length=53
    
  2. Opened keepassxc and began creating a new entry. Expanded generator section, and verified that the length still honors that value:
    2018-09-23_19-09-06
  3. Opened various entries, all having different lengths and verified that they inherit their existing length:
    2018-09-23_19-11-53
    2018-09-23_19-12-18
  4. Created new entry and expanded generator section. It is the last used length (in this case 8). This is identical to the current behavior, happy to change it if another behavior is desired: The length is still the value of [generator]->Length
    2018-09-23_22-59-43
    I then cancelled this entry (this is important for the next step).
  5. Opened standalone password generator, and it is still using the value of [generator]->Length at startup. The generator widget always retains its last state in memory, since it is always just hidden and shown. It does not actually commit its length to the ini until a generated password is actually used, therefore the standalone generator will not inherit the last used value of a cancelled generation.
    2018-09-23_19-19-50
  6. Created a new test entry with a 44 character generated password and saved it. Checked the ini file, and the length is changed to 44. Opened standalone generator, and length is set to 44:
    2018-09-23_19-28-23

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Sep 24, 2018

Thinking about it more, I don't really feel that the password generator widget keeping its last used length value as the in-memory equivalent of [generator]->Length is the best behavior when the password is not applied. For example if I open a new entry, expand the pw gen section, set length to 2, and cancel the close the entry without saving; the next time I open a new entry, the length will still be 2 instead of the most recent applied length.

Instead I feel like it should it should:

  • Reset its value to the saved [generator]->Length upon closing the entry without using the generated password.
  • Retain its value if the entry the generated password is used and the entry is saved. This is the criteria in which [generator]->Length is committed to the ini file, and in my opinion makes more sense.

This would mean that the standalone generator, the generator widget (on a new or passwordless entry), and the ini file are always consistent with each other.

This behavior would be different from how keepassxc currently behaves (regardless of looking at password existing password length), however, I think it makes more sense than having the length that wasn't even used carry over into fresh entries.

@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Sep 24, 2018

Edited PR description to reflect most recent commit.

@droidmonkey
Copy link
Member

droidmonkey left a comment

Good point, agree

@droidmonkey droidmonkey requested review from TheZ3ro and phoerious Sep 24, 2018

@phoerious phoerious force-pushed the kneitinger:feature/password_gen_inherit_curr_length branch from d9d65b7 to 9cf0b35 Sep 25, 2018

@droidmonkey droidmonkey added this to the v2.4.0 milestone Sep 25, 2018

Use length of existing password for the password generator
Add optional length parameter to PasswordGeneratorWidget::reset()
(default=0), which when >0 sets the legth SpinBox's value to the value
passed in. The EditEntryWidget already called reset() when the password
was known, so the current length was just added to this call (accounts
for empty passwords)

If entry edit is cancelled, return the password generator length to the
application default or last committed length if available. This is done
by calling reseting the password generator in EditEntryWidget::cancel()
when the edit page is being closed

Fixes #2180

@phoerious phoerious force-pushed the kneitinger:feature/password_gen_inherit_curr_length branch from 9cf0b35 to 3eae419 Sep 25, 2018

@droidmonkey droidmonkey merged commit d4112fe into keepassxreboot:develop Sep 26, 2018

3 checks passed

CodeFactor No issues found.
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@kneitinger kneitinger deleted the kneitinger:feature/password_gen_inherit_curr_length branch Sep 26, 2018

@TheZ3ro

This comment has been minimized.

Copy link
Member

TheZ3ro commented Sep 26, 2018

Nice contribution!

@kneitinger kneitinger restored the kneitinger:feature/password_gen_inherit_curr_length branch Sep 26, 2018

@kneitinger kneitinger deleted the kneitinger:feature/password_gen_inherit_curr_length branch Sep 26, 2018

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.