fix(authz): obligations should be logged to audit but not returned when not entitled #2847
Conversation
Summary of ChangesHello @jakedoublev, 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 addresses a regression in the authorization system where obligation details were inadvertently exposed to requesters who were not entitled. The core change refactors how obligation information is processed and returned, ensuring that obligations are only included in the final decision response when the requester is fully entitled. Simultaneously, it guarantees that comprehensive obligation data, including for non-entitled resources, is always captured in audit logs for complete transparency and debugging, without leaking sensitive policy details to unauthorized parties. Highlights
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 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 counter productive. 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. 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. Access is granted, If rules are met, then proceed, Audit sees all. Footnotes
|
![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 effectively addresses a regression where obligations were returned to callers even when they were not entitled to a resource, which could lead to information leakage. The changes correctly separate the decision data returned to the caller from the data logged for auditing. The new getResourceDecisionsWithObligations function now properly checks for entitlement before including obligations in the response, while ensuring the audit logs contain the complete information. The related changes in auditDecision are consistent and correct. The overall logic is sound and fixes the reported issue. I have one suggestion to further improve the robustness of the implementation.
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:
|
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:
|
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:
|
This reverts commit 071dc27.
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:
|
This reverts commit 6ac10d7.
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:
|
X-Test Failure Reportopentdf |
…en not entitled (#2847) ### Proposed Changes * Fixes a regression introduced by #2824 where obligations were returned when triggered when the requester was not entitled ### 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 (cherry picked from commit 35da5e3)
|
Successfully created backport PR for |
…en not entitled (#2847) ### Proposed Changes * Fixes a regression introduced by #2824 where obligations were returned when triggered when the requester was not entitled ### 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 (cherry picked from commit 35da5e3)
Proposed Changes
Checklist
Testing Instructions