Old guest users pollute accountsservice

Bug #1259562 reported by Gunnar Hjalmarsson
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
accountsservice
Fix Released
Medium
accountsservice (Ubuntu)
Fix Released
Low
Gunnar Hjalmarsson

Bug Description

When you log out from a guest session, the temporary guest account is removed - but only almost. The folder /var/lib/AccountsService/users gets 'polluted' with files for previous guest users.

Tags: patch
Revision history for this message
Gunnar Hjalmarsson (gunnarhj) wrote :

The attached patch is one way to fix the issue. I know that you are not supposed to edit in /var/lib/AccountsService/users directly, so it's probably not the most proper solution you can think of, but it works. ;-)

tags: added: patch
Revision history for this message
Robert Ancell (robert-ancell) wrote :

Ah, yeah, that's not something lightdm would be allowed to do. I suspect the correct solution is that accounts service should delete directories if the user accounts no longer exist on startup or similar.

Revision history for this message
Gunnar Hjalmarsson (gunnarhj) wrote :

Well, accountsservice provides a DeleteUser method, so from an accountsservice POV you can argue that the correct solution is to call that method (as is done when you delete a regular user via User Accounts in gnome-control-center). I don't know how that would be done from a dash script, though.

Revision history for this message
Robert Ancell (robert-ancell) wrote :

Yeah, calling DeleteUser would help, though it's not symmetrical since we didn't call AddUser in the first place. There's a bunch of ways where accounts could disappear without accounts service being explicitly notified. The most reliable method would be for it to purge it's cache when it detects this.

The attached patch will probably fix this (not tested) since A-S already scans the cache directory on startup.

Changed in lightdm (Ubuntu):
importance: Undecided → Medium
Changed in lightdm:
status: New → Triaged
Changed in lightdm (Ubuntu):
status: New → Triaged
Changed in lightdm:
importance: Undecided → Medium
Changed in lightdm (Ubuntu):
importance: Medium → Low
Changed in lightdm:
importance: Medium → Low
Revision history for this message
Robert Ancell (robert-ancell) wrote :

As an aside, I was thinking when I added guest support to LightDM it would be good to make an AddGuest method to A-S and leave it to A-S to do all the guest account work. In that case it would allow A-S to correctly clean up when the guest account was removed. We could also mark the guest accounts as "delete on reboot" so A-S could properly clean them up if LightDM messes up.

Just never found the time to do it...

Revision history for this message
In , Gunnar Hjalmarsson (gunnarhj) wrote :

This is a forward of the Ubuntu bug https://launchpad.net/bugs/1259562

The guest session feature provided by lightdm has resulted in files for deleted temporary guest users being kept in the cache dir. To fix this, we are about to patch src/daemon.c so it deletes such files, and not just reports their existence in the log.

Since users may be deleted in a number of various ways without accountsservice being notified, we think it would be a good idea to make this change upstream.

Revision history for this message
In , Gunnar Hjalmarsson (gunnarhj) wrote :

Created attachment 90603
Clean up cache dir

Revision history for this message
Gunnar Hjalmarsson (gunnarhj) wrote :

I successfully tested a slightly modified patch, so I uploaded and forwarded upstream.

Changed in accountsservice (Ubuntu):
assignee: nobody → Gunnar Hjalmarsson (gunnarhj)
importance: Undecided → Low
status: New → In Progress
description: updated
Changed in accountsservice:
importance: Unknown → Medium
status: Unknown → Confirmed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package accountsservice - 0.6.35-0ubuntu2

---------------
accountsservice (0.6.35-0ubuntu2) trusty; urgency=low

  [ Robert Ancell ]
  * debian/patches/0017-clean-up-cache-dir.patch:
    Remove user cache files if user account no longer exists
    (LP: #1259562).
 -- Gunnar Hjalmarsson <email address hidden> Wed, 11 Dec 2013 14:22:00 +0100

Changed in accountsservice (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
In , Rstrode (rstrode) wrote :

don't you need to clean up the icon too?

Aside from that, I wonder if we'll run into bug reports after this where users lose their settings after a domain controller blip.

Maybe it should avoid cleaning up remote users?

Revision history for this message
In , Gunnar Hjalmarsson (gunnarhj) wrote :

Right, it's logical to clean up the icon as well. (Guest users typically don't bother with icons, which explains why I missed it.)

Maybe checking errno would provide sufficient safety?

   errno = 0;
   pwent = getpwnam (name);
   if (pwent == NULL && errno == 0) {

Revision history for this message
In , Gunnar Hjalmarsson (gunnarhj) wrote :

From "man getpwnam":

RETURN VALUE
The getpwnam() and getpwuid() functions return a pointer to a passwd structure, or NULL if the matching entry is not found or an error occurs. If an error occurs, errno is set appropriately.

Revision history for this message
In , Gunnar Hjalmarsson (gunnarhj) wrote :

Created attachment 90679
Clean up cache dir 2

Attaching a new patch in accordance with comment #3.

Revision history for this message
In , Rstrode (rstrode) wrote :

I like this patch. Two requests, if you don't mind:

1) Can you make it in git-format-patch format so it has a commit message, etc ?
2) Can you do one preliminary clean up patch before this one that moves the existing cases where the cache files are removed into a standalone function, then change this patch to call that function? This sort of clean up is repeated in two places already, it'd be good to drop the duplication rather than add a third place.

Revision history for this message
In , Gunnar Hjalmarsson (gunnarhj) wrote :

Created attachment 90741
Clean up cache dir 3

I chose to modify the patch so it makes use of a standalone function.

Sorry, but I'm not enough familiar with git, so I leave it to you to convert the patch into the desired format.

Revision history for this message
In , Iain Lane (laney) wrote :

Created attachment 93452
Move cache cleanup out into a common function and clean up icon too

I did as Ray suggested.

Also I passed --author to pretend that Gunnar wrote the git patches. If you find that objectionable, set it to me.

Revision history for this message
In , Iain Lane (laney) wrote :

Created attachment 93453
On startup, clean out the data of removed users

Revision history for this message
In , Michael Catanzaro (mike-catanzaro) wrote :

Seems these patches have been forgotten....

no longer affects: lightdm
no longer affects: lightdm (Ubuntu)
Changed in accountsservice:
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.