GNOME Bugzilla – Bug 643357
Screen should be locked before suspend
Last modified: 2011-03-09 21:00:20 UTC
The current suspend menu item does just suspend the system but not lock the screen, which is problematic as from this point on anyone can access the system without authentication (as compared to shutdown or the screensaver).
Created attachment 181976 [details] [review] Util: Add a spawn_sync method Add a synchronous of Util.spawn()
Created attachment 181977 [details] [review] statusMenu: Lock screen before suspending We need to lock the screen before suspending the system to prevent unauthorized access to the system / account.
Created attachment 181978 [details] [review] Util: Add a spawn_sync method Add a synchronous version of Util.spawn() --- Fix commit message
Comment on attachment 181978 [details] [review] Util: Add a spawn_sync method >+function spawn_sync(argv) { should be spawnSync >+ if (err.code == GLib.SpawnError.G_SPAWN_ERROR_NOENT) { >+ err.message = _("Command not found"); >+ } else { >+ // The exception from gjs contains an error string like: >+ // Error invoking GLib.spawn_command_line_async: Failed to >+ // execute child process "foo" (No such file or directory) >+ // We are only interested in the part in the parentheses. (And >+ // we can't pattern match the text, since it gets localized.) >+ err.message = err.message.replace(/.*\((.+)\)/, '$1'); split this out into a utility method to share between trySpawn and trySpawnSync (and trySpawnDesktop). Especially since we already know the G_SPAWN_ERROR_NOENT is wrong and needs to be fixed, and we don't want it to get fixed in one place but not the other.
Comment on attachment 181977 [details] [review] statusMenu: Lock screen before suspending >+ Util.spawn_sync(['gnome-screensaver-command', '--lock']); will need to be updated. Otherwise good.
Is there a reason we're not using the DBus API?
Just curious, are we checking suspend inhibitors too?
(In reply to comment #6) > Is there a reason we're not using the DBus API? The only reason why I didn't was that the rest of the code doesn't either. (In reply to comment #7) > Just curious, are we checking suspend inhibitors too? No; and wouldn't make sense here as this is a user requested suspend.
We inform the user at power off or logout even if they request it. You have the opportunity to insist but it is helpful to be reminded I think.
Created attachment 182104 [details] [review] statusMenu: Use the screensaver's dbus interface directly Use the dbus interface instead of calling gnome-screensaver-command.
Created attachment 182105 [details] [review] statusMenu: Lock screen before suspending We need to lock the screen before suspending the system to prevent unauthorized access to the system / account.
Created attachment 182107 [details] [review] statusMenu: Use the screensaver's dbus interface directly Use the dbus interface instead of calling gnome-screensaver-command.
Review of attachment 182105 [details] [review]: js/ui/statusMenu.js 214 if (this._haveSuspend && 215 this._suspendOrPowerOffItem.state == PopupMenu.PopupAlternatingMenuItemState.DEFAULT) { 216 this._screenSaverProxy.LockRemote(); This is actually asynchronous. To make it synchronous you'd need to create a recursive mainloop. Something like: this._screenSaverProxy.LockRemote(function (result, excp) { Mainloop.quit('screensaver-lock'); }); Mainloop.run('screensaver-lock') (The gjs dbus stuff is very poorly documented =( Sorry about that ) ::: js/ui/statusMenu.js @@ +214,3 @@ if (this._haveSuspend && this._suspendOrPowerOffItem.state == PopupMenu.PopupAlternatingMenuItemState.DEFAULT) { + this._screenSaverProxy.LockRemote(); This is actually asynchronous. To make it synchronous you'd need to create a recursive mainloop. Something like: this._screenSaverProxy.LockRemote(function (result, excp) { Mainloop.quit('screensaver-lock'); }); Mainloop.run('screensaver-lock')
(In reply to comment #13) > Review of attachment 182105 [details] [review]: > > js/ui/statusMenu.js > 214 if (this._haveSuspend && > 215 this._suspendOrPowerOffItem.state == > PopupMenu.PopupAlternatingMenuItemState.DEFAULT) { > 216 this._screenSaverProxy.LockRemote(); > > This is actually asynchronous. To make it synchronous you'd need to create a > recursive mainloop. > Something like: > this._screenSaverProxy.LockRemote(function (result, excp) { > Mainloop.quit('screensaver-lock'); }); > Mainloop.run('screensaver-lock') > > (The gjs dbus stuff is very poorly documented =( Sorry about that ) This seems to take a very long time to actually be called which leaves the shell frozen for serval seconds without being able to redraw anything. I tried this._screenSaverProxy.LockRemote(Lang.bind(this, function() { this._upClient.suspend_sync(null); })); Which has pretty much the same problem the screen locks and it suspends a while after that. I can even unlock the screen and watch it randomly suspend after that. Any idea what is taking so long for it to respond?
Created attachment 182199 [details] [review] statusMenu: Lock screen before suspending We need to lock the screen before suspending the system to prevent unauthorized access to the system / account. --- With Colin's gnome-screensaver fix this works now (sync dbus call).
(In reply to comment #15) > Created an attachment (id=182199) [details] [review] > statusMenu: Lock screen before suspending > > We need to lock the screen before suspending the system > to prevent unauthorized access to the system / account. > > --- > > With Colin's gnome-screensaver fix this works now (sync dbus call). http://fpaste.org/1a8N/ is the fix.
(In reply to comment #15) > Created an attachment (id=182199) [details] [review] > statusMenu: Lock screen before suspending > > We need to lock the screen before suspending the system > to prevent unauthorized access to the system / account. > > --- > > With Colin's gnome-screensaver fix this works now (sync dbus call). But will make Suspend just hang if invoked with an older gnome-screensaver?
(In reply to comment #17) > (In reply to comment #15) > > Created an attachment (id=182199) [details] [review] [details] [review] > > statusMenu: Lock screen before suspending > > > > We need to lock the screen before suspending the system > > to prevent unauthorized access to the system / account. > > > > --- > > > > With Colin's gnome-screensaver fix this works now (sync dbus call). > > But will make Suspend just hang if invoked with an older gnome-screensaver? Yes :/ it will hang and suspend after the dbus timeout.
I'm OK with just generating a hang with old gnome-screensaver. Need to focus on making things work with GNOME 3.0. I don't think we should add gnome-screensaver to the gnome-shell jhbuild for this, because it's not going to be reliably the gnome-screensaver process running. Only alternative I can think of is to do what gnome-power-manager does and just poll in a loop until the screen is locked, but that's more code.
(In reply to comment #19) > I'm OK with just generating a hang with old gnome-screensaver. Need to focus on > making things work with GNOME 3.0. I don't think we should add > gnome-screensaver to the gnome-shell jhbuild for this, because it's not going > to be reliably the gnome-screensaver process running. > > Only alternative I can think of is to do what gnome-power-manager does and just > poll in a loop until the screen is locked, but that's more code. Given that we use the names of gnome-session tools that do not exist in pre 3.0 gnome-session (be775b9206); it should be fine to add this "breakage" here too (it works it will just hang until the timeout so technically the gnome-session breakage is even worse).
Review of attachment 182199 [details] [review]: Looks fine.
Attachment 182107 [details] pushed as 7f3920d - statusMenu: Use the screensaver's dbus interface directly Attachment 182199 [details] pushed as 162d029 - statusMenu: Lock screen before suspending