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

fix: noop mutator don't overwrite session headers #1091

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

davidspek
Copy link
Contributor

@davidspek davidspek commented Apr 13, 2023

This PR changes the noop mutator so that it doesn't overwrite the current session headers with the request headers.

When using the remote_json authorizer, it is possible to configure headers returned by the remote authorizer that will be forwarded to upstream services with the forward_response_headers_to_upstream field. This effectively allows the authorizer to also act as a mutator. Since rules must contain a mutator, if no further mutations need to be performed you would specify the noop mutator. However, since the noop mutator is setting the session headers to be equal to the request headers, this effectively overwrites the headers set by the remote_json authorizer.

Related issue(s)

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.

Further Comments


@davidspek davidspek requested a review from aeneasr as a code owner April 13, 2023 17:10
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #1091 (887bdc2) into master (0a767e7) will decrease coverage by 0.08%.
The diff coverage is 50.00%.

❗ Current head 887bdc2 differs from pull request most recent head 206e1e1. Consider uploading reports for the commit 206e1e1 to get more accurate results

@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
- Coverage   78.24%   78.17%   -0.08%     
==========================================
  Files          80       80              
  Lines        3843     3853      +10     
==========================================
+ Hits         3007     3012       +5     
- Misses        564      566       +2     
- Partials      272      275       +3     
Impacted Files Coverage Δ
pipeline/mutate/mutator_noop.go 69.56% <50.00%> (-15.06%) ⬇️

@davidspek davidspek force-pushed the remote-json-header-response branch from a555a26 to b657de3 Compare June 21, 2023 14:58
@davidspek davidspek requested a review from aeneasr June 21, 2023 15:02
@davidspek
Copy link
Contributor Author

Friendly ping @zepatrik maybe you can also have a look here.

Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
@davidspek davidspek force-pushed the remote-json-header-response branch from b657de3 to 206e1e1 Compare June 26, 2023 12:56
@davidspek
Copy link
Contributor Author

Ping @aeneasr @zepatrik

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Sorry for the long review, this LGTM!

@aeneasr aeneasr merged commit 3a716f2 into ory:master Jul 5, 2023
21 checks passed
@davidspek
Copy link
Contributor Author

No problem at all :)

@davidspek davidspek deleted the remote-json-header-response branch July 10, 2023 11:39
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.

None yet

2 participants