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

feat(jwt): make KeyFunc public in JWT middleware #1756

Merged
merged 2 commits into from
May 8, 2021

Conversation

antonindrawan
Copy link
Contributor

@antonindrawan antonindrawan commented Jan 12, 2021

It allows a user-defined function to supply the key for a token
verification.

Address #1731

Since this is my first contribution to echo, please suggest any improvements.

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #1756 (1df126e) into master (d9e2354) will increase coverage by 0.00%.
The diff coverage is 84.61%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1756   +/-   ##
=======================================
  Coverage   89.73%   89.74%           
=======================================
  Files          32       32           
  Lines        2669     2671    +2     
=======================================
+ Hits         2395     2397    +2     
  Misses        175      175           
  Partials       99       99           
Impacted Files Coverage Δ
middleware/jwt.go 78.35% <84.61%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9e2354...1df126e. Read the comment docs.

@antonindrawan
Copy link
Contributor Author

I have no idea why codecov/patch failed with '84.61% of diff hit (target 89.43%)'. It would be nice if I could get some pointer and improve it.

Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

Could you shed some light on what is you actual use case is and why his PR is needed?

middleware/jwt.go Show resolved Hide resolved
middleware/jwt.go Outdated Show resolved Hide resolved
middleware/jwt.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@antonindrawan antonindrawan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

Could you shed some light on what is you actual use case is and why his PR is needed?

Sorry, for my late reaction. I didn't notice that you commented 3 weeks ago.

I would like to use Google to issue a sign-in token and echo verifies the token. I came across that someone else wanted to have it too (#1731)

I try to approach it using a custom KeyFunc which does:

Is there another way with the existing JWT Middleware functions?
That would be great if there is one.

middleware/jwt.go Outdated Show resolved Hide resolved
middleware/jwt.go Show resolved Hide resolved
It allows a user-defined function to supply the key for a token
verification.
middleware/jwt.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Approved from my side!
Thanks @antonindrawan

@antonindrawan
Copy link
Contributor Author

antonindrawan commented Mar 5, 2021

Looks good. Approved from my side!
Thanks @antonindrawan

No problem. Thank you for reviewing @lammel .
I will have to create a PR on the echox project (likely this weekend or the next weekend) and will refer the PR here.
Edit: PR for updating the documentation: labstack/echox#196

One other question: I see that the code coverage result fails. I have added test cases but I can't understand the code coverage result. Will it block the pull request to be merged later?

@lammel lammel added this to the v4.3 milestone Mar 6, 2021
@lammel lammel mentioned this pull request Mar 9, 2021
antonindrawan added a commit to antonindrawan/echox that referenced this pull request Mar 13, 2021
Documentation for exposing KeyFunc:
labstack/echo#1756
antonindrawan added a commit to antonindrawan/echox that referenced this pull request Mar 13, 2021
Documentation for exposing KeyFunc:
labstack/echo#1756
@aldas aldas merged commit 76f186a into labstack:master May 8, 2021
aldas pushed a commit to labstack/echox that referenced this pull request May 8, 2021
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

3 participants