<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/129983/">https://git.reviewboard.kde.org/r/129983/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 12th, 2017, 3:49 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/129983/diff/3/?file=492374#file492374line37" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">add_subdirectory(kauth)</pre></td>
</tr>
</tbody>
</table>
<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;">if (UNIX)
add_subdirectory(kauth)
endif()</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Apparently kauth doesn't exist on Windows.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">Shall I move all the kauth specific code to file_unix ?</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 12th, 2017, 3:49 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/129983/diff/3/?file=492376#file492376line1391" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/file.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1391</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">isRoot</span> <span class="o">=</span> <span class="nb">true</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<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;">can you explain the meaning of the isRoot member to me? It sounds like "I am authorized to be root", but here it's set while in "auth required" state, sounds like we're not root yet?
(I know nothing about polkit/kauth but at least this code should make sense to me ;)</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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 name isRoot is misleading so I changed it to mPriviledgeOpStarted. It is used to show that the priviledged operation has started and will be set to false once the job ends. This along with kauthStatus are used to decide whether to show warning dialog or not.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The "execute" method in KAuth is similar to sudo command. The priviledge persist for about 5 minutes. It is during this time we need to show the warning. Now when deleting multiple files we can either delete them using one job(deleteRecursive) or multiple jobs. So to determine whether the call to execWithRoot was from the same job or not and show the warning accordingly I used the aforementioned members.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So my logic here is,
- Prepare the arguments
- If kauth status is Auth Required status(i.e. execWithRoot is called for the first time) then set mPriviledgeOpStarted to true.
- If kauth status is Authorized status (i.e. priviledges persists) and mPriviledgeOpStarted is false(this is a different job) then set it to true.
- If the user entered the correct password or chose continue proceed with the action else halt.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 12th, 2017, 3:49 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/129983/diff/3/?file=492381#file492381line14" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/kauth/helper.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">14</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="n">success</span> <span class="o">=</span> <span class="n">QMetaObject</span><span class="o">::</span><span class="n">invokeMethod</span><span class="p">(</span><span class="k">this</span><span class="p">,</span> <span class="n">op</span><span class="p">.</span><span class="n">data</span><span class="p">(),</span> <span class="n">Qt</span><span class="o">::</span><span class="n">QueuedConnection</span><span class="p">,</span> <span class="n">Q_ARG</span><span class="p">(</span><span class="n">QVariantMap</span><span class="p">,</span> <span class="n">args</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<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 is this using a QueuedConnection? This prevents any sort of error handling because this method returns too early.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Testing that invokeMethod worked is only half the story. If the QueuedConnection can be removed, you can use a return argument of type bool for error handling?
Or better, a KIO error code, so we can give the exact error message (I see that we didn't have "directory not empty" for rmdir, but if this is extended to other actions then it will be useful to have).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In fact, instead of invokeMethod this could be a set of if(method == "...")? Seems simpler to me, but I have no strong opinion.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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 used invokeMethod for compactness but an if-else construct is indeed simpler</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 12th, 2017, 3:49 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/129983/diff/3/?file=492381#file492381line26" style="color: black; font-weight: bold; text-decoration: underline;">src/ioslaves/file/kauth/helper.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">26</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<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;">This statement does nothing, the method would return anyway immediately afterwards. Is there missing error handling here? Shouldn't this return a bool?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">Error handling was supposed to be there but somehow I messed up.</p></pre>
<br />
<p>- Chinmoy Ranjan</p>
<br />
<p>On March 14th, 2017, 2:48 p.m. UTC, Chinmoy Ranjan Pradhan 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 KDE Frameworks, David Faure and Elvis Angelaccio.</div>
<div>By Chinmoy Ranjan Pradhan.</div>
<p style="color: grey;"><i>Updated March 14, 2017, 2:48 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;">This is regarding the GSOC idea https://community.kde.org/GSoC/2017/Ideas#Project:_Polkit_support_in_KIO.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch intends to demonstrate one possible approach to provide polkit support in kio. Here its only for the delete operation. This is based on the patch in task https://phabricator.kde.org/T5070.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The approach is as follows;
1. Whenever file ioslave gets access denied error it calls the method <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">execWithRoot</em> with the action that requires priviledge, the path of items
upon which action needs to be performed and a warning ID as arguments.
2. <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">execWithRoot</em> then executes the KAuth::Action <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">org.kde.kio.file.execute</em>.
3. This Kauth::Action has its Persistence set too 'session'. This means that after authentication the restrictions are dropped for a while, for <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
about 5 minutes. This is similar to the behaviour of sudo command.
4. During this time we can perform any action as a privileged user without any authentication. So to prevent any mishap i added a warning box which
would popup before performing any action(only during this period).
5. After the said time interval the root privileges are droped and calling <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">execWithRoot</em> should show the usual authentication dialog.</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>src/ioslaves/file/CMakeLists.txt <span style="color: grey">(b9132ce)</span></li>
<li>src/ioslaves/file/file.h <span style="color: grey">(109ea80)</span></li>
<li>src/ioslaves/file/file.cpp <span style="color: grey">(eaf6c88)</span></li>
<li>src/ioslaves/file/file_unix.cpp <span style="color: grey">(82eb11a)</span></li>
<li>src/ioslaves/file/kauth/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/ioslaves/file/kauth/file.actions <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/ioslaves/file/kauth/helper.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/ioslaves/file/kauth/helper.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129983/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2017/03/09/d42570e8-aedf-4c02-801e-362a68755c2c__polkit_integration.png">warning dialog</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>