Bug 56729

Created attachment 69500 [details] [review]
0001-util.h-add-missing-headers.patch

Created attachment 69501 [details] [review]
0002-cfg-initial-support-for-a-configuration-file.patch

Created attachment 69502 [details] [review]
0003-cfg-replace-default_excludes-with-configuration-file.patch

Created attachment 69503 [details] [review]
0004-cfg-add-option-for-minimum-uid.patch

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.

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.

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/

(by data dir, i mean $(datadir), i.e. /usr/share/accountsservice)

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.

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.

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).

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.

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

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?

hey, i just got back from a vacation away from computer (since the 16th), i'll look over your stuff soon!

*** Bug 50103 has been marked as a duplicate of this bug. ***

*** Bug 41908 has been marked as a duplicate of this bug. ***

Could you attach a squashed patch here ? I prefer to use splinter to review it

or individual patches, for that matter

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.

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.)

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.

Hi Matthias (or others), can anyone start reviewing the patch?  (puppy-dog eyes)

-- 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.