Copilot commented on code in PR #42297:
URL: https://github.com/apache/superset/pull/42297#discussion_r3781490000
##########
superset/mcp_service/auth.py:
##########
@@ -223,12 +250,16 @@ def _log_scope_denial(
cyclomatic complexity in check.
"""
required_scope = _METHOD_TO_REQUIRED_SCOPE.get(method_permission_name)
+ resource_scope = _required_resource_scope(
+ class_permission_name, method_permission_name
+ )
+ scope_desc = resource_scope or required_scope
Review Comment:
`_log_scope_denial` can log a required scope of `None` when a scoped token
hits an unmapped `method_permission_name` (since
`_METHOD_TO_REQUIRED_SCOPE.get(...)` returns None). This makes the log message
less actionable; using an explicit fallback string keeps logs clear while still
failing closed.
##########
superset/security/manager.py:
##########
@@ -4926,6 +4927,111 @@ 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):
+ continue
+ raise ValueError(
+ f"Requested scope '{scope}' exceeds the issuing user's "
+ "own permissions"
+ )
+ if len(parts) == 2 and parts[0] == "superset" and is_admin:
+ continue
+ raise ValueError(
+ f"Requested scope '{scope}' is not a recognized "
+ "superset:<resource>:<action> scope, or requires Admin to "
Review Comment:
`_validate_requested_api_key_scopes` currently treats any 2-part scope of
the form `superset:<anything>` as valid for Admin users. That allows an Admin
to mint keys with unrecognized flat scopes (e.g. `superset:garbage`),
contradicting the "fail closed" intent and making enforcement behavior
undefined for those tokens. Restrict flat scopes to the supported action
vocabulary (read/write).
--
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]
