-
Notifications
You must be signed in to change notification settings - Fork 24
feat(authz): logic for authz v2 (actions within ABAC decisioning) #2146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Benchmark results, click to expandBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBulk 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 |
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 |
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 |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the core logic for the v2 authorization service, including new gRPC endpoints (GetDecision
, GetDecisionMultiResource
, GetEntitlements
) and helper functions for processing decisions. It also refactors the KAS access PDP to consume these new v2 authorization services. Additionally, a new benchmark tool for GetDecisionMultiResource
and unit tests for decision roll-up logic have been added. The changes are well-structured and the new tests are a welcome addition.
I've identified a critical issue in the new benchmark tool that needs addressing, and a medium-severity suggestion for error handling consistency.
Summary of Findings
- Benchmark Resource Creation: The
benchmark-decision-v2.go
tool reuses the same resource object pointer in a loop, leading to all benchmarked resources being identical except for the lastEphemeralId
set. This needs to be fixed by creating new resource instances within the loop. - Error Handling Consistency: In
service/authorization/v2/authorization.go
, an error from an internal helper function (rollupMultiResourceDecision
) is returned directly. It's recommended to wrap it as aconnect.Error
withconnect.CodeInternal
for consistency with other error returns in the service. - Code Quality and New Features: The PR successfully implements the v2 authorization logic and integrates it into KAS. The addition of unit tests for helper functions and a new benchmark tool are positive contributions.
Merge Readiness
This pull request introduces significant and valuable functionality for v2 authorization. However, there is a critical issue in the benchmark-decision-v2.go
tool that affects its correctness. Additionally, a medium-severity issue regarding error handling consistency has been noted.
I recommend that these issues, particularly the critical one, be addressed before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from team members.
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 |
🤖 I have created a release *beep* *boop* --- ## [0.5.4](service/v0.5.3...service/v0.5.4) (2025-05-29) ### Features * **authz:** access pdp v2 with actions ([#2264](#2264)) ([7afefb7](7afefb7)) * **authz:** logic for authz v2 (actions within ABAC decisioning) ([#2146](#2146)) ([0fdc259](0fdc259)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) ### Bug Fixes * **policy:** DSPX-1151 update of registered resource value always clears existing action attribute values ([#2325](#2325)) ([ca94425](ca94425)) * **policy:** Ensure non active keys cannot be assigned. ([#2321](#2321)) ([207d10d](207d10d)) --- 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> Co-authored-by: Elizabeth Healy <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [0.5.4](opentdf/platform@service/v0.5.3...service/v0.5.4) (2025-05-29) ### Features * **authz:** access pdp v2 with actions ([opentdf#2264](opentdf#2264)) ([7afefb7](opentdf@7afefb7)) * **authz:** logic for authz v2 (actions within ABAC decisioning) ([opentdf#2146](opentdf#2146)) ([0fdc259](opentdf@0fdc259)) * **policy:** Default Platform Keys ([opentdf#2254](opentdf#2254)) ([d7447fe](opentdf@d7447fe)) * **policy:** Update key status's and UpdateKey rpc. ([opentdf#2315](opentdf#2315)) ([7908db9](opentdf@7908db9)) ### Bug Fixes * **policy:** DSPX-1151 update of registered resource value always clears existing action attribute values ([opentdf#2325](opentdf#2325)) ([ca94425](opentdf@ca94425)) * **policy:** Ensure non active keys cannot be assigned. ([opentdf#2321](opentdf#2321)) ([207d10d](opentdf@207d10d)) --- 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> Co-authored-by: Elizabeth Healy <[email protected]>
RPC logic in authorization service consuming new access PDP v2
Out of scope:
GetDecisionsBulk