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.

Sign up for GitHub

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

Jump to bottom

feat(jwt-auth): Fix #11276 #11282

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat(jwt-auth): Fix #11276 #11282

wants to merge 5 commits into from
+115 −10

Conversation

mikyll
Copy link

@mikyll mikyll commented May 23, 2024 •
edited

Description

New feature:

  • Added a new configuration parameter to the schema: key_claim_name (default = "key"), so for example one could use "iss" to check the validity of the JWT;

(similar behaviour to Kong JWT plugin)

Fixes #11276

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@mikyll
Fix apache#11276 + little style refactor
afa520d 
Features:
- config param "key_claim_name" (default = "key"), so for example one could use "iss" to check the validity of the JWT;

Style:
- 2 blank lines between functions;
- 1 blank like before "else" and "elseif";
- jwt -> JWT;
- Capitalized logs and response messages;
- Added description for each schema configuration parameter;
@mikyll mikyll changed the title Fix #11276 + Little style refactor feat(jwt-auth): Fix #11276 + Little style refactor May 23, 2024
@mikyll mikyll marked this pull request as ready for review May 23, 2024 14:50
shreemaan-abhishek
shreemaan-abhishek reviewed May 29, 2024
View reviewed changes
Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment •
edited

Choose a reason for hiding this comment

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

Please remove the style changes and add test cases to support your fix. Thanks.

@mikyll
Copy link
Author

mikyll commented May 29, 2024

@shreemaan-abhishek thank you for the feedback. I understand the importance of keeping PRs focused, but I made the style changes to align with the code_style guide. Could you please clarify if:

  • the style changes should be reverted entirely, despite the guidelines?
  • generally speaking, you prefer these style changes to be submitted as a separate PR, or not be carried out at all?

Anyways, I'll be happy to add the necessary test cases, as soon as possible :-)

@mikyll
Revert style changes to jwt-auth plugin
bd76c43
@mikyll
Added test case for new jwt-auth feature
2af7e15 
I added a new test case for feature apache#11276

Since the default value of the new config parameter "key_claim_name" is "key", "default behaviour" is already validated by other tests.
@mikyll mikyll changed the title feat(jwt-auth): Fix #11276 + Little style refactor feat(jwt-auth): Fix #11276 Jun 4, 2024
@mikyll
Update jwt-auth4.t
92bd025 
+ Removed external httpbin upstream in favor of local endpoint (/hello);
+ Cleaned code;
@mikyll
Copy link
Author

mikyll commented Jun 4, 2024 •
edited

@shreemaan-abhishek
I did the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreemaan-abhishek shreemaan-abhishek Awaiting requested review from shreemaan-abhishek

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mikyll @shreemaan-abhishek