GNOME Bugzilla – Bug 693385
Don't ask for a password on shutdown
Last modified: 2013-02-11 20:06:44 UTC
IRC discussion: <drago01> poettering: we had the discussion re shutdown password a few days ago so ... http://fpaste.org/P70v/ ? <poettering> drago01: did you discuss this with the g-s folks? what they want there? also, g-s would need a dialog or so at least to warn the local user <poettering> drago01: and your patch comment is a bit misleading, since logind won't distuingish between local and remote sessions for the policykit check <poettering> or hmm <poettering> so you mean the default for local users should be that they don't need privs to shut off other people <poettering> you don't care if those other users are local or not <poettering> hmm, i guess your patch does enough for that <poettering> drago01: anyway, we can change that, but I'd like some feedback from the gnome-shell folks first, and mayba bug filed or so, to request that there's a warning about other logged in sessions along with the current inhibitor warnings <drago01> poettering: ok fair enough Basically the point is instead of asking for a password we should just warn that another session is active so that the user can decide to either shutdown anyway or cancel the operation. Asking for a password in that case is just annoying.
I very much agree about the password entry being totally silly. Warning is fine.
Created attachment 235541 [details] [review] Don't ask for a password on shutdown Show a warning instead when multiple sessions are present.
Created attachment 235542 [details] [review] logind: Allow active sessions to always shutdown by default Currently local user are being asked for an admin password when another user is logged into the system. This does not make sense as the user has the power to shut down the system anyway regardless if he/she knows the password or not (by pulling the plug, battery or whatever). So only require the admin authentification for remote sessions. --- Systemd patch.
Created attachment 235546 [details] Screenshot with patch applied
Review of attachment 235541 [details] [review]: Code looks good to me, but I'd like designer OK on this as well - the text sounds a bit technical to me (maybe use "Other users are currently signed in" instead?), not to mention the exclamation mark :-)
(In reply to comment #5) > Review of attachment 235541 [details] [review]: > > Code looks good to me, but I'd like designer OK on this as well - the text > sounds a bit technical to me (maybe use "Other users are currently signed in" > instead?), not to mention the exclamation mark :-) Yeah I know suggestions for better text are welcome ... I just had to put in something ;)
We discussed it on IRC a bit and the string we came up with is "Other users are logged in."
I wonder whether it might make sense to show the list of logged in users (like we do for apps in the terminate session/unmount dialogs), but that will pose problems when the same user is also logged in through SSH/VT. So maybe just saying "Other users are logged in" is the best option here...
I think it would be very useful to see who else is logged in... The screenshot like it is doesn't look particularly helpful to figure out what is going on. I for one would really like to know if that other person is just root (i.e. also me), or somebody completely different... (In reply to comment #8) > I wonder whether it might make sense to show the list of logged in users (like > we do for apps in the terminate session/unmount dialogs), but that will pose > problems when the same user is also logged in through SSH/VT. Well, it's trivial to filter for that. To get a list of current logged in users do this: uid_t *u = NULL; int i, r; r = sd_get_uids(&u); for (i = 0; i < r; i++) { struct passwd *pw; if (u[i] == getuid()) continue; pw = getpwuid(u[i]); if (pw) g_message("User %s is logged in.", pw->pw_name); else g_message("User %lu is logged in.", (unsigned long) u[i]); } free(u); And that's already it. Very simple and synchronous and stuff. If you want to be fancy you could even lookup the user icon or so from the UID via accounts service.
Created attachment 235553 [details] [review] Don't ask for a password on shutdown Show a warning instead when multiple sessions are present. --- Improved strings based on designer feedback.
(In reply to comment #9) > I think it would be very useful to see who else is logged in... The screenshot > like it is doesn't look particularly helpful to figure out what is going on. I > for one would really like to know if that other person is just root (i.e. also > me), or somebody completely different... But what should we do in case the other user is you (via ssh) ? Just ignore that? Show "Lennart (remote)" (do we have this information?). If we have list like Lennart foo root baz It will work fine. But the remote / VT case not so much: Lennart Lennart looks odd. just: Lennart Results into "wtf? you told me *other* users are logged in". etc. I am not really against showing a list just want to know how to handle the "same user via ssh/vt" thing.
A humm, forgot that this is JS, no nice synchronous libsystemd-login API for you then... But filtering out other uids is not that hard even then. Just requires getting the "User" property of the session objects and filtering all out that are != your own UID.
(In reply to comment #12) > A humm, forgot that this is JS, no nice synchronous libsystemd-login API for > you then... > > But filtering out other uids is not that hard even then. Just requires getting > the "User" property of the session objects and filtering all out that are != > your own UID. OK so you are suggesting to ignore duplicate logins from the same user. (And just shutdown without warning when the current active user is logged in via SSH as well?).
(In reply to comment #11) > (In reply to comment #9) > > I think it would be very useful to see who else is logged in... The screenshot > > like it is doesn't look particularly helpful to figure out what is going on. I > > for one would really like to know if that other person is just root (i.e. also > > me), or somebody completely different... > > But what should we do in case the other user is you (via ssh) ? Just ignore > that? Show "Lennart (remote)" (do we have this information?). Well, you have all the options here: a) you could list all sessions and just filter out the one g-s itself is a part of. For that you could use GetSessionByPID() and pass in your PID. Then, filter out the session from the list with the same session bus path. b) you could list all sessions and just filter out all those of other users. For that retrieve the "User" property on the session objects and filter out all those where that's != getuid() c) like b, but still include any remote sessions of your own uid. For that first check "User" as in b), and then check the "Remote" boolean property on the session object, too. For all those props see http://www.freedesktop.org/wiki/Software/systemd/logind
(In reply to comment #12) > A humm, forgot that this is JS, no nice synchronous libsystemd-login API for > you then... The synchronous stuff is because we're a compositor and can't block the mainloop, not because we're in JS.
Oh, and one more thing, you really want to filter out the session of gdm for this, as well as user session created via cron. For that, check the Class property on the Session objects, and ignore all where that is not set to "User".
(In reply to comment #15) > (In reply to comment #12) > > A humm, forgot that this is JS, no nice synchronous libsystemd-login API for > > you then... > > The synchronous stuff is because we're a compositor and can't block the > mainloop, not because we're in JS. Nah, I was just referring to the fact that libsystemd-login is a C API but for g-s it is much easier to do D-Bus calls instead, which provides the same functionality. In C programs the synchronous libsystemd-login C API is usually to use if all you want is some passive state information.
(In reply to comment #16) > Oh, and one more thing, you really want to filter out the session of gdm for > this, as well as user session created via cron. For that, check the Class > property on the Session objects, and ignore all where that is not set to > "User". Huh? I don't see any session for gdm listen in the result of the ListSessions() call. Neither is there any "Class" property: "ListSessions() returns an array with all current sessions. The structures in the array consist of the following fields: session id, user id, user name, seat id, session object path. If a session does not have a seat attached the seat id field will be an empty string."
Oh, and in case this wasn't obvious already, if you want to add an explanatory suffix or so to the list of sessions indicating whether something is remote or not (as in "Lennart (remote)"), you can use the Remote boolean property for that. If you want to know if something is a VT login or a graphical one (as in "Lennart (Console)"), you can check the Type property which is "x11" for graphical logins and "tty" for text logins.
(In reply to comment #18) > (In reply to comment #16) > > Oh, and one more thing, you really want to filter out the session of gdm for > > this, as well as user session created via cron. For that, check the Class > > property on the Session objects, and ignore all where that is not set to > > "User". > > Huh? I don't see any session for gdm listen in the result of the ListSessions() > call. That sessions exists only some times, like when you actually use fast user switching. It normally ceases to exist as soon as you log in, but is created again when you want to add another session. > Neither is there any "Class" property: > "ListSessions() returns an array with all current sessions. The structures in > the array consist of the following fields: session id, user id, user name, seat > id, session object path. If a session does not have a seat attached the seat id > field will be an empty string." ListSessions() returns you a very minimal array of information only. If you want to know anything more fancy you need to get that from the properties of the session object referenced by the session object path you got from ListSessions(). The interface of the session objects is explained further to the end on the wiki page: http://www.freedesktop.org/wiki/Software/systemd/logind
Created attachment 235561 [details] [review] Don't ask for a password on shutdown Show a warning and the list of active users instead when multiple sessions are present. --- Fancy version.
Created attachment 235562 [details] Screenshot of the fancy version
(In reply to comment #22) > Created an attachment (id=235562) [details] > Screenshot of the fancy version <aday> drago01, the screenshot looks nice. anything in particular you want feedback on? <drago01> aday: whether it is ok to land it in this form or not <drago01> aday: I assume "looks nice" means yes <aday> drago01, i haven't explored the issue particularly, but this all seems reasonable <drago01> aday: ok <aday> so, yea So we have a design ack ... now need a code review.
Review of attachment 235561 [details] [review]: Mostly style issues. The commit message would be better if it mentioned the rationale for the change, and the patch would be more readable in splinter/old-fart-terminal with shorter lines (I've only mentioned the worst offenders in the review, there are a couple more places that could use a line break or two) ::: js/ui/userMenu.js @@ +81,3 @@ update: function() { let iconFile = this._user.get_icon_file(); + if (iconFile && !GLib.file_test(iconFile, GLib.FileTest.EXISTS)) Not sure this belongs in this patch - if this was a problem before, it should definitively be a separate commit, otherwise it would still be nice to provide the reasoning why the extra test has become necessary in the commit message. @@ +885,3 @@ + dialog.contentLayout.add(descriptionLabel, { x_fill: true, y_fill: true, y_align: St.Align.START }); + + let scrollView = new St.ScrollView({ style_class: 'end-session-dialog-app-list'}); If the list gets long enough to actually end up scrolled, I think we'll want 'vfade' in style_class as well. @@ +899,3 @@ + + let userLabelText = "";; + let userName = sessions[i].user.get_real_name() ? sessions[i].user.get_real_name() : sessions[i].username; Grmpf, I was going to add a comment about session.userName being unused - please add a line break, e.g. let foo = bar ? baz : quz; @@ +930,3 @@ if (this._haveShutdown && this._suspendOrPowerOffItem.state == PopupMenu.PopupAlternatingMenuItemState.DEFAULT) { + if (!LoginManager.haveSystemd()) LoginManager is supposed to be an abstraction layer, so we *don't* have to spread explicit tests for systemd all over the code. In fact, the fallback for listSessions() should just work fine ... @@ +938,3 @@ + let j = 0; + for (let i = 0; i < result.length; i++) { + let proxy = new SystemdLoginSession(Gio.DBus.system, 'org.freedesktop.login1', result[i][4]); Please deconstruct result[i] first, something like let [id, uid, userName, seat, sessionPath] = result[i]; (or maybe just pick what you're interested in, e.g. let[,,userName,,sessionPath]) @@ +947,3 @@ + + j++; + sessions.push({ user: this._userManager.get_user(result[i][2]), username: result[i][2], session: { "type": proxy.Type, "remote": proxy.Remote } }); Please add some line breaks @@ +949,3 @@ + sessions.push({ user: this._userManager.get_user(result[i][2]), username: result[i][2], session: { "type": proxy.Type, "remote": proxy.Remote } }); + + // limit to 5 entires Typo: s/entires/entries/ - also: magic values should be defined as constants at the top.
(In reply to comment #24) > Review of attachment 235561 [details] [review]: > > Mostly style issues. The commit message would be better if it mentioned the > rationale for the change, and the patch would be more readable in > splinter/old-fart-terminal with shorter lines (I've only mentioned the worst > offenders in the review, there are a couple more places that could use a line > break or two) OK. > ::: js/ui/userMenu.js > @@ +81,3 @@ > update: function() { > let iconFile = this._user.get_icon_file(); > + if (iconFile && !GLib.file_test(iconFile, GLib.FileTest.EXISTS)) > > Not sure this belongs in this patch - if this was a problem before, it should > definitively be a separate commit, otherwise it would still be nice to provide > the reasoning why the extra test has become necessary in the commit message. While testing I got en exception for the case where the user does not have an icon file at all (like root) I agree that this should be in a separate patch though. > @@ +885,3 @@ > + dialog.contentLayout.add(descriptionLabel, { x_fill: true, y_fill: > true, y_align: St.Align.START }); > + > + let scrollView = new St.ScrollView({ style_class: > 'end-session-dialog-app-list'}); > > If the list gets long enough to actually end up scrolled, I think we'll want > 'vfade' in style_class as well. Indeed makes sense. > @@ +899,3 @@ > + > + let userLabelText = "";; > + let userName = sessions[i].user.get_real_name() ? > sessions[i].user.get_real_name() : sessions[i].username; > > Grmpf, I was going to add a comment about session.userName being unused - > please add a line break, e.g. > > let foo = bar ? baz > : quz; OK. > @@ +930,3 @@ > if (this._haveShutdown && > this._suspendOrPowerOffItem.state == > PopupMenu.PopupAlternatingMenuItemState.DEFAULT) { > + if (!LoginManager.haveSystemd()) > > LoginManager is supposed to be an abstraction layer, so we *don't* have to > spread explicit tests for systemd all over the code. > In fact, the fallback for listSessions() should just work fine ... I can try to add a CK code path but cannot really test it. > @@ +938,3 @@ > + let j = 0; > + for (let i = 0; i < result.length; i++) { > + let proxy = new > SystemdLoginSession(Gio.DBus.system, 'org.freedesktop.login1', result[i][4]); > > Please deconstruct result[i] first, something like > let [id, uid, userName, seat, sessionPath] = result[i]; > > (or maybe just pick what you're interested in, e.g. > let[,,userName,,sessionPath]) > > @@ +947,3 @@ > + > + j++; > + sessions.push({ user: > this._userManager.get_user(result[i][2]), username: result[i][2], session: { > "type": proxy.Type, "remote": proxy.Remote } }); > > Please add some line breaks > > @@ +949,3 @@ > + sessions.push({ user: > this._userManager.get_user(result[i][2]), username: result[i][2], session: { > "type": proxy.Type, "remote": proxy.Remote } }); > + > + // limit to 5 entires > > Typo: s/entires/entries/ - also: magic values should be defined as constants at > the top. OK.
(In reply to comment #25) > > LoginManager is supposed to be an abstraction layer, so we *don't* have to > > spread explicit tests for systemd all over the code. > > In fact, the fallback for listSessions() should just work fine ... > > I can try to add a CK code path but cannot really test it. I haven't tested either for obvious reasons, but the code looks like it should already work for the CK path: listSessions() always runs callback([]) => result.length == 0, for loop never runs => j == 0 => shutdown
(In reply to comment #26) > (In reply to comment #25) > > > LoginManager is supposed to be an abstraction layer, so we *don't* have to > > > spread explicit tests for systemd all over the code. > > > In fact, the fallback for listSessions() should just work fine ... > > > > I can try to add a CK code path but cannot really test it. > > I haven't tested either for obvious reasons, but the code looks like it should > already work for the CK path: > > listSessions() always runs callback([]) > => result.length == 0, for loop never runs > => j == 0 => shutdown Ah indeed.
Created attachment 235716 [details] [review] userMenu: Handle the case where the user has no icon file Fix a crash in UserAvatarWidget.update that gets triggered when the user has no icon file.
Created attachment 235717 [details] [review] Don't ask for a password on shutdown Show a warning and the list of active users instead when multiple sessions are present. Should address the review comments.
Review of attachment 235716 [details] [review]: > Fix a crash in UserAvatarWidget.update that gets > triggered when the user has no icon file. Do we know if that already was a problem with the existing code or is it just the new use case? It doesn't really matter of course, just curious ...
Review of attachment 235717 [details] [review]: Please fix the indentation before pushing, the other comments are just suggestions. ::: js/ui/userMenu.js @@ +893,3 @@ + + let scrollView = new St.ScrollView({ style_class: 'end-session-dialog-app-list' }); + scrollView.add_style_class_name('vfade'); ... or use "new St.ScrollView({ style_class: 'end-session-dialog-app-list vfade' });" Your call. @@ +948,3 @@ if (this._haveShutdown && this._suspendOrPowerOffItem.state == PopupMenu.PopupAlternatingMenuItemState.DEFAULT) { + this._loginManager.listSessions(Lang.bind(this, Needs re-indenting after removing the if()else block. @@ +965,3 @@ + continue; + + n++; Makes more sense to me right before the "if (n == MAX_USERS)" test
(In reply to comment #30) > Review of attachment 235716 [details] [review]: > > > Fix a crash in UserAvatarWidget.update that gets > > triggered when the user has no icon file. > > Do we know if that already was a problem with the existing code or is it just > the new use case? It doesn't really matter of course, just curious ... This is kind of a generic thing it should have been a problem with the old code too no idea why we didn't notice that.
(In reply to comment #31) > Review of attachment 235717 [details] [review]: > > Please fix the indentation before pushing, the other comments are just > suggestions. > > ::: js/ui/userMenu.js > @@ +893,3 @@ > + > + let scrollView = new St.ScrollView({ style_class: > 'end-session-dialog-app-list' }); > + scrollView.add_style_class_name('vfade'); > > ... or use "new St.ScrollView({ style_class: 'end-session-dialog-app-list > vfade' });" > > Your call. That was intentional to avoid the need for a line break ;) > @@ +948,3 @@ > if (this._haveShutdown && > this._suspendOrPowerOffItem.state == > PopupMenu.PopupAlternatingMenuItemState.DEFAULT) { > + this._loginManager.listSessions(Lang.bind(this, > > Needs re-indenting after removing the if()else block. Oh indeed. > @@ +965,3 @@ > + continue; > + > + n++; > > Makes more sense to me right before the "if (n == MAX_USERS)" test OK moved.
Created attachment 235730 [details] [review] Don't ask for a password on shutdown Show a warning and the list of active users instead when multiple sessions are present.
Attachment 235716 [details] pushed as 4ab1e89 - userMenu: Handle the case where the user has no icon file Attachment 235730 [details] pushed as b2fb84e - Don't ask for a password on shutdown Leaving the bug open got the logind patch (even though it does not really belong here). Lennart?
(In reply to comment #35) > Leaving the bug open got the logind patch (even though it does not really > belong here). > > Lennart? Isn't that patch against systemd and not gnome-shell?
(In reply to comment #36) > Isn't that patch against systemd and not gnome-shell? Yes, but we can't reassign to fd.o ...
Applied the systemd side of things to systemd git. I guess the bug can be closed now. Thanks for all the work, Adel!
(In reply to comment #38) > Applied the systemd side of things to systemd git. I guess the bug can be > closed now. > > Thanks for all the work, Adel! OK, thanks.