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 url_param config option to redirect error handler. #511

Closed

Conversation

marcinkoziej
Copy link

This change introduces a url_param config option for redirect error handler.
If it contains a url paramter name, the redirect url will have this parameter
set, containing the current url (from which Oathkeeper has redirected the user).

This can be useful in passing the return_to url to Kratos, so user can be
redirected to the page they initially wanted to access after a successfull sign in.

Related issue

https://community.ory.sh/t/how-to-use-the-return-to-query-string-with-oathkeeper-kratos/2149/6

Proposed changes

Add url_param option to redirect error config, which configures a name of paramter that should contain current url (the url that failed the pipeline).

If url_param is given, the redirect error handler will parse the to: url and append url_param parameter with current url.

Checklist

⚠️ Disclaimer: I am not a Golang coder. I'd appreciate the feedback on this patch and I intend to make it better. I have not written documentation yet – unsure if my approach is ok.

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

This change introduces a url_param config option for redirect error handler.
If it contains a url paramter name, the redirect url will have this parameter
set, containing the current url (from which Oathkeeper has redirected the user).

This can be useful in passing the return_to url to Kratos, so user can be
redirected to the page they initially wanted to access after a successfull sign in.
@CLAassistant
Copy link

CLAassistant commented Sep 7, 2020

CLA assistant check
All committers have signed the CLA.

@marcinkoziej marcinkoziej changed the title Add url_param config option to redirect error handler. feat: Add url_param config option to redirect error handler. Sep 7, 2020
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice, thanks for your contribution 👍
I just found some go specific mistakes.

pipeline/errors/error_redirect.go Outdated Show resolved Hide resolved
pipeline/errors/error_redirect.go Outdated Show resolved Hide resolved
pipeline/errors/error_redirect.go Outdated Show resolved Hide resolved
pipeline/errors/error_redirect.go Outdated Show resolved Hide resolved
pipeline/errors/error_redirect.go Outdated Show resolved Hide resolved
pipeline/errors/error_redirect_test.go Outdated Show resolved Hide resolved
@marcinkoziej
Copy link
Author

Thank You so much for the fixes! Oyoyoy that shadowing of a library was nasty >:-}

pipeline/errors/error_redirect.go Outdated Show resolved Hide resolved
pipeline/errors/error_redirect.go Outdated Show resolved Hide resolved
@marcinkoziej
Copy link
Author

Sorry, I clicked accept suggestions which resulted in multiple commints. This PR should be merged-squashed probably.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice thanks

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! I have some ideas how to improve this. Could you also please add some docs to: https://github.com/ory/oathkeeper/blob/master/docs/docs/pipeline/error.md#redirect ? :)

@@ -260,6 +260,13 @@
],
"default": 302
},
"url_param": {
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename this to return_to_query_param to make it more obvious :)

Copy link
Author

Choose a reason for hiding this comment

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

@aeneasr done

.schema/config.schema.json Outdated Show resolved Hide resolved
@marcinkoziej
Copy link
Author

@aeneasr Hi, is there some way I can improve on this PR more?

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, I'm currently on vacation which is why these reviews take a bit longer :) This looks great! Could you please resolve the merge conflicts? Then I'll be merging it right away! :)

@maxbublik
Copy link

@marcinkoziej Great job!
Looking forward to that feature very much.
Unfortunately I'm not a Go developer neither :(

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.

Awesome 🎉

Thank you for your contribution!

@aeneasr aeneasr closed this in b5bb3bc Sep 21, 2020
@maxbublik
Copy link

@marcinkoziej @aeneasr Thanks a lot! I have compiled oathkeeper from source and the feature works!

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