feat(core): add validation rule for the condition syntax check of row-level access controls #1176
Conversation
WalkthroughThis update introduces row-level access control (RLAC) condition syntax validation to the system. It adds new Rust and Python bindings for RLAC-related types and validation logic, exposes these to Python, and integrates a new validation rule in the server. Associated tests are added at both the Rust and Python levels. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Server
participant RLAC Validator (Python)
participant Manifest (Rust)
participant RLAC Validator (Rust)
Client->>API Server: POST /validate/rlac_condition_syntax_is_valid (parameters, manifest)
API Server->>RLAC Validator (Python): validate_rlac_condition_syntax_is_valid(parameters, manifest)
RLAC Validator (Python)->>Manifest (Rust): to_manifest(manifest_str)
Manifest (Rust)-->>RLAC Validator (Python): Manifest object
RLAC Validator (Python)->>Manifest (Rust): get_model(model_name)
Manifest (Rust)-->>RLAC Validator (Python): Model object
RLAC Validator (Python)->>RLAC Validator (Rust): validate_rlac_rule(RLAC, Model)
RLAC Validator (Rust)-->>RLAC Validator (Python): Validation result (Ok/Error)
RLAC Validator (Python)-->>API Server: Success or ValidationError
API Server-->>Client: HTTP 204 (success) or 422 (error)
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
core
ibis
dependencies
goldmedal
force-pushed
the
feat/rls-validation
branch
from
5968f52 to
d146a84
Compare
goldmedal
force-pushed
the
feat/rls-validation
branch
from
d146a84 to
4c159d6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ibis-server/tests/routers/v3/connector/postgres/test_validate.py (1)
124-178: Enhance test coverage for RLAC validation.The test effectively validates basic success and failure cases for RLAC condition syntax validation, including support for both string and boolean formats for the "required" flag. However, there are opportunities to make the tests more comprehensive:
Consider extending the test coverage to include:
- A test case with
"required": trueor"required": Trueto verify that required properties are correctly handled- A test with multiple session properties to ensure all are validated correctly
- A test with invalid condition syntax (e.g., malformed SQL) to verify syntax validation
async def test_validate_rlac_condition_syntax_is_valid( client, manifest_str, connection_info ): # Existing test cases... + # Test with required property set to true + response = await client.post( + url=f"{base_url}/validate/rlac_condition_syntax_is_valid", + json={ + "connectionInfo": connection_info, + "manifestStr": manifest_str, + "parameters": { + "modelName": "orders", + "requiredProperties": [ + {"name": "session_order", "required": True}, + ], + "condition": "@session_order = o_orderkey", + }, + }, + ) + assert response.status_code == 204 + + # Test with multiple session properties + response = await client.post( + url=f"{base_url}/validate/rlac_condition_syntax_is_valid", + json={ + "connectionInfo": connection_info, + "manifestStr": manifest_str, + "parameters": { + "modelName": "orders", + "requiredProperties": [ + {"name": "session_order", "required": "false"}, + {"name": "session_role", "required": "false"}, + ], + "condition": "@session_order = o_orderkey AND @session_role = 'admin'", + }, + }, + ) + assert response.status_code == 204 + + # Test with invalid condition syntax + response = await client.post( + url=f"{base_url}/validate/rlac_condition_syntax_is_valid", + json={ + "connectionInfo": connection_info, + "manifestStr": manifest_str, + "parameters": { + "modelName": "orders", + "requiredProperties": [ + {"name": "session_order", "required": "false"}, + ], + "condition": "@session_order = = o_orderkey", # Invalid syntax + }, + }, + ) + assert response.status_code == 422 + assert "Error during planning: Syntax error" in response.textwren-core-py/tests/test_modeling_core.py (2)
325-336: Good negative test caseThis second test case properly validates error handling when a session property is used in the condition but not declared in the required properties list. The error message assertion should be moved outside the
pytest.raisescontext manager block to ensure it's actually evaluated.with pytest.raises(Exception) as e: validate_rlac_rule(rlac, model) - assert ( - str(e.value) - == "Exception: DataFusion error: Error during planning: The session property @session_user is used, but not found in the session properties" - ) + +assert ( + str(e.value) + == "Exception: DataFusion error: Error during planning: The session property @session_user is used, but not found in the session properties" +)
307-337: Consider adding more test casesThe current tests cover the basic success and failure cases, but could be extended to test other validation scenarios such as:
- Invalid condition syntax
- Referencing columns that don't exist in the model
- Case sensitivity handling for property names
These would provide more comprehensive test coverage of the validation function.
ibis-server/app/model/validator.py (2)
168-176: Consider validating property structureWhile the method converts the incoming properties to SessionProperty objects, it doesn't validate the structure of each property object beforehand. This could lead to runtime errors if the incoming data doesn't match the expected format.
Consider adding validation for each property object before conversion:
for prop in required_properties: if not isinstance(prop, dict): raise ValidationError(f"Property must be an object, got {type(prop)}") if "name" not in prop: raise ValidationError("Property missing required field 'name'") if "required" not in prop: raise ValidationError("Property missing required field 'required'")
189-191: Improve error message handlingThe current implementation passes the exception object directly to
ValidationError, which might result in less informative error messages. Consider extracting and formatting the error message for better clarity.try: validate_rlac_rule(rlac, model) except Exception as e: - raise ValidationError(e) + error_msg = f"Invalid RLAC condition: {str(e)}" + raise ValidationError(error_msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ibis-server/app/model/__init__.py(1 hunks)ibis-server/app/model/validator.py(2 hunks)ibis-server/tests/routers/v3/connector/postgres/test_validate.py(1 hunks)wren-core-base/src/mdl/py_method.rs(3 hunks)wren-core-py/src/errors.rs(1 hunks)wren-core-py/src/extractor.rs(1 hunks)wren-core-py/src/lib.rs(2 hunks)wren-core-py/src/manifest.rs(1 hunks)wren-core-py/src/validation.rs(1 hunks)wren-core-py/tests/test_modeling_core.py(2 hunks)wren-core/core/src/logical_plan/analyze/access_control.rs(3 hunks)wren-core/core/src/logical_plan/analyze/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
wren-core-py/src/extractor.rs (1)
wren-core/core/src/mdl/mod.rs (2)
catalog(266-268)schema(270-272)
ibis-server/tests/routers/v3/connector/postgres/test_validate.py (3)
ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/postgres/test_dry_plan.py (1)
manifest_str(33-34)ibis-server/tests/routers/v3/connector/postgres/conftest.py (1)
connection_info(37-44)
wren-core-py/tests/test_modeling_core.py (8)
wren-core-py/src/manifest.rs (2)
to_json_base64(10-14)to_manifest(18-23)ibis-server/app/mdl/core.py (1)
to_json_base64(17-18)wren-core-py/src/validation.rs (1)
validate_rlac_rule(7-13)wren-core/core/src/logical_plan/analyze/access_control.rs (3)
validate_rlac_rule(82-110)test_validate_rlac_rule(691-756)required_properties(243-260)ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (1)
manifest_str(30-31)wren-core-base/src/mdl/builder.rs (3)
model(68-71)model(288-291)condition(298-301)wren-core-base/src/mdl/py_method.rs (1)
get_model(53-61)wren-core-base/src/mdl/manifest.rs (4)
name(256-258)name(289-291)name(300-302)name(306-308)
wren-core/core/src/logical_plan/analyze/access_control.rs (3)
wren-core-py/src/validation.rs (1)
validate_rlac_rule(7-13)wren-core-base/src/mdl/builder.rs (11)
model(68-71)model(288-291)condition(298-301)new(44-56)new(103-117)new(196-210)new(277-286)new(313-325)new(362-370)new(392-399)new_required(175-181)wren-core-py/tests/test_modeling_core.py (1)
test_validate_rlac_rule(307-336)
🔇 Additional comments (21)
wren-core-py/src/manifest.rs (1)
16-16: Great addition exposing the function to Python!Making the
to_manifestfunction accessible from Python via the#[pyfunction]attribute complements the existingto_json_base64function and provides symmetry in the API. This change enables Python code to easily convert base64-encoded manifest strings to proper Manifest objects, which is essential for the RLAC validation workflow.wren-core/core/src/logical_plan/analyze/mod.rs (1)
1-1: Good visibility change for the access_control moduleMaking the
access_controlmodule public is appropriate since it contains RLAC validation logic that needs to be accessible from other parts of the codebase, particularly through the Python bindings inwren-core-py.ibis-server/app/model/__init__.py (1)
360-360: Appropriate type generalization for validation parametersChanging the
parameterstype from a specificdict[str, str]to a more generaldictis necessary to support the complex nested structures needed for RLAC condition validation, including session properties and condition strings.wren-core-py/src/extractor.rs (1)
59-60: Code readability improvementThe change from
map_or(true, ...)tois_none_or(...)makes the intent of the code clearer while maintaining the same functionality. The filter condition correctly checks if either the table's catalog/schema is None or matches the model's catalog/schema.wren-core-py/src/errors.rs (1)
54-54: Streamlined error handling. LGTM!The change removes the unnecessary "DataFusion error: " prefix, simplifying the error message formatting. Since
DataFusionErrorlikely already contains contextual information, this avoids duplication in error messages.wren-core-py/src/validation.rs (1)
1-14: Well-structured Python binding for RLAC validation. LGTM!The implementation is clean and follows good practices:
- Takes references to input parameters
- Returns a proper
Resulttype for error propagation- Uses the
?operator for concise error handling- Correctly exposes the function to Python via
#[pyfunction]This thin wrapper around the core Rust validation logic provides a clean interface for Python to validate RLAC rules.
wren-core-py/src/lib.rs (3)
10-10: LGTM: Clean module declaration.The new validation module is properly declared following the established pattern in this file.
19-22: LGTM: Appropriate class exports for RLAC.The manifest-related classes needed for RLAC validation are properly exposed to Python.
24-25: LGTM: Properly exposed functions for Python.Both the manifest parser and RLAC validation functions are correctly exposed to Python, enabling the validation rule implementation.
wren-core-py/tests/test_modeling_core.py (2)
6-13: Good additions to the imports listThe new imports clearly indicate the new functionality being tested: row-level access control validation with
RowLevelAccessControl,SessionProperty,to_manifest, andvalidate_rlac_rule.
307-323: Well-structured test setupThe test correctly initializes the test environment by:
- Converting the base64 manifest string to a manifest object
- Retrieving the "customer" model
- Creating a valid RLAC rule with required properties and condition
The condition correctly references the model column "c_name" and session property "@session_user", matching the properties defined in the manifest.
wren-core/core/src/logical_plan/analyze/access_control.rs (2)
78-110: Well-implemented RLAC validation functionThe
validate_rlac_rulefunction is well-designed and follows best practices:
- Clear documentation explaining the purpose
- Proper use of existing functions (
collect_condition) to avoid code duplication- Case-insensitive comparison using
to_lowercase()for property names- Clear error messages that specify which properties are missing
- Efficient filtering using iterators
The function effectively validates that all session properties referenced in the condition are declared in the required properties list.
690-756: Comprehensive test coverageThe
test_validate_rlac_rulefunction provides excellent test coverage, including:
- Valid rules with matching session properties (both single and multiple properties)
- Invalid rules with missing session properties
- Invalid rules with syntax errors
- Invalid rules referencing non-existent columns
Each test case has clear assertions for the expected behavior and error messages.
wren-core-base/src/mdl/py_method.rs (4)
22-22: Appropriate import for new typesThe import statement correctly adds
RowLevelAccessControlandSessionPropertytypes from the manifest module, which will be exposed to Python.
53-61: Useful model retrieval methodThe
get_modelmethod provides a convenient way to retrieve a specific model by name from a manifest. It correctly:
- Searches through the models collection
- Uses
findfor efficient lookup- Handles cloning of the Arc-wrapped model
- Returns an Option to handle the case where the model is not found
This method is essential for the validation workflow where a specific model needs to be validated.
73-83: Good SessionProperty Python bindingThe Python binding for SessionProperty is well-implemented with:
- A clear constructor with appropriate default values
- Support for both required and optional parameters
- Support for optional default expressions
This allows Python code to easily create and manipulate SessionProperty objects.
86-96: Well-defined RowLevelAccessControl Python bindingThe Python binding for RowLevelAccessControl is properly implemented with:
- A clear constructor with appropriate default values
- Support for empty required_properties list by default
- All essential fields exposed to Python
This enables Python code to create RLAC objects for validation purposes.
ibis-server/app/model/validator.py (4)
3-8: Appropriate imports for RLAC validationThe imports clearly bring in all the necessary components for RLAC validation:
RowLevelAccessControlandSessionPropertyfor creating validation objectsto_manifestfor converting manifest strings to objectsvalidate_rlac_rulefor performing the validationThese imports support the new validation functionality.
15-15: Good addition to the validation rules listAdding "rlac_condition_syntax_is_valid" to the rules list makes the new validation rule available to the validation framework.
23-23: Necessary type hint generalizationChanging the type hint from
dict[str, str]todictis appropriate since the RLAC validation needs to handle more complex parameter structures like nested objects in therequiredPropertiesfield.
154-192: Well-implemented RLAC validation methodThe
_validate_rlac_condition_syntax_is_validmethod is well-structured and follows the pattern of other validation methods:
- Checks for required parameters with clear error messages
- Extracts the necessary values from the parameters
- Constructs the required objects (SessionProperty and RowLevelAccessControl)
- Converts the manifest string and retrieves the model
- Performs the validation with proper error handling
The implementation correctly transforms the API parameters into the objects needed for validation and propagates errors appropriately.
Description
Wait #1161
Add the rule to check if the condition of RLAC is valid.
URL
Sample Payload
{ "manifest_str": "...", "modelName": "orders", "requiredProperties": [ { "name": "@SESSION_LEVEL", "required": true }, { "name": "@SESSION_ROLE", "required": true }, { "name": "@SESSION_REGION", "required": true } ], "condition": "level = @SESSION_LEVEL AND role = @SESSION_ROLE OR region = @SESSION_REGION" } <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added validation for row-level access control (RLAC) condition syntax, ensuring all session properties referenced in conditions are properly declared. - Introduced new Python and Rust APIs to support RLAC rule validation and model access. - Expanded validation endpoints and test coverage for RLAC scenarios. - **Bug Fixes** - Improved error messages for validation failures related to missing session properties in RLAC conditions. - **Tests** - Added comprehensive tests for RLAC rule validation, covering both valid and invalid scenarios. - **Documentation** - Enhanced Python bindings documentation by exposing new classes and functions for RLAC validation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->