oscarotero / psr7-middlewares

[DEPRECATED] Collection of PSR-7 middlewares
MIT License
668 stars 56 forks source link

AuraRouter custom failed route rules responses #64

Closed delolmo closed 7 years ago

delolmo commented 7 years ago

Currently, there are 3 predefined responses when the AuraRouter fails to match: (1) the Aura\Router\Rule\Allows returns a 405 response; (2) the Aura\Router\Rule\Accepts returns a 406 response; and (3), by default, every other failed rule returns a 404 response. However, consider the following case:

1) A custom Aura Route is developed to tell between routes that allow anonymous access and routes that don't. For example, the new route could implement a new "disallowAnonymous()" method to set this route attribute to false (i.e. /admin dir). 2) A custom Aura Rule is developed for handling authentication. An authentication service is injected via the rule's constructor (i.e. Zend\Authentication\AuthenticationService). The custom rule checks for $route->anonymous and uses the authentication service to tell whether or not the user is authenticated. If the user is not authenticated, the rule will return false. 3) Currently, there is no way to "catch" this custom rule failure. For example, one could think of implementing the following response:

return $response->withStatus(401)->withHeader('Location', '/login');

The proposed changes, even though a proof of concept, allows injecting your own responses for each failed rule. To take into account backwards compatibility, the proposed solution adds the 'Aura\Router\Rule\Allows' and the 'Aura\Router\Rule\Accepts' by default. This will allow for more flexibility when using the AuraRouter middleware component.

oscarotero commented 7 years ago

To handle the http error codes, there's the ErrorHandler middleware. For example:

use Psr7Middlewares\Middleware;

$middleware = [
    Middleware::errorHandler(function ($request, $response) {
         if ($response->getStatusCode() == 401) {
              return $response->withHeader('Location', '/login');
         }
        echo "There's an error!"
    }),
    Middleware::auraRouter($router)
];

This allows to separate the router and the error handler and allow, for example, that an existing route can return a 404 response, (example: the id is not found in the database: /articles/123) but manage the error in the same place than if the route is not found by the router.

delolmo commented 7 years ago

I agree with your solution, partially. It is true that a 401 error could by handled by ErrorHandler, but you wouldn't be able to trace the error back to the Aura Rule. What if there was an ACL service that also returned a 401 response or anything of the sort? Customizing the response for each failed rule could still prove very valuable (i.e. for adding some kind of attribute to the request that could be used by the ErrorHandler).

Still, this is just an example. What I was trying to prove is that the AuraRouter middleware should provide some kind of mechanism to allow processing failed custom rules (or even the native ones). From my standpoint, adding this functionality would only make the AuraRouter middleware more flexible. Why stick to the current switch ($failedRoute->failedRule) { statement when the behavior could be handled by the developer without having to rewrite the middleware?

oscarotero commented 7 years ago

The problem is that there's several middleware components that can generate errors:

What you're proposing is allow to control the specific errors of AuraRouter but what about the other errors returned by any other middleware (or route controller?). Should we allow to pass an errorHandler to all these middlewares too? I think is a bad idea.

Customizing the response for each failed rule could still prove very valuable (i.e. for adding some kind of attribute to the request that could be used by the ErrorHandler).

This is not possible. The ErrorHandler cannot recover the request of the inner middleware that generate the error (the nested middlewares return the response, not the request).

To have any information about the error responses, I can think of the following solutions:

  1. Create specific error response classes. Example:
//In the controller:
if ($user->notAllowed()) {
    return new AclNotAllowedResponse();
}

//In the error handler
if ($response instanceof AclNotAllowedResponse) {
    //do stuff
}
  1. Use http headers
    
    //In the controller:
    if ($user->notAllowed()) {
    return (new Response())->withHeader('X-Error-Type', 'acl-not-allowed');
    }

//In the error handler if ($response->getHeaderLine('X-Error-Type') === 'acl-not-allowed') { //do stuff }

delolmo commented 7 years ago

Customizing the response for each failed rule could still prove very valuable (i.e. for adding some kind of attribute ~to the request~ that could be used by the ErrorHandler).

True, my mistake.

As for the rest, even though I respect your approach regarding the specific examples of authentication and authorization, the commit's intention was to embed in the middleware the ability to handle failed rules - whatever their nature - not to handle errors per se. However, the more I think about it the more I realize that there's a very thin line between the two.

Closing!