sadpandajoe commented on code in PR #43390:
URL: https://github.com/apache/superset/pull/43390#discussion_r3833203015
##########
superset/commands/tag/delete.py:
##########
@@ -134,10 +134,34 @@ 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))
+ exceptions: list[TagNotFoundError | TagDeleteFailedError] = []
+ 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(TagNotFoundError(tag_name))
+ 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(
+ TagDeleteFailedError(
+ 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 (
Review Comment:
This ownership check only protects `DeleteTagsCommand`, while `TagRestApi`
still exposes the inherited `DELETE /api/v1/tag/<pk>` route. A user with Tag
delete permission can remove another user's custom tag—or a system tag—and its
organization-wide associations without reaching this validation. Should that
route use the same authorization or be disabled?
##########
superset/reports/filters.py:
##########
@@ -43,6 +43,26 @@ 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.
+ """
+
+ def apply(self, query: Query, value: Any) -> Query:
+ if security_manager.can_access_all_datasources():
Review Comment:
`can_access_all_datasources()` is also true for stock Alpha, so a non-editor
Alpha user gets an unscoped execution-log query and can read another schedule's
alert values or database errors by ID. Should this bypass be limited to admins
(or a reporting-specific privilege) instead?
##########
superset/datasets/schemas.py:
##########
@@ -480,9 +480,13 @@ class DatasetColumnDrillInfoSchema(Schema):
class UserSchema(Schema):
+ # Deliberately excludes ``email``: drill_info is reachable by any user
Review Comment:
Removing `UserSchema.email` does not remove editor emails from this
response: `editors` uses `SubjectResponseSchema.secondary_label`, and
user-subject synchronization stores the email in that field. A non-guest
dataset reader can still receive every editor's email. Should `drill_info` omit
or redact that field for editor subjects as well?
##########
superset/security/api.py:
##########
@@ -423,6 +424,11 @@ class UserRegistrationsRestAPI(BaseSupersetModelRestApi):
resource_name = "security/user_registrations"
datamodel = SQLAInterface(RegisterUser)
allow_browser_login = True
+ # This API is read-only by design: restricting the exposed routes keeps
+ # the FAB default POST/PUT/DELETE handlers from ever being registered,
+ # so a mis-granted role cannot create, alter, or silently cancel a
+ # pending registration.
+ include_route_methods = {RouteMethod.GET, RouteMethod.GET_LIST,
RouteMethod.INFO}
Review Comment:
The User Registrations page still renders its Delete action and calls
`DELETE /api/v1/security/user_registrations/<id>`. Omitting
`RouteMethod.DELETE` makes that existing Admin workflow return 405. Should the
UI action be removed as part of this read-only change, or should the Admin-only
delete route remain available?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
