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

Fix google example #110

Merged
merged 1 commit into from Jul 14, 2015
Merged

Fix google example #110

merged 1 commit into from Jul 14, 2015

Conversation

@alanshaw
Copy link
Contributor

alanshaw commented Jun 29, 2015

This fixes the google example to use the location property not redirect_uri as this parameter is inferred from the current URL or the location setting if set. See https://github.com/hapijs/bell/blob/master/lib/oauth.js#L156

@qualquervalor

This comment has been minimized.

Copy link

qualquervalor commented Jun 30, 2015

I think it is more instructive that it continues to use the redirect_uri. When you are using the Google Developer Console to setup credentials for your web app, you establish a client id, client secret and a redirect URI. So, continuing to use redirect_uri at the same time as the clientId and clientSecret in the example feels intuitive.

@alanshaw

This comment has been minimized.

Copy link
Contributor Author

alanshaw commented Jun 30, 2015

...but if you look at the code you'll see that whatever value you set for redirect_uri is completely discarded. You cannot influence the redirect_uri unless you use the location setting.

I think it's misleading to tell people to set a config parameter that isn't used.

In my use case I'm proxying from port 80 to 3000 using nginx - I was using the redirect_uri config option to set it to https://mydomain.com, but google was still trying to redirect to localhost:3000.

@nelsonic

This comment has been minimized.

Copy link

nelsonic commented Jun 30, 2015

👍

@qualquervalor

This comment has been minimized.

Copy link

qualquervalor commented Jun 30, 2015

So, maybe the issue is that the redirect_uri is being overwritten. The providerParams are read in https://github.com/hapijs/bell/blob/master/lib/oauth.js#L149 and then the query.redirect_uri is being overwritten on https://github.com/hapijs/bell/blob/master/lib/oauth.js#L156. What if the line was change to check for the redirect_uri before overwriting it?

query.redirect_uri =  query.redirect_uri  || internals.location(request, protocol, settings.location);

Also, If one example was going to be updated for this change, some of the others also use redirect_uri the same way and would need the same change.

@alanshaw

This comment has been minimized.

Copy link
Contributor Author

alanshaw commented Jun 30, 2015

Whatever works best for you. I did consider doing that, but didn't know if it was by design that the redirect_uri was overwritten and normally prefer not to make a breaking change to the code if possible.

Updating the example seemed like a much less disruptive way of stopping others coming across the same problem. Happy to update the other examples if you like...also happy with the XOR solution.

@geek geek added the bug label Jul 14, 2015
@geek geek added this to the 5.0.0 milestone Jul 14, 2015
@geek geek self-assigned this Jul 14, 2015
@geek geek added example and removed bug labels Jul 14, 2015
geek added a commit that referenced this pull request Jul 14, 2015
Fix google example
@geek geek merged commit 71c248b into hapijs:master Jul 14, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Marsup Marsup added documentation and removed example labels Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.