gabotorresruiz commented on code in PR #42297:
URL: https://github.com/apache/superset/pull/42297#discussion_r3785630668
##########
superset/mcp_service/auth.py:
##########
@@ -400,7 +435,9 @@ def check_tool_permission( # noqa: C901
# advertises scopes. Tokens/deployments that don't use scopes (API
keys,
# scope-less JWTs, dev-mode) fall through to RBAC-only behavior — see
# ``_token_scope_allows``.
- if has_permission and not _token_scope_allows(method_permission_name):
+ if has_permission and not _token_scope_allows(
Review Comment:
This block worries me a bit, though I don't think it's a regression you
introduced. `check_tool_permission` returns True for tools with no
`class_permission_name` (line 408) before `_token_scope_allows` is ever
consulted, so scoped tokens are not constrained at all on those tools. I
verified it on this branch against a live MCP service: an API key scoped only
to `superset:dashboard:read` can call `find_users` and get back user ids,
usernames and names, and can call `get_instance_info`, both directly and
through the `call_tool` proxy. The same held for flat scopes before this PR,
but per-resource keys are what make "this key can only touch dashboards" a
promise users will rely on, and the PR description's "denies chart tools and
all write tools" doesn't cover this surface.
Was leaving permission-less tools outside scope enforcement intentional? If
not, calling `_token_scope_allows(method_permission_name)` in the
permission-less branch would require at least the flat `superset:read` for
these tools when a token advertises scopes, while keeping unscoped tokens on
today's behavior. A test in `test_auth_rbac.py` with a class-permission-less
tool and a token scoped `["superset:dashboard:read"]` would lock it in; the
existing scope tests all use tools that declare a class permission, which is
why this path went untested. Not a blocker either way, but worth deciding
explicitly.
##########
superset/security/manager.py:
##########
@@ -4926,6 +4927,116 @@ def parse_jwt_guest_token(self, raw_token: str) ->
dict[str, Any]:
raw_token, secret, algorithms=[algo], audience=audience
)
+ def get_api_key_scopes(self, api_key_string: str) -> Optional[str]:
+ """Return the ``scopes`` value for a validated API key.
+
+ FAB's ``validate_api_key`` resolves the matching ``ApiKey`` row
+ internally (by lookup hash) but only returns the associated
+ ``User`` — the row's ``scopes`` column is otherwise unreachable by
+ callers. This repeats the same cheap, indexed lookup so MCP's
+ ``CompositeTokenVerifier`` can propagate per-key scopes instead of
+ silently falling back to verifier-global scopes. Call only after
+ ``validate_api_key`` has already succeeded for this token — this
+ method does not itself verify the key hash or active status.
+ """
+ lookup = self._compute_lookup_hash(api_key_string) # type:
ignore[attr-defined]
+ api_key = (
+ self.session.query(self.api_key_model) # type:
ignore[attr-defined]
+ .filter(self.api_key_model.lookup_hash == lookup)
+ .one_or_none()
+ )
+ return api_key.scopes if api_key else None
+
+ def _validate_requested_api_key_scopes(
+ self, user: Any, scopes: Optional[str]
+ ) -> None:
+ """Raise if ``scopes`` would grant a user more than their own RBAC.
+
+ Enforces the "intersection, never broader" rule confirmed for this
+ feature: a user must never be able to mint a token scoped beyond
+ what their own role already permits, even if they hand-author the
+ scopes string themselves at issuance time.
+
+ Per-resource scopes (``superset:<resource>:<action>``) are checked
+ against the user's actual ``can_<method>`` RBAC grant for that
+ resource. Flat scopes (``superset:read``/``superset:write``, the
+ pre-per-resource form) can only be self-issued by Admins — a flat
+ scope grants a method across every resource, and there's no single
+ RBAC check that soundly proves a non-Admin has that for "every
+ resource," so it's rejected for anyone else rather than guessed at.
+ Unrecognized scope strings are rejected outright (fail closed).
+
+ NOTE: this only prevents the request from being honored; it does
+ not (yet) produce a clean 400 response, since FAB's ``ApiKeyApi``
+ has no validation hook this can plug into without replacing the API
+ registration entirely. Raising here surfaces as a 500 via FAB's
+ ``@safe`` decorator until that's addressed — tracked as a known
+ follow-up, not silently accepted.
+ """
+ if not scopes:
+ return
+ # pylint: disable-next=import-outside-toplevel
+ from superset.security.api_key_scopes import (
+ RESOURCE_SCOPE_ACTIONS,
+ RESOURCE_SCOPE_CLASS,
+ )
+
+ is_admin = any(role.name == "Admin" for role in getattr(user, "roles",
[]))
+ for raw_scope in scopes.split(","):
+ scope = raw_scope.strip()
+ if not scope:
+ continue
+ parts = scope.split(":")
+ if len(parts) == 3 and parts[0] == "superset":
+ _, resource_slug, action = parts
+ class_permission_name = RESOURCE_SCOPE_CLASS.get(resource_slug)
+ if class_permission_name is None:
+ raise ValueError(
+ f"Requested scope '{scope}' names an unrecognized "
+ f"resource '{resource_slug}'"
+ )
+ if action not in RESOURCE_SCOPE_ACTIONS:
+ raise ValueError(
+ f"Requested scope '{scope}' names an unrecognized "
+ f"action '{action}'"
+ )
+ if self._has_view_access(user, f"can_{action}",
class_permission_name):
Review Comment:
Not a blocker, everything here fails closed, but three of the sixteen
advertised resource scopes can never be minted by anyone, Admin included. I
verified it live on this branch: `create_api_key` for Admin with
`superset:user:read`, `superset:role:read` or `superset:sqllab:write` is
rejected with "exceeds the issuing user's own permissions", because the
permission-views `can_read` on User/Role and `can_write` on SQLLab don't exist
in an initialized instance (those views register `can_get` and
`can_execute_sql_query` instead, which is exactly why the MCP tools declare
`method_permission_name="get"` for User/Role). Meanwhile the runtime side
happily grants those scopes when presented on a JWT, since `get` and
`execute_sql_query` map to `read`/`write` in `METHOD_PERMISSION_SCOPE_ACTION`.
One way to line the two sides up: accept a requested scope when the user
holds any method permission that maps to that action for the resource (invert
`METHOD_PERMISSION_SCOPE_ACTION`, so `read` checks `can_read` or `can_get`, and
`write` also checks `can_execute_sql_query`), which mirrors what the scope
actually grants at runtime. Also, the "exceeds the issuing user's own
permissions" message is misleading for this case since no user can hold a
permission that doesn't exist. If you'd rather keep it strict, a docstring note
on which scopes are effectively JWT-only would do.
##########
superset/security/manager.py:
##########
@@ -4926,6 +4927,116 @@ def parse_jwt_guest_token(self, raw_token: str) ->
dict[str, Any]:
raw_token, secret, algorithms=[algo], audience=audience
)
+ def get_api_key_scopes(self, api_key_string: str) -> Optional[str]:
+ """Return the ``scopes`` value for a validated API key.
+
+ FAB's ``validate_api_key`` resolves the matching ``ApiKey`` row
+ internally (by lookup hash) but only returns the associated
+ ``User`` — the row's ``scopes`` column is otherwise unreachable by
+ callers. This repeats the same cheap, indexed lookup so MCP's
+ ``CompositeTokenVerifier`` can propagate per-key scopes instead of
+ silently falling back to verifier-global scopes. Call only after
+ ``validate_api_key`` has already succeeded for this token — this
+ method does not itself verify the key hash or active status.
+ """
+ lookup = self._compute_lookup_hash(api_key_string) # type:
ignore[attr-defined]
+ api_key = (
+ self.session.query(self.api_key_model) # type:
ignore[attr-defined]
+ .filter(self.api_key_model.lookup_hash == lookup)
+ .one_or_none()
+ )
+ return api_key.scopes if api_key else None
+
+ def _validate_requested_api_key_scopes(
+ self, user: Any, scopes: Optional[str]
+ ) -> None:
+ """Raise if ``scopes`` would grant a user more than their own RBAC.
+
+ Enforces the "intersection, never broader" rule confirmed for this
+ feature: a user must never be able to mint a token scoped beyond
+ what their own role already permits, even if they hand-author the
+ scopes string themselves at issuance time.
+
+ Per-resource scopes (``superset:<resource>:<action>``) are checked
+ against the user's actual ``can_<method>`` RBAC grant for that
+ resource. Flat scopes (``superset:read``/``superset:write``, the
+ pre-per-resource form) can only be self-issued by Admins — a flat
+ scope grants a method across every resource, and there's no single
+ RBAC check that soundly proves a non-Admin has that for "every
+ resource," so it's rejected for anyone else rather than guessed at.
+ Unrecognized scope strings are rejected outright (fail closed).
+
+ NOTE: this only prevents the request from being honored; it does
+ not (yet) produce a clean 400 response, since FAB's ``ApiKeyApi``
+ has no validation hook this can plug into without replacing the API
+ registration entirely. Raising here surfaces as a 500 via FAB's
+ ``@safe`` decorator until that's addressed — tracked as a known
+ follow-up, not silently accepted.
+ """
+ if not scopes:
+ return
+ # pylint: disable-next=import-outside-toplevel
+ from superset.security.api_key_scopes import (
+ RESOURCE_SCOPE_ACTIONS,
+ RESOURCE_SCOPE_CLASS,
+ )
+
+ is_admin = any(role.name == "Admin" for role in getattr(user, "roles",
[]))
Review Comment:
Just a small NIT: this hardcodes the "Admin" role name, while `is_admin()`
further down this file resolves it from `conf["AUTH_ROLE_ADMIN"]`. On a
deployment with a renamed admin role, real admins can't self-issue flat scopes
(fail closed, so no security concern, and runtime RBAC still bounds any key
regardless), but it's a one-line fix to read the config value and stay
consistent with the rest of the security manager.
--
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]
