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): support configuring key_claim_name #11282

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

feat(jwt-auth): support configuring key_claim_name #11282

wants to merge 5 commits into from

Conversation

mikyll
Copy link
Contributor

@mikyll mikyll commented May 23, 2024 •
edited
Loading

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
Loading

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
Contributor 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
Contributor Author

mikyll commented Jun 4, 2024 •
edited
Loading

@shreemaan-abhishek
I did the following:

@github-actions GitHub Actions
Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Aug 16, 2024
@mikyll
Copy link
Contributor Author

mikyll commented Sep 13, 2024

Hello, any news on this PR?

@github-actions github-actions bot removed the stale label Sep 14, 2024
shreemaan-abhishek
shreemaan-abhishek reviewed Sep 20, 2024
View reviewed changes
apisix/plugins/jwt-auth.lua
local real_payload = {
key = key,
[key_claim_name] = key,
Copy link
Contributor

Choose a reason for hiding this comment

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

although the key_claim_name has changed, the value of key_claim_name will always be key. That doesn't fit into the scene. For example, if the key_claim_name is exp then the value should be the expiry time, not the key. Right?

@shreemaan-abhishek
Copy link
Contributor

@mikyll, please rebase with master there have been some changes in the CI.

@shreemaan-abhishek shreemaan-abhishek changed the title feat(jwt-auth): Fix #11276 feat(jwt-auth): support configuring key_claim_name Sep 20, 2024
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 20, 2024
@shreemaan-abhishek shreemaan-abhishek mentioned this pull request Sep 20, 2024
Draft
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek left review comments

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

Assignees
No one assigned
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
2 participants
@mikyll @shreemaan-abhishek