Bug 56729 - [PATCH] add a configuration file to accountsservice
Summary: [PATCH] add a configuration file to accountsservice
Status: RESOLVED MOVED
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Keywords:
: 41908 50103 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-11-03 21:51 UTC by Matthew Monaco
Modified: 2018-08-07 09:33 UTC (History)
13 users (show)

See Also:
i915 platform:
i915 features:


Attachments
0001-util.h-add-missing-headers.patch (590 bytes, patch)
2012-11-03 21:52 UTC, Matthew Monaco
Details | Splinter Review
0002-cfg-initial-support-for-a-configuration-file.patch (4.42 KB, patch)
2012-11-03 21:52 UTC, Matthew Monaco
Details | Splinter Review
0003-cfg-replace-default_excludes-with-configuration-file.patch (4.22 KB, patch)
2012-11-03 21:52 UTC, Matthew Monaco
Details | Splinter Review
0004-cfg-add-option-for-minimum-uid.patch (5.37 KB, patch)
2012-11-03 21:54 UTC, Matthew Monaco
Details | Splinter Review
squashed patch from github (26.76 KB, patch)
2014-09-28 10:59 UTC, Sam Brightman
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Monaco 2012-11-03 21:51:30 UTC
The hardcoded default_excludes is really annoying. It's about time we had support for a configuration file. The attached patch set includes this support.

It adds support for [UserList]/Excludes which is shipped with the list of users currently hardcoded in default_excludes.

It also adds support for [UserList]/MinUID with -- as far as I can tell -- a common default of 1000. However, this is now easy for distributions to modify.
Comment 1 Matthew Monaco 2012-11-03 21:52:08 UTC
Created attachment 69500 [details] [review]
0001-util.h-add-missing-headers.patch
Comment 2 Matthew Monaco 2012-11-03 21:52:32 UTC
Created attachment 69501 [details] [review]
0002-cfg-initial-support-for-a-configuration-file.patch
Comment 3 Matthew Monaco 2012-11-03 21:52:51 UTC
Created attachment 69502 [details] [review]
0003-cfg-replace-default_excludes-with-configuration-file.patch
Comment 4 Matthew Monaco 2012-11-03 21:54:03 UTC
Created attachment 69503 [details] [review]
0004-cfg-add-option-for-minimum-uid.patch
Comment 5 Ray Strode [halfline] 2012-11-05 17:00:33 UTC
Hey, I'd prefer if you added metadata to each users config file instead of introduciting a new file.

Also, one thing to think about is accountsservice is used in various places to get a list of users

1) the login screen
2) the control center panel

maybe it would make sense to show the user in the panel even when not in the login screen? So we may want two keys.
Comment 6 Matthew Monaco 2012-11-05 18:05:46 UTC
Ray, I agree that more keys can be added, but all this does is pull a really arbitrary hard-coded list out of the binary into a config file. (And re-add Minimum UID because it made sense but no-one could agree on a value).

As for the per-user config file, I gotta disagree with you. Those are in /var/lib/... which doesn't really suggest they are intended to be done by hand. Also, the minimum UID is global behavior. I could see adding an option like Visibility to the per-user config which can be always on, no-cache, or off, .... But why block which IMO is more of a bug-fix because it also makes you want a new feature.
Comment 7 Ray Strode [halfline] 2012-11-12 19:53:36 UTC
I think we also want to keep a separation between admin configuration and distro configuration.

We should probably have two files, one in data dir and one in /etc/accountsservice/
Comment 8 Ray Strode [halfline] 2012-11-12 19:53:59 UTC
(by data dir, i mean $(datadir), i.e. /usr/share/accountsservice)
Comment 9 Matthew Monaco 2012-11-13 20:10:56 UTC
I agree, but I didn't want to put too much time into it before I knew it might be accepted.

Do you think that a single file in /usr that's overridable in /etc is enough? I could see a case for a config.d, lexographically sorted set of config files so that a package could drop in an exclude for a specific username that it installs.
Comment 10 Ray Strode [halfline] 2012-11-13 20:42:25 UTC
I think distros should be able to have their list which includes adm, lp, and nobody, and admins should be able to augment the list with their own users that could include tarball installs of 3rd party software and specific real users they want to conceal.

I'd say the distro file would go in /usr and the admin file in /etc.  Note, it would be bad if the admin file overrode the list instead of augmenting it.

I don't think config.d makes sense.  we have /var/lib/AccountsService/users/foo already that stores metadata about a specific user foo. that metadata could have "exclude from user list" or "exclude from control center panel" in it, I suppose.
Comment 11 Matthew Monaco 2012-11-13 22:20:16 UTC
Ok Ray. Hopefully I can finish it off this weekend. (The only thing about /var is, it seems like it's bad form to install files there, but lets not get hung up on that).
Comment 12 Ray Strode [halfline] 2012-11-14 16:25:23 UTC
well, the files are usually written there by accountsservice in response to dbus calls (e.g. from changes to gnome-control-center or whatever).

I guess we could introduce a accountsctl cli utility or so for managing the files, so the admin doesn't have to edit the /var files directly and doesn't have to use dbus-send.
Comment 13 Matthew Monaco 2012-11-23 01:15:29 UTC
Ok Ray,

I still need to add the config file under /usr/lib, but check it out so far [1].

This takes "Exclude" from either the config file(s) or the per-user file under /var/lib. I tried to make a distinction between a system account and an excluded account. The check for a system account is similar to what the daemon_local_is_excluded() function did. I don't think the check for HAVE_GETUSERSHELL was working because I have those functions but HAVE_GETUSERSHELL wasn't defined. I just removed the ifdef for now...

I still am including the MinimumUID setting. I think it would be best to default this to SYS_UID_MAX from /etc/login.defs, but I don't know the best way to retreive that value.

[1] https://github.com/mmonaco/accountsservice/tree/exclude-v2
Comment 14 Matthew Monaco 2012-11-28 03:39:28 UTC
https://github.com/mmonaco/accountsservice/commits/exclude-v3

Add's a config file to /lib/... and /etc/...
Excluded= and MinimumUID config options.

MinimumUID in /etc overrides /lib. Exclude in /etc is combind with /lib

Add's an excluded property to the User type and the DBus handlers for manipulating this type

Admin users are not cached. Excluded users are still cached, but they are not returned by ListCachedUsers... I think the best thing would be to just add a new method such as ListGreeterUsers() and get the DMs to use this instead. Thoughts?
Comment 15 Ray Strode [halfline] 2012-11-29 22:38:40 UTC
hey, i just got back from a vacation away from computer (since the 16th), i'll look over your stuff soon!
Comment 16 Ray Strode [halfline] 2012-12-06 19:32:39 UTC
*** Bug 50103 has been marked as a duplicate of this bug. ***
Comment 17 Ray Strode [halfline] 2012-12-06 19:33:39 UTC
*** Bug 41908 has been marked as a duplicate of this bug. ***
Comment 18 Matthias Clasen 2013-02-01 19:12:00 UTC
Could you attach a squashed patch here ? I prefer to use splinter to review it
Comment 19 Matthias Clasen 2013-02-01 19:14:51 UTC
or individual patches, for that matter
Comment 20 Ray Strode [halfline] 2013-10-15 20:01:14 UTC
I'm on the same page as Matthias, I think.  I started to look into merging this today, but I ran into a few issues that would be better enumerated using bugzilla.  If you could attach the individual patches that would be great.
Comment 21 Sinlu Bes 2013-11-16 01:54:46 UTC
Hello,

Its great that you want to review the changes. But why do you require them to be uploaded to bugzilla? Why do you not just retrieve them from github?

git clone https://github.com/mmonaco/accountsservice.git
cd accountsservice
git checkout origin/exclude-v3
git format-patch -8

(You could replace "-8" with "master", but due to bad branching this would prepend some unrelated commits from accountsservice.)
Comment 22 Sam Brightman 2014-09-28 10:59:17 UTC
Created attachment 106996 [details] [review]
squashed patch from github

I was surprised and disappointed that this bug had stalled due to disagreement over review methodology. I have followed the instructions above (the history is a little messy) and attached this squashed patch. Individual commit patches are also possible of course.
Comment 23 V字龍(Vdragon) 2016-03-23 08:11:27 UTC
Hi Matthias (or others), can anyone start reviewing the patch?  (puppy-dog eyes)
Comment 24 GitLab Migration User 2018-08-07 09:33:07 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org'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.freedesktop.org/accountsservice/accountsservice/issues/37.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.