chore(ers): expand multi-strategy ERS BDD coverage - #3791
Open
khvirtru wants to merge 2 commits into
Open
Conversation
Expand multi-strategy ERS BDD coverage (DSPX-4100) with two new feature files: - @multi-strategy-ers-failfast: demonstrates that fail-fast stops entity resolution at first strategy error (claims provider fails due to missing JWTClaimsContextKey — see opentdf#3790), preventing LDAP fallback → DENY - @claims-ldap-fallback-ers: validates condition-based strategy routing where claims strategy is skipped (condition unmet) and LDAP provides department Signed-off-by: Ken Ho <kho@virtru.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ken Ho <kho@virtru.com>
Add @claims-only-ers feature that expects PERMIT for an Entity_Claims entity with inline department claim. Currently FAILS with "DECISION_DENY instead of DECISION_PERMIT" because ResolveEntities does not populate JWTClaimsContextKey in context (opentdf#3790). This test will pass once the bug is fixed. Also adds a DocString step definition for creating claims entities with embedded JSON (existing step regex rejects double quotes). Signed-off-by: Ken Ho <kho@virtru.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ken Ho <kho@virtru.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds authorization BDD coverage for claims-based subject entities, claims-only ERS resolution, claims-to-LDAP fallback, and fail-fast strategy behavior. ChangesClaims ERS coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: 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
Comment on lines
+6
to
+9
| NOTE: These scenarios currently FAIL due to a known bug (#3790) where | ||
| ResolveEntities does not populate JWTClaimsContextKey in context, causing | ||
| the claims provider to always error with "JWT claims not found in context". | ||
| Once the bug is fixed, these scenarios should pass. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^tests-bdd/features/claims-only-ers\.feature$|go\.mod$|go\.sum$|main\.go$|cmd/.*)' | head -200
echo
echo "== feature file =="
if [ -f tests-bdd/features/claims-only-ers.feature ]; then
cat -n tests-bdd/features/claims-only-ers.feature
fi
echo
echo "== godog references =="
rg -n "godog|Features|TestMain|claims-only-ers|pending|skip|beforeScenario|afterScenario|Run" tests-bdd . --glob '!vendor/**' --glob '!node_modules/**' | head -250Repository: opentdf/platform
Length of output: 32525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests-bdd/platform_test.go =="
cat -n tests-bdd/platform_test.go
echo
echo "== read-only parsing/test suite option model =="
python3 - <<'PY'
from pathlib import Path
p = Path("tests-bdd/platform_test.go")
text = p.read_text()
for needle in ["var opts = godog.Options{", "opts.Summary", "opts.Concurrent", "opts.ScenarioEvents", "features := godog.Options{Features:", "godog.TestSuite{"]:
print(f"contains {needle!r}: {needle in text}")
PY
echo
echo "== any pending/skip tags for this feature =="
rg -n '`@pending`|`@skip`|claims-only-ers|stateless|godog.tags' tests-bdd/README.md tests-bdd/platform_test.go tests-bdd/features/claims-only-ers.featureRepository: opentdf/platform
Length of output: 6629
Quarantine the known-failing feature from normal BDD runs.
tests-bdd/platform_test.go runs Godog with the default options and no Options.Tags filter, so @claims-only-ers is included in normal BDD invocations despite the in-file note that the scenarios currently fail. Add an executable pending/skip control and exclude it until #3790 is fixed.
🤖 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-bdd/features/claims-only-ers.feature` around lines 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`.
Comment on lines
+52
to
+73
| Scenario: Claims entity with marketing department gets DENY for engineering resource | ||
| Given I submit a request to create a namespace with name "claims-deny.test" and reference id "ns_claims_deny" | ||
| And I send a request to create an attribute with: | ||
| | namespace_id | name | rule | values | | ||
| | ns_claims_deny | department | anyOf | engineering,marketing,security | | ||
| Then the response should be successful | ||
| Given a condition group referenced as "cg_claims_deny" with an "or" operator with conditions: | ||
| | selector_value | operator | values | | ||
| | .department | in | engineering | | ||
| And a subject set referenced as "ss_claims_deny" containing the condition groups "cg_claims_deny" | ||
| And I send a request to create a subject condition set referenced as "scs_claims_deny" containing subject sets "ss_claims_deny" | ||
| And I send a request to create a subject mapping with: | ||
| | reference_id | attribute_value | condition_set_name | standard actions | custom actions | | ||
| | sm_claims_deny | https://claims-deny.test/attr/department/value/engineering | scs_claims_deny | read | | | ||
| Then the response should be successful | ||
| Given there is a "claims" subject entity referenced as "bob_claims" with claims: | ||
| """ | ||
| {"@type":"type.googleapis.com/google.protobuf.Struct","value":{"userName":"bob","department":"marketing"}} | ||
| """ | ||
| When I send a decision request for entity chain "bob_claims" for "read" action on resource "https://claims-deny.test/attr/department/value/engineering" | ||
| Then the response should be successful | ||
| And I should get a "DENY" decision response |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the marketing case prove claims resolution.
DENY is also the documented result when JWT claims are absent, so this passes even if Bob’s department is never resolved. Add a marketing PERMIT case or assert the resolved claim before relying on this negative case.
🤖 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-bdd/features/claims-only-ers.feature` around lines 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.
2 tasks
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
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.@claims-only-ersEntity_Claimsentity with inline department should get PERMIT, but gets DENY becauseResolveEntitiesdoes not populateJWTClaimsContextKeyin context. This test will pass once fix(ers): ResolveEntities does not populate JWTClaimsContextKey for claims provider #3790 is fixed.Also adds a DocString-based step definition for creating claims entities with embedded JSON (the existing step regex rejects double quotes).
Test results (local,
PLATFORM_IMAGE=DEBUG)@multi-strategy-ers(existing)@multi-strategy-ers-failfast@claims-ldap-fallback-ers@claims-only-ersRelated
ResolveEntitiesdoes not populateJWTClaimsContextKeyfor claims providerTest plan
@multi-strategy-ers-failfastand@claims-ldap-fallback-ersshould pass@claims-only-ersexpected to have 1 failure (diana PERMIT scenario) until fix(ers): ResolveEntities does not populate JWTClaimsContextKey for claims provider #3790 is fixed@claims-only-ers— all scenarios should pass🤖 Generated with Claude Code
Summary by CodeRabbit