AclSonataAdmin and Inheritance

[StephanePate]

I have a bundle like the SonataProductBundle where in the sandbox "Travels" and "Goodies" entities have the "Product" entity as parent. When I enable the AclSonataAdminExtension and go to the ProductAdmin, I have an empty list of Products (even if I am OWNER of a list of sub-entities Travels & Goodies for example) because the Acl is managed at the entity level I guess.

Is there a way to avoid this ?

[dunglas]

This bundle doesn't support inheritance (at least, AFAIK this has never been tested). But feedback and PRs about inheritance support are very welcome.

[StephanePate]

Thanks for your quick answer, I'll try to find a way and feed you back if I get something functional !

[StephanePate]

overriding the postPersist method of carAdmin (sandbox) with:

    /**
     * {@inheritdoc}
     */
    public function postPersist($object)
    {
        if ($this->isAclEnabled()) {
            $objectIdentity = new ObjectIdentity($object->getId(), get_parent_class($object));
            $acl = $this->getSecurityHandler()->createAcl($objectIdentity);

            $user = $this->tokenStorage->getToken()->getUser();
            $securityIdentity = UserSecurityIdentity::fromAccount($user);

            $this->getSecurityHandler()->addObjectOwner($acl, $securityIdentity);
            $this->getSecurityHandler()->addObjectClassAces($acl, $this->getSecurityHandler()->buildSecurityInformation($this));
            $this->getSecurityHandler()->updateAcl($acl);           
        }
    }

seems to do the trick (the car list is not empty anymore...), with injection of tokenStorage in the admin class.

It could be done exactly the same way but with a very sligth modification of the createObjectSecurity method of AclSecurityHandler class if it could allow creation of the $objectIdentity = new ObjectIdentity($object->getId(), get_parent_class($object)) ... What do you think ?

[vbartusevicius]

Looking for same functionality.
It seems https://github.com/MrGreenStuff/MrGreenStuffAclSonataAdminExtensionBundle doing exactly what's needed.
Somehow the code looks a bit unoptimized, but will give it a try.

[dunglas]

Can you open a PR to add this feature upstream? Cc @MrGreenStuff

[StephanePate]

Not really the same issue I think : MrGreen addresses the "embedded admin" (with relation 1 to N between "Parent" and "Child" Entities (that are both concrete Classes without inheritance between both).
My concern is between "Parent" and "Child" Entities where the parent is possibly an Abstract Class (like car in the sandbox) and the child is a concrete Class extending the parent.

As the ACL Classes are totally seggregated from the DomainObjects, my proposal is to accept (in AclSecurityHandler) that an abstract class in the DomainObject (the parent) would have acl entries (for the object ids of their children) attached to it, as if it was a concrete one.

[vbartusevicius]

@dunglas - it seems that https://github.com/MrGreenStuff/MrGreenStuffAclSonataAdminExtensionBundle is a bit abandoned. Is it possible to merge that fork here without the author itself?

[dunglas]

Yes as long as it's the same license it should be ok.

[MrGreenStuff]

@dunglas - I've already propose a PR 2 year ago with my fork but it was not accepted because it's for specific case. My Fork it's not well optimized but now you can use this bundle https://github.com/GoDisco/AclTreeBundle and modify my bundle to use it or make a new fork of this bundle and use it in. It's will be more optimized and usable not only in the sonata admin.

[MrGreenStuff]

The old PR if you want to merge : #10

[vbartusevicius]

I already implemented a Voter to forbid accessing items, whitch are not visible by @MrGreenStuff implementation. There currenty is a security issue: if user opens an item for editing, and then he changes the id in URL and submits, then in edit form a requested item will be loaded, ignoring the fact that he does not have permissions to edit.

[vbartusevicius]

@dunglas, any news on merging that PR?

[dunglas]

If you're speaking about #10, it needs some work before being merged (see my comments on that PR).

I'll take a look at the security issue asap.