korbit-ai[bot] commented on code in PR #32289:
URL: https://github.com/apache/superset/pull/32289#discussion_r1959422900
##########
superset/security/manager.py:
##########
@@ -2290,65 +2290,58 @@
assert datasource
- if not (
- self.can_access_schema(datasource)
- or self.can_access("datasource_access", datasource.perm or "")
- or self.is_owner(datasource)
- or (
- # Grant access to the datasource only if dashboard RBAC is
enabled
- # or the user is an embedded guest user with access to the
dashboard
- # and said datasource is associated with the dashboard
chart in
- # question.
- form_data
- and (dashboard_id := form_data.get("dashboardId"))
- and (
- dashboard_ := self.get_session.query(Dashboard)
- .filter(Dashboard.id == dashboard_id)
- .one_or_none()
- )
- and (
- (is_feature_enabled("DASHBOARD_RBAC") and
dashboard_.roles)
- or (
- is_feature_enabled("EMBEDDED_SUPERSET")
- and self.is_guest_user()
- )
- )
- and (
- (
- # Native filter.
- form_data.get("type") == "NATIVE_FILTER"
- and (native_filter_id :=
form_data.get("native_filter_id"))
- and dashboard_.json_metadata
- and (json_metadata :=
json.loads(dashboard_.json_metadata))
- and any(
+ if datasource or query_context or viz:
+ form_data = None
+
+ if query_context:
+ datasource = query_context.datasource
+ form_data = query_context.form_data
+ elif viz:
+ datasource = viz.datasource
+ form_data = viz.form_data
+
+ assert datasource
+
+
+ can_access_schema = self.can_access_schema(datasource)
+ can_access_datasource_permission =
self.can_access("datasource_access", datasource.perm or "")
+ is_owner = self.is_owner(datasource)
+
+
+ dashboard_accessible = False
+ dashboard_ = None
Review Comment:
### Inconsistent variable grouping spacing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Double empty lines between related variable declarations creates unnecessary
visual separation.
###### Why this matters
Inconsistent spacing makes it harder to visually group related code blocks
and understand the logical flow.
###### Suggested change ∙ *Feature Preview*
Use single empty lines to separate related variable groups:
```python
can_access_schema = self.can_access_schema(datasource)
can_access_datasource_permission = self.can_access("datasource_access",
datasource.perm or "")
is_owner = self.is_owner(datasource)
dashboard_accessible = False
dashboard_ = None
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a0a12cb-96ae-4b27-8d6f-ba9d0d5f7be7?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:a06f8ef4-65cd-4a6c-bd74-3da474315b90 -->
##########
superset/security/manager.py:
##########
@@ -2290,65 +2290,58 @@
assert datasource
- if not (
- self.can_access_schema(datasource)
- or self.can_access("datasource_access", datasource.perm or "")
- or self.is_owner(datasource)
- or (
- # Grant access to the datasource only if dashboard RBAC is
enabled
- # or the user is an embedded guest user with access to the
dashboard
- # and said datasource is associated with the dashboard
chart in
- # question.
- form_data
- and (dashboard_id := form_data.get("dashboardId"))
- and (
- dashboard_ := self.get_session.query(Dashboard)
- .filter(Dashboard.id == dashboard_id)
- .one_or_none()
- )
- and (
- (is_feature_enabled("DASHBOARD_RBAC") and
dashboard_.roles)
- or (
- is_feature_enabled("EMBEDDED_SUPERSET")
- and self.is_guest_user()
- )
- )
- and (
- (
- # Native filter.
- form_data.get("type") == "NATIVE_FILTER"
- and (native_filter_id :=
form_data.get("native_filter_id"))
- and dashboard_.json_metadata
- and (json_metadata :=
json.loads(dashboard_.json_metadata))
- and any(
+ if datasource or query_context or viz:
+ form_data = None
+
+ if query_context:
+ datasource = query_context.datasource
+ form_data = query_context.form_data
+ elif viz:
+ datasource = viz.datasource
+ form_data = viz.form_data
+
+ assert datasource
+
+
+ can_access_schema = self.can_access_schema(datasource)
+ can_access_datasource_permission =
self.can_access("datasource_access", datasource.perm or "")
+ is_owner = self.is_owner(datasource)
+
+
+ dashboard_accessible = False
+ dashboard_ = None
+
+ if form_data and (dashboard_id :=
form_data.get("dashboardId")):
+ try:
+ dashboard_ =
self.get_session.query(Dashboard).filter(Dashboard.id ==
dashboard_id).one_or_none()
+ except Exception as e:
+ logging.error(f"Error fetching dashboard
{dashboard_id}: {e}")
Review Comment:
### Unsafe Dashboard Fetch Exception Handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The exception handling for dashboard fetching logs the error but continues
execution, potentially with an invalid dashboard_ value.
###### Why this matters
If there's a database error while fetching the dashboard, the code continues
with dashboard_ = None, which could lead to incorrect access control decisions.
###### Suggested change ∙ *Feature Preview*
Add a re-raise after logging to ensure access is denied on database errors:
```python
try:
dashboard_ = self.get_session.query(Dashboard).filter(Dashboard.id ==
dashboard_id).one_or_none()
except Exception as e:
logging.error(f"Error fetching dashboard {dashboard_id}: {e}")
raise SupersetSecurityException(
self.get_dashboard_access_error_object(None)
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/03e092e1-8574-48ca-96dc-f6c59b04eadc?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:bebb2931-24cf-4cb4-92d2-09c2f3f3d7f9 -->
##########
superset/security/manager.py:
##########
@@ -2290,65 +2290,58 @@ def raise_for_access( # noqa: C901
assert datasource
- if not (
- self.can_access_schema(datasource)
- or self.can_access("datasource_access", datasource.perm or "")
- or self.is_owner(datasource)
- or (
- # Grant access to the datasource only if dashboard RBAC is
enabled
- # or the user is an embedded guest user with access to the
dashboard
- # and said datasource is associated with the dashboard
chart in
- # question.
- form_data
- and (dashboard_id := form_data.get("dashboardId"))
- and (
- dashboard_ := self.get_session.query(Dashboard)
- .filter(Dashboard.id == dashboard_id)
- .one_or_none()
- )
- and (
- (is_feature_enabled("DASHBOARD_RBAC") and
dashboard_.roles)
- or (
- is_feature_enabled("EMBEDDED_SUPERSET")
- and self.is_guest_user()
- )
- )
- and (
- (
- # Native filter.
- form_data.get("type") == "NATIVE_FILTER"
- and (native_filter_id :=
form_data.get("native_filter_id"))
- and dashboard_.json_metadata
- and (json_metadata :=
json.loads(dashboard_.json_metadata))
- and any(
+ if datasource or query_context or viz:
+ form_data = None
+
+ if query_context:
+ datasource = query_context.datasource
+ form_data = query_context.form_data
+ elif viz:
+ datasource = viz.datasource
+ form_data = viz.form_data
+
+ assert datasource
+
+
+ can_access_schema = self.can_access_schema(datasource)
+ can_access_datasource_permission =
self.can_access("datasource_access", datasource.perm or "")
+ is_owner = self.is_owner(datasource)
+
+
+ dashboard_accessible = False
+ dashboard_ = None
+
+ if form_data and (dashboard_id :=
form_data.get("dashboardId")):
+ try:
+ dashboard_ =
self.get_session.query(Dashboard).filter(Dashboard.id ==
dashboard_id).one_or_none()
+ except Exception as e:
+ logging.error(f"Error fetching dashboard
{dashboard_id}: {e}")
Review Comment:
### Insufficient error logging context <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The log message lacks context about the method and operation being
performed. The exception traceback is not captured.
###### Why this matters
Without method context and stack trace, it will be difficult to debug
production issues when dashboard fetching fails. The error log may not provide
enough information to identify the root cause.
###### Suggested change ∙ *Feature Preview*
```python
except Exception as e:
logging.error(
"Error in raise_for_access when fetching dashboard %s: %s",
dashboard_id,
str(e),
exc_info=True
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32b57884-9478-4c2f-9c3b-f064eb87f714?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:613f9558-a275-48a3-a1fc-ef78faff2c60 -->
--
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]
