refactor(authz): use targeted entitlement lookups - #3873
Conversation
Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAuthorization entitlement retrieval now supports separate custom-Rego and built-in-Rego paths. Built-in Rego derives subject properties, matches scoped subject mappings, and retrieves targeted attribute definitions in normalized batches. Tests cover mapping, batching, validation, error propagation, and entitlement evaluation. ChangesAuthorization entitlement flow
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The refactor changes authorization entitlement lookups while preserving the configured custom-policy path. An additional integration test for NOT_IN behavior would improve coverage, but no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant EntityResolution
participant AuthorizationService
participant SubjectMappingAPI
participant AttributeEntitlementAPI
participant RegoEvaluator
EntityResolution->>AuthorizationService: Provide resolved entity representations
AuthorizationService->>AuthorizationService: Flatten subject properties
alt Custom Rego
AuthorizationService->>SubjectMappingAPI: Retrieve active subject mappings
else Built-in Rego
AuthorizationService->>SubjectMappingAPI: Match subject mappings
SubjectMappingAPI-->>AuthorizationService: Return scoped attribute FQNs
AuthorizationService->>AttributeEntitlementAPI: Retrieve targeted definitions
end
AuthorizationService->>RegoEvaluator: Evaluate entitlement input
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
|
||
| var ErrEmptyStringAttribute = errors.New("resource attributes must have at least one attribute value fqn") | ||
|
|
||
| const maxEntitleableFQNsPerRequest = 250 |
There was a problem hiding this comment.
whats the reasoning for 250? is this defined in the proto?
There was a problem hiding this comment.
yes, it's the max from the proto
| } | ||
| if err != nil { | ||
| as.logger.ErrorContext(ctx, "failed to retrieve subject mappings", slog.String("error", err.Error())) | ||
| return nil, connect.NewError(connect.CodeInternal, err) |
There was a problem hiding this comment.
do we want to sanitize this error like the old code flow? ex: connect.NewError(connect.CodeInternal, errors.New("failed to list attributes")) instead of giving the full err stack
There was a problem hiding this comment.
good idea, sanitized
| return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("entitleable attribute %q references missing definition %q", fqn, definitionFQN)) | ||
| } | ||
|
|
||
| attribute := &policy.Attribute{ |
There was a problem hiding this comment.
should we set name here as well? for the fqn builder
There was a problem hiding this comment.
we shouldn't need it bc populateAttrDefValueFqns only builds FQNs when value.GetFqn() == "", but GetEntitleableAttributesByFqns should always be returning FQNs
| value := entitleable.GetValue() | ||
| result[fqn] = &attr.GetAttributeValuesByFqnsResponse_AttributeAndValue{ | ||
| Attribute: attribute, | ||
| Value: &policy.Value{ |
There was a problem hiding this comment.
same here for value.value
| } | ||
| } | ||
| attrDef := attributesMap[entitlement] | ||
| if attrDef == nil { |
There was a problem hiding this comment.
since were not populating non-hierarchy arrd defs in the map anymore, this will trigger a warn log for all non hierarchy attrs
There was a problem hiding this comment.
changed so getComprehensiveHierarchy now keys the map on the requested value FQN too
non-hierarchy attrs should resolve without the warn now
| if as.usesCustomRego() { | ||
| subjectMappings, err = retrieveFullAttributeMappings(ctx, req.Msg.GetScope(), as.sdk) | ||
| } else { | ||
| subjectMappings, err = as.retrieveMatchedAttributeMappings(ctx, ersResp, req.Msg.GetScope()) |
There was a problem hiding this comment.
ai flagged this -- NOT_IN mappings on absent selectors now dropped
the old path handed the whole attribute/subject-mapping universe to Rego, which evaluated every condition set. NOT_IN initializes notInResult := true and only flips false on a match. an empty flattened result means the loop never runs and the condition passes. the new SQL prefilter never gives Rego the chance
"
The MatchSubjectMappings prefilter changes which mappings can entitle. matchSubjectMappings (queries/subject_mappings.sql:388) filters on scs.selector_values && @selectors, so a mapping only returns if the entity's flattened props contain one of its selectors. But EvaluateCondition (subject_mapping_builtin.go:227-243) returns true for NOT_IN when the selector is absent. So .groups NOT_IN ["contractors"] used to entitle an entity with no .groups claim, and now won't. The len(properties) == 0 early return at :508 makes the extreme case explicit — zero props means zero entitlements with no policy evaluation. Intentional? v2 does the same (just_in_time_pdp.go:334), so it may be deliberate alignment, but it's a silent entitlement loss for existing v1 deployments and isn't in the PR description. Worth an xtest run exercising a NOT_IN mapping.
"
There was a problem hiding this comment.
I was trying to align v1's default entitlement path with v2. Both flatten the entity's properties and call MatchSubjectMappings, so only selectors the entity actually presents reach policy. A mapping whose condition references a selector the entity lacks (.groups NOT_IN [...] with no .groups claim) is never returned and so can't entitle. v2's JustInTimePDP.getMatchedSubjectMappings has always behaved this way.
Also, this only affects the built-in rego path. Configured custom rego still loads the full attribute and subject-mapping set via retrieveFullAttributeMappings, so `NOT_IN-on-absent-selector behavior is unchanged there.
Here's the v2 PR: #3912
xtest run here, but decrypt/rewrap path uses the v2 PDP (AuthorizationV2.GetDecision) rather than v1 GetEntitlements
| } | ||
|
|
||
| result := make(map[string]*attr.GetAttributeValuesByFqnsResponse_AttributeAndValue, len(normalizedFQNs)) | ||
| for start := 0; start < len(normalizedFQNs); start += maxEntitleableFQNsPerRequest { |
There was a problem hiding this comment.
possible future improvement: the batches here could be paralellized
- 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 <suchak.krish@gmail.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
2 participants
Proposed Changes
Checklist
Testing Instructions
make fmtgolangci-lint run ./authorizationgo test ./authorizationcd service && go test ./... -race(all packages passed exceptservice/rttests, which requires a platform onlocalhost:8080)cd sdk && go test -run TestREADMECodeBlocksmake lintreaches all modules and reports existing findings in unchanged SDK, server, and BDD filesmake teststops inlib/fixturesbecause Keycloak is unavailable onlocalhost:8888Summary by CodeRabbit
New Features
Bug Fixes
Tests