Impersonate issue or misconfiguration with cache

[jpambrun-vida]

I have a trino database with OPA policies granting different row and columns levels access depending on the user. In superset I have the trino backend set impersonate.

I have a rather serious issue where if a user with wide access loads a dashboard, his cached results are presented to the lower access users.

Am I misconfiguring something? I assume that when impersonate is enable, the cache key need to include the user so that it can't be re-use for user with different level of access?

[dosubot[bot]]

@jpambrun-vida ¡Hola! I'm here to assist you with any issues you encounter. Let's work together to solve this problem.

To ensure that the cache in Superset respects user-specific access levels when using Trino with OPA policies and impersonation enabled, you need to configure impersonation correctly and enable specific feature flags.

1. Update Impersonation Configuration:
   Ensure that the update_impersonation_config method in the TrinoEngineSpec class is correctly updating the connection arguments with the effective username:

   @classmethod
   def update_impersonation_config(
       cls,
       connect_args: dict[str, Any],
       uri: str,
       username: str | None,
       access_token: str | None,
   ) -> None:
       url = make_url_safe(uri)
       backend_name = url.get_backend_name()
   
       if backend_name == "trino" and username is not None:
           connect_args["user"] = username

   This ensures that each user's queries are executed with their specific access levels ^[1].

2. Enable Feature Flags:
   Enable the CACHE_IMPERSONATION and CACHE_QUERY_BY_USER feature flags to ensure that cache keys are generated with user-specific information. This is demonstrated in the unit tests for the QueryObject class:

   @patch("superset.common.query_object.feature_flag_manager")
   @patch("superset.common.query_object.logger")
   def test_cache_key_cache_impersonation_on_with_different_user_and_db_impersonation(
       logger_mock, feature_flag_mock
   ):
       datasource = SqlaTable(
           table_name="test_table",
           columns=[],
           metrics=[],
           main_dttm_col=None,
           database=Database(
               database_name="my_database",
               sqlalchemy_uri="sqlite://",
               impersonate_user=True,
           ),
       )
   
       feature_flag_mock.is_feature_enabled.side_effect = (
           cache_impersonation_flag_side_effect
       )
       query_object = QueryObject(row_limit=1, datasource=datasource)
   
       with override_user(User(username="test_user1")):
           cache_key1 = query_object.cache_key()
   
       with override_user(User(username="test_user2")):
           cache_key2 = query_object.cache_key()
   
       assert cache_key1 != cache_key2
   
       logger_mock.debug.assert_has_calls(
           [
               call(
                   "Adding impersonation key to QueryObject cache dict: %s", "test_user1"
               ),
               call(
                   "Adding impersonation key to QueryObject cache dict: %s", "test_user2"
               ),
           ]
       )

   This ensures that cache keys are different for different users ^[2].

3. Set impersonate_user Field:
   Ensure that the impersonate_user field in the Database model is set to True for the Trino database:

   if self.impersonate_user:
       self.db_engine_spec.update_impersonation_config(
           connect_args,
           str(sqlalchemy_url),
           effective_username,
           access_token,
       )

   This ensures that the impersonation configuration is applied when setting up the connection ^[3].

By following these steps, you can ensure that the cache in Superset respects user-specific access levels when using Trino with OPA policies and impersonation enabled.

To continue talking to Dosu, mention @dosu.

[jpambrun-vida]

The bot hallucinated quite a bit, but gave something to search for. I needed to add

FEATURE_FLAGS = {
    # ...
    "CACHE_IMPERSONATION" : True
}

I imagine this feature is still under testing, but the default (of comingling cache for impersonated queries) is quite unexpected.