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

server: fix registration redirect for non-root URLs #558

Merged

Conversation

ericchiang
Copy link
Contributor

Closes #553

cc @sym3tri @chancez

@ericchiang
Copy link
Contributor Author

there's even a test!

@@ -358,7 +358,7 @@ func handleAuthFunc(srv OIDCServer, idpcs []connector.Connector, tpl *template.T
if ok {
q := url.Values{}
q.Set("code", key)
ru := httpPathRegister + "?" + q.Encode()
ru := strings.TrimRight(baseURL, "/") + "/" + strings.TrimLeft(httpPathRegister, "/") + "?" + q.Encode()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you maybe use path.Join or url.Parse instead? It should remove some of the need to trim, but is also more likely work correctly with joining paths together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll switch it to passing a URL and use path.Join

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it seems weird that you're having to pass the basePath through to so many different places. Is this a thing you only need to do here due to needing to set the Location header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@ericchiang ericchiang force-pushed the fix-local-registration-for-non-root-url branch from e7c15a4 to 47a6a8b Compare August 18, 2016 22:52
@ericchiang
Copy link
Contributor Author

updated

@ericchiang
Copy link
Contributor Author

woops, I have no idea what just happened there. one sec

@ericchiang ericchiang force-pushed the fix-local-registration-for-non-root-url branch from 47a6a8b to 00d5aa9 Compare August 19, 2016 22:49
@ericchiang
Copy link
Contributor Author

cc @chancez ready for another review

@ericchiang ericchiang force-pushed the fix-local-registration-for-non-root-url branch from 00d5aa9 to 22e7377 Compare August 19, 2016 23:07
@@ -210,6 +226,33 @@ func TestHandleAuthFuncResponsesSingleRedirectURL(t *testing.T) {
},
wantCode: http.StatusBadRequest,
},

// reigstration
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@chancez
Copy link
Contributor

chancez commented Aug 19, 2016

LGTM after nits.

@ericchiang ericchiang force-pushed the fix-local-registration-for-non-root-url branch from 22e7377 to fa8f98a Compare August 19, 2016 23:25
@ericchiang ericchiang merged commit 7caaca9 into dexidp:master Aug 19, 2016
@ericchiang ericchiang deleted the fix-local-registration-for-non-root-url branch November 22, 2016 20:06
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