Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Callback should act like a Slim middleware. #29

Closed
npx opened this issue Mar 10, 2016 · 7 comments
Closed

Callback should act like a Slim middleware. #29

npx opened this issue Mar 10, 2016 · 7 comments

Comments

@npx
Copy link

npx commented Mar 10, 2016

I'm using this middleware to verify a token.

In addition to that, I would like to resolve the user account the token has been issued to and attach this to the request, so that the controller function that is called for the given route has access to the user account.

Using this middleware and the callback method, I have no way of manipulating the $request like middleware does.

I also had a quick chat with Akrabat on the Slim IRC:

npx:     hey qq, what would be the preferred way to have a middleware add an argument to the $args (third parameter of a controller method call) [...]
Akrabat: $request = $request->withAttribute('token', $token); 
         It should turn up in $args, but will certainly be in $request->getAttribute('token'); 
npx:     just noticed that tuupola's middleware's callback does not give me the option to manipulate the request haha 
[...]
Akrabat: yeah - tuupola should update that really 
         ideally, the callback would act as middleware 
         or put decoded into the request's attributes anyway 

Suggestions:

  1. if the decoded token is added to the Request's attributes, a Middleware following this one can access the decoded token and do as it pleases. However, that makes the second middleware depended on this one.

  2. change callback or add another callback option that makes the provided function act like a middleware and return the call to $next($request, $response). In this case the function has to handle the error on its own, while it of course would be nice to keep the error handling in this middleware to keep it consistent. Alternatively allow throwing an Exception that will be caught.

  3. have callback return false or an array($request, $response).
    On false return the error response like before.
    On null or true call $next($request, $response) like nothing happened
    and if the array is returned call $next($request, $response) with the values of the array.

Please discuss if you can see other solutions, this was just a quick draft of the top of my head as I have to rush now.

I can play with this more extensively tomorrow and provide a pull request if requested.

@tuupola
Copy link
Owner

tuupola commented Mar 10, 2016

Or you can save the token into container. Although I do like the idea of adding decoded token contents into the $request itself.

$app = new \Slim\App();
$container = $app->getContainer();

$container["jwt"] = function ($container) {
    return new StdClass;
};

$app->add(new \Slim\Middleware\JwtAuthentication([
    "secret" => "supersecretkeyyoushouldnotcommittogithub",
    "callback" => function ($request, $response, $arguments) use ($container) {
        $container["jwt"] = $arguments["decoded"];
    }
]));

@npx
Copy link
Author

npx commented Mar 10, 2016

The reason why I do not like this approach is, that I would need to pass the whole DIC to my controller and I rather only pass the dependencies that are needed directly. Otherwise my class could pull out whatever out of there without anyone on the outside knowing.

On top of that this requires to pass the DIC to pretty much every controller (that handles a protected route, that is) :c

//EDIT: (Quick brainfart before bed)
Give your callback function the encoded and decoded token and let the user return something.
After the callback, assemble an array with the token, the decoded object and the callback result and attach this array under the key jwt_auth to the request. That way every affected route can access the token as well as the result of the callback function.

That way every user can access the token and decoded object the same way and has access to the arbitrary information of their callback under a given key in the jwt_auth array.

@tuupola
Copy link
Owner

tuupola commented Mar 15, 2016

Can you check if fb8541c fixes your problem. I am still thinking about possibility of callback returning either false or [$request, $response].

@npx
Copy link
Author

npx commented Mar 16, 2016

Because I had to move my project forward, I wrote my own middleware that adds all the information I need to the request:

        // augment request object with data
        $jwt = [
            'token' => $tokenstr,
            'decoded' => $decoded,
            'account' => $account
        ];
        $augmentedRequest = $request->withAttribute('jwt', $jwt);

That being said:
Looking at your changes, you now add the token to the request, which is nice, however your callback function still does not allow me to add information like the user's account this token has been issued to, which I can resolve based on the "sub" property in the token.

Assuming I now have a "resolve Account by token" function somewhere (which I do) I would still in every route that needs the account information (e.g. for permission checks) to have to resolve this. That is still a lot of routes. Very much possible.

However, I still think it would be helpful for the user to attach additional information to the "attribute" that you are now supporting using the callback.

If you have a better suggestion how you would resolve my requirement, please go ahead, I would rather switch back to using your middleware :)

@tuupola
Copy link
Owner

tuupola commented Mar 16, 2016

To me it sounds like what you are doing it outside of the scope of this middleware. It was never meant alter the request object, just authenticate and provide the decoded token. What happens after that is up to the developer. I think what you did is the correct solution, another middleware which does the authorisation part.

But like I said, I am still pondering about possibility of callback returning either false or [$request, $response]. I have not decided yet whether it is kludgish or not.

@npx
Copy link
Author

npx commented Mar 16, 2016

Well I am not asking for your middleware to "do these things".
I am asking you about what you imagine your callback function to do, and which options to open for the user?

Now that you have added the token to the request, the user gets a lot of more options and I could solve my problem with it.

But as the callback is right now, I do not see many use cases for it. There is no chance to really transport something out of the callback function unless you store it in a global place, which is usually a bad idea.

Middleware has access to the Request and Response and has the option to modify it. This is what you are offering. My suggestions above are mere suggestions that resolve my issue of "access to the token before it reaches the dispatched method and actually making the processing result accessible to every method that is protected by the middleware".

I do not really like the callback returning false [request, response] thing either. In Python its more natural to return tuples. Hence you can also return callback($request, $response, $next); and therefore give the user the chance to plug in their middleware logic and is forced to return $next($request, $response);

@tuupola
Copy link
Owner

tuupola commented Jul 17, 2017

With 3.x branch you can currently do something like:

$app->add(new \Tuupola\Middleware\JwtAuthentication([
    "secret" => "supersecretkeyyoushouldnotcommittogithub",
    "before" => function ($request, $response, $arguments) {
        return $request->withAttribute("test", "test");
    }
]));

@tuupola tuupola closed this as completed Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants