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

redirect to original path after login #24

Merged
merged 4 commits into from Jan 29, 2019
Merged

redirect to original path after login #24

merged 4 commits into from Jan 29, 2019

Conversation

agentgonzo
Copy link
Contributor

@agentgonzo agentgonzo commented Jan 23, 2019

Description

After the user logs in, redirect them to their original URL rather than just /

Motivation and Context

The user wants to go to their destination, no the root URL.

How Has This Been Tested?

Run it against an upstream. Trash all cookies, then try to access it with a non-root URL

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@agentgonzo agentgonzo requested a review from a team January 23, 2019 16:33
@JoelSpeed JoelSpeed self-assigned this Jan 25, 2019
@JoelSpeed JoelSpeed added the bug label Jan 25, 2019
@JoelSpeed
Copy link
Member

I like the idea behind this PR, but at the moment it would break people using the auth_request directive as described in the documentation.

If I'm using the auth_request directive then Nginx redirects users to /oauth2/sign_in to perform the login and this means req.URL.Path would be /oauth2/sign_in which would mean people would get into a loop of being asked to sign in. We need to work out a way to stop that happening before we can add this enhancement. Any ideas?

@agentgonzo
Copy link
Contributor Author

agentgonzo commented Jan 25, 2019

pseudocode:

if originalrequest.startswith('/oauth)
    redirect = '/'
else
    redirect = originalPath

?

@JoelSpeed
Copy link
Member

JoelSpeed commented Jan 25, 2019

I've just realised that's what

if strings.HasPrefix(redirect, p.ProxyPrefix) {
			redirect = "/"
}

is for... Ignore my previous comment

@JoelSpeed
Copy link
Member

Could you possibly add a test for the GetRedirect method that tests a couple of cases (in particular a random original path and then also one which contains the p.ProxyPrefix) please?

@agentgonzo
Copy link
Contributor Author

Will do. Probably to come on Monday

@agentgonzo
Copy link
Contributor Author

@JoelSpeed - tests added. Let me know if you need anything else

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

One minor nit then it's good to go! Thanks

expectedRedirect: "/foo/bar",
},
{
name: "request under of ProxyPrefix redirects to original URL",
Copy link
Member

Choose a reason for hiding this comment

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

This comment isn't quite right

Suggested change
name: "request under of ProxyPrefix redirects to original URL",
name: "request under ProxyPrefix redirects to root",

@agentgonzo
Copy link
Contributor Author

fixed

JoelSpeed
JoelSpeed previously approved these changes Jan 29, 2019
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Could you add a note to the Changelog noting this bug fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants