Skip to content

Change the default port to 8090 #89

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

Closed
wants to merge 1 commit into from
Closed

Change the default port to 8090 #89

wants to merge 1 commit into from

Conversation

erickoledadevrel
Copy link

The default port 8080 is more likely to be in use, and the library doesn't handle that case gracefully. Fixes #86.

@erickoledadevrel erickoledadevrel requested a review from sqrrrl March 7, 2019 02:12
@sqrrrl
Copy link
Member

sqrrrl commented Mar 7, 2019

This doesn't fix anything. Port conflicts will still happen any time a port is hardcoded like that.

The correct fix is to make a fix in the auth library. You can pass in port 0 which instructs the OS to pick an unused ephemeral port. That should be the default, and for localhost redirect URIs the port is always ignored for this reason -- to allow random ports to be used at runtime.

Anyway, it looks like you can pass port 0 in. The only bug is the auth library won't create the correct redirect URL since it creates the URL before the server port is known.

https://github.com/GoogleCloudPlatform/google-auth-library-python-oauthlib/blob/master/google_auth_oauthlib/flow.py#L401

Switching the order of operations and using local_server.port as the port value would fix this for real.

@sqrrrl
Copy link
Member

sqrrrl commented Mar 7, 2019

Sent a PR to fix this in the auth lib. Once that's in, and assuming we don't make it the default, then the fix here would be to upgrade to that version and then use port 0.

@erickoledadevrel
Copy link
Author

Agreed this change doesn't fix the problem in all cases, just made the situation somewhat more rare. Your solution is much better, and not too far off, so I'll close this PR.

@erickoledadevrel erickoledadevrel deleted the port branch March 8, 2019 02:16
@sqrrrl sqrrrl restored the port branch March 8, 2019 02:16
@sqrrrl
Copy link
Member

sqrrrl commented Mar 8, 2019

Let's keep open. Even with my fix, we still probably need to change the port number to take advantage of it.

@erickoledadevrel
Copy link
Author

This PR was just a simple find and replace over the codebase. Changing the port to 0 is just as much work as starting from scratch.

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