[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 111093: PolKit Actions KCM: do not crash if policies could not be read
From: "Jaime Torres Amate" <jtamate () gmail ! com>
Date: 2013-09-22 8:00:12
Message-ID: 20130922080012.5689.19378 () vidsolbach ! de
[Download RAW message or body]
> On Sept. 21, 2013, 6:40 p.m., Jaime Torres Amate wrote:
> > I've tried to apply it to today master head and it does not apply clean. Some \
> > previous patch has done something similar to your patch. (sorry, not much time to \
> > check it all).
>
> Jonathan Marten wrote:
> Is the definitive source base and master branch the one at \
> git://anongit.kde.org/polkit-kde-kcmodules-1? Just tried updating to the latest \
> from there and the diff produced is identical.
>
yes, the last commit is 6fe05fe98a3f33d72b66c4a55200e6c88e050168.
Ops, I'm sorry. I still had your previous patch for the bug applied (3 months without \
time for KDE things).
I've tried again, now applied right. And it works very nicely without crashes.
You have my ship it!.
- Jaime Torres
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111093/#review40425
-----------------------------------------------------------
On Sept. 18, 2013, 8:37 a.m., Jonathan Marten wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111093/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2013, 8:37 a.m.)
>
>
> Review request for kde-workspace and Polkit KDE Configuration.
>
>
> Description
> -------
>
> The referenced bug describes a crash in the System Settings - System Administration \
> - Actions Policy module. The first time that an action is clicked on, an attempt \
> is made to read the current system policy settings via ActionWidget::reloadPKLAs(). \
> Internally this checks the authorisation for the \
> org.kde.polkitkde1.readauthorizations action. If this action is not allowed, or \
> the authorization fails, then a DBus error is returned - but never checked. There \
> is then a crash (via qFatal) when an attempt is made to read from the invalid \
> returned QDBusArgument.
> Unless the user made a mistake (e.g. typing the root password incorrectly), this \
> indicates a system configuration problem. However, even if the fix needs to be \
> elsewhere, systemsettings should not just crash with no indication of where the \
> problem lies.
> This change checks and reports the DBus error if one is returned. Nothing can be \
> done within this module if this is the case, but at least there is a diagnostic \
> message. The widget is left disabled, but will try the authorization again if \
> another action is selected.
>
> This addresses bug 300050.
> http://bugs.kde.org/show_bug.cgi?id=300050
>
>
> Diffs
> -----
>
> polkitactions/ActionWidget.h ca83bf5
> polkitactions/ActionWidget.cpp c5785c0
>
> Diff: http://git.reviewboard.kde.org/r/111093/diff/
>
>
> Testing
> -------
>
> With the default policy in place for org.kde.polkit1.readauthorizations (active \
> session = auth_admin, inactive session = no), run 'kcmshell4 kcm_polkitactions'. \
> Expand the tree and click on any action. In the password dialogue, either cancel \
> or enter an incorrect password. Check that there is no crash and that the message \
> box is displayed.
> Click on another action, correctly enter the password and observe that there is no \
> message and policies are displayed as expected.
>
> Thanks,
>
> Jonathan Marten
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/111093/">http://git.reviewboard.kde.org/r/111093/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On September 21st, 2013, 6:40 p.m. UTC, <b>Jaime \
Torres Amate</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;">I've tried to apply it to today master head and it does not apply \
clean. Some previous patch has done something similar to your patch. (sorry, not much \
time to check it all).</pre> </blockquote>
<p>On September 22nd, 2013, 7:35 a.m. UTC, <b>Jonathan Marten</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;">Is the definitive source \
base and master branch the one at git://anongit.kde.org/polkit-kde-kcmodules-1? Just \
tried updating to the latest from there and the diff produced is identical.
</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;">yes, the last commit is \
6fe05fe98a3f33d72b66c4a55200e6c88e050168. Ops, I'm sorry. I still had your \
previous patch for the bug applied (3 months without time for KDE things).
I've tried again, now applied right. And it works very nicely without crashes.
You have my ship it!.</pre>
<br />
<p>- Jaime Torres</p>
<br />
<p>On September 18th, 2013, 8:37 a.m. UTC, Jonathan Marten wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kde-workspace and Polkit KDE Configuration.</div>
<div>By Jonathan Marten.</div>
<p style="color: grey;"><i>Updated Sept. 18, 2013, 8:37 a.m.</i></p>
<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;">The referenced bug describes a crash in the System Settings - System \
Administration - Actions Policy module. The first time that an action is clicked on, \
an attempt is made to read the current system policy settings via \
ActionWidget::reloadPKLAs(). Internally this checks the authorisation for the \
org.kde.polkitkde1.readauthorizations action. If this action is not allowed, or the \
authorization fails, then a DBus error is returned - but never checked. There is \
then a crash (via qFatal) when an attempt is made to read from the invalid returned \
QDBusArgument.
Unless the user made a mistake (e.g. typing the root password incorrectly), this \
indicates a system configuration problem. However, even if the fix needs to be \
elsewhere, systemsettings should not just crash with no indication of where the \
problem lies.
This change checks and reports the DBus error if one is returned. Nothing can be \
done within this module if this is the case, but at least there is a diagnostic \
message. The widget is left disabled, but will try the authorization again if \
another action is selected.</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;">With the default policy in place for org.kde.polkit1.readauthorizations \
(active session = auth_admin, inactive session = no), run 'kcmshell4 \
kcm_polkitactions'. Expand the tree and click on any action. In the password \
dialogue, either cancel or enter an incorrect password. Check that there is no crash \
and that the message box is displayed.
Click on another action, correctly enter the password and observe that there is no \
message and policies are displayed as expected.</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=300050">300050</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>polkitactions/ActionWidget.h <span style="color: grey">(ca83bf5)</span></li>
<li>polkitactions/ActionWidget.cpp <span style="color: grey">(c5785c0)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111093/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic