villebro commented on a change in pull request #13773: URL: https://github.com/apache/superset/pull/13773#discussion_r603084218
########## File path: superset/config.py ########## @@ -1123,6 +1123,8 @@ class CeleryConfig: # pylint: disable=too-few-public-methods SQLALCHEMY_DOCS_URL = "https://docs.sqlalchemy.org/en/13/core/engines.html"; SQLALCHEMY_DISPLAY_TEXT = "SQLAlchemy docs" +DASHBOARD_JWT_SECRET = "my secret key replace me" Review comment: I'd rather leave this `None` and then raise if it hasn't been defined. ########## File path: superset/security/manager.py ########## @@ -987,15 +988,24 @@ 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 + dashboard_data_context = dashboard_jwt_manager.parse_jwt(extra_jwt) + + data_source_allowed_in_dashboard = ( + datasource.id in dashboard_data_context.dataset_ids + ) if not ( - self.can_access_schema(datasource) + data_source_allowed_in_dashboard + or self.can_access_schema(datasource) Review comment: I'd rewrite this to make sure there's no way the new logic can leak into the current flow without the FF. Something like ```python data_source_allowed_in_dashboard = False if feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC"): dashboard_data_context = dashboard_jwt_manager.parse_jwt(extra_jwt) data_source_allowed_in_dashboard = ( datasource.id in dashboard_data_context.dataset_ids ) ``` -- 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]
