feat(ci): Enhance authorization bdd test coverage#3298
Open
elizabethhealy wants to merge 57 commits intomainfrom
Open
feat(ci): Enhance authorization bdd test coverage#3298elizabethhealy wants to merge 57 commits intomainfrom
elizabethhealy wants to merge 57 commits intomainfrom
Conversation
…-rr-aav-bdd-namespaced-policy-tests
…-rr-aav-bdd-namespaced-policy-tests
…-rr-aav-bdd-namespaced-policy-tests
…-rr-aav-bdd-namespaced-policy-tests
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
…-rr-aav-bdd-namespaced-policy-tests
…-1311-add-more-authorization-bdd-tests
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
…-rr-aav-bdd-namespaced-policy-tests
…-1311-add-more-authorization-bdd-tests
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
…-rr-aav-bdd-namespaced-policy-tests
…-1311-add-more-authorization-bdd-tests
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Base automatically changed from
dspx-2753-rr-aav-bdd-namespaced-policy-tests
to
main
April 22, 2026 20:52
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/entityresolution/claims/v2/entity_resolution_test.go`:
- Around line 96-153: Add a third test (e.g.,
Test_ClaimsResolveEntityDirectEntitlementsMalformed) in
entity_resolution_test.go that builds a claims struct with malformed/partial
"direct_entitlements" (e.g., an entry missing "attribute_value_fqn" and another
with "actions" as a non-array), call claims.EntityResolution(...) with the
feature flag enabled (true), and assert the function safely ignores malformed
entries (no panic/error and resulting
entityRepresentations[0].GetDirectEntitlements() does not include invalid items
— expect empty or only valid entries) to pin down parser skip/error behavior;
reference claims.EntityResolution and the existing
Test_ClaimsResolveEntityDirectEntitlements/Test_ClaimsResolveEntityDirectEntitlementsDisabled
for structure.
In `@service/entityresolution/claims/v2/entity_resolution.go`:
- Around line 262-298: In parseDirectEntitlementActions, the case handling
[]string is effectively unreachable when inputs come from structpb
(Struct.AsMap/Value_ListValue yields []interface{}); either remove the []string
branch to avoid confusion or keep it but add a brief comment above the case
noting it is defensive for non-structpb inputs, so future readers understand why
it exists; reference the function name parseDirectEntitlementActions and the
[]string case to locate the change.
- Around line 38-49: Replace the stdlib log.Fatalf call in RegisterClaimsERS
with a panic using fmt.Sprintf so startup errors are raised consistently with
other registry-aware ERS constructors; when mapstructure.Decode returns an
error, keep the existing logger.Error line and then call
panic(fmt.Sprintf("Failed to decode claims entity resolution configuration: %v",
err)) so the service registry can catch and handle the panic (refer to
RegisterClaimsERS, mapstructure.Decode and logger.Error in this function).
In `@tests-bdd/cukes/steps_authorization.go`:
- Around line 493-545: In parseClaimsTable, avoid silently swallowing JSON parse
errors: when rawValue is non-empty and looks JSON-like (e.g., starts with '{' or
'[' or a digit) attempt json.Unmarshal and if it fails return that unmarshal
error instead of falling back to the raw string; only use parsed = rawValue as a
fallback for non-JSON-like tokens. Update the json.Unmarshal error handling in
parseClaimsTable to detect JSON-like rawValue and propagate the error for those
cases while preserving the existing string fallback for plain tokens.
- Around line 129-135: The local variable named "entity" shadows the imported
package "entity"; rename the local variable (e.g., to entityObj or
subjectEntity) where it's defined in the Entity literal so it no longer
conflicts with the package, update all subsequent uses (including
scenarioContext.RecordObject(referenceID, ...)) to the new name, and ensure
references to package-level identifiers like entity.Entity_CATEGORY_SUBJECT and
entity.Entity_Claims remain as package-qualified names.
🪄 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
Run ID: b1a5b8cd-7ba8-49cb-b6f5-0e4baf2f576e
📒 Files selected for processing (11)
service/entityresolution/claims/v2/entity_resolution.goservice/entityresolution/claims/v2/entity_resolution_test.gotests-bdd/cukes/resources/platform.direct_entitlements.templatetests-bdd/cukes/steps_authorization.gotests-bdd/cukes/steps_obligations.gotests-bdd/cukes/steps_registeredresources.gotests-bdd/features/attribute-rules.featuretests-bdd/features/direct-entitlements.featuretests-bdd/features/multi-resource.featuretests-bdd/features/obligations.featuretests-bdd/features/registered-resources.feature
💤 Files with no reviewable changes (1)
- tests-bdd/cukes/steps_obligations.go
Comment on lines
+96
to
+153
| func Test_ClaimsResolveEntityDirectEntitlements(t *testing.T) { | ||
| customclaims := map[string]interface{}{ | ||
| "direct_entitlements": []interface{}{ | ||
| map[string]interface{}{ | ||
| "attribute_value_fqn": "https://example.com/attr/department/value/eng", | ||
| "actions": []interface{}{"read", "update"}, | ||
| }, | ||
| }, | ||
| } | ||
| structClaims, err := structpb.NewStruct(customclaims) | ||
| require.NoError(t, err) | ||
|
|
||
| anyClaims, err := anypb.New(structClaims) | ||
| require.NoError(t, err) | ||
|
|
||
| var validBody []*entity.Entity | ||
| validBody = append(validBody, &entity.Entity{EphemeralId: "1234", EntityType: &entity.Entity_Claims{Claims: anyClaims}}) | ||
|
|
||
| req := entityresolutionV2.ResolveEntitiesRequest{Entities: validBody} | ||
|
|
||
| resp, reserr := claims.EntityResolution(t.Context(), &req, logger.CreateTestLogger(), true) | ||
| require.NoError(t, reserr) | ||
|
|
||
| entityRepresentations := resp.GetEntityRepresentations() | ||
| require.Len(t, entityRepresentations, 1) | ||
|
|
||
| entitlements := entityRepresentations[0].GetDirectEntitlements() | ||
| require.Len(t, entitlements, 1) | ||
| assert.Equal(t, "https://example.com/attr/department/value/eng", entitlements[0].GetAttributeValueFqn()) | ||
| assert.ElementsMatch(t, []string{"read", "update"}, entitlements[0].GetActions()) | ||
| } | ||
|
|
||
| func Test_ClaimsResolveEntityDirectEntitlementsDisabled(t *testing.T) { | ||
| customclaims := map[string]interface{}{ | ||
| "direct_entitlements": []interface{}{ | ||
| map[string]interface{}{ | ||
| "attribute_value_fqn": "https://example.com/attr/department/value/eng", | ||
| "actions": []interface{}{"read"}, | ||
| }, | ||
| }, | ||
| } | ||
| structClaims, err := structpb.NewStruct(customclaims) | ||
| require.NoError(t, err) | ||
|
|
||
| anyClaims, err := anypb.New(structClaims) | ||
| require.NoError(t, err) | ||
|
|
||
| req := entityresolutionV2.ResolveEntitiesRequest{Entities: []*entity.Entity{ | ||
| {EphemeralId: "1234", EntityType: &entity.Entity_Claims{Claims: anyClaims}}, | ||
| }} | ||
|
|
||
| resp, reserr := claims.EntityResolution(t.Context(), &req, logger.CreateTestLogger(), false) | ||
| require.NoError(t, reserr) | ||
|
|
||
| entityRepresentations := resp.GetEntityRepresentations() | ||
| require.Len(t, entityRepresentations, 1) | ||
| assert.Empty(t, entityRepresentations[0].GetDirectEntitlements()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — good positive/negative coverage for the new flag.
The two new tests tightly mirror the enabled (true) and disabled (false) configuration paths introduced in EntityResolution, and assert.ElementsMatch on actions correctly avoids coupling to ordering. Paired with the assert.Empty(... GetDirectEntitlements()) on the disabled path, these protect against regressions in either direction (flag off accidentally emitting entitlements, or flag on dropping them).
Optional follow-up (nice-to-have, not blocking): consider a third case that exercises malformed/partial direct_entitlements claim content (e.g., missing attribute_value_fqn, non-array actions) so the parser's error/skip behavior is pinned down by a test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/entityresolution/claims/v2/entity_resolution_test.go` around lines 96
- 153, Add a third test (e.g.,
Test_ClaimsResolveEntityDirectEntitlementsMalformed) in
entity_resolution_test.go that builds a claims struct with malformed/partial
"direct_entitlements" (e.g., an entry missing "attribute_value_fqn" and another
with "actions" as a non-array), call claims.EntityResolution(...) with the
feature flag enabled (true), and assert the function safely ignores malformed
entries (no panic/error and resulting
entityRepresentations[0].GetDirectEntitlements() does not include invalid items
— expect empty or only valid entries) to pin down parser skip/error behavior;
reference claims.EntityResolution and the existing
Test_ClaimsResolveEntityDirectEntitlements/Test_ClaimsResolveEntityDirectEntitlementsDisabled
for structure.
Comment on lines
+38
to
49
| func RegisterClaimsERS(cfg config.ServiceConfig, logger *logger.Logger) (EntityResolutionServiceV2, serviceregistry.HandlerServer) { | ||
| var inputConfig Config | ||
| if err := mapstructure.Decode(cfg, &inputConfig); err != nil { | ||
| logger.Error("failed to decode claims entity resolution configuration", slog.Any("error", err)) | ||
| log.Fatalf("Failed to decode claims entity resolution configuration: %v", err) | ||
| } | ||
| claimsSVC := EntityResolutionServiceV2{ | ||
| logger: logger, | ||
| allowDirectEntitlements: inputConfig.AllowDirectEntitlements, | ||
| } | ||
| return claimsSVC, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other ERS constructors handle Config decode errors.
rg -nP --type=go -C5 'func Register(Keycloak|MultiStrategy|Claims)ERS' service/entityresolution/
# Look for existing log.Fatalf usage in service registry paths to gauge convention.
rg -nP --type=go -C2 'log\.Fatalf' service/Repository: opentdf/platform
Length of output: 12178
Replace log.Fatalf with panic for consistent config decode error handling.
The current log.Fatalf call bypasses deferred cleanup in the server boot path and double-logs (slog Error + stdlib log.Fatalf). Registry-aware ERS constructors like RegisterMultiStrategyERSV2 use panic(fmt.Sprintf(...)) instead, which allows the service registry to catch and handle startup errors gracefully. Align this with that pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/entityresolution/claims/v2/entity_resolution.go` around lines 38 -
49, Replace the stdlib log.Fatalf call in RegisterClaimsERS with a panic using
fmt.Sprintf so startup errors are raised consistently with other registry-aware
ERS constructors; when mapstructure.Decode returns an error, keep the existing
logger.Error line and then call panic(fmt.Sprintf("Failed to decode claims
entity resolution configuration: %v", err)) so the service registry can catch
and handle the panic (refer to RegisterClaimsERS, mapstructure.Decode and
logger.Error in this function).
Comment on lines
+262
to
+298
| func parseDirectEntitlementActions(raw interface{}) ([]string, error) { | ||
| actions := make([]string, 0) | ||
| switch typed := raw.(type) { | ||
| case []interface{}: | ||
| for _, action := range typed { | ||
| actionStr, ok := action.(string) | ||
| if !ok { | ||
| return nil, errors.New("action must be a string") | ||
| } | ||
| actionStr = strings.TrimSpace(strings.ToLower(actionStr)) | ||
| if actionStr != "" { | ||
| actions = append(actions, actionStr) | ||
| } | ||
| } | ||
| case []string: | ||
| for _, action := range typed { | ||
| action = strings.TrimSpace(strings.ToLower(action)) | ||
| if action != "" { | ||
| actions = append(actions, action) | ||
| } | ||
| } | ||
| case string: | ||
| for _, action := range strings.Split(typed, ",") { | ||
| action = strings.TrimSpace(strings.ToLower(action)) | ||
| if action != "" { | ||
| actions = append(actions, action) | ||
| } | ||
| } | ||
| default: | ||
| return nil, errors.New("actions must be an array or string") | ||
| } | ||
|
|
||
| if len(actions) == 0 { | ||
| return nil, errors.New("no actions provided") | ||
| } | ||
| return actions, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: the []string branch is unreachable via structpb.Struct.AsMap().
structpb.Value_ListValue.AsInterface() produces []interface{} — never []string. The case []string (line 276-282) is defensive dead code from this caller's perspective. Safe to keep for robustness if you envision non-structpb inputs later, but worth a brief comment or removal to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/entityresolution/claims/v2/entity_resolution.go` around lines 262 -
298, In parseDirectEntitlementActions, the case handling []string is effectively
unreachable when inputs come from structpb (Struct.AsMap/Value_ListValue yields
[]interface{}); either remove the []string branch to avoid confusion or keep it
but add a brief comment above the case noting it is defensive for non-structpb
inputs, so future readers understand why it exists; reference the function name
parseDirectEntitlementActions and the []string case to locate the change.
Comment on lines
+129
to
+135
| entity := &entity.Entity{ | ||
| EphemeralId: referenceID, | ||
| Category: entity.Entity_CATEGORY_SUBJECT, | ||
| EntityType: &entity.Entity_Claims{Claims: anyClaims}, | ||
| } | ||
| scenarioContext.RecordObject(referenceID, entity) | ||
| return ctx, nil |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid shadowing the entity package name with a local variable.
entity := &entity.Entity{...} compiles (the new variable's scope begins after the := statement per Go spec, so entity.Entity_CATEGORY_SUBJECT and entity.Entity_Claims on the RHS still bind to the package), but the shadowing is confusing and brittle — any future edit that moves these field references outside the struct literal, or adds another statement that references the package between this line and the end of the function, will silently re-bind to the local variable or fail to compile. The rest of the file already avoids this (see iAddClaimsToSubjectEntityWith using entityObj).
♻️ Rename the local variable to avoid shadowing
- entity := &entity.Entity{
+ ent := &entity.Entity{
EphemeralId: referenceID,
Category: entity.Entity_CATEGORY_SUBJECT,
EntityType: &entity.Entity_Claims{Claims: anyClaims},
}
- scenarioContext.RecordObject(referenceID, entity)
+ scenarioContext.RecordObject(referenceID, ent)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entity := &entity.Entity{ | |
| EphemeralId: referenceID, | |
| Category: entity.Entity_CATEGORY_SUBJECT, | |
| EntityType: &entity.Entity_Claims{Claims: anyClaims}, | |
| } | |
| scenarioContext.RecordObject(referenceID, entity) | |
| return ctx, nil | |
| ent := &entity.Entity{ | |
| EphemeralId: referenceID, | |
| Category: entity.Entity_CATEGORY_SUBJECT, | |
| EntityType: &entity.Entity_Claims{Claims: anyClaims}, | |
| } | |
| scenarioContext.RecordObject(referenceID, ent) | |
| return ctx, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests-bdd/cukes/steps_authorization.go` around lines 129 - 135, The local
variable named "entity" shadows the imported package "entity"; rename the local
variable (e.g., to entityObj or subjectEntity) where it's defined in the Entity
literal so it no longer conflicts with the package, update all subsequent uses
(including scenarioContext.RecordObject(referenceID, ...)) to the new name, and
ensure references to package-level identifiers like
entity.Entity_CATEGORY_SUBJECT and entity.Entity_Claims remain as
package-qualified names.
Comment on lines
+493
to
+545
| func parseClaimsTable(tbl *godog.Table) (map[string]interface{}, error) { | ||
| if tbl == nil || len(tbl.Rows) == 0 { | ||
| return nil, errors.New("claims table is empty") | ||
| } | ||
|
|
||
| cellMap := map[string]int{} | ||
| for ri, row := range tbl.Rows { | ||
| if ri == 0 { | ||
| for ci, cell := range row.Cells { | ||
| cellMap[cell.Value] = ci | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| nameIdx, ok := cellMap["name"] | ||
| if !ok { | ||
| return nil, errors.New("claims table requires column name") | ||
| } | ||
| valueIdx, ok := cellMap["value"] | ||
| if !ok { | ||
| return nil, errors.New("claims table requires column value") | ||
| } | ||
|
|
||
| out := map[string]interface{}{} | ||
| for ri, row := range tbl.Rows { | ||
| if ri == 0 { | ||
| continue | ||
| } | ||
| key := strings.TrimSpace(row.Cells[nameIdx].Value) | ||
| if key == "" { | ||
| return nil, errors.New("claims table requires name values") | ||
| } | ||
| rawValue := "" | ||
| if valueIdx < len(row.Cells) { | ||
| rawValue = strings.TrimSpace(row.Cells[valueIdx].Value) | ||
| } | ||
|
|
||
| var parsed interface{} | ||
| if rawValue != "" { | ||
| if err := json.Unmarshal([]byte(rawValue), &parsed); err != nil { | ||
| parsed = rawValue | ||
| } | ||
| } | ||
| out[key] = parsed | ||
| } | ||
|
|
||
| if len(out) == 0 { | ||
| return nil, errors.New("claims table has no rows") | ||
| } | ||
|
|
||
| return out, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: json.Unmarshal fallback into parsed = rawValue hides typed parse errors.
When rawValue is non-empty but not valid JSON, the value is silently stored as a raw string. That's fine for scalar inputs, but if a test author intends to pass a JSON object/array and there's a typo (trailing comma, single quotes, etc.), the string-fallback may produce a passing scenario that never actually exercises the claim shape you expected. Consider returning the unmarshal error when rawValue looks JSON-like (starts with {, [, or a digit) and only falling back to string for plain tokens.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests-bdd/cukes/steps_authorization.go` around lines 493 - 545, In
parseClaimsTable, avoid silently swallowing JSON parse errors: when rawValue is
non-empty and looks JSON-like (e.g., starts with '{' or '[' or a digit) attempt
json.Unmarshal and if it fails return that unmarshal error instead of falling
back to the raw string; only use parsed = rawValue as a fallback for
non-JSON-like tokens. Update the json.Unmarshal error handling in
parseClaimsTable to detect JSON-like rawValue and propagate the error for those
cases while preserving the existing string fallback for plain tokens.
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.
Proposed Changes
Checklist
Testing Instructions
Summary by CodeRabbit
New Features
Tests