Merge lp:~gagern/lightdm/preferred-users into lp:lightdm

Proposed by Martin von Gagern on 2014-04-30
Status: Needs review
Proposed branch: lp:~gagern/lightdm/preferred-users
Merge into: lp:lightdm
Diff against target: 73 lines (+31/-1)
2 files modified
common/user-list.c (+29/-1)
data/users.conf (+2/-0)
To merge this branch: bzr merge lp:~gagern/lightdm/preferred-users
Reviewer Review Type Date Requested Status
Robert Ancell 2014-04-30 Needs Information on 2014-05-08
Review via email: mp+217720@code.launchpad.net

Description of the change

In the absence of AccountsService, this patch allows for a preferred-users setting in the users.conf file which will result in the named users getting sorted to the top of the list, in the order in which they have been specified. This is particularly important if the greeter presents the first user as the default selection, like the gtk greeter does. That way, the person owning the computer can log simply by typing his password, even if there are other accounts on the system.

To post a comment you must log in.
Robert Ancell (robert-ancell) wrote :

I've been mulling this over and I need some convincing this feature is important.

You mention the case where the GTK+ greeter always selects the first user, not the most common user account. Isn't this just a bug in the GTK+ greeter? Would it selecting the last logged in user solve this?

For this to be in LightDM the feature should be useful for all greeters. Given the above statement, I'm not sure why other greeters might want this feature.

If this feature did exist, it should apply to the ordering of the user list even if it's from AccountsService otherwise the behaviour is inconsistent.

There are some bugs:
- You read the config from [UserList] but data/users.conf says it is in [UserAccounts]
- preferred_users should be a field in CommonUserListPrivate, not a static variable. At the moment you overwrite it each time load_passwd_file is called which happens each time /etc/passwd changes and also you may have more than one instance of CommonUserList.

review: Needs Information
Martin von Gagern (gagern) wrote :

Sorry that it took me so long to reply to this.

I patched my lightdm at the same time that I also disabled AccountsService support. It seemed to work at the time, but after your comment I found out that this was more of a coincidence.

It turns out that the GTK+ greeter will indeed store the last selected user, but it gives higher priority to lightdm_greeter_get_select_user_hint. It seems that at least on my system, AccountService sets these hints in a way that doesn't save the most recently used account name.
http://bazaar.launchpad.net/~lightdm-gtk-greeter-team/lightdm-gtk-greeter/trunk/view/285/src/lightdm-gtk-greeter.c#L1841

You are right that there is a discrepancy between [UserList] and [UserAccounts] as the title of the config file section. However, this is the result os some copy and paste, which means that existing code is affected by this discrepancy as well. So I guess you should fix this for the existing settings, although I'm not sure whether to better fix this by changing the program code or the example config.

I would very much welcome consistent behaviour for the case with and without AccountsService. But again this does affect existing settings as well. I see no reason against being able to manually hide users even if AccountsServices is in use.

As it stands, my patch is certainly not suited for merging. I'll try to address the issues you mentioned when I find the time. Before I'd do so, I'd welcome input whether you want the config section to be called [UserList] or [UserAccounts]. I'd also try to understand how these hints work, how AccountsService affects them, and whether it would make sense being able to override some of them.

Robert Ancell (robert-ancell) wrote :

You were right to use [UserList] - that is the correct name. It was pointed out in bug 1069218 that the example configuration had it wrong.

Unmerged revisions

1821. By Martin von Gagern on 2014-04-30

Document preferred-users setting in users.conf.

1820. By Martin von Gagern on 2014-04-30

Merge the rename from LightDMUser to CommonUser.

1819. By Martin von Gagern on 2014-04-30

Allow for a preferred-users setting to affect user list ordering.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'common/user-list.c'
--- common/user-list.c 2014-03-06 02:40:18 +0000
+++ common/user-list.c 2014-04-30 07:41:38 +0000
@@ -222,11 +222,31 @@
222222
223 return NULL;223 return NULL;
224}224}
225 225
226static gchar **preferred_users = NULL;
227
228static int
229prefer_user (CommonUser *user)
230{
231 int i;
232 const gchar *name;
233
234 if (preferred_users == NULL)
235 return 0;
236 name = common_user_get_name (user);
237 for (i = 0; preferred_users[i]; i++)
238 if (strcmp (name, preferred_users[i]) == 0)
239 return i;
240 return i;
241}
242
226static gint243static gint
227compare_user (gconstpointer a, gconstpointer b)244compare_user (gconstpointer a, gconstpointer b)
228{245{
229 CommonUser *user_a = (CommonUser *) a, *user_b = (CommonUser *) b;246 CommonUser *user_a = (CommonUser *) a, *user_b = (CommonUser *) b;
247 int prefer_a = prefer_user (user_a), prefer_b = prefer_user (user_b);
248 if (prefer_a != prefer_b)
249 return prefer_a - prefer_b;
230 return g_strcmp0 (common_user_get_display_name (user_a), common_user_get_display_name (user_b));250 return g_strcmp0 (common_user_get_display_name (user_a), common_user_get_display_name (user_b));
231}251}
232252
@@ -335,6 +355,12 @@
335 hidden_shells = g_strsplit (value, " ", -1);355 hidden_shells = g_strsplit (value, " ", -1);
336 g_free (value);356 g_free (value);
337357
358 value = g_key_file_get_string (config, "UserList", "preferred-users", NULL);
359 if (!value)
360 value = g_strdup ("");
361 preferred_users = g_strsplit (value, " ", -1);
362 g_free (value);
363
338 g_key_file_free (config);364 g_key_file_free (config);
339365
340 setpwent ();366 setpwent ();
@@ -392,6 +418,8 @@
392 }418 }
393 g_strfreev (hidden_users);419 g_strfreev (hidden_users);
394 g_strfreev (hidden_shells);420 g_strfreev (hidden_shells);
421 g_strfreev (preferred_users);
422 preferred_users = NULL;
395423
396 if (errno != 0)424 if (errno != 0)
397 g_warning ("Failed to read password database: %s", strerror (errno));425 g_warning ("Failed to read password database: %s", strerror (errno));
398426
=== modified file 'data/users.conf'
--- data/users.conf 2011-10-18 02:41:06 +0000
+++ data/users.conf 2014-04-30 07:41:38 +0000
@@ -7,8 +7,10 @@
7# minimum-uid = Minimum UID required to be shown in greeter7# minimum-uid = Minimum UID required to be shown in greeter
8# hidden-users = Users that are not shown to the user8# hidden-users = Users that are not shown to the user
9# hidden-shells = Shells that indicate a user cannot login9# hidden-shells = Shells that indicate a user cannot login
10# preferred-users = Users to be sorted to the top of the list
10#11#
11[UserAccounts]12[UserAccounts]
12minimum-uid=50013minimum-uid=500
13hidden-users=nobody nobody4 noaccess14hidden-users=nobody nobody4 noaccess
14hidden-shells=/bin/false /usr/sbin/nologin15hidden-shells=/bin/false /usr/sbin/nologin
16preferred-users=

Subscribers

People subscribed via source and target branches