Skip to content

fix(embedded): resolve guest user permissions in user_view_menu_names#39197

Open
gerbermichi wants to merge 1 commit intoapache:masterfrom
gerbermichi:fix/guest-user-view-menu-permissions
Open

fix(embedded): resolve guest user permissions in user_view_menu_names#39197
gerbermichi wants to merge 1 commit intoapache:masterfrom
gerbermichi:fix/guest-user-view-menu-permissions

Conversation

@gerbermichi
Copy link
Copy Markdown
Contributor

Guest users (embedded dashboards via guest tokens) have is_anonymous=False but no database identity (user_id is None). The existing code path for authenticated users queries the assoc_user_role table by user_id, which returns nothing for guests. This caused all datasource_access, database_access, and schema_access permissions on the guest role to be invisible, breaking features like chart-based annotation layers that rely on ChartDAO.find_by_id (which applies a datasource access filter via ChartFilter).

Fix: detect guest users before the authenticated user path and resolve permissions directly from the roles attached to the guest token.

SUMMARY

user_view_menu_names() in SupersetSecurityManager was broken for guest users (embedded dashboards via guest tokens). Guest users have is_anonymous=False but no database identity (user_id is None). The authenticated user code path queries the assoc_user_role table by user_id, which produces WHERE user_id IS NULL — matching nothing.

This caused all role-based permissions (datasource_access, database_access, schema_access) to be invisible to guest users, even when correctly assigned to the guest role. Any code path using user_view_menu_names() was affected, most notably ChartFilter (via get_dataset_access_filters), which broke chart-based annotation layers on embedded dashboards with a misleading
"Chart not found" error.

Fix: Add a guest user check before the authenticated user path that resolves permissions directly from the roles attached to the guest token, using the same pattern as the existing anonymous user path.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: Charts with annotation layers on embedded dashboards fail with:
Chart with ID (referenced by annotation layer '') was not found.
Please verify that the chart exists and is accessible.

After: Annotation layers load correctly because ChartDAO.find_by_id() can now see the guest role's datasource permissions through ChartFilter.

TESTING INSTRUCTIONS

  1. Create a chart (Chart A) backed by a datasource
  2. Create a second chart (Chart B) that uses Chart A as an annotation layer source (sourceType "line" or "table")
  3. Add Chart B to a dashboard and enable embedding
  4. Assign datasource_access for Chart A's datasource to the Public role (or whichever role is configured via GUEST_ROLE_NAME)
  5. Access the embedded dashboard via guest token
  6. Verify Chart B loads with its annotation layer rendered correctly

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Guest users (embedded dashboards via guest tokens) have
is_anonymous=False but no database identity (user_id is None).
The existing code path for authenticated users queries the
assoc_user_role table by user_id, which returns nothing for guests.
This caused all datasource_access, database_access, and schema_access
permissions on the guest role to be invisible, breaking features like
chart-based annotation layers that rely on ChartDAO.find_by_id
(which applies a datasource access filter via ChartFilter).

Fix: detect guest users before the authenticated user path and resolve
permissions directly from the roles attached to the guest token.
@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Apr 8, 2026

Code Review Agent Run #78326e

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 91b1c5f..91b1c5f
    • superset/security/manager.py
    • tests/unit_tests/security/manager_test.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot Bot added authentication:access-control Rlated to access control embedded labels Apr 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.47%. Comparing base (6ba9096) to head (91b1c5f).

Files with missing lines Patch % Lines
superset/security/manager.py 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #39197   +/-   ##
=======================================
  Coverage   64.47%   64.47%           
=======================================
  Files        2541     2541           
  Lines      131669   131675    +6     
  Branches    30523    30525    +2     
=======================================
+ Hits        84893    84897    +4     
- Misses      45310    45311    +1     
- Partials     1466     1467    +1     
Flag Coverage Δ
hive 40.07% <0.00%> (-0.01%) ⬇️
mysql 60.75% <66.66%> (+<0.01%) ⬆️
postgres 60.83% <66.66%> (+<0.01%) ⬆️
presto 40.09% <0.00%> (-0.01%) ⬇️
python 62.41% <66.66%> (+<0.01%) ⬆️
sqlite 60.46% <66.66%> (+<0.01%) ⬆️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinpark justinpark requested a review from dpgaspar April 15, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant