Lokasi ngalangkungan proxy:   [ UP ]  
[Ngawartoskeun bug]   [Panyetelan cookie]                
Skip to content

feat: allow to disable claim mirroring#3563

Merged
aeneasr merged 8 commits into
ory:masterfrom
dastein1:feat/allow-to-disable-claims-mirroring
Aug 11, 2023
Merged

feat: allow to disable claim mirroring#3563
aeneasr merged 8 commits into
ory:masterfrom
dastein1:feat/allow-to-disable-claims-mirroring

Conversation

@dastein1

@dastein1 dastein1 commented Jul 5, 2023

Copy link
Copy Markdown
Contributor

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
    security@ory.sh) 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

CLAassistant commented Jul 5, 2023

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Jul 5, 2023

Copy link
Copy Markdown

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

@hperl hperl left a comment

Copy link
Copy Markdown
Member

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.

Comment thread oauth2/session.go Outdated
}

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

Copy link
Copy Markdown
Member

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
Copy Markdown
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

@hperl hperl left a comment

Copy link
Copy Markdown
Member

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