Bug 64769 - accountsservice system account misclassification
Summary: accountsservice system account misclassification
Status: RESOLVED FIXED
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:
Depends on:
Blocks:
 
Reported: 2013-05-19 20:03 UTC by Matthias Clasen
Modified: 2013-08-06 16:32 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
daemon: cache users explicitly created via AccountsService (8.28 KB, patch)
2013-05-20 19:14 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: make cache_user do a full save (6.44 KB, patch)
2013-05-20 19:15 UTC, Ray Strode [halfline]
Details | Splinter Review
user: write account type in key file (2.58 KB, patch)
2013-05-20 19:15 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: notice when account changes from system acount to normal account (9.57 KB, patch)
2013-05-20 19:15 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: don't exclude system users (2.76 KB, patch)
2013-05-20 19:16 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: add gnome special users to exclusion list (1.73 KB, patch)
2013-05-20 19:16 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon:Trust SystemAccount key from keyfile over heuristics (3.05 KB, patch)
2013-05-20 19:17 UTC, Ray Strode [halfline]
Details | Splinter Review
user: never treat cached users as system accounts (11.79 KB, patch)
2013-05-20 19:17 UTC, Ray Strode [halfline]
Details | Splinter Review

Description Matthias Clasen 2013-05-19 20:03:33 UTC
I'm trying to track down why the 'Switch user' menuitem in gnome-shell does not always appear as I add and remove extra users on my system.

Here are some findings:

When I create a new account in the control-center user panel, it is initially disabled, and the accounts-daemon classifies the new user as a system account, via 

        user->system_account = daemon_local_user_is_excluded (user->daemon,
                                                              user->user_name,
                                                              pwent->pw_shell,
                                                              passwd);

in user_update_from_pwent.

If I then enable the account, e.g. by setting 'Log in without password' in the user panel, the system_account bit is not updated 

finish_list_cached_users looks at this bit and skips the account, so it never appears in the ListCachedUsers output, so the has-multiple-users property in the client-side ActUserManager object never gets set.
Comment 1 Ray Strode [halfline] 2013-05-20 19:13:13 UTC
git-bz is mishaving so i'll just attach the patchset i came up with manually. one moment.
Comment 2 Ray Strode [halfline] 2013-05-20 19:14:34 UTC
Created attachment 79581 [details] [review]
daemon: cache users explicitly created via  AccountsService

AccountsService can be used for creating user accounts, but shouldn't
be used for creating system accounts.  Therefore, all accounts created
with AccountsService can safely be considered user accounts.

This commit changes the daemon to immediately, implicitly cache all user
accounts created via the accounts service.  This ensures they'll never
get errnoneously tagged as system accounts.
Comment 3 Ray Strode [halfline] 2013-05-20 19:15:06 UTC
Created attachment 79582 [details] [review]
daemon: make cache_user do a full save

The cache_user function currently just does:

comment = g_strdup_printf ("# Cached file for %s\n\n", user_name);
g_file_set_contents (filename, comment, -1, &error);

This commit changes it to write out a full key file.
Comment 4 Ray Strode [halfline] 2013-05-20 19:15:29 UTC
Created attachment 79583 [details] [review]
user: write account type in key file

This commit changes account service to write whether or not the
account is a system account in the key file about the user.
Comment 5 Ray Strode [halfline] 2013-05-20 19:15:52 UTC
Created attachment 79584 [details] [review]
lib: notice when account changes from system acount to  normal account

We currently don't show system users (e.g. 'mysql') in the
control-center user list.  Occasionally a system user gets promoted to
a normal user account. When that happens, we fail to notice.

This commit fixes that.
Comment 6 Ray Strode [halfline] 2013-05-20 19:16:22 UTC
Created attachment 79585 [details] [review]
daemon: don't exclude system users

This client already handles filtering system users
from the cached user list, so there's no point in doing it
daemon side.

Furthermore, it's harmful since then clients never get informed
when a user account type changes from a system account to a user
account.

Let the client do the filtering.
Comment 7 Ray Strode [halfline] 2013-05-20 19:16:55 UTC
Created attachment 79586 [details] [review]
daemon: add gnome special users to exclusion list
Comment 8 Ray Strode [halfline] 2013-05-20 19:17:15 UTC
Created attachment 79587 [details] [review]
daemon:Trust SystemAccount key from keyfile over  heuristics

We use some less than perfect heuristics for determining whether or
not a user is a "system account".

This commit defers to what's in the key file when the key file
explicitly says.
Comment 9 Ray Strode [halfline] 2013-05-20 19:17:44 UTC
Created attachment 79588 [details] [review]
user: never treat cached users as system accounts

Right now the check for "is system user" is less than solid.
It depends on checking things like whether or not a password is set,
and whether or not a shell is set. Unfortunately, sometimes a password
is not set for a non-system account, and sometimes a shell is set for
a system account, so the heuristics aren't perfect.

In one particular case, we currently get things wrong: right after a
user is created before they have a password.

This commit attempts to shore up the heuristics by never treating cached
Comment 10 Ray Strode [halfline] 2013-05-20 19:19:27 UTC
Hopefully that should make things work a little better.  We still treat users created from outside control-center that don't have passwords as "system accounts" but hopefully that's enough of a corner case that it's not much of a problem in practice.

If we need to we could go back to uid ranges or so, but that's been problematic in the past.
Comment 11 Matthias Clasen 2013-05-20 19:25:25 UTC
Comment on attachment 79583 [details] [review]
user: write account type in key file

Review of attachment 79583 [details] [review]:
-----------------------------------------------------------------

Confusing to call it 'account type' in the summary - that means something else in accountsservice, and we're already writing that to the cache file (or do we ?)
Comment 12 Ray Strode [halfline] 2013-05-20 19:26:16 UTC
I guess one thing we're still not getting right is taking into account "locked" users when deciding whether or not to show the Switch User item.
Comment 13 Ray Strode [halfline] 2013-05-20 19:27:28 UTC
> Confusing to call it 'account type' in the summary - that means something
> else in accountsservice, and we're already writing that to the cache file
> (or do we ?)

I considered that at the time but couldn't think of a better way to word it.

maybe "write whether account is system account in key file" ?
Comment 14 Matthias Clasen 2013-05-20 19:27:34 UTC
Comment on attachment 79584 [details] [review]
lib: notice when account changes from system acount to  normal account

Review of attachment 79584 [details] [review]:
-----------------------------------------------------------------

::: src/libaccountsservice/act-user-manager.c
@@ +904,5 @@
> +                                set_has_multiple_users (manager, FALSE);
> +                        }
> +                }
> +        }
> +}

It seems a bit wrong to me that we're synthesizing these signal on the client-side, but I can see that it is easy to do it here.
Comment 15 Ray Strode [halfline] 2013-05-20 19:32:45 UTC
Hi,

(In reply to comment #14) 
> It seems a bit wrong to me that we're synthesizing these signal on the
> client-side, but I can see that it is easy to do it here.
We could maintain dual hash tables on the daemon side (one for normal accounts and one for system accounts) like we have on the client side, if you think it's a good idea to do it all daemon side.  That's a little more intrusive of a change though so I avoided it.
Comment 16 Matthias Clasen 2013-05-21 10:38:44 UTC
(In reply to comment #13)
> > Confusing to call it 'account type' in the summary - that means something
> > else in accountsservice, and we're already writing that to the cache file
> > (or do we ?)
> 
> I considered that at the time but couldn't think of a better way to word it.
> 
> maybe "write whether account is system account in key file" ?

'Store system account classification in key file ?'
Comment 17 Philippe "RzR" Coval 2013-08-06 16:32:38 UTC
Hi,


I see it is in RESOLVED FIXED state but it looks like some problems remains 
according to latest comments from from JBM [1] 
at https://bugzilla.gnome.org/show_bug.cgi?id=701524 

or is it bug to be fixed gnome side ? in this case sorry for the noise
if not then any track to fix it is welcome ...

Regards


[1] 
{

the patches from https://bugs.freedesktop.org/show_bug.cgi?id=64769 fix
partially the issue

with the following step the user list is updated
create new user
enable the new user
switch user

the list is updated

but if I follow this steps the user list is not updated

create new user
switch user
the list is not updated ( that is correct )
go back the previous user
enable the new user
switch user
the list is not updated

}


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.