Skip to content
This repository has been archived by the owner on Jul 3, 2020. It is now read-only.

Problem with Doctrine & Guards (not) persisting permissions #106

Closed
aeneasr opened this issue Dec 3, 2013 · 12 comments
Closed

Problem with Doctrine & Guards (not) persisting permissions #106

aeneasr opened this issue Dec 3, 2013 · 12 comments

Comments

@aeneasr
Copy link
Contributor

aeneasr commented Dec 3, 2013

When trying to update an user, I get: "A new entity was found through the relationship 'User\Entity\Role#permissions' that was not configured to cascade persist operations for entity: controller.user\controller\usercontroller.settings. To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity or configure cascade persist this association in the mapping for example @manytoone(..,cascade={"persist"})."

Obviously, the User has a m:n relation to Role and Role has a m:n relation to Permission. The Controller throwing the exception is User\Controller\UserController::settingsAction().

My zfcrbac config looks like this:

return [
    'zfc_rbac' => [
        'role_providers' => [
            'ZfcRbac\Role\ObjectRepositoryRoleProvider' => [
                'object_manager' => 'doctrine.entitymanager.orm_default',
                'class_name' => 'User\Entity\Role'
            ]
        ],
        'protection_policy' => GuardInterface::POLICY_ALLOW
    ]
];

Role:addPermission:

public function addPermission($name)
    {
        $permission = new Permission();
        $permission->setName($name);
        $this->permissions->add($permission);
        return $this;
    }

The problem originates in ControllerGuard::isGranted() / RouteGuard::isGranted()

// Load the needed permission inside the container
$this->loadRule($allowedRoles, $permission);

I am unsure if this is expected behaviour (guards persisting permissions) but it doesn't seem to me that way.

@bakura10
Copy link
Member

bakura10 commented Dec 3, 2013

I'm not sure to understand. ZfcRbac never flushes or something. Your solution of cascading persist is indeed the right one.

@aeneasr
Copy link
Contributor Author

aeneasr commented Dec 3, 2013

That is true, ZfcRbac doesn't flush, but my UserManager does. Maybe I have to elaborate this a little bit more:

User, Role and Permission are entities managed by Doctrines EntityManager. When AbstractGuard::loadRule('__controller__.user\controller\usercontroller.settings') (called by RouteGuard) executes $rbac->getRole($role)->addPermission($permission);, Role::addPermission() creates a new Permission entity. Doctrine catches that and throws an error, because I did not persist that permission.

In my eyes it would be unwise to persist the new permission as it is only needed by the guards and therefore shouldn't be written to the database. Using 'cascade=(persist)' would solve the Exception but it would also (possibly) create many new permission in the database.

Changing the guards configuration could in result lead to unexpected behaviour:

  • Database returns ['login'] for __controller__.user\controller\usercontroller.settings
  • Guard configuration returns ['admin'] for User\Controller\UserController::settings()

It would however be possible, that addPermission creates a model not handled by doctrine (something like InMemoryRole) but that would be a quite ugly fix.

@bakura10
Copy link
Member

bakura10 commented Dec 3, 2013

Haa I see the problem... Nice finding! Actually, those permissions are just added internally to take advantage of the fact that roles have inheritance. But those permissions have no real meaning. I will try to find a fix to that. Thanks!

@aeneasr
Copy link
Contributor Author

aeneasr commented Dec 3, 2013

Sure, no problem! I'd suggest to use an InMemoryPermission, shipped by zfcrbac, for that purpose.

@bakura10
Copy link
Member

bakura10 commented Dec 3, 2013

Hhmm. can you try to provide a fix? :)

@aeneasr
Copy link
Contributor Author

aeneasr commented Dec 3, 2013

Ah I understand now how it is supposed to work. The only thing I could think of, was to deprecate AbstractGuard::loadRule() in favour of:

AbstractGuard::isAllowed()

    protected function isAllowed(array $allowedRoles)
    {
        return count(array_filter($this->authorizationService->getIdentityRoles(), function($e) use($allowedRoles){
            if($e instanceof \Zend\Permissions\Rbac\RoleInterface){
                /* @var $e \Zend\Permissions\Rbac\RoleInterface */
                $e = $e->getName();
            }
            return in_array($e, $allowedRoles);
        })) !== 0;
    }

And to adjust ControllerGuard::isGranted() and RouteGuard::isGranted() appropriately:

        // Load the needed permission inside the container
        //$this->loadRule($allowedRoles, $permission); // deprecated

        return $this->isAllowed($allowedRoles);

Not sure if thats the way to go tho. And please excuse the CRAP in isAllowed. Maybe you can think of a better (cleaner) way of doing this? Also, the only way (I can think of) of reducing the complexity would be to sort the arrays first. On the other hand, O(n^2) should be okay for such small arrays (I don't think anyone would use > 20 roles).

@aeneasr
Copy link
Contributor Author

aeneasr commented Dec 3, 2013

Got it, that's probably better:

    protected function isAllowed(array $allowedRoles)
    {
        foreach($allowedRoles as $checkRole){
            if($this->authorizationService->getRbac()->hasRole($checkRole))
                return true;
        }
        return false;
    }

@bakura10
Copy link
Member

bakura10 commented Dec 3, 2013

Looks good to me. Can you make a PR please ? :)

@aeneasr aeneasr mentioned this issue Dec 3, 2013
bakura10 added a commit that referenced this issue Dec 7, 2013
@BrunoSpy
Copy link
Contributor

BrunoSpy commented Dec 8, 2013

@arekkas could you please share your entities Role and Permission ?
Those in datas/ aren't complete and don't work properly.
Thanks !

@aeneasr
Copy link
Contributor Author

aeneasr commented Dec 8, 2013

Hm? This issue is already closed :)

@BrunoSpy
Copy link
Contributor

BrunoSpy commented Dec 8, 2013

Yes I know that. But as you managed to create entities that are working with ZfcRbac 1.0 and those provided here in datas/ aren't usable, I thought it would be a great idea to share these entities.

@bakura10
Copy link
Member

bakura10 commented Dec 8, 2013

Why aren't they valid? What error do you have?

Envoyé de mon iPhone

Le 8 déc. 2013 à 19:40, BrunoSpy [email protected] a écrit :

Yes I know that. But as you managed to create entities that are working with ZfcRbac 1.0 and those provided here in datas/ aren't usable, I thought it would be a great idea to share these entities.


Reply to this email directly or view it on GitHub.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants