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: allow to disable claim mirroring #3563

Merged
merged 8 commits into from
Aug 11, 2023

Conversation

dastein1
Copy link
Contributor

@dastein1 dastein1 commented Jul 5, 2023

This PR introduces another config option called oauth2:mirror_top_level_claims which may be used to disable the mirroring of custom claims into the ext claim of the jwt.
This new config option is an opt-in. If unused the behavior remains as-is to ensure backwards compatibility.

Example:

oauth2:
  allowed_top_level_claims:
    - test_claim
  mirror_top_level_claims: false # -> this will prevent test_claim to be mirrored within ext

Related issue(s)

#3348

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@dastein1 dastein1 requested a review from aeneasr as a code owner July 5, 2023 11:45
@CLAassistant
Copy link

CLAassistant commented Jul 5, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #3563 (135da47) into master (6741a49) will increase coverage by 0.10%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 135da47 differs from pull request most recent head e1fa88f. Consider uploading reports for the commit e1fa88f to get more accurate results

@@            Coverage Diff             @@
##           master    #3563      +/-   ##
==========================================
+ Coverage   76.24%   76.34%   +0.10%     
==========================================
  Files         132      132              
  Lines        9901     9888      -13     
==========================================
  Hits         7549     7549              
+ Misses       1837     1824      -13     
  Partials      515      515              
Files Changed Coverage Δ
driver/config/provider.go 83.01% <100.00%> (+0.12%) ⬆️
oauth2/handler.go 67.62% <100.00%> (+0.06%) ⬆️
oauth2/session.go 82.08% <100.00%> (+1.76%) ⬆️
x/oauth2cors/cors.go 87.14% <100.00%> (ø)

... and 5 files with indirect coverage changes

@kmherrmann kmherrmann requested a review from hperl July 20, 2023 09:29
Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

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

This looks very good already, thanks for the contribution!

I had only one remark, please see below.

}

func NewSessionWithCustomClaims(subject string, allowedTopLevelClaims []string) *Session {
func NewSessionWithCustomClaims(subject string, allowedTopLevelClaims []string, mirrorTopLevelClaims bool) *Session {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing each config option, we could pass the configuration provider here and read out the config values in the function body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @hperl. The methods on the config provider require the context. So I've changed the signature to accept ctx first followed by the config provider.

@dastein1 dastein1 requested a review from hperl August 6, 2023 11:00
Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the contribution 🎉

@aeneasr aeneasr merged commit c72a316 into ory:master Aug 11, 2023
32 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.

5 participants