rusackas commented on code in PR #40653:
URL: https://github.com/apache/superset/pull/40653#discussion_r3343850591
##########
superset/mcp_service/auth.py:
##########
@@ -66,6 +66,76 @@
METHOD_PERMISSION_ATTR = "_method_permission_name"
+def _sanitize_iss_for_log(value: Any) -> str:
+ """Escape control characters in an issuer claim before logging.
+
+ The ``iss`` claim is attacker-controlled and may contain newlines that
+ could forge or split log lines; collapse them to keep one log entry.
+ """
+ return (
+ str(value)
+ .replace("\\", "\\\\")
+ .replace("\n", "\\n")
+ .replace("\r", "\\r")
+ .replace("\t", "\\t")
+ )
+
+
+# Maps a tool's method permission (read/write/delete) to the OAuth-style token
+# scope it requires. Used by ``check_tool_permission`` for scope-aware
+# authorization: enforcement is the INTERSECTION of token scopes and DB RBAC.
+# Only applied when the token actually carries scopes — see that function.
+_METHOD_TO_REQUIRED_SCOPE = {
+ "read": "superset:read",
+ "write": "superset:write",
+ "delete": "superset:write",
+}
+
+
+def _get_token_scopes() -> set[str] | None:
+ """Return the set of scopes on the current JWT access token, or None.
+
+ Returns None when there is no JWT context or the token carries no scopes,
+ so callers can fall back to RBAC-only behavior (back-compat for API-key and
+ scope-less JWT deployments). Returns a (possibly populated) set only when
+ the token explicitly advertises scopes.
+ """
+ try:
+ from fastmcp.server.dependencies import get_access_token
+ except ImportError:
+ return None
+
+ try:
+ access_token = get_access_token()
+ except Exception: # noqa: BLE001 - no JWT context for this request
+ return None
+
+ if access_token is None:
+ return None
+
+ scopes = getattr(access_token, "scopes", None)
+ if not scopes:
+ # Token present but no scopes advertised: do NOT enforce scope checks.
+ return None
+ return {str(s) for s in scopes}
+
+
+def _token_scope_allows(method_permission_name: str) -> bool:
+ """Return whether the current token's scopes permit the given method.
+
+ Back-compat: returns True (allow) when the token carries no scopes or there
+ is no JWT context, so deployments not using scopes keep RBAC-only behavior.
+ Only when the token advertises scopes is the mapped required scope
enforced.
+ """
+ token_scopes = _get_token_scopes()
+ if token_scopes is None:
+ return True
+ required_scope = _METHOD_TO_REQUIRED_SCOPE.get(method_permission_name)
+ if required_scope is None:
+ return True
Review Comment:
Fixed. ``_METHOD_TO_REQUIRED_SCOPE`` now maps ``execute_sql_query`` to the
write scope (SQL execution is a write-class privileged operation), and
``_token_scope_allows`` fails closed for any method permission not present in
the map: when a scoped token is presented for an unmapped permission it is
denied (with a warning) rather than silently bypassing scope enforcement, so
adding a tool with a new custom permission can no longer fail open. Added unit
tests covering both the unmapped-method denial and the execute_sql_query
write-scope requirement.
##########
tests/unit_tests/mcp_service/test_auth_rbac.py:
##########
@@ -343,3 +343,105 @@ def
test_visibility_data_model_metadata_allowed(app_context) -> None:
result = is_tool_visible_to_current_user(tool)
assert result is True
+
+
+# -- Scope-aware authorization (intersection of token scopes and RBAC) --
+
+
+def _patch_token_scopes(scopes):
+ """Patch the JWT access-token lookup used by ``_get_token_scopes``.
+
+ ``scopes=None`` simulates no JWT context / no token; a list simulates a
+ token that advertises those scopes; an empty list simulates a token with
+ no scopes (treated as scope-less -> RBAC-only).
+ """
+ if scopes is None:
+ token = None
+ else:
+ token = MagicMock()
+ token.scopes = scopes
+ return patch(
+ "fastmcp.server.dependencies.get_access_token",
+ return_value=token,
+ )
Review Comment:
Done — moved ``_patch_token_scopes`` up into the contiguous helper block
alongside ``_make_tool_func`` and ``_make_mock_tool``, before any test
sections, so a future test inserted ahead of its old location can no longer hit
a NameError.
--
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]
