search

BTBurke / caddy-jwt

JWT middleware for the Caddy server
MIT License
113 stars 40 forks source link

Added two params to control tokens in headers #34

Open georgantasp opened 6 years ago

georgantasp commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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" notifications@github.com 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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.