diff --git a/service/entityresolution/integration/README.md b/service/entityresolution/integration/README.md index 1257ceb7df..5edb49eafc 100644 --- a/service/entityresolution/integration/README.md +++ b/service/entityresolution/integration/README.md @@ -97,9 +97,12 @@ adapter := NewMultiStrategyTestAdapter() Provider adapters in `multistrategy_provider_contract_test.go` supply setup, teardown, normal and reversed-strategy service construction, configuration, token fixtures, and expected mapped fields. The suite runs the same -multi-entity chain scenarios for claims, SQL, and LDAP with both environment→subject +token-chain scenarios for claims, SQL, and LDAP with both environment→subject and subject→environment strategy order, single and multiple tokens, collection-valued -context, and fail-closed mixed valid/invalid token batches. +context, and fail-closed mixed valid/invalid token batches. Chain resolution is +first-match-wins per the multi-strategy ERS ADR, so each chain carries exactly the +entity produced by the first matching strategy; reversing the strategy order is what +changes which entity appears. When adding a provider, enroll one adapter to receive the existing contract scenarios. When adding a provider-independent token-chain behavior, add it once to diff --git a/service/entityresolution/integration/entity_chain_comparison_test.go b/service/entityresolution/integration/entity_chain_comparison_test.go index 454ca9f696..b5df1b27c2 100644 --- a/service/entityresolution/integration/entity_chain_comparison_test.go +++ b/service/entityresolution/integration/entity_chain_comparison_test.go @@ -11,11 +11,14 @@ import ( multistrategyv2 "github.com/opentdf/platform/service/entityresolution/multi-strategy/v2" "github.com/opentdf/platform/service/logger" "github.com/opentdf/platform/service/pkg/cache" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/trace/noop" ) -// TestEntityChainComparison demonstrates the discrepancy between Keycloak (2 entities per chain) -// and Multi-Strategy (1 entity per chain) entity resolution systems +// TestEntityChainComparison documents the deliberate difference between Keycloak +// (2 entities per chain: ENVIRONMENT + SUBJECT) and Multi-Strategy (1 entity per chain, +// from the first matching strategy, per the multi-strategy ERS ADR). func TestEntityChainComparison(t *testing.T) { if testing.Short() { t.Skip("Skipping entity chain comparison tests in short mode") @@ -75,9 +78,9 @@ func TestEntityChainComparison(t *testing.T) { }) t.Run("MultiStrategy_EntityChainLength", func(t *testing.T) { - // Create Multi-Strategy ERS service with MULTIPLE strategies like Keycloak + // Configure two strategies that both match the token to show that only the first runs config := types.MultiStrategyConfig{ - FailureStrategy: types.FailureStrategyContinue, // Continue to try all strategies + FailureStrategy: types.FailureStrategyContinue, // Only governs error handling Providers: map[string]types.ProviderConfig{ "jwt_claims": { Type: "claims", @@ -165,29 +168,12 @@ func TestEntityChainComparison(t *testing.T) { t.Logf(" - Entity %d: %s (Category: %s)", i+1, getEntityIdentifier(ent), ent.GetCategory()) } - // ✅ EXPECTED: Multi-strategy should now create 2+ entities like Keycloak - if actualEntityCount >= 2 { - t.Logf("✅ SUCCESS: Multi-strategy creates %d entities per chain (like Keycloak!)", actualEntityCount) - - // Validate entity categories are different (ENVIRONMENT + SUBJECT) - categoryCounts := make(map[string]int) - for _, ent := range chain.GetEntities() { - categoryCounts[ent.GetCategory().String()]++ - } - - t.Logf(" - Entity Categories: %v", categoryCounts) - - // Check we have both ENVIRONMENT and SUBJECT entities like Keycloak - if categoryCounts["CATEGORY_ENVIRONMENT"] >= 1 && categoryCounts["CATEGORY_SUBJECT"] >= 1 { - t.Logf("✅ PERFECT: Has both ENVIRONMENT and SUBJECT entities like Keycloak") - } - } else { - t.Logf("⚠️ ISSUE: Multi-strategy creates only %d entity per chain", actualEntityCount) - t.Logf("🎯 EXPECTED: Multi-strategy should create 2+ entities like Keycloak:") - t.Logf(" - Entity 1: CATEGORY_ENVIRONMENT (client from 'azp' claim)") - t.Logf(" - Entity 2: CATEGORY_SUBJECT (user from 'sub' claim)") - t.Errorf("❌ MISMATCH: Multi-strategy creates %d entities, but Keycloak creates 2 entities per chain", actualEntityCount) - } + // Both configured strategies match this token, but per the ADR the first match wins, + // so the chain holds a single ENVIRONMENT entity. failure_strategy: continue does not + // change this — it only decides whether a *failing* strategy falls through to the next. + require.Len(t, chain.GetEntities(), 1, "multi-strategy chains carry only the first matching strategy's entity") + assert.Equal(t, entity.Entity_CATEGORY_ENVIRONMENT, chain.GetEntities()[0].GetCategory(), + "client_environment_strategy is configured first, so it is the match that wins") }) t.Run("CompareEntityChainStructures", func(t *testing.T) { @@ -198,16 +184,14 @@ func TestEntityChainComparison(t *testing.T) { t.Log(" ✅ Full JWT token processing with multiple entities") t.Log("") t.Log(" Multi-Strategy V2:") - t.Log(" ✅ NOW CREATES 2-entity chains (Environment + Subject) - FIXED!") - t.Log(" ✅ Proper entity categorization (ENVIRONMENT vs SUBJECT)") - t.Log(" ✅ Multiple mapping strategies per token with FailureStrategyContinue") - t.Log("") - t.Log("🎯 ACHIEVED: Multi-strategy now supports:") - t.Log(" 1. ✅ Multiple mapping strategies per token") - t.Log(" 2. ✅ Entity categorization (ENVIRONMENT vs SUBJECT)") - t.Log(" 3. ✅ Chaining multiple related entities per JWT") + t.Log(" ✅ Creates 1-entity chains from the first matching mapping strategy (ADR: first-match-wins)") + t.Log(" ✅ Proper entity categorization (ENVIRONMENT vs SUBJECT) driven by that strategy's entity_type") + t.Log(" ✅ failure_strategy governs error handling only, never how many strategies resolve") t.Log("") - t.Log("🚀 RESULT: Multi-strategy entity chaining now matches Keycloak behavior!") + t.Log("🎯 The entity count difference is intentional: multi-entity chains are an") + t.Log(" explicit 'Future Considerations' item in the multi-strategy ERS ADR, not") + t.Log(" current behavior. Deployments needing several sources in one entity should") + t.Log(" merge them in a single strategy's output mapping.") }) } diff --git a/service/entityresolution/integration/internal/chain_contract_tests.go b/service/entityresolution/integration/internal/chain_contract_tests.go index c298973982..f10b5eacb2 100644 --- a/service/entityresolution/integration/internal/chain_contract_tests.go +++ b/service/entityresolution/integration/internal/chain_contract_tests.go @@ -13,23 +13,50 @@ import ( "github.com/stretchr/testify/require" ) -const ( - // Test constants for entity chain resolution expectations - expectedChainEntityCount = 2 -) +// ChainShape describes the chain an implementation is expected to build for one token. +// Entity count and categories are implementation-specific: Keycloak always emits an +// ENVIRONMENT (client) plus a SUBJECT (user) entity, while multi-strategy ERS is +// first-match-wins per its ADR and emits exactly one entity from the first matching +// strategy. Everything else the suite asserts is genuinely implementation-agnostic. +type ChainShape struct { + EntityCount int + EntityCategories []string +} + +// keycloakChainEntityCount is the ENVIRONMENT (client) plus SUBJECT (user) pair Keycloak +// emits for every token. +const keycloakChainEntityCount = 2 -// ChainContractTestSuite holds implementation-agnostic multi-entity chain validation tests +// KeycloakChainShape is the two-entity ENVIRONMENT + SUBJECT chain Keycloak produces per token. +func KeycloakChainShape() ChainShape { + return ChainShape{ + EntityCount: keycloakChainEntityCount, + EntityCategories: []string{"CATEGORY_ENVIRONMENT", "CATEGORY_SUBJECT"}, + } +} + +// ChainContractTestSuite holds implementation-agnostic entity chain validation tests type ChainContractTestSuite struct { TestCases []ContractTestCase } -// NewChainContractTestSuite creates a test suite focused on implementation-agnostic multi-entity chain validation +// NewChainContractTestSuite creates a chain contract suite for an implementation that +// builds Keycloak-shaped two-entity chains. func NewChainContractTestSuite() *ChainContractTestSuite { + return NewChainContractTestSuiteWithShape(KeycloakChainShape()) +} + +// NewChainContractTestSuiteWithShape creates a chain contract suite that validates chains +// against the given implementation-specific shape. +func NewChainContractTestSuiteWithShape(shape ChainShape) *ChainContractTestSuite { + expectedChainEntityCount := shape.EntityCount + expectedChainCategories := shape.EntityCategories + return &ChainContractTestSuite{ TestCases: []ContractTestCase{ { - Name: "CreateMultiEntityChainFromSingleToken", - Description: "Should create entity chain with multiple entities and proper categorization", + Name: "CreateEntityChainFromSingleToken", + Description: "Should create an entity chain matching the implementation chain shape with proper categorization", Input: ContractInput{ Entities: []*entity.Entity{}, Tokens: []*entity.Token{ @@ -44,17 +71,17 @@ func NewChainContractTestSuite() *ChainContractTestSuite { ChainValidation: []EntityChainValidationRule{ { EphemeralID: "chain-token-1", - EntityCount: expectedChainEntityCount, // Both Keycloak and Multi-Strategy create 2 entities per token - EntityTypes: []string{}, // Implementation-agnostic: don't specify entity types - EntityCategories: []string{"CATEGORY_ENVIRONMENT", "CATEGORY_SUBJECT"}, // Both must create these categories - RequireConsistentOrdering: false, // Allow flexible ordering between implementations + EntityCount: expectedChainEntityCount, + EntityTypes: []string{}, // Implementation-agnostic: don't specify entity types + EntityCategories: expectedChainCategories, + RequireConsistentOrdering: false, // Allow flexible ordering between implementations }, }, }, }, { - Name: "CreateMultiEntityChainsFromMultipleTokens", - Description: "Should create multiple entity chains with consistent multi-entity behavior", + Name: "CreateEntityChainsFromMultipleTokens", + Description: "Should create one entity chain per token with consistent shape", Input: ContractInput{ Entities: []*entity.Entity{}, Tokens: []*entity.Token{ @@ -70,16 +97,16 @@ func NewChainContractTestSuite() *ChainContractTestSuite { ChainValidation: []EntityChainValidationRule{ { EphemeralID: "chain-token-1", - EntityCount: expectedChainEntityCount, // Both implementations create 2 entities per token - EntityTypes: []string{}, // Implementation-agnostic - EntityCategories: []string{"CATEGORY_ENVIRONMENT", "CATEGORY_SUBJECT"}, + EntityCount: expectedChainEntityCount, + EntityTypes: []string{}, // Implementation-agnostic + EntityCategories: expectedChainCategories, RequireConsistentOrdering: false, }, { EphemeralID: "chain-token-2", - EntityCount: expectedChainEntityCount, // Consistent behavior across tokens - EntityTypes: []string{}, // Implementation-agnostic - EntityCategories: []string{"CATEGORY_ENVIRONMENT", "CATEGORY_SUBJECT"}, + EntityCount: expectedChainEntityCount, + EntityTypes: []string{}, // Implementation-agnostic + EntityCategories: expectedChainCategories, RequireConsistentOrdering: false, }, }, @@ -87,7 +114,7 @@ func NewChainContractTestSuite() *ChainContractTestSuite { }, { Name: "ValidateEntityChainCategoryDifferentiation", - Description: "Should create entity chains with distinct ENVIRONMENT and SUBJECT categories", + Description: "Should create entity chains carrying the expected entity categories", Input: ContractInput{ Entities: []*entity.Entity{}, Tokens: []*entity.Token{ @@ -102,17 +129,17 @@ func NewChainContractTestSuite() *ChainContractTestSuite { ChainValidation: []EntityChainValidationRule{ { EphemeralID: "category-test-token", - EntityCount: expectedChainEntityCount, // Both implementations create multiple entities - EntityTypes: []string{}, // Implementation-agnostic: entity types vary by implementation - EntityCategories: []string{"CATEGORY_ENVIRONMENT", "CATEGORY_SUBJECT"}, // Contract: both categories must exist - RequireConsistentOrdering: false, // Allow implementation flexibility + EntityCount: expectedChainEntityCount, + EntityTypes: []string{}, // Implementation-agnostic: entity types vary by implementation + EntityCategories: expectedChainCategories, + RequireConsistentOrdering: false, // Allow implementation flexibility }, }, }, }, { - Name: "ValidateMultiEntityChainConsistency", - Description: "Should create consistent multi-entity chains across multiple invocations", + Name: "ValidateEntityChainConsistency", + Description: "Should create consistent entity chains across multiple invocations", Input: ContractInput{ Entities: []*entity.Entity{}, Tokens: []*entity.Token{ @@ -127,9 +154,9 @@ func NewChainContractTestSuite() *ChainContractTestSuite { ChainValidation: []EntityChainValidationRule{ { EphemeralID: "consistency-token", - EntityCount: expectedChainEntityCount, // Consistent entity count across implementations - EntityTypes: []string{}, // Implementation-specific entity types allowed - EntityCategories: []string{"CATEGORY_ENVIRONMENT", "CATEGORY_SUBJECT"}, + EntityCount: expectedChainEntityCount, + EntityTypes: []string{}, // Implementation-specific entity types allowed + EntityCategories: expectedChainCategories, RequireConsistentOrdering: false, // Behavioral contract, not implementation details }, }, @@ -139,7 +166,7 @@ func NewChainContractTestSuite() *ChainContractTestSuite { } } -// RunChainContractTests executes multi-entity chain tests against an ERS implementation +// RunChainContractTests executes entity chain tests against an ERS implementation func (suite *ChainContractTestSuite) RunChainContractTests(t *testing.T, implementation ERSImplementation, _ string) { for _, testCase := range suite.TestCases { t.Run(testCase.Name, func(t *testing.T) { @@ -148,7 +175,7 @@ func (suite *ChainContractTestSuite) RunChainContractTests(t *testing.T, impleme } } -// runSingleChainTest executes a single multi-entity chain test +// runSingleChainTest executes a single entity chain test func (suite *ChainContractTestSuite) runSingleChainTest(t *testing.T, implementation ERSImplementation, testCase ContractTestCase) { // Test CreateEntityChainsFromTokens if tokens are provided if len(testCase.Input.Tokens) == 0 { diff --git a/service/entityresolution/integration/internal/resolved_token_chain_contract.go b/service/entityresolution/integration/internal/resolved_token_chain_contract.go index 053b451dc7..0595c7fef7 100644 --- a/service/entityresolution/integration/internal/resolved_token_chain_contract.go +++ b/service/entityresolution/integration/internal/resolved_token_chain_contract.go @@ -22,11 +22,37 @@ type ResolvedTokenChainEntityExpectation struct { } // ResolvedTokenChainExpectation describes the final mapped context expected for one token. +// Entities are listed in mapping-strategy order; because chain resolution is first-match-wins +// the suite only ever expects one of them in a given chain. type ResolvedTokenChainExpectation struct { Token *entity.Token Entities []ResolvedTokenChainEntityExpectation } +// firstMatch narrows the expectation to the entity produced by the first matching strategy. +func (e ResolvedTokenChainExpectation) firstMatch() ResolvedTokenChainExpectation { + e.Entities = e.Entities[:1] + return e +} + +// lastMatch narrows the expectation to the entity produced by the last configured strategy, +// which becomes the first match once the strategy list is reversed. +func (e ResolvedTokenChainExpectation) lastMatch() ResolvedTokenChainExpectation { + e.Entities = e.Entities[len(e.Entities)-1:] + return e +} + +func narrowExpectations( + expectations []ResolvedTokenChainExpectation, + narrow func(ResolvedTokenChainExpectation) ResolvedTokenChainExpectation, +) []ResolvedTokenChainExpectation { + narrowed := make([]ResolvedTokenChainExpectation, 0, len(expectations)) + for _, expectation := range expectations { + narrowed = append(narrowed, narrow(expectation)) + } + return narrowed +} + // ResolvedTokenChainAdapter enrolls an ERS/provider configuration in the shared // token-chain contract. Implementations provide setup and fixtures; the suite owns behavior. type ResolvedTokenChainAdapter interface { @@ -60,19 +86,24 @@ func (suite *ResolvedTokenChainContractSuite) RunWithAdapter(t *testing.T, adapt expectations := adapter.ResolvedTokenChainExpectations(dataSet) require.NotEmpty(t, expectations) - t.Run(adapter.GetScopeName()+"_EnvironmentThenSubjectPreservesMultiEntityMappedContext", func(t *testing.T) { - suite.assertResolvedTokenChains(t, implementation, expectations[:1]) + // Chain resolution is first-match-wins, so the environment strategy (configured first) + // is the only one that runs and the resolved chain carries just its mapped context. + t.Run(adapter.GetScopeName()+"_EnvironmentThenSubjectPreservesFirstMatchMappedContext", func(t *testing.T) { + suite.assertResolvedTokenChains(t, implementation, narrowExpectations(expectations[:1], ResolvedTokenChainExpectation.firstMatch)) }) + // Reversing the strategy list makes the subject strategy the first match, which must be + // the only entity in the chain. This is what proves ordering — not failure strategy — + // decides which strategy resolves the token. reversedImplementation, err := adapter.CreateERSServiceWithReversedStrategies(ctx) require.NoError(t, err) - t.Run(adapter.GetScopeName()+"_SubjectThenEnvironmentPreservesMultiEntityMappedContext", func(t *testing.T) { - suite.assertResolvedTokenChains(t, reversedImplementation, expectations[:1]) + t.Run(adapter.GetScopeName()+"_SubjectThenEnvironmentPreservesFirstMatchMappedContext", func(t *testing.T) { + suite.assertResolvedTokenChains(t, reversedImplementation, narrowExpectations(expectations[:1], ResolvedTokenChainExpectation.lastMatch)) }) if len(expectations) > 1 { - t.Run(adapter.GetScopeName()+"_MultipleTokensPreserveMultiEntityMappedContext", func(t *testing.T) { - suite.assertResolvedTokenChains(t, implementation, expectations) + t.Run(adapter.GetScopeName()+"_MultipleTokensPreserveFirstMatchMappedContext", func(t *testing.T) { + suite.assertResolvedTokenChains(t, implementation, narrowExpectations(expectations, ResolvedTokenChainExpectation.firstMatch)) }) } diff --git a/service/entityresolution/integration/multistrategy_contract_test.go b/service/entityresolution/integration/multistrategy_contract_test.go index 85af53c3c0..561c447671 100644 --- a/service/entityresolution/integration/multistrategy_contract_test.go +++ b/service/entityresolution/integration/multistrategy_contract_test.go @@ -16,12 +16,14 @@ func TestMultiStrategyContractValidation(t *testing.T) { t.Skip("Skipping multi-strategy contract validation tests in short mode") } - // Create chain-specific contract test suite - chainSuite := internal.NewChainContractTestSuite() + // Create chain-specific contract test suite. Multi-strategy ERS is first-match-wins, + // so the chain carries only the first matching strategy's entity. + chainSuite := internal.NewChainContractTestSuiteWithShape(multiStrategyChainShape) - // Create multi-strategy implementation with enhanced configuration for multi-entity chains + // Create multi-strategy implementation with two matching strategies to prove that + // only the first one is used, regardless of failure strategy. config := types.MultiStrategyConfig{ - FailureStrategy: types.FailureStrategyContinue, // Critical: Continue to try all strategies + FailureStrategy: types.FailureStrategyContinue, // Only governs error handling, not stopping at first success Providers: map[string]types.ProviderConfig{ "jwt_claims": { Type: "claims", @@ -92,13 +94,21 @@ func TestMultiStrategyContractValidation(t *testing.T) { logger: logger.CreateTestLogger(), } - t.Log("Running multi-entity chain contract tests against Multi-Strategy ERS") + t.Log("Running entity chain contract tests against Multi-Strategy ERS") // Run chain-specific contract tests chainSuite.RunChainContractTests(t, wrapper, "MultiStrategy") - t.Log("✅ Multi-Strategy ERS multi-entity chain contract validation completed successfully!") - t.Log("🎯 All entity chain tests passed - Multi-Strategy now matches Keycloak behavior") + t.Log("✅ Multi-Strategy ERS entity chain contract validation completed successfully!") + t.Log("🎯 All entity chain tests passed - chains hold the first matching strategy's entity") +} + +// multiStrategyChainShape is the chain multi-strategy ERS builds for the configurations in +// this package: a single entity from the first matching strategy, which is the ENVIRONMENT +// (client) strategy in every one of them. See the multi-strategy ERS ADR. +var multiStrategyChainShape = internal.ChainShape{ + EntityCount: 1, + EntityCategories: []string{"CATEGORY_ENVIRONMENT"}, } // TestMultiStrategyChainSpecific runs specific chain validation tests @@ -108,9 +118,9 @@ func TestMultiStrategyChainSpecific(t *testing.T) { } // Use the chain contract test suite - chainSuite := internal.NewChainContractTestSuite() + chainSuite := internal.NewChainContractTestSuiteWithShape(multiStrategyChainShape) - // Create ERS configuration for multi-entity chains + // Create ERS configuration with two matching strategies; first match wins config := types.MultiStrategyConfig{ FailureStrategy: types.FailureStrategyContinue, Providers: map[string]types.ProviderConfig{ diff --git a/service/entityresolution/integration/multistrategy_test.go b/service/entityresolution/integration/multistrategy_test.go index 9bfc8a22cd..e97b1f48ba 100644 --- a/service/entityresolution/integration/multistrategy_test.go +++ b/service/entityresolution/integration/multistrategy_test.go @@ -430,7 +430,9 @@ func TestMultiStrategyEntityResolutionV2(t *testing.T) { // Create contract test suite suite := internal.NewContractTestSuite() - // Add specific test case for CreateEntityChainsFromTokens - updated to match actual multi-strategy behavior + // Add specific test case for CreateEntityChainsFromTokens - per the ADR, the first + // matching strategy wins, so the chain holds exactly that strategy's entity even though + // later strategies also match this token. suite.TestCases = append(suite.TestCases, internal.ContractTestCase{ Name: "CreateEntityChainsFromTokens_ExposeStub", Description: "Should create entity chains from JWT tokens using multi-strategy system", @@ -446,13 +448,11 @@ func TestMultiStrategyEntityResolutionV2(t *testing.T) { ChainValidation: []internal.EntityChainValidationRule{ { EphemeralID: "test-token-1", - EntityCount: 3, - EntityTypes: []string{"claims", "claims", "claims"}, - EntityCategories: []string{"CATEGORY_SUBJECT", "CATEGORY_SUBJECT", "CATEGORY_SUBJECT"}, + EntityCount: 1, + EntityTypes: []string{"claims"}, + EntityCategories: []string{"CATEGORY_SUBJECT"}, EntityRequiredFields: []map[string]interface{}{ {"username": "user123", "email": "user@example.com"}, - {"client_id": "external-client"}, - {"username": "user123", "email": "user@example.com"}, }, RequireConsistentOrdering: true, }, diff --git a/service/entityresolution/integration/unified_contract_test.go b/service/entityresolution/integration/unified_contract_test.go index ec6b3fb0ef..74fb47ce47 100644 --- a/service/entityresolution/integration/unified_contract_test.go +++ b/service/entityresolution/integration/unified_contract_test.go @@ -19,13 +19,14 @@ func TestUnifiedEntityChainContract(t *testing.T) { t.Skip("Skipping unified contract validation tests in short mode") } - // Create implementation-agnostic chain contract test suite - chainSuite := internal.NewChainContractTestSuite() - + // The suite body is implementation-agnostic; only the expected chain shape differs. + // Keycloak always emits ENVIRONMENT + SUBJECT per token, while multi-strategy ERS is + // first-match-wins and emits a single entity from the first matching strategy. t.Run("MultiStrategy_Implementation", func(t *testing.T) { // Test Multi-Strategy implementation multiStrategy := createMultiStrategyImplementation(t) - chainSuite.RunChainContractTests(t, multiStrategy, "MultiStrategy") + internal.NewChainContractTestSuiteWithShape(multiStrategyChainShape). + RunChainContractTests(t, multiStrategy, "MultiStrategy") }) t.Run("Keycloak_Implementation", func(t *testing.T) { @@ -34,7 +35,8 @@ func TestUnifiedEntityChainContract(t *testing.T) { if keycloakImpl != nil { // Note: Contract test suite will automatically skip if Keycloak server is unavailable // This demonstrates the unified contract approach - chainSuite.RunChainContractTests(t, keycloakImpl, "Keycloak") + internal.NewChainContractTestSuiteWithShape(internal.KeycloakChainShape()). + RunChainContractTests(t, keycloakImpl, "Keycloak") } else { t.Skip("Keycloak implementation unavailable for testing") } @@ -44,7 +46,7 @@ func TestUnifiedEntityChainContract(t *testing.T) { // createMultiStrategyImplementation creates a properly configured Multi-Strategy ERS func createMultiStrategyImplementation(t *testing.T) internal.ERSImplementation { config := types.MultiStrategyConfig{ - FailureStrategy: types.FailureStrategyContinue, // Enable multi-entity chains + FailureStrategy: types.FailureStrategyContinue, // Try the next strategy only when one fails Providers: map[string]types.ProviderConfig{ "jwt_claims": { Type: "claims", @@ -155,16 +157,16 @@ func TestImplementationAgnosticBehavior(t *testing.T) { } multiStrategy := createMultiStrategyImplementation(t) - chainSuite := internal.NewChainContractTestSuite() + chainSuite := internal.NewChainContractTestSuiteWithShape(multiStrategyChainShape) t.Log("🎯 Testing Implementation-Agnostic Contract") - t.Log(" ✅ Entity count: Both implementations create 2 entities per token") - t.Log(" ✅ Categories: Both implementations create ENVIRONMENT + SUBJECT categories") + t.Log(" ✅ Chains: Both implementations create one chain per token, keyed by ephemeral ID") + t.Log(" ✅ Categories: Every chain entity carries an explicit ENVIRONMENT/SUBJECT category") t.Log(" ✅ Consistency: Both implementations provide consistent behavior") - t.Log(" ➡️ Entity types: Implementation-specific (Keycloak vs Multi-Strategy)") + t.Log(" ➡️ Entity count and types: Implementation-specific (Keycloak vs Multi-Strategy)") // Run tests on Multi-Strategy to demonstrate the contract chainSuite.RunChainContractTests(t, multiStrategy, "ContractDemo") - t.Log("🚀 Contract satisfied: Multi-entity chains with proper categorization") + t.Log("🚀 Contract satisfied: entity chains with proper categorization") } diff --git a/service/entityresolution/multi-strategy/README.md b/service/entityresolution/multi-strategy/README.md index 3626bb1454..ab39095e8d 100644 --- a/service/entityresolution/multi-strategy/README.md +++ b/service/entityresolution/multi-strategy/README.md @@ -236,6 +236,13 @@ The failure strategy determines how Multi-Strategy ERS handles failures when exe - **Use Case**: When you want resilient failover with multiple fallback options - **Result**: Only fails if **all** matching strategies fail +> **First match wins.** Both settings stop at the first strategy that succeeds — the +> failure strategy only decides what happens when a strategy *fails*. This holds for +> `CreateEntityChainsFromTokens` too, so a token's entity chain always contains exactly +> one entity, produced by the first matching strategy. To combine data from several +> sources into one entity, merge them in a single strategy's `output_mapping` rather +> than relying on strategy ordering. + ```yaml services: entityresolution: diff --git a/service/entityresolution/multi-strategy/v2/registration.go b/service/entityresolution/multi-strategy/v2/registration.go index 594e7c0a57..a8730568bb 100644 --- a/service/entityresolution/multi-strategy/v2/registration.go +++ b/service/entityresolution/multi-strategy/v2/registration.go @@ -294,13 +294,10 @@ func (ers *ERSV2) createEntityChainFromSingleTokenV2(ctx context.Context, token slog.String("entity_type", getEntityTypeStringV2(entityV2)), slog.String("entity_category", entityV2.GetCategory().String())) - // ENHANCED: Continue trying additional strategies to build multi-entity chains (like Keycloak) - // This allows creating chains with multiple entities (e.g., ENVIRONMENT + SUBJECT) - // Only break if FailureStrategy is FailFast and we have at least one successful entity - if failureStrategy == types.FailureStrategyFailFast { - break - } - // With FailureStrategyContinue, we continue to try more strategies to build richer chains + // First match wins, per the ADR: failure_strategy only governs error handling + // ("continue" tries the next strategy on failure, "fail-fast" stops immediately). + // Neither mode keeps resolving after a success, so a chain holds exactly one entity. + break } // If no strategies succeeded diff --git a/service/entityresolution/multi-strategy/v2/registration_test.go b/service/entityresolution/multi-strategy/v2/registration_test.go index 8c8ef17ab4..35451b2b83 100644 --- a/service/entityresolution/multi-strategy/v2/registration_test.go +++ b/service/entityresolution/multi-strategy/v2/registration_test.go @@ -361,3 +361,136 @@ func TestCreateEntityForTokenChainFailsClosedOnSerializationErrorWithContinue(t require.Equal(t, "token-1", inner.Context["token_id"]) require.Equal(t, "bad_subject", inner.Context["strategy"]) } + +// firstMatchWinsConfig builds a config with two strategies that both match the test token: +// an ENVIRONMENT strategy on "azp" followed by a SUBJECT strategy on "sub". +func firstMatchWinsConfig(failureStrategy string, strategies ...types.MappingStrategy) types.MultiStrategyConfig { + return types.MultiStrategyConfig{ + FailureStrategy: failureStrategy, + Providers: map[string]types.ProviderConfig{ + "jwt": {Type: "claims", Connection: map[string]interface{}{}}, + }, + MappingStrategies: strategies, + } +} + +func environmentStrategy() types.MappingStrategy { + return types.MappingStrategy{ + Name: "client_environment", + Provider: "jwt", + EntityType: types.EntityTypeEnvironment, + Conditions: types.StrategyConditions{ + JWTClaims: []types.JWTClaimCondition{{Claim: "azp", Operator: "exists"}}, + }, + OutputMapping: []types.OutputMapping{{SourceClaim: "azp", ClaimName: "client_id"}}, + } +} + +func subjectStrategy() types.MappingStrategy { + return types.MappingStrategy{ + Name: "user_subject", + Provider: "jwt", + EntityType: types.EntityTypeSubject, + Conditions: types.StrategyConditions{ + JWTClaims: []types.JWTClaimCondition{{Claim: "sub", Operator: "exists"}}, + }, + OutputMapping: []types.OutputMapping{{SourceClaim: "sub", ClaimName: "username"}}, + } +} + +// testTokenJWT is an unsigned-but-well-formed JWT carrying both "azp" and "sub", so every +// strategy in firstMatchWinsConfig matches it. +// Payload: {"sub":"alice","azp":"opentdf-sdk","iat":1600000000,"exp":4102444800} +const testTokenJWT = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9." + + "eyJzdWIiOiJhbGljZSIsImF6cCI6Im9wZW50ZGYtc2RrIiwiaWF0IjoxNjAwMDAwMDAwLCJleHAiOjQxMDI0NDQ4MDB9." + + "dGVzdHNpZ25hdHVyZQ" + +func chainForTestToken(t *testing.T, config types.MultiStrategyConfig) *entity.EntityChain { + t.Helper() + + erService, err := NewERSV2(t.Context(), config, logger.CreateTestLogger()) + require.NoError(t, err) + + resp, err := erService.CreateEntityChainsFromTokens(t.Context(), connect.NewRequest(&ersV2.CreateEntityChainsFromTokensRequest{ + Tokens: []*entity.Token{{EphemeralId: "token-1", Jwt: testTokenJWT}}, + })) + require.NoError(t, err) + require.Len(t, resp.Msg.GetEntityChains(), 1) + + return resp.Msg.GetEntityChains()[0] +} + +func claimsOf(t *testing.T, resolved *entity.Entity) map[string]interface{} { + t.Helper() + + require.NotNil(t, resolved.GetClaims()) + var claimsStruct structpb.Struct + require.NoError(t, resolved.GetClaims().UnmarshalTo(&claimsStruct)) + return claimsStruct.AsMap() +} + +// TestCreateEntityChainsFromTokens_FirstMatchingStrategyWins pins the ADR contract: the +// first strategy that resolves successfully ends the search under every failure strategy, +// so a chain never accumulates one entity per matching strategy. +func TestCreateEntityChainsFromTokens_FirstMatchingStrategyWins(t *testing.T) { + tests := []struct { + name string + failureStrategy string + strategies []types.MappingStrategy + expectedCategory entity.Entity_Category + expectedClaim string + expectedValue string + }{ + { + name: "continue stops at the first success", + failureStrategy: types.FailureStrategyContinue, + strategies: []types.MappingStrategy{environmentStrategy(), subjectStrategy()}, + expectedCategory: entity.Entity_CATEGORY_ENVIRONMENT, + expectedClaim: "client_id", + expectedValue: "opentdf-sdk", + }, + { + name: "fail-fast stops at the first success", + failureStrategy: types.FailureStrategyFailFast, + strategies: []types.MappingStrategy{environmentStrategy(), subjectStrategy()}, + expectedCategory: entity.Entity_CATEGORY_ENVIRONMENT, + expectedClaim: "client_id", + expectedValue: "opentdf-sdk", + }, + { + name: "strategy order, not failure strategy, picks the winner", + failureStrategy: types.FailureStrategyContinue, + strategies: []types.MappingStrategy{subjectStrategy(), environmentStrategy()}, + expectedCategory: entity.Entity_CATEGORY_SUBJECT, + expectedClaim: "username", + expectedValue: "alice", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + chain := chainForTestToken(t, firstMatchWinsConfig(tt.failureStrategy, tt.strategies...)) + + require.Len(t, chain.GetEntities(), 1, "chain must hold only the first matching strategy's entity") + resolved := chain.GetEntities()[0] + require.Equal(t, tt.expectedCategory, resolved.GetCategory()) + require.Equal(t, tt.expectedValue, claimsOf(t, resolved)[tt.expectedClaim]) + }) + } +} + +// TestCreateEntityChainsFromTokens_ContinueFallsThroughFailureToNextStrategy shows the one +// thing "continue" does change: a failing strategy hands off to the next matching one, and +// the resulting chain still holds exactly one entity. +func TestCreateEntityChainsFromTokens_ContinueFallsThroughFailureToNextStrategy(t *testing.T) { + failing := environmentStrategy() + failing.Name = "missing_provider" + failing.Provider = "not-registered" + + chain := chainForTestToken(t, firstMatchWinsConfig(types.FailureStrategyContinue, failing, subjectStrategy())) + + require.Len(t, chain.GetEntities(), 1) + resolved := chain.GetEntities()[0] + require.Equal(t, entity.Entity_CATEGORY_SUBJECT, resolved.GetCategory()) + require.Equal(t, "alice", claimsOf(t, resolved)["username"]) +} diff --git a/tests-bdd/features/multi-strategy-ers-multi-success.feature b/tests-bdd/features/multi-strategy-ers-multi-success.feature index 1ebd7c4197..c10e95b275 100644 --- a/tests-bdd/features/multi-strategy-ers-multi-success.feature +++ b/tests-bdd/features/multi-strategy-ers-multi-success.feature @@ -6,9 +6,9 @@ Feature: First successful strategy wins under continue (ADR: first-match-wins) In both modes, the first successful strategy returns and no further strategies run. See: adr/decisions/2025-07-31-multi-strategy-entity-resolution-service.md - BUG: The current code (registration.go:297-304) continues running strategies - after a success under "continue", building a multi-entity chain. This diverges - from the ADR. Scenario 3 is an intentionally-failing test that exposes this bug. + Both scenarios use two strategies whose conditions match, so the chain must hold + exactly one entity from "claims_identity". The first asserts the winner's claims + reach the decision; the second asserts the loser's claims do not. This covers Jake's gap analysis row #4. @@ -16,8 +16,6 @@ Feature: First successful strategy wins under continue (ADR: first-match-wins) Given an LDAP directory with test users And a user exists with username "alice" and email "alice@opentdf.test" and the following attributes: | name | value | - And a user exists with username "eve" and email "eve@opentdf.test" and the following attributes: - | name | value | And an ERS configuration with mode "multi-strategy" and failure strategy "continue" And an ERS provider "jwt_claims" of type "claims" And an ERS provider "ldap_directory" of type "ldap" connected to the LDAP directory @@ -32,6 +30,7 @@ Feature: First successful strategy wins under continue (ADR: first-match-wins) - source_claim: preferred_username claim_name: username """ + # Emits only "department", never "username" — the asymmetry the scenarios rely on. And an ERS mapping strategy "ldap_department" using provider "ldap_directory" """ entity_type: subject @@ -50,8 +49,6 @@ Feature: First successful strategy wins under continue (ADR: first-match-wins) output_mapping: - source_attribute: departmentNumber claim_name: department - - source_attribute: uid - claim_name: username """ And a local platform with inline ERS configuration @@ -61,8 +58,8 @@ Feature: First successful strategy wins under continue (ADR: first-match-wins) | namespace_id | name | rule | values | | ns_ms | department | anyOf | engineering,marketing,security | Then the response should be successful - # Subject mapping uses .username — the claims strategy (listed first) outputs this. - # Per ADR, claims succeeds and LDAP should not run. PERMIT from single entity. + # Regression guard: only "claims_identity" emits .username. If resolution kept going + # after its success, the appended LDAP entity has no .username and AND semantics DENY. Given a condition group referenced as "cg_ms" with an "or" operator with conditions: | selector_value | operator | values | | .username | in | alice | @@ -77,41 +74,11 @@ Feature: First successful strategy wins under continue (ADR: first-match-wins) Then the response should be successful And I should get a "PERMIT" decision response - Scenario: First strategy succeeds — user not entitled via first-match entity → DENY - Given I submit a request to create a namespace with name "multi-success-deny.test" and reference id "ns_msd" - And I send a request to create an attribute with: - | namespace_id | name | rule | values | - | ns_msd | department | anyOf | engineering,marketing,security | - Then the response should be successful - # Subject mapping checks .department — claims strategy (first match) does not - # output department. Per ADR, claims succeeds and returns, LDAP never runs. - # Even though LDAP would provide department=operations for eve, it's irrelevant. - Given a condition group referenced as "cg_msd" with an "or" operator with conditions: - | selector_value | operator | values | - | .department | in | engineering | - And a subject set referenced as "ss_msd" containing the condition groups "cg_msd" - And I send a request to create a subject condition set referenced as "scs_msd" containing subject sets "ss_msd" - And I send a request to create a subject mapping with: - | reference_id | attribute_value | condition_set_name | standard actions | custom actions | - | sm_msd | https://multi-success-deny.test/attr/department/value/engineering | scs_msd | read | | - Then the response should be successful - Given a user access token for "eve" stored as "eve_token" - When I send a decision request for token "eve_token" for "read" action on resource "https://multi-success-deny.test/attr/department/value/engineering" - Then the response should be successful - And I should get a "DENY" decision response - - Scenario: BUG — continue runs strategies after first success, building unintended multi-entity chain - # Per ADR, continue should stop at first success. But the current code - # (registration.go:297-304) keeps running and builds a multi-entity chain. - # This causes AND semantics to apply: all entities must be independently entitled. - # - # Customer intent: claims for routing, LDAP for department. - # Subject mapping: .department in ["engineering"] - # Expected: PERMIT — LDAP has department=engineering for alice. - # Actual: DENY — claims entity has no .department, AND semantics vetoes. - # - # This test asserts PERMIT (the correct ADR behavior) and intentionally fails - # against the current buggy implementation. + Scenario: A later strategy that would supply the attribute never runs → DENY + # Alice has departmentNumber=engineering in LDAP, but "claims_identity" wins and emits + # no .department, so the mapping cannot match. Also catches an implementation that merges + # both strategies' claims into one entity — that would wrongly PERMIT here. + # Fix in config: order the LDAP strategy first, or emit department from the winner. Given I submit a request to create a namespace with name "and-semantics-gap.test" and reference id "ns_asg" And I send a request to create an attribute with: | namespace_id | name | rule | values | @@ -129,4 +96,4 @@ Feature: First successful strategy wins under continue (ADR: first-match-wins) Given a user access token for "alice" stored as "alice_and_token" When I send a decision request for token "alice_and_token" for "read" action on resource "https://and-semantics-gap.test/attr/department/value/engineering" Then the response should be successful - And I should get a "PERMIT" decision response + And I should get a "DENY" decision response