geido commented on code in PR #43458:
URL: https://github.com/apache/superset/pull/43458#discussion_r3934318828
##########
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:
Review Comment:
Confirmed, and reverted to `message=str(ex)` in 69782fde99.
You're right on both counts. `TemporaryCacheAccessDeniedError.message` is
`_("You don't have permission to modify the value.")` — it never carried a
dataset name, so the hardcoded string fixed no disclosure. And `check_access`
in `commands/explore/form_data/utils.py:43-45` collapses both
`ChartAccessDeniedError` and `DatasetAccessDeniedError` into it, so labelling
every one of those a *datasource* denial was wrong. `str(ex)` restores the
`lazy_gettext`.
Two new tests in `tests/unit_tests/explore/api_test.py` cover it, one
asserting the body echoes the exception's own message and one that no
`datasource` wording appears in it.
Separately, while verifying this I found something worth its own issue:
`GetExploreCommand.run` calls `GetFormDataCommand(...).run()`
(`commands/explore/get.py:78-81`) *before* `security_manager.raise_for_access`
(`:126-129`), and `GetFormDataCommand.run` calls `check_access` itself. So on
any `?form_data_key=` URL — the normal dashboard→Explore link — a dataset
denial always surfaces as `TemporaryCacheAccessDeniedError` and never as
`SupersetSecurityException`. The new request-access UI therefore cannot fire on
that path at all, before or after this PR. Making it work needs `check_access`
to preserve which denial occurred so the API can emit the right `error_type`,
which is a change to the shared form_data contract rather than something to
paper over here.
##########
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:
Right — the flag was inert, so it is gone in 69782fde99 along with the rest
of that handler.
`ErrorMessageWithStackTrace` dispatches on `error_type`, and this branch set
neither `error_type` nor `level`, so `DatasourceSecurityAccessErrorMessage` was
unreachable and the `extra` payload was something the frontend never read.
Rather than inventing an `error_type` here, the handler is back to the plain
`str(ex)` 403 — see the reply on the thread above for why this path cannot
reach the request-access UI at all without a change to how `check_access`
reports the denial kind.
`test_temporary_cache_access_denied_omits_inert_access_flag` in
`tests/unit_tests/explore/api_test.py` locks in that the body carries no
`extra`.
--
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]
