Attachment 131370 Details for Bug 101052 – daemon: don't treat explicitly requested users as "cached"

[patch] daemon: don't treat explicitly requested users as "cached"

daemon-dont-treat-explicitly-requested-users-as-ca.patch (text/plain), 24.76 KB, created by Ray Strode [halfline] on 2017-05-15 21:13:01 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ray Strode [halfline]

Created: 2017-05-15 21:13:01 UTC

Size: 24.76 KB

patch

obsolete

>From edac45754cbcc044216071e19ec838cb13f54bc2 Mon Sep 17 00:00:00 2001
>From: Ray Strode <rstrode@redhat.com>
>Date: Thu, 4 May 2017 12:04:05 -0400
>Subject: [PATCH] daemon: don't treat explicitly requested users as "cached"
>
>The ListCachedUsers method currently returns users that have
>been explicitly requested by a client.  It's weird that merely
>querying a user can make it show up in login screen user lists.
>Furthermore, UncacheUser is broken since commit
>177509e9460b149ecbf85e75c930be2ea00b7d05 because the user has
>been explicitly requested in order to uncache it.  So trying
>to uncache a user inadvertently caches the user.
>
>This commit fixes that.
>
>https://bugs.freedesktop.org/show_bug.cgi?id=101052
>---
> src/daemon.c | 71 +++++++++++++++++++++++++++++++++++++++---------------------
> src/user.c   | 17 +++++++++++++++
> src/user.h   |  3 +++
> 3 files changed, 66 insertions(+), 25 deletions(-)
>
>diff --git a/src/daemon.c b/src/daemon.c
>index 312394a..6e3e4b3 100644
>--- a/src/daemon.c
>+++ b/src/daemon.c
>@@ -329,100 +329,108 @@ entry_generator_requested_users (Daemon       *daemon,
>                 while (node != NULL) {
>                         const char *name;
> 
>                         name = node->data;
>                         node = node->next;
> 
>                         *state = node;
> 
>                         if (!g_hash_table_lookup (users, name)) {
>                                 pwent = getpwnam (name);
>                                 if (pwent == NULL) {
>                                         g_debug ("user '%s' requested previously but not present on system", name);
>                                 } else {
>                                         *shadow_entry = getspnam (pwent->pw_name);
> 
>                                         return pwent;
>                                 }
>                         }
>                 }
>         }
> 
>         /* Last iteration */
> 
>         *state = NULL;
>         return NULL;
> }
> 
> static void
> load_entries (Daemon             *daemon,
>               GHashTable         *users,
>-              gboolean            allow_system_users,
>+              gboolean            explicitly_requested,
>               EntryGeneratorFunc  entry_generator)
> {
>         gpointer generator_state = NULL;
>         struct passwd *pwent;
>         struct spwd *spent = NULL;
>         User *user = NULL;
> 
>         g_assert (entry_generator != NULL);
> 
>         for (;;) {
>                 spent = NULL;
>                 pwent = entry_generator (daemon, users, &generator_state, &spent);
>                 if (pwent == NULL)
>                         break;
> 
>                 /* Skip system users... */
>-                if (!allow_system_users && !user_classify_is_human (pwent->pw_uid, pwent->pw_name, pwent->pw_shell, spent? spent->sp_pwdp : NULL)) {
>+                if (!explicitly_requested && !user_classify_is_human (pwent->pw_uid, pwent->pw_name, pwent->pw_shell, spent? spent->sp_pwdp : NULL)) {
>                         g_debug ("skipping user: %s", pwent->pw_name);
>                         continue;
>                 }
> 
>-                /* ignore duplicate entries */
>-                if (g_hash_table_lookup (users, pwent->pw_name)) {
>-                        continue;
>-                }
>+                /* Only process users that haven't been processed yet.
>+                 * We do always make sure entries get promoted
>+                 * to "cached" status if they are supposed to be
>+                 */
>+
>+                user = g_hash_table_lookup (users, pwent->pw_name);
> 
>-                user = g_hash_table_lookup (daemon->priv->users, pwent->pw_name);
>                 if (user == NULL) {
>-                        user = user_new (daemon, pwent->pw_uid);
>-                } else {
>-                        g_object_ref (user);
>-                }
>+                        user = g_hash_table_lookup (daemon->priv->users, pwent->pw_name);
>+                        if (user == NULL) {
>+                                user = user_new (daemon, pwent->pw_uid);
>+                        } else {
>+                                g_object_ref (user);
>+                        }
> 
>-                /* freeze & update users not already in the new list */
>-                g_object_freeze_notify (G_OBJECT (user));
>-                user_update_from_pwent (user, pwent, spent);
>+                        /* freeze & update users not already in the new list */
>+                        g_object_freeze_notify (G_OBJECT (user));
>+                        user_update_from_pwent (user, pwent, spent);
> 
>-                g_hash_table_insert (users, g_strdup (user_get_user_name (user)), user);
>-                g_debug ("loaded user: %s", user_get_user_name (user));
>+                        g_hash_table_insert (users, g_strdup (user_get_user_name (user)), user);
>+                        g_debug ("loaded user: %s", user_get_user_name (user));
>+                }
>+
>+                if (!explicitly_requested) {
>+                        user_set_cached (user, TRUE);
>+                }
>         }
> 
>         /* Generator should have cleaned up */
>         g_assert (generator_state == NULL);
> }
> 
> static GHashTable *
> create_users_hash_table (void)
> {
>         return g_hash_table_new_full (g_str_hash,
>                                       g_str_equal,
>                                       g_free,
>                                       g_object_unref);
> }
> 
> static void
> reload_users (Daemon *daemon)
> {
>         GHashTable *users;
>         GHashTable *old_users;
>         GHashTable *local;
>         GHashTableIter iter;
>         gpointer name;
>         User *user;
> 
>         /* Track the users that we saw during our (re)load */
>         users = create_users_hash_table ();
> 
>         /*
>          * NOTE: As we load data from all the sources, notifies are
>@@ -432,71 +440,79 @@ reload_users (Daemon *daemon)
> 
>         /* Load the local users into our hash table */
>         load_entries (daemon, users, FALSE, entry_generator_fgetpwent);
>         local = g_hash_table_new (g_str_hash, g_str_equal);
>         g_hash_table_iter_init (&iter, users);
>         while (g_hash_table_iter_next (&iter, &name, NULL))
>                 g_hash_table_add (local, name);
> 
>         /* and add users to hash table that were explicitly requested  */
>         load_entries (daemon, users, TRUE, entry_generator_requested_users);
> 
>         /* Now add/update users from other sources, possibly non-local */
>         load_entries (daemon, users, FALSE, entry_generator_cachedir);
> 
>         wtmp_helper_update_login_frequencies (users);
> 
>         /* Mark which users are local, which are not */
>         g_hash_table_iter_init (&iter, users);
>         while (g_hash_table_iter_next (&iter, &name, (gpointer *)&user))
>                 user_update_local_account_property (user, g_hash_table_lookup (local, name) != NULL);
> 
>         g_hash_table_destroy (local);
> 
>         /* Swap out the users */
>         old_users = daemon->priv->users;
>         daemon->priv->users = users;
> 
>         /* Remove all the old users */
>         g_hash_table_iter_init (&iter, old_users);
>         while (g_hash_table_iter_next (&iter, &name, (gpointer *)&user)) {
>-                if (!g_hash_table_lookup (users, name)) {
>+                User *refreshed_user;
>+
>+                refreshed_user = g_hash_table_lookup (users, name);
>+
>+                if (!refreshed_user || !user_get_cached (refreshed_user)) {
>                         user_unregister (user);
>                         accounts_accounts_emit_user_deleted (ACCOUNTS_ACCOUNTS (daemon),
>                                                              user_get_object_path (user));
>                 }
>         }
> 
>         /* Register all the new users */
>         g_hash_table_iter_init (&iter, users);
>         while (g_hash_table_iter_next (&iter, &name, (gpointer *)&user)) {
>-                if (!g_hash_table_lookup (old_users, name)) {
>+                User *stale_user;
>+
>+                stale_user = g_hash_table_lookup (old_users, name);
>+
>+                if (!stale_user || !user_get_cached (stale_user) && user_get_cached (user)) {
>                         user_register (user);
>                         accounts_accounts_emit_user_added (ACCOUNTS_ACCOUNTS (daemon),
>                                                            user_get_object_path (user));
>                 }
>                 g_object_thaw_notify (G_OBJECT (user));
>         }
> 
>         g_hash_table_destroy (old_users);
> }
> 
> static gboolean
> reload_users_timeout (Daemon *daemon)
> {
>         reload_users (daemon);
>         daemon->priv->reload_id = 0;
> 
>         return FALSE;
> }
> 
> static gboolean load_autologin (Daemon    *daemon,
>                                 gchar    **name,
>                                 gboolean  *enabled,
>                                 GError   **error);
> 
> static gboolean
> reload_autologin_timeout (Daemon *daemon)
> {
>         gboolean enabled;
>         gchar *name = NULL;
>         GError *error = NULL;
>@@ -911,60 +927,65 @@ list_user_data_new (Daemon                *daemon,
> static void
> list_user_data_free (ListUserData *data)
> {
>         g_object_unref (data->daemon);
>         g_free (data);
> }
> 
> static gboolean
> finish_list_cached_users (gpointer user_data)
> {
>         ListUserData *data = user_data;
>         GPtrArray *object_paths;
>         GHashTableIter iter;
>         const gchar *name;
>         User *user;
>         uid_t uid;
>         const gchar *shell;
> 
>         object_paths = g_ptr_array_new ();
> 
>         g_hash_table_iter_init (&iter, data->daemon->priv->users);
>         while (g_hash_table_iter_next (&iter, (gpointer *)&name, (gpointer *)&user)) {
>                 uid = user_get_uid (user);
>                 shell = user_get_shell (user);
> 
>                 if (!user_classify_is_human (uid, name, shell, NULL)) {
>                         g_debug ("user %s %ld excluded", name, (long) uid);
>                         continue;
>                 }
> 
>+                if (!user_get_cached (user)) {
>+                        g_debug ("user %s %ld not cached", name, (long) uid);
>+                        continue;
>+                }
>+
>                 g_debug ("user %s %ld not excluded", name, (long) uid);
>                 g_ptr_array_add (object_paths, (gpointer) user_get_object_path (user));
>         }
>         g_ptr_array_add (object_paths, NULL);
> 
>         accounts_accounts_complete_list_cached_users (NULL, data->context, (const gchar * const *) object_paths->pdata);
> 
>         g_ptr_array_free (object_paths, TRUE);
> 
>         list_user_data_free (data);
> 
>         return FALSE;
> }
> 
> static gboolean
> daemon_list_cached_users (AccountsAccounts      *accounts,
>                           GDBusMethodInvocation *context)
> {
>         Daemon *daemon = (Daemon*)accounts;
>         ListUserData *data;
> 
>         data = list_user_data_new (daemon, context);
> 
>         if (daemon->priv->reload_id > 0) {
>                 /* reload in progress, wait a bit */
>                 g_idle_add (finish_list_cached_users, data);
>         }
>         else {
>                 finish_list_cached_users (data);
>         }
>@@ -1151,123 +1172,123 @@ daemon_cache_user (AccountsAccounts      *accounts,
> static void
> daemon_uncache_user_authorized_cb (Daemon                *daemon,
>                                    User                  *dummy,
>                                    GDBusMethodInvocation *context,
>                                    gpointer               data)
> {
>         const gchar *user_name = data;
>         gchar       *filename;
>         User        *user;
> 
>         sys_log (context, "uncache user '%s'", user_name);
> 
>         user = daemon_local_find_user_by_name (daemon, user_name);
>         if (user == NULL) {
>                 throw_error (context, ERROR_USER_DOES_NOT_EXIST,
>                              "No user with the name %s found", user_name);
>                 return;
>         }
> 
>         /* Always use the canonical user name looked up */
>         user_name = user_get_user_name (user);
> 
>         filename = g_build_filename (USERDIR, user_name, NULL);
>         g_remove (filename);
>         g_free (filename);
> 
>         filename = g_build_filename (ICONDIR, user_name, NULL);
>         g_remove (filename);
>         g_free (filename);
> 
>+        user_set_cached (user, FALSE);
>+
>         accounts_accounts_complete_uncache_user (NULL, context);
> 
>         queue_reload_users (daemon);
> }
> 
> static gboolean
> daemon_uncache_user (AccountsAccounts      *accounts,
>                      GDBusMethodInvocation *context,
>                      const gchar           *user_name)
> {
>         Daemon *daemon = (Daemon*)accounts;
> 
>         daemon_local_check_auth (daemon,
>                                  NULL,
>                                  "org.freedesktop.accounts.user-administration",
>                                  TRUE,
>                                  daemon_uncache_user_authorized_cb,
>                                  context,
>                                  g_strdup (user_name),
>                                  g_free);
> 
>         return TRUE;
> }
> 
> typedef struct {
>         uid_t uid;
>         gboolean remove_files;
> } DeleteUserData;
> 
> static void
> daemon_delete_user_authorized_cb (Daemon                *daemon,
>                                   User                  *dummy,
>                                   GDBusMethodInvocation *context,
>                                   gpointer               data)
> 
> {
>         DeleteUserData *ud = data;
>         GError *error;
>         gchar *filename;
>         struct passwd *pwent;
>         const gchar *argv[6];
>+        User *user;
> 
>         pwent = getpwuid (ud->uid);
> 
>         if (pwent == NULL) {
>                 throw_error (context, ERROR_USER_DOES_NOT_EXIST, "No user with uid %d found", ud->uid);
> 
>                 return;
>         }
> 
>         sys_log (context, "delete user '%s' (%d)", pwent->pw_name, ud->uid);
> 
>-        if (daemon->priv->autologin != NULL) {
>-                User *user;
>+        user = daemon_local_find_user_by_id (daemon, ud->uid);
> 
>-                user = daemon_local_find_user_by_id (daemon, ud->uid);
>+        if (user != NULL) {
>+                user_set_cached (user, FALSE);
> 
>-                g_assert (user != NULL);
>-
>-                if (daemon->priv->autologin == user) {
>+                if (daemon->priv->autologin  == user) {
>                         daemon_local_set_automatic_login (daemon, user, FALSE, NULL);
>                 }
>-
>         }
> 
>         filename = g_build_filename (USERDIR, pwent->pw_name, NULL);
>         g_remove (filename);
>         g_free (filename);
> 
>         filename = g_build_filename (ICONDIR, pwent->pw_name, NULL);
>         g_remove (filename);
>         g_free (filename);
> 
>         argv[0] = "/usr/sbin/userdel";
>         if (ud->remove_files) {
>                 argv[1] = "-f";
>                 argv[2] = "-r";
>                 argv[3] = "--";
>                 argv[4] = pwent->pw_name;
>                 argv[5] = NULL;
>         }
>         else {
>                 argv[1] = "-f";
>                 argv[2] = "--";
>                 argv[3] = pwent->pw_name;
>                 argv[4] = NULL;
>         }
> 
>         error = NULL;
>         if (!spawn_with_login_uid (context, argv, &error)) {
>                 throw_error (context, ERROR_FAILED, "running '%s' failed: %s", argv[0], error->message);
>                 g_error_free (error);
>                 return;
>diff --git a/src/user.c b/src/user.c
>index 802d07a..a83cfe4 100644
>--- a/src/user.c
>+++ b/src/user.c
>@@ -83,60 +83,61 @@ struct User {
>         GKeyFile     *keyfile;
> 
>         uid_t         uid;
>         gid_t         gid;
>         gchar        *user_name;
>         gchar        *real_name;
>         AccountType   account_type;
>         PasswordMode  password_mode;
>         gchar        *password_hint;
>         gchar        *home_dir;
>         gchar        *shell;
>         gchar        *email;
>         gchar        *language;
>         gchar        *x_session;
>         gchar        *location;
>         guint64       login_frequency;
>         gint64        login_time;
>         gint64        expiration_time;
>         gint64        last_change_time;
>         gint64        min_days_between_changes;
>         gint64        max_days_between_changes;
>         gint64        days_to_warn;
>         gint64        days_after_expiration_until_lock;
>         GVariant     *login_history;
>         gchar        *icon_file;
>         gchar        *default_icon_file;
>         gboolean      locked;
>         gboolean      automatic_login;
>         gboolean      system_account;
>         gboolean      local_account;
>+        gboolean      cached;
> 
>         guint        *extension_ids;
>         guint         n_extension_ids;
> };
> 
> typedef struct UserClass
> {
>         AccountsUserSkeletonClass parent_class;
> } UserClass;
> 
> static void user_accounts_user_iface_init (AccountsUserIface *iface);
> 
> G_DEFINE_TYPE_WITH_CODE (User, user, ACCOUNTS_TYPE_USER_SKELETON, G_IMPLEMENT_INTERFACE (ACCOUNTS_TYPE_USER, user_accounts_user_iface_init));
> 
> static gint
> account_type_from_pwent (struct passwd *pwent)
> {
>         struct group *grp;
>         gint i;
> 
>         if (pwent->pw_uid == 0) {
>                 g_debug ("user is root so account type is administrator");
>                 return ACCOUNT_TYPE_ADMINISTRATOR;
>         }
> 
>         grp = getgrnam (ADMIN_GROUP);
>         if (grp == NULL) {
>                 g_debug (ADMIN_GROUP " group not found");
>                 return ACCOUNT_TYPE_STANDARD;
>         }
>@@ -339,109 +340,112 @@ user_update_from_keyfile (User     *user,
>                 user->location = s;
>                 g_object_notify (G_OBJECT (user), "location");
>         }
> 
>         s = g_key_file_get_string (keyfile, "User", "PasswordHint", NULL);
>         if (s != NULL) {
>                 g_free (user->password_hint);
>                 user->password_hint = s;
>                 g_object_notify (G_OBJECT (user), "password-hint");
>         }
> 
>         s = g_key_file_get_string (keyfile, "User", "Icon", NULL);
>         if (s != NULL) {
>                 g_free (user->icon_file);
>                 user->icon_file = s;
>                 g_object_notify (G_OBJECT (user), "icon-file");
>         }
> 
>         if (g_key_file_has_key (keyfile, "User", "SystemAccount", NULL)) {
>             gboolean system_account;
> 
>             system_account = g_key_file_get_boolean (keyfile, "User", "SystemAccount", NULL);
>             if (system_account != user->system_account) {
>                     user->system_account = system_account;
>                     g_object_notify (G_OBJECT (user), "system-account");
>             }
>         }
> 
>         g_clear_pointer (&user->keyfile, g_key_file_unref);
>         user->keyfile = g_key_file_ref (keyfile);
>+        user_set_cached (user, TRUE);
> 
>         g_object_thaw_notify (G_OBJECT (user));
> }
> 
> void
> user_update_local_account_property (User          *user,
>                                     gboolean       local)
> {
>         if (local == user->local_account)
>                 return;
>         user->local_account = local;
>         g_object_notify (G_OBJECT (user), "local-account");
> }
> 
> void
> user_update_system_account_property (User          *user,
>                                      gboolean       system)
> {
>         if (system == user->system_account)
>                 return;
>         user->system_account = system;
>         g_object_notify (G_OBJECT (user), "system-account");
> }
> 
> static void
> user_save_to_keyfile (User     *user,
>                       GKeyFile *keyfile)
> {
>         g_key_file_remove_group (keyfile, "User", NULL);
> 
>         if (user->email)
>                 g_key_file_set_string (keyfile, "User", "Email", user->email);
> 
>         if (user->language)
>                 g_key_file_set_string (keyfile, "User", "Language", user->language);
> 
>         if (user->x_session)
>                 g_key_file_set_string (keyfile, "User", "XSession", user->x_session);
> 
>         if (user->location)
>                 g_key_file_set_string (keyfile, "User", "Location", user->location);
> 
>         if (user->password_hint)
>                 g_key_file_set_string (keyfile, "User", "PasswordHint", user->password_hint);
> 
>         if (user->icon_file)
>                 g_key_file_set_string (keyfile, "User", "Icon", user->icon_file);
> 
>         g_key_file_set_boolean (keyfile, "User", "SystemAccount", user->system_account);
>+
>+        user_set_cached (user, TRUE);
> }
> 
> static void
> save_extra_data (User *user)
> {
>         gchar *filename;
>         gchar *data;
>         GError *error;
> 
>         user_save_to_keyfile (user, user->keyfile);
> 
>         error = NULL;
>         data = g_key_file_to_data (user->keyfile, NULL, &error);
>         if (error == NULL) {
>                 filename = g_build_filename (USERDIR,
>                                              user->user_name,
>                                              NULL);
>                 g_file_set_contents (filename, data, -1, &error);
>                 g_free (filename);
>                 g_free (data);
>         }
>         if (error) {
>                 g_warning ("Saving data for user %s failed: %s",
>                            user->user_name, error->message);
>                 g_error_free (error);
>         }
> }
> 
> static void
> move_extra_data (const gchar *old_name,
>@@ -810,60 +814,73 @@ user_get_user_name (User *user)
> gboolean
> user_get_system_account (User *user)
> {
>         return user->system_account;
> }
> 
> gboolean
> user_get_local_account (User *user)
> {
>         return user->local_account;
> }
> 
> const gchar *
> user_get_object_path (User *user)
> {
>         return user->object_path;
> }
> 
> uid_t
> user_get_uid (User *user)
> {
>         return user->uid;
> }
> 
> const gchar *
> user_get_shell(User *user)
> {
> 	return user->shell;
> }
> 
>+gboolean
>+user_get_cached (User *user)
>+{
>+        return user->cached;
>+}
>+
>+void
>+user_set_cached (User     *user,
>+                 gboolean  cached)
>+{
>+        user->cached = cached;
>+}
>+
> static void
> throw_error (GDBusMethodInvocation *context,
>              gint                   error_code,
>              const gchar           *format,
>              ...)
> {
>         va_list args;
>         gchar *message;
> 
>         va_start (args, format);
>         message = g_strdup_vprintf (format, args);
>         va_end (args);
> 
>         g_dbus_method_invocation_return_error (context, ERROR, error_code, "%s", message);
> 
>         g_free (message);
> }
> 
> static void
> user_change_real_name_authorized_cb (Daemon                *daemon,
>                                      User                  *user,
>                                      GDBusMethodInvocation *context,
>                                      gpointer               data)
> 
> {
>         gchar *name = data;
>         GError *error;
>         const gchar *argv[6];
> 
>         if (g_strcmp0 (user->real_name, name) != 0) {
>diff --git a/src/user.h b/src/user.h
>index 22548f9..39c6f13 100644
>--- a/src/user.h
>+++ b/src/user.h
>@@ -36,47 +36,50 @@ G_BEGIN_DECLS
> #define IS_USER(object) (G_TYPE_CHECK_INSTANCE_TYPE ((object), TYPE_USER))
> 
> typedef enum {
>         ACCOUNT_TYPE_STANDARD,
>         ACCOUNT_TYPE_ADMINISTRATOR,
> #define ACCOUNT_TYPE_LAST ACCOUNT_TYPE_ADMINISTRATOR
> } AccountType;
> 
> typedef enum {
>         PASSWORD_MODE_REGULAR,
>         PASSWORD_MODE_SET_AT_LOGIN,
>         PASSWORD_MODE_NONE,
> #define PASSWORD_MODE_LAST PASSWORD_MODE_NONE
> } PasswordMode;
> 
> /* local methods */
> 
> GType          user_get_type                (void) G_GNUC_CONST;
> User *         user_new                     (Daemon        *daemon,
>                                              uid_t          uid);
> 
> void           user_update_from_pwent       (User          *user,
>                                              struct passwd *pwent,
>                                              struct spwd   *spent);
> void           user_update_from_keyfile     (User          *user,
>                                              GKeyFile      *keyfile);
> void           user_update_local_account_property (User          *user,
>                                                    gboolean       local);
> void           user_update_system_account_property (User          *user,
>                                                     gboolean       system);
>+gboolean       user_get_cached              (User          *user);
>+void           user_set_cached              (User          *user,
>+                                             gboolean       cached);
> 
> void           user_register                (User          *user);
> void           user_unregister              (User          *user);
> void           user_changed                 (User          *user);
> 
> void           user_save                    (User          *user);
> 
> const gchar *  user_get_user_name           (User          *user);
> gboolean       user_get_system_account      (User          *user);
> gboolean       user_get_local_account       (User          *user);
> const gchar *  user_get_object_path         (User          *user);
> uid_t          user_get_uid                 (User          *user);
> const gchar *  user_get_shell               (User          *user);
> 
> G_END_DECLS
> 
> #endif
>-- 
>2.12.2

Actions: View | Splinter Review

Attachments on bug 101052: 131370

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.