fix: add missing ownership checks to tag, report-log, and dataset-schema endpoints

superset/commands/tag/delete.py

@@ -18,19 +18,22 @@
 from functools import partial
 from typing import Any
 
+from marshmallow import ValidationError
+
 from superset import security_manager
 from superset.commands.base import BaseCommand
+from superset.commands.exceptions import TagNotFoundValidationError
 from superset.commands.tag.exceptions import (
     TagDeleteFailedError,
+    TagDeleteForbiddenValidationError,
     TaggedObjectDeleteFailedError,
     TaggedObjectNotFoundError,
     TagInvalidError,
-    TagNotFoundError,
 )
 from superset.commands.tag.utils import to_object_model, to_object_type
 from superset.daos.tag import TagDAO
 from superset.exceptions import SupersetSecurityException
-from superset.tags.models import ObjectType
+from superset.tags.models import ObjectType, TagType
 from superset.utils.decorators import on_error, transaction
 from superset.views.base import DeleteMixin
 
@@ -134,10 +137,42 @@ def run(self) -> None:
         TagDAO.delete_tags(self._tags)
 
     def validate(self) -> None:
-        exceptions = []
-        # Validate tag exists
-        for tag in self._tags:
-            if not TagDAO.find_by_name(tag):
-                exceptions.append(TagNotFoundError(tag))
+        # Every item appended here must be a ValidationError (or subclass),
+        # since TagInvalidError.normalized_messages() calls
+        # .normalized_messages() on each one to build the aggregated 422
+        # response.
+        exceptions: list[ValidationError] = []
+        for tag_name in self._tags:
+            tag_name = tag_name.strip()
+            tag = TagDAO.find_by_name(tag_name)
+            # Validate tag exists
+            if not tag:
+                exceptions.append(
+                    TagNotFoundValidationError(f"Tag with name {tag_name} not found.")
+                )
+                continue
+            # System-generated tags (type:*, editor:*, favorited_by:*) are
+            # maintained by Superset itself and must not be deletable through
+            # the bulk route.
+            if tag.type is not None and tag.type != TagType.custom:
+                exceptions.append(
+                    TagDeleteForbiddenValidationError(
+                        f"Tag {tag_name} is a system tag and cannot be deleted"
+                    )
+                )
+                continue
+            # Deleting a tag cascades removal of all of its associations
+            # org-wide, so existence is not enough: require the user to be an
+            # admin or the tag's creator (the single-association route
+            # enforces per-object access in DeleteTaggedObjectCommand).
+            if not (
+                security_manager.is_admin()
+                or (tag.created_by and tag.created_by == security_manager.current_user)
+            ):
+                exceptions.append(
+                    TagDeleteForbiddenValidationError(
+                        f"Access denied to tag {tag_name}"
+                    )
+                )
         if exceptions:
             raise TagInvalidError(exceptions=exceptions)

superset/commands/tag/exceptions.py

@@ -17,6 +17,7 @@
 from typing import Optional
 
 from flask_babel import lazy_gettext as _
+from marshmallow import ValidationError
 
 from superset.commands.exceptions import (
     CommandException,
@@ -44,6 +45,18 @@ class TagDeleteFailedError(DeleteFailedError):
     message = _("Tag could not be deleted.")
 
 
+class TagDeleteForbiddenValidationError(ValidationError):
+    """A tag exists but may not be deleted (a system-generated tag, or the
+    caller lacks ownership/admin rights). Unlike ``TagDeleteFailedError``,
+    this is a ``ValidationError`` so it can be composited into a
+    ``TagInvalidError`` alongside other validation failures and still
+    support ``CommandInvalidError.normalized_messages()``.
+    """
+
+    def __init__(self, message: str) -> None:
+        super().__init__(message, field_name="tags")
+
+
 class TaggedObjectDeleteFailedError(DeleteFailedError):
     message = _("Tagged Object could not be deleted.")
 

superset/daos/tag.py

@@ -21,7 +21,11 @@
 from sqlalchemy.exc import NoResultFound
 
 from superset.commands.tag.exceptions import TagNotFoundError
-from superset.commands.tag.utils import to_object_type
+from superset.commands.tag.utils import (
+    current_user_can_modify_object,
+    to_object_model,
+    to_object_type,
+)
 from superset.daos.base import BaseDAO
 from superset.daos.chart import ChartDAO
 from superset.daos.dashboard import DashboardDAO
@@ -372,6 +376,18 @@ def create_tag_relationship(
         if not bulk_create:
             # delete relationships that aren't retained from single tag create
             for object_type, object_id in tagged_objects_to_delete:
+                # Only remove associations from objects the current user may
+                # modify, mirroring the per-object check applied to additions.
+                # Look the object up bypassing the access base filter so an
+                # inaccessible object reaches the check instead of resolving
+                # to None and having its association deleted unchecked.
+                model = to_object_model(
+                    object_type,  # type: ignore
+                    object_id,
+                    skip_base_filter=True,
+                )
+                if model and not current_user_can_modify_object(model):
+                    continue
                 # delete objects that were removed
                 TagDAO.delete_tagged_object(
                     object_type,  # type: ignore

superset/datasets/schemas.py

@@ -34,7 +34,6 @@
 from superset.connectors.sqla.models import SqlaTable
 from superset.exceptions import SupersetMarshmallowValidationError
 from superset.models.sql_types import parse_currency_string
-from superset.subjects.schemas import SubjectResponseSchema
 from superset.utils import json
 
 get_delete_ids_schema = {
@@ -480,16 +479,32 @@ class DatasetColumnDrillInfoSchema(Schema):
 
 
 class UserSchema(Schema):
+    # Deliberately excludes ``email``: drill_info is reachable by any user
+    # with read access to the dataset (and, via the dashboard fallback, by
+    # embedded guests), so exposing maintainer emails here would leak user
+    # PII across an access boundary. Mirrors the dashboard/RLS user schemas,
+    # which expose names only.
     first_name = fields.String()
     last_name = fields.String()
-    email = fields.String()
+
+
+class DrillInfoEditorSchema(Schema):
+    # Deliberately excludes ``secondary_label``: for a user-backed Subject,
+    # user-subject synchronization (superset.subjects.sync.sync_user_subject)
+    # stores that user's email in this field, so including it here would
+    # leak the same maintainer PII that ``UserSchema`` above excludes
+    # ``email`` to avoid, just through a different field name.
+    id = fields.Int()
+    label = fields.String()
+    img = fields.String()
+    type = fields.Integer()
 
 
 class DatasetDrillInfoSchema(Schema):
     id = fields.Integer()
     columns = fields.List(fields.Nested(DatasetColumnDrillInfoSchema))
     table_name = fields.String()
-    editors = fields.List(fields.Nested(SubjectResponseSchema))
+    editors = fields.List(fields.Nested(DrillInfoEditorSchema))
     created_by = fields.Nested(UserSchema)
     created_on_humanized = fields.String()
     changed_by = fields.Nested(UserSchema)

superset/reports/filters.py

@@ -22,7 +22,7 @@
 
 from superset import db, security_manager
 from superset.daos.base import _escape_like
-from superset.reports.models import ReportSchedule
+from superset.reports.models import ReportExecutionLog, ReportSchedule
 from superset.subjects.filters import subject_relation_exists_for_current_user
 from superset.views.base import BaseFilter
 
@@ -43,6 +43,34 @@ def _apply_editors(self, query: Query) -> Query:
         return query.filter(ReportSchedule.id.in_(editor_ids_query))
 
 
+class ReportExecutionLogFilter(BaseFilter):  # pylint: disable=too-few-public-methods
+    """
+    Scope execution logs to report schedules the user can edit, mirroring
+    ``ReportScheduleFilter`` on the schedule API. Logs carry evaluated alert
+    values and database error messages, so they must not be readable across
+    ownership boundaries via an attacker-chosen schedule id.
+
+    The unrestricted bypass is gated by ``can_access_all_queries`` rather
+    than ``can_access_all_datasources``: the latter is also granted to
+    stock Alpha, which would let a non-editor Alpha user read every other
+    schedule's evaluated values and database errors. ``can_access_all_queries``
+    is the admin-only permission this codebase already uses to gate the
+    equivalent per-execution data on SQL Lab query history
+    (see ``superset.queries.filters.QueryFilter``).
+    """
+
+    def apply(self, query: Query, value: Any) -> Query:
+        if security_manager.can_access_all_queries():
+            return query
+
+        from superset.subjects.models import report_schedule_editors
+
+        editor_ids_query = db.session.query(
+            report_schedule_editors.c.report_schedule_id
+        ).filter(subject_relation_exists_for_current_user(report_schedule_editors))
+        return query.filter(ReportExecutionLog.report_schedule_id.in_(editor_ids_query))
+
+
 class ReportScheduleAllTextFilter(BaseFilter):  # pylint: disable=too-few-public-methods
     name = _("All Text")
     arg_name = "report_all_text"

superset/reports/logs/api.py

@@ -31,6 +31,7 @@
 
 from superset import is_feature_enabled
 from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.reports.filters import ReportExecutionLogFilter
 from superset.reports.logs.schemas import openapi_spec_methods_override
 from superset.reports.models import ReportExecutionLog
 from superset.views.base_api import BaseSupersetModelRestApi
@@ -53,6 +54,13 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]:
     class_permission_name = "ReportSchedule"
     resource_name = "report"
     allow_browser_login = True
+    # Ownership scoping. Without a base filter the only scoping on these
+    # routes is the caller-controlled ``pk`` path parameter (folded into the
+    # rison filters below), so any role with ReportSchedule read could
+    # iterate every schedule's logs deployment-wide. Mirrors the
+    # ReportScheduleFilter applied by the schedule API
+    # (superset/reports/api.py).
+    base_filters = [["id", ReportExecutionLogFilter, lambda: []]]
 
     show_columns = [
         "id",

superset/security/api.py

@@ -40,6 +40,7 @@
     EmbeddedDashboardNotFoundError,
 )
 from superset.commands.exceptions import ForbiddenError
+from superset.constants import RouteMethod
 from superset.exceptions import SupersetGenericErrorException
 from superset.extensions import db, event_logger
 from superset.security.guest_token import (
@@ -423,6 +424,20 @@ class UserRegistrationsRestAPI(BaseSupersetModelRestApi):
     resource_name = "security/user_registrations"
     datamodel = SQLAInterface(RegisterUser)
     allow_browser_login = True
+    # POST/PUT are intentionally excluded: restricting the exposed routes
+    # keeps the FAB default create/update handlers from ever being
+    # registered, so a mis-granted role cannot silently alter a pending
+    # registration. DELETE is kept: the User Registrations admin page
+    # deletes pending registrations through this route, and the class is
+    # gated Admin-only via ADMIN_ONLY_VIEW_MENUS (keyed on the view-menu
+    # name, i.e. every permission on this class, not just specific ones),
+    # so exposing it does not grant non-Admin roles anything.
+    include_route_methods = {
+        RouteMethod.GET,
+        RouteMethod.GET_LIST,
+        RouteMethod.INFO,
+        RouteMethod.DELETE,
+    }
     # NOTE: registration_hash is intentionally excluded from both list_columns
     # and search_columns. It is a bearer token for the
     # /register/activation/<hash> flow; exposing it in API responses (and thus

superset/security/manager.py

@@ -1432,6 +1432,10 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
         "Security",
         "SQL Lab",
         "User Registrations",
+        # REST counterpart of the FAB "User Registrations" views. FAB derives
+        # the view-menu name from the class name; without this entry
+        # _is_gamma_pvm grants its permissions to stock Gamma and Alpha.
+        "UserRegistrationsRestAPI",
         "User's Statistics",
         # Guarding all AB_ADD_SECURITY_API = True REST APIs
         "RoleRestAPI",

superset/tags/api.py

@@ -486,6 +486,64 @@ def delete_object(
             )
             return self.response_422(message=str(ex))
 
+    @expose("/<pk>", methods=("DELETE",))
+    @protect()
+    @safe
+    @statsd_metrics
+    @event_logger.log_this_with_context(
+        action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete",
+        log_to_statsd=False,
+    )
+    def delete(self, pk: int) -> Response:
+        """Deletes a Tag
+        ---
+        delete:
+          description: >-
+            Delete a Tag by id. This will remove all tagged objects with
+            this tag.
+          parameters:
+          - in: path
+            schema:
+              type: integer
+            name: pk
+          responses:
+            200:
+              description: Tag deleted
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      message:
+                        type: string
+            401:
+              $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/403'
+            404:
+              $ref: '#/components/responses/404'
+            422:
+              $ref: '#/components/responses/422'
+            500:
+              $ref: '#/components/responses/500'
+        """
+        # Overrides the FAB-generated single-object delete route (which would
+        # otherwise call self.datamodel.delete directly, bypassing
+        # DeleteTagsCommand's ownership and system-tag checks) so both the
+        # single-object and bulk-delete routes share the same validation.
+        tag = TagDAO.find_by_id(pk)
+        if not tag:
+            return self.response_404()
+        try:
+            DeleteTagsCommand([tag.name]).run()
+            return self.response(200, message="OK")
+        except TagNotFoundError:
+            return self.response_404()
+        except TagInvalidError as ex:
+            return self.response_422(message=ex.normalized_messages())
+        except TagDeleteFailedError as ex:
+            return self.response_422(message=str(ex))
+
     @expose("/", methods=("DELETE",))
     @protect()
     @safe