Lock database on ScreenSaver/SessionLock by TheZ3ro · Pull Request #545 · keepassxreboot/keepassxc

TheZ3ro · 2017-05-04T21:23:52Z

Description

This is a rebase of #135 by @rockdreamer

Added a configuration entry in security settings called "Close database when session is locked or lid is closed"
Added a class called ScreenLockListener with a private implementation class backed by three platform specific implementations:
- on Mac OS the implementation uses the NSNotificationCenter C API
- on Linux the implementation listens to DBus signals
- on Windows the implementation registers for platform specific messages and registers a platform message handler

Motivation and Context

Implementation of issue #134

How Has This Been Tested?

I have tested on the following platforms:

Mac OS Sierra, Qt 5.7 from online installer, XCode 8, physical laptop: Lid close triggers lock
Ubuntu 16.10, Qt 5.7 from online installer, on Unity desktop, virtual machine: Session lock triggers lock
Windows 7, Qt 5.7 from online installer with libraries from MSYS2, virtual machine: Session lock triggers lock

Types of changes

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

This change brings in the following new dependencies:

Mac OS: Foundation framework (always present on the platform), the build requires XCode 8
Linux: Qt::DBus (available on most Linux distributions nowadays)
Windows: wtsapi32.lib (part of the platform SDK)

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]

TheZ3ro · 2017-05-04T21:25:46Z

This needs test on Unity, Gnome, OSX (@weslly) and Windows. I've tested on Xfce and works fine

weslly · 2017-05-04T21:58:09Z

~~On OSX it works fine when locking the screen from keychain access or after waking up from sleep but not when I use the fast user switching menu.~~

Edit: Actually it seems when you use the fast user switching menu you aren't locking your session, even though you still need to type the user password to come back, so it still will work in most cases (when the user closes the lid or puts the computer to sleep).

weslly · 2017-05-04T22:09:28Z

src/core/ScreenLockListener.h

+    Q_OBJECT
+
+public:
+    ScreenLockListener(QWidget* parent=NULL);


Shouldn't this NULL be nullptr instead?

(Same with other files)

Keisial · 2017-05-04T23:56:30Z

Rather than a checkbox, I would consider allowing a delay (eg. waiting another couple of minutes) for db locking after screen lock. Maybe unneeded after #488, though.

sainaen · 2017-05-06T12:58:06Z

Hey! I just tried this on Gnome 3 (specifically 3.22.3-3 on Debian x64 Testing/Sid) with manual lock via menu and just closing the lid. Everything seems to work as expected.

But I noticed that this patch adds two checkboxes:

autoCloseOnScreenLockCheckBox in SettingsWidgetGeneral.ui:370
lockDatabaseOnScreenLockCheckBox in SettingsWidgetSecurity.ui:126

The first one is in the weird place (outside of the tab widget) and isn't connected to anything. Probably is just an artifact of rebase.

louib · 2017-05-06T15:31:46Z

src/core/ScreenLockListenerDBus.cpp

+
+void ScreenLockListenerDBus::gnomeSessionStatusChanged(uint status)
+{
+    if (status != 0) {


@TheZ3ro ok so from what I can see from gnome's doc :

The status parameter must be one of the following: 0: Available 1: Invisible 2: Busy 3: Idle

Should we lock for all of the above statuses except for Available (Busy, for instance).

Also, We could extract 0 as a constant to make it more clear why we are ignoring this status : if (status != gnomeStatusAvailable)

Mmmmh yes, I think I can do that

louib · 2017-05-06T15:34:14Z

src/core/ScreenLockListenerDBus.cpp

+    }
+}
+
+void ScreenLockListenerDBus::logindPrepareForSleep(bool beforeSleep)


@TheZ3ro why logind?

I think this is because org.freedesktop.login1 is the DBus service for logind daemon

louib · 2017-05-06T15:46:34Z

src/gui/SettingsWidgetGeneral.ui

    </widget>
   </item>
+   <item row="15" column="0">
+    <widget class="QCheckBox" name="autoCloseOnScreenLockCheckBox">


@TheZ3ro where is this used?

This checkbox is appearing OUTSIDE the basic settings view. I think this should really be under the Security Settings anyway.

Yup, I forgot to remove this after the cherry-pick as this was added by the previous (old) commit

droidmonkey · 2017-05-06T16:14:18Z

src/gui/SettingsWidgetGeneral.ui

    </widget>
   </item>
+   <item row="15" column="0">
+    <widget class="QCheckBox" name="autoCloseOnScreenLockCheckBox">


This checkbox is appearing OUTSIDE the basic settings view. I think this should really be under the Security Settings anyway.

TheZ3ro · 2017-05-06T21:53:40Z

@Keisial

Rather than a checkbox, I would consider allowing a delay (eg. waiting another couple of minutes) for db locking after screen lock. Maybe unneeded after #488, though.

We already have an option to lock the database after a delay so this will be partially useless

@sainaen Thanks for the test!
Yes, the option in the General settings is due to the rebase, I've forgot to remove that. I will fix asap

droidmonkey

I approve for Windows

louib · 2017-05-07T15:09:45Z

src/core/ScreenLockListenerWin.cpp

+ * See https://msdn.microsoft.com/en-us/library/aa383841(v=vs.85).aspx
+ * See https://blogs.msdn.microsoft.com/oldnewthing/20060104-50/?p=32783
+ */
+ScreenLockListenerWin::ScreenLockListenerWin(QWidget *parent) 


@TheZ3ro QWidget* parent

louib · 2017-05-07T15:10:45Z

src/core/ScreenLockListenerWin.cpp

+    HPOWERNOTIFY hPnotify = RegisterPowerSettingNotification(
+                reinterpret_cast<HWND>(parent->winId()),
+                &GUID_LIDSWITCH_STATE_CHANGE, DEVICE_NOTIFY_WINDOW_HANDLE);
+    m_powernotificationhandle = reinterpret_cast<void*>(hPnotify);


@TheZ3ro m_powerNotificationHandle

louib · 2017-05-07T15:12:38Z

src/core/ScreenLockListenerWin.cpp

+bool ScreenLockListenerWin::nativeEventFilter(const QByteArray &eventType, void *message, long *)
+{
+    if (eventType == "windows_generic_MSG" || eventType == "windows_dispatcher_MSG") {
+        MSG* m = static_cast<MSG *>(message);


static_cast<MSG*>

louib · 2017-05-11T16:05:17Z

I tested the feature on Mac, it works great so far! I'll give it a try with Unity as soon as I can.

louib · 2017-05-12T01:09:02Z

@TheZ3ro tested with Unity, also working great!

phoerious

Works on Arch with KDE. Just a few little style things need fixing.

phoerious · 2017-05-14T13:20:02Z

src/core/ScreenLockListenerMac.cpp

+    return m_ptr;
+}
+
+void ScreenLockListenerMac::notificationCenterCallBack(CFNotificationCenterRef, void*,


You should add parameter names, even though they are not used.

Won't this brings up some unused warning? Should I use Q_UNUSED?

The names are defined in the header file

I know. It's just a style thing.

phoerious · 2017-05-14T13:21:10Z

src/core/ScreenLockListenerPrivate.cpp

+#if defined(Q_OS_OSX)
+#include "ScreenLockListenerMac.h"
+#endif
+#if defined(Q_OS_LINUX)


phoerious · 2017-05-14T13:22:05Z

src/core/ScreenLockListenerMac.cpp

+
+void ScreenLockListenerMac::onSignalReception()
+{
+    Q_EMIT screenLocked();


phoerious · 2017-05-14T13:22:11Z

src/core/ScreenLockListenerDBus.h

+public:
+    explicit ScreenLockListenerDBus(QWidget *parent = 0);
+
+private Q_SLOTS:


phoerious · 2017-05-14T13:22:27Z

src/core/ScreenLockListener.h

+    ScreenLockListener(QWidget* parent = nullptr);
+    ~ScreenLockListener();
+
+Q_SIGNALS:


phoerious · 2017-05-14T13:22:36Z

src/core/ScreenLockListenerDBus.cpp

+void ScreenLockListenerDBus::gnomeSessionStatusChanged(uint status)
+{
+    if (status != 0) {
+        Q_EMIT screenLocked();


phoerious · 2017-05-14T13:22:54Z

src/core/ScreenLockListenerPrivate.h

+protected:
+    ScreenLockListenerPrivate(QWidget* parent = 0);
+
+Q_SIGNALS:


phoerious · 2017-05-14T13:23:05Z

src/core/ScreenLockListenerWin.cpp

+                if (setting != nullptr && setting->PowerSetting == GUID_LIDSWITCH_STATE_CHANGE) {
+                    const DWORD* state = reinterpret_cast<const DWORD*>(&setting->Data);
+                    if (*state == 0) {
+                        Q_EMIT screenLocked();


weslly

Works on macOS Sierra

…#134

widget. Changed wording and preference variable name for conformity with existing settings.

TheZ3ro · 2017-05-17T11:05:10Z

Fixed and rebased @keepassxreboot/core-developers

droidmonkey · 2017-05-18T01:53:25Z

For the record, this PR introduces a dbus interface to KeePassXC.

- Added YubiKey 2FA integration for unlocking databases [#127] - Added TOTP support [#519] - Added CSV import tool [#146, #490] - Added KeePassXC CLI tool [#254] - Added diceware password generator [#373] - Added support for entry references [#370, #378] - Added support for Twofish encryption [#167] - Enabled DEP and ASLR for in-memory protection [#371] - Enabled single instance mode [#510] - Enabled portable mode [#645] - Enabled database lock on screensaver and session lock [#545] - Redesigned welcome screen with common features and recent databases [#292] - Multiple updates to search behavior [#168, #213, #374, #471, #603, #654] - Added auto-type fields {CLEARFIELD}, {SPACE}, {{}, {}} [#267, #427, #480] - Fixed auto-type errors on Linux [#550] - Prompt user prior to executing a cmd:// URL [#235] - Entry attributes can be protected (hidden) [#220] - Added extended ascii to password generator [#538] - Added new database icon to toolbar [#289] - Added context menu entry to empty recycle bin in databases [#520] - Added "apply" button to entry and group edit windows [#624] - Added macOS tray icon and enabled minimize on close [#583] - Fixed issues with unclean shutdowns [#170, #580] - Changed keyboard shortcut to create new database to CTRL+SHIFT+N [#515] - Compare window title to entry URLs [#556] - Implemented inline error messages [#162] - Ignore group expansion and other minor changes when making database "dirty" [#464] - Updated license and copyright information on souce files [#632] - Added contributors list to about dialog [#629]

sergeevabc · 2018-04-04T20:07:26Z

I asked myself “what is lid” and ended up here.
For desktop users this phrase might sound confusing.
Let’s think what exactly happens “when lid is closed”?
Actually the computer is about to be suspended.
So it would be appreciated if you change the label appropriately to relate both to laptops and desktops.

droidmonkey · 2018-04-05T00:05:52Z

Please open a new issue for this.

Bendodroid · 2019-02-14T13:39:24Z

This doesn't seem to work with i3lock on Manjaro. When unlocking the screen, the Database is still unlocked.
Any suggestions?

droidmonkey · 2019-02-14T18:19:54Z

@Bendodroid please open an issue with all the details, thank you.

kiprasmel · 2019-06-29T15:42:38Z

Same here as @Bendodroid - I'll create an issue soon.
Edit: #3339

TheZ3ro added this to the v2.2.0 milestone May 4, 2017

TheZ3ro self-assigned this May 4, 2017

TheZ3ro requested review from droidmonkey, louib and phoerious May 4, 2017 21:25

weslly reviewed May 4, 2017

View reviewed changes

louib reviewed May 6, 2017

View reviewed changes

droidmonkey requested changes May 6, 2017

View reviewed changes

droidmonkey approved these changes May 7, 2017

View reviewed changes

droidmonkey mentioned this pull request May 7, 2017

Feature/lock db on session lock #134 #135

Closed

louib reviewed May 7, 2017

View reviewed changes

louib approved these changes May 12, 2017

View reviewed changes

phoerious requested changes May 14, 2017

View reviewed changes

weslly approved these changes May 14, 2017

View reviewed changes

rockdreamer added 5 commits May 17, 2017 13:04

Lock database when OS is locked (Windows, DBus, macOS implementations) …

2de5a9d

…#134

enable OS X build with Xcode 8.2

c6ecf48

Fix Linux ScreenLockListener implementation

a3af8fc

Avoid warning in MacOS implementation

44085df

Moved "Lock databases on screen lock" setting to security settings

d1acd75

widget. Changed wording and preference variable name for conformity with existing settings.

rockdreamer and others added 8 commits May 17, 2017 13:04

remove commented code

289e98e

Added freedesktop DBus, fixed codestyle

1b7b2ff

Revert travis settings

8ddd0b2

Corrected nullptr crash on Windows when going to sleep

147c000

Moved locking checkboxes into security settings

3218cb9

Add file header, use nullptr instead of NULL, fix code style

533136f

Styling + CFNotificationName -> CFStringRef

970525c

fix codestyle and use C++11 keywords

b69b50c

TheZ3ro force-pushed the feature/lock-db-on-session-lock-#134 branch from 16e695c to b69b50c Compare May 17, 2017 11:04

TheZ3ro mentioned this pull request May 17, 2017

Feature/430 fix unclean shutdown #580

Merged

phoerious added new feature security labels May 17, 2017

phoerious approved these changes May 17, 2017

View reviewed changes

phoerious merged commit c69a978 into develop May 17, 2017

phoerious deleted the feature/lock-db-on-session-lock-#134 branch May 17, 2017 20:43

droidmonkey mentioned this pull request May 18, 2017

Add DBus support #283

Merged

droidmonkey mentioned this pull request May 30, 2017

Lock database when OS is locked #134

Closed

kiprasmel mentioned this pull request Jun 29, 2019

Lock database on SessionLock does not work with i3lock on arch linux #3339

Closed

orestisfl mentioned this pull request Jul 12, 2019

FR: Emit a lock event on lock i3/i3lock#239

Open

karlhorky mentioned this pull request Aug 15, 2020

Flash of entries in locked database when returning to an auto-locked database #5274

Open

phoerious added pr: new feature Pull request adds a new feature and removed new feature labels Nov 22, 2024

Uh oh!

Conversation

TheZ3ro commented May 4, 2017

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

TheZ3ro commented May 4, 2017

Uh oh!

weslly commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Keisial commented May 4, 2017

Uh oh!

sainaen commented May 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheZ3ro commented May 6, 2017

Uh oh!

droidmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louib commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

louib commented May 12, 2017

Uh oh!

phoerious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weslly left a comment

Choose a reason for hiding this comment

Uh oh!

weslly commented May 4, 2017 •

edited

Loading

louib commented May 11, 2017 •

edited

Loading

sergeevabc commented Apr 4, 2018 •

edited

Loading

kiprasmel commented Jun 29, 2019 •

edited

Loading