Skip to content

feat: support OIDC claim validator (#8772) #11824

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
Merged
merged 14 commits into from
Jul 22, 2025
Merged

feat: support OIDC claim validator (#8772) #11824

merged 14 commits into from
Jul 22, 2025
+319 −12

Conversation

beardnick
Copy link
Contributor

@beardnick beardnick commented Dec 12, 2024 •
edited
Loading

Description

Fixes #8772

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)

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Dec 12, 2024
@bzp2010 bzp2010 self-requested a review December 19, 2024 11:08
@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 Mar 26, 2025
@Baoyuantop
Copy link
Contributor

Hi @beardnick, please make the test pass

@beardnick
Copy link
Contributor Author

Hi @beardnick, please make the test pass

Ok, I'll take a look

@beardnick beardnick force-pushed the feature-oidc-validate branch from a7db44e to d53ef5e Compare March 26, 2025 13:04
Baoyuantop
Baoyuantop reviewed Mar 27, 2025
View reviewed changes
@github-actions github-actions bot removed the stale label Mar 27, 2025
@Baoyuantop Baoyuantop moved this to 🏗 In progress in ⚡️ Apache APISIX Roadmap Mar 28, 2025
@Baoyuantop Baoyuantop moved this from 🏗 In progress to 👀 In review in ⚡️ Apache APISIX Roadmap Mar 28, 2025
@Baoyuantop
Copy link
Contributor

Hi @beardnick, do you have time to continue working on this PR?

@beardnick
Copy link
Contributor Author

Hi @beardnick, do you have time to continue working on this PR?

Sorry, I'm busy last few days. I'll continue work on it tomorrow.

@beardnick
Copy link
Contributor Author

@Baoyuantop I took a more detailed look at the code. Seems this pr(#11987) did something similar to my pr. Do you think my pr is still necessary?

@jmaasing
Copy link

jmaasing commented Apr 6, 2025 •
edited
Loading

@Baoyuantop I took a more detailed look at the code. Seems this pr(#11987) did something similar to my pr. Do you think my pr is still necessary?

I'm not an apisix-developer but a user so I can't say anything about the implementation details. But I am looking to your PR to have the ability to configure the plugin to only allow requests through if the user has a "roles" claim containing one or more specific roles.

The PR you are referencing seems similar but geared towards checking the 'aud' claim only, which is nice but does not cover my use case.

@Baoyuantop
Copy link
Contributor

@Baoyuantop I took a more detailed look at the code. Seems this pr(#11987) did something similar to my pr. Do you think my pr is still necessary?

I will check it

@Baoyuantop
Copy link
Contributor

Hi @beardnick, I don't see this PR as conflicting with #11987, but rather as complementary features, with #11987 providing specific audience validation (in line with the OIDC specification) and #11824 providing a more generalized validation approach. cc @bzp2010

@beardnick beardnick force-pushed the feature-oidc-validate branch from 04c833d to ed02702 Compare May 5, 2025 07:59
@beardnick
Copy link
Contributor Author

Hi @beardnick, I don't see this PR as conflicting with #11987, but rather as complementary features, with #11987 providing specific audience validation (in line with the OIDC specification) and #11824 providing a more generalized validation approach. cc @bzp2010

Hi @Baoyuantop. Thank you for your help. My concern was that the APISIX team might not want to expose a flexible claim validator, like JSON Schema, to users. Since there is no design issue regarding this, I will continue working on this PR. I have updated the documentation.

@beardnick
Copy link
Contributor Author

Hi @Baoyuantop, it seems that the failed tests are not caused by my code. Could you please help me run them again?

@Baoyuantop
Copy link
Contributor

Already rerun, please make sure you have merged the latest master branch

@beardnick
Copy link
Contributor Author

Already rerun, please make sure you have merged the latest master branch

Hi @Baoyuantop, I've already merged the latest master. However, some tests still failed. Could you please help me rerun the failed tests?

@beardnick beardnick requested a review from Baoyuantop May 12, 2025 05:11
Baoyuantop
Baoyuantop reviewed May 12, 2025
View reviewed changes
Copy link
Contributor

@Baoyuantop Baoyuantop 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~

@beardnick beardnick requested a review from Baoyuantop June 14, 2025 08:37
@beardnick
Copy link
Contributor Author

@Baoyuantop Please review this PR again.

1 similar comment
@beardnick
Copy link
Contributor Author

@Baoyuantop Please review this PR again.

Baoyuantop
Baoyuantop reviewed Jun 26, 2025
View reviewed changes
@Baoyuantop Baoyuantop added the wait for update wait for the author's response in this issue/PR label Jul 3, 2025
@beardnick beardnick requested a review from Baoyuantop July 8, 2025 12:52
@beardnick
Copy link
Contributor Author

@Baoyuantop Please help me rerun the failed tests.

@Baoyuantop
Copy link
Contributor

The failed tests are not related to this PR.

@Baoyuantop Baoyuantop removed wait for update wait for the author's response in this issue/PR user responded labels Jul 10, 2025
Baoyuantop
Baoyuantop previously approved these changes Jul 10, 2025
View reviewed changes
membphis
membphis reviewed Jul 10, 2025
View reviewed changes
@beardnick beardnick requested review from membphis and Baoyuantop July 10, 2025 14:50
membphis
membphis approved these changes Jul 11, 2025
View reviewed changes
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM ^_^

@beardnick
Copy link
Contributor Author

@bzp2010 @nic-6443 @Revolyssup @AlinsRan cc

@beardnick
Copy link
Contributor Author

@bzp2010 @nic-6443 @Revolyssup @AlinsRan cc?

@Baoyuantop Baoyuantop merged commit 12b0824 into apache:master Jul 22, 2025
24 of 26 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in ⚡️ Apache APISIX Roadmap Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@membphis membphis membphis approved these changes

@Baoyuantop Baoyuantop Baoyuantop approved these changes

@AlinsRan AlinsRan AlinsRan approved these changes

@bzp2010 bzp2010 Awaiting requested review from bzp2010

@nic-6443 nic-6443 Awaiting requested review from nic-6443

@Revolyssup Revolyssup Awaiting requested review from Revolyssup

Assignees

@beardnick beardnick

Labels
enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.
Projects
⚡️ Apache APISIX Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: As a user, I want to set a validation on custom claims in oidc auth, so that I can restrict access to backend based on that rule
5 participants
@beardnick @Baoyuantop @jmaasing @membphis @AlinsRan