fix(policy): honor route-scoped identity inside policy engine. - #912
Conversation
…olver Signed-off-by: Teryl Taylor <terylt@ibm.com>
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Fixes route-scoped identity in the policy engine by stamping MetaExtension with entity_type/entity_name onto the Extensions passed to the identity hook, so CPEX scopes resolver dispatch to the route's authentication: list instead of running every registered resolver.
Overall Assessment
The core fix is correct and well-motivated. The three new tests are well-structured with descriptive assertion messages and cover the scoped, replace-inherited, and inherited-global scenarios. However, the tests validate the body-phase fix in isolation by calling on_request_body directly, without first calling on_request (which runs identity_gate). This leaves the full production request lifecycle untested.
Findings
| Severity | File | Description |
|---|---|---|
| Large | tests.rs |
Tests skip on_request and don't verify the full lifecycle with identity_gate |
| Large | filter.rs (not in diff) |
identity_gate still passes Extensions::default(), untested with route-scoped configs |
Non-inline Findings
[Large] identity_gate (filter.rs lines 338-367) still calls invoke_named with Extensions::default() -- it was not modified by this PR and is not in the diff. With routing_enabled: true in CPEX, this means the early identity gate runs ALL registered resolvers regardless of route-scoped authentication: blocks. For the scoped-tool scenario (request carries only X-Route-Token, no Authorization), the id-global resolver will not find its header. If CPEX's on_error: fail causes the chain to abort when a resolver finds no token, the identity gate would reject the request at on_request before the fixed body-phase resolve_identity ever runs.
The identity gate intentionally doesn't know the route yet (the classifier hasn't run), so passing empty extensions may be correct -- but the interaction between routing_enabled: true and Extensions::default() at the gate needs to be verified. At minimum, this should be covered by the lifecycle test requested in the inline comment.
| /// to the route's `authentication:` list instead of running every | ||
| /// registered resolver. | ||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn route_authentication_scopes_identity_to_its_resolver() { |
There was a problem hiding this comment.
[Large] These three route-scoped tests all call on_request_body directly via dispatch_tool_with_header, skipping on_request. In production, on_request runs identity_gate (filter.rs line 338), which invokes the identity hook with Extensions::default() -- running ALL registered resolvers, not just the route-scoped ones. For this scoped-tool scenario (request carries only X-Route-Token, no Authorization), the id-global resolver won't find its configured header. If CPEX's on_error: fail causes the chain to abort when a resolver finds no token, the identity gate rejects the request before the fixed body phase ever runs.
Add at least one full-lifecycle test that calls on_request followed by on_request_body for a route with replace_inherited: true and only the route-scoped token present, to confirm the feature works end-to-end.
| ) -> Result<IdentityPayload, Rejection> { | ||
| // Route coordinates must be on the Extensions or the identity hook | ||
| // can't tell which route this is and silently runs every registered | ||
| // resolver instead of the route's `authentication:` list. |
There was a problem hiding this comment.
Consider adding a clarifying comment on identity_gate's default() call explaining why route scoping is intentionally deferred to the body phase. Without that comment, a future reader may see the same pattern this PR fixes and might duplicate. A request with an invalid global-auth token + valid route-scoped token could be rejected at the gate (global resolver errors on the invalid token while running unscoped) even though the body-phase scoped resolution would accept.
| let token = mint_jwt(&standard_claims("alice")); | ||
|
|
||
| let action = dispatch_tool_with_header(&filter, "scoped-tool", "X-Route-Token", &token).await; | ||
| assert!( |
There was a problem hiding this comment.
[Important] The positive-case assertions here and in route_without_authentication_inherits_global_resolver use !matches!(action, FilterAction::Reject(_)) ("didn't reject") rather than matches!(action, FilterAction::BodyDone) ("processed successfully"). While logically equivalent given the possible return values of on_request_body with end_of_stream=true, BodyDone is the precise expected outcome and matches the pattern used by the existing cel_route_allows_matching_subject_and_denies_others test. Per project convention: "Tests must be thorough and verify precise behavior, not just directional correctness."
shaneutt
left a comment
There was a problem hiding this comment.
Bot comments need to be resolved, once they are LGTM.
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: The core fix is correct -- resolve_identity now stamps MetaExtension { entity_type, entity_name } onto the Extensions passed to the identity hook so CPEX scopes resolver dispatch to the route's authentication: list. The three new tests validate the scoped, replace-inherited, and inherited-global scenarios for the body phase.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 (new) |
| Medium | 0 |
No new findings beyond the prior review. The two Large items (lifecycle test gap, identity_gate still passing Extensions::default()) and one Important item (imprecise assertions) from the previous pass remain the actionable feedback for this PR.
Reviewed with claude-opus-4-6-20250806
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: The core fix is correct and well-targeted -- resolve_identity now stamps MetaExtension { entity_type, entity_name } onto the Extensions passed to the identity hook, enabling CPEX to scope resolver dispatch to the route's authentication: list. Both call sites are correctly differentiated: the HTTP authz path uses ENTITY_HTTP/ENTITY_NAME_GLOBAL constants, and the body phase uses classifier-derived coordinates. The #[expect(clippy::large_stack_frames)] is justified by the expanded async future state machine.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 (new) |
| Medium | 0 |
No new findings. The two Large items and one Important item from prior reviews remain the actionable feedback:
- Lifecycle test gap -- tests call
on_request_bodydirectly, skippingidentity_gatewhich still runs all resolvers unscoped viaExtensions::default(). identity_gate+routing_enabledinteraction -- withon_error: failon both plugins and only one header present, the unscoped gate may reject before the fixed body phase runs.- Imprecise assertions --
!matches!(Reject)instead ofmatches!(BodyDone)on positive cases.
Reviewed with claude-opus-4-6-20250806
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: The core fix is correct -- resolve_identity now stamps MetaExtension { entity_type, entity_name } onto the Extensions passed to the identity hook so CPEX scopes resolver dispatch to the route's authentication: list. Both call sites are correctly differentiated: the HTTP authz path uses ENTITY_HTTP/ENTITY_NAME_GLOBAL constants, and the body phase uses classifier-derived coordinates. The #[expect(clippy::large_stack_frames)] is justified by the expanded async future state machine.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 (new) |
| Medium | 0 |
No new findings. The two Large items and one Important item from prior reviews remain the actionable feedback:
- Lifecycle test gap -- tests call
on_request_bodydirectly, skippingidentity_gatewhich still runs all resolvers unscoped viaExtensions::default(). identity_gate+routing_enabledinteraction -- withon_error: failon both plugins and only one header present, the unscoped gate may reject before the fixed body phase runs.- Imprecise assertions --
!matches!(Reject)instead ofmatches!(BodyDone)on positive cases.
Reviewed with claude-opus-4-6-20250806
araujof
left a comment
There was a problem hiding this comment.
LGTM
The bot suggestions seem pretty minor.
Two conflicts, both resolved by keeping each side: server/src/watcher.rs: this branch writes `*content_hash = new_hash` after a successful reload (the hot-reload fix), main added a reload-success metric on the same line. Both belong, state update first. Cargo.lock: regenerated from main's side rather than hand-merged, then re-resolved. The engine stays on the pinned revision. Verified that praxis-proxy#912's route-scoped identity fix survived, since it touches the policy filter this branch rewrote onto the engine: `route_ext` and the `MetaExtension` stamping are present and its tests came across. 1851 filter tests pass with `policy-engine` on, up from 1831. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
What does this PR do?
The
policysecurity filter invoked theidentity.resolvehook withExtensions::default(), so the policy engine never received the request's route coordinates(
entity_type/entity_name). Without them the identity hook can't tell whichroute it is resolving for, so it runs every registered identity resolver
instead of the route's own
authentication:list — route-scoped identitysilently no-op'd. This is the fix: pass the route coordinates so the hook
scopes resolution to the route.
This stamps a
MetaExtension { entity_type, entity_name }onto theExtensionspassed to the hook, at both resolution sites:
ENTITY_HTTP/ENTITY_NAME_GLOBALentity_type/entity_nameWhy
Route-scoped
authentication:is a supported CPEX config surface, but praxiswasn't handing the policy engine the route identity, so it fell back to global resolution. A
route that authenticates solely by, say, a workload SVID —
— had its scoping ignored, and every registered resolver ran anyway.
Tests
Three unit tests in
security/policy/tests.rs:route_authentication_scopes_identity_to_its_resolver— the route's resolver runs for that routeroute_replace_inherited_excludes_global_resolver—replace_inheriteddrops the inherited global resolverroute_without_authentication_inherits_global_resolver— routes with no block still inherit the global resolverNotes
MetaExtensionfrom the existingcpexdep.#[expect(clippy::large_stack_frames, …)]onresolve_identity(async handler over the large CMF/pipeline types).make lintclean (clippy-D warnings, nightlyfmt --check,cargo machete, example-test-coverage, README sync, filter-docs); DCO signed off.Checklist
git commit -s)make lint && make testpasses locallyDoes this introduce a breaking change?
No, it shouldn't. This wires up the routing extensions in the policy engine.
Note: This will enable the identity/delegation use cases in contextforge-org/cpex#131. Updates to demos to reflect these will be next.