aminghadersohi commented on code in PR #42297:
URL: https://github.com/apache/superset/pull/42297#discussion_r3785666572
##########
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:
Addressed in f9f1aba40. Permission-less tools now still enforce the token
method scope: unscoped tokens retain the existing behavior, a flat
`superset:read` token can call a read-class permission-less tool, and a
resource-only token such as `superset:dashboard:read` cannot. Added the
suggested regression coverage.
##########
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:
Addressed in f9f1aba40. Issuance now uses the inverse of the canonical
runtime method-to-action mapping, so `user:read`/`role:read` accept `can_get`
and `sqllab:write` accepts `can_execute_sql_query` while remaining bounded by
the issuing user’s actual permission. Added parameterized regression coverage
for all three cases.
##########
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:
Addressed in f9f1aba40. Flat-scope issuance now resolves the Admin role
through `AUTH_ROLE_ADMIN`, consistent with `is_admin()`, with a
custom-role-name regression test.
--
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]
