passport-oauth2 icon indicating copy to clipboard operation
passport-oauth2 copied to clipboard

Added Option to Use URLs Exactly As Specified

Open mdemoret opened this issue 7 years ago • 9 comments

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 avatar Feb 16 '18 18:02 mdemoret

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

mdemoret avatar Feb 16 '18 18:02 mdemoret

Coverage Status

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

coveralls avatar Feb 16 '18 18:02 coveralls

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.

roboli avatar Feb 23 '18 17:02 roboli

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.

mdemoret avatar Feb 23 '18 21:02 mdemoret

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

roboli avatar Mar 17 '18 16:03 roboli

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

rwky avatar Jul 07 '18 15:07 rwky

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

mdemoret avatar Jul 10 '18 19:07 mdemoret

@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

rwky avatar Jul 10 '18 21:07 rwky

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

mdemoret avatar Jul 10 '18 21:07 mdemoret