Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access to admin menu entries if not member of the sys admin group #5015

Closed
marc-farre opened this issue Apr 18, 2021 · 7 comments
Closed

Access to admin menu entries if not member of the sys admin group #5015

marc-farre opened this issue Apr 18, 2021 · 7 comments
Assignees

Comments

@marc-farre
Copy link
Collaborator

$entry->isVisible = true;

What steps will reproduce the problem?

I will take the example of the Custom pages module, but the problem is the same for other modules.

A user that is not member of the system admin group, but is member of a group for which the permissions "Can manage custom pages" is set to "Allow":
image

What is the expected result?

This user should see the "Custom pages" menu entry in the admin menu:
image

What do you get instead?

The page is accessible: /custom_pages/page (this is normal).
But the menu entry is not show.

Additional info

The cause is somewhere here: https://github.com/humhub/humhub/blob/master/protected/humhub/modules/admin/widgets/AdminMenu.php#L165

public function addEntry(MenuEntry $entry)
    {
        if (!$entry->isVisibilitySet()) {
            $entry->setIsVisible(Yii::$app->user->can(ManageModules::class));
        }

This user cannot manage modules and the entry is not set to visible, as it is not possible to make if visible because of this method:

public static function createByArray($item)

Q A
HumHub version 1.8.1
@yurabakhtin
Copy link
Contributor

@luke- Fixed in PR #5034.

This bug is related only for cases where menu entries are initialized by deprecated method Menu->addItem([...]) like we still have in the custom_pages and many other modules. But there is no bug where we use new method Menu->addEntry(new MenuLink([...])) even without current fix.

@marc-farre
Copy link
Collaborator Author

Thanks @yurabakhtin. I confirm this resolves the problem.

@marc-farre
Copy link
Collaborator Author

@yurabakhtin a problem is remaining: if the permission "Can manage custom pages" is set to "Allow" and all others permissions are set to default (e.g. "Manage users" is set to "Deny"), the menu entry "Administration" is shown in the account menu (this is normal), but when you clic on the menu entry, you arrive to /admin and you have a message telling you that you do not have the permission to access to this page.

@marc-farre marc-farre reopened this Apr 22, 2021
@yurabakhtin
Copy link
Contributor

@funkycram Yes, the problem is still remaining.

@luke- The url /admin is the controller protected/humhub/modules/admin/controllers/IndexController.php that has access rules with ['permissions' => Yii::$app->getModule('admin')->getPermissions()], so user must has at least one of the permissions:

  • ManageModules
  • ManageSettings
  • SeeAdminInformation
  • ManageUsers
  • ManageGroups
  • ManageSpaces

The custom pages menu link can be displayed when user has permission ManageModules or ManagePages, but we cannot include the ManagePages into the getModule('admin')->getPermissions() because it is permission from external module.

So if we need to fix this issue I think we should remove the permission rule completely from the admin index controller because this controller has only single action where we also check first allowed menu entry, i.e. if user has no access to any admin menu event from external module the /admin will restricted as well.
Do you agree to remove the rule from here https://github.com/humhub/humhub/blob/master/protected/humhub/modules/admin/controllers/IndexController.php#L36?

@marc-farre
Copy link
Collaborator Author

It should have the sames conditions as the canAccess() method of the AdminMenu widget. See

public static function canAccess()

@luke-
Copy link
Contributor

luke- commented Apr 22, 2021

@yurabakhtin Yes, good point. i have already removed the permissions and committed it.
@funkycram Can you test it with the latest master?

@luke- luke- closed this as completed in 1ae0a64 Apr 22, 2021
@marc-farre
Copy link
Collaborator Author

I confirm it does work fine. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants