Pass entity to checkAccess method to allow Symfony Voters check the r…

[juanwilde]

In case of getEntity we need to pass the entity to the checkAccess method in order to check some values within Symfony Voters (To check for example y the logged user is the owner of the requested entity

[moein]

I think the same applies to update entity right?

[juanwilde]

Yes. It is for read, update and delete to check if the entity is owned by the user given in the Token

[moein]

$i ++ kind of looks wrong

[juanwilde]

It is the PSR-2 code styling. I will remove this kind of styling before pushing changes

[moein]

This formatting looks way too forced
Maybe you can move the calculation out to a function and that should fix the formatting also makes it more readable

[moein]

Thanks for the PR
I've addressed some issues that imo need to be fixed

[juanwilde]

Updated the code by adding logic to update and also delete an entity

[trivago-msanchez]

This formatting is not respecting the coding style used in the rest of the library

[moein]

@juanwilde Not sure if this is actually going to be a major change or not
If I setup a simple project and say anyone user with ROLE_READ can get any objects and I don't put any voter what does denyAccessUnlessGranted returns?

[juanwilde]

As I see If you don't put any voter the library will work as always since $entity is null by default. The problem is if we always pass null in the entity param, we are not able to check some stuff.. This is a working example of what I mean

    protected function voteOnAttribute($attribute, $subject, TokenInterface $token): bool
    {
        /** @var User|AnotherUser $user */
        $user = $token->getUser();

        if ($user instanceof Anotheruser) {
            return false;
        }

        if (self::READ === $attribute) {
            if ((null === $subject) || (null !== $subject && $subject->isOwnedBy($user))) {
                return true;
            }
        }

        if (self::CREATE === $attribute) {
            return true;
        }

        if (self::UPDATE === $attribute || self::DELETE === $attribute) {
            if ($subject->isOwnedBy($user)) {
                return true;
            }
        }

        return false;
    }

With your approach the $subject is always null and you can not perform any action like check if the logged user is the owner... Makes sense?

[juanwilde]

Ok. I tried the following thing. I have 2 entities, one goes throw a Voter and the other one not. The behaviour is the expected. I you try to get one object of the entity that doesn't have Voter, it works as soon as you define the ROLE_READ in the jade.yml. And for the other one, the same but passing throw the voter after.

I mean. With these changes, the library still working fine if you only use jade.yml and the listeners. But if you add a Voter it will work as well.

[moein]

@juanwilde Sounds good
Did you run functional test in the bundle?

[juanwilde]

Yes. I did it following the package instructions and everythink ok. Just a couple of deprecations notices

1x: The Symfony\Component\ClassLoader\ClassCollectionLoader class is deprecated since Symfony 3.3 and will be removed in 4.0.
    1x in Application::run from Codeception

  1x: The "sensio_framework_extra.routing.loader.annot_class" service is deprecated since version 5.2
    1x in Application::run from Codeception

  1x: The "Sensio\Bundle\FrameworkExtraBundle\Routing\AnnotatedRouteControllerLoader" class is deprecated since version 5.2. Use "Symfony\Bundle\FrameworkBundle\Routing\AnnotatedRouteControllerLoader" instead.
    1x in Application::run from Codeception

  1x: The "sensio_framework_extra.routing.loader.annot_dir" service is deprecated since version 5.2
    1x in Application::run from Codeception

  1x: The "sensio_framework_extra.routing.loader.annot_file" service is deprecated since version 5.2
    1x in Application::run from Codeception