<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/121429/">https://git.reviewboard.kde.org/r/121429/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 15th, 2014, 11:45 p.m. CET, <b>Àlex Fiestas</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;">Code looks good. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could you perhaps add an integration test for this? Since we are "abstracted" by the socket it should be possible. If it is too much work feel free to push it.</p></pre>
</blockquote>
<p>On December 16th, 2014, 8 a.m. CET, <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;">what do you want the integration test to test? I certainly can start the daemon but I'm not sure what it would give us as the only way to return from it requires a password. And that's what the test application in tests already does.</p></pre>
</blockquote>
<p>On December 17th, 2014, 12:09 a.m. CET, <b>Àlex Fiestas</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;">Well, this patch adds a lot of new logic that can be tested, since it does not have unit test (and doing them in ksmserver migh prove difficult) we can test the code with an integraiton test.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I see lots of socket logic
I see logic in addAllowedWindow
And also the biggest method setKsldSocket which has 2 lambdas that (afaik) can't be tested in any other way.</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;">The point is I don't see how to do an integration test for it. If we pull up everything the screen is locked, like locked. It needs a damn password to be entered to get unlocked. I just don't see how this could be integration tested.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you see how to integration test it please provide the code for it.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On December 15th, 2014, 10:29 a.m. CET, 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, Àlex Fiestas and David Edmundson.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Dec. 15, 2014, 10:29 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;">The screenlocker_greet needs to tell the parent ksld process which
windows it created. Ksld sends input events to these windows. So
far this was based on an X property on the window. Unfortunately
ksld didn't validate whether the windows tagged with this property
belong to the screenlocker_greet process it started.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With this change the communication for announcing windows is moved
away from the X11 protocol and instead a custom Wayland protocol is
used.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ksld starts a KWaylandServer when the greet process gets started. It
creates anonymous unix sockets for the connection and passes one
filedescriptor to the started greeter process.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The check for the X property is removed in ksld and instead only
windows ids passed through the Wayland socket connection are
accepted.</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;">Running ksmserver with the patch. Lock/unlock working, my exploit is failing.</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>ksmserver/screenlocker/ksldapp.cpp <span style="color: grey">(22698ce37e9d4be17126111b3ded8133f7c3baa6)</span></li>
<li>ksmserver/screenlocker/lockwindow.h <span style="color: grey">(9938d201269c89a24c9c0bd6275aa5f731bb5535)</span></li>
<li>ksmserver/screenlocker/lockwindow.cpp <span style="color: grey">(3aa963a59e21636862f5ca59e220bbea3bd41ff9)</span></li>
<li>ksmserver/screenlocker/protocols/ksld.xml <span style="color: grey">(PRE-CREATION)</span></li>
<li>ksmserver/screenlocker/waylandserver.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>ksmserver/screenlocker/waylandserver.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>ksmserver/screenlocker/greeter/greeterapp.h <span style="color: grey">(b92b13b63365a9026dba5d71b772dcd8c9ee3d3b)</span></li>
<li>ksmserver/screenlocker/greeter/greeterapp.cpp <span style="color: grey">(30d1821bdba38028959f3457e900a1b32e628192)</span></li>
<li>ksmserver/screenlocker/greeter/main.cpp <span style="color: grey">(12e570107d0cba851b8978131d730b27924529bb)</span></li>
<li>ksmserver/screenlocker/ksldapp.h <span style="color: grey">(095424c9845c134aa156917aeb6c8ddf31e8d25a)</span></li>
<li>CMakeLists.txt <span style="color: grey">(c6d89c14b05f5639937aee5692d305fa2faed974)</span></li>
<li>ksmserver/screenlocker/CMakeLists.txt <span style="color: grey">(5378a10df2be70cee95b5612c23046eae639f610)</span></li>
<li>ksmserver/screenlocker/greeter/CMakeLists.txt <span style="color: grey">(10c473488f08354096f68784b9240392a444af5f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121429/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>