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

fix: [alert] should run alert query from report account #17499

Merged
merged 3 commits into from
Dec 16, 2021

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Nov 22, 2021

SUMMARY

When user setup an alert with query and condition, we found that the query was run by root user. It will cause permission issue, since we give THUMBNAIL_SELENIUM_USER account extra data access, while root does not.

Note, after alert query, the screenshot was taken from THUMBNAIL_SELENIUM_USER config correctly.

Proposed solution: set correct username for the alert query.

TESTING INSTRUCTIONS

CI

@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #17499 (2b0865a) into master (fceabf6) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17499      +/-   ##
==========================================
+ Coverage   68.07%   68.13%   +0.06%     
==========================================
  Files        1653     1653              
  Lines       66374    66375       +1     
  Branches     7121     7121              
==========================================
+ Hits        45181    45224      +43     
+ Misses      19296    19254      -42     
  Partials     1897     1897              
Flag Coverage Δ
hive 81.78% <100.00%> (+<0.01%) ⬆️
mysql 82.15% <100.00%> (+<0.01%) ⬆️
postgres 82.20% <100.00%> (+<0.01%) ⬆️
presto 82.07% <100.00%> (?)
python 82.69% <100.00%> (+0.14%) ⬆️
sqlite 81.88% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/models/core.py 90.00% <100.00%> (+0.73%) ⬆️
superset/reports/commands/alert.py 96.51% <100.00%> (+0.04%) ⬆️
superset/connectors/sqla/models.py 88.42% <0.00%> (+1.34%) ⬆️
superset/db_engine_specs/presto.py 89.97% <0.00%> (+5.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fceabf6...2b0865a. Read the comment docs.

@graceguo-supercat graceguo-supercat changed the title fix: [alert] should run alert query from report account [WIP]fix: [alert] should run alert query from report account Nov 22, 2021
@graceguo-supercat graceguo-supercat changed the title [WIP]fix: [alert] should run alert query from report account fix: [alert] should run alert query from report account Nov 30, 2021
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Dec 2, 2021
@graceguo-supercat
Copy link
Author

ping @dpgaspar updated PR with new solution.

@graceguo-supercat graceguo-supercat force-pushed the gg-fixAlertQueryUser branch 2 times, most recently from e46376a to a057d1d Compare December 2, 2021 00:18
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

This is great @graceguo-supercat. Left a comment, would be nice to add a unittest for get_df to assert the new named parameter username override

@@ -146,8 +146,13 @@ def _execute_query(self) -> pd.DataFrame:
limited_rendered_sql = self._report_schedule.database.apply_limit_to_sql(
rendered_sql, ALERT_SQL_LIMIT
)
query_username = security_manager.get_user_by_username(
Copy link
Member

Choose a reason for hiding this comment

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

since get_df accepts a str and get_user_by_username returns a User this call is unnecessary.

@graceguo-supercat graceguo-supercat force-pushed the gg-fixAlertQueryUser branch 5 times, most recently from 99f6500 to 96b9eb4 Compare December 14, 2021 01:32
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 14, 2021
@pull-request-size pull-request-size bot added size/S and removed size/M labels Dec 14, 2021
@graceguo-supercat graceguo-supercat force-pushed the gg-fixAlertQueryUser branch 2 times, most recently from 323f1b8 to 1e5523c Compare December 14, 2021 02:31
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 14, 2021
@graceguo-supercat
Copy link
Author

@dpgaspar Thanks for code review. I fixed your comment and added an integration test.

@dpgaspar dpgaspar merged commit a01c4c9 into apache:master Dec 16, 2021
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* fix: [alert] should run alert query from report account

* add solution2: override username for get_df

* add integration test
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* fix: [alert] should run alert query from report account

* add solution2: override username for get_df

* add integration test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants