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: add request header in match context #719

Merged
merged 6 commits into from
May 14, 2021
Merged

Conversation

msumit
Copy link
Contributor

@msumit msumit commented May 5, 2021

Related issue

#512

Proposed changes

After wasting a lot of time on why I'm not able to use Headers within the Remote Authorizer, I found out that Oathkeeper isn't setting the Headers while initializing the AuthenticationSession. We use Remote Authorizer and pass on some key information through headers only & without this fix, we can't use Oathkeeper at all, so setting the request headers inside MatchContext.

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

See this PR -> #718

@@ -491,6 +496,7 @@ func TestInitializeSession(t *testing.T) {
RegexpCaptureGroups: []string{"user"},
URL: x.ParseURLOrPanic("http://localhost/user"),
Method: "GET",
Header: newTestHeader(),

Choose a reason for hiding this comment

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

i think for expected results better to use constant / or literal instead of function

@msumit
Copy link
Contributor Author

msumit commented May 6, 2021

@aeneasr can you plz review.

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 this looks great and sorry for the late review - last week was hectic :)

Could you please also update the documentation in docs/docs on this? Thank you!

@aeneasr
Copy link
Member

aeneasr commented May 11, 2021

Thank you, this looks great! The CI is failing because some files are formatted incorrectly. To format them, run:

$ make format
$ git commit -a -m "styles: format code"
$ git push

Thank you!

@aeneasr
Copy link
Member

aeneasr commented May 11, 2021

Hmmm...why isn't the pipeline triggering -.-

@msumit
Copy link
Contributor Author

msumit commented May 11, 2021

@aeneasr can you figure out why the windows go test is failing?

@aeneasr
Copy link
Member

aeneasr commented May 12, 2021

Yeah it's not your fault!

@aeneasr
Copy link
Member

aeneasr commented May 12, 2021

But ci/circleci: test should be running and it's not :/ I have one guide regarding this:


Unfortunately, for some reason, the CircleCI tests are not running. Do you maybe follow your/a fork of this repository on CircleCI? If so, you need to unsubscribe / unwatch from that CircleCI project. Then, make another push to your branch using:

$ git commit --amend --allow-empty
$ git push --force

That should get the CI running! Thank you :)

@msumit
Copy link
Contributor Author

msumit commented May 12, 2021

@aeneasr I think you need to approve something somewhere to kick off the tests.
First-time contributors need a maintainer to approve running workflows. Learn more.

@msumit
Copy link
Contributor Author

msumit commented May 14, 2021

@aeneasr can you look into this

@aeneasr
Copy link
Member

aeneasr commented May 14, 2021

Hey there, sorry for the long delay - yesterday was a holiday :) I've approved the run. I've also create a post on GH community regarding this as the current approval behavior is so annoying.

@aeneasr
Copy link
Member

aeneasr commented May 14, 2021

Hm no, this looks like a circle ci issue. I'll run the tests locally

@aeneasr aeneasr merged commit 22b0dbe into ory:master May 14, 2021
@aeneasr
Copy link
Member

aeneasr commented May 14, 2021

Awesome, thank you! 🎉 Your contribution makes Ory better :)

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