This is an automated email from the ASF dual-hosted git repository. EnxDev pushed a commit to branch enxdev/feat/subjects-group-access-gaps in repository https://gitbox.apache.org/repos/asf/superset.git
commit 4382c754aadcd70fbdf2928e914a6b4cc9d89414 Author: Enzo Martellucci <[email protected]> AuthorDate: Mon Jul 27 11:58:34 2026 +0200 feat(subjects): scope principal listings and default new assets to creator groups Ports preset-io/superset-private#995 onto this branch. Scopes SubjectRestApi principal listings (base_filters + include_ids filtering), adds the opt-in ASSIGN_CREATOR_GROUPS_AS_VIEWERS setting wired through every asset-creation path, emits security.<Action> StatsD counters for group/role audit events, and exposes the caller's groups on /api/v1/me/. Feature code matches the source PR exactly. The only local additions are generic type parameters on the new tests' annotations (dict[str, Any], list[Any]) plus two type: ignore comments, required because this repo's mypy config enforces disallow_any_generics on test files. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --- .github/workflows/superset-frontend.yml | 6 + docs/admin_docs/security/security.mdx | 21 ++ scripts/change_detector.py | 45 ++- superset/commands/chart/importers/v1/utils.py | 8 +- superset/commands/dashboard/importers/v1/utils.py | 8 +- superset/commands/dataset/create.py | 4 +- superset/commands/utils.py | 8 +- superset/config.py | 6 + superset/daos/dashboard.py | 24 +- .../dashboard/tool/generate_dashboard.py | 8 +- superset/models/dashboard.py | 10 +- superset/models/helpers.py | 9 +- superset/security/manager.py | 10 +- superset/subjects/api.py | 3 +- superset/subjects/filters.py | 16 + superset/subjects/utils.py | 89 ++++- superset/views/base_api.py | 11 +- superset/views/core.py | 20 +- superset/views/dashboard/views.py | 6 +- superset/views/users/schemas.py | 8 + tests/integration_tests/subjects/api_tests.py | 22 ++ tests/integration_tests/users/api_tests.py | 9 + .../commands/dataset/test_create_subjects.py | 71 ++++ .../dashboard/tool/test_dashboard_generation.py | 32 ++ tests/unit_tests/scripts/change_detector_test.py | 63 ++++ .../security/test_audit_event_metrics.py | 55 +++ tests/unit_tests/subjects/test_api_filters.py | 82 ++++ .../subjects/test_creator_group_call_sites.py | 415 +++++++++++++++++++++ .../subjects/test_creator_group_defaults.py | 270 ++++++++++++++ tests/unit_tests/views/test_related_include_ids.py | 124 ++++++ tests/unit_tests/views/test_user_schemas.py | 52 +++ 31 files changed, 1486 insertions(+), 29 deletions(-) diff --git a/.github/workflows/superset-frontend.yml b/.github/workflows/superset-frontend.yml index e4e61344a7c..fa748cd7d43 100644 --- a/.github/workflows/superset-frontend.yml +++ b/.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: + contents: read + pull-requests: read outputs: should-run: ${{ steps.check.outputs.frontend }} steps: diff --git a/docs/admin_docs/security/security.mdx b/docs/admin_docs/security/security.mdx index e6c11a7ae85..97c24a9ecb2 100644 --- a/docs/admin_docs/security/security.mdx +++ b/docs/admin_docs/security/security.mdx @@ -247,6 +247,27 @@ This feature is particularly useful for: - Granting access to dashboards without exposing the underlying datasets for other uses - Creating dashboard-specific access patterns that don't align with dataset permissions +#### Assigning the creator's groups automatically + +```python +ASSIGN_CREATOR_GROUPS_AS_VIEWERS = True +``` + +With this enabled, a newly created dashboard or chart is shared read-only with every group its +creator belongs to, unless the create payload names viewers explicitly. It applies to every path +that creates an asset: the REST API, `/dashboard/new/`, save-as from Explore, dashboard copy, +the import commands, and the MCP tools. Datasets are unaffected, as they have editors but no +viewers. + +Note that this **narrows** access rather than widening it. Because an asset with no viewers falls +back to dataset-based access, giving new assets a viewer makes that fallback unreachable for them: +a colleague who could previously open a new dashboard by virtue of dataset access can no longer do +so unless they share one of the creator's groups. Existing assets are untouched — only assets +created after the setting is enabled are affected. + +The setting assigns groups as *viewers*, not editors, so group members get read-only access and +cannot modify or delete the asset. + ### SQL Execution Security Considerations Apache Superset includes features designed to provide safeguards when interacting with connected databases, such as the `DISALLOWED_SQL_FUNCTIONS` configuration setting. This aims to prevent the execution of potentially harmful database functions or system variables directly from Superset interfaces like SQL Lab. diff --git a/scripts/change_detector.py b/scripts/change_detector.py index 55548b270b0..f7a4bf3425a 100755 --- a/scripts/change_detector.py +++ b/scripts/change_detector.py @@ -31,9 +31,12 @@ from urllib.request import Request, urlopen MAX_RETRIES: int = 4 RETRY_BACKOFF_SECONDS: int = 2 REQUEST_TIMEOUT_SECONDS: int = 30 -# GitHub returns 429 (and 403 for secondary rate limits) when throttling, which -# is transient and worth retrying alongside 5xx server errors. -RETRYABLE_STATUS_CODES: frozenset[int] = frozenset({403, 429}) +# GitHub returns 429 when throttling, which is transient and worth retrying +# alongside 5xx server errors. It also returns 403 for two very different +# reasons — a secondary rate limit, and a token missing the required scope — +# so 403 is only retried when the response headers show throttling. Retrying a +# scope failure just delays the error and buries its cause. +RETRYABLE_STATUS_CODES: frozenset[int] = frozenset({429}) # Define patterns for each group of files you're interested in PATTERNS = { @@ -68,6 +71,35 @@ PATTERNS = { GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN") +def _is_rate_limited(err: HTTPError) -> bool: + """Whether a 403 is GitHub throttling rather than a missing token scope.""" + headers = getattr(err, "headers", None) + if headers is None: + return False + try: + return ( + headers.get("x-ratelimit-remaining") == "0" + or headers.get("retry-after") is not None + ) + except AttributeError: + return False + + +def _api_error_detail(err: object) -> str: + """The API's own explanation, e.g. ``Resource not accessible by integration``. + + Without this a 403 is indistinguishable from a rate limit in the CI log. + """ + try: + body = err.read().decode("utf-8", "replace") # type: ignore[attr-defined] + except Exception: # noqa: BLE001 # pylint: disable=broad-except + return "" + try: + return json.loads(body).get("message") or body + except (ValueError, AttributeError): + return body + + def fetch_files_github_api(url: str): # type: ignore """Fetches data using GitHub API, retrying on transient failures.""" req = Request(url) # noqa: S310 @@ -87,9 +119,14 @@ def fetch_files_github_api(url: str): # type: ignore # re-raise once the retry budget is exhausted. status = getattr(err, "code", None) is_transient = ( - status is None or status >= 500 or status in RETRYABLE_STATUS_CODES + status is None + or status >= 500 + or status in RETRYABLE_STATUS_CODES + or (status == 403 and _is_rate_limited(err)) # type: ignore[arg-type] ) if not is_transient or attempt == MAX_RETRIES: + if detail := _api_error_detail(err): + print(f"GitHub API error {status}: {detail}") raise wait = RETRY_BACKOFF_SECONDS * 2 ** (attempt - 1) print( diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index a4e0bba2e1b..9c0e53a66b1 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/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) return chart diff --git a/superset/commands/dashboard/importers/v1/utils.py b/superset/commands/dashboard/importers/v1/utils.py index e556674d8bd..05fe90d6ea2 100644 --- a/superset/commands/dashboard/importers/v1/utils.py +++ b/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) return dashboard diff --git a/superset/commands/dataset/create.py b/superset/commands/dataset/create.py index 7289f168806..310fa9b24d8 100644 --- a/superset/commands/dataset/create.py +++ b/superset/commands/dataset/create.py @@ -120,7 +120,9 @@ class CreateDatasetCommand(CreateMixin, BaseCommand): except SupersetSecurityException as ex: exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message)) - populate_subjects(self._properties, exceptions) + # Datasets have editors only — there is no ``sqlatable_viewers`` table, + # so a ``viewers`` key would be dropped by the DAO's ``setattr`` loop. + populate_subjects(self._properties, exceptions, include_viewers=False) if exceptions: raise DatasetInvalidError(exceptions=exceptions) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index 4caae03761f..d5325a0c9af 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -35,6 +35,7 @@ from superset.daos.tag import TagDAO from superset.subjects.exceptions import SubjectsNotFoundValidationError from superset.subjects.models import Subject from superset.subjects.utils import ( + get_default_viewers_for_new_asset, get_or_create_user_subject as get_user_subject, get_subject, get_user_subject_ids, @@ -146,7 +147,10 @@ def populate_subjects( except ValidationError as ex: exceptions.append(ex) - if include_viewers and properties.get("viewers") is not None: + if not include_viewers: + return + + if properties.get("viewers") is not None: try: properties["viewers"] = populate_subject_list( properties["viewers"], @@ -155,6 +159,8 @@ def populate_subjects( ) except ValidationError as ex: exceptions.append(ex) + elif default_viewers := get_default_viewers_for_new_asset(get_user_id()): + properties["viewers"] = default_viewers def compute_subjects( diff --git a/superset/config.py b/superset/config.py index 4eb87cea2e4..a62aa26679b 100644 --- a/superset/config.py +++ b/superset/config.py @@ -2945,6 +2945,12 @@ EXTRA_RELATED_QUERY_FILTERS: ExtraRelatedQueryFilters = {} # Only effective when ENABLE_VIEWERS is on. VIEWER_PROMISCUOUS_MODE = False +# When True, a newly created asset is shared read-only with every group its +# creator belongs to, unless the create payload names viewers explicitly. +# This narrows access: an asset with no viewers falls back to datasource +# permissions, while one with viewers is limited to its editors and viewers. +ASSIGN_CREATOR_GROUPS_AS_VIEWERS = False + # Default Subject types shown in editor/viewer/subject pickers. # Entity-specific SUBJECTS_RELATED_TYPES_* settings can replace this default. # None = show all types (USER, ROLE, GROUP). diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 4afa24edac8..c4bd384bfe0 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -428,11 +428,22 @@ class DashboardDAO(BaseDAO[Dashboard]): raise DashboardForbiddenError() dash = Dashboard() + # The copied dashboard and every chart cloned below share one creator, + # so both lookups are resolved here rather than inside the loop, where + # they would cost two extra queries for each chart in the dashboard. + creator_editors: list[Any] = [] + creator_viewers: list[Any] = [] if g.user: - from superset.subjects.utils import get_user_subject + from superset.subjects.utils import ( + get_default_viewers_for_new_asset, + get_user_subject, + ) user_subject = get_user_subject(g.user.id) - dash.editors = [user_subject] if user_subject else [] + creator_editors = [user_subject] if user_subject else [] + creator_viewers = get_default_viewers_for_new_asset(g.user.id) + dash.editors = creator_editors + dash.viewers = creator_viewers dash.dashboard_title = data["dashboard_title"] dash.css = data.get("css") @@ -442,11 +453,10 @@ class DashboardDAO(BaseDAO[Dashboard]): # Duplicating slices as well, mapping old ids to new ones for slc in original_dash.slices: new_slice = slc.clone() - if g.user: - from superset.subjects.utils import get_user_subject - - user_subject = get_user_subject(g.user.id) - new_slice.editors = [user_subject] if user_subject else [] + # ``Slice.clone()`` carries over no subjects, so both + # collections start empty on the new chart. + new_slice.editors = list(creator_editors) + new_slice.viewers = list(creator_viewers) db.session.add(new_slice) db.session.flush() new_slice.dashboards.append(dash) diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py b/superset/mcp_service/dashboard/tool/generate_dashboard.py index a244edbd4c8..71319199279 100644 --- a/superset/mcp_service/dashboard/tool/generate_dashboard.py +++ b/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 + ) fresh_charts = ( db.session.query(Slice) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 7ef6d16db07..dbddea06aea 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -71,7 +71,10 @@ def copy_dashboard(_mapper: Mapper, _connection: Connection, target: Dashboard) if dashboard_id is None: return - from superset.subjects.utils import get_user_subject + from superset.subjects.utils import ( + get_default_viewers_for_groups, + get_user_subject, + ) session = sqla.inspect(target).session # pylint: disable=disallowed-name new_user = session.query(User).filter_by(id=target.id).first() @@ -91,6 +94,11 @@ def copy_dashboard(_mapper: Mapper, _connection: Connection, target: Dashboard) json_metadata=template.json_metadata, slices=template.slices, editors=editors, + # Resolved from the in-memory collection: this runs in ``after_insert`` + # for the user, before their ``ab_user_group`` rows are written. + viewers=get_default_viewers_for_groups( + list(getattr(new_user, "groups", []) or []) + ), ) session.add(dashboard) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 1376a6bfbb6..ccf92bce45a 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -646,7 +646,10 @@ class ImportExportMixin(UUIDMixin): def reset_ownership(self) -> None: """object will belong to the current user""" - from superset.subjects.utils import get_user_subject + from superset.subjects.utils import ( + get_default_viewers_for_new_asset, + get_user_subject, + ) # Reset the audit pointers. When a Flask request context is # available we explicitly stamp the current user, otherwise we @@ -664,6 +667,10 @@ class ImportExportMixin(UUIDMixin): 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) else: self.created_by = None self.changed_by = None diff --git a/superset/security/manager.py b/superset/security/manager.py index 1040e00beff..418b38784d9 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -193,10 +193,16 @@ def _log_audit_event(action: str, payload: dict[str, Any]) -> None: configured implementation (DBEventLogger, S3EventLogger, etc.) receives these security audit events. """ - from superset.extensions import ( - event_logger, # pylint: disable=import-outside-toplevel + from superset.extensions import ( # pylint: disable=import-outside-toplevel + event_logger, + stats_logger_manager, ) + try: + stats_logger_manager.instance.incr(f"security.{action}") + except Exception: # pylint: disable=broad-except + logger.warning("Failed to emit audit metric: %s", action, exc_info=True) + user_id = get_user_id() try: event_logger.log( diff --git a/superset/subjects/api.py b/superset/subjects/api.py index 2984dddc3df..dfd9da9de0f 100644 --- a/superset/subjects/api.py +++ b/superset/subjects/api.py @@ -19,7 +19,7 @@ import logging from flask_appbuilder.models.sqla.interface import SQLAInterface from superset.constants import RouteMethod -from superset.subjects.filters import SubjectAllTextFilter +from superset.subjects.filters import SubjectAllTextFilter, SubjectListFilter from superset.subjects.models import Subject from superset.subjects.schemas import openapi_spec_methods_override from superset.views.base_api import BaseSupersetModelRestApi @@ -52,6 +52,7 @@ class SubjectRestApi(BaseSupersetModelRestApi): resource_name = "security/subject" allow_browser_login = True + base_filters = [["id", SubjectListFilter, lambda: []]] openapi_spec_tag = "Security Subjects" openapi_spec_methods = openapi_spec_methods_override diff --git a/superset/subjects/filters.py b/superset/subjects/filters.py index c99fd7df972..8305a07e470 100644 --- a/superset/subjects/filters.py +++ b/superset/subjects/filters.py @@ -101,6 +101,22 @@ def _apply_subject_list_filters(query: Query) -> Query: return _apply_extra_related_query_filters(_apply_excluded_users(query)) +class SubjectListFilter(BaseFilter): # pylint: disable=too-few-public-methods + """Scope a Subject query to the principals a deployment chooses to expose. + + The related-item endpoints backing the editor/viewer pickers already apply + ``EXCLUDE_USERS_FROM_LISTS`` and ``EXTRA_RELATED_QUERY_FILTERS``. + ``SubjectRestApi`` serves the same principal directory — including + ``user.email`` on its show endpoint — so it applies them too. + """ + + name = _("Subject list") + arg_name = "subject_list" + + def apply(self, query: Query, value: Any) -> Query: + return _apply_subject_list_filters(query) + + def subject_type_filter(entity_config_key: str | None = None) -> type[BaseFilter]: """Return a BaseFilter subclass scoped to a specific entity config. diff --git a/superset/subjects/utils.py b/superset/subjects/utils.py index f19bafafc50..5ff834ebf84 100644 --- a/superset/subjects/utils.py +++ b/superset/subjects/utils.py @@ -19,9 +19,10 @@ from __future__ import annotations from typing import TYPE_CHECKING if TYPE_CHECKING: - from flask_appbuilder.security.sqla.models import Role, User - from sqlalchemy.sql import CompoundSelect + from flask_appbuilder.security.sqla.models import Group, Role, User + from sqlalchemy.sql import CompoundSelect, Select +from flask import current_app, has_app_context from sqlalchemy import select, union_all from superset import db @@ -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) + + +def get_default_viewers_for_groups(groups: list[Group]) -> list[Subject]: + """Like :func:`get_default_viewers_for_new_asset`, for in-memory groups. + + Callers inside a SQLAlchemy flush event cannot resolve membership by + querying ``ab_user_group``: for a user being inserted, the association rows + are written *after* the user's own INSERT, so the query returns nothing. + Those callers already hold the group objects on the instance and pass them + here instead. + """ + if not groups or not _assigns_creator_groups_as_viewers(): + return [] + return subjects_from_groups(groups) + + def get_user_subject_ids(user_id: int) -> list[int]: """Return all Subject IDs that a user represents. @@ -176,6 +241,26 @@ def get_or_create_role_subject(role_id: int) -> Subject | None: return get_role_subject(role_id) +def subjects_from_groups(groups: list[Group | int]) -> list[Subject]: + """Convert a list of Group objects or group IDs to GROUP-type Subjects. + + Mirrors :func:`subjects_from_roles`, but resolves the whole list in one + query rather than one per group. Silently skips groups without a matching + Subject row. + """ + group_ids = [group if isinstance(group, int) else group.id for group in groups] + if not group_ids: + return [] + return ( + db.session.query(Subject) + .filter( + Subject.group_id.in_(group_ids), + Subject.type == SubjectType.GROUP, + ) + .all() + ) + + def subjects_from_roles(roles: list[Role | int]) -> list[Subject]: """Convert a list of Role objects or role IDs to role-type Subjects. diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 852602c700f..d556654c58b 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -501,8 +501,15 @@ class BaseSupersetModelRestApi(BaseSupersetApiMixin, ModelRestApi): values = [row["value"] for row in result] ids = [id_ for id_ in ids if id_ not in values] pk_col = datamodel.get_pk() - # Fetch requested values from ids - extra_rows = db.session.query(datamodel.obj).filter(pk_col.in_(ids)).all() + # Fetch requested values from ids, applying the same scoping as the + # unforced query so ``include_ids`` cannot resolve rows the + # related-field filters deliberately hide. + query = db.session.query(datamodel.obj).filter(pk_col.in_(ids)) + if base_filters := self.base_related_field_filters.get(column_name): + query = datamodel.apply_filters( + query, datamodel.get_filters().add_filter_list(base_filters) + ) + extra_rows = query.all() result += self._get_result_from_rows(datamodel, extra_rows, column_name) @event_logger.log_this_with_context( diff --git a/superset/views/core.py b/superset/views/core.py index 37d13f0a1c2..980eec9ce33 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -626,14 +626,22 @@ class Superset(BaseSupersetView): if action == "saveas": if "slice_id" in form_data: form_data.pop("slice_id") # don't save old slice_id - from superset.subjects.utils import get_user_subject + from superset.subjects.utils import ( + get_default_viewers_for_new_asset, + get_user_subject, + ) editors = [] if g.user: subj = get_user_subject(g.user.id) if subj: editors.append(subj) - slc = Slice(editors=editors) + slc = Slice( + editors=editors, + viewers=get_default_viewers_for_new_asset( + g.user.id if g.user else None + ), + ) utils.remove_extra_adhoc_filters(form_data) @@ -685,7 +693,10 @@ class Superset(BaseSupersetView): status=403, ) - from superset.subjects.utils import get_user_subject + from superset.subjects.utils import ( + get_default_viewers_for_new_asset, + get_user_subject, + ) editors = [] if g.user: @@ -695,6 +706,9 @@ class Superset(BaseSupersetView): dash = Dashboard( dashboard_title=request.args.get("new_dashboard_name"), editors=editors, + viewers=get_default_viewers_for_new_asset( + g.user.id if g.user else None + ), ) if dash and slc not in dash.slices: diff --git a/superset/views/dashboard/views.py b/superset/views/dashboard/views.py index b5e788c5dde..6bbe30da1ac 100644 --- a/superset/views/dashboard/views.py +++ b/superset/views/dashboard/views.py @@ -78,7 +78,10 @@ class Dashboard(BaseSupersetView): @expose("/new/") def new(self) -> FlaskResponse: """Creates a new, blank dashboard and redirects to it in edit mode""" - from superset.subjects.utils import get_user_subject + from superset.subjects.utils import ( + get_default_viewers_for_new_asset, + get_user_subject, + ) editors = [] if g.user: @@ -88,6 +91,7 @@ class Dashboard(BaseSupersetView): new_dashboard = DashboardModel( dashboard_title="[ untitled dashboard ]", editors=editors, + viewers=get_default_viewers_for_new_asset(g.user.id if g.user else None), ) db.session.add(new_dashboard) db.session.commit() # pylint: disable=consider-using-transaction diff --git a/superset/views/users/schemas.py b/superset/views/users/schemas.py index f74d1cbca50..6493ba24177 100644 --- a/superset/views/users/schemas.py +++ b/superset/views/users/schemas.py @@ -27,6 +27,13 @@ last_name_description = "The current user's last name" password_description = "The current user's password for authentication" # noqa: S105 +class UserGroupSchema(Schema): + """A group the current user belongs to.""" + + id = Integer() + name = String() + + class UserResponseSchema(Schema): id = Integer() username = String() @@ -36,6 +43,7 @@ class UserResponseSchema(Schema): is_active = Boolean() is_anonymous = Boolean() login_count = Integer() + groups = fields.List(fields.Nested(UserGroupSchema)) class CurrentUserPutSchema(Schema): diff --git a/tests/integration_tests/subjects/api_tests.py b/tests/integration_tests/subjects/api_tests.py index b2e2a698ba2..7bc021707ae 100644 --- a/tests/integration_tests/subjects/api_tests.py +++ b/tests/integration_tests/subjects/api_tests.py @@ -24,6 +24,7 @@ from superset import db from superset.subjects.models import Subject from superset.subjects.types import SubjectType from superset.utils import json +from tests.conftest import with_config from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME @@ -140,3 +141,24 @@ class TestSubjectApi(SupersetTestCase): assert self.client.post("api/v1/security/subject/", json={}).status_code == 405 assert self.client.put("api/v1/security/subject/1", json={}).status_code == 405 assert self.client.delete("api/v1/security/subject/1").status_code == 405 + + @pytest.mark.usefixtures("create_subjects") + @with_config({"EXCLUDE_USERS_FROM_LISTS": ["admin"]}) + def test_get_list_subject_excludes_configured_users(self) -> None: + """Subject API: excluded users are absent from the list""" + self.login(ADMIN_USERNAME) + + rv = self.get_assert_metric("api/v1/security/subject/", "get_list") + + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + excluded = ( + db.session.query(Subject) + .filter(Subject.type == SubjectType.USER) + .join(Subject.user) + .filter_by(username="admin") + .one_or_none() + ) + assert excluded is not None, "expected an admin USER subject to exist" + assert excluded.id not in {row["id"] for row in data["result"]} + assert data["count"] == db.session.query(Subject).count() - 1 diff --git a/tests/integration_tests/users/api_tests.py b/tests/integration_tests/users/api_tests.py index 85985f9f35b..184a76a6660 100644 --- a/tests/integration_tests/users/api_tests.py +++ b/tests/integration_tests/users/api_tests.py @@ -42,6 +42,15 @@ class TestCurrentUserApi(SupersetTestCase): assert True is response["result"]["is_active"] assert False is response["result"]["is_anonymous"] + def test_get_me_includes_groups(self): + self.login(ADMIN_USERNAME) + + rv = self.client.get(meUri) + + assert 200 == rv.status_code + response = json.loads(rv.data.decode("utf-8")) + assert isinstance(response["result"]["groups"], list) + def test_get_me_with_roles(self): self.login(ADMIN_USERNAME) diff --git a/tests/unit_tests/commands/dataset/test_create_subjects.py b/tests/unit_tests/commands/dataset/test_create_subjects.py new file mode 100644 index 00000000000..30c602fd5ca --- /dev/null +++ b/tests/unit_tests/commands/dataset/test_create_subjects.py @@ -0,0 +1,71 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Datasets must never be given viewers. + +``SqlaTable`` has ``sqlatable_editors`` but no viewers table, so a ``viewers`` +key in the create properties would be handed to ``setattr`` by the DAO and +silently dropped instead of granting anything. +""" + +from unittest.mock import Mock, patch + +from superset.commands.dataset.create import CreateDatasetCommand +from superset.models.core import Database +from superset.subjects.models import Subject +from superset.subjects.types import SubjectType + + +def _group_subject(id_: int) -> Subject: + subject = Subject() + subject.id = id_ + subject.type = SubjectType.GROUP + return subject + + +def test_dataset_create_never_populates_viewers(app_context) -> None: + database = Mock(spec=Database) + database.id = 1 + database.get_default_catalog.return_value = None + + command = CreateDatasetCommand( + {"database": 1, "table_name": "some_table", "sql": "SELECT 1"} + ) + + with ( + patch( + "superset.commands.dataset.create.DatasetDAO.get_database_by_id", + return_value=database, + ), + patch( + "superset.commands.dataset.create.DatasetDAO.validate_uniqueness", + return_value=True, + ), + patch("superset.commands.dataset.create.security_manager.raise_for_access"), + patch("superset.commands.utils.populate_subject_list", return_value=[]), + patch("superset.commands.utils.get_user_id", return_value=5), + patch( + "superset.subjects.utils.get_user_group_subjects", + return_value=[_group_subject(11)], + ), + patch.dict( + "superset.commands.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ), + ): + command.validate() + + assert "viewers" not in command._properties # noqa: SLF001 diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py index b401665657a..9271d7d070f 100644 --- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py @@ -2140,3 +2140,35 @@ class TestDashboardSerializationEagerLoading: assert "editors" not in dash assert dash["tags"] == [] assert dash["charts"] == [] + + +class TestGenerateDashboardCreatorGroups: + """The tool creates dashboards outside CreateDashboardCommand, so it has to + apply the creator's default viewers itself.""" + + @patch("superset.models.dashboard.Dashboard") + @patch("superset.daos.dashboard.DashboardDAO.find_by_id") + @patch("superset.db.session") + @pytest.mark.asyncio + async def test_generated_dashboard_gets_the_creators_default_viewers( + self, mock_db_session, mock_find_by_id, mock_dashboard_cls, mcp_server + ): + charts = [_mock_chart(id=1, slice_name="Sales Chart")] + mock_dashboard = _mock_dashboard(id=10, title="Analytics Dashboard") + mock_dashboard.viewers = [] + _setup_generate_dashboard_mocks( + mock_db_session, mock_find_by_id, mock_dashboard_cls, charts, mock_dashboard + ) + viewer = Mock() + + with patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ): + async with Client(mcp_server) as client: + await client.call_tool( + "generate_dashboard", + {"request": {"chart_ids": [1], "dashboard_title": "Analytics"}}, + ) + + assert mock_dashboard.viewers == [viewer] diff --git a/tests/unit_tests/scripts/change_detector_test.py b/tests/unit_tests/scripts/change_detector_test.py index 69cf4dccce2..619549153ec 100644 --- a/tests/unit_tests/scripts/change_detector_test.py +++ b/tests/unit_tests/scripts/change_detector_test.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import io from unittest import mock from urllib.error import HTTPError, URLError @@ -94,3 +95,65 @@ def test_fetch_gives_up_after_max_retries() -> None: change_detector.fetch_files_github_api("http://api";) assert urlopen_mock.call_count == change_detector.MAX_RETRIES + + +def _http_error( + status: int, headers: dict[str, str] | None = None, body: bytes = b"" +) -> HTTPError: + """Builds an HTTPError carrying headers and a readable body, as urllib does.""" + return HTTPError( + "http://api";, + status, + "Forbidden", + headers or {}, # type: ignore[arg-type] + io.BytesIO(body), + ) + + +def test_fetch_does_not_retry_a_permission_denied_403() -> None: + """A missing token scope is deterministic; retrying only delays the failure.""" + error = _http_error( + 403, + headers={"x-ratelimit-remaining": "4998"}, + body=b'{"message": "Resource not accessible by integration"}', + ) + with ( + mock.patch.object( + change_detector, "urlopen", side_effect=error + ) as urlopen_mock, + mock.patch.object(change_detector.time, "sleep") as sleep_mock, + ): + with pytest.raises(HTTPError): + change_detector.fetch_files_github_api("http://api";) + + assert urlopen_mock.call_count == 1 + sleep_mock.assert_not_called() + + +def test_fetch_retries_a_rate_limited_403() -> None: + """GitHub signals rate limiting with 403 and an exhausted remaining count.""" + side_effects: list[object] = [ + _http_error(403, headers={"x-ratelimit-remaining": "0"}), + _make_response(b'[{"filename": "superset/foo.py"}]'), + ] + with ( + mock.patch.object(change_detector, "urlopen", side_effect=side_effects), + mock.patch.object(change_detector.time, "sleep"), + ): + result = change_detector.fetch_files_github_api("http://api";) + + assert result == [{"filename": "superset/foo.py"}] + + +def test_fetch_reports_the_api_error_message(capsys) -> None: + """The response body names the cause; without it a 403 is unactionable.""" + error = _http_error( + 403, + headers={"x-ratelimit-remaining": "4998"}, + body=b'{"message": "Resource not accessible by integration"}', + ) + with mock.patch.object(change_detector, "urlopen", side_effect=error): + with pytest.raises(HTTPError): + change_detector.fetch_files_github_api("http://api";) + + assert "Resource not accessible by integration" in capsys.readouterr().out diff --git a/tests/unit_tests/security/test_audit_event_metrics.py b/tests/unit_tests/security/test_audit_event_metrics.py new file mode 100644 index 00000000000..858d57f99f8 --- /dev/null +++ b/tests/unit_tests/security/test_audit_event_metrics.py @@ -0,0 +1,55 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests that role/group audit events are also observable as StatsD metrics. + +The group and role write endpoints record audit events through +``_log_audit_event``. Operators need the same activity as counters, so that +group management is visible on dashboards without parsing the event log. +""" + +from unittest.mock import MagicMock + +from pytest_mock import MockerFixture + +from superset.security.manager import _log_audit_event + + +def test_audit_event_increments_a_statsd_counter( + mocker: MockerFixture, + app_context, +) -> None: + stats = MagicMock() + mocker.patch("superset.extensions.stats_logger_manager", stats) + mocker.patch("superset.extensions.event_logger") + + _log_audit_event("GroupCreated", {"group_id": 1, "group_name": "tenant-a"}) + + stats.instance.incr.assert_called_once_with("security.GroupCreated") + + +def test_audit_event_is_still_logged_when_statsd_fails( + mocker: MockerFixture, + app_context, +) -> None: + stats = MagicMock() + stats.instance.incr.side_effect = RuntimeError("statsd down") + mocker.patch("superset.extensions.stats_logger_manager", stats) + event_logger = mocker.patch("superset.extensions.event_logger") + + _log_audit_event("GroupDeleted", {"group_id": 1, "group_name": "tenant-a"}) + + event_logger.log.assert_called_once() diff --git a/tests/unit_tests/subjects/test_api_filters.py b/tests/unit_tests/subjects/test_api_filters.py new file mode 100644 index 00000000000..a10c8a42569 --- /dev/null +++ b/tests/unit_tests/subjects/test_api_filters.py @@ -0,0 +1,82 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests that ``SubjectRestApi`` scopes its own list/show queries. + +``SubjectRestApi`` exposes the principal directory (including ``user.email``), +so the ``EXCLUDE_USERS_FROM_LISTS`` and ``EXTRA_RELATED_QUERY_FILTERS`` +deployment settings must apply to it, exactly as they do to the related-item +endpoints that feed the editor/viewer pickers. +""" + +from unittest.mock import MagicMock + +from flask_appbuilder.models.sqla.interface import SQLAInterface +from pytest_mock import MockerFixture +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from superset.subjects.filters import SubjectListFilter +from superset.subjects.models import Subject + + +def _compiled(query) -> str: + engine = create_engine("sqlite://") + return str( + query.statement.compile(engine, compile_kwargs={"literal_binds": True}), + ) + + +def _subject_query(): + session = sessionmaker(bind=create_engine("sqlite://"))() + return session.query(Subject) + + +def test_subject_list_filter_excludes_configured_users( + mocker: MockerFixture, +) -> None: + mock_current_app = MagicMock() + mock_current_app.config = { + "EXCLUDE_USERS_FROM_LISTS": ["service_account"], + "EXTRA_RELATED_QUERY_FILTERS": {}, + } + mocker.patch("superset.subjects.filters.current_app", mock_current_app) + + filter_ = SubjectListFilter("id", SQLAInterface(Subject)) + filtered = filter_.apply(_subject_query(), None) + + assert "service_account" in _compiled(filtered) + + +def test_subject_list_filter_is_a_noop_without_configuration( + mocker: MockerFixture, +) -> None: + mock_current_app = MagicMock() + mock_current_app.config = {} + mocker.patch("superset.subjects.filters.current_app", mock_current_app) + + query = _subject_query() + filter_ = SubjectListFilter("id", SQLAInterface(Subject)) + + assert filter_.apply(query, None) is query + + +def test_subject_rest_api_registers_the_scoping_base_filter() -> None: + from superset.subjects.api import SubjectRestApi + + registered = [spec[1] for spec in SubjectRestApi.base_filters] + + assert SubjectListFilter in registered diff --git a/tests/unit_tests/subjects/test_creator_group_call_sites.py b/tests/unit_tests/subjects/test_creator_group_call_sites.py new file mode 100644 index 00000000000..03ab39f6f95 --- /dev/null +++ b/tests/unit_tests/subjects/test_creator_group_call_sites.py @@ -0,0 +1,415 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests that asset-creation paths attach the creator's default viewers. + +``get_default_viewers_for_new_asset`` is covered on its own elsewhere; these +tests pin the wiring at the call sites that build an asset outside the create +commands, so removing one fails the suite. Covered here: both v1 importers, +the template copy, ``/dashboard/new/``, the dashboard-copy DAO (including its +cloned charts), the legacy explore save paths, and ``reset_ownership`` (used +by the v0 importers). The MCP ``generate_dashboard`` tool is covered by +``tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py``. +""" + +from types import SimpleNamespace +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest + +from superset.subjects.models import Subject +from superset.subjects.types import SubjectType + + +def _group_subject(id_: int = 11) -> Subject: + subject = Subject() + subject.id = id_ + subject.type = SubjectType.GROUP + return subject + + +def _user(user_id: int = 5) -> SimpleNamespace: + return SimpleNamespace(id=user_id) + + +def test_chart_importer_attaches_default_viewers(app_context) -> None: + from superset.commands.chart.importers.v1 import utils as chart_utils + + viewer = _group_subject() + chart = SimpleNamespace(id=1, editors=[], viewers=[]) + + with ( + patch.object(chart_utils, "find_existing_for_import", return_value=None), + patch.object(chart_utils, "get_user", return_value=_user()), + patch.object(chart_utils, "filter_chart_annotations"), + patch.object(chart_utils, "migrate_chart", side_effect=lambda config: config), + patch.object(chart_utils.Slice, "import_from_dict", return_value=chart), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + ): + result = chart_utils.import_chart( + {"uuid": "x", "params": {}}, ignore_permissions=True + ) + + assert result.viewers == [viewer] + + +def test_chart_importer_adds_missing_viewers_without_duplicating_present_ones( + app_context, +) -> None: + from superset.commands.chart.importers.v1 import utils as chart_utils + + already_there = _group_subject(11) + newly_added = _group_subject(12) + chart = SimpleNamespace(id=1, editors=[], viewers=[already_there]) + + with ( + patch.object(chart_utils, "find_existing_for_import", return_value=None), + patch.object(chart_utils, "get_user", return_value=_user()), + patch.object(chart_utils, "filter_chart_annotations"), + patch.object(chart_utils, "migrate_chart", side_effect=lambda config: config), + patch.object(chart_utils.Slice, "import_from_dict", return_value=chart), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[already_there, newly_added], + ), + ): + result = chart_utils.import_chart( + {"uuid": "x", "params": {}}, ignore_permissions=True + ) + + assert result.viewers == [already_there, newly_added] + + +def test_copy_dashboard_attaches_viewers_from_the_users_in_memory_groups( + app_context, +) -> None: + """The template copy runs in ``after_insert``, before ``ab_user_group`` exists.""" + from superset.models import dashboard as dashboard_module + + viewer = _group_subject() + group = MagicMock() + new_user = MagicMock() + new_user.id = 5 + new_user.groups = [group] + session = MagicMock() + # Both lookups (the user, then the template dashboard) go through the same + # mocked chain; a MagicMock stands in for either. + session.query.return_value.filter_by.return_value.first.return_value = new_user + + created: dict[str, Any] = {} + + def _capture(**kwargs): + created.update(kwargs) + return MagicMock() + + with ( + patch.dict(dashboard_module.app.config, {"DASHBOARD_TEMPLATE_ID": 1}), + patch.object( + dashboard_module.sqla, "inspect", return_value=MagicMock(session=session) + ), + patch.object(dashboard_module, "Dashboard", side_effect=_capture), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_groups", + return_value=[viewer], + ) as mock_for_groups, + ): + dashboard_module.copy_dashboard( + MagicMock(), + MagicMock(), + SimpleNamespace(id=5), # type: ignore[arg-type] + ) + + assert created["viewers"] == [viewer] + # Resolved from the in-memory collection, never by querying membership. + mock_for_groups.assert_called_once_with([group]) + + +def test_dashboard_importer_attaches_default_viewers(app_context) -> None: + from superset.commands.dashboard.importers.v1 import utils as dashboard_utils + + viewer = _group_subject() + dashboard = SimpleNamespace(id=1, editors=[], viewers=[]) + + with ( + patch.object(dashboard_utils, "find_existing_for_import", return_value=None), + patch.object(dashboard_utils, "get_user", return_value=_user()), + patch.object( + dashboard_utils.Dashboard, "import_from_dict", return_value=dashboard + ), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + ): + result = dashboard_utils.import_dashboard( + {"uuid": "x", "metadata": {}}, ignore_permissions=True + ) + + assert result.viewers == [viewer] + + +def test_new_dashboard_view_attaches_default_viewers(app_context) -> None: + from superset.views.dashboard import views as dashboard_views + + viewer = _group_subject() + created: dict[str, Any] = {} + + def _capture(**kwargs): + created.update(kwargs) + return MagicMock(id=1) + + # Strip ``@has_access``/``@expose`` so the handler body runs directly. + handler = dashboard_views.Dashboard.new + while hasattr(handler, "__wrapped__"): + handler = handler.__wrapped__ + + with ( + patch.object(dashboard_views, "DashboardModel", side_effect=_capture), + patch.object(dashboard_views, "db"), + patch.object(dashboard_views, "redirect"), + patch.object(dashboard_views, "url_for"), + patch.object(dashboard_views, "g", MagicMock(user=_user())), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + ): + handler(MagicMock()) + + assert created["viewers"] == [viewer] + + +def test_dashboard_copy_dao_attaches_default_viewers(app_context) -> None: + from superset.daos import dashboard as dashboard_dao + + viewer = _group_subject() + original = MagicMock() + original.slices = [] + created: list[Any] = [] + + with ( + patch.object(dashboard_dao.security_manager, "is_editor", return_value=True), + patch.object(dashboard_dao, "g", MagicMock(user=_user())), + patch.object(dashboard_dao, "db"), + patch.object(dashboard_dao.DashboardDAO, "set_dash_metadata"), + patch.object( + dashboard_dao, + "Dashboard", + side_effect=lambda: created.append(MagicMock()) # type: ignore[func-returns-value] + or created[-1], + ), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + ): + dashboard_dao.DashboardDAO.copy_dashboard( + original, {"dashboard_title": "copy", "json_metadata": "{}"} + ) + + assert created[-1].viewers == [viewer] + + +def test_dashboard_copy_dao_attaches_default_viewers_to_cloned_charts( + app_context, +) -> None: + from superset.daos import dashboard as dashboard_dao + + viewer = _group_subject() + clone = MagicMock() + source_slice = MagicMock() + source_slice.clone.return_value = clone + original = MagicMock() + original.slices = [source_slice] + + with ( + patch.object(dashboard_dao.security_manager, "is_editor", return_value=True), + patch.object(dashboard_dao, "g", MagicMock(user=_user())), + patch.object(dashboard_dao, "db"), + patch.object(dashboard_dao.DashboardDAO, "set_dash_metadata"), + patch.object(dashboard_dao, "Dashboard", return_value=MagicMock()), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + ): + dashboard_dao.DashboardDAO.copy_dashboard( + original, + { + "dashboard_title": "copy", + "json_metadata": '{"positions": {}}', + "duplicate_slices": True, + }, + ) + + assert clone.viewers == [viewer] + + +def test_reset_ownership_attaches_default_viewers(app_context) -> None: + """Used by the v0 importers, which never went through the v1 wiring.""" + from superset.models.helpers import ImportExportMixin + + viewer = _group_subject() + + class _Asset(ImportExportMixin): + def __init__(self) -> None: + self.editors: list[Any] = [] + self.viewers: list[Any] = [] + + asset = _Asset() + + with ( + patch("superset.models.helpers.g", MagicMock(user=_user())), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + ): + asset.reset_ownership() + + assert asset.viewers == [viewer] + + +class _ConstructedError(Exception): + """Raised once the asset under test has been constructed.""" + + +def _capturing(store: dict[str, Any]): + """Record constructor kwargs, then abort the surrounding request handler. + + ``save_or_overwrite_slice`` does a lot of work after building the asset; + stopping at construction keeps these tests focused on the wiring. + """ + + def _factory(**kwargs): + store.update(kwargs) + raise _ConstructedError + + return _factory + + +def test_legacy_explore_saveas_attaches_default_viewers(app_context) -> None: + """The ``@deprecated`` explore save route is still mounted and reachable.""" + from superset.views import core as core_views + + viewer = _group_subject() + created: dict[str, Any] = {} + request = MagicMock() + request.args.get.side_effect = lambda key, *a: { + "slice_name": "n", + "action": "saveas", + }.get(key) + + with ( + patch.object(core_views, "request", request), + patch.object( + core_views, "get_form_data", return_value=({"viz_type": "table"}, None) + ), + patch.object(core_views, "Slice", side_effect=_capturing(created)), + patch.object(core_views, "g", MagicMock(user=_user())), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + pytest.raises(_ConstructedError), + ): + core_views.Superset.save_or_overwrite_slice( + None, True, False, False, 1, "table", "t" + ) + + assert created["viewers"] == [viewer] + + +def test_legacy_explore_new_dashboard_attaches_default_viewers(app_context) -> None: + """Saving a chart to a brand new dashboard from legacy explore.""" + from superset.views import core as core_views + + viewer = _group_subject() + created: dict[str, Any] = {} + request = MagicMock() + request.args.get.side_effect = lambda key, *a: { + "slice_name": "n", + "new_dashboard_name": "d", + }.get(key) + + with ( + patch.object(core_views, "request", request), + patch.object( + core_views, "get_form_data", return_value=({"viz_type": "table"}, None) + ), + patch.object(core_views, "Dashboard", side_effect=_capturing(created)), + patch.object(core_views, "g", MagicMock(user=_user())), + patch.object(core_views.security_manager, "can_access", return_value=True), + patch.object(core_views.utils, "remove_extra_adhoc_filters"), + patch("superset.subjects.utils.get_user_subject", return_value=None), + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[viewer], + ), + pytest.raises(_ConstructedError), + ): + core_views.Superset.save_or_overwrite_slice( + MagicMock(), False, False, False, 1, "table", "t" + ) + + assert created["viewers"] == [viewer] + + +def test_dashboard_copy_dao_resolves_creator_subjects_once(app_context) -> None: + """Both lookups are loop-invariant: N cloned charts must not mean N queries.""" + from superset.daos import dashboard as dashboard_dao + + original = MagicMock() + original.slices = [MagicMock(), MagicMock(), MagicMock()] + + with ( + patch.object(dashboard_dao.security_manager, "is_editor", return_value=True), + patch.object(dashboard_dao, "g", MagicMock(user=_user())), + patch.object(dashboard_dao, "db"), + patch.object(dashboard_dao.DashboardDAO, "set_dash_metadata"), + patch.object(dashboard_dao, "Dashboard", return_value=MagicMock()), + patch( + "superset.subjects.utils.get_user_subject", return_value=None + ) as mock_user_subject, + patch( + "superset.subjects.utils.get_default_viewers_for_new_asset", + return_value=[_group_subject()], + ) as mock_viewers, + ): + dashboard_dao.DashboardDAO.copy_dashboard( + original, + { + "dashboard_title": "copy", + "json_metadata": '{"positions": {}}', + "duplicate_slices": True, + }, + ) + + assert mock_user_subject.call_count == 1 + assert mock_viewers.call_count == 1 diff --git a/tests/unit_tests/subjects/test_creator_group_defaults.py b/tests/unit_tests/subjects/test_creator_group_defaults.py new file mode 100644 index 00000000000..ce0afb0dbd5 --- /dev/null +++ b/tests/unit_tests/subjects/test_creator_group_defaults.py @@ -0,0 +1,270 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests for defaulting a new asset's viewers to its creator's groups. + +Assigning the creator's groups as *viewers* (rather than editors) shares the +asset read-only, and is gated behind ``ASSIGN_CREATOR_GROUPS_AS_VIEWERS`` +because populating ``viewers`` changes how access is enforced: once an asset +has any viewer, the datasource-permission fallback no longer applies to it. +""" + +from typing import Any +from unittest.mock import MagicMock, patch + +from superset.commands.utils import populate_subjects +from superset.subjects.models import Subject +from superset.subjects.types import SubjectType +from superset.subjects.utils import ( + get_default_viewers_for_groups, + get_default_viewers_for_new_asset, + get_user_group_subject_ids_subquery, +) + + +def _group_subject(id_: int) -> Subject: + subject = Subject() + subject.id = id_ + subject.type = SubjectType.GROUP + return subject + + +def test_user_group_subject_ids_subquery_joins_group_membership(app_context) -> None: + sql = str( + get_user_group_subject_ids_subquery(7).compile( + compile_kwargs={"literal_binds": True} + ) + ) + + assert "ab_user_group" in sql + assert "user_id = 7" in sql + + +@patch("superset.commands.utils.get_user_id", return_value=5) +@patch("superset.commands.utils.populate_subject_list") +@patch("superset.subjects.utils.get_user_group_subjects") +def test_new_asset_viewers_default_to_creator_groups_when_enabled( + mock_groups: MagicMock, + mock_populate: MagicMock, + mock_user_id: MagicMock, + app_context, +) -> None: + groups = [_group_subject(11), _group_subject(12)] + mock_groups.return_value = groups + mock_populate.return_value = [_group_subject(1)] + properties: dict[str, Any] = {} + + with patch.dict( + "superset.commands.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + populate_subjects(properties, []) + + assert properties["viewers"] == groups + + +@patch("superset.commands.utils.get_user_id", return_value=5) +@patch("superset.commands.utils.populate_subject_list") +@patch("superset.subjects.utils.get_user_group_subjects") +def test_new_asset_gets_no_default_viewers_when_disabled( + mock_groups: MagicMock, + mock_populate: MagicMock, + mock_user_id: MagicMock, + app_context, +) -> None: + mock_groups.return_value = [_group_subject(11)] + mock_populate.return_value = [_group_subject(1)] + properties: dict[str, Any] = {} + + with patch.dict( + "superset.commands.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": False}, + ): + populate_subjects(properties, []) + + assert "viewers" not in properties + + +@patch("superset.subjects.utils.get_user_group_subjects") +def test_default_viewers_are_empty_for_an_anonymous_creator( + mock_groups: MagicMock, + app_context, +) -> None: + with patch.dict( + "superset.subjects.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + assert get_default_viewers_for_new_asset(None) == [] + + mock_groups.assert_not_called() + + +@patch("superset.subjects.utils.get_user_group_subjects") +def test_default_viewers_are_empty_when_the_setting_is_off( + mock_groups: MagicMock, + app_context, +) -> None: + with patch.dict( + "superset.subjects.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": False}, + ): + assert get_default_viewers_for_new_asset(5) == [] + + mock_groups.assert_not_called() + + +@patch("superset.subjects.utils.get_user_group_subjects") +def test_default_viewers_are_the_creators_groups_when_enabled( + mock_groups: MagicMock, + app_context, +) -> None: + groups = [_group_subject(11)] + mock_groups.return_value = groups + + with patch.dict( + "superset.subjects.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + assert get_default_viewers_for_new_asset(5) == groups + + +@patch("superset.commands.utils.get_user_id", return_value=5) +@patch("superset.commands.utils.populate_subject_list") +@patch("superset.subjects.utils.get_user_group_subjects") +def test_explicit_viewers_are_not_replaced_by_creator_groups( + mock_groups: MagicMock, + mock_populate: MagicMock, + mock_user_id: MagicMock, + app_context, +) -> None: + chosen = [_group_subject(99)] + mock_groups.return_value = [_group_subject(11)] + mock_populate.side_effect = [[_group_subject(1)], chosen] + properties: dict[str, Any] = {"viewers": [99]} + + with patch.dict( + "superset.commands.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + populate_subjects(properties, []) + + assert properties["viewers"] == chosen + mock_groups.assert_not_called() + + +@patch("superset.subjects.utils.subjects_from_groups") +def test_default_viewers_for_groups_resolves_in_memory_groups( + mock_from_groups: MagicMock, + app_context, +) -> None: + """Flush-time callers hold groups in memory before ab_user_group is written.""" + groups = [_group_subject(11)] + mock_from_groups.return_value = groups + + with patch.dict( + "superset.subjects.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + assert get_default_viewers_for_groups([MagicMock()]) == groups + + +@patch("superset.subjects.utils.subjects_from_groups") +def test_default_viewers_for_groups_is_empty_when_the_setting_is_off( + mock_from_groups: MagicMock, + app_context, +) -> None: + with patch.dict( + "superset.subjects.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": False}, + ): + assert get_default_viewers_for_groups([MagicMock()]) == [] + + mock_from_groups.assert_not_called() + + +@patch("superset.subjects.utils.get_user_group_subjects") +def test_user_id_zero_is_treated_as_a_real_principal( + mock_groups: MagicMock, + app_context, +) -> None: + """A falsy-but-valid id must not be conflated with 'no user'.""" + groups = [_group_subject(11)] + mock_groups.return_value = groups + + with patch.dict( + "superset.subjects.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + assert get_default_viewers_for_new_asset(0) == groups + + +def test_user_group_subject_ids_subquery_restricts_to_group_subjects( + app_context, +) -> None: + """Defense in depth: don't rely on ``group_id`` alone to imply the type.""" + sql = str( + get_user_group_subject_ids_subquery(7).compile( + compile_kwargs={"literal_binds": True} + ) + ) + + assert "subjects.type = 3" in sql + + +def test_subjects_from_groups_issues_a_single_query(app_context) -> None: + """One query for the whole list, not one per group.""" + from superset.subjects import utils as subjects_utils + + with patch.object(subjects_utils, "db") as mock_db: + mock_db.session.query.return_value.filter.return_value.all.return_value = [] + subjects_utils.subjects_from_groups([MagicMock(id=1), MagicMock(id=2)]) + + assert mock_db.session.query.call_count == 1 + + +@patch("superset.commands.utils.get_user_id", return_value=5) +@patch("superset.commands.utils.populate_subject_list", return_value=[]) +@patch("superset.subjects.utils.get_user_group_subjects") +def test_an_explicit_empty_viewers_list_suppresses_the_group_default( + mock_groups: MagicMock, + mock_populate: MagicMock, + mock_user_id: MagicMock, + app_context, +) -> None: + """``viewers: []`` means "no viewers", not "fall back to my groups".""" + properties: dict[str, Any] = {"viewers": []} + + with patch.dict( + "superset.commands.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + populate_subjects(properties, []) + + assert properties["viewers"] == [] + mock_groups.assert_not_called() + + +@patch("superset.subjects.utils.get_user_group_subjects", return_value=[]) +def test_a_creator_without_groups_leaves_the_dataset_fallback_intact( + mock_groups: MagicMock, + app_context, +) -> None: + """No groups means no viewers, so the asset keeps dataset-based access.""" + with patch.dict( + "superset.subjects.utils.current_app.config", + {"ASSIGN_CREATOR_GROUPS_AS_VIEWERS": True}, + ): + assert get_default_viewers_for_new_asset(5) == [] diff --git a/tests/unit_tests/views/test_related_include_ids.py b/tests/unit_tests/views/test_related_include_ids.py new file mode 100644 index 00000000000..dc9e42802a8 --- /dev/null +++ b/tests/unit_tests/views/test_related_include_ids.py @@ -0,0 +1,124 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests that ``?include_ids=`` cannot bypass related-field scoping. + +The ``/related/<column>`` endpoints scope their results with +``base_related_field_filters``. The ``include_ids`` argument force-fetches +specific rows, and must not be able to resolve principals that those filters +deliberately hide. + +``include_ids`` exists so an edit form can render a value that is already +associated with a record but falls outside the current page or search of the +picker. Scoping it has an accepted consequence: a value that is already +associated but has since been hidden by ``EXCLUDE_USERS_FROM_LISTS`` or +``EXTRA_RELATED_QUERY_FILTERS`` stops resolving, so the form shows the bare id +rather than a label. That is deliberate — a principal a deployment has chosen +to hide must not be resolvable through a side door, and the association itself +is untouched in the database. +""" + +from typing import Any + +from flask_appbuilder.models.filters import BaseFilter +from flask_appbuilder.models.sqla.interface import SQLAInterface +from pytest_mock import MockerFixture +from sqlalchemy import Column, create_engine, Integer, String +from sqlalchemy.orm import declarative_base, sessionmaker + +from superset.views.base_api import BaseSupersetModelRestApi + +Base = declarative_base() + + +class Principal(Base): # type: ignore[misc, valid-type] + __tablename__ = "principal" + + id = Column(Integer, primary_key=True) + name = Column(String(50)) + + def __repr__(self) -> str: + return str(self.name) + + +class ExcludeHiddenFilter(BaseFilter): # pylint: disable=too-few-public-methods + """Stand-in for a deployment's principal-scoping filter.""" + + name = "Exclude hidden" + arg_name = "exclude_hidden" + + def apply(self, query: Any, value: Any) -> Any: + return query.filter(Principal.name != "hidden") + + +def _session(): + engine = create_engine("sqlite://") + Base.metadata.create_all(engine) + session = sessionmaker(bind=engine)() + session.add_all( + [Principal(id=1, name="visible"), Principal(id=2, name="hidden")], + ) + session.commit() + return session + + +def _api(**overrides: Any) -> BaseSupersetModelRestApi: + api = BaseSupersetModelRestApi.__new__(BaseSupersetModelRestApi) + api.text_field_rel_fields = {} + api.extra_fields_rel_fields = {} + api.base_related_field_filters = {} + for key, value in overrides.items(): + setattr(api, key, value) + return api + + +def test_include_ids_cannot_resolve_a_filtered_out_principal( + mocker: MockerFixture, +) -> None: + """Holds even when the principal is already associated with the record. + + This is the case an edit form hits: the client sends the id of a current + owner, and a hidden principal must still not resolve to a label. + """ + session = _session() + mocker.patch("superset.views.base_api.db").session = session + api = _api( + base_related_field_filters={ + "owners": [["name", ExcludeHiddenFilter, None]], + }, + ) + result: list[dict[str, Any]] = [] + + api._add_extra_ids_to_result( # noqa: SLF001 + SQLAInterface(Principal, session), "owners", [1, 2], result + ) + + assert [row["value"] for row in result] == [1] + + +def test_include_ids_still_resolves_when_no_filters_are_configured( + mocker: MockerFixture, +) -> None: + session = _session() + mocker.patch("superset.views.base_api.db").session = session + api = _api() + result: list[dict[str, Any]] = [] + + api._add_extra_ids_to_result( # noqa: SLF001 + SQLAInterface(Principal, session), "owners", [1, 2], result + ) + + assert sorted(row["value"] for row in result) == [1, 2] diff --git a/tests/unit_tests/views/test_user_schemas.py b/tests/unit_tests/views/test_user_schemas.py new file mode 100644 index 00000000000..6fafc344d9c --- /dev/null +++ b/tests/unit_tests/views/test_user_schemas.py @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Unit tests for the current-user (``/api/v1/me/``) response schema.""" + +from types import SimpleNamespace + +from superset.views.users.schemas import UserResponseSchema + + +def _user(**overrides): + """Build a stand-in for a FAB ``User`` row.""" + attrs = { + "id": 1, + "username": "alice", + "email": "[email protected]", + "first_name": "Alice", + "last_name": "Doe", + "is_active": True, + "is_anonymous": False, + "login_count": 3, + "groups": [], + } + attrs.update(overrides) + return SimpleNamespace(**attrs) + + +def test_user_response_includes_the_callers_groups(): + groups = [SimpleNamespace(id=7, name="tenant-a")] + + result = UserResponseSchema().dump(_user(groups=groups)) + + assert result["groups"] == [{"id": 7, "name": "tenant-a"}] + + +def test_user_response_groups_is_empty_list_when_user_has_no_groups(): + result = UserResponseSchema().dump(_user(groups=[])) + + assert result["groups"] == []
