You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/18 14:48:04 UTC

[GitHub] [superset] Samira-El opened a new pull request, #20114: feat(chart/dashboard): Enable caching per user when impersonation is enabled

Samira-El opened a new pull request, #20114:
URL: https://github.com/apache/superset/pull/20114

   ### 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](https://apache-superset.slack.com/archives/C7G8CG0LR/p1572450733117200).
   
   This is an initial solution, happy to adjust it as per maintainers' feedback.
   
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   Run this in a setup where caching is enabled and there is a datasource that allows user impersonation.
   
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r898783170


##########
superset/common/query_object.py:
##########
@@ -396,6 +398,24 @@ def cache_key(self, **extra: Any) -> str:
         if annotation_layers:
             cache_dict["annotation_layers"] = annotation_layers
 
+        # Add an impersonation key to cache if impersonation is enabled on the db
+        if (
+            feature_flag_manager.is_feature_enabled("CACHE_IMPERSONATION")
+            and self.datasource
+            and hasattr(self.datasource, "database")
+            and self.datasource.database.impersonate_user
+        ):
+
+            if key := self.datasource.database.db_engine_spec.get_impersonation_key(
+                get_username()

Review Comment:
   Thanks @betodealmeida for the review, I don't know how I had missed that `g.user` is an instance of `User` model πŸ˜… .
   
   I made the change, please take a look again.



##########
superset/common/query_object.py:
##########
@@ -396,6 +398,24 @@ def cache_key(self, **extra: Any) -> str:
         if annotation_layers:
             cache_dict["annotation_layers"] = annotation_layers
 
+        # Add an impersonation key to cache if impersonation is enabled on the db
+        if (
+            feature_flag_manager.is_feature_enabled("CACHE_IMPERSONATION")
+            and self.datasource
+            and hasattr(self.datasource, "database")
+            and self.datasource.database.impersonate_user
+        ):
+
+            if key := self.datasource.database.db_engine_spec.get_impersonation_key(
+                get_username()

Review Comment:
   Thanks @betodealmeida for the review, I don't know how I had missed that `g.user` is an instance of `User` model πŸ˜….
   
   I made the change, please take a look again.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on PR #20114:
URL: https://github.com/apache/superset/pull/20114#issuecomment-1140812630

   Bump @villebro @nytai


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] betodealmeida commented on pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on PR #20114:
URL: https://github.com/apache/superset/pull/20114#issuecomment-1160992894

   Taking a look, sorry for the delay!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r877789734


##########
superset/connectors/sqla/models.py:
##########
@@ -2026,6 +2027,15 @@ class and any keys added via `ExtraCache`.
         if self.has_extra_cache_key_calls(query_obj):
             sqla_query = self.get_sqla_query(**query_obj)
             extra_cache_keys += sqla_query.extra_cache_keys
+
+        sqlalchemy_url = make_url_safe(self.database.sqlalchemy_uri_decrypted)
+
+        if effective_user := self.database.get_effective_user(sqlalchemy_url):
+            logger.debug("User impersonation is enabled for database '%s', adding "
+                         "current username to '%s' datasource's extra cache keys",
+                         self.database_name, self.datasource_name)
+            extra_cache_keys.append(effective_user)

Review Comment:
   Instead of appending the effective user to `extra_cache_keys`, I'd almost prefer to have a dedicated key in the `cache_key_dict` for this that gets populated if `self.database.impersonate_user` is `True`. Also, to make it possible to customize this behavior per database type, id add functionality to `BaseEngineSpec` to make it possible to override the behavior. So something similar to this:
   
   1. add a `get_impersonation_key(user: Optional[User]): Any` method to `BaseEngineSpec` that defaults to returning the username, but can be overridden where applicable. We may also want to add other objects to the signature, e.g. `Database`, but we could do that later.
   2. In `QueryObject.cache_key()`, we would check if `self.datasource.database.impersonate_user` is truthy, and if so, we call `self.datasource.database.db_engine_spec.get_impersonation_key(user)` and add the value to `cache_dict["impersonation_key"]`
   
   Thoughts?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r881676692


##########
superset/connectors/sqla/models.py:
##########
@@ -2026,6 +2027,15 @@ class and any keys added via `ExtraCache`.
         if self.has_extra_cache_key_calls(query_obj):
             sqla_query = self.get_sqla_query(**query_obj)
             extra_cache_keys += sqla_query.extra_cache_keys
+
+        sqlalchemy_url = make_url_safe(self.database.sqlalchemy_uri_decrypted)
+
+        if effective_user := self.database.get_effective_user(sqlalchemy_url):
+            logger.debug("User impersonation is enabled for database '%s', adding "
+                         "current username to '%s' datasource's extra cache keys",
+                         self.database_name, self.datasource_name)
+            extra_cache_keys.append(effective_user)

Review Comment:
   Hi @villebro, I made changes as per your suggestions, I ran into some issues so the impementation is not exactly as requested:
   
   * I made `get_impersonation_key` expect a username as str rather than a User instance, I didn't want to add a trip to the DB to get the instance by username just to use the username eventually, I'm getting the username from the flask app context using the existing function `get_username`, if there is a better way then please lmk.
   * I noticed that the `datasource` param in QueryObject is always null, it doesn't seem to be set when the Factory class creates an instance of QueryObject, so I changed the Factory class to explicity pass the datasource it has, that way be able to use it to the db and impersonate_user attribute. 
   * As requested by @nytai, there is now a feature flag to enable/disable this caching per user, it's off by default, not sure if the name is ok (naming stuff is hard πŸ˜„ ), happy to change the flag.
   
   
   Can you take a look please? TIA  



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] nytai commented on pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
nytai commented on PR #20114:
URL: https://github.com/apache/superset/pull/20114#issuecomment-1133156492

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] commented on pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20114:
URL: https://github.com/apache/superset/pull/20114#issuecomment-1132522666

   # [Codecov](https://codecov.io/gh/apache/superset/pull/20114?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#20114](https://codecov.io/gh/apache/superset/pull/20114?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2c4d0d4) into [master](https://codecov.io/gh/apache/superset/commit/27cccd105b51d27a6d6d53f75bce9c89f5a3c2a2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (27cccd1) will **decrease** coverage by `12.08%`.
   > The diff coverage is `100.00%`.
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #20114       +/-   ##
   ===========================================
   - Coverage   66.46%   54.38%   -12.09%     
   ===========================================
     Files        1721     1721               
     Lines       64467    64472        +5     
     Branches     6795     6795               
   ===========================================
   - Hits        42847    35062     -7785     
   - Misses      19892    27682     +7790     
     Partials     1728     1728               
   ```
   
   | Flag | Coverage Ξ” | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.55% <100.00%> (+0.01%)` | :arrow_up: |
   | python | `57.61% <100.00%> (-25.01%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `49.38% <20.00%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/20114?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ” | |
   |---|---|---|
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `77.34% <100.00%> (-12.24%)` | :arrow_down: |
   | [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/key\_value/commands/upsert.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3Vwc2VydC5weQ==) | `0.00% <0.00%> (-89.59%)` | :arrow_down: |
   | [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-89.37%)` | :arrow_down: |
   | [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
   | [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
   | [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
   | [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.88% <0.00%> (-68.24%)` | :arrow_down: |
   | [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `30.18% <0.00%> (-67.93%)` | :arrow_down: |
   | ... and [270 more](https://codecov.io/gh/apache/superset/pull/20114/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20114?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Ξ” = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20114?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [27cccd1...2c4d0d4](https://codecov.io/gh/apache/superset/pull/20114?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r881676692


##########
superset/connectors/sqla/models.py:
##########
@@ -2026,6 +2027,15 @@ class and any keys added via `ExtraCache`.
         if self.has_extra_cache_key_calls(query_obj):
             sqla_query = self.get_sqla_query(**query_obj)
             extra_cache_keys += sqla_query.extra_cache_keys
+
+        sqlalchemy_url = make_url_safe(self.database.sqlalchemy_uri_decrypted)
+
+        if effective_user := self.database.get_effective_user(sqlalchemy_url):
+            logger.debug("User impersonation is enabled for database '%s', adding "
+                         "current username to '%s' datasource's extra cache keys",
+                         self.database_name, self.datasource_name)
+            extra_cache_keys.append(effective_user)

Review Comment:
   Hi @villebro, I made changes as per your suggestions, I ran into some issues so the impementation is not exactly as requested:
   
   * I made `get_impersonation_key` expect a username as str rather than a User instance, I didn't want to add a trip to the DB to get the instance by username just to use the username eventually, I'm getting the username from the flask app context using the existing function `get_username`, if there is a better way then please lmk.
   * I noticed that the `datasource` param in QueryObject is always null, it doesn't seem to be set when the Factory class creates an instance of QueryObject, so I changed the Factory class to explicity pass the datasource it has, that way be able to use it to the db and impersonate_user attribute. 
   * As requested by nytai, there is now a feature flag to enable/disable this caching per user, it's off by default, not sure if the name is ok (naming stuff is hard πŸ˜„ ), happy to change the flag.
   
   
   Can you take a look please? TIA  



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] betodealmeida merged pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
betodealmeida merged PR #20114:
URL: https://github.com/apache/superset/pull/20114


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r877818086


##########
superset/connectors/sqla/models.py:
##########
@@ -2026,6 +2027,15 @@ class and any keys added via `ExtraCache`.
         if self.has_extra_cache_key_calls(query_obj):
             sqla_query = self.get_sqla_query(**query_obj)
             extra_cache_keys += sqla_query.extra_cache_keys
+
+        sqlalchemy_url = make_url_safe(self.database.sqlalchemy_uri_decrypted)
+
+        if effective_user := self.database.get_effective_user(sqlalchemy_url):
+            logger.debug("User impersonation is enabled for database '%s', adding "
+                         "current username to '%s' datasource's extra cache keys",
+                         self.database_name, self.datasource_name)
+            extra_cache_keys.append(effective_user)

Review Comment:
   Hi Ville, I really appreciate your review of this PR!
   
   I have no issue with your suggestion, let me make changes, test them out and get back to you. 
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] betodealmeida commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r892671019


##########
superset/common/query_object.py:
##########
@@ -396,6 +398,24 @@ def cache_key(self, **extra: Any) -> str:
         if annotation_layers:
             cache_dict["annotation_layers"] = annotation_layers
 
+        # Add an impersonation key to cache if impersonation is enabled on the db
+        if (
+            feature_flag_manager.is_feature_enabled("CACHE_IMPERSONATION")
+            and self.datasource
+            and hasattr(self.datasource, "database")
+            and self.datasource.database.impersonate_user
+        ):
+
+            if key := self.datasource.database.db_engine_spec.get_impersonation_key(
+                get_username()

Review Comment:
   Passing the user object here instead of the username makes the method more flexible, since other DBs might want to use the ID or the email. You can simply do:
   
   ```suggestion
                   getattr(g, "user", None)
   ```
   
   And add `from flask import g` to the top of the file.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on PR #20114:
URL: https://github.com/apache/superset/pull/20114#issuecomment-1160817590

   Anything else required to merge this @villebro @nytai @betodealmeida ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on PR #20114:
URL: https://github.com/apache/superset/pull/20114#issuecomment-1134268626

   > 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! :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] Samira-El commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

Posted by GitBox <gi...@apache.org>.
Samira-El commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r881676692


##########
superset/connectors/sqla/models.py:
##########
@@ -2026,6 +2027,15 @@ class and any keys added via `ExtraCache`.
         if self.has_extra_cache_key_calls(query_obj):
             sqla_query = self.get_sqla_query(**query_obj)
             extra_cache_keys += sqla_query.extra_cache_keys
+
+        sqlalchemy_url = make_url_safe(self.database.sqlalchemy_uri_decrypted)
+
+        if effective_user := self.database.get_effective_user(sqlalchemy_url):
+            logger.debug("User impersonation is enabled for database '%s', adding "
+                         "current username to '%s' datasource's extra cache keys",
+                         self.database_name, self.datasource_name)
+            extra_cache_keys.append(effective_user)

Review Comment:
   Hi @villebro, I made changes as per your suggestions, I ran into some issues so the impementation is not exactly as requested:
   
   * I made `get_impersonation_key` expect a username as str rather than a User instance, I didn't want to add a trip to the DB to get the instance by username just to use the username eventually, I'm getting the username from the flask app context using the existing function `get_username`, if there is a better way then please lmk.
   * I noticed that the `datasource` param in QueryObject is always null, it doesn't seem to be set when create an instance of this class, so I change the Factory class to explicity pass it, that way be able to use it to the db and impersonate_user flag. 
   * As requested by nytai, there is now a feature flag to enable/disable this caching per user, it's off by default, not sure if the name is ok (naming stuff is hard πŸ˜„ ), happy to change the flag.
   
   
   Can you take a look please? TIA  



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org