fix: honor external_browser_timeout in the externalbrowser flow - #3046
Open
inchang-ing wants to merge 1 commit into
Open
inchang-ing wants to merge 1 commit into
inchang-ing wants to merge 1 commit into
Conversation
AuthByWebBrowser._receive_saml_token waited for the browser callback with select.select() and no timeout, so a login that was never completed blocked the connection forever; neither login_timeout nor external_browser_timeout bounded that wait (snowflakedb#3045). Pass external_browser_timeout down from SnowflakeConnection and use it as an overall deadline for the callback wait. When the budget is spent the authentication fails with ER_OAUTH_SERVER_TIMEOUT and the message the OAuth authorization-code flow already uses for its callback timeout - the two tests in test/auth/test_external_browser.py that are skipped pending SNOW-2007651 assert exactly that message, so this follows the behaviour already planned there rather than inventing a new errno. When external_browser_timeout is not configured the wait stays unbounded, preserving the behaviour of direct AuthByWebBrowser users. The same unbounded select.select() exists in aio/auth/_webbrowser.py; it is left untouched here to keep this change focused, and can be fixed the same way in a follow-up. Fixes snowflakedb#3045 Signed-off-by: inchang-ing <197932532+inchang-ing@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3045.
AuthByWebBrowser._receive_saml_tokenwaited for the browser callback withselect.select([socket_connection], [], [])and no timeout, so when the login was never completed the connection blocked forever —login_timeoutandexternal_browser_timeouthad no effect on that wait.This change passes
external_browser_timeoutfromSnowflakeConnectionintoAuthByWebBrowserand uses it as an overall deadline for the callback wait. When the budget is spent, the authentication fails withER_OAUTH_SERVER_TIMEOUTand the message the OAuth authorization-code flow (_receive_authorization_callback) already uses for its callback timeout. The two tests intest/auth/test_external_browser.pythat are currentlyskipped pendingSNOW-2007651("Adding custom browser timeout") assert exactly that message, so this follows the behaviour already planned there rather than introducing a new errno.When
external_browser_timeoutis not configured, the wait stays unbounded (we passNonethrough), preserving the behaviour of directAuthByWebBrowserusers.Reproduction
Offline (no account, no browser) —
external_browser_timeout=3and a stubbed SSO URL:Before the fix the same script hangs well past the timeout (verified on 4.8.0 / main).
Verification
test/unit/test_auth_webbrowser.py:test_auth_webbrowser_fails_when_browser_login_is_never_completed—external_browser_timeoutset,select.selectnever reports the socket readable →_handle_failureis called once withER_OAUTH_SERVER_TIMEOUT.test_auth_webbrowser_without_timeout_keeps_the_wait_unbounded— without the timeout,select.selectstill getsNonefor its timeout argument.select.selecttimeout is reverted, so they guard the regression.test_auth_webbrowser_get/test_auth_webbrowser_post) mockselect.selectto return the socket, so they are unaffected by the new argument.Scope note
The same unbounded
select.select()exists inaio/auth/_webbrowser.py. I left it untouched to keep this change focused and easily reviewable; it can be fixed the same way in a follow-up if you'd like.(Generated with the help of an AI coding assistant; the root-cause analysis and the version matrix above are from runs I did myself.)