Bug 65926

Summary: [PATCH] Manage groups
Product: accountsservice Reporter: Marius Vollmer <marius.vollmer>
Component: generalAssignee: Matthias Clasen <mclasen>
Status: RESOLVED MOVED QA Contact:
Severity: normal    
Priority: medium CC: jhrozek, marius.vollmer, rstrode, stefw
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Manage group membership.
Daemon: Support for local groups.
Library: Support for groups.
Daemon: Support for local groups.
Library: Support for groups.
Daemon: Support for local groups.
Library: Support for groups.

Description Marius Vollmer 2013-06-19 10:09:30 UTC
Created attachment 81059 [details] [review]
Manage group membership.

It is useful to also expose group membership of users and to allow changing it.
Comment 1 Matthias Clasen 2013-06-30 18:02:08 UTC
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 ?
Comment 2 Stef Walter 2013-07-24 17:01:11 UTC
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
Comment 3 Marius Vollmer 2013-07-24 19:25:33 UTC
(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?
Comment 4 Stef Walter 2013-07-25 07:18:45 UTC
(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.
Comment 5 Marius Vollmer 2013-10-14 07:44:10 UTC
I have started working on this here:

    http://cgit.freedesktop.org/accountsservice/log/?h=groups
Comment 6 Marius Vollmer 2013-10-21 13:19:04 UTC
Created attachment 87926 [details] [review]
Daemon: Support for local groups.
Comment 7 Marius Vollmer 2013-10-21 13:19:29 UTC
Created attachment 87927 [details] [review]
Library: Support for groups.
Comment 8 Marius Vollmer 2013-10-21 13:23:35 UTC
Comment on attachment 87927 [details] [review]
Library: Support for groups.

Oops, those patches were bogus.
Comment 9 Marius Vollmer 2013-10-21 13:49:05 UTC
Created attachment 87930 [details] [review]
Daemon: Support for local groups.
Comment 10 Marius Vollmer 2013-10-21 13:49:39 UTC
Created attachment 87931 [details] [review]
Library: Support for groups.
Comment 11 Marius Vollmer 2013-10-21 13:52:22 UTC
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.
Comment 12 Marius Vollmer 2013-10-23 09:00:50 UTC
Created attachment 88027 [details] [review]
Daemon: Support for local groups.
Comment 13 Marius Vollmer 2013-10-23 09:01:15 UTC
Created attachment 88028 [details] [review]
Library: Support for groups.
Comment 14 Marius Vollmer 2013-10-23 09:03:20 UTC
Now the patches should apply to master.  I use them on top of the patches from bug  70770 and bug 70540.
Comment 15 Colin Walters 2013-11-26 21:27:56 UTC
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);
Comment 16 GitLab Migration User 2018-08-07 09:32:52 UTC
-- 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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.