Bug #3038
closed
J2.5: get_user_permission not checking full access tree
Added by krileon almost 13 years ago. Updated almost 13 years ago.
Description
When for example the moderator group is set to Group A and the user is a member of Group A, Group B, and Group C (all child groups of Registered) the user can not edit other users profiles. If they however are just a member of Group A they can edit other users profiles. Seams the access check isn't doing a recursive check as done in get_groups_below_me to ensure a proper J1.7 ACL tree is given.
Files
| 3038.patch (5.5 KB) 3038.patch | krileon, 29 November 2011 22:35 | ||
| cb.acl.php (26.9 KB) cb.acl.php | krileon, 29 November 2011 22:35 | ||
| 3038-pt2.patch (5.18 KB) 3038-pt2.patch | krileon, 01 February 2012 17:15 | ||
| 3038-pt2_rev1.patch (5.34 KB) 3038-pt2_rev1.patch | krileon, 01 February 2012 17:47 | ||
| 3038-pt2_rev2.patch (6.16 KB) 3038-pt2_rev2.patch | krileon, 01 February 2012 18:11 | ||
| 3038-pt2_rev3.patch (6.15 KB) 3038-pt2_rev3.patch | krileon, 01 February 2012 18:14 | ||
| 3038-part2_rev4.patch (6.21 KB) 3038-part2_rev4.patch | krileon, 01 February 2012 18:37 | ||
| 3038-pt2_rev5.patch (6.2 KB) 3038-pt2_rev5.patch | krileon, 01 February 2012 20:04 | ||
| 3038-pt2_rev6.patch (6.22 KB) 3038-pt2_rev6.patch | krileon, 01 February 2012 23:11 | ||
| 3038-pt2_rev7.patch (7.44 KB) 3038-pt2_rev7.patch | krileon, 01 February 2012 23:47 |
Updated by krileon almost 13 years ago
Closely look shows get_user_moderator is using get_user_group_id, which only returns a single ID and is being checked against $this->get_group_parent_ids( $ueConfig['imageApproverGid'] ). Better approach is to get a full list of users access similar to get_groups_below_me then see if $ueConfig['imageApproverGid'] is in that array.
Entire ACL file needs review for similar usages as get_user_group_id isn't valid usage anymore to check if the users group has permissions as it only gives top most group id.
Updated by krileon almost 13 years ago
- File 3038.patch added
- Status changed from New to Resolved
- Assignee set to beat
- % Done changed from 0 to 100
Upgraded get_groups_below_me to allow supplying a userid other then myid as well as an optional $raw parameter so it outputs an array of gids. Then upgraded get_users_permission, get_user_permission_task, and get_user_moderator to utilize its usage. This results in proper comparison of GID to users gid tree. Will require thorough testing, but quick testing shows it resolves this issue (and probably others we've yet to find).
Updated by krileon almost 13 years ago
- File 3038.patch added
- File cb.acl.php added
Updated by krileon almost 13 years ago
- Status changed from Resolved to Feedback
- Assignee deleted (
beat) - % Done changed from 100 to 80
In the below scenario it doesn't work properly.. or maybe it does.. subject to opinion.
Group 1
- Group 1a
Group 2
In the above Group 1a is the Moderator group, but they can not edit Group 2. It seams to be working as designed as Group 2 is a different parent group, but should it matter? Should moderator access even care what group anyone is? The error (see below) is happening in get_users_permission. It doesn't affect administrators or super administrators because it has a check to see if the user is either/or so they're not subject to child GID trees.
"You cannot ACTION a USERGROUP. Only higher-level users have this power."
Updated by krileon almost 13 years ago
- File 3038.patch added
- File cb.acl.php added
- Subject changed from get_user_permission_task not checking full access tree to get_user_permission not checking full access tree
- Status changed from Feedback to Resolved
- Assignee set to beat
- % Done changed from 80 to 100
Updated by krileon almost 13 years ago
- File 3038.patch 3038.patch added
- File cb.acl.php cb.acl.php added
Final patch submit for review.
Updated by krileon almost 13 years ago
- File 3038-pt2.patch 3038-pt2.patch added
Part 2 changes current SVN to implement a groups above function. It then checks that the "master" has a group above the "user". It then checks that the groups below "master" belong to an admin group. The new checks ensure the "master" is always above the user.
Updated by krileon almost 13 years ago
- File 3038-pt2_rev1.patch 3038-pt2_rev1.patch added
Fixed backend admins not being able to edit themselves
Updated by krileon almost 13 years ago
- File 3038-pt2_rev2.patch 3038-pt2_rev2.patch added
Updated by krileon almost 13 years ago
- File 3038-pt2_rev3.patch 3038-pt2_rev3.patch added
Updated by krileon almost 13 years ago
- File 3038-part2_rev4.patch 3038-part2_rev4.patch added
Updated by krileon almost 13 years ago
- File 3038-pt2_rev5.patch 3038-pt2_rev5.patch added
get_groups_below_me keeps the top most group you belong to plus those below you. get_groups_above_me only returned groups above you and didn't keep what you already belonged to. Fixed it so it includes those above you and the top most group you already belong to. Tested the usage experience against J1.5 and works exactly the same (admins can edit other admins in backend even if not self, confirmed j1.5 as well; so seams intended).
Updated by krileon almost 13 years ago
- File 3038-pt2_rev6.patch 3038-pt2_rev6.patch added
Updated by krileon almost 13 years ago
- File 3038-pt2_rev7.patch 3038-pt2_rev7.patch added
Updated by beat almost 13 years ago
- Subject changed from get_user_permission not checking full access tree to J2.5: get_user_permission not checking full access tree
- Status changed from Resolved to Closed
- Target version set to CB 1.8
- Estimated time set to 6:00 h
Fixed exactly as last patch proposed in r1168 / r1169 for comment.
Thank you very much Kyle for your patience during this review and for the improved proposals and tests.