Comment 2 for bug 1606101

Aaron Wells (u-aaronw) wrote :

To restate the problem here:

1. The LDAP sync cron task can be configured to suspend users. It does so using the function "suspend_user()" in user.php

2. When you suspend a user, there's a column, "usr.suspendingcusr", which is meant to record the user ID of the user who performed the suspension.

3. If you don't provide the suspending user's ID when you call suspend_user(), it defaults to $USER->get('id').

4. That's what happens when you run the LDAP sync cron. And since it runs via CLI, $USER->get('id') is "0". So "usr.suspendingcusr" is set to 0.

5. A lot of existing code throughout Mahara checks to see whether a user is suspended, by doing "if ($usr->suspendingcusr)" or "if (!empty($usr->suspendingcusr))".

6. The value of 0 passes both those checks, hence that existing code treats the user as if they were *not* suspended.

Now, we could go through the code and change all those places that check whether a user is suspended, so that they use usr.suspendedctime instead. But I think it will be less bug-prone if we instead change what we're storing from 0 to a negative number. That will pass the boolean checks, while still giving us a way to see that someone was suspended by a cron task.

The one wrinkle there is that suspend_user(), in generating the suspension notification message, runs display_name() on the suspending user's ID. And display_name() throws an InvalidArgumentException if you pass in the ID of a user who doesn't exist. But we can patch suspend_user() to take care of that.