PolicyKit oddities

[David Zeuthen]

On Fri, 2008-06-06 at 12:02 +0100, Richard Hughes wrote:
> I've just been reviewing how we are using polkit in PackageKit and
> changing it to use an authentication agent rather than hardcoding
> polkit-gnome.
> 
> Some oddities I noticed...
> 
> * polkit_auth_obtain uses DBusError rather than PolKitError when it's
> nothing to do with DBUS...

That's because it happens over the bus; besides you ought to be using

http://hal.freedesktop.org/docs/PolicyKit-gnome/polkit-gnome-polkit-gnome-auth.html#polkit-gnome-auth-obtain

since you really want it to be async.

> * polkit_action_get_action_id returns a action_id as a type (char **)
> and in the docs it says that it shouldn't be freed. Shouldn't this
> therefore be a (char * const *)?

Probably even const * char *; however the pattern is that most libraries
that return an array of strings don't do the const-dance; mostly because
most existing utility functions taking an array of strings are wrong wrt
const, e.g. g_strv_length() and friends. So that's why.

> * polkit_dbus_error_parse_from_strings takes a error_name and
> error_message parameter. Using a GError you only get the domain, code
> and message, and so it might be worth mentioning in the docs to use
> dbus_g_error_get_name on the GError.

Yeah, what I want to do is to have a libpolkit-gobject library that
includes some parts of what's in libpolkit-gnome. Including G-versions
of polkit_dbus_error_parse_from_strings(), polkit_dbus_error_generate(),
polkit_auth_obtain() etc.

> * Also, when using polkit_dbus_error_parse, the error name is hardcoded
> to org.freedesktop.PolicyKit.Error.NotAuthorized when the error
> PackageKit generates is org.freedesktop.PackageKit.RefusedByPolicy - is
> this my bug or yours?

So initially there was no recommendation of what error names to use;
however I decided it would be more useful if everyone just used the same
name. Also, when I add object support the contents of the error will
need to include the object path, e.g. instead of just being

 org.nm.can_dial_number auth_admin

it will be

 org.nm.can_dial_number:telephone-number:1-555-1234-5678 auth_admin

and assuming that software follows this recommendation, adding support
for objects will be mostly free.

So I'd recommend you switch PackageKit to using

 polkit_dbus_error_generate()

on the daemon side; and

 polkit_dbus_error_parse_from_strings()

on the daemon side.

> * It seemed odd to pass polkit_auth_obtain an action_id not a
> PolKitAction - but maybe that's just me.

It's because the "Simple convenience interface" which is a dumbed down
interface.. that interface was mostly added so patching legacy apps like
crontab(1) is easy, e.g.

http://gitweb.freedesktop.org/?p=PolicyKit.git;a=commit;h=a712e78e69220b43695463e00983e9316a646d32

which is a lot simpler than creating PolKitAction and PolKitCaller
objects and what not.

You really should be using polkit_gnome_auth_obtain() instead; it's
almost always wrong to using a sync call for this kind of stuff.

> 
> If you agree with any of this lot, yell, and I'll russle up some patches
> for you.
> 
> Richard.
> 
>