Active user continues to work if it is removed at this time

That's item #2 on phirate's list:

  http://phirate.posterous.com/the-complete-identity-session-checklist

Perhaps we should review our session handling.

Mahara doesn't check every request that the user is deleted or not, for speeeeed.

It's supposed to be checked in the global pieform validation handler - e.g. every time someone submits a form. Sounds like that's not happening.

Though I suppose if you revoke their session then that won't matter, they simply won't be able to continue.

Good luck working out which of the open sessions are theirs though, Mahara currently uses the stock PHP file based sessions. There's some code in MNET that tries to do something like this, but it's a bit messy.

Why not to "subscribe" some function to "deleteuser" event that will flag-up (through the new notification_sysmessages table) that some particular variable ($USER->delete in this case) in USER object has to be changed.

notification_sysmessages table could be of the following format:
Column | Type | Modifiers
--------------+----------------------+-----------
 userid | int | not null
 objectfield | character varying(255) | not null
 value | character varying(255) | not null

Then in init.php we may introduce the check if there are new system messages addressed to the user, and if so - the corresponding variables in USER object will be updated (or the whole USER object will be recreated) and relevant system message will be deleted. I planned to introduce the similar functionality for updating Walled Garden setting in user object.

If we have to check notification_sysmessages on every request we might as well just reload USER in from the usr table anyway.

You are right, Richard. Since '$userreallyadmin' and '$userreallyadminfor' lookups are done anyway (auth/lib.php:330 auth_setup) perhaps it worth updating privilege-related data at auth_setup stage directly. I have made necessary changes and added all relevant checks. Patch is attached.

By the way, the original control statement
if (!$USER->get('admin') && $userdata->admin) {
            // The user has been made into an admin
$USER->admin = 1;
}
nestled in in "if (defined('ADMIN'))" was useless since it is reached only if ADMIN constant is defined (which is not the case if user logged in as non-admin).

Also institution admin privilege update did not take into account changes in number of institutions user is admin for:

$userreallyadminfor = get_column('usr_institution', 'institution', 'usr', $USER->id, 'admin', 1);
        if (!$USER->is_institutional_admin() && !empty($userreallyadminfor)) {
            $USER->set_admin_institutions($userreallyadminfor);
        }
        else if ($USER->is_institutional_admin() && empty($userreallyadminfor)) {
            $USER->set_admin_institutions(array());
        }

Hi Ruslan,

Your patch does fix the problem, but Nigel's comment just makes me wonder about the speed penalty and if it's worth it. I don't know a huge amount about performance of php web apps but introducing another db read on every page load is something we'd like to avoid when going to the local filesystem is usually faster. At the moment we only need to do those userreallyadmin database checks when a user is browsing in the admin section of the site, which is hopefully not very often.

I think it's probably worth checking out that pieform validation stuff -- I might have a look at it and see if I can get it working. Then it'd still be pretty safe because a deleted user's session would hopefully be revoked as soon as they tried to do anything useful like submitting a form.

On form submission there was a check for user suspension, but no check for the user being deleted.

This probably got overlooked because I'm pretty sure at one time the admin had to suspend users before being allowed to delete, but that is no longer true.

I added a check to make sure the user is not deleted.

Just measured performance of auth_setup() in my environment: with my patch it takes ~17ms, without it, it takes ~12ms. The most time consuming call is User->renew() (~7ms) which is called in both cases. I was thinking that one day we will find out that group or institution membership, or group/site/institution admin/tutor/staff settings should be updated immediately. But even now, as I mentioned, if user is admin for more than one institution and will be demoted from being admin in one, it will not come in power until re-login (the same is implied for group admins/membership). It is clear that the number of db queries should be minimized, though I think that three small queries will not affect performance much;))

Ruslan, If your environment has the db and webserver on the same box, you're cheating! But yeah, I wouldn't be surprised if one day we feel the need to update all those things immediately, but let's leave it till we really need it. Thanks for pointing out the bug with people demoted from admin of one institution - I took your suggestion and reload the user's institutions completely, but so far just on every defined('INSTITUTIONALADMIN') page.