diff --git a/runtime/metricsview/ast.go b/runtime/metricsview/ast.go index 4556886f6545..6a4cc71e2e50 100644 --- a/runtime/metricsview/ast.go +++ b/runtime/metricsview/ast.go @@ -363,6 +363,38 @@ func NewAST(mv *runtimev1.MetricsViewSpec, sec MetricsViewSecurity, qry *Query, return ast, nil } +// 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 used when building an AST, and can also be used directly for queries that are built manually, such as dimension summaries. +func SecurityFilterSQL(mv *runtimev1.MetricsViewSpec, sec MetricsViewSecurity, dialect drivers.Dialect) (string, []any, error) { + var res *ExprNode + + if qf := sec.QueryFilter(); qf != nil { + // Compiling an expression requires an AST value for contextual info such as dimension lookups. + a := &AST{ + MetricsView: mv, + Security: sec, + Query: &Query{}, + Dialect: dialect, + } + expr, args, err := a.SQLForExpression(NewExpressionFromProto(qf), nil, false, false) + if err != nil { + return "", nil, fmt.Errorf("failed to compile the security policy's query filter: %w", err) + } + res = res.And(expr, args) + } + + if rf := sec.RowFilter(); rf != "" { + res = res.And(rf, nil) + } + + if res == nil { + return "", nil, nil + } + return res.Expr, res.Args, nil +} + // ResolveDimension returns a dimension spec for the given dimension query. // If the dimension query specifies a computed dimension, it constructs a dimension spec to match it. func (a *AST) ResolveDimension(qd Dimension, visible bool) (*runtimev1.MetricsViewSpec_Dimension, error) { @@ -1022,7 +1054,7 @@ func (a *AST) addReferencedMeasuresToScope(n *SelectNode, referencedMeasures []s } // buildWhereForUnderlyingTable constructs an expression for a WHERE clause for the underlying table. -// It combines the provided where expression with any security policy filters. +// It combines the provided where expression with the security policy's filters. // It allows the input `where` to be nil, and returns nil if there are no conditions to apply. func (a *AST) buildWhereForUnderlyingTable(where *Expression) (*ExprNode, error) { var res *ExprNode @@ -1033,18 +1065,11 @@ func (a *AST) buildWhereForUnderlyingTable(where *Expression) (*ExprNode, error) } res = res.And(expr, args) - if qf := a.Security.QueryFilter(); qf != nil { - e := NewExpressionFromProto(qf) - expr, args, err = a.SQLForExpression(e, nil, false, false) - if err != nil { - return nil, fmt.Errorf("failed to compile the security policy's query filter: %w", err) - } - res = res.And(expr, args) - } - - if rf := a.Security.RowFilter(); rf != "" { - res = res.And(rf, nil) + secExpr, secArgs, err := SecurityFilterSQL(a.MetricsView, a.Security, a.Dialect) + if err != nil { + return nil, err } + res = res.And(secExpr, secArgs) return res, nil } diff --git a/runtime/metricsview/executor/executor.go b/runtime/metricsview/executor/executor.go index c7ec6c26c21c..79f46d3cf83a 100644 --- a/runtime/metricsview/executor/executor.go +++ b/runtime/metricsview/executor/executor.go @@ -170,6 +170,10 @@ func (e *Executor) ValidateQuery(qry *metricsview.Query) error { // Timestamps queries min, max and watermark for the metrics view. // For the primary time dimension it also resolves rollup table timestamps if rollups are present. +// It intentionally does not apply security policy row filters: +// unfiltered timestamps can be computed from database metadata and cached across users, +// and they keep time expressions evaluating consistently for all users. +// The trade-off is that users whose accessible rows don't span the full range may see "no data" for some time ranges. func (e *Executor) Timestamps(ctx context.Context, timeDim string) (metricsview.TimestampsResult, error) { if timeDim == "" { timeDim = e.metricsView.TimeDimension diff --git a/runtime/metricsview/executor/executor_summary.go b/runtime/metricsview/executor/executor_summary.go index 6a43eec1f865..96d4491b9677 100644 --- a/runtime/metricsview/executor/executor_summary.go +++ b/runtime/metricsview/executor/executor_summary.go @@ -9,6 +9,7 @@ import ( runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime" "github.com/rilldata/rill/runtime/drivers" + "github.com/rilldata/rill/runtime/metricsview" ) const ( @@ -42,6 +43,12 @@ func (e *Executor) Summary(ctx context.Context) (*SummaryResult, error) { return nil, runtime.ErrForbidden } + // Compile the security policy's row filter and query filter so the summary only reflects rows the user can access + securityFilter, securityFilterArgs, err := metricsview.SecurityFilterSQL(e.metricsView, e.security, e.olap.Dialect()) + if err != nil { + return nil, fmt.Errorf("failed to compile the security policy's filters: %w", err) + } + // Gather the categorical and time dimensions var dimensions, timeDimensions []*runtimev1.MetricsViewSpec_Dimension for _, dim := range e.metricsView.Dimensions { @@ -85,7 +92,9 @@ func (e *Executor) Summary(ctx context.Context) (*SummaryResult, error) { timeDimensions = timeDimensions[0:SummaryTimeDimensionsLimit] } - // Compute the time dimension summaries + // Compute the time dimension summaries. + // Note that Timestamps intentionally does not apply the security policy's filters (see its docstring), + // so unlike the dimension summaries below, the time ranges reflect all rows in the underlying table. var summaries []DimensionSummary var defaultTimeDimensionSummary DimensionSummary for _, dim := range timeDimensions { @@ -149,16 +158,22 @@ func (e *Executor) Summary(ctx context.Context) (*SummaryResult, error) { } } - // Create a where clause that applies the SummarySampleInterval to the default time dimension - var whereClause string + // Create a where clause that applies the SummarySampleInterval to the default time dimension and the security policy's filters + var whereClauses []string var args []any if timeDimExpr != "" && defaultTimeDimensionSummary.MaxValue != nil { maxTime, _ := defaultTimeDimensionSummary.MaxValue.(time.Time) if !maxTime.IsZero() { - whereClause = fmt.Sprintf("WHERE %s >= ?", timeDimExpr) - args = []any{maxTime.Add(-SummarySampleInterval)} + whereClauses = append(whereClauses, fmt.Sprintf("%s >= ?", timeDimExpr)) + args = append(args, maxTime.Add(-SummarySampleInterval)) } } + // Note the sample interval is derived from the unfiltered max timestamp, + // so if the user's accessible rows are all older than the sample interval, the summary values will be null. + if securityFilter != "" { + whereClauses = append(whereClauses, fmt.Sprintf("(%s)", securityFilter)) + args = append(args, securityFilterArgs...) + } // Build the SQL query var sqlBuilder strings.Builder @@ -166,9 +181,9 @@ func (e *Executor) Summary(ctx context.Context) (*SummaryResult, error) { sqlBuilder.WriteString(strings.Join(selectClauses, ", ")) sqlBuilder.WriteString(" FROM ") sqlBuilder.WriteString(e.olap.Dialect().EscapeTable(e.metricsView.Database, e.metricsView.DatabaseSchema, e.metricsView.Table)) - if whereClause != "" { - sqlBuilder.WriteString(" ") - sqlBuilder.WriteString(whereClause) + if len(whereClauses) > 0 { + sqlBuilder.WriteString(" WHERE ") + sqlBuilder.WriteString(strings.Join(whereClauses, " AND ")) } sqlBuilder.WriteString(" LIMIT 1") sql := sqlBuilder.String() diff --git a/runtime/resolvers/testdata/metrics_summary.yaml b/runtime/resolvers/testdata/metrics_summary.yaml index 6a8e29faecbb..ec28ab8496eb 100644 --- a/runtime/resolvers/testdata/metrics_summary.yaml +++ b/runtime/resolvers/testdata/metrics_summary.yaml @@ -66,6 +66,21 @@ project_files: - if: "'{{ .user.role }}' != 'admin'" names: - country + clickhouse_metrics_restricted.yaml: + type: metrics_view + model: clickhouse_test_data + timeseries: event_time + dimensions: + - name: category + column: category + - name: country + column: country + measures: + - name: total_revenue + expression: sum(revenue) + security: + access: true + row_filter: "country = '{{ .user.country }}'" duckdb_metrics_view.yaml: type: metrics_view model: duckdb_test_data @@ -209,6 +224,8 @@ tests: max_value: "2024-01-08T00:00:00Z" min_value: "2024-01-01T00:00:00Z" name: event_time + # The time range is intentionally unfiltered (see the Timestamps docstring), and since this user's accessible + # rows are all older than the sample interval, the dimension summary values are null (instead of leaking values). - name: duckdb_summary_with_security resolver: metrics_summary properties: @@ -216,6 +233,26 @@ tests: user_attributes: country: "US" role: "viewer" + result: + - dimensions: + - data_type: CODE_TIMESTAMP + max_value: "2024-01-08T00:00:00Z" + min_value: "2024-01-01T00:00:00Z" + name: event_time + - data_type: CODE_STRING + name: category + time_range: + data_type: CODE_TIMESTAMP + max_value: "2024-01-08T00:00:00Z" + min_value: "2024-01-01T00:00:00Z" + name: event_time + - name: duckdb_summary_with_security_admin + resolver: metrics_summary + properties: + metrics_view: duckdb_metrics_restricted + user_attributes: + country: "CA" + role: "admin" result: - dimensions: - data_type: CODE_TIMESTAMP @@ -225,8 +262,78 @@ tests: - data_type: CODE_STRING example_value: Electronics max_value: Electronics - min_value: Clothing + min_value: Electronics name: category + - data_type: CODE_STRING + example_value: CA + max_value: CA + min_value: CA + name: country + time_range: + data_type: CODE_TIMESTAMP + max_value: "2024-01-08T00:00:00Z" + min_value: "2024-01-01T00:00:00Z" + name: event_time + - name: duckdb_summary_with_additional_row_filter + resolver: metrics_summary + properties: + metrics_view: duckdb_metrics_view + additional_rules: + - row_filter: "country = 'CA'" + result: + - dimensions: + - data_type: CODE_TIMESTAMP + max_value: "2024-01-08T00:00:00Z" + min_value: "2024-01-01T00:00:00Z" + name: event_time + - data_type: CODE_STRING + example_value: CA + max_value: CA + min_value: CA + name: country + - data_type: CODE_STRING + example_value: Electronics + max_value: Electronics + min_value: Electronics + name: category + - data_type: CODE_STRING + has_nulls: true + name: product + - data_type: CODE_STRING + has_nulls: true + name: channel + - data_type: CODE_BOOL + example_value: true + max_value: true + min_value: true + name: is_active + time_range: + data_type: CODE_TIMESTAMP + max_value: "2024-01-08T00:00:00Z" + min_value: "2024-01-01T00:00:00Z" + name: event_time + - name: clickhouse_summary_with_security + resolver: metrics_summary + properties: + metrics_view: clickhouse_metrics_restricted + user_attributes: + country: "CA" + result: + - dimensions: + - data_type: CODE_TIMESTAMP + max_value: "2024-01-08T00:00:00Z" + min_value: "2024-01-01T00:00:00Z" + name: event_time + - data_type: CODE_STRING + example_value: Electronics + max_value: Electronics + min_value: Electronics + name: category + - data_type: CODE_STRING + example_value: CA + max_value: CA + min_value: CA + name: country time_range: data_type: CODE_TIMESTAMP max_value: "2024-01-08T00:00:00Z"