gabotorresruiz commented on code in PR #44987:
URL: https://github.com/apache/superset/pull/44987#discussion_r4189871623
##########
superset/security/manager.py:
##########
@@ -5533,6 +5534,24 @@ def get_user_roles(self, user: Optional[User] = None) ->
list[Role]:
return [self.get_public_role()] if public_role else []
return super().get_user_roles(user)
+ def raise_for_unsupported_guest_rls(
+ self, datasource: "BaseDatasource | Explorable"
+ ) -> None:
+ """Deny semantic reads whose guest row restrictions cannot be
enforced."""
+ if (
+ datasource.type == DatasourceType.SEMANTIC_VIEW
+ and self.get_guest_rls_filters(datasource)
+ ):
+ raise SupersetSecurityException(
+ SupersetError(
+
error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message=_(
+ "Semantic views cannot enforce guest row-level
security rules."
Review Comment:
Not a blocker. The summary says the refusal comes back with a clear error,
and that holds for the `/column/<col>/values/` route, but not for the chart
data routes: both handlers in `superset/charts/data/api.py` map
`SupersetSecurityException` to a bare `response_403()`, so a restricted guest
gets `403 {"message":"Forbidden"}` and this message only reaches the server
log. I drove that on this branch across every `result_type` and `result_format`
on `POST /api/v1/chart/data`, and all of them refuse before the provider is
called (which is the part that matters) but none of them carry the reason.
That is the endpoint's pre-existing shape rather than anything you changed
here, so the cheapest fix is probably to reword the docs line to say the reason
is in the server log. Or is there a guest redaction rule that intends it this
way and I am reading it backwards?
##########
superset/common/query_context_processor.py:
##########
@@ -428,7 +428,9 @@ def query_cache_key(self, query_obj: QueryObject, **kwargs:
Any) -> str | None:
"""
Returns a QueryObject cache key for objects in self.queries
"""
- datasource = self._qc_datasource
+ datasource: Explorable = self._qc_datasource
+ # Reject unenforceable restrictions before provider identity or cache
reads.
Review Comment:
Not a blocker, more a note on the wording. Because
`_annotation_cache_context` runs inside `query_cache_key`, this is not the only
`get_rls_cache_key` call that can refuse here: the annotation branch below it
calls the same helper on the *source* chart's datasource, so a plain SQL
dataset chart that merely carries a semantic-backed annotation layer is refused
in full for a guest with an applicable rule, and its own SQL rows never render
either.
I confirmed it on this branch: a `SqlaTable` host chart with one
`sourceType: "line"` layer resolving to a semantic view refuses, while the same
chart with an ordinary SQL annotation source keys normally and still applies
the guest rule. Failing closed is the right call, but the summary reads as
though only the semantic content is affected, so one clause in `embedding.mdx`
noting that a semantic-backed annotation layer takes the whole chart down for a
restricted guest would save a host some debugging.
--
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]
