PolicyKit oddities

[Richard Hughes]

On Fri, 2008-06-06 at 09:35 -0400, David Zeuthen wrote:
> 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.

Sure, this is what I use in the libpackagekit (designed for console too)
library, by I agree with you for the gnome bits.

> > * 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.

Yes, g_strv_length confuses me a bit why it's char **.

> > * 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.

Excellent.

> > * 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.

Cool.

> So I'd recommend you switch PackageKit to using
> 
>  polkit_dbus_error_generate()

Ahh, I'm using the glib bindings, so I have to generate a GError - which
I do using:

*error = g_error_new (PK_TRANSACTION_ERROR,
PK_TRANSACTION_ERROR_REFUSED_BY_POLICY, "%s", error_detail);

> on the daemon side; and
> 
>  polkit_dbus_error_parse_from_strings()

Yup, I use this now, thanks.

> 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.

Agreed. Cheers dude.

Richard.