- 
                Notifications
    You must be signed in to change notification settings 
- Fork 524
SNOW-2194055: Separate server and redirect URIs in AuthHttpServer #2609
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
base: main
Are you sure you want to change the base?
SNOW-2194055: Separate server and redirect URIs in AuthHttpServer #2609
Conversation
a57bbe5    to
    00665af      
    Compare
  
    |  | ||
| def _read_uri_from_env(self) -> str: | ||
| oauth_socket_address = os.getenv( | ||
| ENV_VAR_OAUTH_SOCKET_ADDRESS, "http://localhost" | 
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.
localhost, not http://localhost ?
Generally, this separation of host and port in this way rather than having the address specified as a hostport seems weird to me. Can you ensure the description explains why were are doing it this way?
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.
Left minor comment regarding the flow of configuration
| with AuthHttpServer(self._redirect_uri) as callback_server: | ||
| with AuthHttpServer( | ||
| redirect_uri=self._redirect_uri, | ||
| uri=self._read_uri_from_env(), | 
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.
Do we intend to expose this only through this env variable? Or should customer be able to pass oauth_socket_address through connection params? Is it known at the start time of the application and should be only possible to override with this env var?
Anyway we may need to create a separate Jira ticket to update the docs to include this new parameter in OAuth documentation for Python Driver.
Fixes support for
redirect_uri. Copy of #2400