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

Ajax toggle publish status action has incorrect permission check. #13683

Open
1 task done
mallezie opened this issue Apr 23, 2024 · 1 comment
Open
1 task done

Ajax toggle publish status action has incorrect permission check. #13683

mallezie opened this issue Apr 23, 2024 · 1 comment
Assignees
Labels
bug Issues or PR's relating to bugs roles Anything related to users and roles T1 Low difficulty to fix (issue) or test (PR)

Comments

@mallezie
Copy link
Contributor

mallezie commented Apr 23, 2024

Mautic Version

5.0.x series

Way of installing

I installed with composer using https://github.com/mautic/recommended-project

PHP version

8.1

What browsers are you seeing the problem on?

Not relevant

What happened?

When a user does not have the full access to an entity he cannot use the toggle publish status action.

How can we reproduce this issue?

Step 1: Remove administrator / full access to contact segments for a user. Give edit / view permission.
Try to toggle publish status in segment list overview. See access denied, while this should be granted.

Relevant log output

No response

Code of Conduct

  • I confirm that I have read and agree to follow this project's Code of Conduct




Care about this issue? Want to get it resolved sooner? If you are a member of Mautic, you can add some funds to the Bounties Project so that the person who completes this task can claim those funds once it is merged by a member of the core team! Read the docs here.

@mallezie mallezie added bug Issues or PR's relating to bugs needs-triage For new issues/PRs that need to be triaged labels Apr 23, 2024
@mallezie
Copy link
Contributor Author

The problem lies in the Ajax controllers check for toggle publishStatus action.

This part checks the permissions.

if ($security->checkPermissionExists($permissionBase.':publishown')) {
                $hasPermission = $security->hasEntityAccess($permissionBase.':publishown', $permissionBase.':publishother', $createdBy);
            } elseif ($security->checkPermissionExists($permissionBase.':publish')) {
                $hasPermission = $security->isGranted($permissionBase.':publish');
            } elseif ($security->checkPermissionExists($permissionBase.':manage')) {
                $hasPermission = $security->isGranted($permissionBase.':manage');
            } elseif ($security->checkPermissionExists($permissionBase.':full')) {
                $hasPermission = $security->isGranted($permissionBase.':full');
            } elseif ($security->checkPermissionExists($permissionBase.':editown')) {
                $hasPermission = $security->hasEntityAccess($permissionBase.':editown', $permissionBase.':editother', $createdBy);
            } elseif ($security->checkPermissionExists($permissionBase.':edit')) {
                $hasPermission = $security->isGranted($permissionBase.':edit');
            } else {
                $hasPermission = false;
            }

What this does, is it checks if the full permission exists, which it does. Then checks if permission is granted (which is not), and thus skips the further checks.
Not sure how to fix this securely, so posting an issue for it.

If somebody can provide some pointers to existing code where this is done correctly i'm happy to create a PR.

I think the simplest adjustment is keep checking untill we encounter true, so that would be something like below.

$hasPermission = false;
if ($security->checkPermissionExists($permissionBase.':publishown') && !$hasPermission) {
                $hasPermission = $security->hasEntityAccess($permissionBase.':publishown', $permissionBase.':publishother', $createdBy);
            } 
if ($security->checkPermissionExists($permissionBase.':publish') && !$hasPermission) {
                $hasPermission = $security->isGranted($permissionBase.':publish');
            } 
if ($security->checkPermissionExists($permissionBase.':manage') && !$hasPermission) {
                $hasPermission = $security->isGranted($permissionBase.':manage');
            } 
if ($security->checkPermissionExists($permissionBase.':full') && !$hasPermission) {
                $hasPermission = $security->isGranted($permissionBase.':full');
            } 
if ($security->checkPermissionExists($permissionBase.':editown') && !$hasPermission) {
                $hasPermission = $security->hasEntityAccess($permissionBase.':editown', $permissionBase.':editother', $createdBy);
            } 
if ($security->checkPermissionExists($permissionBase.':edit') && !$hasPermission) {
                $hasPermission = $security->isGranted($permissionBase.':edit');
            }

@RCheesley RCheesley added roles Anything related to users and roles T1 Low difficulty to fix (issue) or test (PR) and removed needs-triage For new issues/PRs that need to be triaged labels Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs roles Anything related to users and roles T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Development

No branches or pull requests

3 participants