Merge lp:~mterry/lightdm/no-dmrc into lp:lightdm

[Michael Terry]

This uses the org.freedesktop.Accounts service instead of dmrc for saving default xsession information.

I could not find where LightDM was loading per-user default xsession information? Seems like it isn't.

If true, we should add that to the liblightdm libraries as a call like get_default_session_hint_for_user and have the lightdm daemon ask accountsservice.

[Robert Ancell (robert-ancell)]

I think we need to continue to support .dmrc for systems that don't have accounts service:
http://lists.freedesktop.org/archives/lightdm/2011-August/000051.html

What we should do is merge user.c into the new accounts.c so that internally LightDM uses the accounts service interface but the backend can fallback to passwd and .dmrc.

Also since it doesn't make sense to have multiple accounts instances inside LightDM I'd make it a singleton and just access through functions for simplicity.

In terms of Unity/GNOME, I definitely think we should be using accounts service.

review: Needs Fixing

[Michael Terry (mterry)]

Each Accounts object is per-user, not global. So a singleton doesn't make as much sense. Unless you want a global Accounts object that hides the per-user stuff and keeps an internal cache of proxies. But that seems like forcing it.

[lp:~mterry/lightdm/no-dmrc]

1057. By Michael Terry on 2011-08-18

add back dmrc support as a fallback for accounts service

1058. By Michael Terry on 2011-08-18

move Accounts class into User class

[Michael Terry (mterry)]

OK, I've updated the branch. Now we write to .dmrc as well as the Accounts service. And we fallback to .dmrc when trying to read from the Accounts service.

Additionally, I've moved the code into the User class.

Could you review again, please?

(Again, this branch writes correctly to Accounts/dmrc, but still doesn't solve the problem that we never load per-user values from them.)

[lp:~mterry/lightdm/no-dmrc]

1059. By Michael Terry on 2011-08-22

liblightdm-gobject: look at Accounts object when checking per-user session and language values

[Michael Terry (mterry)]

Sorry, I was wrong about never loading per-user values from them. I missed the glue in liblightdm-gobject/user.c that did that.

So I've now updated this branch to modify that file to also look at the Accounts service for session info. This fixes bug 818201.

[Robert Ancell (robert-ancell)]

Pushed with some minor changes:
- Missing 'return TRUE' at the end of load_accounts_service ()
- Replace '//' comments with '/*' ones
- Add NEWS entry

review: Approve

=== modified file 'liblightdm-gobject/user.c'
--- liblightdm-gobject/user.c	2011-08-17 00:26:17 +0000
+++ liblightdm-gobject/user.c	2011-08-22 20:39:30 +0000
@@ -1095,11 +1095,82 @@
 
     // FIXME: Watch for changes
 
+    if (priv->language)
+        g_free (priv->language);
+    if (priv->layout)
+        g_free (priv->layout);
+    if (priv->session)
+        g_free (priv->session);
+
     priv->language = g_key_file_get_string (priv->dmrc_file, "Desktop", "Language", NULL);
     priv->layout = g_key_file_get_string (priv->dmrc_file, "Desktop", "Layout", NULL);
     priv->session = g_key_file_get_string (priv->dmrc_file, "Desktop", "Session", NULL);
 }
 
+static gchar *
+get_string_property (GDBusProxy *proxy, const gchar *property)
+{
+    GVariant *answer;
+    gchar *rv;
+
+    if (!proxy)
+        return NULL;
+
+    answer = g_dbus_proxy_get_cached_property (proxy, property);
+
+    if (!answer) {
+        g_warning ("Could not get accounts property %s", property);
+        return NULL;
+    }
+
+    if (!g_variant_is_of_type (answer, G_VARIANT_TYPE ("s"))) {
+        g_warning ("Unexpected accounts property type for %s: %s",
+                   property, g_variant_get_type_string (answer));
+        g_variant_unref (answer);
+        return NULL;
+    }
+
+    g_variant_get (answer, "s", &rv);
+
+    g_variant_unref (answer);
+    return rv;
+}
+
+static gboolean
+load_accounts_service (LightDMUser *user)
+{
+    // First, find AccountObject proxy
+    LightDMUserPrivate *priv = GET_USER_PRIVATE (user);
+    LightDMUserListPrivate *list_priv = GET_LIST_PRIVATE (priv->user_list);
+
+    UserAccountObject *account = NULL;
+    GList *iter;
+    for (iter = list_priv->user_account_objects; iter; iter = iter->next) {
+        if (((UserAccountObject *)iter->data)->user == user) {
+            account = (UserAccountObject *)iter->data;
+            break;
+        }
+    }
+    if (!account)
+        return FALSE;
+
+    // We have proxy, let's grab some properties
+    if (priv->language)
+        g_free (priv->language);
+    if (priv->session)
+        g_free (priv->session);
+    priv->language = get_string_property (account->proxy, "Language");
+    priv->session = get_string_property (account->proxy, "XSession");
+}
+
+/* Loads language/layout/session info for user */
+static void
+load_user_values (LightDMUser *user)
+{
+    load_dmrc (user);
+    load_accounts_service (user); // overrides dmrc values
+}
+
 /**
  * lightdm_user_get_language
  * @user: A #LightDMUser
@@ -1112,7 +1183,7 @@
 lightdm_user_get_language (LightDMUser *user)
 {
     g_return_val_if_fail (LIGHTDM_IS_USER (user), NULL);
-    load_dmrc (user);
+    load_user_values (user);
     return GET_USER_PRIVATE (user)->language;
 }
 
@@ -1128,7 +1199,7 @@
 lightdm_user_get_layout (LightDMUser *user)
 {
     g_return_val_if_fail (LIGHTDM_IS_USER (user), NULL);
-    load_dmrc (user);
+    load_user_values (user);
     return GET_USER_PRIVATE (user)->layout;
 }
 
@@ -1144,7 +1215,7 @@
 lightdm_user_get_session (LightDMUser *user)
 {
     g_return_val_if_fail (LIGHTDM_IS_USER (user), NULL);
-    load_dmrc (user);
+    load_user_values (user);
     return GET_USER_PRIVATE (user)->session; 
 }
 
 
=== modified file 'src/display.c'
--- src/display.c	2011-08-18 00:11:39 +0000
+++ src/display.c	2011-08-22 20:39:30 +0000
@@ -1,4 +1,5 @@
-/*
+/* -*- Mode: C; indent-tabs-mode: nil; tab-width: 4 -*-
+ *
  * Copyright (C) 2010-2011 Robert Ancell.
  * Author: Robert Ancell <[e-mail]>
  *
@@ -18,7 +19,6 @@
 #include "configuration.h"
 #include "user.h"
 #include "pam-session.h"
-#include "dmrc.h"
 #include "ldm-marshal.h"
 #include "greeter.h"
 #include "xserver-local.h" // FIXME: Shouldn't know if it's an xserver
@@ -771,7 +771,6 @@
 start_user_session (Display *display, PAMSession *authentication)
 {
     User *user;
-    GKeyFile *dmrc_file;
     gchar *log_filename;
     gboolean result = FALSE;
 
@@ -779,13 +778,8 @@
 
     user = pam_session_get_user (authentication);
 
-    /* Load the users login settings (~/.dmrc) */
-    dmrc_file = dmrc_load (user_get_name (user));
-
-    /* Update the .dmrc with changed settings */
-    g_key_file_set_string (dmrc_file, "Desktop", "Session", display->priv->user_session);
-    dmrc_save (dmrc_file, user_get_name (user));
-    g_key_file_free (dmrc_file);
+    /* Update user's xsession setting */
+    user_set_session (user, display->priv->user_session);
 
     // FIXME: Copy old error file
     log_filename = g_build_filename (user_get_home_directory (user), ".xsession-errors", NULL);
 
=== modified file 'src/user.c'
--- src/user.c	2011-08-06 15:00:26 +0000
+++ src/user.c	2011-08-22 20:39:30 +0000
@@ -1,4 +1,5 @@
-/*
+/* -*- Mode: C; indent-tabs-mode: nil; tab-width: 4 -*-
+ *
  * Copyright (C) 2010-2011 Robert Ancell.
  * Author: Robert Ancell <[e-mail]>
  * 
@@ -15,12 +16,16 @@
 #include <string.h>
 
 #include "user.h"
+#include "dmrc.h"
 
 struct UserPrivate
 {
     /* Name of user */
     gchar *name;
 
+    /* Accounts interface proxy */
+    GDBusProxy *proxy;
+
     /* User ID */
     uid_t uid;
 
@@ -41,6 +46,157 @@
 
 static gchar *passwd_file = NULL;
 
+static gboolean
+call_method (GDBusProxy *proxy, const gchar *method, GVariant *args,
+             const gchar *expected, GVariant **result)
+{
+    GVariant *answer;
+    GError *error = NULL;
+
+    if (!proxy)
+        return FALSE;
+
+    answer = g_dbus_proxy_call_sync (proxy,
+                                     method,
+                                     args,
+                                     G_DBUS_CALL_FLAGS_NONE,
+                                     -1,
+                                     NULL,
+                                     &error);
+
+    if (!answer) {
+        g_warning ("Could not call %s: %s", method, error->message);
+        g_error_free (error);
+        return FALSE;
+    }
+
+    if (!g_variant_is_of_type (answer, G_VARIANT_TYPE (expected))) {
+        g_warning ("Unexpected response from %s: %s",
+                   method, g_variant_get_type_string (answer));
+        g_variant_unref (answer);
+        return FALSE;
+    }
+
+    if (result)
+        *result = answer;
+    else
+        g_variant_unref (answer);
+    return TRUE;
+}
+
+static gboolean
+get_property (GDBusProxy *proxy, const gchar *property,
+              const gchar *expected, GVariant **result)
+{
+    GVariant *answer;
+
+    if (!proxy)
+        return FALSE;
+
+    answer = g_dbus_proxy_get_cached_property (proxy, property);
+
+    if (!answer) {
+        g_warning ("Could not get accounts property %s", property);
+        return FALSE;
+    }
+
+    if (!g_variant_is_of_type (answer, G_VARIANT_TYPE (expected))) {
+        g_warning ("Unexpected accounts property type for %s: %s",
+                   property, g_variant_get_type_string (answer));
+        g_variant_unref (answer);
+        return FALSE;
+    }
+
+    if (result)
+        *result = answer;
+    else
+        g_variant_unref (answer);
+    return TRUE;
+}
+
+static void
+save_string_to_dmrc (const gchar *username, const gchar *group,
+                     const gchar *key, const gchar *value)
+{
+    GKeyFile *dmrc;
+
+    dmrc = dmrc_load (username);
+    g_key_file_set_string (dmrc, group, key, value);
+    dmrc_save (dmrc, username);
+
+    g_key_file_free (dmrc);
+}
+
+static gchar *
+get_string_from_dmrc (const gchar *username, const gchar *group,
+                      const gchar *key)
+{
+    GKeyFile *dmrc;
+    gchar *value;
+
+    dmrc = dmrc_load (username);
+    value = g_key_file_get_string (dmrc, group, key, NULL);
+
+    g_key_file_free (dmrc);
+    return value;
+}
+
+static GDBusProxy *
+get_accounts_proxy_for_user (const gchar *user)
+{
+    g_return_val_if_fail (user != NULL, NULL);
+
+    GDBusProxy *proxy;
+    GError *error = NULL;
+    GVariant *result;
+    gboolean success;
+    gchar *user_path = NULL;
+
+    proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM,
+                                           G_DBUS_PROXY_FLAGS_NONE,
+                                           NULL,
+                                           "org.freedesktop.Accounts",
+                                           "/org/freedesktop/Accounts",
+                                           "org.freedesktop.Accounts",
+                                           NULL, &error);
+
+    if (!proxy) {
+        g_warning ("Could not get accounts proxy: %s", error->message);
+        g_error_free (error);
+        return NULL;
+    }
+
+    success = call_method (proxy, "FindUserByName", g_variant_new ("(s)", user),
+                           "(o)", &result);
+    g_object_unref (proxy);
+
+    if (!success)
+        return NULL;
+
+    g_variant_get (result, "(o)", &user_path);
+    g_variant_unref (result);
+
+    if (!user_path)
+        return NULL;
+
+    proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM,
+                                           G_DBUS_PROXY_FLAGS_NONE,
+                                           NULL,
+                                           "org.freedesktop.Accounts",
+                                           user_path,
+                                           "org.freedesktop.Accounts.User",
+                                           NULL, &error);
+    g_free (user_path);
+
+    if (!proxy) {
+        g_warning ("Could not get accounts user proxy: %s", error->message);
+        g_error_free (error);
+        return NULL;
+    }
+
+    return proxy;
+}
+
 static User *
 user_from_passwd (struct passwd *user_info)
 {
@@ -53,6 +209,7 @@
     user->priv->gecos = g_strdup (user_info->pw_gecos);
     user->priv->home_directory = g_strdup (user_info->pw_dir);
     user->priv->shell = g_strdup (user_info->pw_shell);
+    user->priv->proxy = get_accounts_proxy_for_user (user->priv->name);
 
     return user;
 }
@@ -236,6 +393,40 @@
     return user->priv->shell;
 }
 
+void
+user_set_session (User *user, const gchar *session)
+{
+    g_return_if_fail (user != NULL);
+
+    call_method (user->priv->proxy, "SetXSession",
+                 g_variant_new ("(s)", session), "()", NULL);
+
+    save_string_to_dmrc (user->priv->name, "Desktop", "Session", session);
+}
+
+gchar *
+user_get_session (User *user)
+{
+    g_return_val_if_fail (user != NULL, NULL);
+
+    GVariant *result;
+    gchar *session;
+
+    if (!get_property (user->priv->proxy, "XSession",
+                       "s", &result))
+        return get_string_from_dmrc (user->priv->name, "Desktop", "Session");
+
+    g_variant_get (result, "s", &session);
+    g_variant_unref (result);
+
+    if (g_strcmp0 (session, "") == 0) {
+        g_free (session);
+        return NULL;
+    }
+
+    return session;
+}
+
 static void
 user_init (User *user)
 {
@@ -243,6 +434,21 @@
 }
 
 static void
+user_dispose (GObject *object)
+{
+    User *self;
+
+    self = USER (object);
+
+    if (self->priv->proxy) {
+        g_object_unref (self->priv->proxy);
+        self->priv->proxy = NULL;
+    }
+
+    G_OBJECT_CLASS (user_parent_class)->dispose (object);
+}
+
+static void
 user_finalize (GObject *object)
 {
     User *self;
@@ -262,6 +468,7 @@
 {
     GObjectClass *object_class = G_OBJECT_CLASS (klass);
 
+    object_class->dispose = user_dispose;
     object_class->finalize = user_finalize;  
 
     g_type_class_add_private (klass, sizeof (UserPrivate));
 
=== modified file 'src/user.h'
--- src/user.h	2011-06-27 11:17:39 +0000
+++ src/user.h	2011-08-22 20:39:30 +0000
@@ -1,4 +1,5 @@
-/*
+/* -*- Mode: C; indent-tabs-mode: nil; tab-width: 4 -*-
+ *
  * Copyright (C) 2010-2011 Robert Ancell.
  * Author: Robert Ancell <[e-mail]>
  * 
@@ -57,6 +58,10 @@
 
 const gchar *user_get_shell (User *user);
 
+gchar *user_get_session (User *user);
+
+void user_set_session (User *user, const gchar *session);
+
 G_END_DECLS
 
 #endif /* _USER_H_ */