Skip to content
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

Use headers instead of URL params for ClickHouse credentials #7255

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martijnthe
Copy link

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other (Security/Improvement)

Description

The ClickHouse driver was passing the credentials via URL query parameters.
This not a good practice because URLs tend to get included in logs.
This PR changes this to use the x-clickhouse-user and x-clickhouse-key headers instead.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Add a ClickHouse DB as datasource. Observe connecting and querying still works as before.

Related Tickets & Documents

N/A

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

N/A

@martijnthe
Copy link
Author

@susodapop could you perhaps take a look?

Copy link
Contributor

@susodapop susodapop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't contributed to redash in a few years but this PR looks safe to me. It's consistent with the clickhouse docs and should thus be safe (i.e. not niche).

Be sure to update the release notes to let Clickhouse users know about this change but I don't imagine this will be a breaking change for Clickhouse users.

@martijnthe martijnthe force-pushed the clickhouse-use-auth-headers branch from c4c4ea8 to 8fb45a7 Compare January 28, 2025 14:14
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.02%. Comparing base (d03a2c4) to head (8fb45a7).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7255   +/-   ##
=======================================
  Coverage   64.01%   64.02%           
=======================================
  Files         163      163           
  Lines       13410    13411    +1     
  Branches     1905     1905           
=======================================
+ Hits         8585     8586    +1     
  Misses       4490     4490           
  Partials      335      335           
Files with missing lines Coverage Δ
redash/query_runner/clickhouse.py 61.06% <100.00%> (+0.29%) ⬆️

@martijnthe martijnthe force-pushed the clickhouse-use-auth-headers branch from 8fb45a7 to 30362ca Compare February 1, 2025 16:30
@martijnthe
Copy link
Author

Be sure to update the release notes to let Clickhouse users know about this change but I don't imagine this will be a breaking change for Clickhouse users.

@susodapop where should I update the release notes? The CHANGELOG file seems very outdated.

I did find release notes here, but I'm not sure if I'm supposed to edit that?

Can you merge this PR? I cannot, even though you approved it already.

@martijnthe
Copy link
Author

where should I update the release notes? The CHANGELOG file seems very outdated.

@arikfr could you perhaps provide guidance? Thank you very much!

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