[liboobs] Store OobsUser's main group as GID rather than OobsGroup

[Milan Bouchet-Valat <milanbv src gnome org>]

commit 97e5266c29fcfe2d9769079e079422956ab6f55e
Author: Milan Bouchet-Valat <nalimilan club fr>
Date:   Wed Dec 8 18:40:14 2010 +0100

    Store OobsUser's main group as GID rather than OobsGroup
    
    Storing main group as a reference to an OobsGroup means we have to
    keep track of OobsGroupsConfig updates, and mess with reference
    cycles. The most accurate information provided by the system is
    the GID of the main group, so it's better to use this and retrieve
    the corresponding OobsGroup when oobs_user_get_main_group() is called.
    
    This fixes issues with OobsGroupsConfig not being updated properly when
    creating a user triggers the creation of its main group in the backends:
    we couldn't update OobsUsersConfig and OobsGroupsConfig correctly since
    they each depended on the other, which means the new main group was not
    found, and made subsequent commits fail (invalid GID). See e.g.
    https://bugs.launchpad.net/ubuntu/+source/gnome-system-tools/+bug/659758

 oobs/oobs-groupsconfig.c |    4 +-
 oobs/oobs-user-private.h |    3 +-
 oobs/oobs-user.c         |   55 ++++++++++++++++++++--------------------------
 oobs/oobs-usersconfig.c  |   31 ++++---------------------
 4 files changed, 32 insertions(+), 61 deletions(-)
---
diff --git a/oobs/oobs-groupsconfig.c b/oobs/oobs-groupsconfig.c
index 79702d2..ba7eb5f 100644
--- a/oobs/oobs-groupsconfig.c
+++ b/oobs/oobs-groupsconfig.c
@@ -431,9 +431,9 @@ oobs_groups_config_get_from_name (OobsGroupsConfig *config, const gchar *name)
 }
 
 /**
- * oobs_groups_config_get_from_uid:
+ * oobs_groups_config_get_from_gid:
  * @config: An #OobsGroupsConfig.
- * @gid: the UID of the wanted group.
+ * @gid: the GID of the wanted group.
  *
  * Gets the (first) group whose GID is @gid. This is a convenience function
  * to avoid walking manually over the groups list.
diff --git a/oobs/oobs-user-private.h b/oobs/oobs-user-private.h
index c99ba72..67ca983 100644
--- a/oobs/oobs-user-private.h
+++ b/oobs/oobs-user-private.h
@@ -29,10 +29,9 @@ G_BEGIN_DECLS
 
 OobsUser *
 _oobs_user_create_from_dbus_reply (OobsUser        *user,
-                                   gid_t           *gid_ptr,
                                    DBusMessage     *reply,
                                    DBusMessageIter  iter);
 
 G_END_DECLS
 
-#endif /* __OOBS_USER_PRIVATE_H */
\ No newline at end of file
+#endif /* __OOBS_USER_PRIVATE_H */
diff --git a/oobs/oobs-user.c b/oobs/oobs-user.c
index e94f11b..e9d85ad 100644
--- a/oobs/oobs-user.c
+++ b/oobs/oobs-user.c
@@ -32,6 +32,7 @@
 #include "oobs-user.h"
 #include "oobs-user-private.h"
 #include "oobs-group.h"
+#include "oobs-groupsconfig.h"
 #include "oobs-defines.h"
 #include "utils.h"
 
@@ -50,10 +51,10 @@ typedef struct _OobsUserPrivate OobsUserPrivate;
 struct _OobsUserPrivate {
   OobsObject *config;
 
-  OobsGroup *main_group;
   gchar *username;
   gchar *password;
   uid_t  uid;
+  gid_t  gid;
   
   gchar *homedir;
   gchar *shell;
@@ -266,8 +267,9 @@ oobs_user_init (OobsUser *user)
   priv->locale        = NULL;
 
   /* This value ensures the backends will use the system default
-   * if UID were not changed manually. */
+   * if UID/GID were not changed manually. */
   priv->uid = G_MAXUINT32;
+  priv->gid = G_MAXUINT32;
 
   priv->passwd_empty    = FALSE;
   priv->passwd_disabled = FALSE;
@@ -441,9 +443,6 @@ oobs_user_finalize (GObject *object)
       g_free (priv->other_data);
       g_free (priv->locale);
 
-      if (priv->main_group)
-	g_object_unref (priv->main_group);
-
       /* Erase password field in case it's not done yet */
       if (priv->password) {
 	memset (priv->password, 0, strlen (priv->password));
@@ -457,10 +456,10 @@ oobs_user_finalize (GObject *object)
 
 OobsUser*
 _oobs_user_create_from_dbus_reply (OobsUser        *user,
-                                   gid_t           *gid_ptr,
                                    DBusMessage     *reply,
                                    DBusMessageIter  struct_iter)
 {
+  OobsUserPrivate *priv;
   DBusMessageIter iter, gecos_iter;
   guint32 uid, gid;
   const gchar *login, *passwd, *home, *shell;
@@ -474,10 +473,7 @@ _oobs_user_create_from_dbus_reply (OobsUser        *user,
   login = utils_get_string (&iter);
   passwd = utils_get_string (&iter);
   uid = utils_get_uint (&iter);
-
   gid = utils_get_uint (&iter);
-  if (gid_ptr)
-    *gid_ptr = gid;
 
   /* GECOS fields */
   dbus_message_iter_recurse (&iter, &gecos_iter);
@@ -521,6 +517,10 @@ _oobs_user_create_from_dbus_reply (OobsUser        *user,
                 "locale", locale,
                 NULL);
 
+  /* GID is only kept internally */
+  priv = user->_priv;
+  priv->gid = gid;
+
   return user;
 }
 
@@ -529,8 +529,8 @@ create_dbus_struct_from_user (OobsUser        *user,
 			      DBusMessage     *message,
 			      DBusMessageIter *iter)
 {
-  OobsGroup *group;
-  guint32 uid, gid;
+  OobsUserPrivate *priv;
+  guint32 uid;
   gchar *login, *password, *shell, *homedir;
   gchar *name, *room_number, *work_phone, *home_phone, *other_data;
   gchar *locale;
@@ -539,6 +539,8 @@ create_dbus_struct_from_user (OobsUser        *user,
   gint passwd_flags, home_flags;
   DBusMessageIter data_iter;
 
+  priv = user->_priv;
+
   g_object_get (user,
 		"name", &login,
 		"password", &password,
@@ -561,20 +563,12 @@ create_dbus_struct_from_user (OobsUser        *user,
    * since home dir, password and shell are allowed to be empty (see man 5 passwd) */
   g_return_val_if_fail (login, FALSE);
 
-  group = oobs_user_get_main_group (user);
-
-  /* G_MAXUINT32 is used to mean no main group */
-  if (group)
-    gid = oobs_group_get_gid (group);
-  else
-    gid = G_MAXUINT32;
-
   passwd_flags = passwd_empty | (passwd_disabled << 1);
 
   utils_append_string (iter, login);
   utils_append_string (iter, password);
   utils_append_uint (iter, uid);
-  utils_append_uint (iter, gid);
+  utils_append_uint (iter, priv->gid);
 
   dbus_message_iter_open_container (iter, DBUS_TYPE_ARRAY, DBUS_TYPE_STRING_AS_STRING, &data_iter);
 
@@ -658,7 +652,8 @@ oobs_user_update (OobsObject *object)
   reply = _oobs_object_get_dbus_message (object);
 
   dbus_message_iter_init (reply, &iter);
-  _oobs_user_create_from_dbus_reply (OOBS_USER (object), NULL, reply, iter);
+
+  _oobs_user_create_from_dbus_reply (OOBS_USER (object), reply, iter);
 }
 
 /**
@@ -764,19 +759,21 @@ oobs_user_set_uid (OobsUser *user, uid_t uid)
  * 
  * Returns the main group of this user.
  * 
- * Return Value: main group for the user. this value is owned
- *               by the #OobsUser object, you do not have to
- *               unref it.
+ * Return Value: main group for the user. Release reference when no longer needed.
  **/
 OobsGroup*
 oobs_user_get_main_group (OobsUser *user)
 {
   OobsUserPrivate *priv;
+  OobsObject *groups_config;
 
   g_return_val_if_fail (OOBS_IS_USER (user), NULL);
 
   priv = user->_priv;
-  return priv->main_group;
+  groups_config = oobs_groups_config_get ();
+
+  return oobs_groups_config_get_from_gid (OOBS_GROUPS_CONFIG (groups_config),
+                                          priv->gid);
 }
 
 /**
@@ -784,8 +781,7 @@ oobs_user_get_main_group (OobsUser *user)
  * @user: An #OobsUser.
  * @main_group: an #OobsGroup, new main group for the user.
  * 
- * Sets the main group for the user, adds a reference to
- * the new main group.
+ * Sets the main group for the user.
  **/
 void
 oobs_user_set_main_group (OobsUser  *user,
@@ -797,10 +793,7 @@ oobs_user_set_main_group (OobsUser  *user,
 
   priv = user->_priv;
 
-  if (priv->main_group)
-    g_object_unref (priv->main_group);
-
-  priv->main_group = (main_group) ? g_object_ref (main_group) : NULL;
+  priv->gid = (main_group) ? oobs_group_get_gid (main_group) : G_MAXUINT32;
 }
 
 /**
diff --git a/oobs/oobs-usersconfig.c b/oobs/oobs-usersconfig.c
index 35ba7e7..f45e6c2 100644
--- a/oobs/oobs-usersconfig.c
+++ b/oobs/oobs-usersconfig.c
@@ -289,21 +289,6 @@ oobs_users_config_get_property (GObject      *object,
 }
 
 static void
-query_groups_foreach (OobsUser *user,
-		      gid_t     gid,
-		      gpointer  data)
-{
-  OobsGroupsConfig *groups_config = OOBS_GROUPS_CONFIG (data);
-  OobsGroup *group;
-
-  group = oobs_groups_config_get_from_gid (groups_config, gid);
-  oobs_user_set_main_group (user, group);
-
-  if (group)
-    g_object_unref (group);
-}
-
-static void
 oobs_users_config_groups_updated (OobsUsersConfig  *users,
 				  OobsGroupsConfig *groups)
 {
@@ -311,8 +296,6 @@ oobs_users_config_groups_updated (OobsUsersConfig  *users,
   OobsGroup *group;
 
   priv = users->_priv;
-  g_hash_table_foreach (priv->groups, (GHFunc) query_groups_foreach, groups);
-
   /* get the default group */
   if (priv->default_gid > 0)
     {
@@ -333,7 +316,6 @@ oobs_users_config_update (OobsObject *object)
   DBusMessageIter  iter, elem_iter;
   OobsListIter     list_iter;
   GObject         *user;
-  gid_t            gid;
 
   priv  = OOBS_USERS_CONFIG (object)->_priv;
   reply = _oobs_object_get_dbus_message (object);
@@ -346,20 +328,13 @@ oobs_users_config_update (OobsObject *object)
 
   while (dbus_message_iter_get_arg_type (&elem_iter) == DBUS_TYPE_STRUCT)
     {
-      user = G_OBJECT (_oobs_user_create_from_dbus_reply (NULL, &gid, reply, elem_iter));
+      user = G_OBJECT (_oobs_user_create_from_dbus_reply (NULL, reply, elem_iter));
 
       oobs_list_append (priv->users_list, &list_iter);
       oobs_list_set    (priv->users_list, &list_iter, G_OBJECT (user));
 
       g_object_unref (user);
 
-      /* keep the group name in a hashtable, this will be needed
-       * each time the groups configuration changes
-       */
-      g_hash_table_insert (priv->groups,
-                           user,
-                           (gpointer) gid);
-
       dbus_message_iter_next (&elem_iter);
     }
 
@@ -488,6 +463,10 @@ oobs_users_config_add_user (OobsUsersConfig *config, OobsUser *user)
   oobs_list_append (priv->users_list, &list_iter);
   oobs_list_set (priv->users_list, &list_iter, G_OBJECT (user));
 
+  /* Adding a user can trigger the creation of its new main group,
+   * which we need to take into account. */
+  oobs_object_update (oobs_groups_config_get ());
+
   return OOBS_RESULT_OK;
 }