You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by as...@apache.org on 2026/10/21 11:30:34 UTC
(superset) branch master updated: feat(improvements): let deployments withhold assets from list queries and commands (#45451)
This is an automated email from the ASF dual-hosted git repository.
alexandrusoare pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 75be8f981a6 feat(improvements): let deployments withhold assets from list queries and commands (#45451)
75be8f981a6 is described below
commit 75be8f981a6c299f1316de22b7caeef64185fb1b
Author: Alexandru Soare <37...@users.noreply.github.com>
AuthorDate: Wed Oct 21 14:30:26 2026 +0300
feat(improvements): let deployments withhold assets from list queries and commands (#45451)
---
docs/admin_docs/security/security.mdx | 63 +++++
superset/charts/filters.py | 8 +
superset/commands/report/base.py | 73 ++++++
superset/config.py | 36 +++
superset/daos/dashboard.py | 7 +
superset/dashboards/filters.py | 7 +
superset/reports/api.py | 15 +-
superset/reports/filters.py | 37 +++
superset/utils/filters.py | 111 ++++++++-
tests/unit_tests/charts/test_filters.py | 110 +++++++++
tests/unit_tests/commands/report/base_test.py | 239 +++++++++++++++++++
.../dashboards/semantic_view_access_test.py | 141 ++++++++++-
tests/unit_tests/reports/filters_test.py | 53 +++++
tests/unit_tests/utils/filters_test.py | 260 ++++++++++++++++++++-
14 files changed, 1151 insertions(+), 9 deletions(-)
diff --git a/docs/admin_docs/security/security.mdx b/docs/admin_docs/security/security.mdx
index 0d9feb9757c..20d602fd6df 100644
--- a/docs/admin_docs/security/security.mdx
+++ b/docs/admin_docs/security/security.mdx
@@ -283,6 +283,69 @@ archive is scoped to editors via a SQL query). It does still run once per row on
dashboard list responses to populate `extra_editors`, so a slow or unavailable external resolver
affects ordinary list requests, not just direct per-object checks.
+#### Widening and Narrowing List Visibility
+
+Two config hooks let a deployment adjust which charts and dashboards a principal sees in list
+queries. They are a pair: one can only widen a result set, the other only narrow it.
+
+`EXTRA_ACCESS_QUERY_FILTERS` grants visibility to assets the role paths would not reach. Its
+conditions are OR'd into `ChartFilter` and `DashboardAccessFilter`, so it can add rows and never
+remove them:
+
+```python
+EXTRA_ACCESS_QUERY_FILTERS = {
+ "charts": lambda user_id: ..., # ids to additionally make visible
+ "dashboards": lambda user_id: ...,
+}
+```
+
+`EXTRA_ACCESS_QUERY_EXCLUSIONS` is the opposite: the ids it names are withheld, as an
+`AND NOT`:
+
+```python
+EXTRA_ACCESS_QUERY_EXCLUSIONS = {
+ "charts": lambda user_id: ..., # ids to withhold
+ "report_target_dashboards": lambda user_id: ...,
+}
+```
+
+Each callable receives the current user id — or `None` on an anonymous request — and returns the
+asset ids as an ORM `Query`, a Core `select()`, or a `.subquery()`; all three are accepted.
+
+Three details govern the behavior:
+
+- Exclusions are applied **before** the `is_admin()` and `can_access_all_datasources()`
+ short-circuits, because those are the principals the role paths stop narrowing. An exclusion
+ therefore withholds an asset from an Admin as well.
+- Embedded guests are **exempt**. A guest token already scopes its holder to the dashboards it
+ names and fails closed, so narrowing further would make an embedded dashboard's contents
+ depend on state the token has no knowledge of.
+- Anonymous requests are **not** exempt. A deployment serving published dashboards through the
+ Public role has callers with no user id, and skipping a withholding rule for them would reveal
+ rather than hide — so the callable is invoked with `None` and decides for itself.
+- Both hooks feed `ChartFilter` and `DashboardAccessFilter`, which are also the DAO base
+ filters. One entry therefore covers list views, `find_by_ids` — and so exports — and
+ related-field lookups together, plus the REST detail endpoints: a withheld dashboard is
+ absent from `/api/v1/dashboard/<id>` and from `/api/v1/dashboard/<uuid>`, which reaches it
+ through a branch that skips the base filter.
+
+An exclusion shapes what a principal is *offered*; it is not an access check. Superset
+decides whether a principal may read a given asset in `raise_for_access`, so a deployment
+that needs an asset to be unreadable rather than merely unlisted enforces that there,
+through a custom security manager. The two are complementary: the check makes the asset
+unreadable, the exclusion keeps it out of the places it would otherwise be offered.
+
+The `report_target_charts` and `report_target_dashboards` keys are a narrower variant, consulted
+only where a scheduled report's target is chosen or validated. A report is rendered by whichever
+principal `ALERT_REPORTS_EXECUTORS` names rather than by its author, so "may this user read this
+asset" and "may this user schedule a report on it" are separate questions a deployment may answer
+differently.
+
+A list filter is not an authorization boundary: an id posted directly to an API never passes
+through one. Report creation and update therefore consult the same exclusion where the posted id
+is resolved, and refuse it with a 403. A deployment adding its own command paths should do the
+same rather than relying on the asset being absent from a picker.
+
### Dashboard Access Control
Access to dashboards is managed via editors (subjects that have edit permissions to the dashboard).
diff --git a/superset/charts/filters.py b/superset/charts/filters.py
index 203ccb79d61..d0296575f7a 100644
--- a/superset/charts/filters.py
+++ b/superset/charts/filters.py
@@ -37,6 +37,7 @@ from superset.subjects.models import chart_editors
from superset.tags.filters import BaseTagIdFilter, BaseTagNameFilter
from superset.utils.core import get_user_id
from superset.utils.filters import (
+ apply_extra_access_exclusions,
get_dataset_access_filters,
guest_embedded_dashboard_filter,
semantic_layer_grant_clause,
@@ -123,6 +124,13 @@ class ChartFilter(BaseFilter): # pylint: disable=too-few-public-methods
if (guest_dashboards := guest_embedded_dashboard_filter()) is not None:
return query.filter(self.model.dashboards.any(guest_dashboards))
+ # Applied before the short-circuit below: an exclusion has to hold for
+ # principals who can reach every dataset, since those are exactly the
+ # ones the role paths stop narrowing. Guests are excepted above --
+ # their token already scopes them, and withholding here would strip
+ # charts from a dashboard they were legitimately granted.
+ query = apply_extra_access_exclusions(query, "charts", Slice.id)
+
if security_manager.can_access_all_datasources():
return query
diff --git a/superset/commands/report/base.py b/superset/commands/report/base.py
index b12aa14cfe9..9810d01a29f 100644
--- a/superset/commands/report/base.py
+++ b/superset/commands/report/base.py
@@ -62,6 +62,7 @@ from superset.sql.parse import SQLScript
from superset.tasks.types import ExecutorType
from superset.utils import json
from superset.utils.core import get_user
+from superset.utils.filters import is_excluded_by_extra_access
if TYPE_CHECKING:
from flask_appbuilder.security.sqla.models import User
@@ -75,6 +76,9 @@ _JINJA_BLOCK_RE = re.compile(r"\{\{.*?\}\}|\{%.*?%\}|\{#.*?#\}", re.DOTALL)
class BaseReportScheduleCommand(BaseCommand):
_properties: dict[str, Any]
+ # Set by the update/delete subclasses, absent on create -- read with
+ # ``getattr(self, "_model", None)`` where both cases are possible.
+ _model: Any
def run(self) -> Any:
pass
@@ -128,6 +132,25 @@ class BaseReportScheduleCommand(BaseCommand):
)
)
+ def _result_is_active(self) -> bool:
+ """Whether this request leaves the schedule running.
+
+ What the exclusion checks guard is delivery, not editing. Refusing a
+ request that leaves the schedule paused would trap its owner: they could
+ neither pause the report nor stop the scheduler selecting it, which is
+ strictly worse than not checking at all.
+
+ Both access paths consult this, because either can carry the pause. The
+ list page's toggle sends ``{"active": false}`` on its own and reaches the
+ stored-target path, while the edit modal always resends the stored
+ chart/dashboard id alongside its own active switch and so reaches the
+ submitted-target path. Answering it in one place and asking it from both
+ is the point; an exemption that only one path could see is what made
+ pausing from the modal a 403.
+ """
+ model = getattr(self, "_model", None)
+ return bool(self._properties.get("active", getattr(model, "active", True)))
+
def _check_object_access(
self,
object_id: int,
@@ -142,6 +165,15 @@ class BaseReportScheduleCommand(BaseCommand):
if not obj:
exceptions.append(not_found_exc())
else:
+ # The related-field filter narrows the picker, but a list filter is
+ # not an authorisation boundary: ``find_by_id`` above applies the
+ # ordinary access filter, so an excluded id posted directly would
+ # otherwise be accepted. Ask the report-target question here, where
+ # the id is actually resolved.
+ if self._result_is_active() and is_excluded_by_extra_access(
+ f"report_target_{kind}s", object_id
+ ):
+ raise ReportScheduleForbiddenError()
try:
security_manager.raise_for_access(**{kind: obj})
except SupersetSecurityException as ex:
@@ -217,6 +249,47 @@ class BaseReportScheduleCommand(BaseCommand):
)
if not effective_chart and not effective_dashboard:
exceptions.append(ReportScheduleEitherChartOrDashboardError())
+ return
+
+ self._check_stored_target_access(
+ model, effective_chart, effective_dashboard
+ )
+
+ def _check_stored_target_access(
+ self,
+ model: Any,
+ effective_chart: Any,
+ effective_dashboard: Any,
+ ) -> None:
+ """Ask of a stored target what is asked of a submitted one.
+
+ Without this, activating a paused report resumes a schedule whose asset
+ has since been withheld, while resubmitting that same asset is refused.
+
+ Only when the result is a *scheduled* report; see
+ :meth:`_result_is_active`.
+ """
+ if not self._result_is_active():
+ return
+
+ kind, target = (
+ ("chart", effective_chart)
+ if effective_chart
+ else ("dashboard", effective_dashboard)
+ )
+ target_id = getattr(target, "id", target)
+ if not isinstance(target_id, int):
+ return
+
+ # Both keys: a submitted target reaches the ordinary one through
+ # ``find_by_id``'s base filter and the report-target one through
+ # ``_check_object_access``, so a stored target asking only the latter
+ # would accept what resubmitting it refuses.
+ if any(
+ is_excluded_by_extra_access(key, target_id)
+ for key in (f"report_target_{kind}s", f"{kind}s")
+ ):
+ raise ReportScheduleForbiddenError()
def _validate_report_extra( # noqa: C901
self, exceptions: list[ValidationError]
diff --git a/superset/config.py b/superset/config.py
index bf15334e8b7..723c366e4c7 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -49,6 +49,7 @@ from pandas._libs.parsers import STR_NA_VALUES
from sqlalchemy.engine.url import URL
from sqlalchemy.orm import Session
from sqlalchemy.orm.query import Query
+from sqlalchemy.sql.expression import Select, Subquery
from superset.advanced_data_type.plugins.internet_address import internet_address
from superset.advanced_data_type.plugins.internet_port import internet_port
@@ -3515,9 +3516,44 @@ class ExtraAccessQueryFilters(TypedDict, total=False):
dashboards: Callable[[int], Query]
+# Extra access query exclusions inject an AND NOT condition into ChartFilter
+# and DashboardAccessFilter, letting a deployment *withhold* asset visibility.
+# ``EXTRA_ACCESS_QUERY_FILTERS`` can only widen a result set, because its
+# conditions are OR'd with the role paths -- so a deployment that needs to keep
+# an asset out of the list, an export or a related-field lookup has no way to
+# express that through it.
+#
+# Exclusions are applied before the ``is_admin`` / ``can_access_all_datasources``
+# short-circuits, so they hold for every principal except embedded guests,
+# whose scope is set by their token alone.
+#
+# The callable receives the current user ID and returns a subquery of the asset
+# IDs to withhold.
+# Receives the current user id, or ``None`` for an anonymous request -- a
+# withholding rule is consulted even then, since skipping it would reveal
+# rather than hide. May return an ORM ``Query``, a Core ``select()`` or an
+# explicit ``.subquery()``; all three are accepted and normalised, matching
+# what the sibling ``EXTRA_ACCESS_QUERY_FILTERS`` hooks may hand to ``.in_()``.
+ExclusionEntry = Callable[[int | None], Query | Select | Subquery]
+
+
+class ExtraAccessQueryExclusions(TypedDict, total=False):
+ charts: ExclusionEntry
+ dashboards: ExclusionEntry
+ # Narrower than the two above: assets selectable as the target of a
+ # scheduled report, which renders as the configured executor rather than
+ # as the user who created it. A deployment may hold assets a user can read
+ # but cannot schedule a report on.
+ report_target_charts: ExclusionEntry
+ report_target_dashboards: ExclusionEntry
+
+
# Extension hooks for deployments to plug in custom access logic.
# Additional query filters for chart/dashboard list views.
EXTRA_ACCESS_QUERY_FILTERS: ExtraAccessQueryFilters = {}
+# Asset IDs to withhold from chart/dashboard list views, exports and
+# related-field lookups. See ``ExtraAccessQueryExclusions`` above.
+EXTRA_ACCESS_QUERY_EXCLUSIONS: ExtraAccessQueryExclusions = {}
# Bypass raise_for_access for specific assets. Return True to skip checks.
# The hook receives the complete query context, so setting it turns off the
# semantic-view chart-data check that otherwise denies before provider metadata
diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py
index cdf64565642..7883c6ab47b 100644
--- a/superset/daos/dashboard.py
+++ b/superset/daos/dashboard.py
@@ -56,6 +56,7 @@ from superset.models.slice import Slice
from superset.utils import json
from superset.utils.core import get_user_id
from superset.utils.dashboard_filter_scopes_converter import copy_filter_scopes
+from superset.utils.filters import is_excluded_by_extra_access
logger = logging.getLogger(__name__)
@@ -423,6 +424,12 @@ class DashboardDAO(BaseDAO[Dashboard]):
if is_uuid(id_or_slug):
# just get dashboard if it's uuid
dashboard = Dashboard.get(id_or_slug)
+ # This branch skips ``base_filter``, so an
+ # ``EXTRA_ACCESS_QUERY_EXCLUSIONS`` entry -- which reaches the id
+ # branch below through it -- would otherwise leave the same
+ # dashboard absent by id and readable by uuid.
+ if dashboard and is_excluded_by_extra_access("dashboards", dashboard.id):
+ raise DashboardNotFoundError()
else:
query = (
db.session.query(Dashboard)
diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py
index f91eb935b58..550be7720cc 100644
--- a/superset/dashboards/filters.py
+++ b/superset/dashboards/filters.py
@@ -35,6 +35,7 @@ from superset.subjects.models import dashboard_editors, dashboard_viewers
from superset.tags.filters import BaseTagIdFilter, BaseTagNameFilter
from superset.utils.core import get_user_id
from superset.utils.filters import (
+ apply_extra_access_exclusions,
get_dataset_access_filters,
guest_embedded_dashboard_filter,
semantic_layer_grant_clause,
@@ -130,6 +131,12 @@ class DashboardAccessFilter(BaseFilter): # pylint: disable=too-few-public-metho
if (guest_condition := guest_embedded_dashboard_filter()) is not None:
return query.filter(guest_condition)
+ # Applied before the short-circuit below: an exclusion has to hold for
+ # admins too, since that is precisely the principal the role paths stop
+ # narrowing. Guests are excepted above -- their token already scopes
+ # them.
+ query = apply_extra_access_exclusions(query, "dashboards", Dashboard.id)
+
if security_manager.is_admin():
return query
diff --git a/superset/reports/api.py b/superset/reports/api.py
index e917fd12d76..7a8302a6a2c 100644
--- a/superset/reports/api.py
+++ b/superset/reports/api.py
@@ -31,7 +31,6 @@ from flask_babel import gettext, ngettext
from marshmallow import ValidationError
from superset import is_feature_enabled, security_manager
-from superset.charts.filters import ChartFilter
from superset.commands.report.create import CreateReportScheduleCommand
from superset.commands.report.delete import DeleteReportScheduleCommand
from superset.commands.report.exceptions import (
@@ -51,11 +50,15 @@ from superset.commands.report.update import UpdateReportScheduleCommand
from superset.commands.report.update_config import UpdateReportConfigCommand
from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
from superset.daos.report import ReportConfigDAO
-from superset.dashboards.filters import DashboardAccessFilter
from superset.databases.filters import DatabaseFilter
from superset.exceptions import SupersetException
from superset.extensions import event_logger
-from superset.reports.filters import ReportScheduleAllTextFilter, ReportScheduleFilter
+from superset.reports.filters import (
+ ReportScheduleAllTextFilter,
+ ReportScheduleFilter,
+ ReportTargetChartFilter,
+ ReportTargetDashboardFilter,
+)
from superset.reports.models import ReportCreationMethod, ReportSchedule
from superset.reports.schemas import (
get_delete_ids_schema,
@@ -307,8 +310,10 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi):
}
base_related_field_filters = {
- "chart": [["id", ChartFilter, lambda: []]],
- "dashboard": [["id", DashboardAccessFilter, lambda: []]],
+ # Narrower than ordinary chart/dashboard visibility: a scheduled report
+ # renders as the deployment's configured executor, not as its author.
+ "chart": [["id", ReportTargetChartFilter, lambda: []]],
+ "dashboard": [["id", ReportTargetDashboardFilter, lambda: []]],
"database": [["id", DatabaseFilter, lambda: []]],
"created_by": [["id", BaseFilterRelatedUsers, lambda: []]],
"changed_by": [["id", BaseFilterRelatedUsers, lambda: []]],
diff --git a/superset/reports/filters.py b/superset/reports/filters.py
index af4781a4e48..d9469782878 100644
--- a/superset/reports/filters.py
+++ b/superset/reports/filters.py
@@ -21,9 +21,14 @@ from sqlalchemy import or_
from sqlalchemy.orm.query import Query
from superset import db, security_manager
+from superset.charts.filters import ChartFilter
from superset.daos.base import _escape_like
+from superset.dashboards.filters import DashboardAccessFilter
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
from superset.reports.models import ReportExecutionLog, ReportSchedule
from superset.subjects.filters import subject_relation_exists_for_current_user
+from superset.utils.filters import apply_extra_access_exclusions
from superset.views.base import BaseFilter
@@ -88,3 +93,35 @@ class ReportScheduleAllTextFilter(BaseFilter): # pylint: disable=too-few-public
ReportSchedule.sql.ilike(ilike_value, escape="\\"),
)
)
+
+
+class ReportTargetChartFilter(ChartFilter): # pylint: disable=too-few-public-methods
+ """Charts selectable as the target of a scheduled report.
+
+ A scheduled report is not rendered by the user who creates it: it runs later,
+ as whichever principal the deployment configures as its executor. So a
+ deployment can hold assets a user may read themselves but may not schedule a
+ report on, and that is a narrower question than ordinary chart visibility.
+
+ Only the exclusion key differs; every other rule is inherited. The exclusion
+ is applied before delegating so it survives the parent's short-circuits.
+ """
+
+ def apply(self, query: Query, value: Any) -> Query:
+ query = apply_extra_access_exclusions(query, "report_target_charts", Slice.id)
+ return super().apply(query, value)
+
+
+class ReportTargetDashboardFilter( # pylint: disable=too-few-public-methods
+ DashboardAccessFilter
+):
+ """Dashboards selectable as the target of a scheduled report.
+
+ See :class:`ReportTargetChartFilter`.
+ """
+
+ def apply(self, query: Query, value: Any) -> Query:
+ query = apply_extra_access_exclusions(
+ query, "report_target_dashboards", Dashboard.id
+ )
+ return super().apply(query, value)
diff --git a/superset/utils/filters.py b/superset/utils/filters.py
index 4eada39f8c4..471f9afd6f5 100644
--- a/superset/utils/filters.py
+++ b/superset/utils/filters.py
@@ -14,13 +14,14 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Optional, TYPE_CHECKING, Union
+from typing import Any, Callable, Optional, TYPE_CHECKING, Union
from flask_appbuilder import Model
-from sqlalchemy import and_, false, or_, true
+from sqlalchemy import and_, exists, false, or_, select, true
from sqlalchemy.sql.elements import BooleanClauseList, ColumnElement
if TYPE_CHECKING:
+ from sqlalchemy.orm.query import Query
from sqlalchemy.orm.util import AliasedClass
from superset.connectors.sqla.models import SqlaTable
@@ -176,3 +177,109 @@ def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
# dashboards, never a plain internal id.
conditions.append(and_(Dashboard.id.in_(int_ids), Dashboard.embedded.any()))
return or_(*conditions) if conditions else false()
+
+
+def apply_extra_access_exclusions(
+ query: "Query",
+ asset_kind: str,
+ id_column: "ColumnElement[Any]",
+) -> "Query":
+ """Withhold the asset IDs an ``EXTRA_ACCESS_QUERY_EXCLUSIONS`` hook names.
+
+ ``EXTRA_ACCESS_QUERY_FILTERS`` can only widen a result set -- its conditions
+ are OR'd with the role paths -- so a deployment with its own notion of a
+ withheld asset cannot express that through it. This is the matching AND NOT.
+
+ Returns ``query`` unchanged when no hook is configured or there is no
+ authenticated user, so the default build is unaffected.
+ """
+
+ clause = extra_access_exclusion_clause(asset_kind, id_column)
+ return query if clause is None else query.filter(clause)
+
+
+def extra_access_exclusion_clause(
+ asset_kind: str,
+ id_column: "ColumnElement[Any]",
+) -> Optional["ColumnElement[bool]"]:
+ """The ``NOT EXISTS`` clause for an exclusion hook, or None when inactive.
+
+ Deliberately ``NOT EXISTS`` rather than ``NOT IN``: a hook subquery that
+ yields a single NULL id makes ``NOT IN`` evaluate to NULL for every row,
+ which would silently empty the entire list -- for admins too. ``NOT EXISTS``
+ simply does not match a NULL, so a malformed hook withholds nothing instead
+ of everything.
+ """
+ hook = _extra_access_exclusion_hook(asset_kind)
+ if not hook:
+ return None
+
+ from superset.utils.core import get_user_id
+
+ # ``None`` for an anonymous request, and the hook is still consulted: a
+ # deployment serving published dashboards through the Public role has
+ # anonymous callers, and skipping a *withholding* rule for them would
+ # reveal rather than hide. A rule about who may see what has to answer for
+ # the principal who is nobody.
+ return ~exists().where(_exclusion_id_column(hook(get_user_id())) == id_column)
+
+
+def _extra_access_exclusion_hook(
+ asset_kind: str,
+) -> Optional[Callable[[Optional[int]], Any]]:
+ """The configured exclusion callable for this kind, or None.
+
+ Returns None for an embedded guest, so exclusions never apply to one. A
+ guest token already scopes its holder to the dashboards it names and fails
+ closed; withholding on top of that would make an embedded dashboard depend
+ on deployment state the token knows nothing about. ``ChartFilter`` and
+ ``DashboardAccessFilter`` return before reaching an exclusion for the same
+ reason -- the check lives here as well so a caller resolving a single id,
+ which has no such branch ahead of it, cannot miss it.
+ """
+ from flask import current_app
+
+ from superset import security_manager
+
+ if security_manager.is_guest_user():
+ return None
+
+ exclusions = current_app.config.get("EXTRA_ACCESS_QUERY_EXCLUSIONS") or {}
+ return exclusions.get(asset_kind)
+
+
+def _exclusion_id_column(excluded: Any) -> "ColumnElement[Any]":
+ """The single id column of whatever shape a hook returned.
+
+ A hook may return an ORM ``Query``, a Core ``select()`` or an explicit
+ ``.subquery()`` -- all three are natural things to write, and the sibling
+ ``EXTRA_ACCESS_QUERY_FILTERS`` hooks are handed straight to ``.in_()``,
+ which accepts any of them. ``Query`` and ``Select`` both offer
+ ``.subquery()`` while a ``Subquery`` does not, so that is what we key on.
+ Reaching for ``.c`` on a ``Select`` would work but is deprecated, and on a
+ ``Query`` it does not exist at all.
+ """
+ if hasattr(excluded, "subquery"):
+ excluded = excluded.subquery()
+ return list(excluded.c)[0]
+
+
+def is_excluded_by_extra_access(asset_kind: str, id_value: int) -> bool:
+ """Whether a single asset id is withheld by an exclusion hook.
+
+ List filters are not an authorisation boundary -- a command that resolves an
+ id directly has to ask the same question, or the asset is merely hidden from
+ the picker rather than withheld.
+ """
+ from superset import db
+ from superset.utils.core import get_user_id
+
+ hook = _extra_access_exclusion_hook(asset_kind)
+ if not hook:
+ return False
+
+ # Anonymous callers are asked about too; see extra_access_exclusion_clause.
+ excluded_id = _exclusion_id_column(hook(get_user_id()))
+ return bool(
+ db.session.execute(select(exists().where(excluded_id == id_value))).scalar()
+ )
diff --git a/tests/unit_tests/charts/test_filters.py b/tests/unit_tests/charts/test_filters.py
index 7c560cad1e2..3ab8b4d098b 100644
--- a/tests/unit_tests/charts/test_filters.py
+++ b/tests/unit_tests/charts/test_filters.py
@@ -275,3 +275,113 @@ def test_chart_filter_no_viewer_table_matches_by_database_access(
assert "slices.datasource_type = 'table'" in compiled
assert "JOIN tables AS tables_1" in compiled
assert "dbs.id IN (1, 2, 3)" in compiled
+
+
+# -- Exclusion-hook placement --
+#
+# Where the exclusion call sits is the whole behaviour: it must survive the
+# ``can_access_all_datasources()`` short-circuit, and must not reach an
+# embedded guest. Both of those are invisible to a test that calls
+# ``apply_extra_access_exclusions`` directly, so these go through
+# ``ChartFilter.apply``.
+
+
+def _compiled(clause: object) -> str:
+ return str(
+ clause.compile( # type: ignore[attr-defined]
+ create_engine("sqlite://"),
+ compile_kwargs={"literal_binds": True},
+ )
+ )
+
+
+def _chart_exclusion_hook() -> Any:
+ from sqlalchemy import select
+
+ from superset.models.slice import Slice
+
+ return lambda _uid: select(Slice.id).where(Slice.id == 99).subquery()
+
+
+def test_chart_filter_exclusion_survives_all_datasource_access(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """An all-datasource principal is exactly who the role paths stop narrowing."""
+ from flask import current_app
+
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.slice import Slice
+
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch(
+ "superset.charts.filters.guest_embedded_dashboard_filter", return_value=None
+ )
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {"EXTRA_ACCESS_QUERY_EXCLUSIONS": {"charts": _chart_exclusion_hook()}},
+ )
+
+ captured: dict[str, Any] = {}
+ query: MagicMock = MagicMock()
+
+ def _capture(clause: object) -> MagicMock:
+ captured["clause"] = clause
+ return query
+
+ query.filter.side_effect = _capture # noqa: E501
+
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+ filt.apply(query, None)
+
+ query.filter.assert_called_once()
+ assert "NOT (EXISTS" in _compiled(captured["clause"])
+
+
+def test_chart_filter_exclusion_skipped_for_embedded_guest(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """A guest token already scopes its holder; narrowing further is wrong."""
+ from flask import current_app
+
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=False
+ )
+ mocker.patch(
+ "superset.charts.filters.guest_embedded_dashboard_filter",
+ return_value=Dashboard.id.in_([1, 2]),
+ )
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {"EXTRA_ACCESS_QUERY_EXCLUSIONS": {"charts": _chart_exclusion_hook()}},
+ )
+
+ captured: dict[str, Any] = {}
+ query: MagicMock = MagicMock()
+
+ def _capture(clause: object) -> MagicMock:
+ captured["clause"] = clause
+ return query
+
+ query.filter.side_effect = _capture # noqa: E501
+
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+ filt.apply(query, None)
+
+ query.filter.assert_called_once()
+ compiled = _compiled(captured["clause"])
+ assert "IN (1, 2)" in compiled
+ assert "NOT (EXISTS" not in compiled
diff --git a/tests/unit_tests/commands/report/base_test.py b/tests/unit_tests/commands/report/base_test.py
index e54fb812f86..deb6bb5027a 100644
--- a/tests/unit_tests/commands/report/base_test.py
+++ b/tests/unit_tests/commands/report/base_test.py
@@ -416,3 +416,242 @@ def test_validate_alert_query_rejects_unauthorized_tables(
assert len(exceptions) == 1
assert isinstance(exceptions[0], AlertQueryDataAccessValidationError)
+
+
+# -- Report target exclusions --
+#
+# The related-field filter narrows the picker, but a list filter is not an
+# authorisation boundary: ``_check_object_access`` resolves the posted id
+# through ``find_by_id``, which applies the ordinary access filter. These go
+# through that method rather than the exclusion helper in isolation, so a
+# regression in the key mapping or the raise is caught.
+
+
+def test_check_object_access_refuses_an_excluded_report_target(
+ mocker: MockerFixture,
+) -> None:
+ """An excluded id posted directly must be refused, not merely hidden."""
+ from superset.commands.report.exceptions import ReportScheduleForbiddenError
+
+ excluded = mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=True,
+ )
+ dao = mocker.MagicMock()
+ dao.find_by_id.return_value = mocker.MagicMock()
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {}
+
+ with pytest.raises(ReportScheduleForbiddenError):
+ cmd._check_object_access(
+ 42,
+ kind="dashboard",
+ dao=dao,
+ not_found_exc=ValueError,
+ exceptions=[],
+ )
+
+ # The key is derived from the kind, which is what a rename would break.
+ assert excluded.call_args.args[0] == "report_target_dashboards"
+
+
+def test_check_object_access_allows_a_target_that_is_not_excluded(
+ mocker: MockerFixture,
+) -> None:
+ """With no exclusion configured the ordinary access check decides."""
+ mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=False,
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.report.base.security_manager"
+ ).raise_for_access
+ dao = mocker.MagicMock()
+ resolved = mocker.MagicMock()
+ dao.find_by_id.return_value = resolved
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {}
+ cmd._check_object_access(
+ 42,
+ kind="chart",
+ dao=dao,
+ not_found_exc=ValueError,
+ exceptions=[],
+ )
+
+ raise_for_access.assert_called_once_with(chart=resolved)
+ assert cmd._properties["chart"] is resolved
+
+
+def test_an_update_omitting_the_target_still_checks_the_stored_one(
+ mocker: MockerFixture,
+) -> None:
+ """Reactivating a report must not resume a schedule on a withheld target.
+
+ ``PUT /api/v1/report/<id>`` with only ``{"active": true}`` reaches the
+ branch that takes the target from the stored model, so without this the
+ schedule resumes while resubmitting that same target is refused.
+ """
+ from superset.commands.report.exceptions import ReportScheduleForbiddenError
+
+ excluded = mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=True,
+ )
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {"active": True}
+ # The class declares no ``_model``; production reads it via ``getattr``.
+ cmd._model = mocker.MagicMock(chart_id=None, dashboard_id=9)
+
+ with pytest.raises(ReportScheduleForbiddenError):
+ cmd.validate_chart_dashboard([], update=True)
+
+ assert excluded.call_args.args == ("report_target_dashboards", 9)
+
+
+def test_an_update_omitting_the_target_passes_when_not_withheld(
+ mocker: MockerFixture,
+) -> None:
+ """With no exclusion configured the stored target is left alone."""
+ mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=False,
+ )
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {"active": True}
+ # The class declares no ``_model``; production reads it via ``getattr``.
+ cmd._model = mocker.MagicMock(chart_id=None, dashboard_id=9)
+
+ exceptions: list[Any] = []
+ cmd.validate_chart_dashboard(exceptions, update=True)
+ assert exceptions == []
+
+
+def test_deactivating_a_report_on_a_withheld_target_is_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """Pausing must never be refused.
+
+ Refusing it traps the owner: they can neither pause the report nor stop the
+ scheduler selecting it, so the withheld asset keeps being delivered. That is
+ strictly worse than not checking at all.
+ """
+ excluded = mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=True,
+ )
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {"active": False}
+ cmd._model = mocker.MagicMock(chart_id=None, dashboard_id=9, active=True)
+
+ exceptions: list[Any] = []
+ cmd.validate_chart_dashboard(exceptions, update=True)
+
+ assert exceptions == []
+ excluded.assert_not_called()
+
+
+def test_pausing_from_the_edit_modal_is_allowed(mocker: MockerFixture) -> None:
+ """A pause that resends the target must be honoured too.
+
+ ``AlertReportModal`` spreads the whole form into its PUT and always sets
+ ``chart``/``dashboard`` from the loaded report, so a pause from there arrives
+ as ``{"active": false, "dashboard": 9}`` and takes the *submitted*-target
+ branch -- not the stored-target branch the list page's toggle reaches, which
+ sends ``{"active": false}`` alone. An exemption only the stored path could
+ see left the modal returning 403, so the owner could not pause at all.
+ """
+ excluded = mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=True,
+ )
+ mocker.patch("superset.commands.report.base.security_manager")
+ dao = mocker.patch("superset.commands.report.base.DashboardDAO")
+ dao.find_by_id.return_value = mocker.MagicMock()
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {"active": False, "dashboard": 9}
+ cmd._model = mocker.MagicMock(chart_id=None, dashboard_id=9, active=True)
+
+ exceptions: list[Any] = []
+ cmd.validate_chart_dashboard(exceptions, update=True)
+
+ assert exceptions == []
+ excluded.assert_not_called()
+
+
+def test_activating_from_the_edit_modal_is_still_refused(
+ mocker: MockerFixture,
+) -> None:
+ """The pause exemption must not become a way in.
+
+ Same payload shape as the test above with the switch the other way: a
+ schedule that will run is checked, so nothing is gained by routing the
+ request through the modal.
+ """
+ from superset.commands.report.exceptions import ReportScheduleForbiddenError
+
+ mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=True,
+ )
+ mocker.patch("superset.commands.report.base.security_manager")
+ dao = mocker.patch("superset.commands.report.base.DashboardDAO")
+ dao.find_by_id.return_value = mocker.MagicMock()
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {"active": True, "dashboard": 9}
+ cmd._model = mocker.MagicMock(chart_id=None, dashboard_id=9, active=False)
+
+ with pytest.raises(ReportScheduleForbiddenError):
+ cmd.validate_chart_dashboard([], update=True)
+
+
+def test_an_already_paused_report_may_be_edited(mocker: MockerFixture) -> None:
+ """The check guards delivery, not editing."""
+ excluded = mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ return_value=True,
+ )
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {"crontab": "0 9 * * *"}
+ cmd._model = mocker.MagicMock(chart_id=None, dashboard_id=9, active=False)
+
+ exceptions: list[Any] = []
+ cmd.validate_chart_dashboard(exceptions, update=True)
+
+ assert exceptions == []
+ excluded.assert_not_called()
+
+
+def test_the_stored_target_asks_both_exclusion_keys(
+ mocker: MockerFixture,
+) -> None:
+ """A submitted target reaches the ordinary key through ``find_by_id``.
+
+ So a stored target asking only ``report_target_*`` would accept what
+ resubmitting the same id refuses.
+ """
+ from superset.commands.report.exceptions import ReportScheduleForbiddenError
+
+ # Withheld by the ordinary key only.
+ def only_ordinary(key: str, _id: int) -> bool:
+ return key == "dashboards"
+
+ mocker.patch(
+ "superset.commands.report.base.is_excluded_by_extra_access",
+ side_effect=only_ordinary,
+ )
+
+ cmd = BaseReportScheduleCommand()
+ cmd._properties = {"active": True}
+ cmd._model = mocker.MagicMock(chart_id=None, dashboard_id=9, active=False)
+
+ with pytest.raises(ReportScheduleForbiddenError):
+ cmd.validate_chart_dashboard([], update=True)
diff --git a/tests/unit_tests/dashboards/semantic_view_access_test.py b/tests/unit_tests/dashboards/semantic_view_access_test.py
index 7c1a19b032e..6c70f8e5ec8 100644
--- a/tests/unit_tests/dashboards/semantic_view_access_test.py
+++ b/tests/unit_tests/dashboards/semantic_view_access_test.py
@@ -36,10 +36,11 @@ from __future__ import annotations
import uuid as uuid_lib
from contextlib import contextmanager, ExitStack
from types import SimpleNamespace
-from typing import cast, Iterator, TYPE_CHECKING
+from typing import Any, cast, Iterator, TYPE_CHECKING
from unittest.mock import MagicMock, patch
import pytest
+from pytest_mock import MockerFixture
from sqlalchemy import event
from sqlalchemy.engine import Connection, Engine, ExecutionContext
from sqlalchemy.orm.session import Session
@@ -1137,3 +1138,141 @@ def test_guest_allowlist_allows_table_member_chart(
),
):
sm.raise_for_access(chart=access_fixtures.table_slice)
+
+
+# -- Exclusion-hook placement --
+#
+# Placement is the behaviour: the exclusion must survive the ``is_admin()``
+# short-circuit and must not reach an embedded guest. Neither is visible to a
+# test that calls ``apply_extra_access_exclusions`` directly.
+
+
+def _dashboard_exclusion_hook(excluded_id: int) -> Any:
+ from sqlalchemy import select
+
+ from superset.models.dashboard import Dashboard
+
+ return lambda _uid: select(Dashboard.id).where(Dashboard.id == excluded_id)
+
+
+def test_dashboard_list_filter_exclusion_survives_admin(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """An admin is exactly the principal the role paths stop narrowing."""
+ from flask import current_app
+
+ from superset.dashboards.filters import DashboardAccessFilter
+ from superset.extensions import db, security_manager
+ from superset.models.dashboard import Dashboard
+
+ withheld = Dashboard(dashboard_title="withheld from admin", published=True)
+ kept = Dashboard(dashboard_title="kept for admin", published=True)
+ db.session.add_all([withheld, kept])
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+ mocker.patch(
+ "superset.dashboards.filters.guest_embedded_dashboard_filter",
+ return_value=None,
+ )
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {
+ "EXTRA_ACCESS_QUERY_EXCLUSIONS": {
+ "dashboards": _dashboard_exclusion_hook(withheld.id)
+ }
+ },
+ )
+
+ flt = DashboardAccessFilter.__new__(DashboardAccessFilter)
+ titles = {
+ dash.dashboard_title for dash in flt.apply(db.session.query(Dashboard), None)
+ }
+
+ assert "withheld from admin" not in titles
+ assert "kept for admin" in titles
+
+
+def test_dashboard_list_filter_exclusion_skipped_for_embedded_guest(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """A guest token already scopes its holder to its own dashboards."""
+ from flask import current_app
+
+ from superset.dashboards.filters import DashboardAccessFilter
+ from superset.extensions import db, security_manager
+ from superset.models.dashboard import Dashboard
+
+ in_token = Dashboard(dashboard_title="in the guest token", published=True)
+ db.session.add(in_token)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "is_admin", return_value=False)
+ mocker.patch(
+ "superset.dashboards.filters.guest_embedded_dashboard_filter",
+ return_value=Dashboard.id.in_([in_token.id]),
+ )
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ # The hook would withhold the very dashboard the token grants.
+ mocker.patch.dict(
+ current_app.config,
+ {
+ "EXTRA_ACCESS_QUERY_EXCLUSIONS": {
+ "dashboards": _dashboard_exclusion_hook(in_token.id)
+ }
+ },
+ )
+
+ flt = DashboardAccessFilter.__new__(DashboardAccessFilter)
+ titles = {
+ dash.dashboard_title for dash in flt.apply(db.session.query(Dashboard), None)
+ }
+
+ assert "in the guest token" in titles
+
+
+def test_dashboard_lookup_by_uuid_honours_the_exclusion(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """Withheld means withheld whichever identifier names the dashboard.
+
+ The uuid branch of ``get_by_id_or_slug`` skips ``base_filter``, so without
+ an explicit check the same dashboard was absent by id and readable by uuid.
+ """
+ from flask import current_app
+
+ from superset.commands.dashboard.exceptions import DashboardNotFoundError
+ from superset.daos.dashboard import DashboardDAO
+ from superset.extensions import db, security_manager
+ from superset.models.dashboard import Dashboard
+
+ withheld = Dashboard(dashboard_title="withheld by uuid too", published=True)
+ db.session.add(withheld)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+ mocker.patch(
+ "superset.dashboards.filters.guest_embedded_dashboard_filter",
+ return_value=None,
+ )
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {
+ "EXTRA_ACCESS_QUERY_EXCLUSIONS": {
+ "dashboards": _dashboard_exclusion_hook(withheld.id)
+ }
+ },
+ )
+
+ # By id: withheld through the base filter.
+ with pytest.raises(DashboardNotFoundError):
+ DashboardDAO.get_by_id_or_slug(str(withheld.id))
+
+ # By uuid: the branch that skips the base filter must agree.
+ with pytest.raises(DashboardNotFoundError):
+ DashboardDAO.get_by_id_or_slug(str(withheld.uuid))
diff --git a/tests/unit_tests/reports/filters_test.py b/tests/unit_tests/reports/filters_test.py
index 3f3367b6959..eb42da0c93e 100644
--- a/tests/unit_tests/reports/filters_test.py
+++ b/tests/unit_tests/reports/filters_test.py
@@ -172,3 +172,56 @@ def test_report_execution_log_filter_non_admin_scoped_to_log_fk(
(filter_expr,) = query.filter.call_args[0]
assert filter_expr.left.table is ReportExecutionLog.__table__
assert filter_expr.left.name == "report_schedule_id"
+
+
+# -- Report target related-field filters --
+#
+# A scheduled report renders as the deployment's configured executor rather
+# than as its author, so the assets selectable as a report target can be a
+# subset of the assets the author may read. These filters express that by
+# reading their own exclusion key, and inherit every other rule.
+
+
+@patch("superset.reports.filters.apply_extra_access_exclusions")
+def test_report_target_chart_filter_uses_its_own_exclusion_key(
+ mock_exclusions: MagicMock,
+) -> None:
+ from superset.charts.filters import ChartFilter
+ from superset.reports.filters import ReportTargetChartFilter
+
+ query = MagicMock()
+ # Deliberately a different object: returning ``query`` unchanged would let a
+ # dropped ``query = ...`` assignment pass this test while excluded targets
+ # reappeared in the picker.
+ narrowed = MagicMock(name="narrowed_query")
+ mock_exclusions.return_value = narrowed
+
+ with patch.object(ChartFilter, "apply", return_value="delegated") as mock_parent:
+ f = ReportTargetChartFilter("id", MagicMock())
+ assert f.apply(query, None) == "delegated"
+
+ # The ordinary "charts" key must not be what narrows a report target list.
+ assert mock_exclusions.call_args.args[1] == "report_target_charts"
+ # ...and the parent's rules run on the *narrowed* query, not the original.
+ mock_parent.assert_called_once_with(narrowed, None)
+
+
+@patch("superset.reports.filters.apply_extra_access_exclusions")
+def test_report_target_dashboard_filter_uses_its_own_exclusion_key(
+ mock_exclusions: MagicMock,
+) -> None:
+ from superset.dashboards.filters import DashboardAccessFilter
+ from superset.reports.filters import ReportTargetDashboardFilter
+
+ query = MagicMock()
+ narrowed = MagicMock(name="narrowed_query")
+ mock_exclusions.return_value = narrowed
+
+ with patch.object(
+ DashboardAccessFilter, "apply", return_value="delegated"
+ ) as mock_parent:
+ f = ReportTargetDashboardFilter("id", MagicMock())
+ assert f.apply(query, None) == "delegated"
+
+ assert mock_exclusions.call_args.args[1] == "report_target_dashboards"
+ mock_parent.assert_called_once_with(narrowed, None)
diff --git a/tests/unit_tests/utils/filters_test.py b/tests/unit_tests/utils/filters_test.py
index e3b13293e5f..47dfa5ba748 100644
--- a/tests/unit_tests/utils/filters_test.py
+++ b/tests/unit_tests/utils/filters_test.py
@@ -16,13 +16,16 @@
# under the License.
from types import SimpleNamespace
+from typing import Any
+import pytest
from pytest_mock import MockerFixture
-from sqlalchemy import create_engine
+from sqlalchemy import create_engine, null as sa_null
from sqlalchemy.sql.elements import ColumnElement, False_
from superset.extensions import security_manager
from superset.utils.filters import (
+ apply_extra_access_exclusions,
get_dataset_access_filters,
guest_embedded_dashboard_filter,
)
@@ -248,3 +251,258 @@ def test_guest_embedded_dashboard_filter_ignores_slug_in_mixed_token(
assert "embedded_dashboards" in compiled
assert "dashboards.id IN" in compiled
assert "dashboards.slug" not in compiled
+
+
+def test_apply_extra_access_exclusions_noop_without_hook(app_context: None) -> None:
+ """The default build configures no hook, so the query is untouched."""
+ from superset import db
+ from superset.models.slice import Slice
+
+ query = db.session.query(Slice)
+ assert apply_extra_access_exclusions(query, "charts", Slice.id) is query
+
+
+def test_apply_extra_access_exclusions_consults_the_hook_anonymously(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """An anonymous caller is asked about too, not waved through.
+
+ A deployment serving published dashboards through the Public role has
+ anonymous callers. Skipping a withholding rule for them would reveal rather
+ than hide, so the hook is consulted with ``None``.
+ """
+ from flask import current_app
+
+ from superset import db
+ from superset.models.slice import Slice
+
+ withheld = Slice(
+ slice_name="hidden from anon", datasource_id=1, datasource_type="table"
+ )
+ kept = Slice(slice_name="public chart", datasource_id=1, datasource_type="table")
+ db.session.add_all([withheld, kept])
+ db.session.flush()
+
+ seen: list[Any] = []
+
+ def hook(user_id: Any) -> Any:
+ seen.append(user_id)
+ return db.session.query(Slice.id).filter(Slice.id == withheld.id)
+
+ mocker.patch("superset.utils.core.get_user_id", return_value=None)
+ mocker.patch.dict(
+ current_app.config,
+ {"EXTRA_ACCESS_QUERY_EXCLUSIONS": {"charts": hook}},
+ )
+
+ query = apply_extra_access_exclusions(db.session.query(Slice), "charts", Slice.id)
+ returned = {slc.id for slc in query.all()}
+
+ assert seen == [None], "the hook must be told there is no user, not skipped"
+ assert withheld.id not in returned
+ assert kept.id in returned
+
+
+def test_apply_extra_access_exclusions_withholds_ids(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """A configured hook withholds the IDs it names.
+
+ Executed rather than string-matched: the point is that a hook returning a
+ ``.subquery()`` -- the shape ``EXTRA_ACCESS_QUERY_FILTERS`` already
+ requires -- composes into ``NOT IN`` and produces valid SQL.
+ """
+ from flask import current_app
+
+ from superset import db
+ from superset.models.slice import Slice
+
+ excluded = Slice(slice_name="excluded", datasource_id=1, datasource_type="table")
+ kept = Slice(slice_name="kept", datasource_id=1, datasource_type="table")
+ db.session.add_all([excluded, kept])
+ db.session.flush()
+
+ def hook(_user_id: int) -> Any:
+ return db.session.query(Slice.id).filter(Slice.id == excluded.id).subquery()
+
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {"EXTRA_ACCESS_QUERY_EXCLUSIONS": {"charts": hook}},
+ )
+
+ query = apply_extra_access_exclusions(db.session.query(Slice), "charts", Slice.id)
+ returned = {slc.id for slc in query.all()}
+
+ assert excluded.id not in returned
+ assert kept.id in returned
+
+
+def test_exclusions_tolerate_a_null_id(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """A NULL in the hook's subquery must withhold nothing, not everything.
+
+ ``NOT IN (..., NULL)`` is NULL for every row, which would empty the whole
+ list -- for admins too. ``NOT EXISTS`` simply does not match it.
+ """
+ from flask import current_app
+
+ from superset import db
+ from superset.models.slice import Slice
+
+ kept = Slice(slice_name="kept past null", datasource_id=1, datasource_type="table")
+ db.session.add(kept)
+ db.session.flush()
+
+ def hook(_user_id: int) -> Any:
+ return db.session.query(sa_null().label("id")).subquery()
+
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {"EXTRA_ACCESS_QUERY_EXCLUSIONS": {"charts": hook}},
+ )
+
+ query = apply_extra_access_exclusions(db.session.query(Slice), "charts", Slice.id)
+ assert kept.id in {slc.id for slc in query.all()}
+
+
+def test_is_excluded_by_extra_access_matches_the_filter(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """The single-id check is what a command uses as its boundary.
+
+ A list filter is not an authorisation boundary: a command resolving an id
+ directly has to ask the same question, or the asset is only hidden from a
+ picker rather than withheld.
+ """
+ from flask import current_app
+
+ from superset import db
+ from superset.models.slice import Slice
+ from superset.utils.filters import is_excluded_by_extra_access
+
+ excluded = Slice(slice_name="withheld", datasource_id=1, datasource_type="table")
+ allowed = Slice(slice_name="fine", datasource_id=1, datasource_type="table")
+ db.session.add_all([excluded, allowed])
+ db.session.flush()
+
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {
+ "EXTRA_ACCESS_QUERY_EXCLUSIONS": {
+ "charts": lambda _uid: db.session.query(Slice.id)
+ .filter(Slice.id == excluded.id)
+ .subquery()
+ }
+ },
+ )
+
+ assert is_excluded_by_extra_access("charts", excluded.id) is True
+ assert is_excluded_by_extra_access("charts", allowed.id) is False
+ # No hook for this kind: nothing is withheld.
+ assert is_excluded_by_extra_access("dashboards", excluded.id) is False
+
+
+@pytest.mark.parametrize("shape", ["query", "select", "subquery"])
+def test_exclusions_accept_every_natural_return_shape(
+ shape: str,
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """A hook may return a Query, a select() or an explicit .subquery().
+
+ All three are natural to write, and the sibling
+ ``EXTRA_ACCESS_QUERY_FILTERS`` hooks are handed straight to ``.in_()``,
+ which accepts any of them. Reading ``.c`` without normalising raised
+ ``AttributeError`` on a ``Query`` -- breaking every chart and dashboard
+ list, for every user.
+ """
+ from flask import current_app
+ from sqlalchemy import select as sa_select
+
+ from superset import db
+ from superset.models.slice import Slice
+
+ withheld = Slice(
+ slice_name=f"shape {shape}", datasource_id=1, datasource_type="table"
+ )
+ kept = Slice(slice_name=f"kept {shape}", datasource_id=1, datasource_type="table")
+ db.session.add_all([withheld, kept])
+ db.session.flush()
+
+ def hook(_user_id: int) -> Any:
+ if shape == "query":
+ return db.session.query(Slice.id).filter(Slice.id == withheld.id)
+ if shape == "select":
+ return sa_select(Slice.id).where(Slice.id == withheld.id)
+ return sa_select(Slice.id).where(Slice.id == withheld.id).subquery()
+
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {"EXTRA_ACCESS_QUERY_EXCLUSIONS": {"charts": hook}},
+ )
+
+ query = apply_extra_access_exclusions(db.session.query(Slice), "charts", Slice.id)
+ returned = {slc.id for slc in query.all()}
+ assert withheld.id not in returned
+ assert kept.id in returned
+
+
+def test_exclusions_never_apply_to_an_embedded_guest(
+ app_context: None,
+ mocker: MockerFixture,
+) -> None:
+ """A guest token scopes its holder; exclusions must not narrow it further.
+
+ The list filters return before reaching an exclusion, but a caller
+ resolving a single id has no such branch ahead of it -- an embedded
+ dashboard is fetched by uuid, so missing this 404s every embed.
+ """
+ from flask import current_app
+
+ from superset import db, security_manager
+ from superset.models.slice import Slice
+ from superset.utils.filters import is_excluded_by_extra_access
+
+ withheld = Slice(
+ slice_name="withheld from users", datasource_id=1, datasource_type="table"
+ )
+ db.session.add(withheld)
+ db.session.flush()
+
+ def hook(_user_id: Any) -> Any:
+ return db.session.query(Slice.id).filter(Slice.id == withheld.id).subquery()
+
+ mocker.patch("superset.utils.core.get_user_id", return_value=1)
+ mocker.patch.dict(
+ current_app.config,
+ {"EXTRA_ACCESS_QUERY_EXCLUSIONS": {"charts": hook}},
+ )
+
+ # An ordinary caller: withheld.
+ mocker.patch.object(security_manager, "is_guest_user", return_value=False)
+ assert is_excluded_by_extra_access("charts", withheld.id) is True
+ assert (
+ apply_extra_access_exclusions(
+ db.session.query(Slice), "charts", Slice.id
+ ).count()
+ == 0
+ )
+
+ # A guest: exempt on both paths.
+ mocker.patch.object(security_manager, "is_guest_user", return_value=True)
+ assert is_excluded_by_extra_access("charts", withheld.id) is False
+ assert (
+ apply_extra_access_exclusions(
+ db.session.query(Slice), "charts", Slice.id
+ ).count()
+ >= 1
+ )