EasyAdminBundle icon indicating copy to clipboard operation
EasyAdminBundle copied to clipboard

Entities filtered out by setEntityPermission cause null values to be passed down to formatValue callback and Action setPermission voters

Open mmjr-x opened this issue 1 year ago • 5 comments

Describe the bug We are currently running into an issue where our easy admin pages are crashing because entities marked as inAcessible by the setEntityPermission configuration are still being passed down to downstream callbacks like the ones used by Action setPermission voters and formatValue configurations on Fields.

An issue of a similar nature was fixed in https://github.com/EasyCorp/EasyAdminBundle/pull/5963 however this fix only works for the broken column sorting and not the other checks I mentioned above.

To Reproduce Easy admin version used: 4.8.6

Minimal setup to reproduce this issue for formatValue

  • Set up a Language Entity with a name text field
  • Set up the following Voter and crudController:
class RejectingVoter extends Voter
{
    final public const LANGUAGE_VIEW = 'language_view';
    protected function supports(string $attribute, mixed $subject): bool
    {
        return $attribute === self::LANGUAGE_VIEW && $subject instanceof Language;
    }

    protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool
    {
        // Always deny access
        return false;
    }
}
class Language2CrudController extends AbstractCrudController
{
    public static function getEntityFqcn(): string
    {
        return Language::class;
    }

    public function configureCrud(Crud $crud): Crud
    {
        return parent::configureCrud($crud)
            ->setEntityPermission(RejectingVoter::LANGUAGE_VIEW);
    }


    public function configureFields(string $pageName): iterable
    {
        return [
            IdField::new('id'),
            TextField::new('name')->formatValue(fn(string $name, Language $language) => sprintf('This should not be called: %s',$name)),
        ];
    }
}
  • Go to the entity's index page in easy admin's and observe the following error:
App\Controller\Admin\Language2CrudController::App\Controller\Admin\{closure}(): Argument #2 ($language) must be of type App\Entity\Language, null given, called in /usr/src/app/vendor/easycorp/easyadmin-bundle/src/Field/Configurator/CommonPostConfigurator.php on line 54

A similar situation occurs when you have an Action setPermission voters that get feed an entity as the subject,

  • Set up an entity with a crud controller with a setEntityPermission voter that always returns false
  • Add an action voter for Action::EDIT that is linked to a voter where the its canUpdate does not allow for a null value to be passed as its subject private function canUpdate(User $user, SubjectEntity $subject): bool
  • Go to the entities crud controller in easy admin, and you will get an error.

mmjr-x avatar Dec 11 '23 10:12 mmjr-x

Seems that the entities are marked as inaccessible in EntityFactory::createCollection but EntityFactory::processFields is still called for inaccessible entities.

Rme2001 avatar Dec 11 '23 10:12 Rme2001

I did some more debugging, and it seems like changing the following 2 methods in the EntityFactory to include an isAccessible check solves the issues:

public function processFields(EntityDto $entityDto, FieldCollection $fields): void
    {
        if (!$entityDto->isAccessible()) {
            return;
        }
        $this->fieldFactory->processFields($entityDto, $fields);
    }
public function processActions(EntityDto $entityDto, ActionConfigDto $actionConfigDto): void
    {
        if (!$entityDto->isAccessible()) {
            return;
        }
        $this->actionFactory->processEntityActions($entityDto, $actionConfigDto);
    }

I am unsure if this is the correct place to do this though as this disables processing of the fields and actions entirely if the entity is not accessible (which might lead to unintended side effects).

If a maintainer can shed some light on this and deems this solution acceptable I am willing to raise a PR with this change?

mmjr-x avatar Dec 11 '23 13:12 mmjr-x

Turns out my above patch breaks the filter views (because renderFilters in the AbstractCrudController seems to call upon an entityDto to get the list of fields for its setup, which in turn gets populated using the processFields method) , so I moved the check to the processFieldsForAll and processActionsForAll, which seems to work better:

public function processFieldsForAll(EntityCollection $entities, FieldCollection $fields): void
    {
        foreach ($entities as $entity) {
            if (!$entity->isAccessible()) {
                continue;
            }
            $this->processFields($entity, clone $fields);
            $entities->set($entity);
        }
    }
public function processActionsForAll(EntityCollection $entities, ActionConfigDto $actionConfigDto): ActionCollection
    {
        foreach ($entities as $entity) {
            if (!$entity->isAccessible()) {
                continue;
            }
            $this->processActions($entity, clone $actionConfigDto);
        }

        return $this->actionFactory->processGlobalActions($actionConfigDto);
    }

It might be even better to just do this in the AbstractCrudController instead as it is more explicit.

mmjr-x avatar Dec 11 '23 15:12 mmjr-x

@javiereguiluz Not to be a naggy, however if you could take a look at this and the above fix is deemed an acceptable fix I would be happy to raise a PR for this?

mmjr-x avatar Feb 29 '24 09:02 mmjr-x

I added this as a patch for my project and it helped me so much! Thanks!

LarsShift avatar Apr 25 '24 15:04 LarsShift