-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Use new OAuth endpoint for login #4267
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
Conversation
// Login walks through the login flow for obtaining a Gitpod token | ||
func Login(ctx context.Context, opts LoginOpts) (token string, err error) { | ||
rl, err := net.Listen("tcp", "localhost:0") | ||
rl, err := net.Listen("tcp", "localhost:64110") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon the "use unused port" way is less error prone - or am I missing a reason to hardcode this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but yes, (unfortunately) the redirect URL has to be known to the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be configurable if the port is already used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not yet at least. The backend validates the full redirect uri, including port, so it would need a way to override that behaviour (which might be worth PR-ing on the lib).
In the interim I've changed it, along with the backend, to try a range of 10 ports and be more explicit about using IPV4 to workaround issues with IPV6 I saw while testing.
c33c7ca
to
3ffccb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying on several ports is a great first step towards making this more resilient.
I tried the login process as part of the OAuth PR. Changes LGTM.
port := STARTING_PORT_NUM | ||
for port < ENDING_PORT_NUM { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more classic for
loop would be easier to understand here I reckon (no searching for port++
).
Remove testing code Use correct scope for all workspaces Clean up logging + error behaviour Use tcp4 and 127.0.0.1 to avoid IPV6 issues
3ffccb8
to
882be00
Compare
/werft run 👍 started the job as gitpod-build-rl-local-app-oauth.7 |
Remove testing code
Use correct scope for all workspaces
Clean up logging + error behaviour