Added two params to control tokens in headers

[georgantasp]

individual_claim_headers: enables the legacy individual claims in headers. single_claim_header: enables a new header "Token-Claims" as described in issue 19.

Doesn't offer much in the way of refactoring, but serves the purpose.

[BTBurke]

Thanks for the contribution. I'm just looking at it on my phone so I may have missed something. Is there a default to pass either individual headers or the combined claims? I think a prudent way to introduce the change is to have the new default be the combined claim header and provide a config parameter to turn on the old individual claims. Then later on I can drop the individual claim functionality.

I'll take a closer look this weekend. Thanks again for the contribution.

[georgantasp]

Hi Bryan. I'm happy to hear from you. So the way I implemented it was to turn off both individual claims AND combined claims by default. This would make users opt-in to either feature. My thinking is that if a downsteam app wants the claim information, it may just as likely parse it from the original jwt_token cookie or Authorization header. I'm happy to default either in the other direction if you disagree.

On Dec 12, 2017 17:33, "Bryan Burke" [email protected] wrote:

Thanks for the contribution. I'm just looking at it on my phone so I may have missed something. Is there a default to pass either individual headers or the combined claims? I think a prudent way to introduce the change is to have the new default be the combined claim header and provide a config parameter to turn on the old individual claims. Then later on I can drop the individual claim functionality.

I'll take a closer look this weekend. Thanks again for the contribution.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/BTBurke/caddy-jwt/pull/34#issuecomment-351217464, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4U7ZNz7vOZCetNkQREiH0MLd4Yu5kcks5s_v9IgaJpZM4Q8vGO .

[georgantasp]

Did some additional refactoring.

I'd also like to propose two additional changes:

1. headers should only be stripped when these header options are enabled.
2. the call to strip headers should be moved to end of the rules loop, just before new headers are set.

Let me know what you think.

[jimjimovich]

I like this idea, but please don't disable the headers by default as this could be disastrous for people using the plugin and getting automated builds from the build server. Personally, I've gone to building it myself with Docker and locking down to specific versions of all plugins to avoid this problem.

Is there any more work that needs done to get this PR accepted? Any way I can help out?

[BTBurke]

I haven't merged this yet because the way it's constructed right now it would be a breaking change and there currently isn't a good way to notify users using automated builds that they would need to alter their configs.

I plan to make one option the default and let the other option be opt-in, but I just haven't had time to refactor this to make that possible.