[libvirt] [PATCH v2] Add new error code VIR_ERROR_AUTH_CANCELLED
And hook it up for policykit auth. This allows virt-manager to detect that the user clicked the policykit 'cancel' button and not throw an 'authentication failed' error message at the user. v2: Don't leak pkout Signed-off-by: Cole Robinson <crobinso@redhat.com> --- daemon/remote.c | 14 ++++++++++++-- include/libvirt/virterror.h | 1 + src/util/virterror.c | 6 ++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 1ada146..cedc26a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2471,6 +2471,8 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, const char *action; int status = -1; char *ident = NULL; + int authdismissed = 0; + char *pkout = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); virCommandPtr cmd = NULL; @@ -2481,6 +2483,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, "org.libvirt.unix.manage"; cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", action, NULL); + virCommandSetOutputBuffer(cmd, &pkout); VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client)); if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { @@ -2509,6 +2512,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, if (virCommandRun(cmd, &status) < 0) goto authfail; + authdismissed = (pkout && strstr(pkout, "dismissed=true")); if (status != 0) { char *tmp = virCommandTranslateStatus(status); VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d: %s"), @@ -2533,9 +2537,15 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, error: virCommandFree(cmd); VIR_FREE(ident); + VIR_FREE(pkout); virResetLastError(); - virNetError(VIR_ERR_AUTH_FAILED, "%s", - _("authentication failed")); + if (authdismissed) { + virNetError(VIR_ERR_AUTH_CANCELLED, "%s", + _("authentication cancelled by user")); + } else { + virNetError(VIR_ERR_AUTH_FAILED, "%s", + _("authentication failed")); + } virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); return -1; diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index e896d67..9844cbe 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -243,6 +243,7 @@ typedef enum { risky domain snapshot revert */ VIR_ERR_OPERATION_ABORTED = 78, /* operation on a domain was canceled/aborted by user */ + VIR_ERR_AUTH_CANCELLED = 79, /* authentication cancelled */ } virErrorNumber; /** diff --git a/src/util/virterror.c b/src/util/virterror.c index 85eec8d..31ddd9d 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1022,6 +1022,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("authentication failed: %s"); break; + case VIR_ERR_AUTH_CANCELLED: + if (info == NULL) + errmsg = _("authentication cancelled"); + else + errmsg = _("authentication cancelled: %s"); + break; case VIR_ERR_NO_STORAGE_POOL: if (info == NULL) errmsg = _("Storage pool not found"); -- 1.7.7.5
Several not uncommon issues can be diagnosed through pkcheck output, like lack of/malfunctioning desktop agent, or lack of/malfunctioning polkit dbus agent. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- daemon/remote.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index cedc26a..093ed87 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2472,7 +2472,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, int status = -1; char *ident = NULL; int authdismissed = 0; - char *pkout = NULL; + char *pkout = NULL, *pkerr = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); virCommandPtr cmd = NULL; @@ -2484,6 +2484,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", action, NULL); virCommandSetOutputBuffer(cmd, &pkout); + virCommandSetErrorBuffer(cmd, &pkerr); VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client)); if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { @@ -2537,15 +2538,20 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, error: virCommandFree(cmd); VIR_FREE(ident); - VIR_FREE(pkout); virResetLastError(); + if (authdismissed) { virNetError(VIR_ERR_AUTH_CANCELLED, "%s", _("authentication cancelled by user")); + } else if (pkout || pkerr) { + virNetError(VIR_ERR_AUTH_FAILED, "%s %s", pkerr, pkout); } else { virNetError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); } + + VIR_FREE(pkout); + VIR_FREE(pkerr); virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); return -1; -- 1.7.7.5
On 01/27/2012 01:44 PM, Cole Robinson wrote:
@@ -2537,15 +2538,20 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, error: virCommandFree(cmd); VIR_FREE(ident); - VIR_FREE(pkout); virResetLastError(); + if (authdismissed) { virNetError(VIR_ERR_AUTH_CANCELLED, "%s", _("authentication cancelled by user")); + } else if (pkout || pkerr) { + virNetError(VIR_ERR_AUTH_FAILED, "%s %s", pkerr, pkout);
This could dereference NULL (well, it won't on glibc, but "(null) error" or "out (null)" don't look any nicer). Change it to: virNetError(VIR_ERR_AUTH_FAILED, "%s", pkerr ? pkerr : pkout); and I'll call it good enough for ACK (that is, even if the app wrote to both stdout and stderr, I think that printing just stderr output in that case is probably reasonable). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/27/2012 04:06 PM, Eric Blake wrote:
On 01/27/2012 01:44 PM, Cole Robinson wrote:
@@ -2537,15 +2538,20 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, error: virCommandFree(cmd); VIR_FREE(ident); - VIR_FREE(pkout); virResetLastError(); + if (authdismissed) { virNetError(VIR_ERR_AUTH_CANCELLED, "%s", _("authentication cancelled by user")); + } else if (pkout || pkerr) { + virNetError(VIR_ERR_AUTH_FAILED, "%s %s", pkerr, pkout);
This could dereference NULL (well, it won't on glibc, but "(null) error" or "out (null)" don't look any nicer).
Change it to:
virNetError(VIR_ERR_AUTH_FAILED, "%s", pkerr ? pkerr : pkout);
and I'll call it good enough for ACK (that is, even if the app wrote to both stdout and stderr, I think that printing just stderr output in that case is probably reasonable).
Hmm, I actually want to always include both outputs. I'm afraid that at some future point pkcheck will add some stderr spew and we won't show the more important stdout output. But there are already cases where stderr is more important, so I think showing both is best. I sent a second patch that should avoid the (null) issue. Thanks, COle
On 01/27/2012 01:44 PM, Cole Robinson wrote:
And hook it up for policykit auth. This allows virt-manager to detect that the user clicked the policykit 'cancel' button and not throw an 'authentication failed' error message at the user.
v2: Don't leak pkout
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- daemon/remote.c | 14 ++++++++++++-- include/libvirt/virterror.h | 1 + src/util/virterror.c | 6 ++++++ 3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 1ada146..cedc26a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2471,6 +2471,8 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, const char *action; int status = -1; char *ident = NULL; + int authdismissed = 0;
bool authdismissed = false; ACK with that nit fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/27/2012 04:03 PM, Eric Blake wrote:
On 01/27/2012 01:44 PM, Cole Robinson wrote:
And hook it up for policykit auth. This allows virt-manager to detect that the user clicked the policykit 'cancel' button and not throw an 'authentication failed' error message at the user.
v2: Don't leak pkout
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- daemon/remote.c | 14 ++++++++++++-- include/libvirt/virterror.h | 1 + src/util/virterror.c | 6 ++++++ 3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 1ada146..cedc26a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2471,6 +2471,8 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, const char *action; int status = -1; char *ident = NULL; + int authdismissed = 0;
bool authdismissed = false;
ACK with that nit fixed.
Thanks, pushed with that nit fixed. - Cole
participants (2)
-
Cole Robinson -
Eric Blake