GNOME Bugzilla – Bug 593996
Add user manager dbus interface to gdmserver
Last modified: 2009-11-17 00:39:57 UTC
There are multiple services that access the list of users able to log in: 1. The GDM greeter 2. The GDM configuration applet 3. The user switch applet This list can take some time to generate and has duplicated code in all of these services. Make this list collected once in the gdmserver and these services should access it via dbus+polkit. Tracked in Ubuntu: https://bugs.launchpad.net/ubuntu/+source/gdm/+bug/423450
Created attachment 142382 [details] [review] Provide org.gnome.DisplayManager.UserManager First effort at providing the user list and associated information in the GDM server.
Note that the fix to bug #557553 just went upstream which will require a bit of rework on this patch.
Created attachment 142958 [details] [review] Provide org.gnome.DisplayManager.UserManager Works now, please review
Thanks. I think your patch is the better way to go. I am going to back out the fix for bug #557553 from master, and I think we should push forward with this version of the patch. Moving the reading of the system user information from the GUI to the daemon makes good sense to me. However, I think if we make tihs change, we should rework how the configuration settings are handled. In the old GDM 2.20, these configuration options were in the daemon configuration in the custom.conf file in the greeter section, as below. In my patch to bug #557553 I decided to move the configuration to the greeter GConf config style, but that was only because in the new code the user access was being handled by the GUI. If we are going to move this back to the daemon, then I think using the daemon configuration style is better. Also, probably better to keep the configuration options similar to the way they used to work with GDM 2.20. What do you think? Would you be agreeable to reworking the patch a little for this? Thanks, I really like the patch. [greeter] [...] # Users listed in Include will be included in the face browser and in the # gdmsetup selection list for Automatic/Timed login. Users should be separated # by commas. #Include= # Users listed in Exclude are excluded from the face browser and from the # gdmsetup selection list for Automatic/Timed login. Excluded users will still # be able to log in, but will have to type their username. Users should be # separated by commas. #Exclude=bin,daemon,adm,lp,sync,shutdown,halt,mail,news,uucp,operator,nobody,gdm ,postgres,pvm,rpm,nfsnobody,pcap # By default, an empty include list means display no users. By setting # IncludeAll to true, the password file will be scanned and all users will be # displayed except users excluded via the Exclude setting and user ID's less # than MinimalUID. Scanning the password file can be slow on systems with # large numbers of users and this feature should not be used in such # environments. The setting of IncludeAll does nothing if Include is set to a # non-empty value. #IncludeAll=false
I backed out the fix for bug #557553 from master, so we can have further discussion about whether that approach or this approach is the right way to fix this. Actually, I think if we move the configuration to the daemon configuration as suggested in the last comment, that this is the better approach, since it keeps the configuration interfaces stable with the old GDM. Thoughts?
Aside from fixing the way this is configured, I also notice that the patch does not yet make the simple-greeter or the fast-user-switch applet use these new D-Bus interfaces. I don't think this patch could go upstream until the simple-greeter and fast-user-switch applets also are patched to use these interfaces, and remove that duplicate code.
A minor issue I notice with the patch is that you modify daemon/Makefile.am to add this line: -DCACHEDIR=\"$(cachedir)\" \ However, the Makefile.am is already setting this as: -DGDM_CACHE_DIR=\""$(localstatedir)/cache/gdm"\" \ Also, the code doesn't seem to use "CACHEDIR" anywhere. Why don't we just use GDM_CACHE_DIR and remove the -DCACHEDIR line from the Makefile.am file? We also probably need to modify the line that sets -DGDM_CACHE_DIR to set the value to $(cachedir) instead.
Thanks for the feedback Brian. I will produce two patches for the greeter and fusa.
Any update? It would be good to have patches ready to review early in the release cycle. This is a fairly big change and so it is more likely to make it in the next release if it has plenty of time for review.
*** Bug 597660 has been marked as a duplicate of this bug. ***
Only slow progress to report. I've got the user switch applet 90% done, and will work on the greeter after that. Due to the Ubuntu 9.10 release at the end of this month there probably wont be any significant progress until then.
At the Boston Summit, there was discussion about this patch. The maintainers felt that it is not appropriate for the root running process to collect the user information. This, for example, could expose user information in the Face Browser that normal users are not supposed to see. So, I don't think that this patch will be accepted upstream. I am closing as WILLNOTFIX. Please re-open if you want to discuss and reconsider Note that the fix for #557553 to add Include/Exclude/IncludeAll features has gone upstream into GDM 2.29.0. Since that bug was mentioned in this discussion, I wanted to provide an update about that.