mistercrunch commented on issue #8175: [metric] Adding security for restricted metrics URL: https://github.com/apache/incubator-superset/pull/8175#issuecomment-529298194 The idea of `assert_viz_permission` in `SecurityManager` makes sense and I understand why you'd do that. The only concern is whether the BaseViz interface is settled enough to expose externally through SecurityManager. `assert_query_context_permission` exposes less surface which is preferable. Taping into the viz object offers lots of flexibility, but little guarantee as to whether you hooks will still work after you upgrade... There's another underlying issue I'd like to fix, it's the fact that `check_datasource_perms` is called from a decorator and recreates a BaseViz object that gets thrown away and recreated later on. Decorators are cool and all, but a clear shortcoming is how it gets ugly in cases like this (context need to be shared across decorator and function body). We need to de-decorate that function and do things right. Also we should clarify if these security assertions call each other or, the view calls them individually/sequentially. It works either way, but what's important is having something super intuitive and readable. Currently there's quite a bit of indirection (the decorator with the callback + a complex method hierarchy) and it's hard to reason about and easy to eff this up and introduce some regression. Knowing all this, what we want to accomplish: * deprecate restricted metrics * de-decorate security assertions * have a set of comprehensive data access assertion in security manager (clear score and hierarchy, consistent naming, clarity on what is private/public), comments on places where interfaces may change (override at your own risk!) I'm not sure how to roll this out best, but a chain of small PRs is probably best. About restricted metrics, you could start partially deprecating here by not trying to migrate the code, idk how involved deprecating the feature will be, but shouldn't be too hard.
---------------------------------------------------------------- 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] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
