Apply security row and query filters in metrics view Summary

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"