chore(ers): expand multi-strategy ERS BDD coverage - #3791
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds BDD coverage for claims-only ERS, claims-to-LDAP fallback, and fail-fast routing. The scenarios verify permit and deny authorization decisions. ChangesClaims ERS routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 2
🤖 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 `@tests-bdd/features/claims-only-ers.feature`:
- Around line 52-73: Strengthen the “Claims entity with marketing department
gets DENY” scenario so it verifies Bob’s marketing claim is resolved rather than
relying only on the DENY result. Add a corresponding marketing resource/action
that must return PERMIT, or assert the resolved department claim before the
existing engineering DENY request, while preserving the current negative case.
- Around line 6-9: Mark the claims-only ERS feature with the repository’s
executable pending/skip control so its scenarios are excluded from normal Godog
runs. Update the feature metadata near the existing NOTE in
claims-only-ers.feature, preserving the `@claims-only-ers` tag and ensuring the
quarantine can be removed once ResolveEntities fixes issue `#3790`.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6f2988a-abd4-41ac-be12-c11291c97f1d
📒 Files selected for processing (4)
tests-bdd/cukes/steps_authorization.gotests-bdd/features/claims-ldap-fallback-ers.featuretests-bdd/features/claims-only-ers.featuretests-bdd/features/multi-strategy-ers-failfast.feature
…s, regex (opentdf#3797) ## Summary Adds BDD test coverage for the three untested condition operators in multi-strategy ERS strategy matching: - **`@ers-condition-equals`** — 3 scenarios: exact match (alice PERMIT), negative match (bob DENY), case-insensitive matching (`values: ["ALICE"]` matches userName `alice`) - **`@ers-condition-contains`** — 3 scenarios: substring match (`"ali"` in `"alice"` PERMIT), negative match (bob DENY), AND logic (two `contains` conditions — only `"diana"` matches both) - **`@ers-condition-regex`** — 3 scenarios: pattern match (`^[a-d].*` matches alice PERMIT), negative match (henry DENY), AND logic (regex + exists conditions) All 9 scenarios use LDAP strategies with `user_name` entities, avoiding the claims provider bug (opentdf#3790). Each feature file is `@stateless` and runs independently. ### Coverage before/after | Operator | Unit tests | BDD tests (before) | BDD tests (after) | |----------|-----------|--------------------|--------------------| | `exists` | Many | Many | Many | | `equals` | 1 (negative only) | 0 | 3 | | `contains` | 0 | 0 | 3 | | `regex` | 0 | 0 | 3 | | AND logic (multiple conditions) | 0 | 0 | 2 | ### Related - opentdf#3790 — claims provider context bug (why these tests use LDAP, not claims) - opentdf#3791 — fail-fast, fallback, and claims-only BDD tests - DSPX-4100 — multi-strategy ERS BDD coverage ## Test plan - [ ] CI: all 9 scenarios should pass (equals, contains, regex) - [ ] Run locally with `--godog.tags=@ers-condition-equals` / `@ers-condition-contains` / `@ers-condition-regex` 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added BDD coverage for ERS condition operators: `contains`, `equals` (including case-insensitive and multi-value), `regex`, and mixed-operator AND logic. * Validated substring and pattern matching semantics, non-matching behavior, and scenario routing outcomes. * Added end-to-end decision requests asserting expected **`PERMIT`** and **`DENY`** results across the new scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
df002c9 to
af75b19
Compare
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 `@tests-bdd/features/multi-strategy-ers-failfast.feature`:
- Around line 70-73: Extend the scenario around the decision request for
“alice_ff” to explicitly verify that the first ERS strategy errors under
fail-fast and that LDAP resolution is not attempted for this user, rather than
relying only on the DENY response. Use the existing BDD steps or add focused
assertions for the ERS error and absence of LDAP resolution while preserving the
successful response and DENY outcome.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c25923f9-f9a4-45e6-a14a-26f87c6240cb
📒 Files selected for processing (2)
tests-bdd/features/claims-ldap-fallback-ers.featuretests-bdd/features/multi-strategy-ers-failfast.feature
…scenarios Three new multi-strategy ERS feature files: - claims-only-ers.feature: Entity_Claims with inline claims resolved by claims-only strategy (PERMIT/DENY based on department) - claims-ldap-fallback-ers.feature: claims→LDAP fallback where LDAP enriches user_name entities with department claim - multi-strategy-ers-failfast.feature: fail-fast strategy aborts at first error, preventing LDAP fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ken Ho <kho@virtru.com>
f12b378 to
211ce37
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
elizabethhealy
left a comment
elizabethhealy
left a comment
There was a problem hiding this comment.
mostly comments on how we can expand coverage, they can be tackled in follow ups if desired
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ken Ho <kho@virtru.com>
…perly claims-ldap-fallback-ers.feature: - Change claims condition from "department exists" to "userName exists" so the claims strategy is actually selected for user_name entities, fails, and continue mode allows LDAP fallback (previously claims was just skipped) - Add Entity_Claims fast-path scenario showing claims resolves directly without needing LDAP when inline claims carry the needed attributes multi-strategy-ers-failfast.feature: - Add department to claims_passthrough output_mapping - Add Entity_Claims PERMIT scenario proving fail-fast only triggers on actual strategy errors, not universally Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ken Ho <kho@virtru.com>
2 participants
Summary
Expands multi-strategy ERS BDD test coverage (DSPX-4100) with three new feature files:
@multi-strategy-ers-failfast— Demonstrates thatfailure_strategy: "fail-fast"stops entity resolution at the first strategy error, preventing LDAP fallback. Alice gets DENY (vs PERMIT undercontinuein the existing feature).@claims-ldap-fallback-ers— Validates condition-based strategy routing: a claims strategy with conditiondepartment existsis skipped when the entity lacks department, and the LDAP strategy with conditionuserName existsprovides it via fallback. Covers both PERMIT (engineering user via LDAP) and DENY (operations user via LDAP).@claims-only-ers— Validates thatEntity_Claimsentities carrying inline claims can be resolved by a claims-only multi-strategy ERS without LDAP. Currently fails due to fix(ers): ResolveEntities does not populate JWTClaimsContextKey for claims provider #3790; will pass once fix(ers): pass inline claims through multi-strategy context #3794 merges.Also adds a DocString-based step definition for creating claims entities with embedded JSON.
Test results
@multi-strategy-ers-failfast@claims-ldap-fallback-ers@claims-only-ers@multi-strategy-ers(existing)Related
ResolveEntitiesdoes not populateJWTClaimsContextKeyfor claims provider@claims-only-ersshould pass)Test plan
@multi-strategy-ers-failfastand@claims-ldap-fallback-erspass@claims-only-ersexpected to fail until fix(ers): pass inline claims through multi-strategy context #3794 merges🤖 Generated with Claude Code
Summary by CodeRabbit