feat(authz): DSPX-895 auth svc registered resource GetDecision support #2392
Conversation
…without values and update tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @ryanulit, 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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.
This PR introduces support for registered resources within the authorization service's Policy Decision Point (PDP) logic. Specifically, it adds the capability to fetch and process registered resources and their associated action-attribute values to determine entitlements for entities identified by a registered resource value FQN. The core change involves modifying the PDP to load registered resources into memory and implementing the GetEntitlementsRegisteredResource method. While the structure for GetDecisionRegisteredResource is added, its full implementation is noted as a future task.
Highlights
- Registered Resource Support: Adds the foundational logic within the PDP to handle registered resources as a type of entity for entitlement checks.
- GetEntitlements Implementation: Implements the
GetEntitlementsRegisteredResourcemethod to calculate entitlements for a given registered resource value FQN based on its defined action-attribute values, including support for comprehensive hierarchy. - Data Loading: Modifies the PDP initialization to fetch all registered resources from the policy service and store their values in memory for efficient lookup.
- Validation and Testing: Adds new validation functions for registered resources and values, along with comprehensive unit tests for the new entitlement logic and validation.
- GetDecision Placeholder: Includes a placeholder method
GetDecisionRegisteredResourcewhich is not yet fully implemented.
Changelog
Click here to see the changelog
- service/internal/access/v2/helpers.go
- Added new error types
ErrInvalidRegisteredResource(line 19) andErrInvalidRegisteredResourceValue(line 20) for validation failures related to registered resources.
- Added new error types
- service/internal/access/v2/just_in_time_pdp.go
- Imported
strings(line 8) andregisteredresourcesprotocol (line 17). - Modified
NewJustInTimePDP(lines 68-72) to fetch all registered resources using a new helper method and pass them to theNewPolicyDecisionPointconstructor. - Updated the
GetDecisionmethod (lines 103-113) to handle theRegisteredResourceValueFqncase, calling the newpdp.GetDecisionRegisteredResource(currently a placeholder). - Updated the
GetEntitlementsmethod (lines 164-168) to handle theRegisteredResourceValueFqncase, calling the newpdp.GetEntitlementsRegisteredResource. - Added
fetchAllRegisteredResourcesmethod (lines 288-313) to retrieve all registered resources from the policy service with pagination support.
- Imported
- service/internal/access/v2/pdp.go
- Imported
slices(line 8) andidentifier(line 12). - Added
allRegisteredResourceValuesByFQNmap (line 52) to thePolicyDecisionPointstruct to store registered resource values by their FQN. - Modified
NewPolicyDecisionPoint(lines 72-73) to acceptallRegisteredResourcesas an argument. - Added logic in
NewPolicyDecisionPoint(lines 130-143) to validate registered resources and populate theallRegisteredResourceValuesByFQNmap. - Added a placeholder
GetDecisionRegisteredResourcemethod (lines 248-255). - Implemented the
GetEntitlementsRegisteredResourcemethod (lines 330-390) to calculate and return entitlements for a given registered resource value FQN.
- Imported
- service/internal/access/v2/pdp_test.go
- Added
createRegisteredResourceValueFQNhelper function (lines 45-50). - Added variables for test registered resource value FQNs (lines 92-98).
- Added fields for test registered resources and values to the
PDPTestSuitestruct (lines 140-146). - Initialized test registered resources and values in
SetupTest(lines 356-468) and created their FQNs (lines 470-475). - Updated
TestNewPolicyDecisionPoint(lines 498, 523, 530) and other existing tests (Test_GetDecision_MultipleResourcesline 553,Test_GetDecision_PartialActionEntitlementlines 749, 861,Test_GetDecision_CombinedAttributeRules_SingleResourceline 908,Test_GetDecision_AcrossNamespacesline 1268,Test_GetEntitlementsline 1563,Test_GetEntitlements_AdvancedHierarchyline 1862) to pass registered resources (or an empty slice) toNewPolicyDecisionPoint. - Added the
Test_GetEntitlementsRegisteredResourcetest suite (lines 1932-2091) with various test cases for the new entitlement logic.
- Added
- service/internal/access/v2/validators.go
- Added
validateRegisteredResourcefunction (lines 106-113) to validate registered resource objects. - Added
validateRegisteredResourceValuefunction (lines 122-129) to validate registered resource value objects.
- Added
- service/internal/access/v2/validators_test.go
- Added
TestValidateRegisteredResourcetest suite (lines 286-323). - Added
TestValidateRegisteredResourceValuetest suite (lines 325-362).
- Added
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A new resource type,
Entitlements take flight.
Code flows, tests pass,
Through the review glass,
Merging soon, shining bright.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for handling registered resources within the authorization service, particularly for entitlement decisions. The changes to fetch, store, and process these resources in NewJustInTimePDP and NewPolicyDecisionPoint are logical. The new GetEntitlementsRegisteredResource method and its tests are well-implemented.
However, a critical issue is that the GetDecisionRegisteredResource method, which is essential for the GetDecision flow for registered resources, is currently not implemented and returns an error. This needs to be addressed for the feature to be complete as per the PR title.
I've also included a suggestion to enhance validation during PDP initialization for better robustness.
Please address the critical issue and consider the suggestion. Also, it would be beneficial to update the PR checklist regarding documentation and integration tests once the feature is fully implemented.
Summary of Findings
- Missing Core Functionality Implementation: The
GetDecisionRegisteredResourcemethod inpdp.gois not implemented and returns an error, despite being called by theGetDecisionflow. This is critical as it prevents the primary goal of the PR (GetDecision support for registered resources) from functioning. - Early Validation of Registered Resource Values: Registered resource values (
policy.RegisteredResourceValue) are not validated during theNewPolicyDecisionPointinitialization. Adding this validation would improve robustness by ensuring all cached resource values are valid upfront. - PR Checklist Update Needed: The PR description checklist items for unit tests (partially complete), integration tests, and documentation are unchecked. These should be reviewed and updated as the feature is finalized.
Merge Readiness
This pull request has made substantial progress in adding support for registered resources. However, due to the critical issue of the GetDecisionRegisteredResource method not being implemented, I recommend that these changes not be merged until this core functionality is complete and tested. Addressing the medium-severity validation suggestion would also enhance the code's robustness. As a reviewer, I am not authorized to approve pull requests; please ensure further review and approval from authorized maintainers after the necessary changes are made.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
jakedoublev
left a comment
jakedoublev
left a comment
There was a problem hiding this comment.
Have not reviewed tests yet and would prefer to review again after the diff is simplified by merging #2358, but this looks great so far.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for registered resources in GetDecision and GetEntitlements flows. The changes include fetching all registered resources, updating the Policy Decision Point (PDP) to utilize them, and implementing new methods for decisions and entitlements based on registered resource value FQNs. New error types, helper functions, and validators have also been added, along with test coverage for some of the new functionality.
The core logic for integrating registered resources seems well-structured. My main concerns are around the handling of attribute hierarchy within the new GetDecisionRegisteredResource method and the need for dedicated unit tests for this critical function.
Summary of Findings
- Hierarchy Handling in GetDecisionRegisteredResource: A TODO comment in
service/internal/access/v2/pdp.go(line 323) questions whether attribute hierarchy should be populated for entitlements derived fromRegisteredResourceValuewithin theGetDecisionRegisteredResourcemethod. This is critical for consistent authorization behavior, especially sinceGetEntitlementsRegisteredResourcedoes support comprehensive hierarchy. Clarification or implementation is needed. - Missing Unit Tests for GetDecisionRegisteredResource: The new
PolicyDecisionPoint.GetDecisionRegisteredResourcemethod, a key component for registered resource-based decisions, lacks dedicated unit tests. Comprehensive test coverage is needed to ensure its correctness across various scenarios.
Merge Readiness
The pull request introduces valuable functionality for registered resources. However, due to the high severity issues identified—specifically the unresolved question about hierarchy handling in GetDecisionRegisteredResource and the missing unit tests for this core decision logic—I recommend that these points be addressed before merging. As a reviewer, I am not authorized to approve pull requests, but addressing these concerns will significantly improve the robustness and clarity of the new features. The existing tests for GetEntitlementsRegisteredResource are a great start!
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
### Proposed Changes Add cleanup for underlying Ristretto cache by tracking and explicitly closing it to avoid goroutine leaks and ensure proper resource release. Details: Modified cache.Manager to store a reference to the underlying *ristretto.Cache. Added a Close() method to Manager that calls .Close() on the underlying Ristretto cache. Why: Ristretto starts background goroutines and allocates internal buffers that aren't automatically cleaned up. Explicitly calling .Close() ensures proper shutdown, especially important in tests and long-running services ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
### Proposed Changes * enables sloglint * updates settings to prefer `slog.Attr` over arbitrary key/values * lint fixes throughout * removes `request` logs and `token` logs where we were previously logging them ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
…Values on RegisteredResourceValues (#2469) ### Proposed Changes * ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
|
prob needs at least one maintainer approval who didn't touch this PR |
🤖 I have created a release *beep* *boop* --- ## [0.7.0](service/v0.6.0...service/v0.7.0) (2025-06-24) ### ⚠ BREAKING CHANGES * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ### Features * **authz:** Add caching to keycloak ERS ([#2466](#2466)) ([f5b0a06](f5b0a06)) * **authz:** auth svc registered resource GetDecision support ([#2392](#2392)) ([5405674](5405674)) * **authz:** authz v2 GetBulkDecision ([#2448](#2448)) ([0da3363](0da3363)) * **authz:** cache entitlement policy within authorization service ([#2457](#2457)) ([c16361c](c16361c)) * **authz:** ensure logging parity between authz v2 and v1 ([#2443](#2443)) ([ef68586](ef68586)) * **core:** add cache manager ([#2449](#2449)) ([2b062c5](2b062c5)) * **core:** consume RPC interceptor request context metadata in logging ([#2442](#2442)) ([2769c48](2769c48)) * **core:** DSPX-609 - add cli-client to keycloak provisioning ([#2396](#2396)) ([48e7489](48e7489)) * **core:** ERS cache setup, fix cache initialization ([#2458](#2458)) ([d0c6938](d0c6938)) * inject logger and cache manager to key managers ([#2461](#2461)) ([9292162](9292162)) * **kas:** expose provider config from key details. ([#2459](#2459)) ([0e7d39a](0e7d39a)) * **main:** Add Close() method to cache manager ([#2465](#2465)) ([32630d6](32630d6)) * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ([30f8cf5](30f8cf5)) * **policy:** Restrict deletion of pc with used key. ([#2414](#2414)) ([3b40a46](3b40a46)) * **sdk:** allow Connect-Protocol-Version RPC header for cors ([#2437](#2437)) ([4bf241e](4bf241e)) ### Bug Fixes * **core:** remove generics on new platform cache manager and client ([#2456](#2456)) ([98c3c16](98c3c16)) * **core:** replace opentdf-public client with cli-client ([#2422](#2422)) ([fb18525](fb18525)) * **deps:** bump github.com/casbin/casbin/v2 from 2.106.0 to 2.107.0 in /service in the external group ([#2416](#2416)) ([43afd48](43afd48)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to 0.5.0 in /service ([#2470](#2470)) ([3a73fc9](3a73fc9)) * **deps:** bump github.com/opentdf/platform/sdk from 0.4.7 to 0.5.0 in /service ([#2473](#2473)) ([ad37476](ad37476)) * **deps:** bump the external group across 1 directory with 2 updates ([#2450](#2450)) ([9d8d1f1](9d8d1f1)) * **deps:** bump the external group across 1 directory with 2 updates ([#2472](#2472)) ([d45b3c8](d45b3c8)) * only request a token when near expiration ([#2370](#2370)) ([556d95e](556d95e)) * **policy:** fix casing bug and get provider config on update. ([#2403](#2403)) ([a52b8f9](a52b8f9)) * **policy:** properly formatted pem in test fixtures ([#2409](#2409)) ([54ffd23](54ffd23)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Proposed Changes
Checklist
Testing Instructions