[libvirt] [PATCH v2 1/3] polkit: Adjust message when action-id isn't found

[John Ferlan]

On 02/12/2016 04:47 AM, Daniel P. Berrange wrote:
> On Thu, Feb 11, 2016 at 06:38:12PM -0500, John Ferlan wrote:
>> When there isn't a ssh -X type session running and a user has not
>> been added to the libvirt group, attempts to run 'virsh -c qemu:///system'
>> commands from an otherwise unprivileged user will fail with rather
>> generic or opaque error message:
>>
>>     "error: authentication failed: no agent is available to authenticate"
>>
>> This patch will adjust the error message to help reflect not only
>> that it was a polkit agent, but which 'action-id' was not found. It
>> does so carefully, by adding to the text rather than adjusting it
>> since virpolkittest is looking for a specific string. The result on
>> a failure then becomes:
>>
>>     "error: authentication failed: no agent is available to authenticate -
>>      no polkit agent for action 'org.libvirt.unix.manage'"
>>
>> A bit more history on this - at one time a failure generated the
>> following type message when running the 'pkcheck' as a subprocess:
>>
>> "error: authentication failed: polkit\56retains_authorization_after_challenge=1
>> Authorization requires authentication but no agent is available."
>>
>> but, a patch was generated to adjust the error message to help provide
>> more details about what failed. This was pushed as commit id '96a108c99'.
>> That patch prepended a "polkit: " to the output. It really didn't solve
>> the problem, but gave a hint.
>>
>> After some time it was deemed using DBus API calls directly was a
>> better way to go (since pkcheck calls them anyway). So, commit id
>> '1b854c76' (more or less) copied the code from remoteDispatchAuthPolkit
>> and adjusted it. Then commit id 'c7542573' adjusted the remote.c
>> code to call the new API (virPolkitCheckAuth). Finally, commit id
>> '308c0c5a' altered the code to call DBus APIs directly. In doing
>> so, it reverted the failing error message to the generic message
>> that would have been received from DBus anyway.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virpolkit.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>> index 8da91f2..d837a14 100644
>> --- a/src/util/virpolkit.c
>> +++ b/src/util/virpolkit.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * virpolkit.c: helpers for using polkit APIs
>>   *
>> - * Copyright (C) 2013, 2014 Red Hat, Inc.
>> + * Copyright (C) 2013, 2014, 2016 Red Hat, Inc.
>>   *
>>   * This library is free software; you can redistribute it and/or
>>   * modify it under the terms of the GNU Lesser General Public
>> @@ -121,8 +121,10 @@ int virPolkitCheckAuth(const char *actionid,
>>              virReportError(VIR_ERR_AUTH_CANCELLED, "%s",
>>                             _("user cancelled authentication process"));
>>          else if (is_challenge)
>> -            virReportError(VIR_ERR_AUTH_FAILED, "%s",
>> -                           _("no agent is available to authenticate"));
>> +            virReportError(VIR_ERR_AUTH_FAILED,
>> +                           _("no agent is available to authenticate - "
>> +                             "no polkit agent for action '%s'"),
> 
> This is a littled repetitive still. ACK if you make it say
> 
>    "No polkit agent available to authenticate action '%s'"
> 

OK - that will require a change to virpolkittest.c too (adding the
domain and code checks as well):

index 1ef7635..56f98b2 100644
--- a/tests/virpolkittest.c
+++ b/tests/virpolkittest.c
@@ -220,8 +220,9 @@ static int testPolkitAuthChallenge(const void
*opaque ATTRIBUTE_UNUSED)
     }

     err = virGetLastError();
-    if (!err || !strstr(err->message,
-                       _("no agent is available to authenticate"))) {
+    if (!err || err->domain != VIR_FROM_POLKIT ||
+        err->code != VIR_ERR_AUTH_FAILED || !strstr(err->message,
+                       _("no polkit agent available to authenticate"))) {
         fprintf(stderr, "Incorrect error response\n");
         goto cleanup;
     }


Although my thoughts in reply to the patch3 review may need to tweak
this more...

John