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

Add mutator for modifying authenticationSession with external API #240

Merged
merged 16 commits into from
Aug 16, 2019

Conversation

kubadz
Copy link
Contributor

@kubadz kubadz commented Aug 12, 2019

Related issue

#228
@aeneasr @piotrmsc

Proposed changes

As stated in the linked issue - this PR adds a mutator which make an upstream call to an API with AuthenticationSession as payload and expect AuthenticationSession as a response.

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 necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

Further comments

@aeneasr
Copy link
Member

aeneasr commented Aug 12, 2019

sorry - I squash-merged the other PR causing some merge conflicts

@kubadz
Copy link
Contributor Author

kubadz commented Aug 12, 2019

sorry - I squash-merged the other PR causing some merge conflicts

No problem :) fortunately, there are not many conflicts

@aeneasr
Copy link
Member

aeneasr commented Aug 13, 2019

This looks great!! All that's left is adding docs here: https://github.com/ory/docs/blob/master/docs/oathkeeper/pipeline/mutator.md :)

…rovide a test case for request with empty Extras field

type externalAPIConfig struct {
Url string `json:"url"`
Authn *Authentication `json:"authn,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This would probably be authorization right? Because we're modifying HTTP Authorization?

Copy link
Contributor Author

@kubadz kubadz Aug 14, 2019

Choose a reason for hiding this comment

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

Sure, we are modifying HTTP Authorization header, but it is a header used for authentication. And the config is actually specifying Authentication scheme and credentials. But if this argumentation doesn't convince you and you prefer it to be Authorization I will not argue to change that 😄

Copy link
Member

Choose a reason for hiding this comment

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

We could also just change it to auth, then it implies both 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, changed it. Let me know what you think about it now 😄
I will adjust documentation in a second

@aeneasr aeneasr merged commit b38b0f4 into ory:master Aug 16, 2019
@aeneasr
Copy link
Member

aeneasr commented Aug 16, 2019

Wohoooo!

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.

3 participants