DEV-1718: refactor RLS SessionPolicy into a single ruleset - #260
Merged
Conversation
Replace SessionPolicy.data_filters (a list of ColumnFilterRule/JoinFilterRule with a mandatory block-backstop) with a single required ruleset that is one of two mutually-exclusive kinds: - ColumnFilterRuleset: the broadcast column filter (mirrors the old single ColumnFilterRule; on_unapplicable governs a missing column; has_column probe is now used only on this branch). - JoinFilterRuleset: a single-anchor model (table+column+value) with explicit JoinFilterRules and a whitelist. Classification is fully structural/DB-free: anchor -> direct wrap, target -> correlated EXISTS, whitelist -> passthrough, anything else -> fail closed (the whitelist replaces the block-backstop). Nested JoinFilterRule drops column/value/name; join_path may begin OR end with the anchor (oriented_hops normalizes to target-first); the anchor must appear exactly once in the oriented path. The kind discriminator is explicit (no inference). No name attribute anywhere; ForcedFilterError drops rule_name. Hard break: no data_filters shim. Updates the SQL rewrite, engine wiring (_apply_policy / _policy_has_join_rules), all policy tests, the RLS concept doc + notebook, CLAUDE.md and the overview skill. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces ChangesRow-level security ruleset migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant QueryEngine
participant SessionPolicy
participant apply_session_policy
participant SQLAst
QueryEngine->>SessionPolicy: provide ruleset
QueryEngine->>apply_session_policy: rewrite final SQL
apply_session_policy->>SQLAst: inspect physical tables
apply_session_policy->>SQLAst: emit column predicates or correlated EXISTS
SQLAst-->>QueryEngine: return scoped SQL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
slayer/sql/session_policy.py (1)
194-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold
_terminal_predicateinto_build_predicate.The two builders differ only by the table qualifier on the column.
♻️ Single predicate builder
-def _build_predicate(column: str, value) -> exp.Expression: - """Unqualified ``column = value`` / ``column IN (...)`` predicate.""" - col = exp.column(column) +def _build_predicate(column: str, value, *, table: Optional[str] = None) -> exp.Expression: + """``column = value`` / ``column IN (...)``, optionally qualified by + ``table`` — values always via ``exp.convert`` (injection-safe).""" + col = exp.column(column, table=table) if table else exp.column(column) if isinstance(value, tuple): return exp.In(this=col, expressions=[exp.convert(v) for v in value]) return exp.EQ(this=col, expression=exp.convert(value))Then call
_build_predicate(ruleset.column, ruleset.value, table=_hop_alias(len(hops) - 1))at line 245 and drop_terminal_predicate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/sql/session_policy.py` around lines 194 - 200, Optionally remove the redundant _terminal_predicate helper and extend _build_predicate to accept the table qualifier needed for its column expression. Update the terminal predicate construction near the ruleset-building flow to call _build_predicate with ruleset.column, ruleset.value, and _hop_alias(len(hops) - 1), preserving the existing scalar and tuple/IN behavior.slayer/core/policy.py (1)
346-383: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnchor-target guard ordering makes its error message unreachable, and the test can't tell. In
_validate_anchor_reachabilitythe path checks run before the "a join rule may not target the anchor" check, so a rule targeting the anchor trips the "anchor must appear exactly once" branch instead and reports a misleading reason. Both sites still fail closed — this is diagnostics quality plus test precision.
slayer/core/policy.py#L346-L383: hoist the_table_names_match(rule.target_table, master)guard to the top of thefor rule in self.joinsloop, beforerule.oriented_hops().tests/test_policy_models.py#L528-L538: assert on the message (e.g.pytest.raises(ValidationError, match="may not target the anchor")) so the test pins the intended guard rather than anyValidationError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/core/policy.py` around lines 346 - 383, The anchor-target validation in _validate_anchor_reachability must run before path orientation and reachability checks so it produces the intended “may not target the anchor” error. Update slayer/core/policy.py lines 346-383 by moving that guard to the start of the loop, and update tests/test_policy_models.py lines 528-538 to assert ValidationError with a message matching “may not target the anchor”.tests/test_session_policy_engine.py (1)
394-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
_ch_ds()outside thepytest.raisesblock.Matches the SonarCloud finding: two call expressions (
_ch_ds()and_apply_policy(...)) sit inside thewith pytest.raises(...)block. The very next test (test_apply_policy_join_rule_ok_when_version_supported) already follows the safer pattern of resolvingdsbeforehand.♻️ Proposed fix
def test_apply_policy_join_rule_fails_closed_when_version_unknown( join_engine, monkeypatch ): monkeypatch.setattr(join_engine, "_column_present", lambda **k: True) + ds = _ch_ds() with pytest.raises(ForcedFilterError): join_engine._apply_policy( - sql="SELECT * FROM orders", dialect="clickhouse", datasource=_ch_ds() + sql="SELECT * FROM orders", dialect="clickhouse", datasource=ds )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_session_policy_engine.py` around lines 394 - 401, Extract the _ch_ds() call into a ds variable before the pytest.raises block in test_apply_policy_join_rule_fails_closed_when_version_unknown, then pass ds to _apply_policy inside the block. Keep the existing assertion and policy invocation behavior unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/examples/10_row_level_security/row_level_security_nb.ipynb`:
- Around line 17-19: Update the introductory notebook text near the
SessionPolicy example to clarify that the column ruleset is demonstrated first,
followed later by a separate join policy/engine for the customers table. Replace
the sequential “First ... then ...” wording with language that does not imply
both rulesets are composed in one SessionPolicy.
---
Nitpick comments:
In `@slayer/core/policy.py`:
- Around line 346-383: The anchor-target validation in
_validate_anchor_reachability must run before path orientation and reachability
checks so it produces the intended “may not target the anchor” error. Update
slayer/core/policy.py lines 346-383 by moving that guard to the start of the
loop, and update tests/test_policy_models.py lines 528-538 to assert
ValidationError with a message matching “may not target the anchor”.
In `@slayer/sql/session_policy.py`:
- Around line 194-200: Optionally remove the redundant _terminal_predicate
helper and extend _build_predicate to accept the table qualifier needed for its
column expression. Update the terminal predicate construction near the
ruleset-building flow to call _build_predicate with ruleset.column,
ruleset.value, and _hop_alias(len(hops) - 1), preserving the existing scalar and
tuple/IN behavior.
In `@tests/test_session_policy_engine.py`:
- Around line 394-401: Extract the _ch_ds() call into a ds variable before the
pytest.raises block in
test_apply_policy_join_rule_fails_closed_when_version_unknown, then pass ds to
_apply_policy inside the block. Keep the existing assertion and policy
invocation behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 046d8b15-2dce-4043-bfc1-49357e3c78b6
📒 Files selected for processing (15)
.claude/skills/slayer-overview.mdCLAUDE.mddocs/concepts/row-level-security.mddocs/examples/10_row_level_security/row_level_security_nb.ipynbdocs/getting-started/python.mdslayer/core/errors.pyslayer/core/policy.pyslayer/engine/query_engine.pyslayer/sql/session_policy.pytests/integration/test_integration_rls.pytests/test_client_policy.pytests/test_policy_models.pytests/test_query_cache.pytests/test_session_policy.pytests/test_session_policy_engine.py
- Codex (major): reject a qualified anchor reached via a less-qualified join-path endpoint (_reaches_anchor), so a qualified anchor's schema is never dropped from the emitted EXISTS. Add construction tests. - Codex (minor): _build_exists wraps oriented_hops() ValueError as a fail-closed ForcedFilterError at the SQL boundary. Add a corrupted-copy test. - Sonar S5778 (15): hoist inline factory/constructor calls out of every pytest.raises block so each has exactly one throwing call. - CodeRabbit: fold _terminal_predicate into _build_predicate(table=...); reword the RLS notebook intro to clarify the two rulesets are shown separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The SQL-boundary fail-closed check in _build_exists used _table_names_match, which would accept a model_copy-corrupted qualified anchor reached via a bare terminal (dropping the schema qualifier). Use _reaches_anchor to mirror the construction-time validator so such a corrupted policy fails closed instead of emitting the tenant predicate against the wrong-schema table. Add a pinning test. (Codex round-2 finding.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…QL boundary Codex round-3: _build_exists re-checked reachability but not the "anchor appears exactly once" invariant, so a model_copy-corrupted anchor-as-intermediate path could slip past the SQL boundary and emit a mis-scoped EXISTS. Extract _validate_join_rule_anchor (reachability + anchor-exactly-once + not-targeting-anchor, returning oriented hops) as the single source of truth, called by both the JoinFilterRuleset validator and _build_exists (wrapping ValueError as a fail-closed ForcedFilterError). No invariant drift between the two layers. Add a pinning test for the corrupted anchor-as-intermediate case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolves the CLAUDE.md conflict: main decluttered CLAUDE.md and moved the design-decision log into DECISIONS.md, deleting the detailed RLS bullets this branch had rewritten. Took main's CLAUDE.md and recorded the DEV-1718 ruleset redesign as a new appended DECISIONS.md entry instead.
Cut the RLS policy/rewrite docs down to what a reader needs to orient quickly, leaving the details to the code. Drops the rationale essays, the restated mechanism at every call site, and the ticket/review references.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces
SessionPolicy.data_filters(a list ofColumnFilterRule/JoinFilterRuleplus a mandatory block-backstop) with a single requiredruleset, one of two mutually-exclusive kinds. Hard break — nodata_filtersshim (SessionPolicyis engine-init state only, never persisted).ColumnFilterRuleset— the broadcast column filter (mirrors the old singleColumnFilterRule:column/value/on_unapplicable). Thehas_columnintrospection probe is now used only on this branch.JoinFilterRuleset— a single-anchor model: onetable+column+valueholds the tenant identifier; nestedJoinFilterRules reach it via explicit join paths (correlatedEXISTS); awhitelistnames shared pass-through tables. Classification is fully structural / DB-free: anchor → direct wrap, target → EXISTS, whitelist → passthrough, anything else → fail closed (the whitelist replaces the old block-backstop).Key design points
JoinFilterRuledropscolumn/value/name(hoisted to the ruleset).join_pathmay begin OR end with the anchor;oriented_hops()normalizes to target-first. The anchor must appear exactly once in the oriented path (rejects master-as-intermediate)._build_existsre-checks the oriented terminal reaches the anchor (defensive vs a corruptmodel_copy).kinddiscriminator is explicit — no inference; a kind-less dict ruleset raises.nameattribute anywhere;ForcedFilterErrordropsrule_name.rulesetis required — no-filtering ispolicy=None, so a bareSessionPolicy()raises.Process
Spec + Codex plan review (round 3) + Codex test-coverage review (Step 5) were completed before implementation; all Codex findings were folded in or documented.
Tests
ruffclean.test_policy_models.py,test_session_policy.py,test_session_policy_engine.py; updatedtest_client_policy.py,test_query_cache.py,test_integration_rls.py.Docs
docs/concepts/row-level-security.mdrewritten (two ruleset kinds + Trust-model note; backstop/override sections removed).docs/getting-started/python.md,CLAUDE.md,.claude/skills/slayer-overview.md, and the RLS notebook updated.Closes DEV-1718.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Breaking Changes
ruleset.Documentation