EnxDev commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3673822329
##########
superset/subjects/utils.py:
##########
@@ -79,6 +80,117 @@ 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.
+
+ Any group whose Subject row is missing is backfilled on demand (see
+ :func:`get_or_create_group_subject`) rather than skipped: a single viewer
+ disables an asset's datasource-access fallback, so a partial viewer set
+ would silently lock out the members of an unsynced group.
+ """
+ from flask_appbuilder.security.sqla.models import assoc_user_group
+
+ subjects = (
+ db.session.query(Subject)
+ .filter(Subject.id.in_(get_user_group_subject_ids_subquery(user_id)))
+ .all()
+ )
+ member_group_ids = {
+ row[0]
+ for row in db.session.execute(
+ select(assoc_user_group.c.group_id).where(
+ assoc_user_group.c.user_id == user_id
+ )
+ )
+ }
+ for group_id in member_group_ids - {subject.group_id for subject in
subjects}:
+ if subject := get_or_create_group_subject(group_id):
+ subjects.append(subject)
+ return subjects
+
+
+def _assigns_creator_groups_as_viewers() -> bool:
+ from superset import is_feature_enabled
+
+ return has_app_context() and
is_feature_enabled("ASSIGN_CREATOR_GROUPS_AS_VIEWERS")
Review Comment:
Verified: raise_for_access enforces if chart.viewers: / if
dashboard.viewers: with no ENABLE_VIEWERS gate
([manager.py:4258](vscode-webview://1n3qeu6lokkdh8n4kcfq7jjiihc09vemauq6qorqce14f8ekkt21/superset/security/manager.py#L4258)).
So enabling ASSIGN_CREATOR_GROUPS_AS_VIEWERS while ENABLE_VIEWERS is off would
assign viewers that are still enforced, silently stripping the datasource
fallback from new assets. This is exactly the dependency I flagged earlier.
Fixed: _assigns_creator_groups_as_viewers now also requires ENABLE_VIEWERS;
documented in config, security.mdx, and feature-flags.json; added a gate test
for the "ASSIGN on, ENABLE_VIEWERS off → no-op" case.
--
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]
