Feat implement - #1
Merged
Merged
Conversation
Vendor the Cedar evaluator sources from nxe-cedar (cd3d1df5c5642a75b27f40fa502022c864272ed3) under src/cedar/ with these mechanical rewrites applied during the import: - Symbol prefixes: nxe_cedar_* -> php_cedar_*, NXE_CEDAR_* -> PHP_CEDAR_* - File names follow the same rename - NGINX-specific types and helpers are renamed to the php_cedar_* namespace as well (ngx_pool_t -> php_cedar_pool_t, ngx_log_error -> php_cedar_log_error, NGX_OK -> PHP_CEDAR_OK, ...); the actual definitions land in the next commit - u_char -> unsigned char to drop the NGINX-flavored typedef The lexer / parser / expression evaluator logic itself is untouched to avoid introducing porting bugs. Files do not build yet; the compatibility layer that provides the php_cedar_* types and helpers arrives in the next commit. src/cedar/UPSTREAM.md records the snapshot commit, the rewrite checklist, and the re-import policy.
|
Warning Review limit reached
More reviews will be available in 45 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughThis PR adds a complete PHP extension that implements Cedar policy lexing, parsing, expression evaluation, policy evaluation, Zend bindings (PolicyStore, AuthorizationClient), build/config files, documentation, and 34 PHPT tests exercising AVP-compatible request/response behavior. ChangesCedar Extension Implementation
Sequence DiagramsequenceDiagram
participant Client as PHP Client
participant Auth as Cedar.AuthorizationClient
participant Eval as php_cedar_eval
participant Store as PolicyStore
Client->>Auth: isAuthorized / isAuthorizedWithToken request
Auth->>Store: ensure policyStoreId matches, provide policy sets
Auth->>Eval: build eval context (principal/action/resource/context)
Eval->>Store: read policies & evaluate scopes/conditions
Eval-->>Auth: decision + determiningPolicies + errors
Auth-->>Client: AVP-shaped response (decision, determiningPolicies, errors[, principal])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
11-18: ⚡ Quick winAdd a CI timeout to prevent hung runs.
build-and-testhas no timeout, so a stuckconfigure/make/make testcan run until runner limits. Addtimeout-minutesat job level.Proposed change
jobs: build-and-test: name: PHP ${{ matrix.php-version }} (${{ matrix.ts }}) runs-on: ubuntu-latest + timeout-minutes: 20🤖 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 @.github/workflows/ci.yml around lines 11 - 18, The build-and-test job currently has no timeout and can hang indefinitely; add a job-level timeout by inserting a timeout-minutes field under the build-and-test job (the job named "build-and-test") to abort stuck runs (e.g., timeout-minutes: 60) so long-running configure/make/make test steps are cut off by the runner.
🤖 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 `@cedar.c`:
- Around line 749-754: The current branch uses if (...) { set tgt =
CEDAR_TARGET_PRINCIPAL } else if (...) { set tgt = CEDAR_TARGET_RESOURCE }, so
when e_type/e_id equal both principal and resource identifiers only one target
is processed; replace this exclusive branch with independent checks using
cedar_str_equal for principal (e_type/e_id vs p_type/p_id) and for resource
(e_type/e_id vs r_type/r_id) so you can mark and populate both targets when both
match (update matched_subject and tgt handling accordingly — e.g., set flags or
add both CEDAR_TARGET_PRINCIPAL and CEDAR_TARGET_RESOURCE rather than
overwriting), and adjust later code that depends on tgt to iterate/apply
attributes and parents for each set target (refer to variables e_type, e_id,
p_type, p_id, r_type, r_id, tgt, CEDAR_TARGET_PRINCIPAL, CEDAR_TARGET_RESOURCE,
matched_subject).
- Around line 466-470: The code currently coerces types (e.g., truncating
doubles to "long" and using zend_is_true for booleans); update the type checks
in cedar_apply_record_attr() and cedar_apply_set_element() (and the other
similar branches handling "long" and "boolean") to be strict: for "long" accept
only Z_TYPE_P(inner) == IS_LONG (reject IS_DOUBLE and others), and for "boolean"
accept only explicit boolean zval types (check Z_TYPE_P(inner) == IS_TRUE ||
Z_TYPE_P(inner) == IS_FALSE or the equivalent boolean type check) instead of
using zend_is_true; remove the truncating cast paths and truthiness branches so
malformed primitives return PHP_CEDAR_ERROR.
- Around line 179-183: The loadFile path silently returns when
php_stream_open_wrapper fails (stream == NULL); instead throw a
PolicyParseException from loadFile so callers receive a parse/read error.
Replace the early return in the block that calls
php_stream_open_wrapper(ZSTR_VAL(path), "rb", REPORT_ERRORS, NULL) with code
that constructs and throws a PolicyParseException (include the
path/ZSTR_VAL(path) and any available error info) using the same exception type
used elsewhere in this module so the failure flows through the documented
parse/read exception path.
- Around line 105-117: The fallback branch currently writes a "ps-...-..."
string that breaks the expected 32-character lowercase hex format; change the
fallback to populate the same raw[] buffer and hex-encode it like the success
path. Specifically, when php_random_bytes_silent(...) == FAILURE, build
deterministic bytes from the existing static unsigned int seq and unsigned long
t (e.g., fill raw[0..15] using t and ++seq/rotations/xors) and then run the same
hex-encoding loop that uses digits[] to set hex[0..31] and hex[32]='\0' before
returning via zend_string_init; keep variable names raw, seq, t, hex, digits and
preserve the existing zend_string_init call.
In `@php_cedar.h`:
- Line 11: The header php_cedar.h is not self-contained because it declares
extern zend_module_entry cedar_module_entry without including the Zend/PHP type
definitions; update php_cedar.h to include the appropriate PHP header (e.g.,
include "php.h" or the minimal Zend header that defines zend_module_entry) at
the top so any translation unit including php_cedar.h gets the zend_module_entry
definition; ensure the include is guarded and placed before the extern
declaration to avoid forward-declaration/build failures when files include
php_cedar.h without including php.h first.
In `@src/cedar/php_cedar_compat.c`:
- Around line 102-103: The allocations that compute byte counts (e.g., the call
assigning a->elts = php_cedar_palloc(pool, n * size) and the other
multiplications near the growth/push code) lack overflow checks; add guards
before any multiplication of count * size using size_t (e.g., if (size != 0 && n
> SIZE_MAX / size) { handle error/return NULL; }) and similar guards for
expressions like (n+something) * size so you never pass an overflowed size to
php_cedar_palloc; ensure the error path aborts allocation cleanly (or returns
failure) and avoid calling memcpy/other writes when the guard fails.
In `@src/cedar/php_cedar_lexer.c`:
- Around line 329-335: The allocation-failure branch that calls
php_cedar_palloc(lexer->pool, len) sets token.type to PHP_CEDAR_TOKEN_ERROR and
populates token.value but leaves token.raw uninitialized; update this error path
to initialize token.raw (e.g., set token.raw = NULL or an empty pointer/zero
length) just like the other error branches so the returned token is fully
initialized and safe to inspect.
In `@tests/005-policy-store-load-file.phpt`:
- Around line 7-14: The test currently writes to a fixed path ($path = __DIR__ .
"/_tmp.cedar") which can collide or fail; change it to create a unique temporary
file with tempnam(), write the policy to that temp file, and ensure removal
inside a try/finally (or equivalent) so unlink() always runs; update usage
points that reference $path and leave the PolicyStore usage (new
Cedar\PolicyStore(); $store->loadFile("p1", $path); $store->policyIds();)
unchanged aside from using the temp file variable.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 11-18: The build-and-test job currently has no timeout and can
hang indefinitely; add a job-level timeout by inserting a timeout-minutes field
under the build-and-test job (the job named "build-and-test") to abort stuck
runs (e.g., timeout-minutes: 60) so long-running configure/make/make test steps
are cut off by the runner.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37786d80-d260-4ba0-9d1b-6bd301bfaa9c
📒 Files selected for processing (52)
.github/workflows/ci.yml.gitignoreLICENSEREADME.mdcedar.ccedar.stub.phpcedar_arginfo.hcomposer.jsonconfig.m4php_cedar.hsrc/cedar/UPSTREAM.mdsrc/cedar/php_cedar_compat.csrc/cedar/php_cedar_compat.hsrc/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_lexer.hsrc/cedar/php_cedar_parser.csrc/cedar/php_cedar_parser.hsrc/cedar/php_cedar_types.hsrc/cedar/php_cedar_util.htests/001-extension-loaded.phpttests/002-policy-store-id.phpttests/003-policy-store-load.phpttests/004-policy-store-parse-error.phpttests/005-policy-store-load-file.phpttests/006-isauthorized-allow.phpttests/007-isauthorized-forbid.phpttests/008-isauthorized-default-deny.phpttests/009-isauthorized-mismatch.phpttests/010-isauthorized-with-token-allow.phpttests/011-isauthorized-multi-policy.phpttests/012-isauthorized-context-scalar.phpttests/013-isauthorized-entities-attrs.phpttests/014-isauthorized-entities-parents.phpttests/015-isauthorized-attr-set.phpttests/016-isauthorized-attr-record.phpttests/017-isauthorized-attr-ipaddr-decimal.phpttests/018-isauthorized-attr-entity-id.phpttests/019-isauthorized-attr-malformed.phpttests/020-isauthorized-with-token-deny.phpttests/021-isauthorized-with-token-accesstoken-fallback.phpttests/022-isauthorized-with-token-errors.phpttests/023-isauthorized-with-token-custom-claim.phpttests/024-isauthorized-determining-policies-forbid-only.phpttests/030-avp-photoflash-sample.phpttests/031-syntax-is-and-is-in.phpttests/032-syntax-like.phpttests/033-syntax-if-then-else.phpttests/034-syntax-annotations.phpt
Provide php_cedar_compat.{h,c} so the evaluator sources imported from
nxe-cedar can build without NGINX. The header defines drop-in
replacements for the types and helpers used by the lexer / parser /
eval code:
- Types: php_cedar_pool_t / php_cedar_str_t / php_cedar_array_t /
php_cedar_log_t / php_cedar_int_t / php_cedar_uint_t /
php_cedar_flag_t
- Helpers: php_cedar_palloc / pcalloc / pool_create / pool_destroy,
php_cedar_array_create / array_push,
php_cedar_log_error, php_cedar_memcmp / memcpy / memzero
- Return codes: PHP_CEDAR_OK / PHP_CEDAR_ERROR / PHP_CEDAR_DECLINED
- Log levels: PHP_CEDAR_LOG_EMERG ... PHP_CEDAR_LOG_DEBUG
Memory allocation switches at build time. When PHP_CEDAR_USE_ZEND_MM
is defined (set by config.m4 in the extension build), the layer routes
allocations through the Zend Memory Manager (emalloc/efree) and log
output through php_error_docref. Otherwise it falls back to libc
(malloc/free + fprintf to stderr), which keeps the layer usable in
standalone tests or future non-PHP host integrations.
The compatibility implementation includes config.h via HAVE_CONFIG_H
so the PHP_CEDAR_USE_ZEND_MM macro is observed when the extension
build defines it.
After this commit, all src/cedar/ files compile cleanly.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cedar.c`:
- Around line 124-139: The constructor Cedar_PolicyStore (__construct) must
detect failed pool allocation from cedar_policy_store_create(): after obtaining
intern (cedar_policy_store_t) and calling/after cedar_policy_store_create(),
check intern->pool for NULL and immediately raise a PHP exception and return to
avoid later passing a NULL pool into php_cedar_parse() (which happens from
loadString()/loadFile()); ensure you set intern->id as before but if pool is
NULL free any partially-initialized resources if necessary, call
zend_throw_exception_ex or similar with a clear message about allocation
failure, and return from Caesar_PolicyStore::__construct to fail fast.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 439217d2-1c44-489f-9520-fa90cad6b881
📒 Files selected for processing (44)
.github/workflows/ci.yml.gitignoreLICENSEREADME.mdcedar.ccedar.stub.phpcedar_arginfo.hcomposer.jsonconfig.m4php_cedar.hsrc/cedar/php_cedar_compat.csrc/cedar/php_cedar_compat.htests/001-extension-loaded.phpttests/002-policy-store-id.phpttests/003-policy-store-load.phpttests/004-policy-store-parse-error.phpttests/005-policy-store-load-file.phpttests/006-isauthorized-allow.phpttests/007-isauthorized-forbid.phpttests/008-isauthorized-default-deny.phpttests/009-isauthorized-mismatch.phpttests/010-isauthorized-with-token-allow.phpttests/011-isauthorized-multi-policy.phpttests/012-isauthorized-context-scalar.phpttests/013-isauthorized-entities-attrs.phpttests/014-isauthorized-entities-parents.phpttests/015-isauthorized-attr-set.phpttests/016-isauthorized-attr-record.phpttests/017-isauthorized-attr-ipaddr-decimal.phpttests/018-isauthorized-attr-entity-id.phpttests/019-isauthorized-attr-malformed.phpttests/020-isauthorized-with-token-deny.phpttests/021-isauthorized-with-token-accesstoken-fallback.phpttests/022-isauthorized-with-token-errors.phpttests/023-isauthorized-with-token-custom-claim.phpttests/024-isauthorized-determining-policies-forbid-only.phpttests/025-policy-store-load-file-missing.phpttests/026-isauthorized-attr-strict-primitive.phpttests/027-isauthorized-entity-principal-equals-resource.phpttests/030-avp-photoflash-sample.phpttests/031-syntax-is-and-is-in.phpttests/032-syntax-like.phpttests/033-syntax-if-then-else.phpttests/034-syntax-annotations.phpt
✅ Files skipped from review due to trivial changes (7)
- tests/007-isauthorized-forbid.phpt
- tests/001-extension-loaded.phpt
- .gitignore
- README.md
- LICENSE
- tests/031-syntax-is-and-is-in.phpt
- tests/014-isauthorized-entities-parents.phpt
🚧 Files skipped from review as they are similar to previous changes (27)
- tests/008-isauthorized-default-deny.phpt
- composer.json
- tests/006-isauthorized-allow.phpt
- tests/021-isauthorized-with-token-accesstoken-fallback.phpt
- tests/009-isauthorized-mismatch.phpt
- config.m4
- tests/034-syntax-annotations.phpt
- tests/015-isauthorized-attr-set.phpt
- tests/012-isauthorized-context-scalar.phpt
- tests/003-policy-store-load.phpt
- tests/016-isauthorized-attr-record.phpt
- tests/011-isauthorized-multi-policy.phpt
- .github/workflows/ci.yml
- tests/030-avp-photoflash-sample.phpt
- tests/024-isauthorized-determining-policies-forbid-only.phpt
- tests/032-syntax-like.phpt
- tests/017-isauthorized-attr-ipaddr-decimal.phpt
- tests/020-isauthorized-with-token-deny.phpt
- tests/010-isauthorized-with-token-allow.phpt
- tests/033-syntax-if-then-else.phpt
- src/cedar/php_cedar_compat.h
- tests/013-isauthorized-entities-attrs.phpt
- tests/019-isauthorized-attr-malformed.phpt
- tests/018-isauthorized-attr-entity-id.phpt
- tests/002-policy-store-id.phpt
- src/cedar/php_cedar_compat.c
- tests/022-isauthorized-with-token-errors.phpt
Wire the imported Cedar evaluator into a PHP 8 extension that exposes
an AVP-compatible authorization API.
Build glue
- config.m4 enables --enable-cedar, compiles cedar.c plus the
src/cedar/ sources, defines PHP_CEDAR_USE_ZEND_MM so the
compatibility layer routes allocations through the Zend Memory
Manager, and registers src/cedar/ on the include path
- php_cedar.h declares the module entry and version
- .gitignore filters out phpize/autotools artifacts and gen_stub
legacy output
Surface (declared in cedar.stub.php, generated into cedar_arginfo.h)
- Cedar\\PolicyStore::__construct(?string \$policyStoreId = null) -
auto-generates a 32-char lowercase hex id when none is given
- Cedar\\PolicyStore::loadFile(string \$policyId, string \$path): static
- Cedar\\PolicyStore::loadString(string \$policyId, string \$cedarText): static
- Cedar\\PolicyStore::id(): string
- Cedar\\PolicyStore::policyIds(): list<string>
- Cedar\\AuthorizationClient::__construct(PolicyStore \$policyStore)
- Cedar\\AuthorizationClient::isAuthorized(array \$params): array
- Cedar\\AuthorizationClient::isAuthorizedWithToken(array \$params): array
- Cedar\\Exception\\PolicyParseException, EvaluationException,
ResourceNotFoundException (all extend \\RuntimeException)
PolicyStore internals
- A php_cedar_pool_t owned by the object holds the parsed
php_cedar_policy_set_t bundles
- A HashTable maps policyId (zend_string) to its policy set pointer
- loadFile / loadString call php_cedar_parse(); parse failures and
duplicate policy ids raise PolicyParseException with the offending
id in the message
- Internal log level is set to 0 so the evaluator stays quiet; error
details surface through exceptions instead
AuthorizationClient::isAuthorized() golden path
- Validates 'policyStoreId' as a string and compares it against the
bound store id; mismatch raises ResourceNotFoundException (named
after AVP's same-named error)
- Extracts {entityType, entityId} for principal/resource and
{actionType, actionId} for action and feeds them into a
request-scoped php_cedar_eval_ctx_t
- Walks every policy_set in the store and combines decisions per
Cedar semantics: any matched forbid -> DENY, otherwise any matched
permit -> ALLOW, otherwise implicit DENY
- Returns the AVP-shaped result array {decision,
determiningPolicies, errors}; determiningPolicies lists the
policyId of each bundle that produced a contributing decision
isAuthorizedWithToken() is stubbed out for this release: it throws
RuntimeException pointing callers at isAuthorized() with an
externally-verified principal. JWT verification is intentionally left
to the caller because the extension does not bundle an identity
source (Cognito / OIDC) like AVP does.
context.contextMap, entities.entityList, the non-scalar AttributeValue
union members (ipaddr / decimal / entityIdentifier / set / record),
and transitive parent resolution are deferred as follow-up work.
Eleven phpt cases (make test reports 11/11 PASS) split into two groups. PolicyStore (tests 001-005) - 001: extension loads, the expected classes exist, and the three Cedar\\Exception\\* classes inherit \\RuntimeException - 002: id() returns a 32-char hex when no id is given, returns the supplied string verbatim when one is given, and auto-generated ids differ across instances - 003: loadString registers policies, returns $this for fluent chaining, and policyIds() reflects every registered id in order - 004: parse failure raises PolicyParseException with the offending id in the message; loading the same policy id twice raises the same exception - 005: loadFile reads a cedar policy file via php_stream and registers it under the supplied id AuthorizationClient (tests 006-011) - 006: a single permit policy yields decision=ALLOW and lists the matched policyId in determiningPolicies - 007: a forbid policy yields decision=DENY and lists the matched policyId in determiningPolicies (i.e. forbid, not implicit deny) - 008: an empty store gives the implicit DENY with an empty determiningPolicies array - 009: a policyStoreId that does not match the bound PolicyStore::id raises ResourceNotFoundException; omitting the key raises Error - 010: isAuthorizedWithToken throws RuntimeException for now to signal the unimplemented path - 011: with both permit and forbid loaded, forbid wins for a matching resource and permit wins for a non-matching resource, confirming the cross-bundle combination logic
… AttributeValue
Extend AuthorizationClient::isAuthorized() so the optional 'context'
and 'entities' inputs from the AVP request shape are wired into the
evaluator alongside the previously-handled principal / action /
resource scalars.
AttributeValue Union (single-key associative array) now maps to the
matching php_cedar_eval_ctx_add_*_attr* helpers for every kind the
evaluator supports:
- string -> add_*_attr
- long -> add_*_attr_long (accepts PHP long or float, cast to int64)
- boolean -> add_*_attr_bool
- ipaddr -> add_*_attr_ip
- decimal -> add_*_attr_decimal
- entityIdentifier -> add_*_attr_entity (uses {entityType, entityId})
- set -> add_*_attr_set + recursive set_add_*
- record -> add_*_attr_record + recursive record_add_*
set and record nest arbitrarily via the mutually-recursive
cedar_apply_set_element / cedar_apply_record_attr helpers. The
upstream evaluator does not support 'datetime' / 'duration', so those
Union kinds (and any malformed or empty AttributeValue) are surfaced
through the AVP-shaped errors[] array as
{errorDescription: 'unsupported or malformed AttributeValue for ...'}
rather than aborting the request.
entities.entityList walks each entry and:
- Matches the identifier against the request principal / resource;
when it matches, the entry's attributes feed
add_principal_attr_* / add_resource_attr_*.
- Forwards parents to add_principal_parent / add_resource_parent.
Per Cedar semantics, callers are expected to flatten the parent
closure themselves (same contract as AVP).
- Entries that match neither the principal nor the resource are
silently skipped on attributes; their parents have no anchor in the
eval_ctx shape and are skipped too.
context.contextMap iterates {name => AttributeValue} entries and
funnels each through the same top-level dispatcher with
CEDAR_TARGET_CONTEXT.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cedar.c`:
- Around line 738-740: cedar_apply_entities currently only matches
principal/resource and drops any action attributes in entities.entityList;
extend its signature to accept action type/id (a_type, a_id) alongside
p_type/p_id and r_type/r_id, update all callers to pass the action type/id
through, and inside cedar_apply_entities iterate entities.entityList and, in
addition to populating CEDAR_TARGET_PRINCIPAL and CEDAR_TARGET_RESOURCE, also
match entity entries against a_type/a_id and apply their attributes to
CEDAR_TARGET_ACTION using the same logic used for principal/resource so action
attributes are hydrated correctly (ensure you reference php_cedar_eval_ctx_t,
entities.entityList, CEDAR_TARGET_ACTION, and the new a_type/a_id parameters
when making the changes).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d32dbe77-357b-4e2b-8334-726bc00c7200
📒 Files selected for processing (42)
.github/workflows/ci.yml.gitignoreLICENSEREADME.mdcedar.ccedar.stub.phpcedar_arginfo.hcomposer.jsonconfig.m4php_cedar.htests/001-extension-loaded.phpttests/002-policy-store-id.phpttests/003-policy-store-load.phpttests/004-policy-store-parse-error.phpttests/005-policy-store-load-file.phpttests/006-isauthorized-allow.phpttests/007-isauthorized-forbid.phpttests/008-isauthorized-default-deny.phpttests/009-isauthorized-mismatch.phpttests/010-isauthorized-with-token-allow.phpttests/011-isauthorized-multi-policy.phpttests/012-isauthorized-context-scalar.phpttests/013-isauthorized-entities-attrs.phpttests/014-isauthorized-entities-parents.phpttests/015-isauthorized-attr-set.phpttests/016-isauthorized-attr-record.phpttests/017-isauthorized-attr-ipaddr-decimal.phpttests/018-isauthorized-attr-entity-id.phpttests/019-isauthorized-attr-malformed.phpttests/020-isauthorized-with-token-deny.phpttests/021-isauthorized-with-token-accesstoken-fallback.phpttests/022-isauthorized-with-token-errors.phpttests/023-isauthorized-with-token-custom-claim.phpttests/024-isauthorized-determining-policies-forbid-only.phpttests/025-policy-store-load-file-missing.phpttests/026-isauthorized-attr-strict-primitive.phpttests/027-isauthorized-entity-principal-equals-resource.phpttests/030-avp-photoflash-sample.phpttests/031-syntax-is-and-is-in.phpttests/032-syntax-like.phpttests/033-syntax-if-then-else.phpttests/034-syntax-annotations.phpt
✅ Files skipped from review due to trivial changes (7)
- composer.json
- tests/005-policy-store-load-file.phpt
- LICENSE
- .gitignore
- README.md
- tests/022-isauthorized-with-token-errors.phpt
- tests/034-syntax-annotations.phpt
🚧 Files skipped from review as they are similar to previous changes (26)
- tests/026-isauthorized-attr-strict-primitive.phpt
- tests/008-isauthorized-default-deny.phpt
- tests/019-isauthorized-attr-malformed.phpt
- tests/018-isauthorized-attr-entity-id.phpt
- tests/021-isauthorized-with-token-accesstoken-fallback.phpt
- tests/010-isauthorized-with-token-allow.phpt
- tests/009-isauthorized-mismatch.phpt
- tests/003-policy-store-load.phpt
- tests/032-syntax-like.phpt
- tests/020-isauthorized-with-token-deny.phpt
- tests/031-syntax-is-and-is-in.phpt
- tests/011-isauthorized-multi-policy.phpt
- tests/002-policy-store-id.phpt
- tests/016-isauthorized-attr-record.phpt
- tests/023-isauthorized-with-token-custom-claim.phpt
- tests/025-policy-store-load-file-missing.phpt
- tests/017-isauthorized-attr-ipaddr-decimal.phpt
- tests/001-extension-loaded.phpt
- tests/012-isauthorized-context-scalar.phpt
- tests/030-avp-photoflash-sample.phpt
- tests/004-policy-store-parse-error.phpt
- tests/027-isauthorized-entity-principal-equals-resource.phpt
- tests/033-syntax-if-then-else.phpt
- .github/workflows/ci.yml
- tests/024-isauthorized-determining-policies-forbid-only.phpt
- tests/014-isauthorized-entities-parents.phpt
…ue variants
Add eight .phpt cases for the follow-up coverage (make test now
reports 19/19 PASS):
- 012: context.contextMap with scalar string / long / boolean entries
drives a when-clause; flipping each scalar flips the decision
- 013: entities.entityList supplies principal.tier and resource.public
attributes; toggling resource.public flips the decision
- 014: entities.entityList parents wire 'principal in Group::"admins"',
including the negative case (parents=[viewers]) and the missing-
parents case (implicit DENY)
- 015: set AttributeValue with contains("editors"), match + non-match
- 016: record AttributeValue with two members (string + long) and
nested attribute access principal.profile.tier / .age
- 017: ipaddr (isInRange against ip("10.0.0.0/8")) and decimal
(lessThan against decimal("5.0")), both with passing and failing
inputs
- 018: entityIdentifier AttributeValue for resource.owner ==
principal, both self-owned and other-owned
- 019: malformed AttributeValue (unknown 'datetime' kind, empty
array) does not abort the request; instead the entries land in
errors[] with an errorDescription, while a valid scalar in the
same contextMap still produces ALLOW
The token path takes a verified claims array (caller is responsible
for JWT signature / issuer / expiry verification) and a constructor-
supplied identitySource config, then derives the principal and group
parents before delegating to the shared evaluation routine.
AuthorizationClient::__construct(PolicyStore $store, array $options = [])
- Accepts an optional 'identitySource' array on $options with keys:
principalEntityType (string, required when used)
principalIdClaim (string, default "sub")
groupEntityType (string, optional)
groupIdsClaim (string, optional)
- Storing the option array as a zval lets identitySource survive
across multiple isAuthorizedWithToken() calls on the same client.
AuthorizationClient::isAuthorizedWithToken(array $params)
- Required: policyStoreId, action, resource, and at least one of
identityToken / accessToken (both as verified claims arrays;
identityToken wins when both are present, matching AVP's behavior).
- 'principal' must NOT be supplied (it is derived from the token);
passing it raises Error.
- Missing identitySource on the client also raises Error.
- The principal id claim is looked up by principalIdClaim and joined
with principalEntityType; if the claim is absent or non-string,
Error is raised before any evaluation runs.
- groupIdsClaim, when configured, is read off the payload as a list
of strings and each entry becomes a {groupEntityType, <id>} parent
registered against the principal via add_principal_parent.
- Response carries the extracted principal as
{ entityType, entityId }, matching AVP's IsAuthorizedWithToken
output shape (in addition to decision / determiningPolicies /
errors).
Refactor: a new helper, cedar_evaluate_request(), now performs the
shared parts of both isAuthorized() and isAuthorizedWithToken()
(policyStoreId check, eval_pool / eval_ctx, context / entities, the
policy_set loop, response shaping, and optional principal emission).
The two PHP_METHODs only handle parameter parsing and principal
resolution.
This removed the "isAuthorized(): " prefix from one shared error
message; tests/009 is updated to match.
Five new .phpt cases (full suite: 23/23 PASS):
- 010: claim mapper derives principal from 'sub' and turns
cognito:groups entries into MyApp::Group parents; principal
appears in the response as { entityType, entityId }
- 020: missing or non-matching group claim falls back to implicit
DENY, but the response still echoes the extracted principal
- 021: accessToken is used when identityToken is absent;
identityToken takes precedence when both are supplied (matches AVP)
- 022: error surfaces — no identitySource on the client, 'principal'
supplied alongside the token, neither identityToken nor accessToken,
missing principal claim, and policyStoreId mismatch
- 023: principalIdClaim can be customized (e.g. "user_id" instead of
"sub"); context.contextMap and entities.entityList still flow
through alongside the derived principal
Add five .phpt cases driving the full suite to 28/28 PASS. - 030 mirrors the canonical AVP PhotoFlash sample: per-user permit, group-mediated permit, and the documented IsAuthorized request for alice / updatePhoto / VacationPhoto94.jpg with the photo nested in alice_folder. Covers principal-by-equality, principal-in-group via entities.parents, resource-in-album, the multi-policy determining list, and the negative cases for bob. - 031 covers the 'is' type-check (User vs Service principal) and 'is ... in' combined type + parent check against Group::"admins". - 032 covers the 'like' wildcard operator on resource.path with positive matches at the root and nested levels, plus the cases where the prefix exists without a trailing path or the path is in /private/. - 033 covers 'if ... then ... else' inside a when-clause, exercising both branches (admin role short-circuits true, otherwise the principal.tier check applies). - 034 covers @id / @advice annotations: the parser must accept them and the policy must still evaluate normally; the matched policyId shows the annotated bundle id in determiningPolicies.
Prepare the repository for distribution via PIE (the modern PHP
extension installer replacing PECL).
README.md
- Quick-start example and full API reference for Cedar\\PolicyStore
and Cedar\\AuthorizationClient (including the identitySource option
used by isAuthorizedWithToken).
- Mapping table for AVP compatibility and the AttributeValue Union.
- Explicit "unsupported features" list inherited from the bundled
Cedar evaluator (datetime / duration, entity tags, policy templates,
schema validation, dynamic identity sources).
- Performance/persistence note pointing at APCu / opcache.preload
as the interim sharing story, plus a forward reference to a
future pemalloc-based persistent variant.
- "One PolicyStore per AuthorizationClient" caveat with a roadmap
pointer for multi-store support.
- "Token verification is caller's responsibility" section explaining
why isAuthorizedWithToken takes a verified claims array rather
than a raw JWT string, with a firebase/php-jwt example.
- ZTS status section recording the static review and the pending
--enable-zts verification.
LICENSE
- MIT for the extension code itself.
- The bundled src/cedar/ snapshot keeps its upstream license; the
LICENSE file points at src/cedar/UPSTREAM.md.
composer.json (PIE manifest)
- type: "php-ext" with extension-name "cedar", priority 80.
- support-zts: false / support-nts: true (conservative until ZTS
is verified end-to-end).
- require php ^8.4 because php_random_bytes_silent moved into
ext/random/ in PHP 8.4 and the source uses that header directly.
.github/workflows/ci.yml
- Matrix over {PHP 8.4, 8.5} x {NTS, ZTS} on ubuntu-latest using
shivammathur/setup-php with phpts env.
- Runs phpize -> ./configure --enable-cedar -> make -j2 -> make
test with NO_INTERACTION / REPORT_EXIT_STATUS so a single failing
.phpt fails the job.
- ZTS rows are marked continue-on-error: true (informational) until
the ZTS support roadmap item is closed; NTS rows must pass.
- Failed runs upload the .phpt diff/log/out artifacts so a
regression can be inspected without re-running locally.
Show how to swap Cedar\AuthorizationClient and the AVP SDK behind a single interface via dependency injection. The new section appears right after Quick start to surface AVP compatibility as the headline value proposition, with an adapter pattern that coerces Aws\Result to a plain array on the AVP side.
The shared module lacked ZEND_TSRMLS_CACHE_EXTERN/DEFINE/UPDATE, so on ZTS builds the DSO had no _tsrm_ls_cache definition. Loading cedar.so failed with "undefined symbol: _tsrm_ls_cache" and every test was skipped as the extension never loaded. Add the three standard ext_skel macros, guarded by ZTS && COMPILE_DL_CEDAR so NTS builds are unaffected.
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
New Features
Tests
Documentation
Chores