Skip to content

Commit eebe163

Browse files
Implement visibility-follows-access and fix policy enforcement bugs
- Refactor EngineCache to cache raw CachedCatalog per datasource instead of SessionContext; add build_user_context() and compute_user_visibility() to build per-user filtered SessionContext at connect time based on policy obligations - Add ConnectionStore (DashMap-backed) in handler.rs to track per-connection SessionContext, user_id, and datasource_name; add rebuild_contexts_for_datasource() on ProxyHandler to immediately apply schema changes to active connections - Add AdminState.proxy_handler field; call rebuild_contexts_for_datasource() from policy mutation handlers (update, delete, assign, remove) alongside policy cache invalidation so column visibility changes take effect without reconnect - Fix: enforce column_access deny obligations from deny-effect policies in PolicyHook (previously only permit-policy column denies were applied at query time) - Fix: exclude disabled policies from compute_user_visibility() so disabling a policy removes its schema-hiding effect immediately - Fix: return SQLSTATE 42501 when all selected columns are denied instead of returning empty rows, preventing silent data access confusion - Add dashmap = "6" dependency; update docs, CLAUDE.md, ideas.md, and permission_stories.md
1 parent f4e5b2b commit eebe163

15 files changed

Lines changed: 1257 additions & 139 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

admin-ui/src/components/PolicyForm.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,7 @@ export function PolicyForm({ initial, onSubmit, submitLabel, isSubmitting, error
244244
placeholder="* or public"
245245
className="w-full border border-gray-300 rounded px-2 py-1.5 text-xs focus:outline-none focus:ring-1 focus:ring-blue-500"
246246
/>
247+
<p className="text-xs text-gray-400 mt-1">Use <code className="bg-gray-100 px-1 rounded">*</code> to match all schemas.</p>
247248
</div>
248249
<div>
249250
<label className="block text-xs font-medium text-gray-600 mb-1">Table</label>
@@ -254,6 +255,7 @@ export function PolicyForm({ initial, onSubmit, submitLabel, isSubmitting, error
254255
placeholder="* or orders"
255256
className="w-full border border-gray-300 rounded px-2 py-1.5 text-xs focus:outline-none focus:ring-1 focus:ring-blue-500"
256257
/>
258+
<p className="text-xs text-gray-400 mt-1">Use <code className="bg-gray-100 px-1 rounded">*</code> to match all tables.</p>
257259
</div>
258260
</div>
259261

@@ -316,6 +318,9 @@ export function PolicyForm({ initial, onSubmit, submitLabel, isSubmitting, error
316318
placeholder="ssn, credit_card, phone"
317319
className="w-full border border-gray-300 rounded px-2 py-1.5 text-xs font-mono focus:outline-none focus:ring-1 focus:ring-blue-500"
318320
/>
321+
<p className="text-xs text-gray-400 mt-1">
322+
Combined with <code className="bg-gray-100 px-1 rounded">schema: *</code> and <code className="bg-gray-100 px-1 rounded">table: *</code> to deny a column across all tables. The table itself remains visible — only the column is hidden.
323+
</p>
319324
</div>
320325
)}
321326
</div>

docs/permission-security-tests.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,3 +153,42 @@ async fn join_both_tables_filtered() { ... }
153153
#[tokio::test]
154154
async fn policy_required_no_policy_returns_empty() { ... }
155155
```
156+
157+
---
158+
159+
### 9. `column_access deny` on deny-effect policies ignored at query time
160+
161+
**Vector**: Admin creates a **deny-effect** policy with a `column_access deny` obligation on `ssn`, expecting the column to be stripped from query results immediately.
162+
163+
**Bug**: `PolicyHook` only processed `column_access` obligations from *permit* policies (the loop at `handle_query` iterated over `session.permit_policies`). Deny-effect policies were only checked for table-level "Access denied" errors. So `column_access deny` on a deny policy had no query-time effect — the column appeared in results until the user reconnected (when `compute_user_visibility()` hid it from the schema).
164+
165+
**Defense**: `PolicyHook::handle_query` now runs a second loop over `session.deny_policies` after the permit loop, processing only `column_access` obligations and adding matched columns to `column_denies`.
166+
167+
**Test**: Create a deny-effect policy with `column_access deny` on `ssn`. Assign to datasource. Without reconnecting, run `SELECT ssn FROM employees`. Verify `ssn` column is absent from the result set.
168+
169+
---
170+
171+
### 10. Disabled policies still enforced in visibility layer
172+
173+
**Vector**: Admin disables a policy with `column_access deny`, expecting the column to reappear in `information_schema.columns` on next reconnect.
174+
175+
**Bug**: `compute_user_visibility()` loaded obligations for ALL assigned policy IDs, including those belonging to disabled policies. The `column_access deny` block didn't check if the parent policy was enabled, so disabling a policy had no effect on schema visibility.
176+
177+
**Defense**: `compute_user_visibility()` now loads obligations only for *enabled* policy IDs (those from the `is_enabled = true` filtered query). Disabled policies contribute neither to `visible_tables` nor `denied_columns`.
178+
179+
**Test**:
180+
- Unit: `engine::tests::test_disabled_policy_column_deny_not_applied` — disabled policy → `denied_columns` is empty.
181+
- Unit: `engine::tests::test_enabled_policy_column_deny_applied` — enabled policy → `denied_columns` contains `ssn`.
182+
- Manual: Disable a policy with `column_access deny`. Without reconnecting, verify `ssn` reappears in `information_schema.columns` on the next query (policy changes trigger an immediate `SessionContext` rebuild for all active connections).
183+
184+
---
185+
186+
### 11. `SELECT <denied-column>` returns silent empty rows instead of an error
187+
188+
**Vector**: User runs `SELECT ssn FROM customers` where `ssn` is denied. They receive many rows with empty/null values and incorrectly conclude the column is empty in the database.
189+
190+
**Bug**: When all selected columns were stripped by `column_access deny`, `new_exprs` became empty. `LogicalPlanBuilder::project([])` produced a zero-column projection that DataFusion executed successfully — returning N rows with no column data. Clients rendered this as empty rows.
191+
192+
**Defense**: `PolicyHook` now checks for an empty `new_exprs` after column stripping and returns SQLSTATE `42501` (insufficient_privilege) listing the denied columns, before attempting to build the projection.
193+
194+
**Test**: Create a policy with `column_access deny` on `ssn`. Run `SELECT ssn FROM customers`. Verify the response is an error with SQLSTATE `42501` and not an empty result set.

docs/permission-system.md

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,19 @@ BetweenRows has a policy-based permission system that controls what data users c
77
Permissions are defined as **policies**. A policy is a named, reusable unit that contains one or more **obligations** — rules that specify what transformation to apply to a query. Policies are then **assigned** to a datasource (optionally scoped to a specific user).
88

99
When a user runs a query:
10-
1. The proxy loads all policies assigned to the datasource for that user.
11-
2. Any `deny` policies short-circuit with an error.
12-
3. `permit` policies are applied — their obligations rewrite the query in-flight (row filters, column masks, column access controls).
10+
1. The proxy loads all **enabled** policies assigned to the datasource for that user.
11+
2. `deny` policies are evaluated first. A `row_filter` match on a deny policy rejects the query with an error. `column_access deny` obligations on deny policies are collected alongside those from permit policies (step 3).
12+
3. `permit` policies are applied — their obligations rewrite the query in-flight (row filters, column masks, column access controls). Column denies from both permit and deny policies are combined.
1313
4. The rewritten query executes against the upstream database.
1414

1515
## Policy effects
1616

1717
- **permit** — applies obligations to the query (row filtering, masking, etc.)
18-
- **deny**blocks the query with an error. Useful for blocking access to sensitive datasources entirely.
18+
- **deny**primarily used to block access with an error. Can also carry `column_access deny` obligations to strip specific columns from results.
1919

20-
Deny policies are checked first and short-circuit. If any deny policy matches, the query is rejected immediately.
20+
Deny policies are evaluated before permit policies. If a deny policy has a matching `row_filter` obligation, the query is rejected immediately with an error. `column_access deny` obligations on deny policies strip columns from results just like they do on permit policies — they do not cause an error.
21+
22+
> **`is_enabled` flag**: only enabled policies are enforced. Disabling (or enabling) a policy removes (or adds) all its effects immediately — both for query-time enforcement and for schema visibility — without requiring a reconnect.
2123
2224
## Obligation types
2325

@@ -66,7 +68,9 @@ Removes the specified columns from query results entirely.
6668
}
6769
```
6870

69-
Denied columns are unioned across all matching policies — if any policy denies a column, it is removed.
71+
Denied columns are unioned across all matching **enabled** policies, regardless of effect — if any enabled policy (permit or deny) denies a column, it is removed from the result. The column is also hidden from schema metadata (`information_schema.columns`) on the user's connection.
72+
73+
If the query selects **only** denied columns (e.g. `SELECT ssn FROM customers`), the proxy returns SQLSTATE `42501` (insufficient privilege) with a message identifying the restricted columns rather than returning empty rows.
7074

7175
## Template variables
7276

@@ -127,9 +131,38 @@ Each policy assignment has a `priority` (integer, lower = higher precedence, def
127131
|---|---|
128132
| Multiple permit policies, same table | Row filters are OR'd |
129133
| Multiple column masks, same column | Lowest priority number wins |
130-
| column_access deny from any policy | Column is always removed |
134+
| column_access deny from any enabled policy (permit or deny) | Column is always removed |
131135
| Equal priority, user-specific vs wildcard | User-specific assignment wins |
132136

137+
## Policy design guidelines
138+
139+
### When to create a new policy
140+
141+
- The rules serve different **roles or use cases** (e.g., "admin access", "support read-only")
142+
- You need to **mix/match** these rules across different datasources
143+
- The rules have **different effects** (mixing permit and deny in one policy gets confusing)
144+
145+
### When to add obligations to existing policy
146+
147+
- The rules are **tightly related** to the same purpose
148+
- They always need to **apply together**
149+
- They're for the **same role or user type**
150+
151+
### Practical heuristics
152+
153+
| Scenario | Recommendation |
154+
|----------|----------------|
155+
| "Admins can see everything" | Single policy with multiple obligations |
156+
| "Support can read, but mask SSN" | Two obligations (row_filter + column_mask) in one policy |
157+
| "Finance can see costs, others can't" | Separate policy for cost visibility rules |
158+
| Same rule needed on multiple datasources | Likely a separate policy to reuse |
159+
160+
### General advice
161+
162+
Favor **smaller, composable policies** over monolithic ones. Your system supports policy assignment with priority, so you can layer policies. This makes it easier to debug ("why can't user X see this?") when each policy has a clear, narrow purpose.
163+
164+
Start with simple policies and split them when they become hard to reason about.
165+
133166
## Access mode
134167

135168
Each datasource has an `access_mode`:

0 commit comments

Comments
 (0)