diff --git a/superset/commands/tag/delete.py b/superset/commands/tag/delete.py index dfc5686d497d..6b8693a3dd3f 100644 --- a/superset/commands/tag/delete.py +++ b/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) diff --git a/superset/commands/tag/exceptions.py b/superset/commands/tag/exceptions.py index 6778c8e221a1..6fd838c32a3b 100644 --- a/superset/commands/tag/exceptions.py +++ b/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.") diff --git a/superset/daos/tag.py b/superset/daos/tag.py index 943132071b3b..139be7633475 100644 --- a/superset/daos/tag.py +++ b/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 diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index 59aa2185c78c..17425bee014c 100644 --- a/superset/datasets/schemas.py +++ b/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) diff --git a/superset/reports/filters.py b/superset/reports/filters.py index ad08a4a234a1..af4781a4e482 100644 --- a/superset/reports/filters.py +++ b/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" diff --git a/superset/reports/logs/api.py b/superset/reports/logs/api.py index 9a2cea964e3b..96673d1ef3db 100644 --- a/superset/reports/logs/api.py +++ b/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", diff --git a/superset/security/api.py b/superset/security/api.py index bb8995d46657..63291e14d540 100644 --- a/superset/security/api.py +++ b/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/ flow; exposing it in API responses (and thus diff --git a/superset/security/manager.py b/superset/security/manager.py index 79c262dfddca..21f0729d4101 100644 --- a/superset/security/manager.py +++ b/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", diff --git a/superset/tags/api.py b/superset/tags/api.py index 38e76f4fedbc..09a640d52e47 100644 --- a/superset/tags/api.py +++ b/superset/tags/api.py @@ -486,6 +486,64 @@ def delete_object( ) return self.response_422(message=str(ex)) + @expose("/", 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 diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 56f3e38efd78..cfd32fe9deda 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -3402,6 +3402,73 @@ def test_get_drill_info_admin_user(self): self.items_to_delete = [dataset] + def test_get_drill_info_does_not_expose_user_emails(self): + """ + Dataset API: drill_info must not leak creator/modifier email addresses. + + The nested user schema exposes first/last name only; email is PII and + is not part of the endpoint's select_columns contract. + """ + self.login(ADMIN_USERNAME) + dataset = self.insert_dataset( + table_name="test_drill_dataset_no_email", + editor_user_ids=[], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.get_assert_metric(uri, "get_drill_info") + assert rv.status_code == 200 + + result = json.loads(rv.data.decode("utf-8"))["result"] + for user_field in ("created_by", "changed_by"): + assert "email" not in (result.get(user_field) or {}) + + self.items_to_delete = [dataset] + + def test_get_drill_info_does_not_expose_editor_emails(self): + """ + Dataset API: drill_info must not leak an editor's email address + through the ``editors`` list. + + User-subject synchronization stores a user's email in the Subject's + ``secondary_label`` field, and ``editors`` nests Subjects directly, + so email must be excluded the same way it is for created_by/changed_by. + """ + self.login(ADMIN_USERNAME) + gamma_user = self.get_user(GAMMA_USERNAME) + dataset = self.insert_dataset( + table_name="test_drill_dataset_no_editor_email", + editor_user_ids=[gamma_user.id], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.get_assert_metric(uri, "get_drill_info") + assert rv.status_code == 200 + + result = json.loads(rv.data.decode("utf-8"))["result"] + editors = result.get("editors") or [] + assert len(editors) == 1 + assert "secondary_label" not in editors[0] + assert gamma_user.email not in json.dumps(editors) + + self.items_to_delete = [dataset] + def test_get_drill_info_admin_user_dataset_not_found(self): """ Dataset API: Test drill_info endpoint returns 404 for non-existent dataset. diff --git a/tests/integration_tests/tags/api_tests.py b/tests/integration_tests/tags/api_tests.py index e34fb4e622bb..15d0ec2ec0bc 100644 --- a/tests/integration_tests/tags/api_tests.py +++ b/tests/integration_tests/tags/api_tests.py @@ -570,6 +570,49 @@ def test_delete_tags(self): tags = db.session.query(Tag).filter(Tag.name.in_(example_tag_names)) assert tags.count() == 0 + @pytest.mark.usefixtures("create_tags") + def test_delete_tag_by_pk(self): + """ + Tag API: the single-object ``DELETE /api/v1/tag/`` route must + share the same ownership/system-tag validation as bulk_delete + (DeleteTagsCommand), not the FAB-generated model delete. + """ + tag = db.session.query(Tag).filter(Tag.name == "example_tag_1").one() + system_tag = Tag(name="system:pk_delete_example", type=TagType.type) + db.session.add(system_tag) + db.session.commit() + + try: + # a non-admin, non-creator user may not delete via the pk route + self.login(GAMMA_USERNAME) + rv = self.client.delete(f"api/v1/tag/{tag.id}", follow_redirects=True) + assert rv.status_code == 422 + assert db.session.query(Tag).filter(Tag.id == tag.id).count() == 1 + + # system-generated tags are refused outright, even for an admin + self.logout() + self.login(ADMIN_USERNAME) + rv = self.client.delete( + f"api/v1/tag/{system_tag.id}", follow_redirects=True + ) + assert rv.status_code == 422 + assert db.session.query(Tag).filter(Tag.id == system_tag.id).count() == 1 + + # an admin may delete a custom tag via the pk route + rv = self.client.delete(f"api/v1/tag/{tag.id}", follow_redirects=True) + assert rv.status_code == 200 + assert db.session.query(Tag).filter(Tag.id == tag.id).count() == 0 + finally: + db.session.query(Tag).filter(Tag.id == system_tag.id).delete() + db.session.commit() + + def test_delete_tag_by_pk_not_found(self): + self.login(ADMIN_USERNAME) + existing_ids = [tag_id for (tag_id,) in db.session.query(Tag.id).all()] + non_existent_id = max(existing_ids, default=0) + 1 + rv = self.client.delete(f"api/v1/tag/{non_existent_id}", follow_redirects=True) + assert rv.status_code == 404 + @pytest.mark.usefixtures("create_tags") def test_delete_favorite_tag(self): self.login(ADMIN_USERNAME) diff --git a/tests/integration_tests/tags/commands_tests.py b/tests/integration_tests/tags/commands_tests.py index 055c51f09adc..f83e4f06c395 100644 --- a/tests/integration_tests/tags/commands_tests.py +++ b/tests/integration_tests/tags/commands_tests.py @@ -33,13 +33,14 @@ from superset.commands.importers.exceptions import IncorrectVersionError # noqa: F401 from superset.commands.tag.create import CreateCustomTagCommand from superset.commands.tag.delete import DeleteTaggedObjectCommand, DeleteTagsCommand +from superset.commands.tag.exceptions import TagInvalidError from superset.connectors.sqla.models import SqlaTable # noqa: F401 from superset.models.core import Database # noqa: F401 from superset.models.dashboard import Dashboard from superset.models.slice import Slice # noqa: F401 from superset.tags.models import ObjectType, Tag, TaggedObject, TagType from tests.integration_tests.base_tests import SupersetTestCase -from tests.integration_tests.constants import ADMIN_USERNAME +from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME from tests.integration_tests.fixtures.importexport import ( chart_config, # noqa: F401 dashboard_config, # noqa: F401 @@ -127,6 +128,53 @@ def test_delete_tags_command(self): tags = db.session.query(Tag).filter(Tag.name.in_(example_tags)) assert tags.count() == 0 + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + @pytest.mark.usefixtures("with_tagging_system_feature") + def test_delete_tags_command_requires_authorization(self): + """ + Regression test: DeleteTagsCommand used to check only that each named + tag existed, letting any Gamma user bulk-delete tags (and every + association they carry) they neither created nor own. It must now + require admin-or-creator, and must refuse system-generated tags + outright for everyone. + """ + example_dashboard = ( + db.session.query(Dashboard) + .filter_by(dashboard_title="World Bank's Data") + .one() + ) + self.login(ADMIN_USERNAME) + example_tags = {"delete tag authz example"} + CreateCustomTagCommand( + ObjectType.dashboard.value, example_dashboard.id, example_tags + ).run() + + system_tag = Tag(name="type:delete_tag_authz", type=TagType.type) + db.session.add(system_tag) + db.session.commit() + + try: + # a non-admin who is not the tag's creator may not delete it + self.logout() + self.login(GAMMA_USERNAME) + with pytest.raises(TagInvalidError): + DeleteTagsCommand(example_tags).run() + assert db.session.query(Tag).filter(Tag.name.in_(example_tags)).count() == 1 + + # system-generated tags are refused outright, even for an admin + self.logout() + self.login(ADMIN_USERNAME) + with pytest.raises(TagInvalidError): + DeleteTagsCommand([system_tag.name]).run() + assert db.session.query(Tag).filter_by(name=system_tag.name).count() == 1 + finally: + # cleanup + self.logout() + self.login(ADMIN_USERNAME) + DeleteTagsCommand(example_tags).run() + db.session.query(Tag).filter_by(name=system_tag.name).delete() + db.session.commit() + # test delete tagged objects command class TestDeleteTaggedObjectCommand(SupersetTestCase): diff --git a/tests/unit_tests/datasets/schema_tests.py b/tests/unit_tests/datasets/schema_tests.py index dcea6e795a63..5ca7e99dd29b 100644 --- a/tests/unit_tests/datasets/schema_tests.py +++ b/tests/unit_tests/datasets/schema_tests.py @@ -48,6 +48,51 @@ def test_validate_python_date_format_raises(payload) -> None: validate_python_date_format(payload) +def test_drill_info_user_schema_does_not_expose_email() -> None: + """ + Regression test: the drill_info-local ``UserSchema`` used to declare an + ``email`` field. ``DatasetDrillInfoSchema`` nests it (unfiltered by + ``select_columns``) for both ``created_by`` and ``changed_by``, so any + user with dataset-read access -- or, via the dashboard fallback, + embedded guests -- received maintainer email addresses. Dataset-read + access does not imply entitlement to other users' PII. + """ + from superset.datasets.schemas import UserSchema + + class _FakeUser: + first_name = "Jane" + last_name = "Doe" + email = "jane.doe@example.com" + + dumped = UserSchema().dump(_FakeUser()) + assert "email" not in dumped + assert dumped == {"first_name": "Jane", "last_name": "Doe"} + + +def test_drill_info_editor_schema_does_not_expose_secondary_label() -> None: + """ + Regression test: ``DatasetDrillInfoSchema.editors`` used to nest the + shared ``SubjectResponseSchema``, which includes ``secondary_label``. + For a user-backed Subject, user-subject synchronization + (``superset.subjects.sync.sync_user_subject``) stores that user's email + in ``secondary_label``, so nesting it unfiltered leaked the same + maintainer PII that dropping ``email`` from ``UserSchema`` was meant to + close, just under a different field name. + """ + from superset.datasets.schemas import DrillInfoEditorSchema + + class _FakeEditorSubject: + id = 1 + label = "Jane Doe" + secondary_label = "jane.doe@example.com" + img = "avatar.png" + type = 1 + + dumped = DrillInfoEditorSchema().dump(_FakeEditorSubject()) + assert "secondary_label" not in dumped + assert dumped == {"id": 1, "label": "Jane Doe", "img": "avatar.png", "type": 1} + + def test_dataset_post_schema_has_all_put_scalar_fields() -> None: """ Every scalar model field accepted by DatasetPutSchema should also be accepted diff --git a/tests/unit_tests/reports/filters_test.py b/tests/unit_tests/reports/filters_test.py index bdcb2c181fb1..3f3367b69598 100644 --- a/tests/unit_tests/reports/filters_test.py +++ b/tests/unit_tests/reports/filters_test.py @@ -106,3 +106,69 @@ def test_report_schedule_all_text_filter_coerces_non_string( mock_report_schedule.sql, ): column.ilike.assert_called_once_with(expected, escape="\\") + + +@patch("superset.reports.filters.security_manager", new_callable=MagicMock) +def test_report_execution_log_filter_admin_sees_all(mock_sm: MagicMock) -> None: + """ + Regression test: ``ReportExecutionLogRestApi`` had no base filter at all, + so any role with generic ReportSchedule read could iterate every + schedule's logs by pk. An admin (can_access_all_queries) must still + see everything unfiltered. + """ + from superset.reports.filters import ReportExecutionLogFilter + + mock_sm.can_access_all_queries.return_value = True + query = MagicMock() + f = ReportExecutionLogFilter("id", MagicMock()) + result = f.apply(query, None) + assert result is query + query.filter.assert_not_called() + + +@patch("superset.reports.filters.security_manager", new_callable=MagicMock) +def test_report_execution_log_filter_stock_alpha_is_scoped( + mock_sm: MagicMock, +) -> None: + """ + Regression test: the unrestricted bypass used to key off + ``can_access_all_datasources``, which is also granted to stock Alpha + (see ``SupersetSecurityManager.ALPHA_ONLY_PERMISSIONS``), letting a + non-editor Alpha user read every other schedule's evaluated alert + values and database errors. The bypass must require + ``can_access_all_queries`` (admin-only) instead, matching the + equivalent per-execution SQL Lab query history filter + (``superset.queries.filters.QueryFilter``). + """ + from superset.reports.filters import ReportExecutionLogFilter + + mock_sm.can_access_all_datasources.return_value = True + mock_sm.can_access_all_queries.return_value = False + query = MagicMock() + f = ReportExecutionLogFilter("id", MagicMock()) + f.apply(query, None) + query.filter.assert_called_once() + + +@patch("superset.reports.filters.security_manager", new_callable=MagicMock) +@patch("superset.reports.filters.db") +def test_report_execution_log_filter_non_admin_scoped_to_log_fk( + mock_db: MagicMock, mock_sm: MagicMock +) -> None: + """ + A non-admin must be scoped by ``ReportExecutionLog.report_schedule_id`` + directly (not by an unjoined filter on ``ReportSchedule.id``, which would + pass for any log row as long as the caller edits at least one schedule). + """ + from superset.reports.filters import ReportExecutionLogFilter + from superset.reports.models import ReportExecutionLog + + mock_sm.can_access_all_queries.return_value = False + query = MagicMock() + f = ReportExecutionLogFilter("id", MagicMock()) + f.apply(query, None) + + query.filter.assert_called_once() + (filter_expr,) = query.filter.call_args[0] + assert filter_expr.left.table is ReportExecutionLog.__table__ + assert filter_expr.left.name == "report_schedule_id" diff --git a/tests/unit_tests/reports/logs_api_test.py b/tests/unit_tests/reports/logs_api_test.py new file mode 100644 index 000000000000..e081f54e58ff --- /dev/null +++ b/tests/unit_tests/reports/logs_api_test.py @@ -0,0 +1,37 @@ +# 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. +from superset.reports.filters import ReportExecutionLogFilter +from superset.reports.logs.api import ReportExecutionLogRestApi + + +def test_execution_log_api_has_ownership_base_filter() -> None: + """ + Regression test: the schedule id in the ``/log/`` and ``/log/`` + routes is caller-controlled (folded into the rison filters), so an + editor-scoped base filter must be applied to both the list and item + routes -- otherwise any role with generic ReportSchedule read can iterate + every schedule's logs. + """ + assert ReportExecutionLogRestApi.base_filters, ( + "ReportExecutionLogRestApi must apply an ownership base filter; " + "without one, logs are readable across schedules regardless of " + "ownership" + ) + assert any( + filter_class is ReportExecutionLogFilter + for _, filter_class, _ in ReportExecutionLogRestApi.base_filters + ) diff --git a/tests/unit_tests/security/api_test.py b/tests/unit_tests/security/api_test.py index f2962a82a66c..6d87a8fbf75f 100644 --- a/tests/unit_tests/security/api_test.py +++ b/tests/unit_tests/security/api_test.py @@ -19,8 +19,10 @@ import pytest from marshmallow import ValidationError +from superset.constants import RouteMethod from superset.extensions import csrf -from superset.security.api import RlsRuleSchema +from superset.security.api import RlsRuleSchema, UserRegistrationsRestAPI +from superset.security.manager import SupersetSecurityManager @pytest.mark.parametrize( @@ -172,3 +174,32 @@ def test_rls_rule_schema_rejects_falsy_dataset(dataset: Any) -> None: with pytest.raises(ValidationError) as exc_info: RlsRuleSchema().load({"dataset": dataset, "clause": "tenant_id = 1"}) assert "dataset" in exc_info.value.messages + + +def test_user_registrations_rest_api_is_admin_only() -> None: + """ + The API is documented Admin-only, but the admin gate is membership in + ADMIN_ONLY_VIEW_MENUS keyed by the FAB-derived view-menu name (the class + name). If the entry is missing, ``superset init`` grants the API's + permissions to stock Gamma and Alpha via ``_is_gamma_pvm``, exposing + pending registrants' PII and registration deletion. + """ + assert "UserRegistrationsRestAPI" in SupersetSecurityManager.ADMIN_ONLY_VIEW_MENUS + + +def test_user_registrations_rest_api_excludes_create_and_update() -> None: + """ + The FAB default POST/PUT handlers must not exist on this API, so a + mis-granted role cannot create or silently alter a pending registration. + DELETE stays registered: the User Registrations admin page deletes + pending registrations through it, and the route is still fully gated + Admin-only (see test_user_registrations_rest_api_is_admin_only). + """ + assert UserRegistrationsRestAPI.include_route_methods == { + RouteMethod.GET, + RouteMethod.GET_LIST, + RouteMethod.INFO, + RouteMethod.DELETE, + } + assert "post" not in UserRegistrationsRestAPI.include_route_methods + assert "put" not in UserRegistrationsRestAPI.include_route_methods diff --git a/tests/unit_tests/tags/api_test.py b/tests/unit_tests/tags/api_test.py new file mode 100644 index 000000000000..9cd4f77a4006 --- /dev/null +++ b/tests/unit_tests/tags/api_test.py @@ -0,0 +1,164 @@ +# 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. +from typing import Any +from unittest.mock import MagicMock + +from pytest_mock import MockerFixture + + +def test_delete_tag_by_pk_routes_through_delete_tags_command( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + Regression test: ``DELETE /api/v1/tag/`` used to fall through to the + FAB-generated single-object delete route, which deletes the row directly + via the datamodel and never runs ``DeleteTagsCommand.validate`` (the + admin-or-creator check, and the system-tag refusal). The pk route must + be overridden to share that same validation instead of duplicating it. + """ + mock_tag = MagicMock(id=1) + mock_tag.name = "example_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.return_value = None + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 200 + mock_command.assert_called_once_with(["example_tag"]) + + +def test_delete_tag_by_pk_not_found( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """DELETE /api/v1/tag/ returns 404 when the tag does not exist.""" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=None) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + + response = client.delete("/api/v1/tag/999") + + assert response.status_code == 404 + mock_command.assert_not_called() + + +def test_delete_tag_by_pk_denied_surfaces_as_422( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + A non-admin, non-creator caller (or an attempt to delete a system tag) + is rejected by DeleteTagsCommand.validate with TagInvalidError; the pk + route must surface that as 422, not silently succeed. + """ + from superset.commands.tag.exceptions import TagInvalidError + + mock_tag = MagicMock(id=1) + mock_tag.name = "someone_elses_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagInvalidError() + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 422 + + +def test_delete_tag_by_pk_denied_with_populated_exceptions_surfaces_as_422( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + Regression test for an AttributeError that only surfaced once + DeleteTagsCommand.validate's TagInvalidError actually carried the + exceptions it composites at runtime (a plain TagDeleteFailedError, + not a ValidationError). The pk route calls + ``ex.normalized_messages()`` on the TagInvalidError it catches, which + previously crashed with a 500 instead of returning 422 because + TagDeleteFailedError has no ``normalized_messages()`` method. A + TagInvalidError() with no exceptions (as in the test above) does not + exercise that aggregation loop, so this test populates it the way the + real command does. + """ + from superset.commands.tag.exceptions import ( + TagDeleteForbiddenValidationError, + TagInvalidError, + ) + + mock_tag = MagicMock(id=1) + mock_tag.name = "system:some_type" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagInvalidError( + exceptions=[ + TagDeleteForbiddenValidationError( + "Tag system:some_type is a system tag and cannot be deleted" + ) + ] + ) + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 422 + assert "tags" in response.json["message"] + + +def test_delete_tag_by_pk_race_with_bulk_delete_surfaces_as_404( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + If the tag is deleted concurrently between the ``find_by_id`` lookup and + ``DeleteTagsCommand.validate``'s own lookup, the command raises + TagNotFoundError; the pk route must surface that as 404. + """ + from superset.commands.tag.exceptions import TagNotFoundError + + mock_tag = MagicMock(id=1) + mock_tag.name = "example_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagNotFoundError("example_tag") + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 404 + + +def test_delete_tag_by_pk_delete_failed_surfaces_as_422( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """DeleteTagsCommand raising TagDeleteFailedError surfaces as 422.""" + from superset.commands.tag.exceptions import TagDeleteFailedError + + mock_tag = MagicMock(id=1) + mock_tag.name = "example_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagDeleteFailedError() + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 422 diff --git a/tests/unit_tests/tags/commands/delete_test.py b/tests/unit_tests/tags/commands/delete_test.py new file mode 100644 index 000000000000..7bd8888457aa --- /dev/null +++ b/tests/unit_tests/tags/commands/delete_test.py @@ -0,0 +1,198 @@ +# 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. +from unittest.mock import PropertyMock + +import pytest +from pytest_mock import MockerFixture +from sqlalchemy.orm.session import Session + +from superset import db + + +@pytest.fixture +def session_with_tags(session: Session): + from flask_appbuilder.security.sqla.models import User + + from superset.tags.models import Tag, TagType + + engine = session.get_bind() + Tag.metadata.create_all(engine) # pylint: disable=no-member + User.metadata.create_all(engine) # pylint: disable=no-member + + owner = User( + first_name="owner", last_name="owner", username="owner", email="owner@x.com" + ) + other = User( + first_name="other", last_name="other", username="other", email="other@x.com" + ) + session.add(owner) + session.add(other) + session.flush() + + owned_tag = Tag( + name="owned_tag", + type=TagType.custom, + created_by_fk=owner.id, + created_by=owner, + ) + system_tag = Tag(name="type:some_type", type=TagType.type) + + session.add(owned_tag) + session.add(system_tag) + session.commit() + return session + + +def test_delete_tags_command_admin_can_delete_any_custom_tag( + session_with_tags: Session, mocker: MockerFixture +): + from superset.commands.tag.delete import DeleteTagsCommand + from superset.tags.models import Tag + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + DeleteTagsCommand(["owned_tag"]).run() + + assert db.session.query(Tag).filter_by(name="owned_tag").one_or_none() is None + + +def test_delete_tags_command_creator_can_delete_own_tag( + session_with_tags: Session, mocker: MockerFixture +): + from flask_appbuilder.security.sqla.models import User + + from superset.commands.tag.delete import DeleteTagsCommand + from superset.tags.models import Tag + + owner = db.session.query(User).filter_by(username="owner").one() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + mocker.patch( + "superset.security.SupersetSecurityManager.current_user", + new_callable=PropertyMock, + return_value=owner, + ) + + DeleteTagsCommand(["owned_tag"]).run() + + assert db.session.query(Tag).filter_by(name="owned_tag").one_or_none() is None + + +def test_delete_tags_command_non_creator_non_admin_denied( + session_with_tags: Session, mocker: MockerFixture +): + """Regression test: DeleteTagsCommand.validate previously checked only + that each named tag existed, letting any user with can_delete on Tag + (default Gamma) bulk-delete tags -- and every association they carry -- + they neither created nor own. + """ + from flask_appbuilder.security.sqla.models import User + + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + from superset.tags.models import Tag + + other = db.session.query(User).filter_by(username="other").one() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + mocker.patch( + "superset.security.SupersetSecurityManager.current_user", + new_callable=PropertyMock, + return_value=other, + ) + + with pytest.raises(TagInvalidError): + DeleteTagsCommand(["owned_tag"]).run() + + # the tag must survive the denied deletion + assert db.session.query(Tag).filter_by(name="owned_tag").one_or_none() is not None + + +def test_delete_tags_command_refuses_system_tag_even_for_admin( + session_with_tags: Session, mocker: MockerFixture +): + """System-generated tags (type:*, editor:*, favorited_by:*) are + maintained by Superset itself and must not be deletable through the + bulk route, regardless of the caller's role. + """ + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + from superset.tags.models import Tag + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + with pytest.raises(TagInvalidError): + DeleteTagsCommand(["type:some_type"]).run() + + assert ( + db.session.query(Tag).filter_by(name="type:some_type").one_or_none() is not None + ) + + +def test_delete_tags_command_refused_tag_reports_normalized_messages( + session_with_tags: Session, mocker: MockerFixture +): + """Regression test: DeleteTagsCommand.validate previously appended a + plain CommandException (TagDeleteFailedError) into the TagInvalidError + it raises, which crashed with AttributeError as soon as anything called + .normalized_messages() on that TagInvalidError (as the single-object + DELETE /api/v1/tag/ route does). Every exception composited into + TagInvalidError must be a ValidationError so normalized_messages() can + aggregate it. + """ + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + with pytest.raises(TagInvalidError) as excinfo: + DeleteTagsCommand(["type:some_type"]).run() + + messages = excinfo.value.normalized_messages() + assert "tags" in messages + assert "system tag" in messages["tags"][0] + + +def test_delete_tags_command_not_found_reports_normalized_messages( + session_with_tags: Session, mocker: MockerFixture +): + """A nonexistent tag name is also composited into TagInvalidError; it + must likewise support normalized_messages() without raising. + """ + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + with pytest.raises(TagInvalidError) as excinfo: + DeleteTagsCommand(["does_not_exist"]).run() + + messages = excinfo.value.normalized_messages() + assert "tags" in messages + assert "not found" in messages["tags"][0] diff --git a/tests/unit_tests/tags/commands/update_test.py b/tests/unit_tests/tags/commands/update_test.py index edd41991fce1..24a8bd43369c 100644 --- a/tests/unit_tests/tags/commands/update_test.py +++ b/tests/unit_tests/tags/commands/update_test.py @@ -264,3 +264,155 @@ def test_update_command_remove_all_tagged_objects( assert ( len(db.session.query(TaggedObject).filter_by(tag_id=updated_tag.id).all()) == 0 ) + + +def test_update_command_skips_removal_of_inaccessible_objects( + session_with_data: Session, mocker: MockerFixture +): + """Associations on objects the user cannot modify must survive an update. + + Regression test: the deletion branch of ``create_tag_relationship`` + removed every association absent from the submitted set with no + per-object check, so a low-privilege user could strip a tag from + objects they are not allowed to modify (or from every object, by + sending an empty ``objects_to_tag``). + """ + from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand + from superset.commands.tag.update import UpdateTagCommand + from superset.daos.tag import TagDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + from superset.tags.models import ObjectType, TaggedObject + + dashboard = db.session.query(Dashboard).first() + chart = db.session.query(Slice).first() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + mocker.patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart) + mocker.patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ) + + # An admin tags both a dashboard and a chart + CreateCustomTagWithRelationshipsCommand( + data={ + "name": "test_tag", + "objects_to_tag": [ + (ObjectType.dashboard, dashboard.id), + (ObjectType.chart, chart.id), + ], + } + ).run() + + tag = TagDAO.find_by_name("test_tag") + assert len(tag.objects) == 2 + + # A non-admin who may modify the chart but not the dashboard submits + # only the chart: the dashboard association must not be deleted. + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + + def can_modify(model): + return isinstance(model, Slice) + + mocker.patch( + "superset.commands.tag.update.current_user_can_modify_object", + side_effect=can_modify, + ) + mocker.patch( + "superset.daos.tag.current_user_can_modify_object", + side_effect=can_modify, + ) + + UpdateTagCommand( + tag.id, + { + "name": "test_tag", + "description": "test_description", + "objects_to_tag": [(ObjectType.chart, chart.id)], + }, + ).run() + + remaining = { + (obj.object_type, obj.object_id) + for obj in db.session.query(TaggedObject).filter_by(tag_id=tag.id).all() + } + assert (ObjectType.dashboard, dashboard.id) in remaining + assert (ObjectType.chart, chart.id) in remaining + + +def test_update_command_empty_objects_to_tag_only_removes_accessible( + session_with_data: Session, mocker: MockerFixture +): + """An empty/omitted objects_to_tag must not mass-delete every association. + + A PUT with no objects_to_tag used to be treated as "delete every current + association" with zero per-object check. Now each deletion is + access-checked, so objects the caller cannot modify keep their + association. + """ + from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand + from superset.commands.tag.update import UpdateTagCommand + from superset.daos.tag import TagDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + from superset.tags.models import ObjectType, TaggedObject + + dashboard = db.session.query(Dashboard).first() + chart = db.session.query(Slice).first() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + mocker.patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart) + mocker.patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ) + + CreateCustomTagWithRelationshipsCommand( + data={ + "name": "test_tag", + "objects_to_tag": [ + (ObjectType.dashboard, dashboard.id), + (ObjectType.chart, chart.id), + ], + } + ).run() + + tag = TagDAO.find_by_name("test_tag") + assert len(tag.objects) == 2 + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + + def can_modify(model): + return isinstance(model, Slice) + + mocker.patch( + "superset.commands.tag.update.current_user_can_modify_object", + side_effect=can_modify, + ) + mocker.patch( + "superset.daos.tag.current_user_can_modify_object", + side_effect=can_modify, + ) + + UpdateTagCommand( + tag.id, + { + "name": "test_tag", + "description": "test_description", + "objects_to_tag": [], + }, + ).run() + + remaining = { + (obj.object_type, obj.object_id) + for obj in db.session.query(TaggedObject).filter_by(tag_id=tag.id).all() + } + assert (ObjectType.dashboard, dashboard.id) in remaining + assert (ObjectType.chart, chart.id) not in remaining