codeant-ai-for-open-source[bot] commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3656324885
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -449,10 +449,16 @@ 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)
+ for viewer in get_default_viewers_for_new_asset(user.id):
+ if viewer not in dashboard.viewers:
+ dashboard.viewers.append(viewer)
Review Comment:
**Suggestion:** The importer unconditionally appends default group viewers
even when the dashboard payload already specifies viewers. This makes an
explicit viewer list non-exclusive and grants access to every creator group
despite the payload's requested permissions. Check whether viewers were
supplied before applying defaults. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Explicit dashboard viewer restrictions are not preserved.
- ⚠️ Creator groups receive unintended dashboard access.
- ⚠️ Imported dashboard visibility differs from requested permissions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the v1 dashboard import flow that reaches `import_dashboard()` in
`superset/commands/dashboard/importers/v1/utils.py:439`, using a new
dashboard payload
with an explicit viewer list and a user.
2. `Dashboard.import_from_dict(config, recursive=False)` at line 447 applies
the payload,
including its viewer relationships, to `dashboard`.
3. Because this is a new dashboard, the `if not existing and user` condition
at line 451
is true.
4. With creator-group assignment enabled, lines 460-462 append every
creator-group viewer
not already present, so an explicit dashboard viewer list is silently
expanded with the
creator's groups.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3521c3f1da75416b9553f5b1bec22f93&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=3521c3f1da75416b9553f5b1bec22f93&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:** 460:462
**Comment:**
*Logic Error: The importer unconditionally appends default group
viewers even when the dashboard payload already specifies viewers. This makes
an explicit viewer list non-exclusive and grants access to every creator group
despite the payload's requested permissions. Check whether viewers were
supplied 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=0a6f9f4b2055cdef8d63d916e2050b7dc573c52c9053a0561574676f62ac5fa8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=0a6f9f4b2055cdef8d63d916e2050b7dc573c52c9053a0561574676f62ac5fa8&reaction=dislike'>👎</a>
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -202,11 +202,17 @@ def import_chart(
db.session.flush()
if 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 chart.editors:
chart.editors.append(subj)
+ for viewer in get_default_viewers_for_new_asset(user.id):
+ if viewer not in chart.viewers:
+ chart.viewers.append(viewer)
Review Comment:
**Suggestion:** The default groups are appended for every imported chart,
including charts that `Slice.import_from_dict` updates rather than creates.
This causes re-importing an existing chart to change its viewer permissions and
can grant creator-group access to assets that were not newly created. Apply
these defaults only when the import created a new chart. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Existing chart permissions change during v1 re-imports.
- ⚠️ Creator groups may gain unintended chart read access.
- ⚠️ Permission changes can affect users relying on imported charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke the existing v1 chart import flow that reaches `import_chart()` in
`superset/commands/chart/importers/v1/utils.py:192`, supplying a payload
that identifies
an existing chart and a user.
2. `Slice.import_from_dict(config, recursive=False, allow_reparenting=True)`
at
`superset/commands/chart/importers/v1/utils.py:200` loads or updates the
chart; the
following `chart.id is None` check at line 201 confirms this function
handles both new and
existing charts.
3. After the update, execution enters the `if user` block at line 204
without checking
whether the chart was newly created.
4. With `ASSIGN_CREATOR_GROUPS_AS_VIEWERS` enabled and the importing user
belonging to
groups, lines 213-215 append those group subjects to the existing chart's
viewers,
unexpectedly changing its permissions during re-import.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5aa2f94f821d4830825ff2d0216e0a46&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=5aa2f94f821d4830825ff2d0216e0a46&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:** 213:215
**Comment:**
*Logic Error: The default groups are appended for every imported chart,
including charts that `Slice.import_from_dict` updates rather than creates.
This causes re-importing an existing chart to change its viewer permissions and
can grant creator-group access to assets that were not newly created. Apply
these defaults only when the import created a new chart.
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=16faa1af96aa072b211a00e7f0665ea54dfe2844bcd7392f557a100ceb8cfb9a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=16faa1af96aa072b211a00e7f0665ea54dfe2844bcd7392f557a100ceb8cfb9a&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -664,6 +667,10 @@ 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.
+ if hasattr(self, "viewers"):
+ self.viewers = get_default_viewers_for_new_asset(g.user.id)
Review Comment:
**Suggestion:** The unconditional assignment replaces any viewers explicitly
supplied in the create payload with the creator's default groups. This causes
the documented explicit-viewer override to be ignored whenever
`reset_ownership()` runs after payload values have been applied; only assign
default viewers when no viewer list was explicitly provided. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Explicit dashboard and chart viewers are discarded.
- ⚠️ Asset access differs from the creator's requested audience.
- ⚠️ Import and copy workflows can assign unintended group viewers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable `ASSIGN_CREATOR_GROUPS_AS_VIEWERS` and create a user with at least
one group,
matching the feature's configured behavior.
2. Create a dashboard or chart through one of the PR's asset-creation paths,
including an
explicit `viewers` payload: the create commands, `/dashboard/new/`, template
copy, either
v1 importer, or the MCP `generate_dashboard` tool.
3. During creation, `reset_ownership()` in `superset/models/helpers.py:647`
runs with an
active Flask user; `superset/models/helpers.py:664-669` assigns the creator
and editor,
then `superset/models/helpers.py:672-673` unconditionally replaces
`self.viewers`.
4. Observe that the explicitly supplied viewer list is discarded and
replaced by
`get_default_viewers_for_new_asset(g.user.id)`, so the documented
explicit-viewer override
is not honored.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0baf629ccb054b2abe941c0a16e2f99e&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=0baf629ccb054b2abe941c0a16e2f99e&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:** 672:673
**Comment:**
*Logic Error: The unconditional assignment replaces any viewers
explicitly supplied in the create payload with the creator's default groups.
This causes the documented explicit-viewer override to be ignored whenever
`reset_ownership()` runs after payload values have been applied; only assign
default viewers when no viewer list was explicitly provided.
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=f7624ef409e0caf315769f448f1ca861f580c6c4714114e3bb1b28c4790b70e0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=f7624ef409e0caf315769f448f1ca861f580c6c4714114e3bb1b28c4790b70e0&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]
