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: Ability to Configure Remote Authorizers to set Headers in AuthenticationSession #717

Merged
merged 8 commits into from
May 24, 2021
Merged

Conversation

wesleyfantinel
Copy link
Contributor

@wesleyfantinel wesleyfantinel commented May 4, 2021

Related issue

Proposed changes

The remote authorizers may have useful context from user's permissions. So with this changes, custom authorizers using remote and remote_json can return some useful headers to be forward into the AuthenticationSession, meaning that these headers will be passed to upstream services.

For example, an user containing scopes/branches inside an organization profile has some level of data addressed to him. In this case, the upstream service need to know that, and "filter" the data according to his "branch_id". The permission that is given to the user (and the remote authorizers manages) has a record of the "branch_id", for the following responses will be returned as status code 200 (if granted) and containing a header like X-Branch-Id.

The upstream service receives the X-Branch-Id and does your thing.

The configuration requires to configure a list of "allowed headers" returning from remote authorizer, that will be accepted in the pipeline.


Without this, we have to perform another request using the mutator Hydrador to retrieve the users' "branch_id", that is not a good performance approach in my vision for this set.

Checklist

  • I have read the contributing 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 green light (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.

Further comments

@wesleyfantinel wesleyfantinel changed the title feat: Ability to Configure Authorizers to "mutate" Headers into AuthenticationSession feat: Ability to Configure Remote Authorizers to "mutate" Headers into AuthenticationSession May 5, 2021
@wesleyfantinel wesleyfantinel changed the title feat: Ability to Configure Remote Authorizers to "mutate" Headers into AuthenticationSession feat: Ability to Configure Remote Authorizers to set Headers in AuthenticationSession May 5, 2021
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.

Thank you for the contribution! Unfortunately we can not accept contributions without tests. These are important to ensure that your feature does what it should, and it ensures that it keeps working even if others change your code!

if you need help with the tests feel free to ping a maintainer!

@wesleyfantinel
Copy link
Contributor Author

Ok, sending the tests.

@aeneasr
Copy link
Member

aeneasr commented May 11, 2021

Looks like tests are failing for the new feature, could you take a look maybe? 🧐

@wesleyfantinel
Copy link
Contributor Author

Alright, got it fixed.

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.

This looks ready to be merged! The tests look great and the implementation too. I just would like to ask to rename the field to make it more obvious what it does :)

docs/docs/pipeline/authz.md Outdated Show resolved Hide resolved
.schema/config.schema.json Outdated Show resolved Hide resolved
.schema/config.schema.json Outdated Show resolved Hide resolved
.schemas/authorizers.remote_json.schema.json Outdated Show resolved Hide resolved
docs/docs/pipeline/authz.md Outdated Show resolved Hide resolved
docs/docs/pipeline/authz.md Outdated Show resolved Hide resolved
internal/config/.oathkeeper.yaml Outdated Show resolved Hide resolved
internal/config/.oathkeeper.yaml Outdated Show resolved Hide resolved
pipeline/authz/remote.go Outdated Show resolved Hide resolved
pipeline/authz/remote_json.go Outdated Show resolved Hide resolved
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.

Great job, thank you for your hard work!

@aeneasr aeneasr merged commit b3d117b into ory:master May 24, 2021
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.

2 participants