search

mate-desktop / mate-screensaver

MATE screen saver and locker
https://mate-desktop.org
GNU General Public License v2.0
48 stars 40 forks source link

Ensure lock on suspend and unlock on resume #202

Closed apaan closed 5 years ago

apaan commented 5 years ago

This patch fixes two issues #145 and #175.

It is adopted from xfce4-screensaver patch: https://git.xfce.org/apps/xfce4-screensaver/commit/?id=9e53bb2866

Introduced new Inhibitor lock to make sure lock was in place and user will be greeted with unlock dialog when resumes from suspend/ hibernate.

lukefromdc commented 5 years ago

Can users turn this on and off with a gsetting? Not everyone wants the screen locked when coming out of suspend. Some need is and some do not.

The obvious example of this wouldbe many users of encrypted laptops, who must maintain absolute custody of the machine anyway whenever it is booted as cold boot attacks and other means can recover keys from encrypted computers stolen/confiscated while running and decrypted.

apaan commented 5 years ago

Testing on my laptop, without this patch, mate-screensaver behaves according to option "Lock screen when screensaver is active" in Screensaver Preferences (stored in lock-enabled gsetting):

If this is correct, i will modify the patch so that the behavior will not change.

Thanks

lukefromdc commented 5 years ago

The described behavior in master "Lock screen when screensaver is active" should be the correct behavior and is simpler than adding a new and separate action.

monsta commented 5 years ago

Xfce's screensaver is a fork of this one... how does it work without that new inhibitor?

monsta commented 5 years ago

The described behavior in master "Lock screen when screensaver is active" should be the correct behavior and is simpler than adding a new and separate action.

I recall we have a bunch of "lock-on-something" keys in mate-power-manager schema. However, it's probably only related to cases when suspend is activated on (hardware) button press or on closing the laptop lid...

apaan commented 5 years ago

Xfce's screensaver is a fork of this one... how does it work without that new inhibitor?

Actually I started by copying the Xfce's patch (I even put a link to it in the commit message). But it didn't always work on my notebook. So i dug logind documentation and found this (taken from https://www.freedesktop.org/wiki/Software/systemd/inhibit/):

Note that watching PrepareForShutdown(true)?/PrepareForSleep(true) without taking a delay lock is racy and should not be done, as any code that an application might want to execute on this signal might not actually finish before the suspend/shutdown cycle is executed. Again: if you watch PrepareForSuspend(true), then you really should have taken a delay lock first.

apaan commented 5 years ago

The described behavior in master "Lock screen when screensaver is active" should be the correct behavior and is simpler than adding a new and separate action.

I recall we have a bunch of "lock-on-something" keys in mate-power-manager schema. However, it's probably only related to cases when suspend is activated on (hardware) button press or on closing the laptop lid...

Just checked that. mate-power-manager has lock-use-screensaver: Whether to use the screen lock setting of mate-screensaver to decide if the screen is locked after a hibernate, suspend or blank screen.

Current behavior is different from master in this regard. Will look into it later

monsta commented 5 years ago

Ok, so we delay the suspend until we display the lock dialog, and then release the lock...

Note that watching PrepareForShutdown(true)?/PrepareForSleep(true) without taking a delay lock is racy and should not be done, as any code that an application might want to execute on this signal might not actually finish before the suspend/shutdown cycle is executed. Again: if you watch PrepareForSuspend(true), then you really should have taken a delay lock first.

So that's why we also acquire the lock after resuming? To play it safe for the next PrepareForSleep...

monsta commented 5 years ago

Interesting that Xfce's screensaver still doesn't have neither inhibit nor showing the dialog before the suspend...

raveit65 commented 5 years ago

So that's why we also acquire the lock after resuming? To play it safe for the next PrepareForSleep...

Why not simply testing the PR? it's self explanatory....

apaan commented 5 years ago

The described behavior in master "Lock screen when screensaver is active" should be the correct behavior and is simpler than adding a new and separate action.

I recall we have a bunch of "lock-on-something" keys in mate-power-manager schema. However, it's probably only related to cases when suspend is activated on (hardware) button press or on closing the laptop lid...

Pushed a change: This patch now lets mate-power-manager to lock or not. This patch only ensures that the first thing that we see on resume is unlock dialog. Same behavior as before, but now, this patch behaves correctly on hidden mate-power-manager settings.

raveit65 commented 5 years ago

@apaan I found the working version of your pull request. https://github.com/mate-desktop/mate-screensaver/compare/410fcb6fcbff537ba0cb24b834a86c68721cdb7d..4dbbbe7b8e72e3a54b5d0657a94b794bcee1ab04.patch Thanks for that :) I will use that for fedora and step back from the review here, because I have no idea why reviewers breaks your work here with their ideas....

vkareh commented 5 years ago

Still need to figure out why it doesn't work for @raveit65 - that's a potential blocker...

monsta commented 5 years ago

This also works for me if I disable locking in screensaver settings, but explicitly tell the power manager to ask the screensaver to lock on suspend.

$ gsettings get org.mate.screensaver lock-enabled
false
$ gsettings get org.mate.power-manager lock-use-screensaver
false
$ gsettings get org.mate.power-manager lock-suspend
true

Suspend was tested by setting the power button to perform suspend in power manager settings, then pressing the power button (obviously).

monsta commented 5 years ago

The commit message should probably be changed, because lock-on-suspend is already implemented, and this change just ensures it...

vkareh commented 5 years ago

I agree - confused me the first time I tried this

raveit65 commented 5 years ago

Ok, i was able to reproduce the issue and i don't think it is related to PR. On a system with 2 monitors (non laptop) is is possible that the lock-dialog is on a monitor which isn't powered on after coming from suspend-to-ram = Computer are stupid! Than you see only a black screen at the monitor which is on. In details:

  1. Both monitors are enabled in mate-window-properties
  2. Suspend the system
  3. Plug off the second monitor from electrical net to save power.
  4. Go shopping (forget everything!)
  5. Come back from shopping, and start system from suspend with one monitor.
  6. Have a black screen without a lock-dialog.

Note, those steps aren't always reproducible and i think this is a monitor detection issue with a multi-monitor setup, not related to mate-screensaver.

apaan commented 5 years ago

Force-pushed a new version of this patch: