From 97047cfed9e472177aa026c93aac3a2feb9fd0c5 Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Thu, 13 Aug 2026 14:01:19 -0400 Subject: [PATCH 1/2] refactor(authz): use targeted entitlement lookups Signed-off-by: Krish Suchak --- service/authorization/authorization.go | 202 +++++++++++---- .../authorization_entitleable_test.go | 236 ++++++++++++++++++ service/authorization/authorization_test.go | 5 + .../authorization_test_structures.go | 98 +++++++- 4 files changed, 486 insertions(+), 55 deletions(-) create mode 100644 service/authorization/authorization_entitleable_test.go diff --git a/service/authorization/authorization.go b/service/authorization/authorization.go index 0ff99d24c4..6e283cb2db 100644 --- a/service/authorization/authorization.go +++ b/service/authorization/authorization.go @@ -15,6 +15,7 @@ import ( "github.com/go-viper/mapstructure/v2" "github.com/open-policy-agent/opa/v1/ast" "github.com/open-policy-agent/opa/v1/rego" + "github.com/opentdf/platform/lib/flattening" "github.com/opentdf/platform/protocol/go/authorization" "github.com/opentdf/platform/protocol/go/authorization/authorizationconnect" "github.com/opentdf/platform/protocol/go/common" @@ -40,6 +41,8 @@ import ( var ErrEmptyStringAttribute = errors.New("resource attributes must have at least one attribute value fqn") +const maxEntitleableFQNsPerRequest = 250 + type AuthorizationService struct { //nolint:revive // AuthorizationService is a valid name for this struct sdk *otdf.SDK config *Config @@ -284,70 +287,99 @@ func makeScopeMap(scope *authorization.ResourceAttribute) map[string]bool { return scopeMap } -func (as *AuthorizationService) GetEntitlements(ctx context.Context, req *connect.Request[authorization.GetEntitlementsRequest]) (*connect.Response[authorization.GetEntitlementsResponse], error) { - as.logger.DebugContext(ctx, "getting entitlements") - - ctx, span := as.Start(ctx, "GetEntitlements") - defer span.End() - +func retrieveFullAttributeMappings(ctx context.Context, scope *authorization.ResourceAttribute, sdk *otdf.SDK) (map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue, error) { var nextOffset int32 attrsList := make([]*policy.Attribute, 0) subjectMappingsList := make([]*policy.SubjectMapping, 0) - // If quantity of attributes exceeds maximum list pagination, all are needed to determine entitlements for { - listed, err := as.sdk.Attributes.ListAttributes(ctx, &attr.ListAttributesRequest{ + listed, err := sdk.Attributes.ListAttributes(ctx, &attr.ListAttributesRequest{ State: common.ActiveStateEnum_ACTIVE_STATE_ENUM_ACTIVE, Pagination: &policy.PageRequest{ Offset: nextOffset, }, }) if err != nil { - as.logger.ErrorContext(ctx, "failed to list attributes", slog.String("error", err.Error())) - return nil, connect.NewError(connect.CodeInternal, errors.New("failed to list attributes")) + return nil, fmt.Errorf("failed to list attributes: %w", err) } nextOffset = listed.GetPagination().GetNextOffset() attrsList = append(attrsList, listed.GetAttributes()...) - - // offset becomes zero when list is exhausted if nextOffset <= 0 { break } } - // If quantity of subject mappings exceeds maximum list pagination, all are needed to determine entitlements nextOffset = 0 for { - listed, err := as.sdk.SubjectMapping.ListSubjectMappings(ctx, &subjectmapping.ListSubjectMappingsRequest{ + listed, err := sdk.SubjectMapping.ListSubjectMappings(ctx, &subjectmapping.ListSubjectMappingsRequest{ Pagination: &policy.PageRequest{ Offset: nextOffset, }, }) if err != nil { - as.logger.ErrorContext(ctx, "failed to list subject mappings", slog.String("error", err.Error())) - return nil, connect.NewError(connect.CodeInternal, errors.New("failed to list subject mappings")) + return nil, fmt.Errorf("failed to list subject mappings: %w", err) } nextOffset = listed.GetPagination().GetNextOffset() subjectMappingsList = append(subjectMappingsList, listed.GetSubjectMappings()...) - - // offset becomes zero when list is exhausted if nextOffset <= 0 { break } } - // create a lookup map of attribute value FQNs (based on request scope) - scopeMap := makeScopeMap(req.Msg.GetScope()) - // create a lookup map of subject mappings by attribute value ID + + scopeMap := makeScopeMap(scope) subMapsByVal := makeSubMapsByValLookup(subjectMappingsList) - // create a lookup map of attribute values by FQN (for rego query) - fqnAttrVals := makeValsByFqnsLookup(attrsList, subMapsByVal, scopeMap) - avf := &attr.GetAttributeValuesByFqnsResponse{ - FqnAttributeValues: fqnAttrVals, + return makeValsByFqnsLookup(attrsList, subMapsByVal, scopeMap), nil +} + +func subjectPropertiesFromEntityRepresentations(ersResp *entityresolution.ResolveEntitiesResponse) ([]*policy.SubjectProperty, error) { + properties := make([]*policy.SubjectProperty, 0) + seen := make(map[string]struct{}) + for _, entityRepresentation := range ersResp.GetEntityRepresentations() { + for _, additionalProps := range entityRepresentation.GetAdditionalProps() { + flattened, err := flattening.Flatten(additionalProps.AsMap()) + if err != nil { + return nil, fmt.Errorf("failed to flatten entity representation: %w", err) + } + for _, item := range flattened.Items { + if _, ok := seen[item.Key]; ok { + continue + } + seen[item.Key] = struct{}{} + properties = append(properties, &policy.SubjectProperty{ExternalSelectorValue: item.Key}) + } + } } - subjectMappings := avf.GetFqnAttributeValues() - as.logger.DebugContext(ctx, "retrieved subject mappings", slog.Int("count", len(subjectMappings))) + return properties, nil +} + +func matchedAttributeFQNs(mappings []*policy.SubjectMapping, scope *authorization.ResourceAttribute) []string { + scopeMap := makeScopeMap(scope) + seen := make(map[string]struct{}) + fqns := make([]string, 0, len(mappings)) + for _, mapping := range mappings { + fqn := strings.ToLower(mapping.GetAttributeValue().GetFqn()) + if fqn == "" { + continue + } + if scopeMap != nil && !scopeMap[fqn] { + continue + } + if _, ok := seen[fqn]; ok { + continue + } + seen[fqn] = struct{}{} + fqns = append(fqns, fqn) + } + return fqns +} + +func (as *AuthorizationService) GetEntitlements(ctx context.Context, req *connect.Request[authorization.GetEntitlementsRequest]) (*connect.Response[authorization.GetEntitlementsResponse], error) { + as.logger.DebugContext(ctx, "getting entitlements") + + ctx, span := as.Start(ctx, "GetEntitlements") + defer span.End() // TODO: this could probably be moved to proto validation https://github.com/opentdf/platform/issues/1057 if req.Msg.Entities == nil { @@ -365,6 +397,19 @@ func (as *AuthorizationService) GetEntitlements(ctx context.Context, req *connec return nil, err } + var subjectMappings map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue + if as.usesCustomRego() { + subjectMappings, err = retrieveFullAttributeMappings(ctx, req.Msg.GetScope(), as.sdk) + } else { + subjectMappings, err = as.retrieveMatchedAttributeMappings(ctx, ersResp, req.Msg.GetScope()) + } + if err != nil { + as.logger.ErrorContext(ctx, "failed to retrieve subject mappings", slog.String("error", err.Error())) + return nil, connect.NewError(connect.CodeInternal, err) + } + as.logger.DebugContext(ctx, "retrieved subject mappings", slog.Int("count", len(subjectMappings))) + avf := &attr.GetAttributeValuesByFqnsResponse{FqnAttributeValues: subjectMappings} + // call rego on all entities in, err := entitlements.OpaInput(subjectMappings, ersResp) if err != nil { @@ -451,6 +496,30 @@ func (as *AuthorizationService) GetEntitlements(ctx context.Context, req *connec return resp, nil } +func (as *AuthorizationService) usesCustomRego() bool { + return as.config != nil && as.config.Rego.Path != "" +} + +func (as *AuthorizationService) retrieveMatchedAttributeMappings(ctx context.Context, ersResp *entityresolution.ResolveEntitiesResponse, scope *authorization.ResourceAttribute) (map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue, error) { + properties, err := subjectPropertiesFromEntityRepresentations(ersResp) + if err != nil { + return nil, err + } + if len(properties) == 0 { + return make(map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue), nil + } + + matched, err := as.sdk.SubjectMapping.MatchSubjectMappings(ctx, &subjectmapping.MatchSubjectMappingsRequest{ + SubjectProperties: properties, + }) + if err != nil { + return nil, fmt.Errorf("failed to match subject mappings: %w", err) + } + + fqns := matchedAttributeFQNs(matched.GetSubjectMappings(), scope) + return retrieveAttributeDefinitions(ctx, fqns, as.sdk) +} + func getAttributesFromRas(ras []*authorization.ResourceAttribute) ([]string, error) { var attrFqns []string repeats := make(map[string]bool) @@ -727,31 +796,68 @@ func (as *AuthorizationService) getDecisions(ctx context.Context, dr *authorizat } func retrieveAttributeDefinitions(ctx context.Context, attrFqns []string, sdk *otdf.SDK) (map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue, error) { - if len(attrFqns) == 0 { - return make(map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue), nil - } - - resp, err := sdk.Attributes.GetAttributeValuesByFqns(ctx, &attr.GetAttributeValuesByFqnsRequest{ - Fqns: attrFqns, - }) - if err != nil { - return nil, err - } - // If `allow_traversal` is true for an attribute definition - // it will return an attribute definition for a missing - // value. Where before you would receive a 404 error. - // Since v1 does not expect direct entitlements - // and expects a value, we fail if there is no - // value. - fqnAttrVals := resp.GetFqnAttributeValues() + normalizedFQNs := make([]string, 0, len(attrFqns)) + seen := make(map[string]struct{}, len(attrFqns)) for _, fqn := range attrFqns { normalized := strings.ToLower(fqn) - attributeAndValue, ok := fqnAttrVals[normalized] - if !ok || attributeAndValue == nil || attributeAndValue.GetValue() == nil { - return nil, status.Error(codes.NotFound, db.ErrTextNotFound) + if _, ok := seen[normalized]; ok { + continue + } + seen[normalized] = struct{}{} + normalizedFQNs = append(normalizedFQNs, normalized) + } + + result := make(map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue, len(normalizedFQNs)) + for start := 0; start < len(normalizedFQNs); start += maxEntitleableFQNsPerRequest { + end := min(start+maxEntitleableFQNsPerRequest, len(normalizedFQNs)) + batch := normalizedFQNs[start:end] + resp, err := sdk.Attributes.GetEntitleableAttributesByFqns(ctx, &attr.GetEntitleableAttributesByFqnsRequest{ + Fqns: batch, + }) + if err != nil { + return nil, err + } + + for _, fqn := range batch { + entitleable, ok := resp.GetFqnEntitleableAttributes()[fqn] + if !ok || entitleable.GetValue().GetValueId() == "" { + return nil, status.Error(codes.NotFound, db.ErrTextNotFound) + } + + definitionFQN := entitleable.GetDefinitionFqn() + definition, ok := resp.GetDefinitions()[definitionFQN] + if definitionFQN == "" || !ok || definition == nil { + return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("entitleable attribute %q references missing definition %q", fqn, definitionFQN)) + } + + attribute := &policy.Attribute{ + Fqn: definitionFQN, + Namespace: definition.GetNamespace(), + Rule: definition.GetRule(), + } + if definition.GetRule() == policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_HIERARCHY { + attribute.Values = make([]*policy.Value, 0, len(definition.GetValues())) + for _, value := range definition.GetValues() { + attribute.Values = append(attribute.Values, &policy.Value{ + Id: value.GetValueId(), + Fqn: value.GetFqn(), + SubjectMappings: value.GetSubjectMappings(), + }) + } + } + + value := entitleable.GetValue() + result[fqn] = &attr.GetAttributeValuesByFqnsResponse_AttributeAndValue{ + Attribute: attribute, + Value: &policy.Value{ + Id: value.GetValueId(), + Fqn: value.GetFqn(), + SubjectMappings: value.GetSubjectMappings(), + }, + } } } - return fqnAttrVals, nil + return result, nil } func getComprehensiveHierarchy(attributesMap map[string]*policy.Attribute, avf *attr.GetAttributeValuesByFqnsResponse, entitlement string, as *AuthorizationService, entitlements []string) []string { diff --git a/service/authorization/authorization_entitleable_test.go b/service/authorization/authorization_entitleable_test.go new file mode 100644 index 0000000000..7fd2a345ce --- /dev/null +++ b/service/authorization/authorization_entitleable_test.go @@ -0,0 +1,236 @@ +package authorization + +import ( + "errors" + "fmt" + "strings" + "testing" + + "connectrpc.com/connect" + "github.com/open-policy-agent/opa/v1/ast" + "github.com/open-policy-agent/opa/v1/rego" + "github.com/opentdf/platform/protocol/go/authorization" + "github.com/opentdf/platform/protocol/go/entityresolution" + "github.com/opentdf/platform/protocol/go/policy" + attr "github.com/opentdf/platform/protocol/go/policy/attributes" + otdf "github.com/opentdf/platform/sdk" + "github.com/opentdf/platform/service/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/trace/noop" + "google.golang.org/protobuf/types/known/structpb" +) + +func resetTargetedLookupMocks(t *testing.T) { + t.Helper() + getEntitleableAttributesResponse = nil + errGetAttributesByValueFqns = nil + errMatchSubjectMappings = nil + getEntitleableAttributesRequests = nil + matchSubjectMappingsRequests = nil + listAttributesCallCount = 0 + listSubjectMappingsCallCount = 0 + getAttributeValuesCallCount = 0 + t.Cleanup(func() { + getAttributesByValueFqnsResponse = attr.GetAttributeValuesByFqnsResponse{} + getEntitleableAttributesResponse = nil + errGetAttributesByValueFqns = nil + errMatchSubjectMappings = nil + getEntitleableAttributesRequests = nil + matchSubjectMappingsRequests = nil + resolveEntitiesResp = entityresolution.ResolveEntitiesResponse{} + }) +} + +func TestRetrieveAttributeDefinitionsMapsEntitleableResponse(t *testing.T) { + resetTargetedLookupMocks(t) + + definitionFQN := "https://example.com/attr/classification" + valueFQN := definitionFQN + "/value/confidential" + lowerValueFQN := definitionFQN + "/value/public" + subjectMapping := &policy.SubjectMapping{Id: "subject-mapping-id"} + getEntitleableAttributesResponse = &attr.GetEntitleableAttributesByFqnsResponse{ + Definitions: map[string]*attr.GetEntitleableAttributesByFqnsResponse_EntitleableDefinition{ + definitionFQN: { + Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_HIERARCHY, + Namespace: &policy.Namespace{Id: "namespace-id", Fqn: "https://example.com"}, + Values: []*attr.GetEntitleableAttributesByFqnsResponse_EntitleableValue{ + {Fqn: valueFQN, ValueId: "confidential-id", SubjectMappings: []*policy.SubjectMapping{subjectMapping}}, + {Fqn: lowerValueFQN, ValueId: "public-id"}, + }, + }, + }, + FqnEntitleableAttributes: map[string]*attr.GetEntitleableAttributesByFqnsResponse_EntitleableAttribute{ + valueFQN: { + DefinitionFqn: definitionFQN, + Value: &attr.GetEntitleableAttributesByFqnsResponse_EntitleableValue{ + Fqn: valueFQN, ValueId: "confidential-id", SubjectMappings: []*policy.SubjectMapping{subjectMapping}, + }, + }, + }, + } + + sdk := &otdf.SDK{Attributes: &myAttributesClient{}} + mapped, err := retrieveAttributeDefinitions(t.Context(), []string{strings.ToUpper(valueFQN), valueFQN}, sdk) + require.NoError(t, err) + require.Len(t, mapped, 1) + require.Len(t, getEntitleableAttributesRequests, 1) + assert.Equal(t, []string{valueFQN}, getEntitleableAttributesRequests[0].GetFqns()) + assert.Zero(t, getAttributeValuesCallCount) + + attributeAndValue := mapped[valueFQN] + require.NotNil(t, attributeAndValue) + assert.Equal(t, definitionFQN, attributeAndValue.GetAttribute().GetFqn()) + assert.Equal(t, policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_HIERARCHY, attributeAndValue.GetAttribute().GetRule()) + assert.Equal(t, "namespace-id", attributeAndValue.GetAttribute().GetNamespace().GetId()) + require.Len(t, attributeAndValue.GetAttribute().GetValues(), 2) + assert.Equal(t, []string{valueFQN, lowerValueFQN}, []string{ + attributeAndValue.GetAttribute().GetValues()[0].GetFqn(), + attributeAndValue.GetAttribute().GetValues()[1].GetFqn(), + }) + assert.Equal(t, "confidential-id", attributeAndValue.GetValue().GetId()) + assert.Equal(t, valueFQN, attributeAndValue.GetValue().GetFqn()) + assert.Equal(t, "subject-mapping-id", attributeAndValue.GetValue().GetSubjectMappings()[0].GetId()) +} + +func TestRetrieveAttributeDefinitionsBatchesRequests(t *testing.T) { + resetTargetedLookupMocks(t) + + definitionFQN := "https://example.com/attr/classification" + response := &attr.GetEntitleableAttributesByFqnsResponse{ + Definitions: map[string]*attr.GetEntitleableAttributesByFqnsResponse_EntitleableDefinition{ + definitionFQN: {Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_ANY_OF}, + }, + FqnEntitleableAttributes: make(map[string]*attr.GetEntitleableAttributesByFqnsResponse_EntitleableAttribute), + } + fqns := make([]string, maxEntitleableFQNsPerRequest+1) + for i := range fqns { + fqns[i] = fmt.Sprintf("%s/value/value-%03d", definitionFQN, i) + response.FqnEntitleableAttributes[fqns[i]] = &attr.GetEntitleableAttributesByFqnsResponse_EntitleableAttribute{ + DefinitionFqn: definitionFQN, + Value: &attr.GetEntitleableAttributesByFqnsResponse_EntitleableValue{Fqn: fqns[i], ValueId: fmt.Sprintf("value-%03d-id", i)}, + } + } + getEntitleableAttributesResponse = response + + mapped, err := retrieveAttributeDefinitions(t.Context(), fqns, &otdf.SDK{Attributes: &myAttributesClient{}}) + require.NoError(t, err) + assert.Len(t, mapped, len(fqns)) + require.Len(t, getEntitleableAttributesRequests, 2) + assert.Len(t, getEntitleableAttributesRequests[0].GetFqns(), maxEntitleableFQNsPerRequest) + assert.Len(t, getEntitleableAttributesRequests[1].GetFqns(), 1) +} + +func TestRetrieveAttributeDefinitionsRejectsMissingDefinition(t *testing.T) { + resetTargetedLookupMocks(t) + + fqn := "https://example.com/attr/classification/value/confidential" + getEntitleableAttributesResponse = &attr.GetEntitleableAttributesByFqnsResponse{ + FqnEntitleableAttributes: map[string]*attr.GetEntitleableAttributesByFqnsResponse_EntitleableAttribute{ + fqn: { + DefinitionFqn: "https://example.com/attr/classification", + Value: &attr.GetEntitleableAttributesByFqnsResponse_EntitleableValue{Fqn: fqn, ValueId: "value-id"}, + }, + }, + } + + result, err := retrieveAttributeDefinitions(t.Context(), []string{fqn}, &otdf.SDK{Attributes: &myAttributesClient{}}) + require.Error(t, err) + assert.Nil(t, result) + assert.Equal(t, connect.CodeInternal, connect.CodeOf(err)) + assert.Contains(t, err.Error(), "references missing definition") +} + +func TestRetrieveMatchedAttributeMappingsFiltersScope(t *testing.T) { + resetTargetedLookupMocks(t) + + fqn1 := "https://example.com/attr/classification/value/confidential" + fqn2 := "https://example.com/attr/classification/value/public" + getAttributesByValueFqnsResponse = attr.GetAttributeValuesByFqnsResponse{ + FqnAttributeValues: map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue{ + fqn1: {Attribute: &policy.Attribute{Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_ANY_OF}, Value: &policy.Value{Id: "one", Fqn: fqn1}}, + fqn2: {Attribute: &policy.Attribute{Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_ANY_OF}, Value: &policy.Value{Id: "two", Fqn: fqn2}}, + }, + } + props1, err := structpb.NewStruct(map[string]any{"department": "engineering", "region": "east"}) + require.NoError(t, err) + props2, err := structpb.NewStruct(map[string]any{"department": "engineering"}) + require.NoError(t, err) + ersResp := &entityresolution.ResolveEntitiesResponse{EntityRepresentations: []*entityresolution.EntityRepresentation{ + {OriginalId: "one", AdditionalProps: []*structpb.Struct{props1}}, + {OriginalId: "two", AdditionalProps: []*structpb.Struct{props2}}, + }} + as := &AuthorizationService{sdk: &otdf.SDK{Attributes: &myAttributesClient{}, SubjectMapping: &mySubjectMappingClient{}}} + + mapped, err := as.retrieveMatchedAttributeMappings(t.Context(), ersResp, &authorization.ResourceAttribute{ + AttributeValueFqns: []string{strings.ToUpper(fqn1)}, + }) + require.NoError(t, err) + require.Len(t, matchSubjectMappingsRequests, 1) + selectors := make([]string, 0, len(matchSubjectMappingsRequests[0].GetSubjectProperties())) + for _, property := range matchSubjectMappingsRequests[0].GetSubjectProperties() { + selectors = append(selectors, property.GetExternalSelectorValue()) + } + assert.ElementsMatch(t, []string{".department", ".region"}, selectors) + require.Len(t, getEntitleableAttributesRequests, 1) + assert.Equal(t, []string{fqn1}, getEntitleableAttributesRequests[0].GetFqns()) + assert.Contains(t, mapped, fqn1) + assert.NotContains(t, mapped, fqn2) + assert.Zero(t, listAttributesCallCount) + assert.Zero(t, listSubjectMappingsCallCount) + assert.Zero(t, getAttributeValuesCallCount) +} + +func TestRetrieveMatchedAttributeMappingsPropagatesMatchError(t *testing.T) { + resetTargetedLookupMocks(t) + + props, err := structpb.NewStruct(map[string]any{"department": "engineering"}) + require.NoError(t, err) + errMatchSubjectMappings = errors.New("match failed") + as := &AuthorizationService{sdk: &otdf.SDK{Attributes: &myAttributesClient{}, SubjectMapping: &mySubjectMappingClient{}}} + + mapped, err := as.retrieveMatchedAttributeMappings(t.Context(), &entityresolution.ResolveEntitiesResponse{ + EntityRepresentations: []*entityresolution.EntityRepresentation{{AdditionalProps: []*structpb.Struct{props}}}, + }, nil) + require.Error(t, err) + assert.Nil(t, mapped) + assert.Contains(t, err.Error(), "failed to match subject mappings") + assert.Empty(t, getEntitleableAttributesRequests) +} + +func TestGetEntitlementsEvaluatesRegoWithoutMatchedSelectors(t *testing.T) { + resetTargetedLookupMocks(t) + getAttributesByValueFqnsResponse = attr.GetAttributeValuesByFqnsResponse{} + resolveEntitiesResp = entityresolution.ResolveEntitiesResponse{ + EntityRepresentations: []*entityresolution.EntityRepresentation{{OriginalId: "e1"}}, + } + + prepared, err := rego.New( + rego.SetRegoVersion(ast.RegoV0), + rego.Query("data.example.p"), + rego.Module("example.rego", `package example + p = {"e1":["https://example.com/attr/classification/value/confidential"]} { true }`), + ).PrepareForEval(t.Context()) + require.NoError(t, err) + as := &AuthorizationService{ + logger: logger.CreateTestLogger(), + sdk: &otdf.SDK{ + SubjectMapping: &mySubjectMappingClient{}, + Attributes: &myAttributesClient{}, + EntityResoution: &myERSClient{}, + }, + eval: prepared, + Tracer: noop.NewTracerProvider().Tracer(""), + } + + resp, err := as.GetEntitlements(t.Context(), &connect.Request[authorization.GetEntitlementsRequest]{ + Msg: &authorization.GetEntitlementsRequest{Entities: []*authorization.Entity{{Id: "e1"}}}, + }) + require.NoError(t, err) + require.Len(t, resp.Msg.GetEntitlements(), 1) + assert.Equal(t, []string{"https://example.com/attr/classification/value/confidential"}, resp.Msg.GetEntitlements()[0].GetAttributeValueFqns()) + assert.Empty(t, matchSubjectMappingsRequests) + assert.Empty(t, getEntitleableAttributesRequests) + assert.Zero(t, listAttributesCallCount) + assert.Zero(t, listSubjectMappingsCallCount) +} diff --git a/service/authorization/authorization_test.go b/service/authorization/authorization_test.go index ae50f085f0..00be00b487 100644 --- a/service/authorization/authorization_test.go +++ b/service/authorization/authorization_test.go @@ -737,6 +737,10 @@ func Test_GetEntitlementsFqnCasing(t *testing.T) { func Test_GetEntitlements_HandlesPagination(t *testing.T) { logger := logger.CreateTestLogger() + smPaginationOffset = 3 + smListCallCount = 0 + attrPaginationOffset = 3 + attrListCallCount = 0 listAttributeResp = attr.ListAttributesResponse{} attrDef := policy.Attribute{ @@ -784,6 +788,7 @@ func Test_GetEntitlements_HandlesPagination(t *testing.T) { require.NoError(t, err) as := AuthorizationService{ + config: &Config{Rego: CustomRego{Path: "custom.rego"}}, logger: logger, sdk: &otdf.SDK{ SubjectMapping: &paginatedMockSubjectMappingClient{}, diff --git a/service/authorization/authorization_test_structures.go b/service/authorization/authorization_test_structures.go index 6eeb7b6ae2..ff1e9e9ed3 100644 --- a/service/authorization/authorization_test_structures.go +++ b/service/authorization/authorization_test_structures.go @@ -2,7 +2,9 @@ package authorization import ( "context" + "errors" "fmt" + "strings" "github.com/opentdf/platform/protocol/go/entityresolution" "github.com/opentdf/platform/protocol/go/policy" @@ -10,9 +12,83 @@ import ( sm "github.com/opentdf/platform/protocol/go/policy/subjectmapping" ) +func entitleableResponseFromLegacy(response *attr.GetAttributeValuesByFqnsResponse) *attr.GetEntitleableAttributesByFqnsResponse { + entitleable := &attr.GetEntitleableAttributesByFqnsResponse{ + Definitions: make(map[string]*attr.GetEntitleableAttributesByFqnsResponse_EntitleableDefinition), + FqnEntitleableAttributes: make(map[string]*attr.GetEntitleableAttributesByFqnsResponse_EntitleableAttribute), + } + for fqn, attributeAndValue := range response.GetFqnAttributeValues() { + definitionFQN := attributeAndValue.GetAttribute().GetFqn() + if definitionFQN == "" { + if valueIndex := strings.LastIndex(fqn, "/value/"); valueIndex >= 0 { + definitionFQN = fqn[:valueIndex] + } + } + + definition := &attr.GetEntitleableAttributesByFqnsResponse_EntitleableDefinition{ + Rule: attributeAndValue.GetAttribute().GetRule(), + Namespace: attributeAndValue.GetAttribute().GetNamespace(), + } + if definition.GetRule() == policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_HIERARCHY { + for _, value := range attributeAndValue.GetAttribute().GetValues() { + valueFQN := value.GetFqn() + if valueFQN == "" && definitionFQN != "" && value.GetValue() != "" { + valueFQN = definitionFQN + "/value/" + value.GetValue() + } + valueID := value.GetId() + if valueID == "" && value != nil { + valueID = valueFQN + } + definition.Values = append(definition.Values, &attr.GetEntitleableAttributesByFqnsResponse_EntitleableValue{ + Fqn: valueFQN, + ValueId: valueID, + SubjectMappings: value.GetSubjectMappings(), + }) + } + } + entitleable.Definitions[definitionFQN] = definition + + value := attributeAndValue.GetValue() + valueFQN := value.GetFqn() + if valueFQN == "" { + valueFQN = fqn + } + valueID := value.GetId() + if valueID == "" && value != nil { + valueID = valueFQN + } + entitleable.FqnEntitleableAttributes[strings.ToLower(fqn)] = &attr.GetEntitleableAttributesByFqnsResponse_EntitleableAttribute{ + DefinitionFqn: definitionFQN, + Value: &attr.GetEntitleableAttributesByFqnsResponse_EntitleableValue{ + Fqn: valueFQN, + ValueId: valueID, + SubjectMappings: value.GetSubjectMappings(), + }, + } + } + return entitleable +} + +func matchedSubjectMappingsFromLegacy(response *attr.GetAttributeValuesByFqnsResponse) *sm.MatchSubjectMappingsResponse { + matched := &sm.MatchSubjectMappingsResponse{} + for fqn := range response.GetFqnAttributeValues() { + matched.SubjectMappings = append(matched.SubjectMappings, &policy.SubjectMapping{ + AttributeValue: &policy.Value{Fqn: fqn}, + }) + } + return matched +} + var ( getAttributesByValueFqnsResponse attr.GetAttributeValuesByFqnsResponse + getEntitleableAttributesResponse *attr.GetEntitleableAttributesByFqnsResponse errGetAttributesByValueFqns error + errMatchSubjectMappings error + getEntitleableAttributesRequests []*attr.GetEntitleableAttributesByFqnsRequest + matchSubjectMappingsRequests []*sm.MatchSubjectMappingsRequest + listAttributesCallCount int + listSubjectMappingsCallCount int + getAttributeValuesCallCount int listAttributeResp attr.ListAttributesResponse errListAttributes error listSubjectMappings sm.ListSubjectMappingsResponse @@ -32,19 +108,25 @@ var ( type myAttributesClient struct{} func (*myAttributesClient) ListAttributes(_ context.Context, _ *attr.ListAttributesRequest) (*attr.ListAttributesResponse, error) { + listAttributesCallCount++ return &listAttributeResp, errListAttributes } func (*myAttributesClient) GetAttributeValuesByFqns(_ context.Context, _ *attr.GetAttributeValuesByFqnsRequest) (*attr.GetAttributeValuesByFqnsResponse, error) { - return &getAttributesByValueFqnsResponse, errGetAttributesByValueFqns + getAttributeValuesCallCount++ + return nil, errors.New("deprecated GetAttributeValuesByFqns called") } func (*myAttributesClient) GetKeyMappingsByFqns(_ context.Context, _ *attr.GetKeyMappingsByFqnsRequest) (*attr.GetKeyMappingsByFqnsResponse, error) { return &attr.GetKeyMappingsByFqnsResponse{}, nil } -func (*myAttributesClient) GetEntitleableAttributesByFqns(_ context.Context, _ *attr.GetEntitleableAttributesByFqnsRequest) (*attr.GetEntitleableAttributesByFqnsResponse, error) { - return &attr.GetEntitleableAttributesByFqnsResponse{}, nil +func (*myAttributesClient) GetEntitleableAttributesByFqns(_ context.Context, req *attr.GetEntitleableAttributesByFqnsRequest) (*attr.GetEntitleableAttributesByFqnsResponse, error) { + getEntitleableAttributesRequests = append(getEntitleableAttributesRequests, req) + if getEntitleableAttributesResponse != nil { + return getEntitleableAttributesResponse, errGetAttributesByValueFqns + } + return entitleableResponseFromLegacy(&getAttributesByValueFqnsResponse), errGetAttributesByValueFqns } func (*myAttributesClient) ListAttributeValues(_ context.Context, _ *attr.ListAttributeValuesRequest) (*attr.ListAttributeValuesResponse, error) { @@ -130,11 +212,13 @@ func (*myERSClient) ResolveEntities(_ context.Context, _ *entityresolution.Resol type mySubjectMappingClient struct{} func (*mySubjectMappingClient) ListSubjectMappings(_ context.Context, _ *sm.ListSubjectMappingsRequest) (*sm.ListSubjectMappingsResponse, error) { + listSubjectMappingsCallCount++ return &listSubjectMappings, nil } -func (*mySubjectMappingClient) MatchSubjectMappings(_ context.Context, _ *sm.MatchSubjectMappingsRequest) (*sm.MatchSubjectMappingsResponse, error) { - return &sm.MatchSubjectMappingsResponse{}, nil +func (*mySubjectMappingClient) MatchSubjectMappings(_ context.Context, req *sm.MatchSubjectMappingsRequest) (*sm.MatchSubjectMappingsResponse, error) { + matchSubjectMappingsRequests = append(matchSubjectMappingsRequests, req) + return matchedSubjectMappingsFromLegacy(&getAttributesByValueFqnsResponse), errMatchSubjectMappings } func (*mySubjectMappingClient) GetSubjectMapping(_ context.Context, _ *sm.GetSubjectMappingRequest) (*sm.GetSubjectMappingResponse, error) { @@ -202,7 +286,7 @@ func (*paginatedMockSubjectMappingClient) ListSubjectMappings(_ context.Context, } func (*paginatedMockSubjectMappingClient) MatchSubjectMappings(_ context.Context, _ *sm.MatchSubjectMappingsRequest) (*sm.MatchSubjectMappingsResponse, error) { - return &sm.MatchSubjectMappingsResponse{}, nil + return matchedSubjectMappingsFromLegacy(&getAttributesByValueFqnsResponse), nil } func (*paginatedMockSubjectMappingClient) GetSubjectMapping(_ context.Context, _ *sm.GetSubjectMappingRequest) (*sm.GetSubjectMappingResponse, error) { @@ -278,7 +362,7 @@ func (*paginatedMockAttributesClient) GetKeyMappingsByFqns(_ context.Context, _ } func (*paginatedMockAttributesClient) GetEntitleableAttributesByFqns(_ context.Context, _ *attr.GetEntitleableAttributesByFqnsRequest) (*attr.GetEntitleableAttributesByFqnsResponse, error) { - return &attr.GetEntitleableAttributesByFqnsResponse{}, nil + return entitleableResponseFromLegacy(&getAttributesByValueFqnsResponse), errGetAttributesByValueFqns } func (*paginatedMockAttributesClient) ListAttributeValues(_ context.Context, _ *attr.ListAttributeValuesRequest) (*attr.ListAttributeValuesResponse, error) { From 12bd86b2b4976d8aebbfc3bbe1776224f12da891 Mon Sep 17 00:00:00 2001 From: Krish Suchak Date: Thu, 20 Aug 2026 18:10:26 -0400 Subject: [PATCH 2/2] fix(authz): address entitlement lookup review feedback - sanitize the subject-mapping retrieval error returned to clients - document that the 250 batch size mirrors the proto max_items limit - stop getComprehensiveHierarchy warning on non-hierarchy attributes - add a unit test that absent NOT_IN selectors are not forwarded Signed-off-by: Krish Suchak --- service/authorization/authorization.go | 11 ++++- .../authorization_entitleable_test.go | 40 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/service/authorization/authorization.go b/service/authorization/authorization.go index 6e283cb2db..79c7f03cac 100644 --- a/service/authorization/authorization.go +++ b/service/authorization/authorization.go @@ -41,6 +41,9 @@ import ( var ErrEmptyStringAttribute = errors.New("resource attributes must have at least one attribute value fqn") +// maxEntitleableFQNsPerRequest mirrors the proto max_items limit on +// GetEntitleableAttributesByFqnsRequest.fqns, so batches never exceed what the +// server-side request validation accepts. const maxEntitleableFQNsPerRequest = 250 type AuthorizationService struct { //nolint:revive // AuthorizationService is a valid name for this struct @@ -405,7 +408,7 @@ func (as *AuthorizationService) GetEntitlements(ctx context.Context, req *connec } if err != nil { as.logger.ErrorContext(ctx, "failed to retrieve subject mappings", slog.String("error", err.Error())) - return nil, connect.NewError(connect.CodeInternal, err) + return nil, connect.NewError(connect.CodeInternal, errors.New("failed to retrieve subject mappings")) } as.logger.DebugContext(ctx, "retrieved subject mappings", slog.Int("count", len(subjectMappings))) avf := &attr.GetAttributeValuesByFqnsResponse{FqnAttributeValues: subjectMappings} @@ -865,7 +868,11 @@ func getComprehensiveHierarchy(attributesMap map[string]*policy.Attribute, avf * if len(attributesMap) == 0 { // Go through all attribute definitions attrDefs := avf.GetFqnAttributeValues() - for _, attrDef := range attrDefs { + for fqn, attrDef := range attrDefs { + // map the requested value FQN directly; non-hierarchy definitions + // carry no sibling values, so without this they would miss the map + // and log a spurious "no attribute definition found" warning. + attributesMap[fqn] = attrDef.GetAttribute() for _, attrVal := range attrDef.GetAttribute().GetValues() { attributesMap[attrVal.GetFqn()] = attrDef.GetAttribute() } diff --git a/service/authorization/authorization_entitleable_test.go b/service/authorization/authorization_entitleable_test.go index 7fd2a345ce..b9774ce585 100644 --- a/service/authorization/authorization_entitleable_test.go +++ b/service/authorization/authorization_entitleable_test.go @@ -198,6 +198,46 @@ func TestRetrieveMatchedAttributeMappingsPropagatesMatchError(t *testing.T) { assert.Empty(t, getEntitleableAttributesRequests) } +// TestRetrieveMatchedAttributeMappingsOmitsAbsentNotInSelectors pins the v1 +// consequence of the targeted lookup for NOT_IN subject mappings. The old +// full-policy load handed every subject mapping to Rego, and a NOT_IN condition +// passes when its selector is absent from the entity, so a mapping such as +// ".groups NOT_IN [...]" entitled an entity that carried no ".groups" claim. The +// targeted path forwards only the selectors the entity actually has to the +// server-side MatchSubjectMappings prefilter, so a mapping conditioned on an +// absent selector is never returned and no longer entitles. This matches the v2 +// JustInTimePDP, which uses the same flatten -> MatchSubjectMappings approach. +func TestRetrieveMatchedAttributeMappingsOmitsAbsentNotInSelectors(t *testing.T) { + resetTargetedLookupMocks(t) + + // The entity carries ".clientId" but not ".groups" (the NOT_IN selector). + props, err := structpb.NewStruct(map[string]any{"clientId": "abc"}) + require.NoError(t, err) + ersResp := &entityresolution.ResolveEntitiesResponse{EntityRepresentations: []*entityresolution.EntityRepresentation{ + {OriginalId: "e1", AdditionalProps: []*structpb.Struct{props}}, + }} + as := &AuthorizationService{sdk: &otdf.SDK{Attributes: &myAttributesClient{}, SubjectMapping: &mySubjectMappingClient{}}} + + // getAttributesByValueFqnsResponse is left empty, so the mock + // MatchSubjectMappings returns no mappings, mirroring a server that finds no + // mapping selector matching ".clientId". + mapped, err := as.retrieveMatchedAttributeMappings(t.Context(), ersResp, nil) + require.NoError(t, err) + assert.Empty(t, mapped) + + // Only the entity's present selector is forwarded; ".groups" is never sent, + // so a NOT_IN mapping keyed on it cannot be matched and cannot entitle. + require.Len(t, matchSubjectMappingsRequests, 1) + selectors := make([]string, 0, len(matchSubjectMappingsRequests[0].GetSubjectProperties())) + for _, property := range matchSubjectMappingsRequests[0].GetSubjectProperties() { + selectors = append(selectors, property.GetExternalSelectorValue()) + } + assert.Equal(t, []string{".clientId"}, selectors) + assert.NotContains(t, selectors, ".groups") + // No entitleable lookup happens when no mappings match. + assert.Empty(t, getEntitleableAttributesRequests) +} + func TestGetEntitlementsEvaluatesRegoWithoutMatchedSelectors(t *testing.T) { resetTargetedLookupMocks(t) getAttributesByValueFqnsResponse = attr.GetAttributeValuesByFqnsResponse{}