RedirectStrategy doesn't work with "ZF3" Eventmanager

[DennisDobslaf]

I've installed version 2.6.3 of zendframework/zend-eventmanager (method 'triggerEvent' is available => your condition in https://github.com/ZF-Commons/zfc-rbac/blob/master/src/ZfcRbac/Guard/AbstractGuard.php#L76). So ZfcRbac decides to go the "ZF3 EventManager way".

But the method isn't new in Version 3 of the EventManager!

In my case, this leads to a non executed RedirectStrategey, because "stopPropagation" is not set to false as a default and the eventmanager stops the execution right after the very first attached listener.

Asking for a method to exist is a bad idea for checking the version in my eyes. Unfortunately zf eventmanager doesn't provide version information via the interface. Ignoring the fact, setEventPrototype seems to be a new (public) method in ZF3 EventManager.

Changing Line 76 to: if (method_exists($eventManager, 'setEventPrototype')) { works for me.

[svycka]

hmm yep this whole check is not necessary could be just:

$event->setName(MvcEvent::EVENT_DISPATCH_ERROR);
$eventManager->triggerEvent($event);

because this would work with EventManager v2 and v3

[DennisDobslaf]

L71 should be $event->stopPropagation(false); then.

Because v2 Eventmanager doesn't set this in triggerEvent but in trigger (which wouldn't be called anymore).

Added: v3 Eventmanager sets stopPropagation to false in triggerListeners which v2 doesn't.

That's a weired switch... from v2 to v3

[svycka]

I think this by default is false but maybe I am wrong.

[svycka]

woops missed the point :D hmm just a quick look but this $event->stopPropagation(true); also not required haven't tested but why we need to stop? again missing something? :)

[DennisDobslaf]

Off course, you are right. false is the default value.

So, changing the lines like your comment and deleting stopPropagation(true) works (for me) with v2. Currently i'm not able to validate this with v3.

This is the complete AbstractGuard::onResult

/**
 * @private
 * @param  MvcEvent $event
 * @return void
 */
public function onResult(MvcEvent $event)
{
    if ($this->isGranted($event)) {
        return;
    }

    $event->setError(self::GUARD_UNAUTHORIZED);
    $event->setParam('exception', new Exception\UnauthorizedException(
        'You are not authorized to access this resource',
        403
    ));

    $application  = $event->getApplication();
    $eventManager = $application->getEventManager();

    $event->setName(MvcEvent::EVENT_DISPATCH_ERROR);
    $eventManager->triggerEvent($event);
}

Added: Guess you can't be sure what happend to $event previously

[svycka]

guess should work, even if not this looks like a bug and should be like this then:

$event->setName(MvcEvent::EVENT_DISPATCH_ERROR);
$eventManager->triggerEvent($event);
$event->stopPropagation(true);

to be really sure MvcEvent::EVENT_ROUTE is really stopped but can't imagine situation when we have MvcEvent::EVENT_DISPATCH_ERROR and continue with dispatch process.

[DennisDobslaf]

stopPropagation should only be changed in the called listeners i guess. And be the default previously.

[svycka]

@DennisDobslaf yes I know, but imagine situation when RouteGuard detects that permission is not granted then fires MvcEvent::EVENT_DISPATCH_ERROR and this does not stop dispatch process(does not return response because of some buggy listener) then MvcEvent::EVENT_ROUTE will call next listener if $event->stopPropagation(true); not set and route will be found allowing user to access this protected route. I am not 100% sure this is possible, but well if permission is not granted then must stop propagation at least or return default Response with error to prevent further processing MvcEvent::EVENT_ROUTE event. Unless we believe that

$event->setName(MvcEvent::EVENT_DISPATCH_ERROR);
$eventManager->triggerEvent($event);

will 100% stop dispatch process and will return error message. But Imagine situation when we don't have any listeners for MvcEvent::EVENT_DISPATCH_ERROR then we have a problem.

Ok this is just a corner case situation and I am not even sure this is possible, but anyway this should be tested and fixed.