feat(chart): Enable caching per user when user impersonation is enabled #20114
Conversation
Samira-El
changed the title
Samira-El
changed the title
Codecov Report
@@ Coverage Diff @@
## master #20114 +/- ##
==========================================
- Coverage 66.70% 66.55% -0.16%
==========================================
Files 1739 1724 -15
Lines 65136 64804 -332
Branches 6897 6822 -75
==========================================
- Hits 43452 43132 -320
- Misses 19931 19938 +7
+ Partials 1753 1734 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for taking on this challenge @Samira-El ! I left some thoughts, let me know what you think.
|
Can we also add a config to disable this behavior? We have a use case where we would like user impersonation enabled for query tracing purposes and for access control in SQL Lab, but we would still like to keep a shared cache for charts/dashboards. |
Hi ntai, sure no problem! :) |
There was a problem hiding this comment.
This looks great, sorry for the delay! I've had a ticket to fix this for a few months now, and I just started a PR yesterday. Glad to see this implementation!
I left a suggestion on how to pass the user object without having to query the DB, I think it makes the feature more useful in the future. Other than that, this looks great!
|
Anything else required to merge this @villebro @nytai @betodealmeida ? |
|
Taking a look, sorry for the delay! |
…ed (#20114) * add username to extra cache keys when impersonation is enabled. * don't put effective_user in extra_cache_key * get_impersonation_key method in engine_spec class to construct an impersonation key * pass datasource when creating query objects * adding an impersonation key when construction cache key * add feature flag to control caching per user * revert changes * make precommit and pylint happy * pass a User instance * remove unnecessary import (cherry picked from commit 68af598)
mistercrunch
added
🍒 2.0.0
🍒 2.0.1
🏷️ bot
SUMMARY
In order to mitigate sensitive data leaking to less-privileged users when they load a cached dashboard/chart, this PR enables caching per user when a database has impersonation enabled.
In our setup, we delegate permissions to database (Snowflake via OAuth) and we don't want to create and maintain another layer of ACLs in Superset.
Discussion on this on slack.
This is an initial solution, happy to adjust it as per maintainers' feedback.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Run this in a setup where caching is enabled and there is a datasource that allows user impersonation.
ADDITIONAL INFORMATION