Fix Trino connector progress/cancel/SSL and CSRF token staleness on login - #4358
Open
scott2112 wants to merge 3 commits into
Open
Fix Trino connector progress/cancel/SSL and CSRF token staleness on login#4358scott2112 wants to merge 3 commits into
scott2112 wants to merge 3 commits into
Conversation
…ogin
- trino.py: check_status() no longer reports RUNNING queries as 'available'.
Previously hardcoded status to 'available' for Trino's RUNNING state with a
"# need to verify" comment left in place, causing the UI progress bar to show
the query as finished seconds after it moved past QUEUED, while it was still
actually executing in Trino.
- trino.py: add a cancel() method. TrinoApi had no cancel(), unlike every other
connector (sql_alchemy.py, hiveserver2.py, jdbc.py), so clicking Cancel on a
running query raised 'TrinoApi' object has no attribute 'cancel'. Delegates
to the existing close_statement(), which already does the right DELETE on
the query's next_uri.
- trino.py: support a self-signed/internal CA cert for HTTPS connections via a
new ssl_cert_path interpreter option. TrinoRequest was never passed a verify
argument, so certificate validation always used the default system/certifi
CA bundle with no way to trust an internal CA, causing "certificate verify
failed: unable to get local issuer certificate". The new option merges the
custom cert into a combined bundle with the certifi defaults (written once
per process) rather than replacing the trust store outright, so other
outbound HTTPS calls Hue makes still validate against public CAs normally.
- global_js_constants.mako: use django.middleware.csrf.get_token(request)
instead of reading request.COOKIES.get('csrftoken') directly to seed
window.CSRF_TOKEN. The old approach embedded whatever CSRF cookie arrived
with the request, which is stale if the same request rotates the CSRF
token (e.g. via login()) later in its own processing - every subsequent
AJAX POST would then send a token that no longer matched the session,
failing CSRF validation and leaving the UI on a blank screen after login.
- settings.py: move CsrfViewMiddleware to run immediately after
SessionMiddleware, before AuthenticationMiddleware/SpnegoMiddleware/
HueRemoteUserMiddleware (matching Django's own recommended default
ordering). Those middlewares can call login()/rotate_token() inline while
handling a request (e.g. RemoteUserDjangoBackend auto-login from a
trusted proxy header). With CsrfViewMiddleware positioned after them,
its process_request() would unconditionally reset the just-rotated CSRF
secret back to the stale incoming cookie value, reintroducing the same
token-mismatch/blank-screen symptom independent of the template fix above.
Verified via pytest against a full build (all 23 tests pass, including the 17 pre-existing ones - make test/hue test uses stale nose-era flags with no corresponding custom Django test command, while the repo's root conftest.py confirms pytest is the actual intended runner). - test_check_status_running: confirms RUNNING is now reported as 'running' instead of 'available', matching the progress-bar fix. - test_cancel: confirms cancel() delegates to close_statement(). - test_init_verify_defaults_to_true_without_ssl_cert_path / test_init_verify_uses_combined_ca_bundle_with_ssl_cert_path: confirm the verify kwarg passed to TrinoRequest in both the default and ssl_cert_path-configured cases. - test_get_combined_ca_bundle_merges_certifi_and_extra_cert / test_get_combined_ca_bundle_is_cached_after_first_call: confirm the CA bundle merge and once-per-process caching behavior.
…h_result check_status() polls Trino's next_uri to learn query state, but that GET advances Trino's pagination cursor and can consume a page of row data along with the status check. That data and the advanced next_uri were being discarded, so a later poll or fetch_result() call could re-request an already-consumed URI and get back an unpredictable subset of the real rows. - trino.py: check_status() now surfaces any polled row data in its response instead of dropping it, and always returns the correctly advanced next_uri. - api.py: _check_status() persists that advanced next_uri and any polled row data into the snippet's handle, so fetch_result() can recover it even from a different worker process. - snippet.js: the legacy KO editor only copied next_uri out of a check_status response in the running/waiting branch, never in the available branch that triggers fetchResult() -- fixed to apply unconditionally, including seeding any row data delivered alongside that poll. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
What changes were proposed in this pull request?
trino.py: check_status() no longer reports RUNNING queries as 'available'. Previously hardcoded status to 'available' for Trino's RUNNING state with a "# need to verify" comment left in place, causing the UI progress bar to show the query as finished seconds after it moved past QUEUED, while it was still actually executing in Trino.
trino.py: add a cancel() method. TrinoApi had no cancel(), unlike every other connector (sql_alchemy.py, hiveserver2.py, jdbc.py), so clicking Cancel on a running query raised 'TrinoApi' object has no attribute 'cancel'. Delegates to the existing close_statement(), which already does the right DELETE on the query's next_uri.
trino.py: support a self-signed/internal CA cert for HTTPS connections via a new ssl_cert_path interpreter option. TrinoRequest was never passed a verify argument, so certificate validation always used the default system/certifi CA bundle with no way to trust an internal CA, causing "certificate verify failed: unable to get local issuer certificate". The new option merges the custom cert into a combined bundle with the certifi defaults (written once per process) rather than replacing the trust store outright, so other outbound HTTPS calls Hue makes still validate against public CAs normally.
trino.py: check_status() now surfaces any polled row data in its response instead of dropping it, and always returns the correctly advanced next_uri.
api.py: _check_status() persists that advanced next_uri and any polled row data into the snippet's handle, so fetch_result() can recover it even from a different worker process.
snippet.js: the legacy KO editor only copied next_uri out of a check_status response in the running/waiting branch, never in the available branch that triggers fetchResult() -- fixed to apply unconditionally, including seeding any row data delivered alongside that poll.
global_js_constants.mako: use django.middleware.csrf.get_token(request) instead of reading request.COOKIES.get('csrftoken') directly to seed window.CSRF_TOKEN. The old approach embedded whatever CSRF cookie arrived with the request, which is stale if the same request rotates the CSRF token (e.g. via login()) later in its own processing - every subsequent AJAX POST would then send a token that no longer matched the session, failing CSRF validation and leaving the UI on a blank screen after login.
settings.py: move CsrfViewMiddleware to run immediately after SessionMiddleware, before AuthenticationMiddleware/SpnegoMiddleware/ HueRemoteUserMiddleware (matching Django's own recommended default ordering). Those middlewares can call login()/rotate_token() inline while handling a request (e.g. RemoteUserDjangoBackend auto-login from a trusted proxy header). With CsrfViewMiddleware positioned after them, its process_request() would unconditionally reset the just-rotated CSRF secret back to the stale incoming cookie value, reintroducing the same token-mismatch/blank-screen symptom independent of the template fix above.
How was this patch tested?
Verified via pytest against a full build (all 23 tests pass, including the 17 pre-existing ones - make test/hue test uses stale nose-era flags with no corresponding custom Django test command, while the repo's root conftest.py confirms pytest is the actual intended runner).
test_check_status_running: confirms RUNNING is now reported as 'running' instead of 'available', matching the progress-bar fix.
test_cancel: confirms cancel() delegates to close_statement().
test_init_verify_defaults_to_true_without_ssl_cert_path / test_init_verify_uses_combined_ca_bundle_with_ssl_cert_path: confirm the verify kwarg passed to TrinoRequest in both the default and ssl_cert_path-configured cases.
test_get_combined_ca_bundle_merges_certifi_and_extra_cert / test_get_combined_ca_bundle_is_cached_after_first_call: confirm the CA bundle merge and once-per-process caching behavior.
Please review Hue Contributing Guide before opening a pull request.