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

Custom Voter on new action to have also the entity #5133

Open
yalit opened this issue Mar 19, 2022 · 7 comments · May be fixed by #6061
Open

Custom Voter on new action to have also the entity #5133

yalit opened this issue Mar 19, 2022 · 7 comments · May be fixed by #6061

Comments

@yalit
Copy link
Contributor

yalit commented Mar 19, 2022

Following #4390, action permissions are checked with the entity BUT the new action sends a "null" entity.

The issue is that if we want to restrict a Custom Voter to a specific entity, it's not possible for the new action...

Suggestion is to move the creation of the entity in the context before the authorization check in:

if (!$this->isGranted(Permission::EA_EXECUTE_ACTION, ['action' => Action::NEW, 'entity' => null])) {
throw new ForbiddenActionException($context);
}
if (!$context->getEntity()->isAccessible()) {
throw new InsufficientEntityPermissionException($context);
}
$context->getEntity()->setInstance($this->createEntity($context->getEntity()->getFqcn()));

like this :

        $context->getEntity()->setInstance($this->createEntity($context->getEntity()->getFqcn()));
        if (!$this->isGranted(Permission::EA_EXECUTE_ACTION, ['action' => Action::NEW, 'entity' => $context->getEntity()])) {
            throw new ForbiddenActionException($context);
        }


        if (!$context->getEntity()->isAccessible()) {
            throw new InsufficientEntityPermissionException($context);
        }

I'm happy to push a PR for this if accepted

@quentinCharrier
Copy link

quentinCharrier commented Oct 20, 2022

Hello,

I got the same problem, as soon as i use configureCrud, in this case I'm trying to limit the access to the user entity to admin and the owner (the user himself) :

public function configureCrud(Crud $crud): Crud
{
        parent::configureCrud($crud);
        $crud->setEntityPermission('ADMIN_OR_OWNER_EDIT');

        return $crud;
}

And then create a customVoter :

<?php

namespace App\Security\Voter;

use App\Entity\User;
use LogicException;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
use Symfony\Component\Security\Core\Security;

class UserVoter extends Voter
{
    private Security $security;

    public function __construct(Security $security)
    {
        $this->security = $security;
    }

    protected function supports(string $attribute, $subject): bool
    {
        return in_array($attribute, ['ADMIN_OR_OWNER_EDIT')
            && $subject instanceof User;
    }

    protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token): bool
    {
        $user = $token->getUser();

        if (!$user instanceof User) {
            throw new LogicException('Subject is not an instance of User?');
        }

        switch ($attribute) {
            case 'ADMIN_OR_OWNER_EDIT':
                return $user === $subject || $this->security->isGranted('ROLE_ADMIN');
            default:
                throw new LogicException(sprintf('This code should not be reached : %s', $attribute));
        }

        // return false;
    }
}

Then when trying to access to the action new I got the following error :

You don't have enough permissions to access this instance of the "App\Entity\User" entity.

Did you find any solutions ?

@yalit
Copy link
Contributor Author

yalit commented Oct 21, 2022

yes, the workaround is in your supports function allow even if it's not an instance of the entity (in your case User) and then in the voteOnAttribute check if the $subject is null or not and then handle the security with that...

but it means that you need to be conscious that this Voter is used only in specific places

@quentinCharrier
Copy link

I'll try this, thanks for the reply. Any idea how to limit this to the "new" actions ?

@yalit
Copy link
Contributor Author

yalit commented Oct 26, 2022

you can also set the permission per action... I've not tested to assign the global permission and then the permission for a specific action... it might override the first one and then setup a specific permission for the new action (to be tested as said, just a thought)

@gregoire-jianda
Copy link

In a similar fashion, it would be great to have more context on actions without an explicit entity.
For example, in the index action :

if (!$this->isGranted(Permission::EA_EXECUTE_ACTION, ['action' => Action::INDEX, 'entity' => null])) {
    throw new ForbiddenActionException($context);
}

There is no way for the Voter to determine which Action::INDEX it is actually voting on. Adding the controller Fcqn for example would make this far more useful 🙏

@DevertNet
Copy link

+1

@yalit
Copy link
Contributor Author

yalit commented Dec 9, 2023

@gbelorgey the issue here is that the Voter mechanism is based on the VoterInterface of Symfony which does not support more than 2 parameters for the supports function ($attribute & $subject)
By default, the subject is the data you are trying to access (here the entity) => Changing this would bring a potential BC issue with all the voters already implemented

Anyway, if your voter if specific to that controller, I would define a specific attribute in your specific Voter and assign it as permission to the new action in your controller like something

public class SpecificControllerVoter extends Voter {
    public const SPECIFIC_PERMISSION = "<specificController-or-specificContext>_NEW";

    public function supports(string $attribute, $subject) {
        return $attribute === self::SPECIFIC_PERMISSION;
    }

...
}

and in the controller in your configureActions function

$actions->setPermission(Action::NEW, SpecificController::SPECIFIC_PERMISSION);

That way your voter will know that it's only for that specific permission

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