feat(ers): add direct entitlement support to the claims ERS + bdd tests for the feature#3671
feat(ers): add direct entitlement support to the claims ERS + bdd tests for the feature#3671elizabethhealy wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements direct entitlement support within the Entity Resolution Service. By allowing entitlements to be embedded directly within claims, the system gains more flexibility in authorization decisions. The changes include logic to parse these entitlements, a configuration toggle to control the feature, and a suite of BDD tests to ensure correct behavior under various scenarios. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Claims now hold a secret key, / Direct entitlements, wild and free. / With a toggle set to true, / The system knows just what to do, / And checks the path for you and me. Footnotes
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for direct entitlements within the claims-based entity resolution service, allowing the platform to parse and evaluate direct entitlements (attribute value FQNs and actions) from entity claims when configured. It includes updates to service registration, configuration decoding, unit tests, and BDD scenarios. The review feedback suggests improving error handling by replacing log.Fatalf with panic during service registration to prevent abrupt process termination and allow proper cleanup, which also enables removing the unused "log" import. Additionally, it recommends adding a nil check for the claims map to safely handle empty structs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "log" | ||
| "log/slog" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "connectrpc.com/connect" | ||
| "github.com/go-viper/mapstructure/v2" | ||
| "github.com/lestrrat-go/jwx/v2/jwt" | ||
| "github.com/opentdf/platform/protocol/go/entity" | ||
| entityresolutionV2 "github.com/opentdf/platform/protocol/go/entityresolution/v2" |
There was a problem hiding this comment.
The "log" package is no longer needed if we replace log.Fatalf with panic in RegisterClaimsERS. We can clean up the imports by removing it.
| import ( | |
| "context" | |
| "errors" | |
| "fmt" | |
| "log" | |
| "log/slog" | |
| "strconv" | |
| "strings" | |
| "connectrpc.com/connect" | |
| "github.com/go-viper/mapstructure/v2" | |
| "github.com/lestrrat-go/jwx/v2/jwt" | |
| "github.com/opentdf/platform/protocol/go/entity" | |
| entityresolutionV2 "github.com/opentdf/platform/protocol/go/entityresolution/v2" | |
| import ( | |
| "context" | |
| "errors" | |
| "fmt" | |
| "log/slog" | |
| "strconv" | |
| "strings" | |
| "connectrpc.com/connect" | |
| "github.com/go-viper/mapstructure/v2" | |
| "github.com/lestrrat-go/jwx/v2/jwt" | |
| "github.com/opentdf/platform/protocol/go/entity" | |
| entityresolutionV2 "github.com/opentdf/platform/protocol/go/entityresolution/v2" |
| func RegisterClaimsERS(cfg config.ServiceConfig, logger *logger.Logger) (EntityResolutionServiceV2, serviceregistry.HandlerServer) { | ||
| var inputConfig ClaimsConfig | ||
| if err := mapstructure.Decode(cfg, &inputConfig); err != nil { | ||
| logger.Error("failed to decode claims entity resolution configuration", slog.Any("error", err)) | ||
| log.Fatalf("Failed to decode claims entity resolution configuration: %v", err) | ||
| } | ||
| claimsSVC := EntityResolutionServiceV2{ | ||
| logger: logger, | ||
| allowDirectEntitlements: inputConfig.AllowDirectEntitlements, | ||
| } | ||
| return claimsSVC, nil | ||
| } |
There was a problem hiding this comment.
Using log.Fatalf inside a service registration function is discouraged. It abruptly terminates the process via os.Exit(1), which prevents deferred cleanup functions from running and bypasses structured logging (printing directly to stderr in standard log format instead of structured JSON). Since the error is already logged via logger.Error, we should use panic to halt initialization gracefully if configuration decoding fails.
| func RegisterClaimsERS(cfg config.ServiceConfig, logger *logger.Logger) (EntityResolutionServiceV2, serviceregistry.HandlerServer) { | |
| var inputConfig ClaimsConfig | |
| if err := mapstructure.Decode(cfg, &inputConfig); err != nil { | |
| logger.Error("failed to decode claims entity resolution configuration", slog.Any("error", err)) | |
| log.Fatalf("Failed to decode claims entity resolution configuration: %v", err) | |
| } | |
| claimsSVC := EntityResolutionServiceV2{ | |
| logger: logger, | |
| allowDirectEntitlements: inputConfig.AllowDirectEntitlements, | |
| } | |
| return claimsSVC, nil | |
| } | |
| func RegisterClaimsERS(cfg config.ServiceConfig, logger *logger.Logger) (EntityResolutionServiceV2, serviceregistry.HandlerServer) { | |
| var inputConfig ClaimsConfig | |
| if err := mapstructure.Decode(cfg, &inputConfig); err != nil { | |
| logger.Error("failed to decode claims entity resolution configuration", slog.Any("error", err)) | |
| panic(fmt.Errorf("failed to decode claims entity resolution configuration: %w", err)) | |
| } | |
| claimsSVC := EntityResolutionServiceV2{ | |
| logger: logger, | |
| allowDirectEntitlements: inputConfig.AllowDirectEntitlements, | |
| } | |
| return claimsSVC, nil | |
| } |
| func parseDirectEntitlementsFromClaims(entityStruct *structpb.Struct) ([]*entityresolutionV2.DirectEntitlement, error) { | ||
| if entityStruct == nil { | ||
| return nil, nil | ||
| } | ||
| claims := entityStruct.AsMap() | ||
| rawEntitlements, ok := claims["direct_entitlements"] |
There was a problem hiding this comment.
If entityStruct is empty (e.g., &structpb.Struct{}), entityStruct.AsMap() will return nil. While reading from a nil map is safe in Go and returns the zero value, it is safer and more explicit to add a nil check for claims to prevent any future refactoring issues or confusion.
| func parseDirectEntitlementsFromClaims(entityStruct *structpb.Struct) ([]*entityresolutionV2.DirectEntitlement, error) { | |
| if entityStruct == nil { | |
| return nil, nil | |
| } | |
| claims := entityStruct.AsMap() | |
| rawEntitlements, ok := claims["direct_entitlements"] | |
| func parseDirectEntitlementsFromClaims(entityStruct *structpb.Struct) ([]*entityresolutionV2.DirectEntitlement, error) { | |
| if entityStruct == nil { | |
| return nil, nil | |
| } | |
| claims := entityStruct.AsMap() | |
| if claims == nil { | |
| return nil, nil | |
| } | |
| rawEntitlements, ok := claims["direct_entitlements"] |
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
1 participant
Proposed Changes
Checklist
Testing Instructions