Address type mismatch false positive by implementing context validation #1651
Conversation
mishjude
force-pushed
the
context_validation
branch
from
3f8386e to
c855834
Compare
Signed-off-by: Mish Jude <[email protected]>
mishjude
force-pushed
the
context_validation
branch
from
c855834 to
f13bd28
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 71.83% Status: FAILED ❌ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.90% Status: PASSED ✅ Details
|
| action: &ast::EntityUID, | ||
| extensions: &Extensions<'a>, | ||
| ) -> std::result::Result<(), RequestValidationError> { | ||
| // Following the same logic in validate_request |
There was a problem hiding this comment.
I think this comment is outdated now that validate_request calls this function
| ) -> std::result::Result<(), RequestValidationError> { | ||
| // Following the same logic in validate_request | ||
| // Get the action ID | ||
| let action_arc = Arc::new(action.clone()); |
There was a problem hiding this comment.
can we avoid cloning the action and constructing an Arc on the happy path? I think the clone and Arc are only needed on error paths
cedar-policy/src/api.rs
Outdated
| }; | ||
| use cedar_policy_core::validator::json_schema; | ||
| use cedar_policy_core::validator::typecheck::{PolicyCheck, Typechecker}; | ||
| pub use err::RequestValidationError; |
There was a problem hiding this comment.
Why is this necessary? I think there is already a pub use err::* below?
…into context_validation
…into context_validation
| } | ||
| })?; | ||
| let validator_action_id = | ||
| self.get_action_id(&Arc::new(action.clone())) |
There was a problem hiding this comment.
Can this just be let validator_action_id = self.get_action_id(action), or does that not work?
There was a problem hiding this comment.
Yeah, I think this should work
There was a problem hiding this comment.
noted! made that change!
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 73.13% Status: FAILED ❌ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.89% Status: PASSED ✅ Details
|
…into context_validation
…into context_validation
…into context_validation
…into context_validation
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 70.15% Status: FAILED ❌ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.88% Status: PASSED ✅ Details
|
…on (cedar-policy#1651) Fix sign-off issue Signed-off-by: Mish Jude <[email protected]> Co-authored-by: Mish Jude <[email protected]> Signed-off-by: Mish Jude <[email protected]>
…on (#1651) Signed-off-by: Mish Jude <[email protected]> Co-authored-by: Mish Jude <[email protected]>
Description of changes
Address type mismatch false positive for context validation by:
-Creating a new method validate_context to determine whether a context is valid against the provided schema and action
-Calling validate_context in check_parse_context and returned CheckParseAnswer::failure if an error was returned
-Writing a test case to address a faulty context situation and ensuring it passes locally
Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy.I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):(I am like 70% sure the documentation was written under the assumption that the issue I was trying to fix was not an issue (and that context validation worked as intended), so I don't think the documentation needs to be updated, but I'm also not 100% sure)