john-bodley commented on code in PR #24808:
URL: https://github.com/apache/superset/pull/24808#discussion_r1276544754
##########
superset/security/manager.py:
##########
@@ -2063,6 +2064,27 @@ def can_access_based_on_dashboard(datasource:
"BaseDatasource") -> bool:
)
exists = db.session.query(query.exists()).scalar()
+
+ # check for datasets that are only used by filters
Review Comment:
I _think_ this logic makes sense. The one thing I struggle with regarding
this method (for both the existing and proposed logic) is how is agnostic of
the specific dashboard in question and thus iterates over all dashboards said
user has access to. This raises two questions i) correctness, and ii)
efficiency.
Currently I can't formulate a situation where (i) is a problem, however for
(ii) this method seems highly inefficient, e.g. we loop over all the dashboards
a user has access to in relation to said dataset/datasource, whereas in
actuality we likely know the context a priori.
Note in https://github.com/apache/superset/pull/24789 this method is slated
for removal, but the addition of the integration test will ensure that the
logic will be preserved.
--
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]
