From 5660f013874af9d5a1a2faa5f859528c7d9ec3be Mon Sep 17 00:00:00 2001 From: jakedoublev Date: Mon, 10 Nov 2025 14:35:36 -0800 Subject: [PATCH 1/6] fix(authz): deny resources granularly when attribute value FQNs not found --- service/internal/access/v2/evaluate.go | 30 +- service/internal/access/v2/evaluate_test.go | 240 +++++++++++++++- service/internal/access/v2/pdp_test.go | 304 ++++++++++++++++++++ 3 files changed, 566 insertions(+), 8 deletions(-) diff --git a/service/internal/access/v2/evaluate.go b/service/internal/access/v2/evaluate.go index f1a57fd296..e787fa62ec 100644 --- a/service/internal/access/v2/evaluate.go +++ b/service/internal/access/v2/evaluate.go @@ -55,11 +55,6 @@ func getResourceDecision( slog.Any("resource", resource.GetResource()), ) - if len(accessibleAttributeValues) == 0 { - l.WarnContext(ctx, "resource is not able to be entitled", slog.Any("resource", resource.GetResource())) - return failure, nil - } - switch resource.GetResource().(type) { case *authz.Resource_RegisteredResourceValueFqn: registeredResourceValueFQN = strings.ToLower(resource.GetRegisteredResourceValueFqn()) @@ -110,6 +105,12 @@ func getResourceDecision( return nil, fmt.Errorf("unsupported resource type: %w", ErrInvalidResource) } + // Cannot entitle any resource + if len(accessibleAttributeValues) == 0 { + l.WarnContext(ctx, "resource is not able to be entitled", slog.Any("resource", resource.GetResource())) + return failure, nil + } + return evaluateResourceAttributeValues(ctx, l, resourceAttributeValues, resourceID, registeredResourceValueFQN, action, entitlements, accessibleAttributeValues) } @@ -128,17 +129,34 @@ func evaluateResourceAttributeValues( // Group value FQNs by parent definition definitionFqnToValueFqns := make(map[string][]string) definitionsLookup := make(map[string]*policy.Attribute) + notFoundFQNs := make([]string, 0) for _, valueFQN := range resourceAttributeValues.GetFqns() { attributeAndValue, ok := accessibleAttributeValues[valueFQN] if !ok { - return nil, fmt.Errorf("%w: %s", ErrFQNNotFound, valueFQN) + notFoundFQNs = append(notFoundFQNs, valueFQN) + continue } definition := attributeAndValue.GetAttribute() definitionFqnToValueFqns[definition.GetFqn()] = append(definitionFqnToValueFqns[definition.GetFqn()], valueFQN) definitionsLookup[definition.GetFqn()] = definition } + // If ANY FQNs are missing, DENY the resource + if len(notFoundFQNs) > 0 { + l.WarnContext(ctx, "attribute value FQN(s) not found - denying access to resource", + slog.Any("not_found_fqns", notFoundFQNs), + slog.String("resource_id", resourceID)) + result := &ResourceDecision{ + Entitled: false, + ResourceID: resourceID, + } + if resourceName != "" { + result.ResourceName = resourceName + } + return result, nil + } + // Evaluate each definition by rule, resource attributes, action, and entitlements passed := true dataRuleResults := make([]DataRuleResult, 0) diff --git a/service/internal/access/v2/evaluate_test.go b/service/internal/access/v2/evaluate_test.go index 783edcbc8e..d988cc688e 100644 --- a/service/internal/access/v2/evaluate_test.go +++ b/service/internal/access/v2/evaluate_test.go @@ -761,8 +761,9 @@ func (s *EvaluateTestSuite) TestEvaluateResourceAttributeValues() { entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{ levelMidFQN: []*policy.Action{actionRead}, }, + // Should NOT error - but should DENY resource (ANY missing FQN = DENY) expectAccessible: false, - expectError: true, + expectError: false, }, } @@ -788,13 +789,23 @@ func (s *EvaluateTestSuite) TestEvaluateResourceAttributeValues() { s.Equal(tc.expectAccessible, resourceDecision.Entitled) // Check results array has the correct length based on grouping by definition + // If ANY FQN is missing, DataRuleResults should be empty (resource is denied without evaluation) definitions := make(map[string]bool) + allFQNsExist := true for _, fqn := range tc.resourceAttrs.GetFqns() { if attrAndValue, ok := s.accessibleAttrValues[fqn]; ok { definitions[attrAndValue.GetAttribute().GetFqn()] = true + } else { + allFQNsExist = false } } - s.Len(resourceDecision.DataRuleResults, len(definitions)) + + if allFQNsExist { + s.Len(resourceDecision.DataRuleResults, len(definitions)) + } else { + // Any missing FQN means DENY without evaluation + s.Len(resourceDecision.DataRuleResults, 0) + } } }) } @@ -943,3 +954,228 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() { }) } } + +// Test_evaluateResourceAttributeValues_AllFQNsNotFound tests that when all attribute FQNs +// in a resource don't exist in accessibleAttributeValues, the resource is DENIED (not error). +// This ensures fine-grained decisions where individual resources with non-existent FQNs +// are denied without failing the entire request. +func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_AllFQNsNotFound() { + // Create FQNs that don't exist in the system + nonExistentFQN1 := createAttrValueFQN(baseNamespace, "classification", "topsecret") + nonExistentFQN2 := createAttrValueFQN(baseNamespace, "classification", "secret") + + resourceAttributeValues := &authz.Resource_AttributeValues{ + Fqns: []string{ + nonExistentFQN1, + nonExistentFQN2, + }, + } + + // Empty accessible attributes map simulating FQNs not found + emptyAccessibleAttributes := make(map[string]*attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue) + + entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{} + + // Call evaluateResourceAttributeValues + decision, err := evaluateResourceAttributeValues( + s.T().Context(), + s.logger, + resourceAttributeValues, + "resource-1", + "", + s.action, + entitlements, + emptyAccessibleAttributes, + ) + + // Should NOT return an error - should return DENY decision + s.Require().NoError(err, "Should not return error when FQNs not found") + s.Require().NotNil(decision, "Decision should not be nil") + s.False(decision.Entitled, "Resource should be DENIED when all FQNs not found") + s.Equal("resource-1", decision.ResourceID) +} + +// Test_evaluateResourceAttributeValues_PartialFQNsNotFound tests that when some (but not all) +// attribute FQNs don't exist, the resource is DENIED (not evaluated with partial data). +func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_PartialFQNsNotFound() { + nonExistentFQN := createAttrValueFQN(baseNamespace, "classification", "topsecret") + + resourceAttributeValues := &authz.Resource_AttributeValues{ + Fqns: []string{ + levelMidFQN, // This exists + nonExistentFQN, // This doesn't exist + levelHighestFQN, // This exists + }, + } + + // Entity is entitled to the existing FQNs + entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{ + levelMidFQN: []*policy.Action{actionRead}, + levelHighestFQN: []*policy.Action{actionRead}, + } + + decision, err := evaluateResourceAttributeValues( + s.T().Context(), + s.logger, + resourceAttributeValues, + "resource-2", + "", + s.action, + entitlements, + s.accessibleAttrValues, + ) + + // Should NOT return an error - but should DENY the resource (any missing FQN = DENY) + s.Require().NoError(err, "Should not return error with partial FQNs missing") + s.Require().NotNil(decision, "Decision should not be nil") + // ANY missing FQN means DENY - we don't evaluate with partial data + s.False(decision.Entitled, "Resource should be DENIED when ANY FQN is missing") + s.Equal("resource-2", decision.ResourceID) +} + +// Test_getResourceDecision_AttributeValueFQNsNotFound tests the full flow through +// getResourceDecision when attribute value FQNs don't exist. +func (s *EvaluateTestSuite) Test_getResourceDecision_AttributeValueFQNsNotFound() { + nonExistentFQN1 := createAttrValueFQN(baseNamespace, "clearance", "cosmic") + nonExistentFQN2 := createAttrValueFQN(baseNamespace, "clearance", "ultrasecret") + + resource := &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{nonExistentFQN1, nonExistentFQN2}, + }, + }, + EphemeralId: "resource-with-missing-fqns", + } + + emptyAccessibleAttributes := make(map[string]*attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue) + entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{} + + decision, err := getResourceDecision( + s.T().Context(), + s.logger, + emptyAccessibleAttributes, + s.accessibleRegisteredResourceValues, + entitlements, + s.action, + resource, + ) + + // Should NOT error - should return DENY decision + s.Require().NoError(err, "getResourceDecision should not error on missing FQNs") + s.Require().NotNil(decision, "Decision should not be nil") + s.False(decision.Entitled, "Resource with missing FQNs should be DENIED") + s.Equal("resource-with-missing-fqns", decision.ResourceID) +} + +// Test_getResourceDecision_RegisteredResourceValueFQNNotFound verifies that when a +// registered resource value FQN doesn't exist, the resource is DENIED (not error). +// This pattern should be consistent with attribute value FQN handling. +func (s *EvaluateTestSuite) Test_getResourceDecision_RegisteredResourceValueFQNNotFound() { + nonExistentRegResFQN := createRegisteredResourceValueFQN("secret-system", "classified") + + resource := &authz.Resource{ + Resource: &authz.Resource_RegisteredResourceValueFqn{ + RegisteredResourceValueFqn: nonExistentRegResFQN, + }, + EphemeralId: "resource-with-missing-reg-res", + } + + entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{ + levelHighestFQN: []*policy.Action{actionRead}, + } + + decision, err := getResourceDecision( + s.T().Context(), + s.logger, + s.accessibleAttrValues, + s.accessibleRegisteredResourceValues, + entitlements, + s.action, + resource, + ) + + // Should NOT error - should return DENY decision + s.Require().NoError(err, "getResourceDecision should not error on missing registered resource FQN") + s.Require().NotNil(decision, "Decision should not be nil") + s.False(decision.Entitled, "Resource with missing registered resource FQN should be DENIED") + s.Equal("resource-with-missing-reg-res", decision.ResourceID) + s.Equal(nonExistentRegResFQN, decision.ResourceName) +} + +// Test_getResourceDecision_MixedResources_IndividualDenials tests that in a batch of +// resources, those with missing FQNs are individually denied without affecting others. +// This validates fine-grained decision-making. +func (s *EvaluateTestSuite) Test_getResourceDecision_MixedResources_IndividualDenials() { + nonExistentFQN := createAttrValueFQN(baseNamespace, "clearance", "cosmic") + + // Resource 1: Valid FQN, entity is entitled + resource1 := &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{levelHighestFQN}, + }, + }, + EphemeralId: "valid-resource-1", + } + + // Resource 2: Non-existent FQN + resource2 := &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{nonExistentFQN}, + }, + }, + EphemeralId: "invalid-resource-2", + } + + // Resource 3: Valid FQN, entity is entitled + resource3 := &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{levelMidFQN}, + }, + }, + EphemeralId: "valid-resource-3", + } + + entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{ + levelHighestFQN: []*policy.Action{actionRead}, + levelMidFQN: []*policy.Action{actionRead}, + } + + // Process each resource individually (simulating batch processing) + testCases := []struct { + name string + resource *authz.Resource + expectedEntitled bool + }{ + {"valid resource 1", resource1, true}, + {"invalid resource 2 (missing FQN)", resource2, false}, + {"valid resource 3", resource3, true}, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + // For resource 2, we need empty accessible attributes to simulate FQN not found + accessibleAttrs := s.accessibleAttrValues + if tc.resource.GetEphemeralId() == "invalid-resource-2" { + accessibleAttrs = make(map[string]*attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue) + } + + decision, err := getResourceDecision( + s.T().Context(), + s.logger, + accessibleAttrs, + s.accessibleRegisteredResourceValues, + entitlements, + s.action, + tc.resource, + ) + + s.Require().NoError(err, "Should not error for resource: %s", tc.name) + s.Require().NotNil(decision) + s.Equal(tc.expectedEntitled, decision.Entitled, "Entitlement mismatch for: %s", tc.name) + }) + } +} diff --git a/service/internal/access/v2/pdp_test.go b/service/internal/access/v2/pdp_test.go index 1d5c0ea016..55b6eec5c0 100644 --- a/service/internal/access/v2/pdp_test.go +++ b/service/internal/access/v2/pdp_test.go @@ -3304,3 +3304,307 @@ func findResourceDecision(decision *Decision, resourceID string) *ResourceDecisi } return nil } + +// Test_GetDecision_NonExistentAttributeFQN tests that when a resource contains +// an attribute FQN that doesn't exist in the system, the resource is individually +// denied without failing the entire request. +func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { + f := s.fixtures + + // Create a PDP with only classification attribute + pdp, err := NewPolicyDecisionPoint( + s.T().Context(), + s.logger, + []*policy.Attribute{f.classificationAttr}, + []*policy.SubjectMapping{f.secretMapping, f.topSecretMapping}, + []*policy.RegisteredResource{}, + ) + s.Require().NoError(err) + s.Require().NotNil(pdp) + + s.Run("Single resource with non-existent FQN - should DENY", func() { + entity := s.createEntityWithProps("test-user", map[string]interface{}{ + "clearance": "ts", + }) + + // Create resource with FQN that doesn't exist in the system + nonExistentFQN := createAttrValueFQN(testBaseNamespace, "clearance", "cosmic") + resources := []*authz.Resource{ + { + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{nonExistentFQN}, + }, + }, + EphemeralId: "resource-with-missing-fqn", + }, + } + + decision, entitlements, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) + + // Should NOT error - should return decision with resource denied + s.Require().NoError(err, "GetDecision should not error on non-existent FQN") + s.Require().NotNil(decision) + s.False(decision.AllPermitted, "AllPermitted should be false") + s.Len(decision.Results, 1) + + // Verify the resource is denied + s.False(decision.Results[0].Entitled, "Resource with non-existent FQN should be DENIED") + s.Equal("resource-with-missing-fqn", decision.Results[0].ResourceID) + + // Entitlements should still be returned (for the entity's entitled values) + s.Require().NotNil(entitlements) + }) + + s.Run("Multiple resources with mixed valid/invalid FQNs - fine-grained denial", func() { + entity := s.createEntityWithProps("test-user", map[string]interface{}{ + "clearance": "ts", + }) + + // Create resources: 2 valid, 1 with non-existent FQN + nonExistentFQN := createAttrValueFQN(testBaseNamespace, "clearance", "ultrasecret") + resources := []*authz.Resource{ + { + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{testClassSecretFQN}, // Valid - entity is entitled + }, + }, + EphemeralId: "valid-resource-1", + }, + { + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{nonExistentFQN}, // Invalid - doesn't exist + }, + }, + EphemeralId: "invalid-resource-2", + }, + { + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{testClassTopSecretFQN}, // Valid - entity is entitled + }, + }, + EphemeralId: "valid-resource-3", + }, + } + + decision, _, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) + + // Should NOT error + s.Require().NoError(err, "GetDecision should not error with mixed FQNs") + s.Require().NotNil(decision) + s.False(decision.AllPermitted, "AllPermitted should be false (one resource denied)") + s.Len(decision.Results, 3) + + // Verify individual resource decisions + expectedResults := map[string]bool{ + "valid-resource-1": true, // Entitled + "invalid-resource-2": false, // Denied due to non-existent FQN + "valid-resource-3": true, // Entitled + } + + for _, result := range decision.Results { + expected, exists := expectedResults[result.ResourceID] + s.Require().True(exists, "Unexpected resource ID: %s", result.ResourceID) + s.Equal(expected, result.Entitled, "Entitlement mismatch for resource: %s", result.ResourceID) + } + }) +} + +// Test_GetDecision_PartialFQNsInResource tests that when a resource contains +// some valid and some non-existent FQNs, the resource is DENIED (not evaluated with partial data). +func (s *PDPTestSuite) Test_GetDecision_PartialFQNsInResource() { + f := s.fixtures + + // Create a PDP with classification attribute (hierarchical rule) + pdp, err := NewPolicyDecisionPoint( + s.T().Context(), + s.logger, + []*policy.Attribute{f.classificationAttr}, + []*policy.SubjectMapping{f.secretMapping, f.topSecretMapping, f.confidentialMapping}, + []*policy.RegisteredResource{}, + ) + s.Require().NoError(err) + s.Require().NotNil(pdp) + + s.Run("Resource with mix of valid and invalid FQNs - DENIED", func() { + entity := s.createEntityWithProps("test-user", map[string]interface{}{ + "clearance": "ts", + }) + + // Create resource with 2 valid FQNs and 1 non-existent FQN + nonExistentFQN := createAttrValueFQN(testBaseNamespace, "classification", "cosmic") + resources := []*authz.Resource{ + { + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{ + testClassSecretFQN, // Valid + nonExistentFQN, // Non-existent - causes DENY + testClassTopSecretFQN, // Valid + }, + }, + }, + EphemeralId: "mixed-fqn-resource", + }, + } + + decision, _, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) + + // Should NOT error - but should DENY the resource (ANY missing FQN = DENY) + s.Require().NoError(err, "GetDecision should not error with partial invalid FQNs") + s.Require().NotNil(decision) + s.Len(decision.Results, 1) + + // ANY missing FQN means DENY - we don't evaluate with partial data + s.False(decision.AllPermitted, "AllPermitted should be false") + s.False(decision.Results[0].Entitled, "Resource should be DENIED when ANY FQN is missing") + }) +} + +// Test_GetDecisionRegisteredResource_NonExistentFQN tests that when a registered +// resource value FQN doesn't exist, the resource is denied without error. +func (s *PDPTestSuite) Test_GetDecisionRegisteredResource_NonExistentFQN() { + f := s.fixtures + + // Create a PDP with classification attribute + pdp, err := NewPolicyDecisionPoint( + s.T().Context(), + s.logger, + []*policy.Attribute{f.classificationAttr}, + []*policy.SubjectMapping{f.secretMapping}, + []*policy.RegisteredResource{f.classificationRegRes}, // Only classification registered resource + ) + s.Require().NoError(err) + s.Require().NotNil(pdp) + + s.Run("Registered resource with non-existent FQN - should DENY", func() { + entity := s.createEntityWithProps("test-user", map[string]interface{}{ + "clearance": "secret", + }) + + // Create resource with registered resource FQN that doesn't exist + nonExistentRegResFQN := createRegisteredResourceValueFQN("secret-system", "classified") + resources := []*authz.Resource{ + { + Resource: &authz.Resource_RegisteredResourceValueFqn{ + RegisteredResourceValueFqn: nonExistentRegResFQN, + }, + EphemeralId: "missing-reg-res", + }, + } + + decision, _, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) + + // Should NOT error - should return DENY decision + s.Require().NoError(err, "GetDecision should not error on non-existent registered resource FQN") + s.Require().NotNil(decision) + s.False(decision.AllPermitted) + s.Len(decision.Results, 1) + + // Verify the resource is denied + s.False(decision.Results[0].Entitled, "Resource with non-existent registered resource FQN should be DENIED") + s.Equal("missing-reg-res", decision.Results[0].ResourceID) + s.Equal(nonExistentRegResFQN, decision.Results[0].ResourceName) + }) + + s.Run("Mix of valid registered resource and non-existent FQN - fine-grained", func() { + entity := s.createEntityWithProps("test-user", map[string]interface{}{ + "clearance": "secret", + }) + + nonExistentRegResFQN := createRegisteredResourceValueFQN("secret-system", "classified") + resources := []*authz.Resource{ + { + Resource: &authz.Resource_RegisteredResourceValueFqn{ + RegisteredResourceValueFqn: testClassSecretRegResFQN, // Valid + }, + EphemeralId: "valid-reg-res", + }, + { + Resource: &authz.Resource_RegisteredResourceValueFqn{ + RegisteredResourceValueFqn: nonExistentRegResFQN, // Non-existent + }, + EphemeralId: "invalid-reg-res", + }, + } + + decision, _, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) + + // Should NOT error + s.Require().NoError(err) + s.Require().NotNil(decision) + s.False(decision.AllPermitted, "AllPermitted should be false (one resource denied)") + s.Len(decision.Results, 2) + + // Verify individual decisions + for _, result := range decision.Results { + if result.ResourceID == "valid-reg-res" { + s.True(result.Entitled, "Valid registered resource should be PERMITTED") + } else if result.ResourceID == "invalid-reg-res" { + s.False(result.Entitled, "Non-existent registered resource should be DENIED") + } else { + s.Fail("Unexpected resource ID: %s", result.ResourceID) + } + } + }) +} + +// Test_GetDecision_NoPolicies tests that when no policies exist in the system, +// all resources are denied without errors. +func (s *PDPTestSuite) Test_GetDecision_NoPolicies() { + // Create a PDP with NO attributes, NO mappings, NO registered resources + pdp, err := NewPolicyDecisionPoint( + s.T().Context(), + s.logger, + []*policy.Attribute{}, + []*policy.SubjectMapping{}, + []*policy.RegisteredResource{}, + ) + s.Require().NoError(err) + s.Require().NotNil(pdp) + + s.Run("No policies in database - should DENY all resources", func() { + entity := s.createEntityWithProps("test-user", map[string]interface{}{ + "clearance": "ts", + }) + + resources := []*authz.Resource{ + { + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{testClassSecretFQN}, + }, + }, + EphemeralId: "resource-1", + }, + { + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{testClassTopSecretFQN}, + }, + }, + EphemeralId: "resource-2", + }, + } + + decision, entitlements, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) + + // Should NOT error - should return DENY decisions + s.Require().NoError(err, "GetDecision should not error with no policies") + s.Require().NotNil(decision) + s.False(decision.AllPermitted, "AllPermitted should be false with no policies") + s.Len(decision.Results, 2) + + // All resources should be denied + for _, result := range decision.Results { + s.False(result.Entitled, "All resources should be DENIED with no policies") + } + + // Entitlements should be empty (no subject mappings) + s.Require().NotNil(entitlements) + s.Empty(entitlements, "Entitlements should be empty with no policies") + }) +} From 90beb68c99c55f21a48076692a100a9dcc9689f4 Mon Sep 17 00:00:00 2001 From: jakedoublev Date: Mon, 10 Nov 2025 14:52:26 -0800 Subject: [PATCH 2/6] cleanup --- service/internal/access/v2/evaluate_test.go | 78 +++++++-------------- 1 file changed, 27 insertions(+), 51 deletions(-) diff --git a/service/internal/access/v2/evaluate_test.go b/service/internal/access/v2/evaluate_test.go index d988cc688e..ad04ef44cf 100644 --- a/service/internal/access/v2/evaluate_test.go +++ b/service/internal/access/v2/evaluate_test.go @@ -955,14 +955,9 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() { } } -// Test_evaluateResourceAttributeValues_AllFQNsNotFound tests that when all attribute FQNs -// in a resource don't exist in accessibleAttributeValues, the resource is DENIED (not error). -// This ensures fine-grained decisions where individual resources with non-existent FQNs -// are denied without failing the entire request. func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_AllFQNsNotFound() { - // Create FQNs that don't exist in the system - nonExistentFQN1 := createAttrValueFQN(baseNamespace, "classification", "topsecret") - nonExistentFQN2 := createAttrValueFQN(baseNamespace, "classification", "secret") + nonExistentFQN1 := createAttrValueFQN(baseNamespace, "significance", "critical") + nonExistentFQN2 := createAttrValueFQN(baseNamespace, "significance", "major") resourceAttributeValues := &authz.Resource_AttributeValues{ Fqns: []string{ @@ -988,27 +983,24 @@ func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_AllFQNsNotFound emptyAccessibleAttributes, ) - // Should NOT return an error - should return DENY decision - s.Require().NoError(err, "Should not return error when FQNs not found") - s.Require().NotNil(decision, "Decision should not be nil") - s.False(decision.Entitled, "Resource should be DENIED when all FQNs not found") + s.Require().NoError(err) + s.Require().NotNil(decision) + s.False(decision.Entitled) s.Equal("resource-1", decision.ResourceID) } -// Test_evaluateResourceAttributeValues_PartialFQNsNotFound tests that when some (but not all) -// attribute FQNs don't exist, the resource is DENIED (not evaluated with partial data). func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_PartialFQNsNotFound() { - nonExistentFQN := createAttrValueFQN(baseNamespace, "classification", "topsecret") + nonExistentFQN := createAttrValueFQN(baseNamespace, "significance", "high") resourceAttributeValues := &authz.Resource_AttributeValues{ Fqns: []string{ - levelMidFQN, // This exists - nonExistentFQN, // This doesn't exist - levelHighestFQN, // This exists + levelMidFQN, // Exists + nonExistentFQN, // Eoesn't exist + levelHighestFQN, // Exists }, } - // Entity is entitled to the existing FQNs + // Entitled to the existing FQNs entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{ levelMidFQN: []*policy.Action{actionRead}, levelHighestFQN: []*policy.Action{actionRead}, @@ -1025,19 +1017,16 @@ func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_PartialFQNsNotF s.accessibleAttrValues, ) - // Should NOT return an error - but should DENY the resource (any missing FQN = DENY) - s.Require().NoError(err, "Should not return error with partial FQNs missing") - s.Require().NotNil(decision, "Decision should not be nil") - // ANY missing FQN means DENY - we don't evaluate with partial data - s.False(decision.Entitled, "Resource should be DENIED when ANY FQN is missing") + // No error but deny + s.Require().NoError(err) + s.Require().NotNil(decision) + s.False(decision.Entitled) s.Equal("resource-2", decision.ResourceID) } -// Test_getResourceDecision_AttributeValueFQNsNotFound tests the full flow through -// getResourceDecision when attribute value FQNs don't exist. func (s *EvaluateTestSuite) Test_getResourceDecision_AttributeValueFQNsNotFound() { - nonExistentFQN1 := createAttrValueFQN(baseNamespace, "clearance", "cosmic") - nonExistentFQN2 := createAttrValueFQN(baseNamespace, "clearance", "ultrasecret") + nonExistentFQN1 := createAttrValueFQN(baseNamespace, "space", "cosmic") + nonExistentFQN2 := createAttrValueFQN(baseNamespace, "space", "planetary") resource := &authz.Resource{ Resource: &authz.Resource_AttributeValues_{ @@ -1061,18 +1050,15 @@ func (s *EvaluateTestSuite) Test_getResourceDecision_AttributeValueFQNsNotFound( resource, ) - // Should NOT error - should return DENY decision - s.Require().NoError(err, "getResourceDecision should not error on missing FQNs") - s.Require().NotNil(decision, "Decision should not be nil") - s.False(decision.Entitled, "Resource with missing FQNs should be DENIED") + // No error but deny + s.Require().NoError(err) + s.Require().NotNil(decision) + s.False(decision.Entitled) s.Equal("resource-with-missing-fqns", decision.ResourceID) } -// Test_getResourceDecision_RegisteredResourceValueFQNNotFound verifies that when a -// registered resource value FQN doesn't exist, the resource is DENIED (not error). -// This pattern should be consistent with attribute value FQN handling. func (s *EvaluateTestSuite) Test_getResourceDecision_RegisteredResourceValueFQNNotFound() { - nonExistentRegResFQN := createRegisteredResourceValueFQN("secret-system", "classified") + nonExistentRegResFQN := createRegisteredResourceValueFQN("special-system", "classified") resource := &authz.Resource{ Resource: &authz.Resource_RegisteredResourceValueFqn{ @@ -1095,19 +1081,16 @@ func (s *EvaluateTestSuite) Test_getResourceDecision_RegisteredResourceValueFQNN resource, ) - // Should NOT error - should return DENY decision - s.Require().NoError(err, "getResourceDecision should not error on missing registered resource FQN") - s.Require().NotNil(decision, "Decision should not be nil") - s.False(decision.Entitled, "Resource with missing registered resource FQN should be DENIED") + // No error but deny + s.Require().NoError(err) + s.Require().NotNil(decision) + s.False(decision.Entitled) s.Equal("resource-with-missing-reg-res", decision.ResourceID) s.Equal(nonExistentRegResFQN, decision.ResourceName) } -// Test_getResourceDecision_MixedResources_IndividualDenials tests that in a batch of -// resources, those with missing FQNs are individually denied without affecting others. -// This validates fine-grained decision-making. func (s *EvaluateTestSuite) Test_getResourceDecision_MixedResources_IndividualDenials() { - nonExistentFQN := createAttrValueFQN(baseNamespace, "clearance", "cosmic") + nonExistentFQN := createAttrValueFQN(baseNamespace, "space", "cosmic") // Resource 1: Valid FQN, entity is entitled resource1 := &authz.Resource{ @@ -1144,7 +1127,6 @@ func (s *EvaluateTestSuite) Test_getResourceDecision_MixedResources_IndividualDe levelMidFQN: []*policy.Action{actionRead}, } - // Process each resource individually (simulating batch processing) testCases := []struct { name string resource *authz.Resource @@ -1157,16 +1139,10 @@ func (s *EvaluateTestSuite) Test_getResourceDecision_MixedResources_IndividualDe for _, tc := range testCases { s.Run(tc.name, func() { - // For resource 2, we need empty accessible attributes to simulate FQN not found - accessibleAttrs := s.accessibleAttrValues - if tc.resource.GetEphemeralId() == "invalid-resource-2" { - accessibleAttrs = make(map[string]*attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue) - } - decision, err := getResourceDecision( s.T().Context(), s.logger, - accessibleAttrs, + s.accessibleAttrValues, s.accessibleRegisteredResourceValues, entitlements, s.action, From e176c3eb18570dc4eeee500dc8a1f8f0ff1a20d8 Mon Sep 17 00:00:00 2001 From: jakedoublev Date: Mon, 10 Nov 2025 15:02:12 -0800 Subject: [PATCH 3/6] cleanup --- service/internal/access/v2/pdp_test.go | 91 +++++++++++--------------- 1 file changed, 38 insertions(+), 53 deletions(-) diff --git a/service/internal/access/v2/pdp_test.go b/service/internal/access/v2/pdp_test.go index 55b6eec5c0..0b32f70bec 100644 --- a/service/internal/access/v2/pdp_test.go +++ b/service/internal/access/v2/pdp_test.go @@ -3305,9 +3305,6 @@ func findResourceDecision(decision *Decision, resourceID string) *ResourceDecisi return nil } -// Test_GetDecision_NonExistentAttributeFQN tests that when a resource contains -// an attribute FQN that doesn't exist in the system, the resource is individually -// denied without failing the entire request. func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { f := s.fixtures @@ -3327,8 +3324,7 @@ func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { "clearance": "ts", }) - // Create resource with FQN that doesn't exist in the system - nonExistentFQN := createAttrValueFQN(testBaseNamespace, "clearance", "cosmic") + nonExistentFQN := createAttrValueFQN(testBaseNamespace, "space", "cosmic") resources := []*authz.Resource{ { Resource: &authz.Resource_AttributeValues_{ @@ -3342,17 +3338,15 @@ func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { decision, entitlements, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) - // Should NOT error - should return decision with resource denied - s.Require().NoError(err, "GetDecision should not error on non-existent FQN") + // No error, but deny decision + s.Require().NoError(err) s.Require().NotNil(decision) - s.False(decision.AllPermitted, "AllPermitted should be false") + s.False(decision.AllPermitted) s.Len(decision.Results, 1) - // Verify the resource is denied - s.False(decision.Results[0].Entitled, "Resource with non-existent FQN should be DENIED") + // Resource is denied + s.False(decision.Results[0].Entitled) s.Equal("resource-with-missing-fqn", decision.Results[0].ResourceID) - - // Entitlements should still be returned (for the entity's entitled values) s.Require().NotNil(entitlements) }) @@ -3362,7 +3356,7 @@ func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { }) // Create resources: 2 valid, 1 with non-existent FQN - nonExistentFQN := createAttrValueFQN(testBaseNamespace, "clearance", "ultrasecret") + nonExistentFQNBadDefinition := createAttrValueFQN(testBaseNamespace, "severity", "high") resources := []*authz.Resource{ { Resource: &authz.Resource_AttributeValues_{ @@ -3375,7 +3369,7 @@ func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { { Resource: &authz.Resource_AttributeValues_{ AttributeValues: &authz.Resource_AttributeValues{ - Fqns: []string{nonExistentFQN}, // Invalid - doesn't exist + Fqns: []string{nonExistentFQNBadDefinition}, // Invalid - doesn't exist }, }, EphemeralId: "invalid-resource-2", @@ -3393,9 +3387,9 @@ func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { decision, _, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) // Should NOT error - s.Require().NoError(err, "GetDecision should not error with mixed FQNs") + s.Require().NoError(err) s.Require().NotNil(decision) - s.False(decision.AllPermitted, "AllPermitted should be false (one resource denied)") + s.False(decision.AllPermitted) s.Len(decision.Results, 3) // Verify individual resource decisions @@ -3413,8 +3407,6 @@ func (s *PDPTestSuite) Test_GetDecision_NonExistentAttributeFQN() { }) } -// Test_GetDecision_PartialFQNsInResource tests that when a resource contains -// some valid and some non-existent FQNs, the resource is DENIED (not evaluated with partial data). func (s *PDPTestSuite) Test_GetDecision_PartialFQNsInResource() { f := s.fixtures @@ -3434,16 +3426,15 @@ func (s *PDPTestSuite) Test_GetDecision_PartialFQNsInResource() { "clearance": "ts", }) - // Create resource with 2 valid FQNs and 1 non-existent FQN - nonExistentFQN := createAttrValueFQN(testBaseNamespace, "classification", "cosmic") + nonExistentValueFQNBadNamespace := createAttrValueFQN("does.notexist", "classification", "cosmic") resources := []*authz.Resource{ { Resource: &authz.Resource_AttributeValues_{ AttributeValues: &authz.Resource_AttributeValues{ Fqns: []string{ - testClassSecretFQN, // Valid - nonExistentFQN, // Non-existent - causes DENY - testClassTopSecretFQN, // Valid + testClassSecretFQN, // Valid + nonExistentValueFQNBadNamespace, // Non-existent - causes DENY + testClassTopSecretFQN, // Valid }, }, }, @@ -3453,19 +3444,16 @@ func (s *PDPTestSuite) Test_GetDecision_PartialFQNsInResource() { decision, _, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) - // Should NOT error - but should DENY the resource (ANY missing FQN = DENY) - s.Require().NoError(err, "GetDecision should not error with partial invalid FQNs") + // No error, but deny decision + s.Require().NoError(err) s.Require().NotNil(decision) s.Len(decision.Results, 1) - // ANY missing FQN means DENY - we don't evaluate with partial data - s.False(decision.AllPermitted, "AllPermitted should be false") - s.False(decision.Results[0].Entitled, "Resource should be DENIED when ANY FQN is missing") + s.False(decision.AllPermitted) + s.False(decision.Results[0].Entitled) }) } -// Test_GetDecisionRegisteredResource_NonExistentFQN tests that when a registered -// resource value FQN doesn't exist, the resource is denied without error. func (s *PDPTestSuite) Test_GetDecisionRegisteredResource_NonExistentFQN() { f := s.fixtures @@ -3485,8 +3473,7 @@ func (s *PDPTestSuite) Test_GetDecisionRegisteredResource_NonExistentFQN() { "clearance": "secret", }) - // Create resource with registered resource FQN that doesn't exist - nonExistentRegResFQN := createRegisteredResourceValueFQN("secret-system", "classified") + nonExistentRegResFQN := createRegisteredResourceValueFQN("special-system", "classified") resources := []*authz.Resource{ { Resource: &authz.Resource_RegisteredResourceValueFqn{ @@ -3498,14 +3485,14 @@ func (s *PDPTestSuite) Test_GetDecisionRegisteredResource_NonExistentFQN() { decision, _, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) - // Should NOT error - should return DENY decision - s.Require().NoError(err, "GetDecision should not error on non-existent registered resource FQN") + // No error, but deny decision + s.Require().NoError(err) s.Require().NotNil(decision) s.False(decision.AllPermitted) s.Len(decision.Results, 1) - // Verify the resource is denied - s.False(decision.Results[0].Entitled, "Resource with non-existent registered resource FQN should be DENIED") + // Resource is denied + s.False(decision.Results[0].Entitled) s.Equal("missing-reg-res", decision.Results[0].ResourceID) s.Equal(nonExistentRegResFQN, decision.Results[0].ResourceName) }) @@ -3536,26 +3523,25 @@ func (s *PDPTestSuite) Test_GetDecisionRegisteredResource_NonExistentFQN() { // Should NOT error s.Require().NoError(err) s.Require().NotNil(decision) - s.False(decision.AllPermitted, "AllPermitted should be false (one resource denied)") + s.False(decision.AllPermitted) s.Len(decision.Results, 2) // Verify individual decisions for _, result := range decision.Results { - if result.ResourceID == "valid-reg-res" { - s.True(result.Entitled, "Valid registered resource should be PERMITTED") - } else if result.ResourceID == "invalid-reg-res" { - s.False(result.Entitled, "Non-existent registered resource should be DENIED") - } else { + switch result.ResourceID { + case "valid-reg-res": + s.True(result.Entitled) + case "invalid-reg-res": + s.False(result.Entitled) + default: s.Fail("Unexpected resource ID: %s", result.ResourceID) } } }) } -// Test_GetDecision_NoPolicies tests that when no policies exist in the system, -// all resources are denied without errors. func (s *PDPTestSuite) Test_GetDecision_NoPolicies() { - // Create a PDP with NO attributes, NO mappings, NO registered resources + // Empty PDP with no attributes, subject mappings, or registered resources pdp, err := NewPolicyDecisionPoint( s.T().Context(), s.logger, @@ -3566,7 +3552,7 @@ func (s *PDPTestSuite) Test_GetDecision_NoPolicies() { s.Require().NoError(err) s.Require().NotNil(pdp) - s.Run("No policies in database - should DENY all resources", func() { + s.Run("No policy found - should DENY all resources", func() { entity := s.createEntityWithProps("test-user", map[string]interface{}{ "clearance": "ts", }) @@ -3592,19 +3578,18 @@ func (s *PDPTestSuite) Test_GetDecision_NoPolicies() { decision, entitlements, err := pdp.GetDecision(s.T().Context(), entity, testActionRead, resources) - // Should NOT error - should return DENY decisions - s.Require().NoError(err, "GetDecision should not error with no policies") + // No error, but deny decision + s.Require().NoError(err) s.Require().NotNil(decision) - s.False(decision.AllPermitted, "AllPermitted should be false with no policies") + s.False(decision.AllPermitted) s.Len(decision.Results, 2) - // All resources should be denied for _, result := range decision.Results { - s.False(result.Entitled, "All resources should be DENIED with no policies") + s.False(result.Entitled) } - // Entitlements should be empty (no subject mappings) + // Empty entitlements without available policy s.Require().NotNil(entitlements) - s.Empty(entitlements, "Entitlements should be empty with no policies") + s.Empty(entitlements) }) } From 262c0cf39254ab94d4dc1487bee2253b12af70d7 Mon Sep 17 00:00:00 2001 From: jakedoublev Date: Mon, 10 Nov 2025 15:05:03 -0800 Subject: [PATCH 4/6] cleanup --- service/internal/access/v2/evaluate_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/service/internal/access/v2/evaluate_test.go b/service/internal/access/v2/evaluate_test.go index ad04ef44cf..d7a93620b9 100644 --- a/service/internal/access/v2/evaluate_test.go +++ b/service/internal/access/v2/evaluate_test.go @@ -966,12 +966,10 @@ func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_AllFQNsNotFound }, } - // Empty accessible attributes map simulating FQNs not found emptyAccessibleAttributes := make(map[string]*attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue) entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{} - // Call evaluateResourceAttributeValues decision, err := evaluateResourceAttributeValues( s.T().Context(), s.logger, @@ -1017,7 +1015,7 @@ func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_PartialFQNsNotF s.accessibleAttrValues, ) - // No error but deny + // No error, but deny decision s.Require().NoError(err) s.Require().NotNil(decision) s.False(decision.Entitled) @@ -1050,7 +1048,7 @@ func (s *EvaluateTestSuite) Test_getResourceDecision_AttributeValueFQNsNotFound( resource, ) - // No error but deny + // No error, but deny decision s.Require().NoError(err) s.Require().NotNil(decision) s.False(decision.Entitled) @@ -1081,7 +1079,7 @@ func (s *EvaluateTestSuite) Test_getResourceDecision_RegisteredResourceValueFQNN resource, ) - // No error but deny + // No error, but deny decision s.Require().NoError(err) s.Require().NotNil(decision) s.False(decision.Entitled) From 8a7e75b5ef19a3832f7f0c684b7a9eef8799bbcc Mon Sep 17 00:00:00 2001 From: jakedoublev Date: Mon, 10 Nov 2025 15:08:30 -0800 Subject: [PATCH 5/6] lint fixes --- service/internal/access/v2/evaluate_test.go | 41 +++++----- service/internal/access/v2/pdp_test.go | 90 ++++++++++----------- 2 files changed, 66 insertions(+), 65 deletions(-) diff --git a/service/internal/access/v2/evaluate_test.go b/service/internal/access/v2/evaluate_test.go index d7a93620b9..6eec7a2703 100644 --- a/service/internal/access/v2/evaluate_test.go +++ b/service/internal/access/v2/evaluate_test.go @@ -783,30 +783,31 @@ func (s *EvaluateTestSuite) TestEvaluateResourceAttributeValues() { if tc.expectError { s.Require().Error(err) - } else { - s.Require().NoError(err) - s.NotNil(resourceDecision) - s.Equal(tc.expectAccessible, resourceDecision.Entitled) - - // Check results array has the correct length based on grouping by definition - // If ANY FQN is missing, DataRuleResults should be empty (resource is denied without evaluation) - definitions := make(map[string]bool) - allFQNsExist := true - for _, fqn := range tc.resourceAttrs.GetFqns() { - if attrAndValue, ok := s.accessibleAttrValues[fqn]; ok { - definitions[attrAndValue.GetAttribute().GetFqn()] = true - } else { - allFQNsExist = false - } - } + return + } - if allFQNsExist { - s.Len(resourceDecision.DataRuleResults, len(definitions)) + s.Require().NoError(err) + s.NotNil(resourceDecision) + s.Equal(tc.expectAccessible, resourceDecision.Entitled) + + // Check results array has the correct length based on grouping by definition + // If ANY FQN is missing, DataRuleResults should be empty (resource is denied without evaluation) + definitions := make(map[string]bool) + allFQNsExist := true + for _, fqn := range tc.resourceAttrs.GetFqns() { + if attrAndValue, ok := s.accessibleAttrValues[fqn]; ok { + definitions[attrAndValue.GetAttribute().GetFqn()] = true } else { - // Any missing FQN means DENY without evaluation - s.Len(resourceDecision.DataRuleResults, 0) + allFQNsExist = false } } + + if allFQNsExist { + s.Len(resourceDecision.DataRuleResults, len(definitions)) + } else { + // Any missing FQN means DENY without evaluation + s.Empty(resourceDecision.DataRuleResults) + } }) } } diff --git a/service/internal/access/v2/pdp_test.go b/service/internal/access/v2/pdp_test.go index 0b32f70bec..bdbf33ae3b 100644 --- a/service/internal/access/v2/pdp_test.go +++ b/service/internal/access/v2/pdp_test.go @@ -3148,50 +3148,6 @@ func (s *PDPTestSuite) Test_GetEntitlementsRegisteredResource() { }) } -// Helper functions for all tests - -// assertDecisionResult is a helper function to assert that a decision result for a given FQN matches the expected pass/fail state -func (s *PDPTestSuite) assertDecisionResult(decision *Decision, fqn string, shouldPass bool) { - resourceDecision := findResourceDecision(decision, fqn) - s.Require().NotNil(resourceDecision, "No result found for FQN: "+fqn) - s.Equal(shouldPass, resourceDecision.Entitled, "Unexpected result for FQN %s. Expected (%t), got (%t)", fqn, shouldPass, resourceDecision.Entitled) -} - -// assertAllDecisionResults tests all FQNs in a map of FQN to expected pass/fail state -func (s *PDPTestSuite) assertAllDecisionResults(decision *Decision, expectedResults map[string]bool) { - for fqn, shouldPass := range expectedResults { - s.assertDecisionResult(decision, fqn, shouldPass) - } - // Verify we didn't miss any results - s.Len(decision.Results, len(expectedResults), "Number of results doesn't match expected count") -} - -// createEntityWithProps creates an entity representation with the specified properties -func (s *PDPTestSuite) createEntityWithProps(entityID string, props map[string]interface{}) *entityresolutionV2.EntityRepresentation { - propsStruct := &structpb.Struct{ - Fields: make(map[string]*structpb.Value), - } - - for k, v := range props { - value, err := structpb.NewValue(v) - if err != nil { - panic(fmt.Sprintf("Failed to convert value %v to structpb.Value: %v", v, err)) - } - propsStruct.Fields[k] = value - } - - return &entityresolutionV2.EntityRepresentation{ - OriginalId: entityID, - AdditionalProps: []*structpb.Struct{ - { - Fields: map[string]*structpb.Value{ - "properties": structpb.NewStructValue(propsStruct), - }, - }, - }, - } -} - // createAttributeValueResource creates a resource with attribute values func createAttributeValueResource(ephemeralID string, attributeValueFQNs ...string) *authz.Resource { return &authz.Resource{ @@ -3534,7 +3490,7 @@ func (s *PDPTestSuite) Test_GetDecisionRegisteredResource_NonExistentFQN() { case "invalid-reg-res": s.False(result.Entitled) default: - s.Fail("Unexpected resource ID: %s", result.ResourceID) + s.Failf("Unexpected resource ID: %s", result.ResourceID) } } }) @@ -3593,3 +3549,47 @@ func (s *PDPTestSuite) Test_GetDecision_NoPolicies() { s.Empty(entitlements) }) } + +// Helper functions for all tests + +// assertDecisionResult is a helper function to assert that a decision result for a given FQN matches the expected pass/fail state +func (s *PDPTestSuite) assertDecisionResult(decision *Decision, fqn string, shouldPass bool) { + resourceDecision := findResourceDecision(decision, fqn) + s.Require().NotNil(resourceDecision, "No result found for FQN: "+fqn) + s.Equal(shouldPass, resourceDecision.Entitled, "Unexpected result for FQN %s. Expected (%t), got (%t)", fqn, shouldPass, resourceDecision.Entitled) +} + +// assertAllDecisionResults tests all FQNs in a map of FQN to expected pass/fail state +func (s *PDPTestSuite) assertAllDecisionResults(decision *Decision, expectedResults map[string]bool) { + for fqn, shouldPass := range expectedResults { + s.assertDecisionResult(decision, fqn, shouldPass) + } + // Verify we didn't miss any results + s.Len(decision.Results, len(expectedResults), "Number of results doesn't match expected count") +} + +// createEntityWithProps creates an entity representation with the specified properties +func (s *PDPTestSuite) createEntityWithProps(entityID string, props map[string]interface{}) *entityresolutionV2.EntityRepresentation { + propsStruct := &structpb.Struct{ + Fields: make(map[string]*structpb.Value), + } + + for k, v := range props { + value, err := structpb.NewValue(v) + if err != nil { + panic(fmt.Sprintf("Failed to convert value %v to structpb.Value: %v", v, err)) + } + propsStruct.Fields[k] = value + } + + return &entityresolutionV2.EntityRepresentation{ + OriginalId: entityID, + AdditionalProps: []*structpb.Struct{ + { + Fields: map[string]*structpb.Value{ + "properties": structpb.NewStructValue(propsStruct), + }, + }, + }, + } +} From 39ae4ebda00c0133d07ac3e7a81e83fe285b5aa6 Mon Sep 17 00:00:00 2001 From: jakedoublev Date: Mon, 10 Nov 2025 15:57:01 -0800 Subject: [PATCH 6/6] reduce quantity of tests / prefer table-driven --- service/internal/access/v2/evaluate_test.go | 220 ++++++++------------ 1 file changed, 84 insertions(+), 136 deletions(-) diff --git a/service/internal/access/v2/evaluate_test.go b/service/internal/access/v2/evaluate_test.go index 6eec7a2703..9e096afe1a 100644 --- a/service/internal/access/v2/evaluate_test.go +++ b/service/internal/access/v2/evaluate_test.go @@ -751,11 +751,11 @@ func (s *EvaluateTestSuite) TestEvaluateResourceAttributeValues() { expectError: false, }, { - name: "unknown attribute value FQN", + name: "partial FQNs not found - should DENY", resourceAttrs: &authz.Resource_AttributeValues{ Fqns: []string{ levelMidFQN, - "https://namespace.com/attr/department/value/unknown", // This FQN doesn't exist in accessibleAttributeValues + "https://namespace.com/attr/department/value/unknown", }, }, entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{ @@ -765,6 +765,19 @@ func (s *EvaluateTestSuite) TestEvaluateResourceAttributeValues() { expectAccessible: false, expectError: false, }, + { + name: "all FQNs not found - should DENY", + resourceAttrs: &authz.Resource_AttributeValues{ + Fqns: []string{ + createAttrValueFQN(baseNamespace, "significance", "critical"), + createAttrValueFQN(baseNamespace, "significance", "major"), + }, + }, + entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{}, + // Should NOT error - but should DENY resource (no FQNs exist) + expectAccessible: false, + expectError: false, + }, } for _, tc := range tests { @@ -899,7 +912,7 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() { expectPass: false, }, { - name: "nonexistent registered resource value", + name: "nonexistent registered resource value - should DENY", resource: &authz.Resource{ Resource: &authz.Resource_RegisteredResourceValueFqn{ RegisteredResourceValueFqn: nonExistentRegResValueFQN, @@ -910,6 +923,73 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() { expectError: false, expectPass: false, }, + { + name: "attribute value FQNs not found, namespace & definition exist - should DENY", + resource: &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{ + createAttrValueFQN(baseNamespace, "department", "doesnotexist"), + }, + }, + }, + EphemeralId: "test-attr-missing-fqns", + }, + entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{}, + expectError: false, + expectPass: false, + }, + { + name: "attribute value FQNs not found, namespace exists - should DENY", + resource: &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{ + createAttrValueFQN(baseNamespace, "unknown", "doesnotexist"), + }, + }, + }, + EphemeralId: "test-attr-missing-fqns", + }, + entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{}, + expectError: false, + expectPass: false, + }, + { + name: "attribute value FQNs not found, namespace does not exist - should DENY", + resource: &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{ + "https://doesnot.exist/attr/severity/value/high", + }, + }, + }, + EphemeralId: "test-attr-missing-fqns", + }, + entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{}, + expectError: false, + expectPass: false, + }, + { + name: "attribute value FQNs partially exist - should DENY", + resource: &authz.Resource{ + Resource: &authz.Resource_AttributeValues_{ + AttributeValues: &authz.Resource_AttributeValues{ + Fqns: []string{ + levelMidFQN, + "https://doesnot.exist/attr/severity/value/high", + }, + }, + }, + EphemeralId: "test-attr-values-partially-exist", + }, + entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{ + levelMidFQN: []*policy.Action{actionRead}, + }, + expectError: false, + expectPass: false, + }, { name: "invalid nil resource", resource: nil, @@ -956,139 +1036,7 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() { } } -func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_AllFQNsNotFound() { - nonExistentFQN1 := createAttrValueFQN(baseNamespace, "significance", "critical") - nonExistentFQN2 := createAttrValueFQN(baseNamespace, "significance", "major") - - resourceAttributeValues := &authz.Resource_AttributeValues{ - Fqns: []string{ - nonExistentFQN1, - nonExistentFQN2, - }, - } - - emptyAccessibleAttributes := make(map[string]*attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue) - - entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{} - - decision, err := evaluateResourceAttributeValues( - s.T().Context(), - s.logger, - resourceAttributeValues, - "resource-1", - "", - s.action, - entitlements, - emptyAccessibleAttributes, - ) - - s.Require().NoError(err) - s.Require().NotNil(decision) - s.False(decision.Entitled) - s.Equal("resource-1", decision.ResourceID) -} - -func (s *EvaluateTestSuite) Test_evaluateResourceAttributeValues_PartialFQNsNotFound() { - nonExistentFQN := createAttrValueFQN(baseNamespace, "significance", "high") - - resourceAttributeValues := &authz.Resource_AttributeValues{ - Fqns: []string{ - levelMidFQN, // Exists - nonExistentFQN, // Eoesn't exist - levelHighestFQN, // Exists - }, - } - - // Entitled to the existing FQNs - entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{ - levelMidFQN: []*policy.Action{actionRead}, - levelHighestFQN: []*policy.Action{actionRead}, - } - - decision, err := evaluateResourceAttributeValues( - s.T().Context(), - s.logger, - resourceAttributeValues, - "resource-2", - "", - s.action, - entitlements, - s.accessibleAttrValues, - ) - - // No error, but deny decision - s.Require().NoError(err) - s.Require().NotNil(decision) - s.False(decision.Entitled) - s.Equal("resource-2", decision.ResourceID) -} - -func (s *EvaluateTestSuite) Test_getResourceDecision_AttributeValueFQNsNotFound() { - nonExistentFQN1 := createAttrValueFQN(baseNamespace, "space", "cosmic") - nonExistentFQN2 := createAttrValueFQN(baseNamespace, "space", "planetary") - - resource := &authz.Resource{ - Resource: &authz.Resource_AttributeValues_{ - AttributeValues: &authz.Resource_AttributeValues{ - Fqns: []string{nonExistentFQN1, nonExistentFQN2}, - }, - }, - EphemeralId: "resource-with-missing-fqns", - } - - emptyAccessibleAttributes := make(map[string]*attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue) - entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{} - - decision, err := getResourceDecision( - s.T().Context(), - s.logger, - emptyAccessibleAttributes, - s.accessibleRegisteredResourceValues, - entitlements, - s.action, - resource, - ) - - // No error, but deny decision - s.Require().NoError(err) - s.Require().NotNil(decision) - s.False(decision.Entitled) - s.Equal("resource-with-missing-fqns", decision.ResourceID) -} - -func (s *EvaluateTestSuite) Test_getResourceDecision_RegisteredResourceValueFQNNotFound() { - nonExistentRegResFQN := createRegisteredResourceValueFQN("special-system", "classified") - - resource := &authz.Resource{ - Resource: &authz.Resource_RegisteredResourceValueFqn{ - RegisteredResourceValueFqn: nonExistentRegResFQN, - }, - EphemeralId: "resource-with-missing-reg-res", - } - - entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{ - levelHighestFQN: []*policy.Action{actionRead}, - } - - decision, err := getResourceDecision( - s.T().Context(), - s.logger, - s.accessibleAttrValues, - s.accessibleRegisteredResourceValues, - entitlements, - s.action, - resource, - ) - - // No error, but deny decision - s.Require().NoError(err) - s.Require().NotNil(decision) - s.False(decision.Entitled) - s.Equal("resource-with-missing-reg-res", decision.ResourceID) - s.Equal(nonExistentRegResFQN, decision.ResourceName) -} - -func (s *EvaluateTestSuite) Test_getResourceDecision_MixedResources_IndividualDenials() { +func (s *EvaluateTestSuite) Test_getResourceDecision_MultiResources_GranularDenials() { nonExistentFQN := createAttrValueFQN(baseNamespace, "space", "cosmic") // Resource 1: Valid FQN, entity is entitled