Apply security row and query filters in metrics view Summary (#9717)

* Apply security row and query filters in metrics view summary

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.

* Address review feedback

- 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.

Author: nishantmonu51

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
 }

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

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,26 +158,32 @@ 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
 	sqlBuilder.WriteString("SELECT ")
 	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()

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,13 +224,35 @@ 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:
       metrics_view: duckdb_metrics_restricted
     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"