search

traefik-plugins / traefik-jwt-plugin

Traefik plugin which checks JWT tokens for required fields. Supports Open Policy Agent (OPA) and signature validation with JWKS
Apache License 2.0
98 stars 34 forks source link

Add OpaDebugMode to set the opa response in the http response body wh… #48

Closed yoeluk closed 1 year ago

yoeluk commented 1 year ago

this fixes #47

yoeluk commented 1 year ago

@eshepelyuk I don't know anything about conventional commit messages and frankly find the idea rather tiresome (i.e. I don't have the time to read all that documentation you linked). If this is about hinting if this is a braking change I wage that it is. This changes the response to the client and someone might have integrated with that. Please suggest what commit message you would like to see here.

eshepelyuk commented 1 year ago

@eshepelyuk I don't know anything about conventional commit messages and frankly find the idea rather tiresome (i.e. I don't have the time to read all that documentation you linked). If this is about hinting if this is a braking change I wage that it is. This changes the response to the client and someone might have integrated with that. Please suggest what commit message you would like to see here.

it's quite sad that you don't have ~ 5 mins for reading - but you can start from inspecting the previous commit messages and also inspect output of failed CI build.

blagerweij commented 1 year ago

@yoeluk Thanks for the updated README. One last request: could you add a unit-test for the 'OpaDebugMode' flag? Verifying that we only write 'Forbidden' if the flag is false, and we include the Opa response when set to true?

eshepelyuk commented 1 year ago

@yoeluk Thanks for the updated README. One last request: could you add a unit-test for the 'OpaDebugMode' flag? Verifying that we only write 'Forbidden' if the flag is false, and we include the Opa response when set to true?

@blagerweij tests were already requested in the initial review.

yoeluk commented 1 year ago

@blagerweij @eshepelyuk are we okay with merging this?

eshepelyuk commented 1 year ago

@blagerweij @eshepelyuk are we okay with merging this?

The CI job must be green for this. There is no any successful run yet, so no - it can't be merged yet.

yoeluk commented 1 year ago

@eshepelyuk is this part of my change? https://github.com/team-carepay/traefik-jwt-plugin/actions/runs/5249175409/jobs/9497234087 I wouldn't think so, so how do we fix that?

eshepelyuk commented 1 year ago

@eshepelyuk is this part of my change? https://github.com/team-carepay/traefik-jwt-plugin/actions/runs/5249175409/jobs/9497234087 I wouldn't think so, so how do we fix that?

Looks like it also fails in main branch, so most probably unrelated to your changes. I don't have possibility to address it this week, so if tou can research and fix it - welcome.

yoeluk commented 1 year ago

@eshepelyuk I fixed the linting on the project sources but we still have linting issue with the golang source which might be a bug on the linter... can we update the golangci-lint version to latest version: v1.53. It is set with an environment variable and it looks like it is currently set to v1.50

eshepelyuk commented 1 year ago

@eshepelyuk I fixed the linting on the project sources but we still have linting issue with the golang source which might be a bug on the linter... can we update the golangci-lint version to latest version: v1.53. It is set with an environment variable and it looks like it is currently set to v1.50

Yeah, lets try.

yoeluk commented 1 year ago

@eshepelyuk my fork build action is green after bumping golangci-lint to latest

yoeluk commented 1 year ago

thanks @eshepelyuk @blagerweij for the comments and prompt responses! 👍