feat(jwt) add jwt token to ngx.ctx

[albertored]

Summary

In this way subsequent plugins can do things with the token
(specific payload validation, check for revocation, etc..) without
having to obtain it from the request.

[kikito]

Thanks for sending this PR. Could you add some specs to test that the code works as intended?

[albertored]

Sure, tomorrow I will add some test Il gio 26 ott 2017 18:25 Enrique García Cota <[email protected]> ha scritto:

…

Thanks for sending this PR. Do you think you could add some specs to test that the code works as intended? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2988 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFY4OKKHPtpdb7aYtU7g304mbaybUhMpks5swLJ7gaJpZM4QH1QN> .

[albertored]

I just took a look to existing tests of jwt plugin but there is no test for variables added to ngx.ctx.

The only way I can think for test this situation is to add a fake plugin that reads values from ngx.ctx and returns them in a place (body or header of the response) where the tests can check them.

Anyway I think this is more a test for the lua-nginx-module than for kong itself so maybe there is no need of adding such tests here.

What do you think?

[Tieske]

I don't think we should be merging this. I get the idea, but this is not the right moment. The planned plugin api should take all this "runloop" logic out of the plugins, as the plugins sole responsibility is authenticating. Updating the request itself is a Kong-core responsibility.

Besides that we will probably provide another mechanism for some inter-plugin comms, so introducing this now, and building dependencies on top of it would only lead to breaking them later.

@thibaultcha thoughts?

[albertored]

Yes, if there is a bigger plan for a better inter-plugin communication I agree that populating directly the context is not a good thing. Also, since it is a very small change, I can live a personal fork of kong with this modification applied until the plugin api will allow it in a different way.

My only last suggestion is the following. Maybe you can merge this but writing somewhere (directly on the code or in the documentation) a warning about using it. Something like Use it at your own risk, it will be changed soon. This warning should be added also to existing plugin interactions like setting the consumer in ngx.ctx.authenticated_consumer.

[thibaultcha]

I think it would be ok to accept this (minor change), especially considering it is synchronized with #2810 thanks to @albertored 's efforts on communicating with @ikogan. Plugins already store other kind of values in ngx.ctx today, although they should not... Until we break free of this paradigm and provide a complete solution with our Lua Plugins API, I'd say this is a minor change to support... I can understand the value in other plugins accessing the OAuth2/JWT tokens... Even when we switch to a Lua Plugins API, some plugins (including those two) will need to make some type of information available to other plugins for further usage. Those plugins do a bit more than just authentication, they also expose how the user was authenticated, something that, say, key-auth might not have to do.

I'd be in favor of merging this, and, as always, all of our current API around plugins development (especially ngx.ctx values) should be considered experimental, so this doesn't derive too much from our current promise.

[albertored]

Hi, any news on this?

[thibaultcha]

@albertored @ikogan I still am in favor of merging this PR and #2818, granted that they follow the same approach and eventually provide tests for this feature. (Updating one of the plugins under spec/fixtures/custom_plugins (or creating a new one) to consume this value and assert that it is being set would be ideal... We should also rebase both of these PRs.

[albertored]

@thibaultcha @ikogan PR rebased and tests added

I have added a custom plugin for checking values in ngx.ctx, let me know if this is ok

[ikogan]

Sorry about how long it's taken me to respond, I had to get my Kong development environment working again.

I'm a little confused by the addition of @albertored's custom plugin. Is the purpose to allow for a universal header to be set based on the presence of a field in the context? What if you want to set more than one header? Does it's usefulness extend beyond the unit tests?

Since I don't have this custom plugin, I'll have to rebase around your branch or implement my own logic in the tests. If we find that this additional plugin is useful, I'll go ahead and rebase.

[albertored]

Yes, the idea is exactly that. If in future (or now) we think it can be useful to extend it to handle more than one context field we can do it.

For what regards your PR I think you should wait for this to be merged, rebase and use the same plugin for your tests. All this clearly apply only if no one has other suggestion to improve/change the way the feature is tested.

[ikogan]

Sounds good to me, I'll wait for this to be merged.

[thibaultcha]

Merged and pushed to next - thank you @albertored! I made a few modifications to the patch, especially to avoid spreading the test regarding the returned value from the ctx-checker plugin. Better make that a test of its own for the sake of atomicity.