sadpandajoe commented on code in PR #33924:
URL: https://github.com/apache/superset/pull/33924#discussion_r3606670573
##########
superset/security/api.py:
##########
@@ -69,6 +69,9 @@ class UserSchema(PermissiveSchema):
username = fields.String()
first_name = fields.String()
last_name = fields.String()
+ attributes = fields.Dict(
+ keys=fields.String(), values=fields.Raw(allow_none=True),
allow_none=True
+ )
Review Comment:
The generated OpenAPI spec (`docs/static/resources/openapi.json`) was not
regenerated for this new field, so `GuestTokenCreate`'s user schema still omits
`attributes` and generated clients cannot discover or type it. Can we
regenerate the spec with this schema change?
##########
superset/jinja_context.py:
##########
@@ -305,6 +311,67 @@ def url_param(
self.cache_key_wrapper(result)
return result
+ def get_guest_user_attribute(
+ self,
+ attribute_name: str,
+ default: JsonValue = None,
+ add_to_cache_keys: bool = True,
+ escape_result: bool = True,
+ ) -> JsonValue:
+ """
+ Get a specific user attribute from guest user.
+
+ This function retrieves attributes from the guest user token and
supports
+ all JSON-native types (string, number, boolean, array, object, null).
+
+ Args:
+ attribute_name: Name of the attribute to retrieve
+ default: Default value if attribute not found (can be any
JSON-native type)
+ add_to_cache_keys: Whether the value should be included in the
cache key
+ escape_result: Escape string values (and strings inside lists)
using
+ the database dialect's quoting so they are safe to interpolate
+ into SQL, mirroring ``url_param``. Enabled by default;
non-string
+ JSON types are returned unchanged. Set to False for the raw
value.
+
+ Returns:
+ The attribute value from the guest user token, or the default
value.
+ Can be any JSON-native type: string, number, boolean, array,
object, or
+ null.
+
+ Examples:
+ {{ get_guest_user_attribute('department') }} # Returns:
"Engineering"
+ {{ get_guest_user_attribute('is_admin') }} # Returns: True
+ {{ get_guest_user_attribute('permissions') }} # Returns: ["read",
"write"]
+ {{ get_guest_user_attribute('config') }} # Returns: {"theme":
"dark"}
+ {{ get_guest_user_attribute('missing', 'default') }} # Returns:
"default"
+ """
+
+ # The macro only applies to guest users (embedded). is_guest_user()
+ # handles the feature-flag and request-context checks internally.
+ if not security_manager.is_guest_user():
+ return default
+
+ token = g.user.guest_token
+ user_attributes = token.get("user", {}).get("attributes") or {}
+
+ # Only add to the cache key if the attribute exists in the guest token
+ if attribute_name not in user_attributes:
+ return default
+
+ result = user_attributes[attribute_name]
+ if add_to_cache_keys and result is not None:
+ # Use json.dumps for consistent serialization of all JSON-native
types
+ cache_value = json.dumps(result, sort_keys=True)
+ self.cache_key_wrapper(
+ f"guest_user_attribute:{attribute_name}:{cache_value}"
+ )
Review Comment:
When an attribute is null or absent (falling back to `default`), or when
`add_to_cache_keys=False`, the value is interpolated into the SQL but never
added to the cache key, so two guests whose tokens render different SQL can
collide on the same chart and RLS cache key and receive each other's rows.
Unlike `url_param` (which keys unconditionally) and `current_user_*` (no
caller-supplied default), the diverging default breaks the isolation the cache
key provides. Should the resolved value, including the default and null branch,
always contribute to the key, and does guest-token RLS using this macro get
picked up by `has_extra_cache_key_calls`, which currently scans only persisted
RLS?
##########
superset/jinja_context.py:
##########
@@ -305,6 +311,67 @@ def url_param(
self.cache_key_wrapper(result)
return result
+ def get_guest_user_attribute(
+ self,
+ attribute_name: str,
+ default: JsonValue = None,
+ add_to_cache_keys: bool = True,
+ escape_result: bool = True,
+ ) -> JsonValue:
+ """
+ Get a specific user attribute from guest user.
+
+ This function retrieves attributes from the guest user token and
supports
+ all JSON-native types (string, number, boolean, array, object, null).
+
+ Args:
+ attribute_name: Name of the attribute to retrieve
+ default: Default value if attribute not found (can be any
JSON-native type)
+ add_to_cache_keys: Whether the value should be included in the
cache key
+ escape_result: Escape string values (and strings inside lists)
using
+ the database dialect's quoting so they are safe to interpolate
+ into SQL, mirroring ``url_param``. Enabled by default;
non-string
+ JSON types are returned unchanged. Set to False for the raw
value.
+
+ Returns:
+ The attribute value from the guest user token, or the default
value.
+ Can be any JSON-native type: string, number, boolean, array,
object, or
+ null.
+
+ Examples:
+ {{ get_guest_user_attribute('department') }} # Returns:
"Engineering"
+ {{ get_guest_user_attribute('is_admin') }} # Returns: True
+ {{ get_guest_user_attribute('permissions') }} # Returns: ["read",
"write"]
+ {{ get_guest_user_attribute('config') }} # Returns: {"theme":
"dark"}
+ {{ get_guest_user_attribute('missing', 'default') }} # Returns:
"default"
+ """
+
+ # The macro only applies to guest users (embedded). is_guest_user()
+ # handles the feature-flag and request-context checks internally.
+ if not security_manager.is_guest_user():
+ return default
+
+ token = g.user.guest_token
+ user_attributes = token.get("user", {}).get("attributes") or {}
+
+ # Only add to the cache key if the attribute exists in the guest token
+ if attribute_name not in user_attributes:
+ return default
+
+ result = user_attributes[attribute_name]
+ if add_to_cache_keys and result is not None:
+ # Use json.dumps for consistent serialization of all JSON-native
types
+ cache_value = json.dumps(result, sort_keys=True)
+ self.cache_key_wrapper(
+ f"guest_user_attribute:{attribute_name}:{cache_value}"
+ )
+ # Guest attributes are interpolated into the rendered SQL, so escape
+ # string values (and strings within lists) with the dialect's quoting
+ # by default, mirroring url_param. Non-string JSON types pass through.
+ if escape_result:
+ result = self._escape_value(result)
Review Comment:
On MySQL/MariaDB this escaping is bypassable. `_escape_value` doubles the
quote but does not escape the backslash that MySQL treats as an escape
character, so a guest attribute of `x\' OR 1=1 -- ` renders to `WHERE col =
'x\'' OR 1=1 -- '`, which parses as `WHERE col = 'x' OR 1=1`. The docstring
says the value is safe to interpolate into SQL, but that only holds where
backslash is literal (Postgres, SQLite), and it conflicts with the docs note
that says attributes are not escaped. Can we bind these values (or require
allowlisting) instead of interpolating, and reconcile the docstring with that
note? The same gap exists in `url_param`, which shares this helper.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
