GNOME Bugzilla – Bug 791860
Use automatic login configuration from AccountsService
Last modified: 2018-05-24 11:44:47 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?
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.
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?
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.
https://bugs.freedesktop.org/show_bug.cgi?id=104564
Created attachment 366614 [details] [review] Use automatic login configuration from AccountsService Updated patch to use recently landed new A-S property.
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
Created attachment 366616 [details] [review] Use automatic login configuration from AccountsService
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.
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.
any news on this ?
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.
-- 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.