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(plugin): implement golang version of plugin jwt-auth #743

Merged
merged 15 commits into from
Jun 6, 2024

Conversation

Ink-33
Copy link
Contributor

@Ink-33 Ink-33 commented Dec 31, 2023

Ⅰ. Describe what this PR did

This merge request contains the jwt-auth plugin implemented in golang.

Ⅱ. Does this pull request fix one issue?

#232 , #591

Ⅲ. Why don't you add test cases (unit test/integration test)?

Test cases are still in progress due to the complexity.

Ⅳ. Describe how to verify it

As #589, all behaviors of this plugin should be same with the currently cpp verison of jwt-auth. Maybe we need to use plently of tese cases to verify it.

Ⅴ. Special notes for reviews

Same with IV. Since I am not particularly familiar with cpp, I hope that reviewers will understand some errors and I will correct them in time🙏

@codecov-commenter
Copy link

codecov-commenter commented Dec 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (89c7277) 38.27% compared to head (bbacf81) 38.24%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #743      +/-   ##
==========================================
- Coverage   38.27%   38.24%   -0.04%     
==========================================
  Files          60       60              
  Lines        9979     9979              
==========================================
- Hits         3819     3816       -3     
- Misses       5869     5871       +2     
- Partials      291      292       +1     

see 1 file with indirect coverage changes

@Ink-33 Ink-33 force-pushed the jwt-auth branch 2 times, most recently from 45ad2f7 to bbacf81 Compare December 31, 2023 08:15
@johnlanni
Copy link
Collaborator

@Ink-33 Is this draft PR ready for review, or is there still unfinished work?

@Ink-33
Copy link
Contributor Author

Ink-33 commented Mar 4, 2024

@Ink-33 Is this draft PR ready for review, or is there still unfinished work?

Yes it's ready for review and I will update the branch later today.

@johnlanni
Copy link
Collaborator

@Ink-33 Please add an e2e test for it.

plugins/wasm-go/extensions/jwt-auth/go.mod Outdated Show resolved Hide resolved
plugins/wasm-go/extensions/jwt-auth/go.mod Outdated Show resolved Hide resolved
@johnlanni
Copy link
Collaborator

@Ink-33
Copy link
Contributor Author

Ink-33 commented May 16, 2024

@Ink-33 Please add e2e test cases, refer to: https://github.com/alibaba/higress/pull/759/files#diff-00090f4cf6417f2566ebc91e56b6b5d0e12a834a184f4b814d51bc89581d796b

got it. I would like to add three parts of test cases including normal, rejected, and normal with all features one by one.

btw, how to run e2e test case on my local dev environments?

@johnlanni
Copy link
Collaborator

johnlanni commented May 16, 2024

@Ink-33 Please add e2e test cases, refer to: https://github.com/alibaba/higress/pull/759/files#diff-00090f4cf6417f2566ebc91e56b6b5d0e12a834a184f4b814d51bc89581d796b

got it. I would like to add three parts of test cases including normal, rejected, and normal with all features one by one.

btw, how to run e2e test case on my local dev environments?

Please refer this guide: https://github.com/alibaba/higress/tree/main/test

PLUGIN_NAME=jwt-auth make higress-wasmplugin-test

@Ink-33
Copy link
Contributor Author

Ink-33 commented May 20, 2024

@johnlanni allow 里面配置多个 consumer 且全部验证失败应该返回哪个错误码给请求,或者直接用 verify failed 兜底呢?

Image_1716193181343.png

@johnlanni
Copy link
Collaborator

johnlanni commented May 20, 2024

@johnlanni allow 里面配置多个 consumer 且全部验证失败应该返回哪个错误码给请求,或者直接用 verify failed 兜底呢?

Image_1716193181343.png

返回 403 即可,错误原因可以用 not allowed,跟jwt本身凭证不合法区分下

Copy link
Collaborator

@johnlanni johnlanni left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@johnlanni johnlanni merged commit ed976c6 into alibaba:main Jun 6, 2024
11 checks passed
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