After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 557553 - choose which users are visable
choose which users are visable
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
2.24.x
Other All
: Normal enhancement
: ---
Assigned To: Brian Cameron
GDM maintainers
: 568326 602641 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-10-23 06:46 UTC by Valent Turkovic
Modified: 2010-07-02 19:12 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
patch fixing issue (15.94 KB, patch)
2009-09-01 01:02 UTC, Brian Cameron
needs-work Details | Review
Docs for this change (2.73 KB, patch)
2009-09-01 01:02 UTC, Brian Cameron
needs-work Details | Review
updated patch (18.97 KB, patch)
2009-09-10 00:00 UTC, Brian Cameron
needs-work Details | Review
Updated patch (21.80 KB, patch)
2009-10-30 22:47 UTC, Brian Cameron
needs-work Details | Review
updated patch (22.38 KB, patch)
2009-11-09 21:32 UTC, Brian Cameron
accepted-commit_now Details | Review
updated patch which resolves Ray's concerns (22.00 KB, patch)
2009-11-17 00:01 UTC, Brian Cameron
none Details | Review

Description Valent Turkovic 2008-10-23 06:46:19 UTC
Hi,
I asked on gdm mailing list how to choose which users are visable and I got an answer that in versions after 2.20 there is no such option.

So please make this option available again.

Thank you.
Comment 1 Valent Turkovic 2008-10-23 13:31:00 UTC
just to add that gdm 2.20 and previous ones had option that could be set in /etc/gdm/custom.conf:

IncludeAll=false
Include=user1,user2,etc...

not that functionality is not there any more.
Comment 2 Ray Strode [halfline] 2008-10-23 14:02:16 UTC
So GDM gets its list of users dynamically based on two things:

1) users who log in frequently (see ConsoleKit's ck-history --frequent)
2) local users

Would having some way to disable 2) be sufficient for what you want?
Comment 3 Matthias Clasen 2008-10-23 14:56:20 UTC
It should be possible to bring back to old config option for doing this,
but can I ask first what your use case for this feature is ?
Why isn't the current dynamic/heuristic approach good enough ?
Comment 4 Pacho Ramos 2008-10-23 17:41:40 UTC
I am not the reporter but, in my case, I use this because one of the systems I administrate has a lot of users (22) that don't login never in graphical mode, while only 7 users use gdm for loggining in
Comment 5 Ray Strode [halfline] 2008-10-23 17:44:29 UTC
Pacho, what does ck-history --frequent say in your case?
Comment 6 Pacho Ramos 2008-10-23 18:01:18 UTC
It doesn't output anything, but. maybe, because I don't launch its service by default :-(
Comment 7 Patrik Martinsson 2008-10-31 07:26:50 UTC
Hi everybody, 

I sent out an mail on the mailing list about this and was directed to this report, anyhow, this is my suggestion regarding this subject. 

1. Users that logs in ONLY through gdm gets added to the "user list at gdm-login", NOT the users that logs in through ssh or something else. 

2. Those users that are visible in the "user list at gdm-login" should be handled in that way a sysadmin easily could remove himself ( if he for some reason logs in through the gdm ), or other users that for different reasons have been logging in through gdm. 

Thanks for a great job guys !

Best regards, 
Patrik Martinsson, Sweden. 

Comment 8 Valent Turkovic 2008-10-31 11:28:44 UTC
Hi, I reported this bug and the issue I reported is the same as in Comment #4.

Cheers.
Comment 9 Valent Turkovic 2008-10-31 11:38:02 UTC
I have one request. I would like to have an option to display only one user on gdm screen, even if there are more users that login through gdm from time to time.
Comment 10 Cale Fairchild 2008-12-01 19:10:09 UTC
I am wondering if this discussion is still active. We have about 1000 student accounts on our machines but very few actually log in so I would be very interested in being able to disable the user list again. I am wondering if this request is still alive?
Comment 11 Neil Bird 2009-01-18 12:40:26 UTC
<metoo/>

I've just updated to Fedora 10 which has the new gdm (from F8 which didn't).

I have a small no. of not-quite system users (e.g., spam recipient) who never log in but who show up on gdm (not ck-history --frequent;  in factm, this gives the limited list I'd like to see), and another batch who only log in remotely & don't need faces.
Comment 12 Valent Turkovic 2009-06-28 10:30:49 UTC
Is this on anybody's radar? 

I guess the best thing to do right now is not to use GDM and use KDM from KDE4 because it is in much better shape, has more features and has a graphical editor tool.
Comment 13 Ray Strode [halfline] 2009-07-16 17:53:00 UTC
Note the userlist can get disabled via GConf, e.g.,

gconftool-2 --direct --config-source xml:readwrite:/etc/gconf/gconf.xml.defaults --type bool --set /apps/gdm/simple-greeter/disable_user_list true

This isn't documented in the user manual but should be.  I'll open a separate bug report for that.
Comment 14 Ray Strode [halfline] 2009-07-16 17:58:23 UTC
see bug 588804 for the documentation issue mentioned in comment 13
Comment 15 Brian Cameron 2009-09-01 01:02:04 UTC
Created attachment 142182 [details] [review]
patch fixing issue


This patch adds back the include, include_all, and exclude options to the new GDM.

Can this go upstream?
Comment 16 Brian Cameron 2009-09-01 01:02:32 UTC
Created attachment 142183 [details] [review]
Docs for this change

patch for the docs for this change.
Comment 17 Ray Strode [halfline] 2009-09-09 18:16:00 UTC
seems okay.
Comment 18 Ray Strode [halfline] 2009-09-09 18:17:46 UTC
well actually:

+        for (pwent = fgetpwent (fp); (pwent != NULL && manager->priv->include_all == TRUE); pwent = fgetpwent (fp)) {

That's a little sloppy. Shouldn't check include_all every itereation of the loop.
Comment 19 Brian Cameron 2009-09-10 00:00:37 UTC
Created attachment 142839 [details] [review]
updated patch


This patch moves the for-loop into an if-test so we only check it once. 

Can this go upstream now that this is fixed?
Comment 20 Brian Cameron 2009-09-10 01:13:16 UTC
I committed the latest patch upstream to master.  I didn't notice at first that
you marked this accepted-commit-now, but I still fixed the minor issue that you raised.
Comment 21 Brian Cameron 2009-09-12 01:14:15 UTC
I backed out my fix for this bug from master.  It broke string freeze, and
I think there needs to be more discussion about whether the fix in bug
#593996 is a better approach.
Comment 22 Brian Cameron 2009-10-30 22:47:45 UTC
Created attachment 146604 [details] [review]
Updated patch


This updated patch is similar to the last patch except that it makes the client programs (like the login GUI) use the custom.conf configuration to set the Include, Exclude, and IncludeAll options.  This makes the patch backwards compatible with the old GDM, so it uses the old configuration options.  I've tested and it works well.  

Can this version of the patch go upstream?
Comment 23 Valent Turkovic 2009-10-31 08:33:28 UTC
Please sent this patch upstream, there are lots of users waiting for this. Thank you Brian very, very much for writing this patch.
Comment 24 Ray Strode [halfline] 2009-11-09 15:20:33 UTC
Review of attachment 146604 [details] [review]:

Getting pretty close.

::: gdm-2.28.1/data/gdm.schemas.in.in-orig
@@ +591,3 @@
+        return g_strcmp0 ((char *) a,
+                          (char *) b);
+}

why don't you jsut pass g_strcmp0 for the custom find func?  Also, note the missing braces.

@@ +594,3 @@
+
+static gboolean
+check_excludes (GdmUserManager *manager, const char *user)

maybe this should be called
user_in_exclude_list ?

Also, parameters need to be on their own lines

@@ +603,3 @@
+        /* always exclude the "gdm" user. */
+        if (user == NULL || (strcmp (user, GDM_USERNAME) == 0))
+                return TRUE;

missing braces

@@ +610,3 @@
+                                             match_name_cmpfunc);
+                if (found != NULL)
+                        ret = TRUE;

missing braces

@@ +1386,3 @@
+                return;
+        }
+}

is add_included_user missing some code?

@@ +1422,3 @@
+        if (manager->priv->include_all != TRUE) {
+                g_debug ("GdmUserManager: include_all is FALSE");
+        } else {

instead of moving all this code over an indention level, maybe just if (!manager->priv->include_all) { return ; }  Or, just don't call reload_passwd if include_all is false.

@@ +1681,3 @@
+
+        g_strfreev (temparr);
+}

Should the be moved to the gdm-settings api?
Comment 25 Brian Cameron 2009-11-09 21:32:09 UTC
Created attachment 147326 [details] [review]
updated patch

Updated patch.
Comment 26 Brian Cameron 2009-11-09 21:45:56 UTC
Here is an updated patch that resolves the issues raised.  Note that Ray and I had a discussion about this patch on IRC, and I'll explain what was discussed.


> ::: gdm-2.28.1/data/gdm.schemas.in.in-orig
> why don't you jsut pass g_strcmp0 for the custom find func?  Also, note the
> missing braces.

Note that the file you mean is gui/simple-greeter/gdm-user-manager.c in the function match_name_cmpfunc.  Note there are other functions in this file (match_real_name_cmpfunc and match_real_name_hrfunc) that also use g_strcmp0, so I am just being consistent with the other functions here.  Ray agreed it was
okay to leave this.

>+static gboolean
>+check_excludes (GdmUserManager *manager, const char *user)
>
> maybe this should be called
> user_in_exclude_list ?

Changed.

> Also, parameters need to be on their own lines

Fixed.  I also fixed the "missing braces" issues mentioned.

> @@ +1386,3 @@
> +                return;
> +        }
> +}
>
> is add_included_user missing some code?

No, note that the call to gdm_user_manager_get_user adds the user if it is 
valid and not already in the hash.  I added a comment to make this more clear.

> @@ +1422,3 @@
> +        if (manager->priv->include_all != TRUE) {
> +                g_debug ("GdmUserManager: include_all is FALSE");
> +        } else {
>
> instead of moving all this code over an indention level, maybe just if
> (!manager->priv->include_all) { return ; }  Or, just don't call reload_passwd
> if include_all is false.

No, note there is code at the end of the function that still needs to be called
even if include_all is FALSE.  

After looking at this more closely, Ray agreed with the above, but he felt that 
looping over and adding the users specified by greeter/Include was a bit weird 
for the reload_passwd function since included users do not come from the passwd 
file.  So, this logic was moved to a separate function "add_included_users" 
which is called after reload_passwd is called.

> @@ +1681,3 @@
> +
> +        g_strfreev (temparr);
> +}
>
> Should the be moved to the gdm-settings api?

I did not feel it made sense to add the infrastructure to do this when this is 
only used by the Include/Exclude options in this single file.  Looking at the 
old GDM code, these are the only options which use string lists (aside from
daemon/ConsoleCannotHandle which is unlikely to be ported to the new GDM), so 
such infrastructure probably will not ever be needed.

However, if more configuration options need to use string lists in the future, 
then it would make sense to add more infrastructure to support this more 
generally.

Also, it isn't clear how to best add this infrastructure.  Should it be 
managed at the GConf level, or at a higher level (as it is currently)?  If
more options start using this feature, then we can consider how to best add
such infrastructure at that time.  So, I added a comment to highlight this.
Comment 27 Ray Strode [halfline] 2009-11-09 21:59:06 UTC
Review of attachment 147326 [details] [review]:

Looking good. Feel free to commit after the remaining niggles are ironed out.

::: gdm-2.28.1/common/gdm-settings-keys.h-orig
@@ +1443,3 @@
+        } else {
+                g_debug ("GdmUserManager: include_all is TRUE");
+

should be able to avoid the extra indentation now, right?

@@ +1677,3 @@
+gdm_set_string_list (char *value, GSList **retval)
+{
+        char **temparr;

I would probably call this array or temp_array.  variable names aren't normally squeezed together like that.

@@ +1689,3 @@
+
+        temparr = g_strsplit (value, ",", 0);
+        for (i=0 ; temparr != NULL && temparr[i] != NULL ; i++) {

spacing's wrong here, there's extra check per iteration we don't need either, should be:

for (i = 0; temparr[i] != NULL; i++) {

@@ +1714,3 @@
+        res = gdm_settings_client_get_string  (GDM_KEY_INCLUDE,
+                                               &temp);
+        gdm_set_string_list (temp, &(manager->priv->include));

weird extra parentheses here

@@ +1722,3 @@
+
+        res = gdm_settings_client_get_boolean (GDM_KEY_INCLUDE_ALL,
+                                               &(manager->priv->include_all));

and here.
Comment 28 Brian Cameron 2009-11-17 00:01:39 UTC
Created attachment 147942 [details] [review]
updated patch which resolves Ray's concerns

Ray approved committing this version of the patch into master.  This resolves the issues he raised.
Comment 29 Brian Cameron 2009-11-23 17:38:30 UTC
*** Bug 602641 has been marked as a duplicate of this bug. ***
Comment 30 William Jon McCann 2010-06-16 23:32:05 UTC
*** Bug 568326 has been marked as a duplicate of this bug. ***
Comment 31 Valent Turkovic 2010-06-17 07:38:39 UTC
Are these options available only for 2.30.x and newer gdm only?
http://library.gnome.org/admin/gdm/2.30/configuration.html.en

Will these patches be applied to 2.24 and 2.28?