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

Implement automatic locking when minimizing #57

Merged
merged 2 commits into from Oct 28, 2016

Conversation

JosefVitu
Copy link
Contributor

@JosefVitu JosefVitu commented Oct 25, 2016

Description

This PR adds two security-related options: Lock databases after minimizing to system tray and Lock databases after minimizing to taskbar.

The autolocking is suspended when GUI/MinimizeOnStartup is true (or to be more precise, MainWindow::configuredMinimizeWindow() is left intact), as it wouldn't make much sense to lock the DB right after unlocking it.

Motivation and Context

It enables additional workflows for using this program, and is implemented in the original KeePass as well.

How Has This Been Tested?

Manually tested all possible combinations of the following settings:
GUI/MinimizeOnClose
GUI/MinimizeOnStartup
GUI/MinimizeToTray
security/lockdatabasetray
security/lockdatabaseminimize

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • ✅ My code follows the code style of this project.
  • ❎ My change requires a change to the documentation.
  • ❎ I have updated the documentation accordingly.
  • ✅ I have read the CONTRIBUTING document.
  • ❎ I have added tests to cover my changes.
  • ✅ All new and existing tests passed.

Could you please review the PR and merge if deemed appropriate? Thank you.

@TheZ3ro TheZ3ro changed the title 🔒 Implement automatic locking when minimizing Implement automatic locking when minimizing Oct 25, 2016
TheZ3ro
TheZ3ro previously approved these changes Oct 25, 2016
Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

By me it's all good, nice contribution 😉

@droidmonkey
Copy link
Member

Is there a need to distinguish between minimize and tray? I am concerned by confusing numbers/permutations of security options. KeePass suffers from this issue as it is.

This is part of a larger conversation of cleaning up config settings and making the UI more intuitive.

I'm going to try this out before I merge it to make sure it's not adding to the config soup.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Oct 25, 2016

I think @droidmonkey is right here, maybe we can make an unified option security/lockdatabaseminimize with text "Lock databases after minimizing the window" or "Lock databases after minimize"

@JosefVitu
Copy link
Contributor Author

Thank you for the comments. I've been using this distinction for temporarily "hiding" (minimizing to taskbar) the window when I knew I'd need it in a few minutes again (and didn't want to type the password endlessly), and putting it away (minimizing to tray) for a long time. No idea how other people are using it, that's just how I've got used to it while using Windows and KeePass for some time.

But while I prefer having more choices with sensible and safe defaults (which most people don't change anyway), I understand your point about needless complexity.

P.S.: The system tray settings in the General tab have some superfluous hierarchy, so those should be IMHO simplified as well, but that would be probably better discussed elsewhere.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Oct 27, 2016

After testing this, I think on GNU/Linux there is no need to have 2 different option.

I'm using XFCE desktop environment and with the "Hide window to system tray when minimized" option enabled the behaviour is the same.

@JosefVitu can you make this a single option?

Anyway right now I don't think the default options in settings are totally safe IMHO.

P.S.: With "system tray settings" you refer to Hiding on app-start and Hiding on app-exit ?

@JosefVitu
Copy link
Contributor Author

I'm using Xfce too and have the "Hide window to system tray when minimized" option turned off just because I want to distinguish between minimizing to taskbar (no autolock) and hiding to system tray (autolock), so these two options accomplish different behaviors.

But I guess having only one option would be clearer to the users (and better than not having any), so I'll merge them and form a new habit for myself :) What should be the default value of that single option? It's false now so the users wouldn't be surprised by the new behavior after upgrading, but setting it to true may be a little more secure.

And yes, I was referring to those two checkboxes. Maybe I'm missing something, but I have no idea why Hiding on app start needs to depend on Hiding on exit. Then having both depend on "Hide window to system tray when minimized" is IMHO a little questionable as well, but I can somewhat understand the reasoning behind that.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Oct 28, 2016

How do you minimize to systray on XFCE? I can do this only if I have the "Hide window to system tray when minimized" option set to true, so there is no difference for me between the 2 (systray and taskbar)

IMHO the default option should be "true", but after seeing the Security tab i think the default settings prefer usability instead of security.

Anyway what about this, but I think we are overcomplicating a bit:

  • If I have the option "Hide window to system tray when minimized" set to false,
    when minimized to taskbar lock based on the "Lock when minimized" option,
    when minimized to systray ALWAYS lock.
  • If I have the option set to true, i
    when minimized to taskbar or systray (doesn't matter) lock based on the "Lock when minimized" option

About the App-Exit/App-Start, this are different event in every case. SysTray/TaskBar most of the time are the same event.
We can discuss the need of an App-Start minimizing option, but it's out of this PR' scope

@JosefVitu
Copy link
Contributor Author

@TheZ3ro By a left-click on the systray icon.

OK, let's simplify this to a single option that enables locking the databases no matter how the user minimizes/hides the window (i.e. be it either to taskbar or systray).

Will open an issue about the other thing.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Oct 28, 2016

@JosefVitu ok nice 😄

@JosefVitu
Copy link
Contributor Author

Rebased and edited the patch as discussed.

@TheZ3ro TheZ3ro dismissed their stale review October 28, 2016 12:19

Old codebase

Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

Good job 👍

@droidmonkey droidmonkey merged commit 82ace81 into keepassxreboot:develop Oct 28, 2016
@JosefVitu JosefVitu deleted the lock-on-minimize branch October 28, 2016 14:35
@phoerious phoerious modified the milestone: v2.1.0 Jan 14, 2017
@Germano0 Germano0 mentioned this pull request Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants