suddjian commented on a change in pull request #13773:
URL: https://github.com/apache/superset/pull/13773#discussion_r606032514
##########
File path: superset/security/manager.py
##########
@@ -987,15 +989,28 @@ def raise_for_access( # pylint:
disable=too-many-arguments,too-many-branches
)
if datasource or query_context or viz:
+ extra_jwt = None
if query_context:
datasource = query_context.datasource
+ extra_jwt = query_context.extra_jwt
elif viz:
datasource = viz.datasource
+ extra_jwt = viz.extra_jwt
assert datasource
+ ds_allowed_in_dashboard = False
+ if feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC"):
+ dashboard_data_context =
dashboard_jwt_manager.parse_jwt(extra_jwt)
+
+ if dashboard_data_context:
+ ds_allowed_in_dashboard = (
+ datasource.id in dashboard_data_context.dataset_ids
+ )
+
if not (
- self.can_access_schema(datasource)
+ ds_allowed_in_dashboard
Review comment:
> I would be happy to do a follow up PR just to support SPA , no worries
there
In the SPA project we have already done the work to remove
dashboard-specific bootstrap data so that the apps can be cleanly merged. I
think rather than adding new bootstrap properties at this stage, it would be
better to use an endpoint. That said, if you're okay with this feature breaking
in the very near future, I won't stand in your way. The compatibility
discussion is a relatively minor issue.
I think the security concerns have already been discussed somewhat in
[SIP-51](https://github.com/apache/superset/issues/10408). Anyone using the
feature flag should understand that dashboard owners have the ability to grant
data access to others arbitrarily. These effects should be well documented.
It's an entirely different security profile for the application.
I am of the opinion that the security manager should make a database query
to find the information it needs, rather than using this new JWT pattern. I
haven't seen anything that indicates that a JWT is actually required here,
though I could be mistaken.
Introducing new authorization patterns specific to a given feature flag will
make it more difficult to onboard into the Superset codebase, as well as making
it more complicated to change system behavior. The JWT pattern also makes it
complicated to revoke or change a user's access, and introduces a new
responsibility for the client to correctly manage the JWT.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
