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

Added Option to Use URLs Exactly As Specified #97

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mdemoret
Copy link

I added in a simple option useExactURLs that prevents the following code from being executed if the callbackURL is missing the HTTP protocol:

var parsed = url.parse(callbackURL);
if (!parsed.protocol) {
  // The callback URL is relative, resolve a fully qualified URL from the
  // URL of the originating request.
  callbackURL = url.resolve(utils.originalURL(req, { proxy: this._trustProxy }), callbackURL);
}

This was needed due to an issue I encountered trying to use JupyterHub (https://github.com/jupyterhub/jupyterhub) as an OAuth2 provider. JupyterHub is expecting the callbackURL to be a relative path. If it is anything different (for example, http://localhost:8000/my_service/oauth_callback vs /my_service/oauth_callback) the process fails and the callback is never called.

The only way to fix this (that I could think of) was to force passport-oauth2 to keep the URLs exactly as they are typed, or to change JupyterHub's authentication to accept more URLs. I can only assume that the JupyterHub team assumed that since every service (necessary to get the client_id) runs as a child process, that they will all be on the same hostname and relatives paths would be all that's necessary.

I chose to add the useExactURLs option since its a much smaller fix and its possible someone else might could benefit from the additional option in the future. If you can think of something better for the option name than useExactURLs please let me know. I struggled to come up with something concise that would also have a default value of false.

FYI, I will be submitting an issue to the JupyterHub team as well. Hopefully, they can remedy this issue in one of their future releases.

@mdemoret
Copy link
Author

Oh also, I added the typings file for Typescript support. I took this directly from @Typings/passport-oauth2

@coveralls
Copy link

coveralls commented Feb 16, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 360bb6a on mdemoret:master into a948096 on jaredhanson:master.

@roboli
Copy link

roboli commented Feb 23, 2018

Any chance this merge happening soon? I'm having a similar issue, but URL is not even a relative URL but a simple string value which resolves to an URL value on the provider side, but I believe this PR will help.

@mdemoret
Copy link
Author

Feel free to use my fork with the changes until this gets merged: https://github.com/mdemoret/passport-oauth2

You can add this to your packages with npm install -P git+https://github.com/mdemoret/passport-oauth2.git

If/when it gets merged, it should be very simple to switch back.

@roboli
Copy link

roboli commented Mar 17, 2018

Hi again... So I have this https://github.com/roboli/passport-ebay ... Hopefully this PR will be merge soon.

@rwky
Copy link

rwky commented Jul 7, 2018

@mdemoret if you make a PR against https://github.com/passport-next/passport-oauth2 is will get looked into.

@mdemoret
Copy link
Author

@rwky Its been a while since I worked on my Oauth stuff so forgive my ignorance, but how is passport-next different than its predecessors? Would love to get this merged since using a github repo doesnt always work well in npm.

@rwky
Copy link

rwky commented Jul 10, 2018

@mdemoret Passport next is managed by an organisation so if one maintainer goes away it will still be maintained and other devs can be added unlike this library which has a sole maintainer and no one else can release to npm. You can install passport next via npm install @passport-next/passport-oauth2

@mdemoret
Copy link
Author

@rwky Great. I'll make a pull request soon. Shouldn't take long. The changes were minor.

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.

4 participants