I really think we should deprecate restricted metrics as they don't work as expected since the adhoc metrics have been added.

Also agree with moving more data-access logic and context to the security manager.

@mistercrunch are you ok with the security manager having logic like assert_viz_permission?

I agree that we should probably deprecate restricted metrics seeing as i) ad-hoc metrics completely bypass the restrictedness, and ii) there’s currently no check for SQL datasources.

Do you think it best if I provide the change to deprecate restricted metrics in this PR or a followup PR?

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.

I was curious to give deprecation of restricted metrics a shot and it took me ~15 minutes to get to this draft: #8197

I merged the PR deprecating is_restricted and related logic/docs, it should simplify this one.

Thanks @mistercrunch. I’ll update the logic in this PR. I agree that the security manager is overly complex and will try to simplify where possible.

@mistercrunch thanks for deprecating the restricted metrics (as a side note I wonder if there should be another migration to remove all the obsolete metric_access permissions).

I've updated the PR which removes the now obsolete metric checks. Are you still supportive of having the:

• assert_query_context_permission
• assert_viz_permission

methods which now have the same behavior as the datasource access but simply provide more context, i.e., the fully defined current context, if a custom security manager would like to override.

Let me know how best to proceed.

@mistercrunch could you take another look at this?

Noted @mistercrunch.