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 791860 - Use automatic login configuration from AccountsService
Use automatic login configuration from AccountsService
Status: RESOLVED OBSOLETE
Product: gdm
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2017-12-21 23:24 UTC by Robert Ancell
Modified: 2018-05-24 11:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use automatic login configuration from AccountsService (6.41 KB, patch)
2017-12-21 23:24 UTC, Robert Ancell
none Details | Review
Use automatic login configuration from AccountsService (6.36 KB, patch)
2018-01-10 21:00 UTC, Robert Ancell
none Details | Review
Use automatic login configuration from AccountsService (6.34 KB, patch)
2018-01-10 21:19 UTC, Robert Ancell
none Details | Review

Description Robert Ancell 2017-12-21 23:24:25 UTC
Created attachment 365859 [details] [review]
Use automatic login configuration from AccountsService

As requested in https://bugs.freedesktop.org/show_bug.cgi?id=40020 display managers should pull the autologin information from AccountsService instead of configuring it themselves.

Attached patch does this. Not as AccountsService currently runs it just means that A-S now does the /etc/gdm/custom.conf read. We can separately migrate A-S to writing it's own autologin config (/var/lib/AccountsService/autologin).

The side effect of this is any other GDM configuration backend cannot configure autologin information. I'm guessing this is not an important feature and AccoutsService is always expected to be there?
Comment 1 Ray Strode [halfline] 2018-01-03 21:35:08 UTC
iterating over every cached user for this seems like a bad idea.  I'd rather we fixed accountsservice first to export AutomaticLoginUser or AutomaticLoginUsers or something first, rather than iterating over all cached users.  

gnome-shell, for instance, used to always ListCachedUsers to see if there was more than one user on the system. it ended up causing a big performance issue for certain large ldap deployments, so we added "HasMultipleUsers" here https://cgit.freedesktop.org/accountsservice/commit/?id=c323b2356eb46ecc7047b46cf20672ad9ab5a7ee

granted, the greeter will have to do ListCachedUsers regardless if the user list isn't disabled, but we shouldn't need to do it for autologin.
Comment 2 Robert Ancell 2018-01-08 03:42:00 UTC
I was initially working on that solution in A-S and ran into some API difficulties, perhaps you can help me resolve them:

I looked at adding an AutologinUser property that would return the D-Bus address of the autologin user. However, there doesn't seem to be an obvious thing to do when there is no user set. D-Bus return the '/' address, which is kind of weird.

I also tried making a GetAutologinUser method, but this either has to return '/' for no user or throw an exception (an exception seems a pain to handle in a client).

Other options are having a HasAutologinUser property that marks is AutologinUser is valid (seems redundant if '/' implies this) or returning the username in the property or '' for no user (seems a bad mismatch for the existing A-S API).

Any preferences / better ideas?
Comment 3 Ray Strode [halfline] 2018-01-08 14:34:27 UTC
I think HasAutologinUser makes sense because we already have HasMultipeUsers and HasNoUsers, so it fits in well.

Another advantage is if a program just wants to make sure autologin is disabled, it doesn't have to get an object path it doesn't care about (and potentially created a proxy it doesn't need).

re the '/'... hmm, so autologin is a user property at the moment, and while no greeter supports multiple autologins at the same, maybe accountsservice could support it from a configuration point of view?  I don't think it's a super critical feature, but it does let us side step the sentinel problem, because we could just return array of objects of length 0 for the no-has case.
Comment 4 Robert Ancell 2018-01-09 23:14:36 UTC
https://bugs.freedesktop.org/show_bug.cgi?id=104564
Comment 5 Robert Ancell 2018-01-10 21:00:37 UTC
Created attachment 366614 [details] [review]
Use automatic login configuration from AccountsService

Updated patch to use recently landed new A-S property.
Comment 6 Ray Strode [halfline] 2018-01-10 21:12:55 UTC
Review of attachment 366614 [details] [review]:

please test before pushing!

::: daemon/gdm-manager.c
@@ +1296,3 @@
+                                                       "org.freedesktop.Accounts",
+                                                       "/org/freedesktop/Accounts",
+                                                       "org.freedesktop.Accounts.User",

shouldn't have the .User right?  Maybe we should use codegen? of course we're already calling this in act_user_manager_get_default, so maybe these apis should get exported through the library, not sure. It's a little unfortunate to create the proxy twice, but maybe not a big deal.

@@ +1335,3 @@
+
+                username = g_dbus_proxy_get_cached_property (proxy, "UserName");
+                return g_strdup (g_variant_get_string (username, NULL));

could be g_variant_dup_string
Comment 7 Robert Ancell 2018-01-10 21:19:33 UTC
Created attachment 366616 [details] [review]
Use automatic login configuration from AccountsService
Comment 8 Robert Ancell 2018-01-10 21:23:16 UTC
Yes, should have mentioned I haven't put this through a full test run yet - just want to check you're OK with the structure first.

I did look at adding a method to ActUserManager but it turned out to be more complex than I'd hoped. You've also mentioned replacing that code at some point so I figured it could be done then.

We could use codegen but yeah, it's a bit of a mismatch using codegen and ActUserManager.

I'm happy to add the method to ActUserManager if you prefer and bump the libaccountsservice dependency.
Comment 9 Ray Strode [halfline] 2018-01-10 21:30:36 UTC
well, i assume dropping the act_manager dependency first and switching to codegen as an upfront commit is out of scope for you? (doesn't hurt to ask right ?) :-)

failing, that I don't really care what approach we do, nothing is optimal and it should all get smooth out eventually.
Comment 10 Ray Strode [halfline] 2018-02-12 14:49:44 UTC
any news on this ?
Comment 11 Robert Ancell 2018-02-13 01:11:02 UTC
I did have a look at the codegen and yeah, that's a bit too much work to do right now.

I haven't had the time to fully test this, but I figure we can switch to it next cycle.
Comment 12 GNOME Infrastructure Team 2018-05-24 11:44:47 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gdm/issues/352.