geido commented on code in PR #43458:
URL: https://github.com/apache/superset/pull/43458#discussion_r3934320393
##########
superset/explore/api.py:
##########
@@ -135,7 +135,11 @@ def get(self) -> Response:
return self.response(404, message=str(ex))
except WrongEndpointError as ex:
return self.response(302, redirect=ex.redirect)
- except TemporaryCacheAccessDeniedError as ex:
- return self.response(403, message=str(ex))
+ except TemporaryCacheAccessDeniedError:
+ return self.response(
+ 403,
+ message="You do not have permission to access this datasource",
+ extra={"is_access_denial": True},
+ )
Review Comment:
Good catch — fixed in 69782fde99, though by removing the hardcoded string
rather than wrapping it.
The handler is back to `self.response(403, message=str(ex))`.
`TemporaryCacheAccessDeniedError.message` is already `lazy_gettext`-wrapped in
`superset/commands/temporary_cache/exceptions.py:44-45`, so echoing the
exception restores translation without a second `_()` call. @msyavuz flagged
the same loss of i18n from the other direction on the thread above, plus the
fact that the string described the wrong resource — `check_access` raises this
for chart denials too.
##########
superset/security/manager.py:
##########
@@ -226,7 +228,7 @@ def _render_permission_instructions_link(
for token, value in (
("datasource_id", datasource_id),
- ("datasource_name", datasource_name),
+ ("datasource_name", ""), # removed to prevent information disclosure
Review Comment:
Agreed — changed in 69782fde99 to leave the placeholder literal and log a
warning.
`_render_permission_instructions_link` no longer substitutes
`{datasource_name}` at all. A deployment configured with
`https://acme/request/{datasource_name}` now renders that URL verbatim and gets
a WARNING naming `PERMISSION_INSTRUCTIONS_LINK`, instead of a plausible-looking
`https://acme/request/` nobody notices. The three supported placeholders still
blank when unsupplied; this is the one deliberate exception, and the docstring
says so.
Added an UPDATING.md entry too, since this is a real behaviour change —
`origin/master` does substitute `{datasource_name}` today, so deployments
templating it are affected.
Covered by `test_retired_datasource_name_placeholder_is_left_literal` and
`..._warns_the_operator`, with
`test_link_without_retired_placeholder_does_not_warn` guarding against warning
on configs that do not use it.
One thing I did not change, flagging it in case you feel differently: the
warning fires per denial rather than once per process, so a dashboard of N
charts on a denied dataset logs N identical lines. I kept it at the point of
use because that is where the config is actually read and the volume tracks
genuine denials, but a startup-time config validation would be the tidier home
for it.
--
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]
