Skip to content

Commit 802db02

Browse files
authored
fix(authz): deny resources granularly when attribute value FQNs not found (#2896)
### Proposed Changes * If attribute value FQNs not found, deny access to the resource containing the FQN instead of returning error to request ### Checklist - [X] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
1 parent 003a0a0 commit 802db02

File tree

3 files changed

+531
-65
lines changed

3 files changed

+531
-65
lines changed

service/internal/access/v2/evaluate.go

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,6 @@ func getResourceDecision(
5555
slog.Any("resource", resource.GetResource()),
5656
)
5757

58-
if len(accessibleAttributeValues) == 0 {
59-
l.WarnContext(ctx, "resource is not able to be entitled", slog.Any("resource", resource.GetResource()))
60-
return failure, nil
61-
}
62-
6358
switch resource.GetResource().(type) {
6459
case *authz.Resource_RegisteredResourceValueFqn:
6560
registeredResourceValueFQN = strings.ToLower(resource.GetRegisteredResourceValueFqn())
@@ -110,6 +105,12 @@ func getResourceDecision(
110105
return nil, fmt.Errorf("unsupported resource type: %w", ErrInvalidResource)
111106
}
112107

108+
// Cannot entitle any resource
109+
if len(accessibleAttributeValues) == 0 {
110+
l.WarnContext(ctx, "resource is not able to be entitled", slog.Any("resource", resource.GetResource()))
111+
return failure, nil
112+
}
113+
113114
return evaluateResourceAttributeValues(ctx, l, resourceAttributeValues, resourceID, registeredResourceValueFQN, action, entitlements, accessibleAttributeValues)
114115
}
115116

@@ -128,17 +129,34 @@ func evaluateResourceAttributeValues(
128129
// Group value FQNs by parent definition
129130
definitionFqnToValueFqns := make(map[string][]string)
130131
definitionsLookup := make(map[string]*policy.Attribute)
132+
notFoundFQNs := make([]string, 0)
131133

132134
for _, valueFQN := range resourceAttributeValues.GetFqns() {
133135
attributeAndValue, ok := accessibleAttributeValues[valueFQN]
134136
if !ok {
135-
return nil, fmt.Errorf("%w: %s", ErrFQNNotFound, valueFQN)
137+
notFoundFQNs = append(notFoundFQNs, valueFQN)
138+
continue
136139
}
137140
definition := attributeAndValue.GetAttribute()
138141
definitionFqnToValueFqns[definition.GetFqn()] = append(definitionFqnToValueFqns[definition.GetFqn()], valueFQN)
139142
definitionsLookup[definition.GetFqn()] = definition
140143
}
141144

145+
// If ANY FQNs are missing, DENY the resource
146+
if len(notFoundFQNs) > 0 {
147+
l.WarnContext(ctx, "attribute value FQN(s) not found - denying access to resource",
148+
slog.Any("not_found_fqns", notFoundFQNs),
149+
slog.String("resource_id", resourceID))
150+
result := &ResourceDecision{
151+
Entitled: false,
152+
ResourceID: resourceID,
153+
}
154+
if resourceName != "" {
155+
result.ResourceName = resourceName
156+
}
157+
return result, nil
158+
}
159+
142160
// Evaluate each definition by rule, resource attributes, action, and entitlements
143161
passed := true
144162
dataRuleResults := make([]DataRuleResult, 0)

service/internal/access/v2/evaluate_test.go

Lines changed: 174 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -751,18 +751,32 @@ func (s *EvaluateTestSuite) TestEvaluateResourceAttributeValues() {
751751
expectError: false,
752752
},
753753
{
754-
name: "unknown attribute value FQN",
754+
name: "partial FQNs not found - should DENY",
755755
resourceAttrs: &authz.Resource_AttributeValues{
756756
Fqns: []string{
757757
levelMidFQN,
758-
"https://namespace.com/attr/department/value/unknown", // This FQN doesn't exist in accessibleAttributeValues
758+
"https://namespace.com/attr/department/value/unknown",
759759
},
760760
},
761761
entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{
762762
levelMidFQN: []*policy.Action{actionRead},
763763
},
764+
// Should NOT error - but should DENY resource (ANY missing FQN = DENY)
764765
expectAccessible: false,
765-
expectError: true,
766+
expectError: false,
767+
},
768+
{
769+
name: "all FQNs not found - should DENY",
770+
resourceAttrs: &authz.Resource_AttributeValues{
771+
Fqns: []string{
772+
createAttrValueFQN(baseNamespace, "significance", "critical"),
773+
createAttrValueFQN(baseNamespace, "significance", "major"),
774+
},
775+
},
776+
entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{},
777+
// Should NOT error - but should DENY resource (no FQNs exist)
778+
expectAccessible: false,
779+
expectError: false,
766780
},
767781
}
768782

@@ -782,19 +796,30 @@ func (s *EvaluateTestSuite) TestEvaluateResourceAttributeValues() {
782796

783797
if tc.expectError {
784798
s.Require().Error(err)
785-
} else {
786-
s.Require().NoError(err)
787-
s.NotNil(resourceDecision)
788-
s.Equal(tc.expectAccessible, resourceDecision.Entitled)
789-
790-
// Check results array has the correct length based on grouping by definition
791-
definitions := make(map[string]bool)
792-
for _, fqn := range tc.resourceAttrs.GetFqns() {
793-
if attrAndValue, ok := s.accessibleAttrValues[fqn]; ok {
794-
definitions[attrAndValue.GetAttribute().GetFqn()] = true
795-
}
799+
return
800+
}
801+
802+
s.Require().NoError(err)
803+
s.NotNil(resourceDecision)
804+
s.Equal(tc.expectAccessible, resourceDecision.Entitled)
805+
806+
// Check results array has the correct length based on grouping by definition
807+
// If ANY FQN is missing, DataRuleResults should be empty (resource is denied without evaluation)
808+
definitions := make(map[string]bool)
809+
allFQNsExist := true
810+
for _, fqn := range tc.resourceAttrs.GetFqns() {
811+
if attrAndValue, ok := s.accessibleAttrValues[fqn]; ok {
812+
definitions[attrAndValue.GetAttribute().GetFqn()] = true
813+
} else {
814+
allFQNsExist = false
796815
}
816+
}
817+
818+
if allFQNsExist {
797819
s.Len(resourceDecision.DataRuleResults, len(definitions))
820+
} else {
821+
// Any missing FQN means DENY without evaluation
822+
s.Empty(resourceDecision.DataRuleResults)
798823
}
799824
})
800825
}
@@ -887,7 +912,7 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() {
887912
expectPass: false,
888913
},
889914
{
890-
name: "nonexistent registered resource value",
915+
name: "nonexistent registered resource value - should DENY",
891916
resource: &authz.Resource{
892917
Resource: &authz.Resource_RegisteredResourceValueFqn{
893918
RegisteredResourceValueFqn: nonExistentRegResValueFQN,
@@ -898,6 +923,73 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() {
898923
expectError: false,
899924
expectPass: false,
900925
},
926+
{
927+
name: "attribute value FQNs not found, namespace & definition exist - should DENY",
928+
resource: &authz.Resource{
929+
Resource: &authz.Resource_AttributeValues_{
930+
AttributeValues: &authz.Resource_AttributeValues{
931+
Fqns: []string{
932+
createAttrValueFQN(baseNamespace, "department", "doesnotexist"),
933+
},
934+
},
935+
},
936+
EphemeralId: "test-attr-missing-fqns",
937+
},
938+
entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{},
939+
expectError: false,
940+
expectPass: false,
941+
},
942+
{
943+
name: "attribute value FQNs not found, namespace exists - should DENY",
944+
resource: &authz.Resource{
945+
Resource: &authz.Resource_AttributeValues_{
946+
AttributeValues: &authz.Resource_AttributeValues{
947+
Fqns: []string{
948+
createAttrValueFQN(baseNamespace, "unknown", "doesnotexist"),
949+
},
950+
},
951+
},
952+
EphemeralId: "test-attr-missing-fqns",
953+
},
954+
entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{},
955+
expectError: false,
956+
expectPass: false,
957+
},
958+
{
959+
name: "attribute value FQNs not found, namespace does not exist - should DENY",
960+
resource: &authz.Resource{
961+
Resource: &authz.Resource_AttributeValues_{
962+
AttributeValues: &authz.Resource_AttributeValues{
963+
Fqns: []string{
964+
"https://doesnot.exist/attr/severity/value/high",
965+
},
966+
},
967+
},
968+
EphemeralId: "test-attr-missing-fqns",
969+
},
970+
entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{},
971+
expectError: false,
972+
expectPass: false,
973+
},
974+
{
975+
name: "attribute value FQNs partially exist - should DENY",
976+
resource: &authz.Resource{
977+
Resource: &authz.Resource_AttributeValues_{
978+
AttributeValues: &authz.Resource_AttributeValues{
979+
Fqns: []string{
980+
levelMidFQN,
981+
"https://doesnot.exist/attr/severity/value/high",
982+
},
983+
},
984+
},
985+
EphemeralId: "test-attr-values-partially-exist",
986+
},
987+
entitlements: subjectmappingbuiltin.AttributeValueFQNsToActions{
988+
levelMidFQN: []*policy.Action{actionRead},
989+
},
990+
expectError: false,
991+
expectPass: false,
992+
},
901993
{
902994
name: "invalid nil resource",
903995
resource: nil,
@@ -943,3 +1035,70 @@ func (s *EvaluateTestSuite) TestGetResourceDecision() {
9431035
})
9441036
}
9451037
}
1038+
1039+
func (s *EvaluateTestSuite) Test_getResourceDecision_MultiResources_GranularDenials() {
1040+
nonExistentFQN := createAttrValueFQN(baseNamespace, "space", "cosmic")
1041+
1042+
// Resource 1: Valid FQN, entity is entitled
1043+
resource1 := &authz.Resource{
1044+
Resource: &authz.Resource_AttributeValues_{
1045+
AttributeValues: &authz.Resource_AttributeValues{
1046+
Fqns: []string{levelHighestFQN},
1047+
},
1048+
},
1049+
EphemeralId: "valid-resource-1",
1050+
}
1051+
1052+
// Resource 2: Non-existent FQN
1053+
resource2 := &authz.Resource{
1054+
Resource: &authz.Resource_AttributeValues_{
1055+
AttributeValues: &authz.Resource_AttributeValues{
1056+
Fqns: []string{nonExistentFQN},
1057+
},
1058+
},
1059+
EphemeralId: "invalid-resource-2",
1060+
}
1061+
1062+
// Resource 3: Valid FQN, entity is entitled
1063+
resource3 := &authz.Resource{
1064+
Resource: &authz.Resource_AttributeValues_{
1065+
AttributeValues: &authz.Resource_AttributeValues{
1066+
Fqns: []string{levelMidFQN},
1067+
},
1068+
},
1069+
EphemeralId: "valid-resource-3",
1070+
}
1071+
1072+
entitlements := subjectmappingbuiltin.AttributeValueFQNsToActions{
1073+
levelHighestFQN: []*policy.Action{actionRead},
1074+
levelMidFQN: []*policy.Action{actionRead},
1075+
}
1076+
1077+
testCases := []struct {
1078+
name string
1079+
resource *authz.Resource
1080+
expectedEntitled bool
1081+
}{
1082+
{"valid resource 1", resource1, true},
1083+
{"invalid resource 2 (missing FQN)", resource2, false},
1084+
{"valid resource 3", resource3, true},
1085+
}
1086+
1087+
for _, tc := range testCases {
1088+
s.Run(tc.name, func() {
1089+
decision, err := getResourceDecision(
1090+
s.T().Context(),
1091+
s.logger,
1092+
s.accessibleAttrValues,
1093+
s.accessibleRegisteredResourceValues,
1094+
entitlements,
1095+
s.action,
1096+
tc.resource,
1097+
)
1098+
1099+
s.Require().NoError(err, "Should not error for resource: %s", tc.name)
1100+
s.Require().NotNil(decision)
1101+
s.Equal(tc.expectedEntitled, decision.Entitled, "Entitlement mismatch for: %s", tc.name)
1102+
})
1103+
}
1104+
}

0 commit comments

Comments
 (0)