msyavuz commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3656408652
##########
superset/subjects/utils.py:
##########
@@ -79,6 +80,70 @@ def get_user_subject_ids_subquery(user_id: int) ->
CompoundSelect:
return union_all(user_subj, role_subj, group_subj, group_role_subj)
+def get_user_group_subject_ids_subquery(user_id: int) -> Select:
+ """Return a Select of GROUP-type Subject IDs for a user's groups.
+
+ Unlike :func:`get_user_subject_ids_subquery` this covers group membership
+ only — it excludes the user's own subject and any roles.
+ """
+ from flask_appbuilder.security.sqla.models import assoc_user_group
+
+ return (
+ select(Subject.id)
+ .join(assoc_user_group, Subject.group_id ==
assoc_user_group.c.group_id)
+ .where(
+ assoc_user_group.c.user_id == user_id,
+ Subject.type == SubjectType.GROUP,
+ )
+ )
+
+
+def get_user_group_subjects(user_id: int) -> list[Subject]:
+ """Return the GROUP-type Subjects for every group a user belongs to."""
+ return (
+ db.session.query(Subject)
+ .filter(Subject.id.in_(get_user_group_subject_ids_subquery(user_id)))
+ .all()
+ )
+
+
+def _assigns_creator_groups_as_viewers() -> bool:
+ return bool(
+ has_app_context() and
current_app.config.get("ASSIGN_CREATOR_GROUPS_AS_VIEWERS")
+ )
+
+
+def get_default_viewers_for_new_asset(user_id: int | None) -> list[Subject]:
+ """Return the viewers a newly created asset should default to.
+
+ The single place deciding how a creator's group membership propagates to a
+ new asset, so the paths that build assets outside the create commands
+ (save-as, importers, MCP tools) stay consistent with them. Empty unless
+ ``ASSIGN_CREATOR_GROUPS_AS_VIEWERS`` is enabled.
+
+ Only assets with a ``viewers`` relationship apply — datasets have editors
+ only. Callers inside a flush event use
+ :func:`get_default_viewers_for_groups` instead.
+ """
+ if user_id is None or not _assigns_creator_groups_as_viewers():
+ return []
+ return get_user_group_subjects(user_id)
Review Comment:
`subjects_from_groups` silently skips groups with no Subject row — should
this get-or-create like `get_or_create_role_subject` does? A partial viewer set
is worse than none, since any viewer disables the dataset fallback and the
unsynced group is locked out.
##########
.github/workflows/superset-frontend.yml:
##########
@@ -23,6 +23,12 @@ jobs:
frontend-build:
runs-on: ubuntu-26.04
timeout-minutes: 30
+ # The change detector reads the PR's file list, which needs
+ # `pull-requests: read`. Public repos serve that endpoint without it;
+ # private forks return 403.
+ permissions:
Review Comment:
Twelve other workflows call `./.github/actions/change-detector/` with only
`contents: read` — don't they need `pull-requests: read` too?
##########
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):
Review Comment:
This block also runs when `chart` is an existing row being overwritten —
should it be guarded with `if not existing` like the dashboard importer?
`_prepare_existing_chart_for_import` returns `None` on the overwrite/restore
paths, so re-importing an existing chart attaches the importer's groups and
permanently disables its dataset-access fallback.
--
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]
