LDAP user sync using root ID to suspend users

Bug #1606101 reported by Ghada El-Zoghbi
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mahara
Fix Released
Medium
Ghada El-Zoghbi
15.04
Fix Released
Medium
Unassigned
15.10
Fix Released
Medium
Unassigned
16.04
Fix Released
Medium
Unassigned
16.10
Fix Released
Medium
Ghada El-Zoghbi

Bug Description

Mahara: 16.04.2
DB: Postgres
OS: Linux

There is a bug with the LDAP sync when it is suspending users: htdocs/auth/ldap/cli/sync_users.php

The user that is running the cron LDAP sync job is 'root' - has an ID of 0.

Mahara updates the suspended user record with the userid that is doing the suspending (i.e. suspendedcusr = 0).

When validating if a user record is suspended, we check if the suspending user id is empty. Because the suspending user ID is '0', it thinks that to the suspended user is not suspended - when they should be suspended.

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "master" branch: https://reviews.mahara.org/6750

Revision history for this message
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.

Changed in mahara:
importance: Undecided → Medium
milestone: none → 16.10.0
status: New → In Progress
assignee: nobody → Aaron Wells (u-aaronw)
Revision history for this message
Aaron Wells (u-aaronw) wrote :

Abandoned patch 6750 after discussing it with Ghada, who has convinced me that putting an invalid user ID in that column creates too many potential bugs.

So right now our options are:

1. Arbitrarily pick one of the site admin users, and use their ID as the suspendedcusr.

2. Locate every place that checks (bool) usr.suspendedcusr, and change it to check (bool) usr.suspendedctime instead.

Option 1 is easier, but it has the nasty side effect that, well, we're arbitrarily picking one admin user and very rarely using them as the suspendedcusr. The name of that admin user is included in the message that gets sent to the suspended user, and so depending on how things are set up in that site, this could cause confusion. (Like, "Why did Tony suspend me?").

Option 2 would be cleaner. Or better yet, add a "usr.suspended" column, which is 0 or 1, and check that instead. That would prevent future devs from accidentally going back to relying on suspendedcusr. But the downside to 2, is that suspendedcusr shows up 27 different times throughout the code, and sometimes does double-duty as a boolean and as the ID of the suspending user, in ways that may be tricky to debug and test. So maybe, since the LDAP sync is the only way this situation comes up, it would be best to just stick with the low-effort Option 1?

Revision history for this message
Ghada El-Zoghbi (ghada-z) wrote :

I'll submit a patch for option 1.

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "master" branch: https://reviews.mahara.org/6751

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6751
Committed: https://git.mahara.org/mahara/mahara/commit/ead553ee6b9fbec1c3766b93b2c7041dc89621ff
Submitter: Aaron Wells (<email address hidden>)
Branch: master

commit ead553ee6b9fbec1c3766b93b2c7041dc89621ff
Author: Ghada El-Zoghbi <email address hidden>
Date: Mon Jul 25 17:14:01 2016 +1000

Bug 1606101: usr.suspendedcusr must be non-zero

It turns out a lot of existing code checks the boolean
value of usr.suspendedcusr to determine if a user should
be treated as suspended or not. The LDAP sync cron (and,
indeed, any code suspending users via a cron task) was
setting usr.suspendedcusr to 0, which is boolean false,
so these users would be treated as not suspended.

We are going to update all usr.suspendedcusr = 0
to a valid site admin ID.

Change-Id: Iecfbfd8a4cdd98d5d07149bb40c64308262ea234
behatnotneeded: Test to come later

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "16.04_STABLE" branch: https://reviews.mahara.org/6765

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/6766

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "15.04_STABLE" branch: https://reviews.mahara.org/6767

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6765
Committed: https://git.mahara.org/mahara/mahara/commit/04defa2c15d7cb7505114efc09c1728e08ad1439
Submitter: Robert Lyon (<email address hidden>)
Branch: 16.04_STABLE

commit 04defa2c15d7cb7505114efc09c1728e08ad1439
Author: Ghada El-Zoghbi <email address hidden>
Date: Mon Jul 25 17:14:01 2016 +1000

Bug 1606101: usr.suspendedcusr must be non-zero

It turns out a lot of existing code checks the boolean
value of usr.suspendedcusr to determine if a user should
be treated as suspended or not. The LDAP sync cron (and,
indeed, any code suspending users via a cron task) was
setting usr.suspendedcusr to 0, which is boolean false,
so these users would be treated as not suspended.

We are going to update all usr.suspendedcusr = 0
to a valid site admin ID.

Change-Id: Iecfbfd8a4cdd98d5d07149bb40c64308262ea234
behatnotneeded: Test to come later

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6767
Committed: https://git.mahara.org/mahara/mahara/commit/1b577978725239c9737d1e15407325006c23c49c
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.04_STABLE

commit 1b577978725239c9737d1e15407325006c23c49c
Author: Ghada El-Zoghbi <email address hidden>
Date: Mon Jul 25 17:14:01 2016 +1000

Bug 1606101: usr.suspendedcusr must be non-zero

It turns out a lot of existing code checks the boolean
value of usr.suspendedcusr to determine if a user should
be treated as suspended or not. The LDAP sync cron (and,
indeed, any code suspending users via a cron task) was
setting usr.suspendedcusr to 0, which is boolean false,
so these users would be treated as not suspended.

We are going to update all usr.suspendedcusr = 0
to a valid site admin ID.

Change-Id: Iecfbfd8a4cdd98d5d07149bb40c64308262ea234
behatnotneeded: Test to come later

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6766
Committed: https://git.mahara.org/mahara/mahara/commit/dbaebc656a35fbed870ed5b3c9162ccb5bcb9464
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit dbaebc656a35fbed870ed5b3c9162ccb5bcb9464
Author: Ghada El-Zoghbi <email address hidden>
Date: Mon Jul 25 17:14:01 2016 +1000

Bug 1606101: usr.suspendedcusr must be non-zero

It turns out a lot of existing code checks the boolean
value of usr.suspendedcusr to determine if a user should
be treated as suspended or not. The LDAP sync cron (and,
indeed, any code suspending users via a cron task) was
setting usr.suspendedcusr to 0, which is boolean false,
so these users would be treated as not suspended.

We are going to update all usr.suspendedcusr = 0
to a valid site admin ID.

Change-Id: Iecfbfd8a4cdd98d5d07149bb40c64308262ea234
behatnotneeded: Test to come later

Robert Lyon (robertl-9)
Changed in mahara:
milestone: 16.10.0 → none
status: Fix Committed → 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.