MantisBT - v3.4 Release (Current)
View Issue Details
0001683v3.4 Release (Current)Core Infrastructurepublic2016-02-02 21:212019-01-03 12:53
Reporterstevez 
Assigned Tocaseydk 
PriorityhighSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
PlatformallOSallOS Versionall
Product Version 
Target VersionFixed in Version 
Summary0001683: Administrators cannot set user preferences / only user can set preferences
Descriptionhttp://support.web2project.net/questions/1951/access-denied-in-edit-user-preferences.html#comment-2621

Steps To Reproducelogin as any admin user / attempt to edit preferences for any other user ... (Access Denied)
Additional InformationModule:\modules\system\do_preference_aed.php

Line17:

     if ((!($AppUI->user_id == $pref_user) && !canEdit('admin')) && $pref_user) {
         $AppUI->redirect(ACCESS_DENIED);
     }
     

Needs to change to:

     if ($AppUI->user_id != $pref_user) {
         if (!(canEdit('users'))) {
             $AppUI->redirect(ACCESS_DENIED);
         };
     }
TagsNo tags attached.
Attached Files

Notes
(0003764)
Themoulos   
2016-02-07 12:16   
thanks for the correction.
(0003765)
karstenmtr   
2016-04-14 22:41   
(Last edited: 2016-04-15 13:17)
The mentioned fix might work, but my guess is, that this fix doesn't represent the intended behavior and do not address the underlying changes in web2project, causing this bug.

In my opinion the correct fix should be

if ((!($AppUI->user_id == $pref_user) && !canEdit('users')) && $pref_user) {
    $AppUI->redirect(ACCESS_DENIED);
}

As you can see, I just changed the parameter of the canEdit function. This is because I think, there is no admin-module anymore. The permission check inside canEdit is done against the gacl_permissons table, which doesn't contain an module called admin. But there is one named users. And I think this is the right module to check the edit users permission against.

Edit: I'd like to revoke my previous statement. It seems I was in a hurry and didn't catch the essence of the proposed fix. Thinking one more time about it, the fix including the rewritten statement is an improvement. The nested canEdit call should be an improvement, because the permission check against the database is only executed, when required.

(0003767)
stevez   
2016-04-15 13:21   
Decomposing the original "if" statement as follows yields one parsing problem.

(
  (
    !($AppUI->user_id == $pref_user)
  &&
    !canEdit('users')
   )
  && $pref_user
)

The evaluative order leaves $pref_user as a boolean which is not explicit and therefore possibly a source of problems later.

All I was suggesting in my order was that the explicit test:

($AppUI->user_id != $pref_user)

This form of the test may be more reliable (both semantically and logically).

Your mileage may vary but my original fix has been working in production for a couple months now.
(0003768)
karstenmtr   
2016-04-15 13:26   
Your completely right, stevez. I missed this today in the morning when I wrote my note, but I just edited my statement a few minutes ago as I got the point in your proposed fix. Sorry for my mistake.

(0003769)
stevez   
2016-04-15 13:29   
It's always nice when another pair of eyes verifies! Thanks karstenmtr
(0003865)
caseydk   
2016-12-31 00:03   
Nice catch!

You are correct that the short fix was replacing 'admin' with 'user' but I made another change to the canEdit to make sure the user has the ability to edit that specific user. For example, where an Admin can manage their users but not other Admins.

Resolved:
https://github.com/web2project/web2project/commit/4ce991516793d5691ee6c5e37541c2bb1e121b17
(0003951)
caseydk   
2019-01-03 12:53   
In the 31 Dec 2018 release: http://docs.web2project.net/release-notes/3.4.html

Issue History
2016-02-02 21:21stevezNew Issue
2016-02-07 12:16ThemoulosNote Added: 0003764
2016-04-14 22:41karstenmtrNote Added: 0003765
2016-04-15 13:16karstenmtrNote Edited: 0003765bug_revision_view_page.php?bugnote_id=3765#r143
2016-04-15 13:17karstenmtrNote Edited: 0003765bug_revision_view_page.php?bugnote_id=3765#r144
2016-04-15 13:21stevezNote Added: 0003767
2016-04-15 13:26karstenmtrNote Added: 0003768
2016-04-15 13:26karstenmtrNote Edited: 0003768bug_revision_view_page.php?bugnote_id=3768#r146
2016-04-15 13:29stevezNote Added: 0003769
2016-12-26 23:34caseydkProjectv3.3 Release => v3.4 Release (Current)
2016-12-29 23:49caseydkAssigned To => caseydk
2016-12-29 23:49caseydkStatusnew => assigned
2016-12-31 00:03caseydkStatusassigned => resolved
2016-12-31 00:03caseydkResolutionopen => fixed
2016-12-31 00:03caseydkNote Added: 0003865
2019-01-03 12:53caseydkNote Added: 0003951
2019-01-03 12:53caseydkStatusresolved => closed