zfcampus / zf-api-problem

BSD 3-Clause "New" or "Revised" License
27 stars 26 forks source link

Does not respect status code set on response #21

Closed svycka closed 10 years ago

svycka commented 10 years ago

I am trying to change status code to 403 on zfcRbac module like this:

namespace Application;

use Zend\EventManager\EventManagerInterface;
use Zend\Http\Response as HttpResponse;
use Zend\Mvc\MvcEvent;
use ZfcRbac\View\Strategy\AbstractStrategy;
use ZfcRbac\Exception\UnauthorizedExceptionInterface;

class ApiProblemStrategy extends AbstractStrategy
{
    /**
     * {@inheritDoc}
     */
    public function attach(EventManagerInterface $events)
    {
        $this->listeners[] = $events->attach(MvcEvent::EVENT_DISPATCH_ERROR, [$this, 'onError'], 1000);
    }

    public function onError(MvcEvent $event)
    {
        // Do nothing if no error or if response is not HTTP response
        if (!($exception = $event->getParam('exception') instanceof UnauthorizedExceptionInterface)
            || ($result = $event->getResult() instanceof HttpResponse)
            || !($response = $event->getResponse() instanceof HttpResponse)
        ) {
            return;
        }

        $response = $event->getResponse() ?: new HttpResponse();
        $response->setStatusCode(403);

        $event->setResponse($response);
    }
}

I had to increase event priority to win agains apiProblem, my guess it stops propagation, but still seems like ApiProblem overrides my status code and I dont know how can I make it work or maybe someone can suggest a workaround for this?

weierophinney commented 10 years ago

You have two problems with this listener. First, you're not returning the response, which means the rest of the dispatch cycle continues, and which means the response may get replaced at any point. So, do that first -- just add return $response; at the end of the method. (In fact, if you return a response, you do not need to set it in the event.)

Second, if you intend for the response to be an API-Problem no matter what, then you have another option: return an ApiProblemResponse. That ends up looking like this:

// Assume you've imported ZF\ApiProblem\ApiProblem and ZF\ApiProblem\ApiProblemResponse
return new ApiProblemResponse(
    new ApiProblem(403, 'The current user is not authorized for this request')
);

or even build the ApiProblem from your exception:

return new ApiProblemResponse(
    new ApiProblem(403, $exception)
);

The "SendResponse" event is triggered by the MVC in order to send the response object at the time that it is ready to, well, send the response. zf-api-problem registers ZF\ApiProblem\Listener\SendApiProblemResponseListener with this event. It will perform work if, and only if, the response is an ApiProblemResponse. In all other situations, it does nothing.

It's hard to tell what your intentions are exactly, as the naming in your code and your description of the problem differ -- I'm not sure if you're trying to return an "ApiProblem" response in the case that the user is unauthorized (which is what the naming of your listener implies), or if you are wanting to return a vanilla 403 response in that case.

So:

svycka commented 10 years ago

Sorry but I can't get this to work. returning $response or:

return new ApiProblemResponse(
    new ApiProblem(403, 'The current user is not authorized for this request')
);

does not help at all ignores everyting and returns whatewer it wants. Response example:

{
    type: "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html",
    title: "Internal Server Error",
    status: 500,
    detail: "You are not authorized to access this resource"
}

when it should be:

{
    type: "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html",
    title: "Forbidden",
    status: 403,
    detail: "The current user is not authorized for this request"
}

But I find out that it almost works like this:

$event->setResponse(new ApiProblemResponse(
    new ApiProblem(403, 'The current user is not authorized for this request')
));

But I don't want this because it is always api problem response. So the problem still not solved.

@weierophinney basicly I want something like this https://github.com/ZF-Commons/zfc-rbac/blob/master/src/ZfcRbac/View/Strategy/UnauthorizedStrategy.php plus if this is api request return ApiProblem response with 403 and error message instead of html.

svycka commented 10 years ago

Ok I find out why this does not work. It is becouse thats error respone and ApiProblemListener uses not response code but exception code here.

So now I am not sure about best way to solve this maybe like this:

// first we get actual exception
$prev_exception = $event->getParam('exception');
// set exception code 403 to be used as http response code
$exception = new \ZfcRbac\Exception\UnauthorizedException($prev_exception->getMessage(), 403, $prev_exception);
// and now we change real exception with upgraded one
$event->setParam('exception', $exception);