Feature/lock db on session lock #134 by rockdreamer · Pull Request #135 · keepassxreboot/keepassxc

rockdreamer · 2016-12-31T09:21:43Z

Description

Added a configuration entry in general 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

Screenshots (if appropriate):

Types of changes

❎ Bug fix (non-breaking change which fixes an issue)
✅ New feature (non-breaking change which adds functionality)
❎ Breaking change (fix or feature that would cause existing functionality to change)

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]
❎ My change requires a change to the documentation.
❎ I have updated the documentation accordingly.
❎ I have added tests to cover my changes.

…keepassxreboot#134

TheZ3ro · 2016-12-31T10:54:42Z

src/core/ScreenLockListenerDBus.cpp

+                this, //receiver
+                SLOT(unityLocked()));
+
+    /* Currently unable to get the current user session from login1.Manager


Are this comments useful or they can be removed?

Hi TheZ3ro and Happy new year :)
I would remove the comment and add a separate issue.
In theory the login1.Manager interface should provide a common API for handling session status notifications, however I had issues making it work. when I passed the current pid to the getsession call, it wouldn't return a valid session url. Maybe it only works on consoles but it would be nice if it worked across desktop managers.
Once this issue is resolved, I would de-comment and add a signal listener on the returned session much like what is currently done for unity, gnome session etc.

TheZ3ro · 2016-12-31T11:00:14Z

I think it's appropriate moving the option under the Security configuration screen. All the configurations that interacts with the DB are placed there.

The code seems good, I will test this in the next few days.
(Happy 2017 to all the project contributors and maintainers 🎉 )

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

TheZ3ro · 2017-01-01T15:22:10Z

Tested on my Ubuntu/XFCE/LightDM. When I lock the screen KeePassXC doesn't lock.

The only dbus signal emitted on locking is this:

signal time=1483283213.169993 sender=:1.0 -> destination=(null destination) serial=4200 path=/com/ubuntu/Upstart; interface=com.ubuntu.Upstart0_6; member=EventEmitted
   string "bdi-device-added"
   array [
      string "KERNEL=0:46"
      string "DEVPATH=/devices/virtual/bdi/0:46"
      string "SUBSYSTEM=bdi"
      string "ACTION=add"
      string "SEQNUM=7951"
      string "USEC_INITIALIZED=1564711295871"
   ]
signal time=1483283218.842816 sender=:1.0 -> destination=(null destination) serial=4201 path=/com/ubuntu/Upstart; interface=com.ubuntu.Upstart0_6; member=EventEmitted
   string "bdi-device-removed"
   array [
      string "KERNEL=0:46"
      string "DEVPATH=/devices/virtual/bdi/0:46"
      string "SUBSYSTEM=bdi"
      string "ACTION=remove"
      string "SEQNUM=7952"
      string "USEC_INITIALIZED=1564716968162"
   ]

So I think we should add more DBus signal

Under xUbuntu/XFCE:
interface=com.ubuntu.Upstart0_6; path=/com/ubuntu/Upstart; member=EventEmitted

Under KDE it's something like: source
interface=org.freedesktop.ScreenSaver, path=/org/freedesktop/ScreenSaver, member=GetActive

Under Cinnamon something like: source
interface=org.cinnamon.ScreenSaver, path=/org/cinnamon/ScreenSaver

I think we should cover them all (unfortunately)

phoerious · 2017-01-19T00:02:25Z

I did a little research and downloaded a couple of live systems to have look at what D-Bus signals are emitted on different distributions.

I guess it would be enough to add

// ...

    sessionBus.connect(
                "org.freedesktop.ScreenSaver", // service
                "/org/freedesktop/ScreenSaver", // path
                "org.freedesktop.ScreenSaver", // interface
                "ActiveChanged", // signal name
                this, //receiver
                SLOT(freedesktopScreenSaver(bool)));

// ...

void ScreenLockListenerDBus::freedesktopScreenSaver(bool status)
{
    if (status)
        Q_EMIT screenLocked();
}

That locks the database on my main KDE system and the same signal is also emitted in the Manjaro XFCE VM I tried. Looks like this is the freedesktop.org standard interface, unfortunately not implemented by all desktop environments.

com.canonical.Unity.Session appears redundant to me because apparently, Ubuntu also sends org.gnome.SessionManager.Presence. Also the member name Locked seems to be incorrect. At least my 16.10 Ubuntu VM used Lock instead. If you are Ubuntu users, you can maybe test if everything still works if you remove the connection to com.canonical.Unity. If that is the case, I would prefer to remove it and only use the org.gnome.SessionManager.Presence interface for Unity or Gnome-based distributions.

I did not test org.freedesktop.login1.Manager because I couldn't get the live system in my VM to suspend and resume properly.

Additionally, I think we should enable the option to lock the database when the screen is locked by default. You shouldn't accidentally leave your password database unlocked when you leave the PC.

phoerious

I added what I summarized in the posting above as inline comments. Please have a look.

phoerious · 2017-01-19T00:12:02Z

src/gui/SettingsWidget.cpp

    m_secUi->lockDatabaseIdleCheckBox->setChecked(config()->get("security/lockdatabaseidle").toBool());
    m_secUi->lockDatabaseIdleSpinBox->setValue(config()->get("security/lockdatabaseidlesec").toInt());
    m_secUi->lockDatabaseMinimizeCheckBox->setChecked(config()->get("security/lockdatabaseminimize").toBool());
+    m_secUi->lockDatabaseOnScreenLockCheckBox->setChecked(config()->get("security/lockdatabasescreenlock").toBool());


Enable this option by default.

phoerious · 2017-01-19T00:12:54Z

src/core/ScreenLockListenerWin.cpp

+    HWND h= reinterpret_cast<HWND>(static_cast<QWidget*>(parent())->winId());
+    WTSUnRegisterSessionNotification(h);
+
+    if(m_powernotificationhandle){


Coding style, please add a space after if. Also applies to the following if conditions in this file. Also add spaces before opening braces.

phoerious · 2017-01-19T00:13:14Z

src/core/ScreenLockListenerWin.cpp

+            }
+        }
+        if(m->message == WM_WTSSESSION_CHANGE){
+            if (m->wParam==WTS_CONSOLE_DISCONNECT){


Again coding style, please add spaces around == operators and before opening braces.

phoerious · 2017-01-19T00:13:37Z

src/core/ScreenLockListenerWin.cpp

+                Q_EMIT screenLocked();
+                return true;
+            }
+            if (m->wParam==WTS_SESSION_LOCK){


phoerious · 2017-01-19T00:14:44Z

src/core/ScreenLockListenerDBus.h

+private Q_SLOTS:
+    void gnomeSessionStatusChanged(uint status);
+    void logindPrepareForSleep(bool beforeSleep);
+    void unityLocked();


Add

void freedesktopScreenSaver(bool status);

unityLocked() probably not needed.

phoerious · 2017-01-19T00:16:00Z

src/core/ScreenLockListenerDBus.cpp

+void ScreenLockListenerDBus::unityLocked()
+{
+    Q_EMIT screenLocked();
+}


Add

void ScreenLockListenerDBus::freedesktopScreenSaver(bool status) { if (status) Q_EMIT screenLocked(); }

phoerious · 2017-01-19T00:16:11Z

src/core/ScreenLockListenerDBus.cpp

+    }
+}
+
+void ScreenLockListenerDBus::unityLocked()


Probably not needed.

phoerious · 2017-01-19T00:16:17Z

src/core/ScreenLockListenerDBus.cpp

+                this, //receiver
+                SLOT(logindPrepareForSleep(bool)));
+
+    sessionBus.connect(


com.canonical.Unity probably not needed.

phoerious · 2017-01-19T00:16:36Z

src/core/ScreenLockListenerDBus.cpp

+                "StatusChanged", // signal name
+                this, //receiver
+                SLOT(gnomeSessionStatusChanged(uint)));
+


Add

sessionBus.connect( "org.freedesktop.ScreenSaver", // service "/org/freedesktop/ScreenSaver", // path "org.freedesktop.ScreenSaver", // interface "ActiveChanged", // signal name this, //receiver SLOT(freedesktopScreenSaver(bool)));

droidmonkey · 2017-04-16T12:54:45Z

Ping @rockdreamer , this pr needs a fair bit of work. Are you still interested in contributing to it?

droidmonkey · 2017-05-07T02:47:02Z

I believe this PR has become overcome by #545. Recommend closing.

rockdreamer added 4 commits December 24, 2016 01:04

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

f616de2

…keepassxreboot#134

enable OS X build with Xcode 8.2

6528012

Fix Linux ScreenLockListener implementation

d858a5f

Avoid warning in MacOS implementation

b402b42

TheZ3ro reviewed Dec 31, 2016

View reviewed changes

rockdreamer added 2 commits January 1, 2017 14:08

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

6bb4dce

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

remove commented code

775dd7e

droidmonkey added new feature security labels Jan 5, 2017

droidmonkey added this to the v2.2.0 milestone Jan 5, 2017

phoerious requested changes Jan 19, 2017

View reviewed changes

This was referenced Jan 23, 2017

Efficient DB locking mechanisms (global user inactivity, session lock, etc.) #100

Closed

Lock database when locking screen / suspending #189

Closed

Auto-lock DB when locking computer (Mac) #249

Closed

phoerious mentioned this pull request Feb 10, 2017

Add DBus support #283

Merged

TheZ3ro mentioned this pull request Feb 15, 2017

Add lock timeout based on overall system activity #310

Open

phoerious added the needs work label Feb 20, 2017

phoerious mentioned this pull request Mar 9, 2017

Locking the DB after locking the lap / hibernating #384

Closed

droidmonkey added platform: Linux platform: macOS platform: Windows labels Apr 16, 2017

TheZ3ro self-assigned this May 4, 2017

TheZ3ro mentioned this pull request May 4, 2017

Lock database on ScreenSaver/SessionLock #545

Merged

droidmonkey removed this from the v2.2.0 milestone May 7, 2017

TheZ3ro closed this May 7, 2017

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

Uh oh!

Conversation

rockdreamer commented Dec 31, 2016

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

TheZ3ro Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockdreamer Jan 1, 2017

Choose a reason for hiding this comment

Uh oh!

TheZ3ro commented Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheZ3ro commented Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phoerious commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phoerious left a comment

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

droidmonkey commented Apr 16, 2017

Uh oh!

droidmonkey commented May 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TheZ3ro Dec 31, 2016 •

edited

Loading

TheZ3ro commented Dec 31, 2016 •

edited

Loading

TheZ3ro commented Jan 1, 2017 •

edited

Loading

phoerious commented Jan 19, 2017 •

edited

Loading

phoerious Jan 19, 2017 •

edited

Loading

phoerious Jan 19, 2017 •

edited

Loading

phoerious Jan 19, 2017 •

edited

Loading