Skip to content

Commit 4e5b464

Browse files
Add RLS to Migration framework (closes #32, #36, #37) (#38)
## Summary - Implements platform-independent row-level security in the Migration framework — single YAML schema produces native `CREATE POLICY` on Postgres + trigger-based emulation on SQLite - Closes NAP Tier 1 blockers: declarative RLS (#32), raw-SQL escape hatch for SECURITY DEFINER functions (#36), `FORCE ROW LEVEL SECURITY` (#37) - 73 new RLS tests passing locally including **9 EXTREME end-to-end NAP-shape tests** that prove tenant isolation, idempotent re-apply, drift detection with rename, and 100-row cross-tenant stress against a real Postgres testcontainer ## What ships **Core (`Nimblesite.DataProvider.Migration.Core`)** - `RlsPolicySetDefinition` — `Enabled`, `Forced`, `Policies` - `RlsPolicyDefinition` — LQL via `UsingLql`/`WithCheckLql`, raw-SQL escape hatch via `UsingSql`/`WithCheckSql` (#36), `IsPermissive`, `Operations`, `Roles` - `RlsOperation` enum — `All`/`Select`/`Insert`/`Update`/`Delete` - 6 schema operations: `EnableRlsOperation`, `EnableForceRlsOperation` (#37), `CreateRlsPolicyOperation`, `DropRlsPolicyOperation`, `DisableRlsOperation`, `DisableForceRlsOperation` — last three destructive - `RlsPredicateTranspiler` — substitutes `current_user_id()` per platform; for `exists(pipeline)` LQL, delegates the inner pipeline to `LqlStatementConverter` and wraps the transpiled SQL with `EXISTS (...)` - `SchemaDiff` extended for RLS — emits Enable/Force ops for new RLS state, `CreatePolicy` for new policies, drops/disable when `allowDestructive=true`. Forward-only by default; orphan drift cleanup is opt-in - YAML round-trippable with `[YamlMember]` aliases for `using`/`withCheck`/`usingSql`/`withCheckSql`/`permissive`/`forced` - Error codes: `MIG-E-RLS-EMPTY-PREDICATE`, `-EMPTY-CHECK`, `-LQL-PARSE`, `-LQL-TRANSPILE`, `-MSSQL-UNSUPPORTED`, `-RAW-SQL-UNSUPPORTED-ON-PLATFORM`, `-FORCE-UNSUPPORTED-ON-PLATFORM` **Postgres (`Nimblesite.DataProvider.Migration.Postgres`)** - Native `CREATE POLICY` with `PERMISSIVE`/`RESTRICTIVE`, `FOR ALL|SELECT|INSERT|UPDATE|DELETE`, `TO PUBLIC|roles`, `USING`/`WITH CHECK` - Inspector reads `pg_class.relrowsecurity` + `pg_class.relforcerowsecurity` + `pg_policies.qual`/`with_check` into `RlsPolicySetDefinition`. Predicates round-trip as raw SQL (we don't attempt SQL→LQL reverse mapping) **SQLite (`Nimblesite.DataProvider.Migration.SQLite`)** - `__rls_context` single-row context table auto-generated on first `EnableRlsOperation` - Trigger emulation: `BEFORE INSERT/UPDATE/DELETE` triggers with `RAISE(ABORT, 'RLS-SQLITE: ...')` evaluating predicate against `NEW`/`OLD` rows - `{Tbl}_secure` view filtering `SELECT` (SQLite triggers don't intercept `SELECT`) - RESTRICTIVE policies emit `MIG-W-RLS-SQLITE-RESTRICTIVE-APPROX` warning comment - Inspector reverse-maps `rls_*_{Table}` triggers → `RlsPolicySetDefinition` ## Test plan **73 new RLS tests, all green locally:** - [x] 8 YAML round-trip (`RlsYamlSerializerTests`) - [x] 13 transpiler unit — per-platform `current_user_id` substitution, `exists()` subquery wrapping, parse/transpile error paths (`RlsPredicateTranspilerTests`) - [x] 7 error code shape (`RlsErrorCodesTests`) - [x] 11 Postgres DDL string-shape (`PostgresRlsDdlTests`) - [x] 8 SchemaDiff RLS unit (`SchemaDiffRlsTests`) - [x] 8 SQLite RLS DDL + inspector E2E (`SqliteRlsMigrationTests`) - [x] 6 Postgres RLS E2E with real testcontainer + NOBYPASSRLS app role (`PostgresRlsE2ETests`) — cross-user blocked, INSERT WITH CHECK enforced, inspector round-trip, schema diff add/drop - [x] **9 EXTREME NAP-shape E2E** (`PostgresRlsNapShapeTests`) — 4 tenant tables × 2 policies all FORCE'd, cross-tenant isolation, admin role sees everything, idempotent re-apply emits ZERO ops, drift rename drops 4 + creates 4, OR-combination predicates round-trip, `DropForceRls` requires `allowDestructive`, 100-row stress with exact per-tenant counts **Local CI dry-run all green:** - [x] `make fmt-check` (csharpier 434 files + cargo fmt) - [x] `make lint` (analyzers + clippy + eslint) - [x] `dotnet build DataProvider.sln -c Debug` — 0 warnings, 0 errors - [x] `make _test_dotnet` for Migration tests — 363/363 pass, coverage 77.99% (ratcheted from 74% → 77%) - [x] `cd Lql/lql-lsp-rust && cargo build` ## NAP integration NapSupport2 has been notified via TMC. Local nupkgs packed at `/tmp/nap-rls-nuget` as `0.1.0-rls.preview1` so NAP can pin against their feed while this lands on main. Comments posted on issues #32 / #36 / #37. ## Out of scope - CREATE FUNCTION / CREATE ROLE / GRANT (#33/#34/#35) — NAP's bootstrap retains these - SQL Server implementation — `MIG-E-RLS-MSSQL-UNSUPPORTED` until `Nimblesite.DataProvider.Migration.SqlServer` ships 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 parent 84f5b2a commit 4e5b464

32 files changed

Lines changed: 5626 additions & 80 deletions

Lql/Nimblesite.Lql.Core/Parsing/LqlToAstVisitor.cs

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,19 @@ private static string ProcessComparisonToSql(
355355
{
356356
return ProcessCaseExpressionToSql(expr.caseExpr(), lambdaScope);
357357
}
358+
359+
// expr matched the `IDENT '(' argList? ')'` branch — a bare
360+
// function call (e.g. is_member(u, t)) used as a boolean predicate
361+
// inside a lambda body without an explicit comparison operator.
362+
// GitHub issue #40: NAP needs SECURITY DEFINER fn calls to
363+
// survive LQL transpilation; rewriting via exists() loses
364+
// SECURITY DEFINER semantics. Emit the call verbatim so RLS
365+
// policy bodies can call user-defined Postgres functions.
366+
if (expr.IDENT() != null && expr.ChildCount >= 3)
367+
{
368+
return ProcessFnCallExprToSql(expr, lambdaScope);
369+
}
370+
358371
// No fallback - fail hard if expr type is not handled
359372
throw new SqlErrorException(
360373
CreateSqlErrorStatic(
@@ -706,6 +719,101 @@ context as ParserRuleContext
706719
);
707720
}
708721

722+
/// <summary>
723+
/// Process an expr that matched the <c>IDENT '(' argList? ')'</c> branch
724+
/// (a function call) into SQL text. Lambda-scope-aware so qualified
725+
/// idents like <c>p.tenant_id</c> strip the <c>p.</c> prefix when
726+
/// <c>p</c> is bound. Implements GitHub issue #40 — RLS policies must
727+
/// be able to call user-defined SECURITY DEFINER functions verbatim.
728+
/// </summary>
729+
private static string ProcessFnCallExprToSql(
730+
LqlParser.ExprContext expr,
731+
HashSet<string>? lambdaScope
732+
)
733+
{
734+
var fnName = expr.IDENT().GetText();
735+
var args = expr.argList();
736+
if (args == null)
737+
{
738+
return $"{fnName}()";
739+
}
740+
var argTexts = args.arg().Select(a => ProcessFnCallArgToSql(a, lambdaScope)).ToList();
741+
return $"{fnName}({string.Join(", ", argTexts)})";
742+
}
743+
744+
private static string ProcessFnCallArgToSql(
745+
LqlParser.ArgContext arg,
746+
HashSet<string>? lambdaScope
747+
)
748+
{
749+
// arg grammar matches `columnAlias` first when the arg is a
750+
// qualified identifier like c.tenant_id. The columnAlias rule
751+
// itself wraps qualifiedIdent / IDENT / arithmeticExpr / functionCall.
752+
// Walk through to find the actual shape and apply lambda-scope
753+
// stripping for qualified idents.
754+
if (arg.columnAlias() != null)
755+
{
756+
var ca = arg.columnAlias();
757+
if (ca.qualifiedIdent() != null)
758+
{
759+
return ProcessQualifiedIdentifierToSql(ca.qualifiedIdent(), lambdaScope);
760+
}
761+
if (ca.arithmeticExpr() != null)
762+
{
763+
return ProcessArithmeticExpressionToSql(ca.arithmeticExpr(), lambdaScope);
764+
}
765+
if (ca.functionCall() != null)
766+
{
767+
return ExtractFunctionCall(ca.functionCall());
768+
}
769+
if (ca.IDENT() != null && ca.IDENT().Length > 0)
770+
{
771+
return ca.IDENT()[0].GetText();
772+
}
773+
}
774+
if (arg.arithmeticExpr() != null)
775+
{
776+
return ProcessArithmeticExpressionToSql(arg.arithmeticExpr(), lambdaScope);
777+
}
778+
if (arg.functionCall() != null)
779+
{
780+
return ExtractFunctionCall(arg.functionCall());
781+
}
782+
if (arg.expr() != null)
783+
{
784+
var inner = arg.expr();
785+
if (inner.IDENT() != null && inner.ChildCount >= 3)
786+
{
787+
return ProcessFnCallExprToSql(inner, lambdaScope);
788+
}
789+
if (inner.qualifiedIdent() != null)
790+
{
791+
return ProcessQualifiedIdentifierToSql(inner.qualifiedIdent(), lambdaScope);
792+
}
793+
if (inner.IDENT() != null)
794+
{
795+
return inner.IDENT().GetText();
796+
}
797+
if (inner.STRING() != null)
798+
{
799+
return inner.STRING().GetText();
800+
}
801+
if (inner.INT() != null)
802+
{
803+
return inner.INT().GetText();
804+
}
805+
if (inner.DECIMAL() != null)
806+
{
807+
return inner.DECIMAL().GetText();
808+
}
809+
}
810+
if (arg.comparison() != null)
811+
{
812+
return ProcessComparisonToSql(arg.comparison(), lambdaScope);
813+
}
814+
return ExtractIdentifier(arg);
815+
}
816+
709817
/// <summary>
710818
/// Processes a qualified identifier to SQL text, removing lambda variable prefixes.
711819
/// </summary>
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
using Nimblesite.Lql.Postgres;
2+
using Nimblesite.Sql.Model;
3+
using Xunit;
4+
5+
namespace Nimblesite.Lql.Tests;
6+
7+
// Coverage for ProcessFnCallExprToSql + ProcessFnCallArgToSql added in
8+
// LqlToAstVisitor for GitHub issues #40/#41 (NAP RLS bare fn calls in
9+
// lambda bodies, e.g. exists(parent |> filter(fn(p) => p.id = id and
10+
// is_member(app_user_id(), p.tenant_id)))).
11+
12+
/// <summary>
13+
/// Targeted unit tests for the bare-function-call branch of LQL's lambda
14+
/// body and its argument-shape handling. These shapes are not exercised by
15+
/// the file-based fixture tests but are required for RLS predicates that
16+
/// call SECURITY DEFINER functions.
17+
/// </summary>
18+
public sealed class LqlFnCallInLambdaTests
19+
{
20+
private static string ToPg(string lql)
21+
{
22+
var stmt = LqlStatementConverter.ToStatement(lql);
23+
Assert.True(
24+
stmt is Outcome.Result<LqlStatement, SqlError>.Ok<LqlStatement, SqlError>,
25+
stmt is Outcome.Result<LqlStatement, SqlError>.Error<LqlStatement, SqlError> e
26+
? e.Value.Message
27+
: "expected Ok"
28+
);
29+
var ok = (Outcome.Result<LqlStatement, SqlError>.Ok<LqlStatement, SqlError>)stmt;
30+
var result = ok.Value.ToPostgreSql();
31+
Assert.True(
32+
result is Outcome.Result<string, SqlError>.Ok<string, SqlError>,
33+
result is Outcome.Result<string, SqlError>.Error<string, SqlError> e2
34+
? e2.Value.Message
35+
: "expected transpile Ok"
36+
);
37+
return ((Outcome.Result<string, SqlError>.Ok<string, SqlError>)result).Value;
38+
}
39+
40+
[Fact]
41+
public void Lambda_BareFnCall_NoArgs_PassesThrough()
42+
{
43+
var sql = ToPg("t |> filter(fn(x) => some_fn())");
44+
Assert.Contains("some_fn()", sql, StringComparison.Ordinal);
45+
}
46+
47+
[Fact]
48+
public void Lambda_BareFnCall_StringArgs_PassesThrough()
49+
{
50+
var sql = ToPg("t |> filter(fn(x) => is_member('a', 'b'))");
51+
Assert.Contains("is_member('a', 'b')", sql, StringComparison.Ordinal);
52+
}
53+
54+
[Fact]
55+
public void Lambda_BareFnCall_QualifiedIdentArg_StripsLambdaPrefix()
56+
{
57+
// x is the lambda var -> x.tenant_id should emit as 'tenant_id'.
58+
var sql = ToPg("t |> filter(fn(x) => is_member('u', x.tenant_id))");
59+
Assert.Contains("is_member('u', tenant_id)", sql, StringComparison.Ordinal);
60+
Assert.DoesNotContain("x.tenant_id", sql, StringComparison.Ordinal);
61+
}
62+
63+
[Fact]
64+
public void Lambda_BareFnCall_NestedFnCallArg_PassesThrough()
65+
{
66+
var sql = ToPg("t |> filter(fn(x) => is_member(app_user_id(), app_tenant_id()))");
67+
// Outer fn is emitted via ProcessFnCallExprToSql (lowercase preserved).
68+
// Nested fn args go through ExtractFunctionCall which uppercases the
69+
// function name -- Postgres treats unquoted names as case-insensitive
70+
// so APP_USER_ID() and app_user_id() resolve to the same function.
71+
Assert.Contains("is_member(", sql, StringComparison.Ordinal);
72+
Assert.Contains("APP_USER_ID()", sql, StringComparison.Ordinal);
73+
Assert.Contains("APP_TENANT_ID()", sql, StringComparison.Ordinal);
74+
}
75+
76+
[Fact]
77+
public void Lambda_AndCombinationWithBareFnCall_ParsesAndEmits()
78+
{
79+
// The right-hand side of AND is a bare fn call -- must not raise
80+
// 'Unsupported expr type in comparison'.
81+
var sql = ToPg(
82+
"t |> filter(fn(x) => x.id = '00000000-0000-0000-0000-000000000000' and is_member('u', x.tenant_id))"
83+
);
84+
Assert.Contains("AND", sql, StringComparison.Ordinal);
85+
Assert.Contains("is_member(", sql, StringComparison.Ordinal);
86+
}
87+
88+
[Fact]
89+
public void Lambda_OrCombinationWithBareFnCall_ParsesAndEmits()
90+
{
91+
var sql = ToPg(
92+
"t |> filter(fn(x) => x.id = '00000000-0000-0000-0000-000000000000' or is_member('u', x.tenant_id))"
93+
);
94+
Assert.Contains("OR", sql, StringComparison.Ordinal);
95+
Assert.Contains("is_member(", sql, StringComparison.Ordinal);
96+
}
97+
98+
[Fact]
99+
public void Lambda_BareFnCall_IntArg_PassesThrough()
100+
{
101+
var sql = ToPg("t |> filter(fn(x) => has_role(42))");
102+
Assert.Contains("has_role(42)", sql, StringComparison.Ordinal);
103+
}
104+
105+
[Fact]
106+
public void Lambda_BareFnCall_DecimalArg_PassesThrough()
107+
{
108+
var sql = ToPg("t |> filter(fn(x) => has_balance(1.5))");
109+
Assert.Contains("has_balance(1.5)", sql, StringComparison.Ordinal);
110+
}
111+
112+
[Fact]
113+
public void Lambda_BareFnCall_IdentArg_PassesThrough()
114+
{
115+
var sql = ToPg("t |> filter(fn(x) => some_fn(other_col))");
116+
Assert.Contains("some_fn(", sql, StringComparison.Ordinal);
117+
Assert.Contains("other_col", sql, StringComparison.Ordinal);
118+
}
119+
}

0 commit comments

Comments
 (0)