Anonymous Login
2019-01-17 05:12 PST

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0001683v3.4 Release (Current)Core Infrastructurepublic2019-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

-Relationships
+Relationships

-Notes

~0003764

Themoulos (reporter)

thanks for the correction.

~0003765

karstenmtr (reporter)

Last edited: 2016-04-15 13:17

View 3 revisions

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 (reporter)

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 (reporter)

Last edited: 2016-04-15 13:26

View 2 revisions

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 (reporter)

It's always nice when another pair of eyes verifies! Thanks karstenmtr

~0003865

caseydk (administrator)

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 (administrator)

In the 31 Dec 2018 release: http://docs.web2project.net/release-notes/3.4.html
+Notes

-Issue History
Date Modified Username Field Change
2016-02-02 21:21 stevez New Issue
2016-02-07 12:16 Themoulos Note Added: 0003764
2016-04-14 22:41 karstenmtr Note Added: 0003765
2016-04-15 13:16 karstenmtr Note Edited: 0003765 View Revisions
2016-04-15 13:17 karstenmtr Note Edited: 0003765 View Revisions
2016-04-15 13:21 stevez Note Added: 0003767
2016-04-15 13:26 karstenmtr Note Added: 0003768
2016-04-15 13:26 karstenmtr Note Edited: 0003768 View Revisions
2016-04-15 13:29 stevez Note Added: 0003769
2016-12-26 23:34 caseydk Project v3.3 Release => v3.4 Release (Current)
2016-12-29 23:49 caseydk Assigned To => caseydk
2016-12-29 23:49 caseydk Status new => assigned
2016-12-31 00:03 caseydk Status assigned => resolved
2016-12-31 00:03 caseydk Resolution open => fixed
2016-12-31 00:03 caseydk Note Added: 0003865
2019-01-03 12:53 caseydk Note Added: 0003951
2019-01-03 12:53 caseydk Status resolved => closed
+Issue History