thephpleague / route

Fast PSR-7 based routing and dispatch component including PSR-15 middleware, built on top of FastRoute.
http://route.thephpleague.com
MIT License
651 stars 126 forks source link

Add an option to get the target route from the middleware #318

Closed Concentum closed 2 years ago

Concentum commented 2 years ago

Please add an option to get the target route from the middleware, this is necessary to implement authorization in the API when the JWT token contains the user role, and valid permissions for roles are loaded from the configuration.

I think it is wrong to pass the check to the controller

And it also seems strange that middleware does not know for what purpose it is being executed.

philipobenito commented 2 years ago

Hi @Concentum - just a note first of all, any pull requests require passing tests and to follow the coding standards for the project.

Secondly, this feels a bit too much like framework/app logic as things are, could you give me a full example of what you're trying to achieve and the problem you're solving?

Are you saying that your roles are defined by access to a controller method? If that is the case I'd suggest that that check is indeed passed off to the controller, even if the logic is invoked elsewhere.

By introducing something aimed for use in middleware defined by a specific attribute key, we'd be tying any applications that use that functionality to league/route, which is something I try to avoid.

Concentum commented 2 years ago

Hi @philipobenito sorry for the hasty PR, please explain how it happened that the route does not have an official way to get the end point.

philipobenito commented 2 years ago

@Concentum the simple answer is that route isn't a framework, it doesn't care about your controller at all. If you're defining roles by controller method then I'd suggesting that mechanism being attached to the controller, not the router or middleware, if you'd prefer to handle it in middleware, then I'd suggesting attaching roles to HTTP paths/methods instead, as that is what the router/dispatcher/middleware does deal with.

Concentum commented 2 years ago

@philipobenito, sorry for my importunity Here is the code of my middleware

`<?php declare(strict_types=1);

namespace App\Middleware;

use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Laminas\Diactoros\Response\RedirectResponse; use League\Route\Http\Exception\UnauthorizedException; use League\Route\Http\Exception\ForbiddenException; use App\Helper\Identity;

class Authorization implements MiddlewareInterface { private $container;

public function __construct($container)
{
    $this->container = $container;
}
private function getPermissions($action) 
{ 
    $permissions = $this->container->get('permissions');
    return isset($permissions[$action]) ? $permissions[$action] : [];
}
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{   
    // so or something like that I planned to get the goal of the current route
    $target = $request->getAttribute('routeTarget');
    if ($request->hasHeader('Authorization') === false) {
        throw new UnauthorizedException('Access token is required.');
    }
    $header = $request->getHeader('Authorization');
    try {
        $identity = new Identity(
            trim(str_replace('Bearer', '', $header[0])), 
            $this->container->get('settings')['secretKey']
        );
    } catch (\InvalidArgumentException $e) {
        throw new UnauthorizedException($e->getMessage());
    }
    if (!empty($this->getPermissions($target)) && 
        empty(
            array_intersect(
                $identity->getRoleIds(),
                $this->getPermissions($target)
            )
        )) {
        throw new ForbiddenException('Access is denied.');
    }
    return $handler->handle($request);
}

}`

It seemed to me that middleware is the place for such client code, because doing checks in each action of each controller is a pain :(

I understand what you're trying to avoid by defining specific keys, but you can come up with one open method without side effects in the route class, right?

philipobenito commented 2 years ago

Why does your target have to be Class::method? Can't it be GET::/route for example? As you already have access to that and it's a standardised thing rather than something very specific which is what league/route is avoiding