! Please note that this is a snapshot of our old Bugzilla server, which is read only since May 29, 2020. Please go to gitlab.xfce.org for our new server !
[PATCH] Extend xflock4 with custom screensaver application support
Status:
RESOLVED: FIXED
Product:
Xfce4-session
Component:
General

Comments

Description Andrew Sichevoi 2012-06-07 17:18:53 CEST
Created attachment 4496 
Patch for xflock4 to add custom screensavers support

Currently the entry point for locking the screen for ``xfce4-session'' and ``xfce4-power-manager'' is ``xflock4'' script which is shipped with ``xfce4-session'' package.
     The script checks the list of "known" screensavers and when the first available is found it will be launched.
     By default its knowledge is limited by the following applications: xscreensaver, gnome-screensaver, xlock, slock. No other are supported.
     So the only way to add support of user-preferred screensaver is to hack ``xflock4'' contents which could be overwritten during the next update.

     Please consider the following patch to be reviewed/added.

     The patch allows to specify a custom screensaver application in XfConf registry by the following path: xfce4-session/screenlock/command
     Please find the patch below and on Pastebin: http://pastebin.ca/2158914

=== xflock4-add-custom-screensaver-support.patch ===

diff --git scripts/xflock4 scripts/xflock4
index ec4d05d..f9846ce 100644
--- scripts/xflock4
+++ scripts/xflock4
@@ -24,6 +24,12 @@
 PATH=/bin:/usr/bin
 export PATH
 
+# Lock by user specified screen lock application
+lock_cmd=$(xfconf-query -c xfce4-session -p /screenlock/command 2>/dev/null)
+if [ $? -a ! -z $lock_cmd ]; then
+    $lock_cmd >/dev/null 2>&1 && exit
+fi
+
 # Lock by xscreensaver or gnome-screensaver, if a respective daemon is running
 for lock_cmd in \
     "xscreensaver-command -lock" \

=== xflock4-add-custom-screensaver-support.patch ===
Comment 1 Guido Berhoerster 2012-06-07 19:09:51 CEST
(In reply to comment #0)
> +# Lock by user specified screen lock application
> +lock_cmd=$(xfconf-query -c xfce4-session -p /screenlock/command 2>/dev/null)
> +if [ $? -a ! -z $lock_cmd ]; then

This is broken, $lock_cmd must be quoted since it undergoes word splitting, also there is no need to use the non-portable -a switch and check $?, finally ! -z is better expressed as -n:

if lock_cmd=$(xfconf-query -c xfce4-session -p /screenlock/command 2>/dev/null) && [ -n "${lock_cmd}" ]; then

> +    $lock_cmd >/dev/null 2>&1 && exit

The problem with that is that there will be no consistent behavior with regard to whether xflock4 exits upon locking the screen, tools such as xlock or slock remain in the foreground.
Comment 2 Andrew Sichevoi 2012-06-09 05:11:55 CEST
(In reply to comment #1)

> (In reply to comment #0) 
> This is broken,

Guido, thanks for the correction.

> The problem with that is that there will be no consistent behavior with
> regard to whether xflock4 exits upon locking the screen, tools such as xlock
> or slock remain in the foreground.

Is not it possible to make a contract that any custom screensaver application specified in that way should run on the background? So for some applications which remain in the foreground after start a user will be able to create a simple wrapping script which does so.
Comment 3 Guido Berhoerster 2012-06-09 10:12:48 CEST
(In reply to comment #2)
> > The problem with that is that there will be no consistent behavior with
> > regard to whether xflock4 exits upon locking the screen, tools such as xlock
> > or slock remain in the foreground.
> 
> Is not it possible to make a contract that any custom screensaver
> application specified in that way should run on the background? So for some
> applications which remain in the foreground after start a user will be able
> to create a simple wrapping script which does so.

Yes, you would have to make that a requirement that the custom command exits after trying to lock the screen. IMHO that's not very elegant but I'm not a Xfce developer who gets to decide on that.
Comment 4 Jarno Suni 2014-01-03 14:29:20 CET
Couldn't the property value have & in its value, if the preferred command needs manual backgrounding? I am using Xfce 4.10.1, and I can not create a property named /screenlock/command for xfce4-session using the xfce-settings-editor.

Anyway, I have proposed xflock4 that supports more alternatives at https://bugzilla.xfce.org/show_bug.cgi?id=3770
One of them, named xautolock, adds support for custom lockers, but it does not have Xfce specific configuration. It needs to be started as a daemon, instead.
Comment 5 Raphael Groner 2014-11-22 17:14:19 CET
See also bug #11324.
Comment 6 Raphael Groner 2014-11-22 17:15:24 CET
*** Bug 11324 has been marked as a duplicate of this bug. ***
Comment 7 Raphael Groner 2015-02-06 19:16:11 CET
What's the progress here? It would be nice to have this patch in the official 4.12 release.
Comment 8 Steve Dodier-Lazaro editbugs 2015-02-07 12:37:22 CET
I am against this patch being applied. Xflock4 already needs fixing in that it allows people to launch arbitrary screensavers (e.g. ~/.local/bin/xscreensaver) by tweaking their session environment, which is insecure.

The locker could of course have a form of xfconf key to help users choose their screensaver, but all screensavers should be looked up exclusively in /usr, and we should check the actual binaries are owned by root. I'll make a separate report with more details about that.

Wrt. the patch itself, why not add the content of the xfconf key to the existing locker lists? And in fact if there are two very specific behaviours, two separate xfconf keys could be used. In any case the script would need to be rewritten to ensure it picks the list with the user-chosen locker first.
Comment 9 Guido Berhoerster 2015-02-07 15:53:18 CET
(In reply to Steve Dodier-Lazaro from comment #8)
> I am against this patch being applied. Xflock4 already needs fixing in that
> it allows people to launch arbitrary screensavers (e.g.
> ~/.local/bin/xscreensaver) by tweaking their session environment, which is
> insecure.
> 
> The locker could of course have a form of xfconf key to help users choose
> their screensaver, but all screensavers should be looked up exclusively in
> /usr, and we should check the actual binaries are owned by root. I'll make a
> separate report with more details about that.

Steve, please do so on bug #10217 so that it is all in one place and have a look at comments #20, #21, and #22 (you can ignore the rest) about a sane redesign and reliable locking as proposed by Eric, me and Simon.

I'm afraid this is a messy topic with lots of different bug reports where people try to add ever more layers upon a fundamentally flawed design we currently have.

> Wrt. the patch itself, why not add the content of the xfconf key to the
> existing locker lists? And in fact if there are two very specific
> behaviours, two separate xfconf keys could be used. In any case the script
> would need to be rewritten to ensure it picks the list with the user-chosen
> locker first.

No need to, the script just needs to go away, see the proposal in bug #10217.
Comment 10 Ali Akcaagac 2015-02-20 23:24:51 CET
I just wanted to open a bug about lightdm lock support and was redirected to this bug here (due duplication). lightdm has an own locking mechanism which can be fired up with "dm-tool lock" (provided with lightdm). My xflock4.sh file only contains a oneliner:

#!/bin/bash --login
# Begin xflock4.sh

dm-tool lock

exit
# End xflock4.sh

I would be thankful if dm-took lock could be added bevor or behind "slock" within the original xflock4.sh file before 4.12. Thanks.
Comment 11 Jarno Suni 2016-01-29 21:32:03 CET
(In reply to Steve Dodier-Lazaro from comment #8)
> I am against this patch being applied. Xflock4 already needs fixing in that
> it allows people to launch arbitrary screensavers (e.g.
> ~/.local/bin/xscreensaver) by tweaking their session environment, which is
> insecure.

True, PATH setting is not relevant there, when you can give an absolute path in the xfconf variable.

> The locker could of course have a form of xfconf key to help users choose
> their screensaver, but all screensavers should be looked up exclusively in
> /usr, and we should check the actual binaries are owned by root. I'll make a
> separate report with more details about that.

In bug 12282 it requested that locker should be accepted outside /usr, too. How would you check, if the locker command in arbitrary string is owned by root? Note, that the string could be e.g. 'time dash -c "mylocker --myoption"'

> Wrt. the patch itself, why not add the content of the xfconf key to the
> existing locker lists? And in fact if there are two very specific
> behaviours, two separate xfconf keys could be used. In any case the script
> would need to be rewritten to ensure it picks the list with the user-chosen
> locker first.

What do you mean by using two separate xfconf keys?
Comment 12 Jarno Suni 2016-02-02 16:15:10 CET
I think the xfconf variable could be used to select a locker from a list that is maintained by administrator; that way (with additional information given by administrator) system would know, if the selected locker command forks, and could rely on the command being reasonable. Also the possible respective daemon could be started on demand.

On the other hand, as for Andrew Sichevoi's concern about custom xflock4 being overwritten by update, at least in Xubuntu Linux I have used the following strategy:
I store custom xflock4 in /usr/local/bin which is set in PATH before /usr/bin where the supplied xflock4 is located. (That does not work, if I call xflock4 securely by `command -p xflock4`.)
Comment 13 socexema 2016-03-14 21:05:17 CET
> I am against this patch being applied. Xflock4 already needs fixing in that it allows people to launch arbitrary screensavers (e.g. ~/.local/bin/xscreensaver) by tweaking their session environment, which is insecure.

But as I know, third-party cannot modify anything in user's home directory. To modify files in home directory one must have corresponding authority. Is it really insecure?

By the way, is there any progress concerning this enhancement?
Comment 14 Jarno Suni 2016-03-15 09:50:38 CET
It is in current Git, but using a different xfconf variable: http://git.xfce.org/xfce/xfce4-session/tree/scripts/xflock4
Comment 15 Jarno Suni 2016-03-15 09:55:03 CET
Steve Dodier-Lazaro, are you still there? Where is the report you referred to?
Comment 16 socexema 2016-03-16 05:49:08 CET
So, does it mean that I can put any command string to /general/LockCommand, and this command will be invoked on xflock4 invocation?

By the way, do we have something like that for "Switch user"? As I know, it currently looks for gdmflexiserver, but it is only provided by gdm, though other display managers may have their own mechanisms: for example, light-locker has "dm-tool switch-to-greeter"

And one final question (sorry for something like offtopic): Do we have something to automatically enable screensaver/screenlocker on session idle/suspend/hibernate? I mean, some screensavers (simple ones like i3lock, slock, etc.) do not have such mechanisms and are only invoked directly by command. It would be great if xfce user could open xfce4-settings-manager and find settings like "Run command when session id idle for N minutes" and "Run command when going/returning from hibernate/suspend"
Comment 17 Jarno Suni 2016-03-16 13:29:57 CET
(In reply to socexema from comment #16)
> So, does it mean that I can put any command string to /general/LockCommand,
> and this command will be invoked on xflock4 invocation?

Yes.

> By the way, do we have something like that for "Switch user"? As I know, it
> currently looks for gdmflexiserver, but it is only provided by gdm, though
> other display managers may have their own mechanisms: for example,
> light-locker has "dm-tool switch-to-greeter"

Switch User may not work, but you can make a script to do the job and setup a panel launcher to run it. xflock4 should be run successfully before running dm-tool.

> And one final question (sorry for something like offtopic): Do we have
> something to automatically enable screensaver/screenlocker on session
> idle/suspend/hibernate? I mean, some screensavers (simple ones like i3lock,
> slock, etc.) do not have such mechanisms and are only invoked directly by
> command. It would be great if xfce user could open xfce4-settings-manager
> and find settings like "Run command when session id idle for N minutes" and
> "Run command when going/returning from hibernate/suspend"

Yes we do. It is in the session settings or power manager settings. You can choose, if xflock4 is called or not. IIRC the power manager may call some locker even if xflock4 is not found. Also screensavers may have their own settings.
Comment 18 socexema 2016-03-16 13:40:54 CET
In power management I found an option to lock screen when system is going for sleep. And what about locker activation on session idle, i.e. when I do not do anything for N minutes?
Comment 19 Jarno Suni 2016-03-16 17:13:31 CET
(In reply to socexema from comment #18)
> In power management I found an option to lock screen when system is going
> for sleep. And what about locker activation on session idle, i.e. when I do
> not do anything for N minutes?

Why don't you inspect the settings of your screensaver like I hinted before and continue discussion in a support channel of the operating system you are using?
Comment 20 Jarno Suni 2016-03-16 17:50:04 CET
(In reply to socexema from comment #18)
> In power management I found an option to lock screen when system is going
> for sleep. And what about locker activation on session idle, i.e. when I do
> not do anything for N minutes?

Or use Xfce's discussion channels: http://www.xfce.org/community
Comment 21 socexema 2016-03-16 18:27:11 CET
Of course, sorry for offtopic. Thank you.

Bug #8993

Reported by:
Andrew Sichevoi
Reported on: 2012-06-07
Last modified on: 2019-04-19
Duplicates (1):
  • 11324 RFE: Exo plugin for xflock4 / Customizable default locker

People

Assignee:
Steve Dodier-Lazaro
CC List:
8 users

Version

Version:
4.10.0

Attachments

Additional information