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

Remove unneeded/duplicate jwt middleware in favor of https://github.com/labstack/echo-jwt #2508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bakatz
Copy link
Contributor

@bakatz bakatz commented Aug 21, 2023

I was looking at one of my projects and realized there was an old JWT library reference in there. Then I realized that echo still uses some old version of the jwt middleware baked into the echo repo, while simultaneously noting that https://github.com/labstack/echo-jwt is the officially supported jwt middleware library.

Problem: I think this is a confusing strategy - why not just recommend the use of https://github.com/labstack/echo-jwt instead of having 2 separate jwt middlewares, one baked into the echo lib itself with an old version of the jwt lib?

This PR removes the old jwt middleware with an old version of golang-jwt and updates dependencies.

I would recommend a minor or major version bump and just ask people to use the echo-jwt library as it uses the latest JWT version and is otherwise exactly the same code.

@bakatz
Copy link
Contributor Author

bakatz commented Aug 21, 2023

@aldas it looks like you might have been the creator of the echo-jwt repo. Curious for your thoughts on this PR - did I miss anything or does this seem reasonable to merge?

@aldas
Copy link
Contributor

aldas commented Aug 21, 2023

We can not remove JWT middleware from this repo due backwards compatibility promises we are trying to keep. https://github.com/labstack/echo-jwt was created just for this reason - we could do these kind of changes.

I do not remember exactly but I think seeing linters/checks warning about deprecation notices like

// Deprecated: Please use https://github.com/labstack/echo-jwt instead
at least Jetbrains Goland IDE is doing this.

@bakatz
Copy link
Contributor Author

bakatz commented Aug 21, 2023

@aldas
Ok, got it - thank you for the quick response. I still think an exception should be made in this case for the following reasons:

  • VSCode (I'm not sure about market share but it must be in the top 5) doesn't honor the above deprecation warning/I had no idea. There's nothing clearly visible to me as the developer that indicates this is deprecated when I'm adding middleware.
  • Similar to the above point: it's really easy to accidentally use the old version of the JWT lib. If you don't explicitly add an import with an alias to the new jwt middleware lib, you might end up using this old version that could have a security hole of some kind. Okay, to be fair, I don't think there are any known security holes with 3.2.2 in the wild at the moment - so maybe a bit of a moot point - but it's better to always keep security related libraries up-to-date.
  • The change should be straightforward for people to make and should only occur if people decide to upgrade echo to the latest major or minor version (whatever you all choose, maybe a major version bump is more appropriate). The input to the library and the behavior based on the tests seems to be the same, so it would just be a matter of go get newversion && go mod tidy

Let me know what you think

EDIT: my first 2 points are somewhat mitigated - seems like after restarting vs code it recognized the deprecated method, behavior seems to be flaky though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants