codeant-ai-for-open-source[bot] commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3657691688
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -449,10 +451,23 @@ def import_dashboard( # noqa: C901
db.session.flush()
if not existing and user:
- from superset.subjects.utils import get_user_subject
+ from superset.subjects.utils import (
+ get_default_viewers_for_new_asset,
+ get_user_subject,
+ )
subj = get_user_subject(user.id)
if subj and subj not in dashboard.editors:
dashboard.editors.append(subj)
+ # Resolved once by bulk importers and passed in; recomputed here only
+ # for direct callers that omit it (one membership query per asset).
+ viewers = (
+ default_viewers
+ if default_viewers is not None
+ else get_default_viewers_for_new_asset(user.id)
+ )
+ for viewer in viewers:
+ if viewer not in dashboard.viewers:
+ dashboard.viewers.append(viewer)
Review Comment:
**Suggestion:** The default groups are appended unconditionally for every
newly created dashboard, even when the import payload already specifies
explicit viewers. This can grant groups access that the bundle intentionally
omitted and contradicts the stated explicit-viewer override semantics. Only
apply `viewers` defaults when the payload does not define viewers explicitly.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Imported dashboards grant unintended group viewer access.
- ⚠️ Dashboard bundle permissions are not authoritative.
- ❌ Group members can access dashboards omitted from imports.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the creator-group sharing setting and ensure the importing user
belongs to a
group; dashboard v1 imports are reached through `import_()` at
`superset/dashboards/api.py:2219`, which invokes `ImportDashboardsCommand`.
2. Import a v1 dashboard bundle containing an explicit viewer list, causing
`ImportDashboardsCommand` to call `import_dashboard()` at
`superset/commands/dashboard/importers/v1/utils.py:277`.
3. `Dashboard.import_from_dict()` at
`superset/commands/dashboard/importers/v1/utils.py:449` loads the bundle's
viewers, and
the new-dashboard branch at lines `453-467` resolves the creator's default
viewers.
4. The loop at `superset/commands/dashboard/importers/v1/utils.py:469-471`
appends default
groups absent from the explicit list, granting those groups dashboard access
despite the
bundle naming viewers explicitly; the documented override behavior is
therefore violated.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a09a4efe2db747debb4fc4841d48eb65&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a09a4efe2db747debb4fc4841d48eb65&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dashboard/importers/v1/utils.py
**Line:** 469:471
**Comment:**
*Logic Error: The default groups are appended unconditionally for every
newly created dashboard, even when the import payload already specifies
explicit viewers. This can grant groups access that the bundle intentionally
omitted and contradicts the stated explicit-viewer override semantics. Only
apply `viewers` defaults when the payload does not define viewers explicitly.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=55fe10230d23d8482833d7a75abf5cc6ada64b71a94ab90135743d6914450086&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=55fe10230d23d8482833d7a75abf5cc6ada64b71a94ab90135743d6914450086&reaction=dislike'>👎</a>
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -201,12 +203,29 @@ def import_chart(
if chart.id is None:
db.session.flush()
- if user:
- from superset.subjects.utils import get_user_subject
+ # Only newly created charts inherit the creator's editor/viewer defaults;
+ # re-importing over an existing chart (overwrite or soft-delete restore)
+ # must not silently grant the importer's groups access. Mirrors the
+ # dashboard importer's ``not existing`` guard.
+ if not existing and user:
+ from superset.subjects.utils import (
+ get_default_viewers_for_new_asset,
+ get_user_subject,
+ )
subj = get_user_subject(user.id)
if subj and subj not in chart.editors:
chart.editors.append(subj)
+ # Resolved once by bulk importers and passed in; recomputed here only
+ # for direct callers that omit it (one membership query per asset).
+ viewers = (
+ default_viewers
+ if default_viewers is not None
+ else get_default_viewers_for_new_asset(user.id)
+ )
+ for viewer in viewers:
+ if viewer not in chart.viewers:
+ chart.viewers.append(viewer)
Review Comment:
**Suggestion:** The default groups are appended unconditionally for every
newly created chart, even when the import payload already specifies explicit
viewers. This violates the documented override behavior and can grant
additional groups access beyond the permissions requested by the bundle. Only
apply `viewers` defaults when the payload does not define viewers explicitly.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Imported charts grant unintended group viewer access.
- ⚠️ Chart bundle permissions no longer restrict viewers explicitly.
- ❌ Group members can discover charts omitted from the bundle.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the creator-group sharing setting and ensure the importing user
belongs to at
least one group; the chart import path is exposed through `import_()` at
`superset/dashboards/api.py:2219`, which invokes `ImportChartsCommand`.
2. Submit a v1 chart bundle whose permissions explicitly define one or more
viewers, then
let `ImportChartsCommand` call `import_chart()` at
`superset/commands/chart/importers/v1/utils.py:137`.
3. `Slice.import_from_dict()` at
`superset/commands/chart/importers/v1/utils.py:202`
populates `chart.viewers` from the bundle, after which the new-chart branch
at lines
`210-224` resolves the creator's default viewers.
4. The loop at `superset/commands/chart/importers/v1/utils.py:226-228`
appends every
resolved default group that is not already present, so the imported chart
grants access to
creator groups even though explicit viewers were supplied; the expected
explicit-viewer
override is not honored.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=28c2bb495d774ded97f99fe63bf66888&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=28c2bb495d774ded97f99fe63bf66888&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/chart/importers/v1/utils.py
**Line:** 226:228
**Comment:**
*Logic Error: The default groups are appended unconditionally for every
newly created chart, even when the import payload already specifies explicit
viewers. This violates the documented override behavior and can grant
additional groups access beyond the permissions requested by the bundle. Only
apply `viewers` defaults when the payload does not define viewers explicitly.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=c12df108f7b1ad69006a9449917b529e1626c88fd9feb6a99cf939ffe03aaf03&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=c12df108f7b1ad69006a9449917b529e1626c88fd9feb6a99cf939ffe03aaf03&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/generate_dashboard.py:
##########
@@ -347,11 +347,17 @@ def generate_dashboard( # noqa: C901
.first()
)
if current_user:
- from superset.subjects.utils import get_user_subject
+ from superset.subjects.utils import (
+ get_default_viewers_for_new_asset,
+ get_user_subject,
+ )
subj = get_user_subject(current_user.id)
if subj:
dashboard.editors = [subj]
+ dashboard.viewers = get_default_viewers_for_new_asset(
+ current_user.id
+ )
Review Comment:
**Suggestion:** Assigning `dashboard.viewers` unconditionally overwrites any
viewers explicitly provided in the request or assigned earlier during dashboard
construction. This makes the MCP tool ignore caller-specified viewers whenever
creator-group defaults are enabled; apply the defaults only when no explicit
viewers were supplied. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP-created dashboards lose explicitly selected viewers.
- ⚠️ Access is replaced by creator-group defaults when enabled.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke the MCP `generate_dashboard` tool, whose dashboard-construction
path reaches
`superset/mcp_service/dashboard/tool/generate_dashboard.py:337-368`.
2. Provide explicit dashboard viewers through the tool's dashboard payload,
so
`dashboard.viewers` contains those principals before the creator-default
assignment at
`generate_dashboard.py:358`.
3. Complete the creation path with `ASSIGN_CREATOR_GROUPS_AS_VIEWERS`
enabled; after the
creator is resolved at `generate_dashboard.py:344-349`,
`get_default_viewers_for_new_asset(current_user.id)` is called at lines
358-360.
4. Observe that the unconditional assignment replaces the previously selected
`dashboard.viewers` collection rather than preserving the explicit viewers;
the resulting
dashboard contains only the creator's default groups. The assignment should
be conditional
on there being no explicit viewers.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bc70f743758142189a3c71beadbff490&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bc70f743758142189a3c71beadbff490&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/generate_dashboard.py
**Line:** 358:360
**Comment:**
*Logic Error: Assigning `dashboard.viewers` unconditionally overwrites
any viewers explicitly provided in the request or assigned earlier during
dashboard construction. This makes the MCP tool ignore caller-specified viewers
whenever creator-group defaults are enabled; apply the defaults only when no
explicit viewers were supplied.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=37de69d8470b5242689feec1e403f24b02909456e02eb0e4fbb042a978738609&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=37de69d8470b5242689feec1e403f24b02909456e02eb0e4fbb042a978738609&reaction=dislike'>👎</a>
##########
superset/subjects/utils.py:
##########
@@ -176,6 +288,65 @@ def get_or_create_role_subject(role_id: int) -> Subject |
None:
return get_role_subject(role_id)
+def get_group_subject(group_id: int) -> Subject | None:
+ """Get the GROUP-type Subject for a given group ID."""
+ return (
+ db.session.query(Subject)
+ .filter_by(
+ group_id=group_id,
+ type=SubjectType.GROUP,
+ )
+ .first()
+ )
+
+
+def get_or_create_group_subject(group_id: int) -> Subject | None:
+ """Get or create the GROUP-type Subject for a given group ID.
+
+ Mirrors ``get_or_create_role_subject``: a group that exists but has no
+ Subject row yet (e.g. created before the sync hooks were installed and not
+ yet backfilled) is synced on demand rather than skipped. Returns ``None``
+ only when no such group exists.
+ """
+ if subject := get_group_subject(group_id):
+ return subject
+
+ from superset import security_manager
+ from superset.subjects.sync import sync_group_subject
+
+ group = db.session.get(security_manager.group_model, group_id)
+ if not group:
+ return None
+
+ sync_group_subject(group)
+ db.session.flush()
+ return get_group_subject(group_id)
Review Comment:
**Suggestion:** The check-then-create sequence in
`get_or_create_group_subject` is not safe under concurrent requests. Two
workers can both observe that the group has no `Subject`, both attempt
`sync_group_subject`, and one can fail during `flush()` with an integrity error
if the Subject table enforces uniqueness for a group/type pair. Handle the
uniqueness conflict by reloading the existing Subject, or use an atomic
insert/upsert. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Concurrent asset creation can fail during group-subject backfill.
- ⚠️ Creator-group viewer defaults may be unavailable for the failed request.
- ⚠️ The worker transaction may remain unusable until rollback.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure `ASSIGN_CREATOR_GROUPS_AS_VIEWERS` and ensure a creator belongs
to a group
whose `Subject` row is missing; `get_default_viewers_for_new_asset()` at
`superset/subjects/utils.py:136-150` calls `get_user_group_subjects()`,
which backfills
missing subjects.
2. Start two application workers and concurrently create assets for users
belonging to
that group, causing both requests to execute `get_user_group_subjects()` at
`superset/subjects/utils.py:101-127`.
3. Both workers reach `get_or_create_group_subject()` at
`superset/subjects/utils.py:303-323`; each executes `get_group_subject()` at
lines 311-299
equivalent lookup and observes no existing `Subject`.
4. Both workers call `sync_group_subject(group)` at
`superset/subjects/utils.py:314-321`;
one transaction inserts the group subject while the other reaches
`db.session.flush()` at
line 322 with a duplicate `(group_id, type)` subject and receives an
integrity error
instead of reloading the subject.
5. Observe that the affected asset-creation request fails and its SQLAlchemy
transaction
requires rollback, unless the caller or transaction boundary handles the
integrity error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=46a410a725fb49b893d9f00095f51584&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=46a410a725fb49b893d9f00095f51584&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/subjects/utils.py
**Line:** 311:323
**Comment:**
*Race Condition: The check-then-create sequence in
`get_or_create_group_subject` is not safe under concurrent requests. Two
workers can both observe that the group has no `Subject`, both attempt
`sync_group_subject`, and one can fail during `flush()` with an integrity error
if the Subject table enforces uniqueness for a group/type pair. Handle the
uniqueness conflict by reloading the existing Subject, or use an atomic
insert/upsert.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=9372c6c0b330964d2d53b75ae3454f72f20d09da65d5dea542af36794d171898&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=9372c6c0b330964d2d53b75ae3454f72f20d09da65d5dea542af36794d171898&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -664,6 +667,12 @@ def reset_ownership(self) -> None:
user_subject = get_user_subject(g.user.id)
if user_subject:
self.editors = [user_subject]
+ # Only dashboards and charts have ``viewers``; this mixin also
+ # serves datasets, which have editors only. Defaults never replace
+ # viewers already present on the instance, matching the create
+ # commands' "explicit viewers win" rule.
+ if hasattr(self, "viewers") and not self.viewers: # type:
ignore[has-type]
Review Comment:
**Suggestion:** The `not self.viewers` check cannot distinguish an omitted
viewers field from an explicitly supplied empty list. As a result, creating an
asset with `viewers=[]` will still assign the creator's default groups,
violating the explicit-viewers-wins behavior and unexpectedly sharing the
asset. Track whether viewers were explicitly provided before applying defaults.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Explicitly private new dashboards or charts gain unintended group
viewers.
- ⚠️ Creator-group access settings override intentional empty viewer
payloads.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the `ASSIGN_CREATOR_GROUPS_AS_VIEWERS` setting and create a
dashboard or chart
through one of the PR's asset-creation paths, which eventually invokes the
ownership
mixin's `reset_ownership()` method at `superset/models/helpers.py:647`.
2. Submit the asset payload with an explicitly supplied empty viewers list
(`viewers=[]`),
intending to create the asset without any group viewers.
3. During `reset_ownership()`, the current request user is resolved at
`superset/models/helpers.py:664`, and the asset's existing viewers
relationship is checked
at line 674.
4. Because an explicitly empty relationship is falsy, line 674 calls
`get_default_viewers_for_new_asset()` at line 675 and assigns the creator's
groups, so the
explicit empty viewers list does not suppress default sharing.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c90f17c05f9b48b0ab4fddcb126b847c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c90f17c05f9b48b0ab4fddcb126b847c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 674:674
**Comment:**
*Logic Error: The `not self.viewers` check cannot distinguish an
omitted viewers field from an explicitly supplied empty list. As a result,
creating an asset with `viewers=[]` will still assign the creator's default
groups, violating the explicit-viewers-wins behavior and unexpectedly sharing
the asset. Track whether viewers were explicitly provided before applying
defaults.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=f07a8222a108c26f6c862aa6fee395bc93ffb3d4865f653cc132a15c634c7508&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=f07a8222a108c26f6c862aa6fee395bc93ffb3d4865f653cc132a15c634c7508&reaction=dislike'>👎</a>
--
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]
