EnxDev commented on PR #38831:
URL: https://github.com/apache/superset/pull/38831#issuecomment-4900753311
## EnxDev's Review Agent — apache/superset#38831 · HEAD `b6f0641`
**request changes** — the Subject model, migration scaffolding, and picker
plumbing are solidly built, but there are verified access-control regressions
(group-derived roles, embedded-guest RLS) plus several subject-id/user-id
mix-ups that break core flows.
Scope: 471 files / ~105k lines, reviewed by area (migration, security
manager, subjects module, access filters, commands/DAOs/APIs, frontend,
MCP/executors/tags). CI is green; codecov reports **63.9% patch coverage (347
uncovered lines)** — much of it in the new security paths below — and the
translation bot flags new fuzzy strings in `de`/`fi`/`th`.
### 🔴 Functional
- **`superset/subjects/utils.py:44`** · _High_ —
`get_user_subject_ids_subquery` unions the user subject + direct roles
(`assoc_user_role`) + groups, but never roles held **via a group**
(`assoc_group_role`). FAB 5.2's effective roles include those (`user.roles +
group.roles`), and master's RLS matched on `get_user_roles()`. A REGULAR RLS
rule tied to a role a user holds only through a group is silently not applied
(data leak); BASE exemptions break; the same gap hits `is_editor`/`is_viewer`,
every access filter, and migrated `dashboard_roles` viewers. Fix: add a fourth
union branch through `assoc_group_role`. **regression test:** user in group G,
G holds role R, RLS rule on R's subject → assert the clause is applied.
- **`superset/security/manager.py:4193` and `:4276`** · _High_ —
`get_rls_filters` (and the cache-populate variant) now short-circuits `return
[]` for guest users. On master, guests flowed through role matching: guest-role
REGULAR rules applied and **BASE rules applied to guests**. Embedded guests now
bypass all DB-defined RLS. Fix: resolve guest subjects from `g.user.roles`
(`subjects_from_roles`), or run the normal query with an empty subject set so
`notin_([])` keeps BASE rules applied. Principal: Embedded guest. **regression
test:** BASE RLS on a table + embedded guest query → assert the filter is
present.
- **`superset-frontend/src/explore/components/SaveModal.tsx:652`** · _High_
— filter opr `dashboard_is_editable` doesn't exist (the registered `arg_name`
is `is_editable`; FAB silently drops unknown oprs), so the "save to dashboard"
picker lists every readable dashboard and saving to a non-editable one fails
late. Every other call site uses `is_editable`. **regression test:** unmocked
list call asserting the filter is honored.
- **`superset-frontend/src/features/rls/RowLevelSecurityModal.tsx:194`** ·
_High_ — existing subjects are mapped with `id: s.value`, but the API returns
`id`; editing any pre-existing rule submits `subjects: [null, …]` (`:261`) →
400, or wipes the rule's subjects — an RLS rule that applies to nobody fails
open. Fix: `id: s.id`. **regression test:** open an existing rule, save
unchanged → assert subject ids survive in the payload (current test only
asserts a label renders).
- **`superset-frontend/src/features/alerts/AlertReportModal.tsx:1898`** ·
_High_ — new alerts default editors to `value: currentUser.userId` and submit
it (`:969`), but `editors` takes **subject** PKs: creation 400s, or attaches
whichever subject happens to own that PK (possibly a role or another user).
Same user-id-as-subject-id bug at `AlertReportList/index.tsx:404` and
`DatasetList/index.tsx:894` (edit affordances) and `SliceAdder.tsx:157` ("Show
only my charts"). Fix: resolve the user's USER-type subject (bootstrap
`user_subjects` pattern used by `permissionUtils.ts`). **regression test:**
mocks where subject ids ≠ user ids (they currently collide).
-
**`superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:330`** ·
_High_ — the overwrite dropdown filters `col: 'editors', opr: 'rel_m_m', value:
userId`, comparing subject PKs to a user id; users stop seeing datasets they
can overwrite. Datasets have no `is_editable` filter registered — add one or
pass user-subject ids.
- **`superset/mcp_service/system/schemas.py:275`** · _High_ —
`type_val.name` raises `AttributeError` for DB-loaded subjects (`Subject.type`
is a plain `Integer` column; same-session objects mask this in tests), so MCP
tools that serialize editors error wholesale. Fix:
`SubjectType(type_val).name`. **regression test:** serialize a subject
re-loaded from the DB.
- **`superset/cli/sync_subjects.py:72`** · _High_ — `from
flask_appbuilder.models.sqla.interface import Group` is an `ImportError` (Group
lives in `flask_appbuilder.security.sqla.models`), swallowed by the blanket
`except` as "Group table not found" — the documented recovery CLI never syncs
groups, and the prune step skips GROUP orphans. **regression test:** run the
CLI against an unsynced group.
- **`superset/dashboards/schemas.py:575`** · _High_ —
`ImportV1DashboardSchema` dropped `roles`, and `load_configs` uses
marshmallow's default `unknown=RAISE`: any bundle exported from a prior version
with dashboard roles set now fails import. Fix: accept-and-ignore `roles`
(ideally map role names → viewer subjects). **regression test:** import a
master-era export containing `roles`.
-
**`superset/migrations/versions/2026-03-23_10-00_b1c2d3e4f5a6_add_subjects_tables.py:393`**
· _Medium_ — `_seed_subjects` inserts into unique-constrained junction tables
without `DISTINCT`; the legacy sources (`dashboard_user`, `slice_user`,
`sqlatable_user`, `dashboard_roles`, `rls_filter_roles`) have no unique
constraint (your own downgrade recreates them without one) and real deployments
carry duplicate pairs — the upgrade aborts mid-migration on dirty data. Fix:
`.distinct()` on the seed selects.
- **`superset/security/manager.py:4050` /
`superset/dashboards/filters.py:209`** · _High confidence, documented but
fail-open_ — the migration converts `dashboard_roles` → viewers, but with
`ENABLE_VIEWERS` off (the default) neither `raise_for_access` nor
`DashboardAccessFilter._apply_legacy` consults `viewers`: previously
role-restricted dashboards become visible to anyone with dataset access the
moment the upgrade runs, and role-viewers lose access, until the operator
applies the UPDATING.md config. Consider enforcing viewers whenever
`dashboard.viewers` is non-empty regardless of the flag, or at least a startup
warning when `DASHBOARD_RBAC` appears in `FEATURE_FLAGS`.
### 🟡 Should-fix
- `superset/subjects/filters.py` — the new
`related/editors|viewers|subjects` pickers never apply
`EXCLUDE_USERS_FROM_LISTS` (master's `BaseFilterRelatedUsers` did):
deliberately hidden accounts become enumerable with label + email. The
docstring at `manager.py:3605` still claims editors are covered.
- `superset/dashboards/filters.py:131` — `_apply_viewers` drops the
`EXTRA_ACCESS_QUERY_FILTERS["dashboards"]` hook that `_apply_legacy` keeps
(`:261`) → enabling `ENABLE_VIEWERS` silently disables operator-configured
access grants.
- `superset/security/manager.py:3883`, `:1598` — the promiscuous paths check
only `VIEWER_PROMISCUOUS_MODE`, but `config.py:2794` says "Only effective when
ENABLE_VIEWERS is on". As written, a chart-viewer subject gets datasource query
access via `form_data.slice_id` even with viewers off. Gate on the flag to
match the documented contract.
- `superset/security/manager.py:4066` — the chart branch falls back to
datasource access even when `chart.viewers` is set, while
`ChartFilter._apply_viewers` excludes those charts (`~has_viewers`) and the PR
description says assigning viewers opts a resource into explicit access. Lists
and access checks disagree — gate the fallback on empty viewers (as the
dashboard branch does).
- Legacy-path filters match USER-type subjects only (`EditableFilter`
viewers-off branch `subjects/filters.py:117`,
`DashboardAccessFilter._apply_legacy`, `ReportScheduleFilter._apply_legacy`,
both `*DeletedStateFilter`s) while `is_editor` honors role/group subjects
unconditionally: group editors can edit/restore assets that never appear in
their lists. Fails closed, but split-brain. Also: magic literal `1` instead of
`SubjectType.USER` throughout.
- `superset/tasks/utils.py:55` — `_get_indirect_editor_user` resolves
role/group editors via `order_by(User.id).first()` with no `User.active`
filter: an alert can execute as a **deactivated** lowest-id member, and adding
a broad role/group as editor silently widens who reports run as. Add
`.filter(User.active)`; consider restricting the EDITOR fallback to USER
subjects. Related: `_send_error` recipients and the dashboard deactivation
email only consider USER-type editors — role/group-only reports notify no one.
- `Subject.uuid` is never populated: no model default, not seeded by the
migration, not set in `sync.py` — the PR description calls it a "stable UUID
identifier" but it's NULL everywhere (its only consumer is the admin show
view). Add a default + backfill, or drop the column.
- `superset/row_level_security/schemas.py` — `subjects` is optional where
`roles` was required; a REGULAR rule with zero subjects is a silent no-op.
Require non-empty subjects for REGULAR-type rules (schema-level).
- `superset/commands/utils.py:70` —
`populate_subject_list(default_to_user=True)` silently returns `[]` when the
creating user has no Subject row → resource created with zero editors (creator
lockout). Get-or-create the USER subject here; FAB's legacy `/users/add`-style
form routes and `FAB_SECURITY_SIGNALS_ENABLED = False` both bypass the sync
signals, so the row can legitimately be missing.
- Migration `:853` — the `owner:%` → `editor:%` tag rename isn't scoped by
`type = 'owner'`: a custom tag literally named `owner:foo` gets renamed, and a
name collision with an existing `editor:foo` aborts the migration.
- `superset/mcp_service/dataset/schemas.py:493` — MCP `editors` accepts any
subject id (existence check only) while MCP `find_users` returns *user* ids; an
agent following the documented workflow binds the wrong subject (possibly a
role). Restrict MCP editors to USER subjects or accept user ids.
- Save As / copy tightened unconditionally: `commands/dashboard/copy.py:50`
+ `permissionUtils.ts:77` now require editorship (master required ownership
only under `DASHBOARD_RBAC`) — non-editors lose duplication of viewable
dashboards on default configs. If intended, call it out in UPDATING.md.
Related: `hydrateExplore.ts:182` dropped the `extra_owners` check and nothing
at HEAD reads `extra_editors`, so `EXTRA_EDITORS_RESOLVER` deployments lose
Explore's Overwrite option.
- Patch coverage 63.9% on a security-model rewrite — the guest-RLS,
group-role, and promiscuous paths above are exactly the uncovered ones; the
`de`/`fi`/`th` fuzzy translations also need resolving before merge.
### 🔵 Nits
- `superset/subjects/views.py:33` — `name` isn't a `Subject` attribute; the
list/show column renders empty. Drop it or use `label`.
- `superset-frontend/src/views/CRUD/utils.tsx:208` — `is_editable` returns
unfiltered for admins, so an admin's Home "Mine" tab now lists every asset in
the instance instead of their own.
- `superset-frontend/src/hooks/apiResources/dashboards.ts:44` — still
requests the removed `roles` column.
- `AccessSection.tsx:79`/`:107` — `onChangeEditors!`/`onChangeViewers!`
non-null assertions on optional props would crash if omitted; make the props
required.
- `DashboardList`/`ChartList` viewer filters pass `dropdownStyle`, but
ListView only reads `popupStyle` — the width override never applies.
### 🙌 Praise
- Migration hygiene is excellent: idempotent shared helpers throughout, the
MySQL enum two-step for `tag.type`, and a downgrade that genuinely repopulates
the legacy tables.
- `mergeSubjectPickerValues` structurally prevents the historic "removing
one owner blanks the others' labels" bug, and the `SubjectPile` tests are
genuinely behavioral (ordering, input non-mutation, per-type avatar shapes).
<!-- enxdev-review-agent:b6f0641 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD b6f0641._
--
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]
