Visitar URL original
fix(mcp): preserve embedded guest identity during permalink lookup by rebenitez1802 · Pull Request #45346 · apache/superset · GitHub
Skip to content

fix(mcp): preserve embedded guest identity during permalink lookup #45346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rebenitez1802 merged 1 commit into from
Oct 15, 2026
Merged

fix(mcp): preserve embedded guest identity during permalink lookup #45346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions superset/mcp_service/dashboard/permalink.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,13 @@ def extract_dashboard_permalink_key(value: str) -> str:


def refresh_request_user_for_permalink_access() -> None:
"""Reload the request user before permalink access checks."""
"""Reload stored users while preserving token-scoped guest identities."""
if not has_request_context() or not getattr(g, "user", None):

@gabotorresruiz gabotorresruiz Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, and the guard itself looks right to me. Flagging because it is easy to miss: when the MCP service runs on its own (superset mcp run, streamable-http or stdio) a tool call executes under an app context only. _mcp_tool_call_context() pushes app_context() and never a request context, so has_request_context() is False on this line and the helper returns before the anonymous branch or your new guest branch is reached. I confirmed that on a live get_dashboard_info call with a signed guest token.

That does not make the fix wrong, it makes it the right shape for a deployment that pushes a request context around the tool call, which is the g.user fallback get_user_from_request() documents for external middleware. Two small things:

  • worth a line in the docstring that the refresh only applies when a request context is present?
  • is has_request_context() the gate you actually want? Under the standalone server a stored user's roles are never refreshed either, which is what this helper exists for. If that ever becomes has_app_context() the guest guard goes from defensive to load bearing, so landing it first is right either way.

Or am I missing a transport that does push a request context?

return
current_user = g.user
if getattr(current_user, "is_anonymous", False):
if getattr(current_user, "is_anonymous", False) or getattr(
current_user, "is_guest_user", False
):
Comment on lines +82 to +84

@gabotorresruiz gabotorresruiz Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting for whoever reviews after me: worker.py:704 already carries this exact predicate, guest = user if getattr(user, "is_guest_user", False) else None, and line 726 puts the guest back after re-hydrating g.user by id on a worker thread. So both the spelling and the precedent were already upstream and this helper was the one place missing it. Good argument for the fix living right here.

return
username = getattr(current_user, "username", None)
email = getattr(current_user, "email", None)
Expand Down
40 changes: 39 additions & 1 deletion tests/unit_tests/mcp_service/dashboard/test_permalink.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

"""Tests for MCP dashboard permalink helpers."""

from typing import Any, cast
from unittest.mock import Mock, patch, PropertyMock

import pytest
Expand All @@ -34,6 +35,7 @@
lookup_dashboard_reference,
refresh_request_user_for_permalink_access,
)
from superset.security.guest_token import GuestToken, GuestUser

FOUND = "dashboard"
PERMALINK_URL = "https://example.test/dashboard/p/shared-key/"
Expand Down Expand Up @@ -162,6 +164,42 @@ def test_refresh_request_user_for_permalink_access_keeps_user_when_reload_fails(
mock_load.assert_called_once_with(username="admin")


def test_permalink_lookup_keeps_guest_identity(app: Any) -> None:

@gabotorresruiz gabotorresruiz Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small NIT: the other three tests in this file take app: Flask, and Any is imported only for this one annotation, so the import on line 20 can go back to cast alone.

Suggested change
def test_permalink_lookup_keeps_guest_identity(app: Any) -> None:
def test_permalink_lookup_keeps_guest_identity(app: Flask) -> None:

"""Guest identities come from their token and are not reloaded as stored users."""
guest = GuestUser(
cast(
GuestToken,
{
"user": {"username": "embedded-viewer"},
"resources": [{"type": "dashboard", "id": "1"}],
"rls_rules": [{"dataset": "5", "clause": "amount > 0"}],
"datasets": [5],
"iat": 0,
"exp": 9999999999,
},
),
[],
)
stored_user = object()
with (
app.test_request_context("/mcp"),
patch(
"superset.mcp_service.dashboard.permalink.load_user_with_relationships",
return_value=stored_user,
) as reload_user,
patch(
"superset.commands.dashboard.permalink.get.GetDashboardPermalinkCommand.run",
return_value=None,
),
):
g.user = guest
assert get_dashboard_permalink("guest-state") is None
assert g.user is guest
assert g.user.guest_token["datasets"] == [5]
assert g.user.rls == [{"dataset": "5", "clause": "amount > 0"}]
reload_user.assert_not_called()


@pytest.mark.parametrize(
("reference", "expected_match"),
[
Expand Down Expand Up @@ -380,7 +418,7 @@ def test_lookup_dashboard_reference_unresolvable_reference(mock_get_permalink) -
@pytest.mark.parametrize("denied", [False, True])
def test_stacking_resolves_with_calling_user(app: Flask, denied: bool) -> None:
"""The real get command checks access using the refreshed calling user."""
user = Mock(username="viewer", is_anonymous=False)
user = Mock(username="viewer", is_anonymous=False, is_guest_user=False)
refreshed_user = Mock(username="viewer")
value: DashboardPermalinkValue = {
"dashboardId": "42",
Expand Down
Loading