Created attachment 78245 [details] [review]
User: hold on to our keyfile

When updating a User object from keyfile, keep reference on the keyfile
object passed in and modify the Daemon not to destroy it.

When saving a User, instead of creating a new keyfile, reuse the one we
stored on the object.

We still record the properties from the User object into the keyfile in
the previous way (and include a modification to clear out the 'User'
group before doing this).

The intention here is to allow other groups stored in the keyfile to be
preserved.  These other groups are likely to correspond to extension
interfaces (which will be introduced in future patches).

An alternative approach would have been to only preserve groups for
extensions that we currently have loaded but this was abandoned as being
excessively brittle since the result of a temporarily missing extension
file would be the destruction of data.

Created attachment 78246 [details] [review]
service: add support for extension interfaces

First pass at what a patch might look like.  Requires the new GDBus
async property handling changes.

Comment on attachment 78245 [details] [review]
User: hold on to our keyfile

Review of attachment 78245 [details] [review]:
-----------------------------------------------------------------

Looks good.

Comment on attachment 78246 [details] [review]
service: add support for extension interfaces

Review of attachment 78246 [details] [review]:
-----------------------------------------------------------------

Looked over things. I haven't tested this yet. As noted I'm not sure how this actually works with the o.f.d.Properties method invocations.

Needs clear documentation on how this works, and an example, in the doc directory.

I'm not thrilled with the validation of the symlinks. It seems unnecessary, and unlikely that future implementations of this accounts service API would do things exactly the same way (down to the symlink validation) either. In addition the use of XDG_DATA_DIRS seems incompatible with the symlink validation, and perhaps itself also unnecessary (should we just use one standard directory).

Does it make sense to break out the additional code in daemon.c into a new extension.c, as daemon.c is really long already.

::: src/daemon.c
@@ +1,4 @@
>  /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*-
>   *
>   * Copyright (C) 2009-2010 Red Hat, Inc.
> + * Copyright © 2013 Canonical Limited

Do we really have to put utf-8 chars here? :D

@@ +735,5 @@
>          queue_reload_autologin (daemon);
>  }
>  
> +static gchar *
> +read_symlink (const gchar *filename)

Should we just use g_file_read_link(), and if it doesn't work shouldn't we fix g_file_read_link()?

@@ +805,5 @@
> +        if (g_hash_table_contains (ifaces, iface->name))
> +                return;
> +
> +        /* Only accept interfaces with only properties */
> +        if ((iface->methods && *iface->methods)|| (iface->signals && *iface->signals))

Using methods[0] and signals[0] would be clearer here. Also whitespace.

@@ +826,5 @@
> +        gchar *contents;
> +        gint i;
> +
> +        if (!g_file_get_contents (filename, &contents, NULL, NULL))
> +                return;

Maybe a g_warning() with the error for easier debugging? Or is this supposed to be able to fail?

@@ +828,5 @@
> +
> +        if (!g_file_get_contents (filename, &contents, NULL, NULL))
> +                return;
> +
> +        node = g_dbus_node_info_new_for_xml (contents, NULL);

Again, a g_warning about parsing failure?

@@ +842,5 @@
> +
> +static void
> +daemon_read_extension_directory (GHashTable  *ifaces,
> +                                 const gchar *path)
> +{

Can you put a comment here as to why we're checking and ensuring these are symlinks? Something about the plan to later just lookup annotations in the interfaces directory? Because without that knowledge it seems like a bunch of needless complexity.

@@ +864,5 @@
> +                }
> +
> +                /* Ensure it looks like "../../dbus-1/interfaces/${name}" */
> +                const gchar * const prefix = "../../dbus-1/interfaces/";
> +                if (g_str_has_prefix (symlink, prefix) && g_str_equal (symlink + strlen (prefix), name))

If we're going to do this, then you need to print messages for invalid files, otherwise this is a nightmare to debug.

@@ +883,5 @@
> +        gint i;
> +
> +        ifaces = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify) g_dbus_interface_info_unref);
> +
> +        data_dirs = g_get_system_data_dirs ();

It seems wierd to me that we get all intense about the fact that the files are symlinks (above), and then allow the location of those symlinks to be littered all over the place.

Also, the above code checks for a certain prefix to the symlinks. The symlinks (if they do indeed point to the "dbus-1/interfaces" directory as intended) won't have that prefix when using a non-standard XDG_DATA_DIRS.

In general I would just use the prefix, and also drop this verification stuff unless we have a really good reason.

::: src/user.c
@@ +463,5 @@
> +
> +                if (g_str_equal (annotation->key, "org.freedesktop.Accounts.DefaultValue.String")) {
> +                        if (g_str_equal (property->signature, "s"))
> +                                return g_variant_ref_sink (g_variant_new_string (annotation->value));
> +                }

Can't we just fallback g_variant_new_string if signature is "s" and g_variant_parse fails? That is, rather than have both org.freedesktop.Accounts.DefaultValue.String and org.freedesktop.Accounts.DefaultValue.

@@ +480,5 @@
> +user_extension_authentication_done (Daemon                *daemon,
> +                                    User                  *user,
> +                                    GDBusMethodInvocation *invocation,
> +                                    gpointer               user_data)
> +{

I think it would make sense to break out the Get() GetAll() Set() implementations into their own functions, and call them from here. This function seems very long.

@@ +497,5 @@
> +                        g_dbus_method_invocation_return_value (invocation, g_variant_new ("(v)", value));
> +                        g_variant_unref (value);
> +                }
> +                else {
> +                        g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, G_DBUS_ERROR_FILE_NOT_FOUND,

Is this really the error standard code returned from o.f.d.Properties.Get() when an invalid property is passedin?

@@ +662,5 @@
> +         */
> +        while (g_hash_table_iter_next (&iter, NULL, &iface))
> +                user->extension_ids[i++] = g_dbus_connection_register_object (user->system_bus_connection,
> +                                                                              user->object_path, iface,
> +                                                                              &vtable, user, NULL, NULL);

So I'm a bit confused here. How does this actually work? How are Get()/Set() and friends for extension properties routed into user_extension_method_call() but other (standard) properties unaffected?

Ryan,

are you going to revise the second patch ?

Ryan, is there a glib patch for g_dbus_method_invocation_get_property_info()? I can't find it in the glib sources, or a quick search on bugzilla.gnome.org.

Comment on attachment 78246 [details] [review]
service: add support for extension interfaces

Review of attachment 78246 [details] [review]:
-----------------------------------------------------------------

::: src/daemon.c
@@ +1,4 @@
>  /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*-
>   *
>   * Copyright (C) 2009-2010 Red Hat, Inc.
> + * Copyright © 2013 Canonical Limited

removed

@@ +735,5 @@
>          queue_reload_autologin (daemon);
>  }
>  
> +static gchar *
> +read_symlink (const gchar *filename)

I skipped this because I thought it was a method on GFile and I didn't want to create one of those, but now I see that I assumed wrong.

Good suggestion.  Changed.

@@ +805,5 @@
> +        if (g_hash_table_contains (ifaces, iface->name))
> +                return;
> +
> +        /* Only accept interfaces with only properties */
> +        if ((iface->methods && *iface->methods)|| (iface->signals && *iface->signals))

done and done

@@ +826,5 @@
> +        gchar *contents;
> +        gint i;
> +
> +        if (!g_file_get_contents (filename, &contents, NULL, NULL))
> +                return;

added

@@ +828,5 @@
> +
> +        if (!g_file_get_contents (filename, &contents, NULL, NULL))
> +                return;
> +
> +        node = g_dbus_node_info_new_for_xml (contents, NULL);

added

@@ +842,5 @@
> +
> +static void
> +daemon_read_extension_directory (GHashTable  *ifaces,
> +                                 const gchar *path)
> +{

Comment added.

@@ +864,5 @@
> +                }
> +
> +                /* Ensure it looks like "../../dbus-1/interfaces/${name}" */
> +                const gchar * const prefix = "../../dbus-1/interfaces/";
> +                if (g_str_has_prefix (symlink, prefix) && g_str_equal (symlink + strlen (prefix), name))

added a warning here and also above for the case that we discover that the file is not a symlink.

@@ +883,5 @@
> +        gint i;
> +
> +        ifaces = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify) g_dbus_interface_info_unref);
> +
> +        data_dirs = g_get_system_data_dirs ();

The idea is that you should install the file, primarily, as a D-Bus interface, in the normal D-Bus interfaces directory, because that's where interfaces belong.

The symlink is only there to help us find which files are interesting to us.

As for the prefix thing: you will always install in the dbus-1/interfaces/ subdirectory of your prefix (which may or may not be /usr) and always put the symlink under the prefix as well.  This is why the symlink should always look like "../../"...  you will never want to have a case where you install a symlink in some weird prefix and link it back to /usr.

::: src/user.c
@@ +463,5 @@
> +
> +                if (g_str_equal (annotation->key, "org.freedesktop.Accounts.DefaultValue.String")) {
> +                        if (g_str_equal (property->signature, "s"))
> +                                return g_variant_ref_sink (g_variant_new_string (annotation->value));
> +                }

No.  I don't like this at all.  These sort of "try to parse one way then fall back to another way" where the two methods of parsing are not totally disjoint leads to very annoying and confusing behaviours.

Consider that this approach allows:

 <tag>foo</tag>

to be parsed as string "foo" but then if I add quotes,

 <tag>'foo'</tag>

will still be parsed as string "foo", not "'foo'".

If I actually literally want quotes, I start to get frustrated at this point, wondering why there seems to be some special magic handling here making my life difficult.

@@ +480,5 @@
> +user_extension_authentication_done (Daemon                *daemon,
> +                                    User                  *user,
> +                                    GDBusMethodInvocation *invocation,
> +                                    gpointer               user_data)
> +{

sure.

@@ +497,5 @@
> +                        g_dbus_method_invocation_return_value (invocation, g_variant_new ("(v)", value));
> +                        g_variant_unref (value);
> +                }
> +                else {
> +                        g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, G_DBUS_ERROR_FILE_NOT_FOUND,

Nope.  It's not.  Changed this to _ERROR_INVALID_ARGS, which is what GDBus itself uses.

@@ +662,5 @@
> +         */
> +        while (g_hash_table_iter_next (&iter, NULL, &iface))
> +                user->extension_ids[i++] = g_dbus_connection_register_object (user->system_bus_connection,
> +                                                                              user->object_path, iface,
> +                                                                              &vtable, user, NULL, NULL);

In general, registering objects in GDBus is done on a per-interface basis.  Two separate people can register '/some/path' for two separate interfaces and handle them completely separately, without any impact on the other.

My patch to add support for async property handling didn't change this: it's still done per-interface.

This is what you see here: we only register using this handler for the extension interfaces.  The normal interfaces are completely unaffected.

Created attachment 81276 [details] [review]
User: hold on to our keyfile

I modified this patch slightly: it now adds the keyfile to the user object on _init because I was hitting places where this could be NULL if the user's keyfile did not already exist and we tried to set some properties.

I'm not exactly sure how this is supposed to work, and I think my Fedora 19 system is currently getting it wrong.  I have a user on my system that has no keyfile and it doesn't show up in the accounts service.... (using the system version).  Help?

Created attachment 81277 [details] [review]
service: add support for extension interfaces

Updated, taking into account all of your comments except for the ones I addressed.

We still do the symlink check, but document it better and give better warnings.  We also still have the two different attribute types for string vs. not.

Created attachment 81278 [details] [review]
Provide a sample extension interface

Here's an example extension interface and corresponding polkit file.

I don't really know where to include this into "more proper" documentation because we only really have a library API reference and a D-Bus API reference...

These commits look good to me. Will push them, with a dependency on a newer gio.

Again, although I understand why you've done the symlink stuff, and I'm fine with committing it for now. I think that restriction will likely be dropped in the future if this code finds its way into another project (like sssd).