Looks reasonable, but maybe there needs to be a way to get the list of all existing groups, for this to be a complete api ?

Yes, Groups should also be first class DBus objects, and we really should be using dbus paths to refer to them. This is because groups will be coming from various sources (eg: domains).

Here's a rough sketch of what Group objects would look like:

https://fedorahosted.org/sssd/wiki/DesignDocs/AccountsService#GroupObjectsAllNew

(In reply to comment #2)
> Yes, Groups should also be first class DBus objects, and we really should be
> using dbus paths to refer to them. This is because groups will be coming
> from various sources (eg: domains).

Right, makes sense.

(But don't groups from domains also need to work with getgroups/getgrnam/getgrgid etc?  I.e., there isn't a strict _need_ to make them D-Bus objects, it's just the right way to extend the existing API.  Non?)

> Here's a rough sketch of what Group objects would look like:
> 
> https://fedorahosted.org/sssd/wiki/DesignDocs/
> AccountsService#GroupObjectsAllNew

What if I follow that design without the domain and nested group bits, making sure that SSSD can still cleanly provide a comatible superset of the API?

(In reply to comment #3)
> What if I follow that design without the domain and nested group bits,
> making sure that SSSD can still cleanly provide a comatible superset of the
> API?

Yes, that makes sense.

I have started working on this here:

    http://cgit.freedesktop.org/accountsservice/log/?h=groups

Created attachment 87926 [details] [review]
Daemon: Support for local groups.

Created attachment 87927 [details] [review]
Library: Support for groups.

Comment on attachment 87927 [details] [review]
Library: Support for groups.

Oops, those patches were bogus.

Created attachment 87930 [details] [review]
Daemon: Support for local groups.

Created attachment 87931 [details] [review]
Library: Support for groups.

Here are the first version of group support based on

https://fedorahosted.org/sssd/wiki/DesignDocs/AccountsService#GroupObjectsAllNew

They don't apply to master because I haven't moved to a new enough glib yet and am not using master myself right now.  That will happen soonish.

Created attachment 88027 [details] [review]
Daemon: Support for local groups.

Created attachment 88028 [details] [review]
Library: Support for groups.

Now the patches should apply to master.  I use them on top of the patches from bug  70770 and bug 70540.

Comment on attachment 88027 [details] [review]
Daemon: Support for local groups.

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

Some minor comments.  Basically looks OK to me, would like Ray's review too.

::: data/org.freedesktop.Accounts.Group.xml
@@ +16,5 @@
> +    <doc:doc>
> +      <doc:description>
> +        <doc:para>
> +          Sets the groups groupname. Note that it is usually not allowed
> +          to have multiple groups with the same groupname.

Why not just explicitly disallow?

@@ +46,5 @@
> +    </arg>
> +    <doc:doc>
> +      <doc:description>
> +        <doc:para>
> +          Adds a user to the group as a direct member.

Should say what happens if the user is already a member - which in this case is throw an error.

@@ +76,5 @@
> +    </arg>
> +    <doc:doc>
> +      <doc:description>
> +        <doc:para>
> +          Removes a user from the direct members of a group.

Ditto for removal.

::: data/org.freedesktop.Accounts.User.xml
@@ +809,5 @@
> +          The cached groups that the user is a member of (directly or
> +          indirectly).  This is not an exhaustive list of all groups
> +          that the user is a member of; the user might be a member of
> +          groups that are not currently cached.  To find all such
> +          groups, use the FindGroups method.

It feels odd to have this explicit caching but not also describe under what conditions the cache is updated.

In the current implementation that's basically only local groups, right?   I guess the idea is later when we support network groups we can cache the result here?

::: src/user.c
@@ +400,5 @@
> +void
> +user_reset_cached_groups (User *user)
> +{
> +        user->cached_groups_stage = g_malloc_n (1, sizeof(gchar*));
> +        user->cached_groups_stage[0] = NULL;

This might be a bit better as a GPtrArray:

user->cached_groups_stage = g_ptr_array_new_with_free_func (g_free);
g_ptr_array_add (user->cached_groups_stage, NULL);

@@ +412,5 @@
> +        n += 1;
> +        user->cached_groups_stage = g_realloc_n (user->cached_groups_stage,
> +                                                 n+1, sizeof(gchar*));
> +        user->cached_groups_stage[n-1] = g_strdup (group_get_object_path (group));
> +        user->cached_groups_stage[n] = NULL;

Then:

g_ptr_array_remove_index_fast (user->cached_groups_stage, user->cached_groups_stage->len - 1);
g_ptr_array_add (user->cached_groups_stage, g_strdup (group_get_object_path (group));
g_ptr_array_add (user->cached_groups_stage, NULL);

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/accountsservice/accountsservice/issues/36.