<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D6830" rel="noreferrer">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6830#inline-29318" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:244</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #304a96">#endif</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">dest_file</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="n">remove</span><span class="p">(<span class="bright">);</span></span> <span style="color: #74777d">// don't keep partly copied file</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">!!</span></span><span class="bright"></span><span class="n"><span class="bright">QFile</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="n">remove</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span class="n"><span class="bright">dest</span></span><span class="bright"></span><span class="p"><span class="bright">))</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span> <span style="color: #74777d">// don't keep partly copied file</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">execWithElevatedPrivilege</span><span class="p">(</span><span class="n">DEL</span><span class="p">,</span> <span class="n">_dest</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Double negation, looks like one too many.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6830#inline-29319" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:290</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #304a96">#endif</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">dest_file</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="n">remove</span><span class="p">(<span class="bright">);</span></span> <span style="color: #74777d">// don't keep partly copied file</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">!!</span></span><span class="bright"></span><span class="n"><span class="bright">QFile</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="n">remove</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span class="n"><span class="bright">dest</span></span><span class="bright"></span><span class="p"><span class="bright">))</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span> <span style="color: #74777d">// don't keep partly copied file</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">execWithElevatedPrivilege</span><span class="p">(</span><span class="n">DEL</span><span class="p">,</span> <span class="n">_dest</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">same</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6830#inline-29320" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:327</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span> <span style="color: #aa4000">else</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">execWithElevatedPrivilege</span><span class="p">(</span><span class="n">CHOWN</span><span class="p">,</span> <span class="n">_dest</span><span class="p">,</span> <span class="n">buff_src</span><span class="p">.</span><span class="n">st_uid</span><span class="p">,</span> <span class="n">buff_src</span><span class="p">.</span><span class="n">st_gid</span><span class="p">))</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">qCWarning</span><span class="p">(</span><span class="n">KIO_FILE</span><span class="p">)</span> <span style="color: #aa2211"><<</span> <span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"Couldn't preserve group for '%1'"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">dest</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Can you test copying a file onto a FAT partition mounted as another uid? The ownership and permissions cannot be applied due to FAT limitations. Does that then trigger a kauth prompt, with this patch? That would be annoying, and wrong since root can't do better anyway.</p>
<p style="padding: 0; margin: 8px;">If I'm right (please test it) then I think this method should remember "I needed root permissions for the main operation" and only in that case use elevated privileges for the small operations at the end like chmod, chown or utime.</p>
<p style="padding: 0; margin: 8px;">Please review whether other methods need the same kind of change.</p></div></div></div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D6830" rel="noreferrer">https://phabricator.kde.org/D6830</a></div></div><br /><div><strong>To: </strong>chinmoyr, dfaure, Frameworks<br /><strong>Cc: </strong>elvisangelaccio, Frameworks<br /></div>