Add datetime and duration attribute support#2
Merged
Conversation
Re-snapshot src/cedar/ from upstream tag v0.3.0 (4f120bd1d1c05868b09e307f5636c5d52a604a2b), applying the established mechanical rewrites (nxe_cedar_ -> php_cedar_, ngx_ types/functions -> php_cedar_compat equivalents). No new ngx dependencies are introduced, so php_cedar_compat.* and config.m4 are unchanged. Brings in the datetime / duration extension types, bare-context record materialization, attribute/has/in resolution on request entity literals, total == / != type-mismatch evaluation, lexer error-path initialization, a make_ip NULL guard, and const-qualified input APIs.
Wire the "datetime" and "duration" union kinds into the three AttributeValue injection paths (top-level eval context attribute, record member, set element), mirroring the existing "decimal" handling. Values are materialized at insertion time; a malformed value makes the injection API return an error, so the entry is skipped and surfaced in the response errors[] just like any other unsupported AttributeValue.
Move datetime / duration out of the Unsupported features list, add them to the AttributeValue union examples, and update the malformed-value note to reference a genuinely unsupported case.
Add a test exercising datetime / duration through the top-level, record-member, and set-element paths plus duration method dispatch. Update the malformed-attribute test to use an unknown union key and an invalid datetime string now that valid datetime values are accepted.
📝 WalkthroughWalkthroughThis PR extends the PHP Cedar extension with full support for ChangesDateTime and Duration Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/019-isauthorized-attr-malformed.phpt (1)
17-21: ⚡ Quick winKeep one malformed
datetimecase on thecontext.contextMappath.This change drops direct coverage of invalid datetime parsing for context attributes, so the new context-side datetime route is no longer exercised in the malformed test. I'd keep one bad
datetimeentry here in addition to the unsupported/empty cases.📌 Suggested test tweak
"context" => ["contextMap" => [ "okScalar" => ["string" => "hello"], "badUnknown" => ["mysteryType" => "whatever"], + "badDatetime" => ["datetime" => "not-a-valid-datetime"], "badEmpty" => [], ]],🤖 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/019-isauthorized-attr-malformed.phpt` around lines 17 - 21, Keep one malformed datetime case inside the context.contextMap array so the context-side datetime parsing route is exercised; specifically, add a key (e.g., "badDatetime") under the "context" => ["contextMap" => ...] structure with a datetime-typed attribute containing an invalid value (for example a non-ISO string) in addition to the existing "badUnknown" and "badEmpty" entries so the malformed datetime path is still covered by the test.tests/036-isauthorized-attr-datetime-duration.phpt (1)
11-54: ⚡ Quick winAdd one positive
entities.entityListcase for the new kinds.This test proves the context path, but it never asserts that valid
datetime/durationvalues survive theentities.entityListroute. Since that is a separate public input shape, a small positive case here would tighten end-to-end coverage for the PHP integration layer.🧪 Minimal follow-up case
/* duration methods dispatch on the injected value. */ $m = new Cedar\PolicyStore("dur"); $m->loadString("p1", 'permit(principal, action, resource) when { context.window.toHours() == 1 };'); $cm = new Cedar\AuthorizationClient($m); echo "method:", $cm->isAuthorized([ @@ ])["decision"], PHP_EOL; + +$e = new Cedar\PolicyStore("ent"); +$e->loadString("p1", + 'permit(principal, action, resource) when { principal.createdAt < datetime("2026-06-01T00:00:00Z") && principal.ttl == duration("1h") };'); +$ce = new Cedar\AuthorizationClient($e); +echo "entity:", $ce->isAuthorized([ + "policyStoreId" => "ent", + "principal" => ["entityType" => "User", "entityId" => "a"], + "action" => ["actionType" => "Action", "actionId" => "x"], + "resource" => ["entityType" => "Doc", "entityId" => "d"], + "entities" => ["entityList" => [[ + "identifier" => ["entityType" => "User", "entityId" => "a"], + "attributes" => [ + "createdAt" => ["datetime" => "2026-01-01T00:00:00Z"], + "ttl" => ["duration" => "1h"], + ], + ]]], +])["decision"], PHP_EOL; ?> --EXPECT-- allow: ALLOW deny: DENY method:ALLOW +entity:ALLOW🤖 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/036-isauthorized-attr-datetime-duration.phpt` around lines 11 - 54, Add a small positive test that sends datetime/duration values via the entities.entityList input shape and asserts authorization succeeds; update the test near the existing $req closure and the $c->isAuthorized calls by creating a new request (similar to $req or the final anonymous request) that uses "entities" => ["entityList" => [ ... ]] with the same "contextMap" entries ("now" datetime, "window" duration, etc.) and call $c->isAuthorized(...) or $cm->isAuthorized(...) to echo or assert an "allow" decision so the path through entities.entityList is exercised for datetime/duration values. Ensure you reference the existing policyStore IDs ("dt" or "dur") and reuse the permit policy used by loadString("p1") so the test is positive.
🤖 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.
Nitpick comments:
In `@tests/019-isauthorized-attr-malformed.phpt`:
- Around line 17-21: Keep one malformed datetime case inside the
context.contextMap array so the context-side datetime parsing route is
exercised; specifically, add a key (e.g., "badDatetime") under the "context" =>
["contextMap" => ...] structure with a datetime-typed attribute containing an
invalid value (for example a non-ISO string) in addition to the existing
"badUnknown" and "badEmpty" entries so the malformed datetime path is still
covered by the test.
In `@tests/036-isauthorized-attr-datetime-duration.phpt`:
- Around line 11-54: Add a small positive test that sends datetime/duration
values via the entities.entityList input shape and asserts authorization
succeeds; update the test near the existing $req closure and the
$c->isAuthorized calls by creating a new request (similar to $req or the final
anonymous request) that uses "entities" => ["entityList" => [ ... ]] with the
same "contextMap" entries ("now" datetime, "window" duration, etc.) and call
$c->isAuthorized(...) or $cm->isAuthorized(...) to echo or assert an "allow"
decision so the path through entities.entityList is exercised for
datetime/duration values. Ensure you reference the existing policyStore IDs
("dt" or "dur") and reuse the permit policy used by loadString("p1") so the test
is positive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33d72240-617a-4655-8a3e-f0ff01d058dc
📒 Files selected for processing (13)
README.mdcedar.csrc/cedar/UPSTREAM.mdsrc/cedar/php_cedar_eval.csrc/cedar/php_cedar_eval.hsrc/cedar/php_cedar_expr.csrc/cedar/php_cedar_expr.hsrc/cedar/php_cedar_lexer.csrc/cedar/php_cedar_parser.csrc/cedar/php_cedar_types.hsrc/cedar/php_cedar_util.htests/019-isauthorized-attr-malformed.phpttests/036-isauthorized-attr-datetime-duration.phpt
kjdev
changed the title
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
1 participant
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 by CodeRabbit
Release Notes
New Features
datetimeanddurationattribute types in policy expressions and context attributes.toDate(),toMilliseconds()).Documentation
datetimeanddurationas supported attribute value types.Tests