<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120577/">https://git.reviewboard.kde.org/r/120577/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 14th, 2014, 9:02 a.m. CEST, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could we possibly get "Suspend" button instead? Suspend stores the session state into memory and then restores it without any issues (and we lock screen on wakeup anyway); many times I have found my laptop with locked screen and I had to unlock, click the suspend button in the panel only to get it locked again and suspended.</p></pre>
</blockquote>
<p>On October 14th, 2014, 9:12 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I expected this question to come :-) - I'm not sure whether it's a valid use case, at least for laptops. Closing the lid should send it to suspend, the suspend button should still work (if not, we should fix it). So there are already quite some decent ways to get the system to suspend when locked. Especially with lid closing it does what it's supposed to do.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In fact the functionality should even be available through the Change Session functionality as that drops you to the login manager where you can suspend. Sure not as comfortable as directly exposing it. But we don't need to expose every option which might make sense ;-)</p></pre>
</blockquote>
<p>On October 14th, 2014, 9:26 a.m. CEST, <b>Kai Uwe Broulik</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Although closing the lid apparently reliably suspends the device, even when locked, my gut makes me not trust it. :/</p></pre>
</blockquote>
<p>On October 14th, 2014, 9:32 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Although closing the lid apparently reliably suspends the device, even when locked, my gut makes me not trust it. :/</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">At least my notebook(s) have a visible LED indicating it is in sleep mode.</p></pre>
</blockquote>
<p>On October 14th, 2014, 9:47 a.m. CEST, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are still usecases however where either the user does not want/need to close the lid and/or sets his laptop to not suspend on the lid close (personally I have that disabled when on charger). Plus there is the whole non-laptop world. It's not about exposing every option that makes sense, it's about replacing one senseless with one that makes more sense ;)</p></pre>
</blockquote>
<p>On October 14th, 2014, 9:58 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's not about exposing every option that makes sense, it's about replacing one senseless with one that makes more sense ;)</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't see it that way. The option got added in a strange way shortly before the 5.0 release with the maintainers of the screenlocker not having a chance to review it (let's say it simple: if I would have reviewed the code, it would not have gone in, in the first place). So I'm comparing to the last state where it was working which is 4.11 and there the screen locker did not expose such an option and I want to go back to that working state. Let's look from there whether it's a valid option to expose the suspend option and not from the broken state.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To me it's not a valid use case to expose suspend in the lock screen. To a certain degree it circumvents security. Suspend/Hibernate is allowed on a per-user logged in base. That's implemented by checking whether suspend/hibernate is supported at runtime. By exposing it in the screenlocker it circumvents the check. A not logged in user (or a user who is not allowed to supsend in his session) is able to suspend (multi user system just needs to switch to a logged session which allows suspend). Thus I think from a security perspective the option may not be there in the first place.</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:16 a.m. CEST, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Oh come on, that's hardly any security circumvention. By that logic we should not show battery state, current user's date/time, user name and user picture in the lock screen either because it's exposing things which other users might have restricted access to.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Plus you said in the very first comment that it's possible to go around this to the login screen and from there anyone can suspend...</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:24 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Plus you said in the very first comment that it's possible to go around this to the login screen and from there anyone can suspend...</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">yes and no. In a multi-user system you can of course configure the system in a way to not allow suspend/hibernate from login manager.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It all boils down to: why do we go all the hassle to check whether this options should be enabled from a security point of view, when we allow it for not logged-in users. If we go for allow for not logged in users, we can remove quite a decent amount of code...</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:29 a.m. CEST, <b>Kai Uwe Broulik</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Having a suspend button is a security issue but closing the lid, which causes it to suspend, is not?</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:43 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Having a suspend button is a security issue but closing the lid, which causes it to suspend, is not?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">no, as that is controlled by logind. If the system is configured to not allow it, logind won't suspend the system if the lid closes.</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:45 a.m. CEST, <b>Kai Uwe Broulik</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, the button should also honor that policy?</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:49 a.m. CEST, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, the button should also honor that policy?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The thing is (I think) that having the button exposed for user A would allow user B to suspend the session while user B has it restricted. But I think that at that very point closing the lid would simply work because it's user A that's logged in and it's his active sesison. No?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(I still don't see any security issue in user B suspending the machine from user's A session though, there will be no data leak nor will user B gain anything by doing that)</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:50 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, the button should also honor that policy?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">yes, but our API doesn't allow that. Our API checks for "is current logged in user allowed to suspend", while we need "is not logged in user allowed to suspend". So supporting that would need quite a change in our implementation, it's not as simple as just swapping the buttons.</p></pre>
</blockquote>
<p>On October 14th, 2014, 10:56 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">to be precise the documentation from logind:</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">CanPowerOff(), CanReboot(), CanSuspend(), CanHibernate(), CanHybridSleep() tests whether the system supports the respective operation and whether the calling user is eligible for the desired operation. Returns one of "na", "yes", "no" or "challenge". If "na" is returned the operation is not available because hardware, kernel or drivers do not support it. If "yes" is returned the operation is supported and the user may execute the operation without further authentication. If "no" is returned the operation is available but the user is not allowed to execute the operation. If "challenge" is returned the operation is available, but only after authorization.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This means to me that we cannot test for not logged in user.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now concerning Martin's lid argument: that's then a security vulnerability. Let's please not get to the point where we allow adding further because there is a not fixed one.</p></pre>
</blockquote>
<p>On October 14th, 2014, 11:05 a.m. CEST, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why exactly is that a security vulnerability though? User B would not breach into user A's session. It's an annoyance at most.</p></pre>
</blockquote>
<p>On October 14th, 2014, 11:12 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why exactly is that a security vulnerability though? User B would not breach into user A's session. It's an annoyance at most.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">really? Do I have to explain that?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok scenario: lab with multiple systems. Each employee in the lab is allowed to login in on each system but has one main system for her work. Only the employee who owns the system is allowed to suspend it, for the other employees it's just meant to be able to quickly check something in the Internet etc. Now employee A uses her system to do a week long calculation for research while being on vacations, employee B wants to sabotize the work by interrupting the calculation (let's assume the lab is setup in a way that you cannot just unplug the system).</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, that's a very valid scenairo, I would just not consider it a "security vulnerability" as the system security as such is not compromised in this case.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On October 14th, 2014, 7:41 a.m. CEST, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma and Aleix Pol Gonzalez.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Oct. 14, 2014, 7:41 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Logging out from the locked screen is impossible. Logging out means
interaction with the session due to the session manager. The session
manager asks all applications to quit and applications are allowed to
ask for example saving changes. The session manager stopps the
logout in this case and asks the window manager to focus this window
and the session manager will only continue the logout once the
application resolved the situation. At any time in this process the
user is still able to abort the logout!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Switching to the application which needs interaction is impossible,
though as the screen is still locked. The result is a seemingly
frozen system as the logout cannot continue and there is no indication
what is going on.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Of course the lock screen cannot unlock the session for the logout as
that would circumvent the security. If the lock screen would unlock
one would just have to click the button and abort the logout really
fast to have the system unlocked. So this is clearly not an option.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The result is: we cannot implement this functionality in a working
and secure manner, so it's better to remove it.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">run kscreenlocker_greeter --testing and locked the screen - button is gone.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>lookandfeel/contents/lockscreen/LockScreen.qml <span style="color: grey">(7d730cf1ebd8241dfe1c00a2bef86ec4a3f0212d)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120577/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>