Apply security row and query filters in metrics view Summary#9717
Conversation
The summary executor built its SQL by hand and never applied the security policy's row filter or query filter, so dimension example/min/max values were computed over all rows regardless of row-level security. - Add metricsview.SecurityFilterSQL to compile the policy's row filter and query filter into a WHERE expression for the underlying table. - Apply it in Executor.Summary to both the categorical summary query and the time dimension min/max resolution (the sample window is derived from the max timestamp, so it must be filtered too). - Include the claims' AdditionalRules and SkipChecks in the resolver result cache key; previously two magic auth tokens with different row filters but identical user attributes shared cached results.
| } | ||
| } else { | ||
| res, err = e.resolveTimestampsForTable(ctx, mv.Database, mv.DatabaseSchema, mv.Table, timeExpr, mv.WatermarkExpression) | ||
| res, err = e.resolveTimestampsForTable(ctx, mv.Database, mv.DatabaseSchema, mv.Table, timeExpr, mv.WatermarkExpression, "", nil) |
There was a problem hiding this comment.
Would be nice to add a docstring to the function that it intentionally does not apply security policies. The reasons are described here: https://rilldata.slack.com/archives/C01A9DYP013/p1776348750584469
There was a problem hiding this comment.
Added, summarizing the reasons from the linked thread.
| var defaultTimeDimensionSummary DimensionSummary | ||
| for _, dim := range timeDimensions { | ||
| timeRange, err := e.Timestamps(ctx, dim.Name) | ||
| timeRange, err := e.summaryTimestamps(ctx, dim.Name, securityFilter, securityFilterArgs) |
There was a problem hiding this comment.
Note this changes summary calls (used by MCP) from a pretty cheap operation that only checks metadata and a small time range to potentially a full scan of all time (since there's a filter to handle when computing min/max time).
Not sure if it's worth it (for example, for normal dashboard loads, we don't apply security policies for time ranges to make sure min/max time can be computed as only a metadata operation).
There was a problem hiding this comment.
Agreed, reverted. Time ranges now come from the unfiltered Timestamps as before; only the categorical dimension query (already bounded by the 24h sample window) gets the security filter. If a user's accessible rows are older than the window, the values come back null, mirroring the accepted trade-off for dashboards.
| // SecurityFilterSQL compiles the security policy's query filter and row filter for the given metrics view | ||
| // into a SQL expression and args that can be used in a WHERE clause against the metrics view's underlying table. | ||
| // It returns an empty SQL string if the security policy does not restrict row access. | ||
| // It is intended for queries that are built manually instead of through an AST, such as dimension summaries. | ||
| func SecurityFilterSQL(mv *runtimev1.MetricsViewSpec, sec MetricsViewSecurity, dialect drivers.Dialect) (string, []any, error) { | ||
| a := &AST{ | ||
| MetricsView: mv, | ||
| Security: sec, | ||
| Query: &Query{}, | ||
| Dialect: dialect, | ||
| } | ||
|
|
||
| res, err := a.buildWhereForUnderlyingTable(nil) | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
| if res == nil { | ||
| return "", nil, nil | ||
| } | ||
| return res.Expr, res.Args, nil | ||
| } |
There was a problem hiding this comment.
- This is pretty hacky usage of
buildWhereForUnderlyingTable. I think it would be cleaner to move the security filter logic frombuildWhereForUnderlyingTableinto this function, and then makebuildWhereForUnderlyingTablecall this function. That would avoid creating a 'fake'ASTvalue and abusingbuildWhereForUnderlyingTable. - This function is placed in a weird place in this file. It breaks the style guide rules about a) grouping by receiver, b) placing exported functions before internal ones. See https://github.com/uber-go/guide/blob/master/style.md#function-grouping-and-ordering
There was a problem hiding this comment.
Done. buildWhereForUnderlyingTable now calls SecurityFilterSQL, which owns the filter compilation, and it is moved up with the exported functions.
| if opts.Claims.SkipChecks { | ||
| if _, err := hash.Write([]byte("skip_checks")); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
When skip_checks is true, it probably should not write any access related fields (like user attributes or additional rules) to the hash.
There was a problem hiding this comment.
Additional rules are now skipped when skip_checks is true. I kept user attributes in the key though: SkipChecks claims carry real attributes in some paths (ManageInstances claims, skip_nested_security APIs) and attributes feed {{ .user }} templating regardless of security checks, so excluding them could serve one user's templated results to another.
- Revert the security filtering of time ranges in the summary: per the
earlier decision on time expressions, min/max timestamps intentionally
remain unfiltered so they stay metadata-only operations and cache across
users. Document this on Timestamps. If a user's accessible rows are all
older than the sample interval, the summary values are null.
- Move the security filter compilation out of buildWhereForUnderlyingTable
into SecurityFilterSQL and place it with the exported functions.
- Skip hashing the claims' additional rules in the resolver cache key when
SkipChecks is true. User attributes stay in the key even with SkipChecks
because they also drive templating ({{ .user }}), e.g. for ManageInstances
claims and skip_nested_security APIs which carry real user attributes.
…ary-row-filter # Conflicts: # runtime/resolver.go
2 participants
Executor.Summarybuilt its SQL by hand and never applied the security policy's row filter or query filter, soexample_value/min_value/max_valuefor accessible dimensions were computed over all rows, leaking values across row-level security boundaries (reachable via themetrics_summaryresolver and the AIquery_metrics_view_summarytool).metricsview.SecurityFilterSQL, which compiles the policy's query filter and row filter into a WHERE expression for the underlying table. It is now the shared implementation used bybuildWhereForUnderlyingTable(AST path) and applied to the summary's categorical dimension query.Timestamps. As a consequence, if a user's accessible rows are all older than the summary's 24h sample window, their summary values are null.AdditionalRulesin the resolver result cache key (skipped whenSkipChecksis true, since security is not applied then). Previously, two magic auth tokens with different row filters but identical user attributes shared cached results (the newadditional_rulessummary test fails deterministically without this).metrics_summaryresolver test expectations and adds cases for a viewer whose values are null rather than leaked, an admin user, ClickHouse, and an additional-rules (query filter) policy.Checklist: