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

Allow dex to work at non base url #521

Merged

Conversation

ericchiang
Copy link
Contributor

This change updates #520 by having the handlers register using the issuerURL. It also updates the development environment by having the dex-worker and examle-app default to a non-base URL. This will hopefully catch any future bugs that assume operating from a root path.

Going to have to manually test all the connectors and registration flows :( Until then, do not merge.

Closes #502

@ericchiang
Copy link
Contributor Author

@xaka has noted that most proxies trim the prefix of all paths. For instance the following nginx config trims /test/ from the path before

location /test/ {
  proxy_pass http://localserver.com/;
}

Therefore, while URLs rendered in the HTML need to be absolute from the issuer URL, the actual paths that handlers are registered on shouldn't be.

@alon-argus was wondering if you had any comments on this point.

@ericchiang ericchiang force-pushed the allow-dex-to-work-at-non-base-url branch from c3c23f0 to 435cadf Compare July 26, 2016 00:07
@ericchiang
Copy link
Contributor Author

Have gone through the local and gmail flows with registration and email verification. Think I got all the edge cases.

@ericchiang
Copy link
Contributor Author

Going to do another round of manual testing and add some documentation to the issuer flat.

@ericchiang
Copy link
Contributor Author

@chancez or @xaka want to take a look at this? I've run through GitHub, Google, LDAP, and local connector and they all work.

@xaka
Copy link
Contributor

xaka commented Jul 26, 2016

tested locally with http proxy, etc. so LGTM 👍

@ericchiang ericchiang merged commit d1bb106 into dexidp:master Jul 26, 2016
roidelapluie added a commit to roidelapluie/dex that referenced this pull request Aug 15, 2016
The discovery URL has changed and now ends with /dex. As the scripts
were updated in dexidp#525, the documentation was not.

Broken since dexidp#521.

[skip ci]

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
remohammadi added a commit to remohammadi/dex that referenced this pull request Sep 4, 2016
* In a9dce1c, the defaults are set to the `.example` paths, except for `emailer.json` which is instead committed into the git repository. So there's no need to duplicate those file in order to start dex-worker.
* The default value for discovery is moved from `/` to `/dex` in dexidp#521
* Typo in `client-secret` value.
@ericchiang ericchiang deleted the allow-dex-to-work-at-non-base-url branch November 22, 2016 20:07
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.

None yet

2 participants