disallow non-Superusers to manage admins #986
Conversation
|
Hmm, how annoying the CI fails again. |
There was a problem hiding this comment.
This looks ok. It is removing the ability for an ordinary admin to edit/view their own admin details though. I don't use ordinary admins so don't know how important that might be. Perhaps that can be put back later but removing the current ability to change their own privileges.
public_html/lists/admin/connect.php
Outdated
| @@ -808,6 +801,17 @@ function pageTitle($page) | |||
| $GLOBALS['pagecategories']['system']['pages'][] = 'update'; | |||
| $GLOBALS['pagecategories']['system']['menulinks'][] = 'update'; | |||
| } | |||
| if (isSuperUser()) { | |||
| foreach (array('admins','admin','importadmin','adminattributes') as $adminPage) { | |||
| $GLOBALS['pagecategories']['config']['menulinks'][] = $adminPage; | |||
There was a problem hiding this comment.
This is adding an extra page to the Config menu. Currently that menu does not include the admin page.
There was a problem hiding this comment.
Yes, but that's not a big issue.
public_html/lists/admin/connect.php
Outdated
| @@ -808,6 +801,17 @@ function pageTitle($page) | |||
| $GLOBALS['pagecategories']['system']['pages'][] = 'update'; | |||
| $GLOBALS['pagecategories']['system']['menulinks'][] = 'update'; | |||
| } | |||
| if (isSuperUser()) { | |||
There was a problem hiding this comment.
When you login the displayed Config menu does not include these items. They are shown only when you display a page after logging-in. I noticed that for other menu items in the past but never tried to figure out why.
The reason is that these statements are run before the login action has been processed, so the admin isn't a super user at that point.
One way to fix this is to redirect on successfully logging-in. I can do a pull request for that.
There was a problem hiding this comment.
Interesting, I haven't noticed that.
There was a problem hiding this comment.
Ah, interesting, that must be the reason that the CI jobs failed. First time you login, it's not there. I've commented them out for now, but once you have a PR to fix that (or I) we can add that again
There was a problem hiding this comment.
My idea of redirecting on successful login works for the browser but not for the REST API plugin.
Firstly, the client needs to have enabled CURLOPT_FOLLOWLOCATION which currently isn't needed, so there is a possibility of breaking a client. The sample client doesn't do that, https://github.com/michield/phplist-restapi-client
Returning a 302 code will make the client request the page again using GET, but the REST API plugin treats a GET as a documentation page request.
Returning a 307 code will make the client request the page using the original method of POST. This does work when CURLOPT_FOLLOWLOCATION is enabled.
There was a problem hiding this comment.
The reason it takes two page loads is that accesscheck.php defines the "isSuperuser" and it only gets included in home.php after the header (with the menu) is generated. In a way, I'm puzzled it works at all, as isSuperuser should really be at a higher level. And then it does get set on first login in https://github.com/phpList/phplist3/blob/main/public_html/lists/admin/index.php#L319
Ah, but then "connect.php" has already been included, so the menu additions won't get added at that stage. So, I'll move that down a bit.
public_html/lists/admin/connect.php
Outdated
| @@ -1103,7 +1107,7 @@ function topMenu() | |||
| ) { | |||
| continue; | |||
| } | |||
|
|
|||
| var_dump($category);exit; | |||
There was a problem hiding this comment.
Ah, yes, that's oversight.
Yes, for now this resolves a security issue where a sub-admin can update the details of a superuser, which is bad. We can add that later again. |
|
This commit will make the items appear in the menu on first load: 774a075 |
|
Yes, that has fixed the problem. Now the four menu items appear after logging-in. |
|
This pull request has been mentioned on phpList Discuss. There might be relevant details there: https://discuss.phplist.org/t/3-6-14-release-candidate-ready-for-testing/9109/1 |
|
This pull request has been mentioned on phpList Discuss. There might be relevant details there: https://discuss.phplist.org/t/phplist-3-6-14-released-security-release/9158/1 |
Description
non Superusers should not be allowed to manage admins, even when they have "config" permissions
Related Issue
Screenshots (if appropriate):