odan / slim4-skeleton

A Slim 4 Skeleton
https://odan.github.io/slim4-skeleton/
MIT License
439 stars 80 forks source link

Slim Role Auth #14

Closed fvtorres closed 4 years ago

fvtorres commented 4 years ago

Hello @odan,

Do you think your JWT authorization is compatible with this dependency? For role auth https://github.com/jaywilliams/slim-role-auth

It should be loaded after the JWT was authorized right? The role data should be inside the JWT?

Thanks!

odan commented 4 years ago

Hi @fvtorres

In theory it should work together if you pass the role from the JWT into a RoleProvider object.

But I see some issues with this library:

fvtorres commented 4 years ago

Hey, I guess I posted the fork, not the master. My bad

They have updated to work with Slim-4 Here is the link: https://github.com/tkhamez/slim-role-auth Sorry for that

fvtorres commented 4 years ago

So by your experience it might be better to implement something from scratch? Or maybe even implement this inside JWTmiddleware?

odan commented 4 years ago

Ok this repository looks better.

Yes, I would try to implement this inside the JwtMiddleware. Copy the roles from the JWT into a custom RoleProvider object.

To set the roles into the token, you may also modify the createJwt method a little bit.

fvtorres commented 4 years ago

Hey @odan,

I've been digging into this and came up with two approaches. I tried to implement that library but didn`t have the knowledge to do that, so I am trying to implement the functionality my own.

I could think in 2 scenarios: Scenario 1 Add a separate middleware, and add a parameter when calling it in the route -Pros: I can use the group functionality -Con: It might polute the route file. Im having extreme difficulty in passing the parameter in the route. To do so I had to wipe the dependencies of the constructor (Is it ok to do that? - Is there another way?) And now I`m having issue trying to call the responsefactory, as it isn't being injected as before

Here is the code I used: In Routes: use App\Middleware\RoleMiddleware as Role; $app->group('', function (RouteCollectorProxy $group) { $group->get('/users/{username}', \App\Action\UserReadAction::class); })->add(new Role(['admin','user']))->add(JwtMiddleware::class);

And RoleMiddleware: `<?php

namespace App\Middleware;

use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ResponseFactoryInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\RequestHandlerInterface; use Slim\Psr7\Response;

/**

Scenario 2 Add the role validation inside JWTMiddleware, and have an array of Routes, Methods and Roles in a sepparate file. -Pros: I made it work hehe -Con: Will have to declare every single route and method, without using the group function.

Code in JWTmiddleware: `//Get Role from token $role = $parsedToken->getClaim('role');

    //Get Route
    $method = $request->getMethod();
    $route = RouteContext::fromRequest($request)->getRoute()->getPattern();

    $validateRole = $this->roleAuth->validateRole($role,$method,$route);

    if (!$role || !$validateRole) {
        return $this->responseFactory->createResponse()
            ->withHeader('Content-Type', 'application/json')
            ->withStatus(401, 'Unauthorized');
    }

    // Append the user role as request attribute
    $request = $request->withAttribute('role', $parsedToken->getClaim('role'));`

And RoleAuth: `<?php

namespace App\Auth;

use InvalidArgumentException; use UnexpectedValueException;

final class RoleAuth { public function validateRole($role,$method,$route) { $routeRole = [ //Route => Method(s) => Role(s) permitted '/users/{username}' => ['GET' => ['user','admin']], ];

    if (in_array($role,$routeRole[$route][$method])){
        return true;
    }
    return false;
}

}`

Witch way you think I should Go?

odan commented 4 years ago

Both solutions have their pros and cons.

Scenario 1: Single role checking middleware

Scenario 2:

More options:

Scenario 3: Checking the group within the Action class

Scenario 4: Single role checking middleware in combination with multiple Role based middlewares

$app->group('/users', function (RouteCollectorProxy $group) {
       // ...
})
    ->add(RoleCheckerMiddleware::class) // <--- The role checking middleware
    ->add(RoleAdminMiddleware::class) // <---- The Role middleware (can be combined)
    ->add(JwtMiddleware::class);

I would try Scenario 4.

fvtorres commented 4 years ago

Scenario 4 is brilliant!! Thanks for the light at the end of the tunnel. Will go that way! Thanks!! I thought I was dumb by not being able to use DI and call functions with arguments. Nice to know it is difficult

One question, you think I would cause a problem to call specific methods in the Class that adds the role attribute? So that I could have only 1 Master Class that have all the roles attributes functions? Something like: ->add(RoleMiddleware::admin()) That runs the admin method inside the class, that adds the admin attribute. (I guess I've seen another way to write that, like RoleMiddleware::class somethinghere, but can`t find where I saw that again)

This approach would be even better if there are a lot of roles, centralising them in one file only

odan commented 4 years ago

I see at least 2 options if you want to centralize it a little bit more:

1. Using a public const with a list of fully qualified Role middlewares

Example

class MiddlewareRole
{
    public const ADMIN = \App\Middleware\Role\RoleAdminMiddleware::class;
    public const USER = \App\Middleware\Role\RoleUserMiddleware::class;
    // add more...
}

Usage

->add(MiddlewareRole::ADMIN);

Pros:

Cons:

2. A middleware factory that returns a dynamic callable

Configuration:

RoleMiddleware::setResponseFactory($app->getResponseFactory());

Usage

->add(RoleMiddleware::admin());

Pro:

Cons:

fvtorres commented 4 years ago

Hello @odan ,

I`ve been working with the What you said and came with a problem. If I call the RoleCheckingMiddleware on a big group, and add roles the routes inside it. it won't work, because the Checking middleware will be called before than adding the roles. Should I run the checking after every route call? Or should I run it globally after Routing middleware? Not sure if its OK

Ex:

$app->group('/api', function (RouteCollectorProxy $app) {
            //User Routes
            $app->group('user', function (RouteCollectorProxy $app) {
                $app->patch('/users', \App\Action\UserUpdateDataAction::class);
            })->add(RoleMiddleware::USER);
            //Admin Routes
            $app->group('admin', function (RouteCollectorProxy $app) {
                $app->post('/users', \App\Action\UserCreateAction::class);
            })->add(RoleMiddleware::ADMIN);
})->add(RoleCheckerMiddleware::class);

And about the number 2, where should I insert the RoleMiddleware::setResponseFactory($app->getResponseFactory()); This is to request object and handler be passed to the function? Why would it generate "unnecessary middlewares for each request'?

Thanks Bro

odan commented 4 years ago

1) It works only per group and not for the sub-groups, because Slim uses a Last In, First Out (LIFO) model for middleware.

$app->group('/api', function (RouteCollectorProxy $app) {
    //User Routes
    $app->group('user', function (RouteCollectorProxy $app) {
        $app->patch('/users', \App\Action\UserUpdateDataAction::class);
    })->add(RoleCheckerMiddleware::class)
      ->add(RoleMiddleware::USER);

    //Admin Routes
    $app->group('admin', function (RouteCollectorProxy $app) {
        $app->post('/users', \App\Action\UserCreateAction::class);
    })->add(RoleCheckerMiddleware::class)
      ->add(RoleMiddleware::ADMIN);
});

2) I think this is not required anymore.

odan commented 4 years ago

Can we close this issue?

fvtorres commented 4 years ago

Hello Daniel! Sure! Your answer worked perfectly!

Thanks!!