kokhlo commented on issue #44433:
URL: https://github.com/apache/superset/issues/44433#issuecomment-5736986741
Verified against current master — the authorization check the issue asks for
is already in effect, and the proposed decorator would actually break the
endpoint.
**How `@protect()` resolves the permission.** Per flask_appbuilder's
`security/decorators.py`, when no `@permission_name` is applied, the wrapper
sets `f._permission_name = f.__name__`, and at request time the checked
permission is `PERMISSION_PREFIX + f._permission_name` — i.e. for this route it
is exactly `can_estimate_query_cost` on the `SqlLabRestApi` class
(`class_permission_name = "SQLLab"`). The docstring of `protect()` states this
directly: "By default the permission's name is the methods name."
**Why the class-level map doesn't change that.** `superset/sqllab/api.py`
sets `method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP`
(`superset/constants.py:136`), but that dict has no `estimate_query_cost` key,
so both the runtime lookup in the decorator and the `get_method_permission()`
call that assembles `base_permissions` fall back to the method name. That's
also why `can_estimate_query_cost` shows up in the API's `base_permissions` —
without it, the decorator's `permission_str not in self.base_permissions` guard
would 403 for everyone, which would have been noticed immediately.
**The permission exists and is wired into the sql_lab role.**
`superset/security/manager.py:1906` declares `("can_estimate_query_cost",
"SQLLab")` in `SQLLAB_ONLY_PERMISSIONS`, so the pairing the endpoint checks is
materialized and granted via the sql_lab role — the same default-name mechanism
`execute_sql_query` relies on.
**On the two proposed fixes:**
- `@permission_name("can_estimate_query_cost")` would prepend the prefix a
second time (`PERMISSION_PREFIX` is `"can_"`), producing
`can_can_estimate_query_cost`, which exists in no role — the endpoint would 403
for all users including admins.
- `@permission_name("read")` (as `format_sql` does) would *weaken* the check
to the generic `can_read` on `SQLLab` rather than enforce the dedicated
permission.
So there's no missing authorization here — `@protect()` alone already
enforces exactly the dedicated `can_estimate_query_cost` capability. IMO no
change is needed; closing as not-a-bug seems right unless someone can
demonstrate a role that lacks the permission but still gets a 200 from `POST
/api/v1/sqllab/estimate/`.
--
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]
