-
Notifications
You must be signed in to change notification settings - Fork 24
feat(sdk): Add obligations support. #2759
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
base: main
Are you sure you want to change the base?
Conversation
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:
|
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:
|
/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 significant and valuable enhancements for handling obligations within the SDK. The refactoring of the bulk decryption process and the introduction of NanoTDFReader
are excellent architectural improvements that increase the flexibility and usability of the library.
I've identified a few areas for improvement, including a critical issue related to variable shadowing, a high-severity issue concerning context propagation, and some medium-severity suggestions to enhance efficiency and maintainability. Addressing these points will further solidify the quality of this new feature.
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:
|
Caution
DO NOT MERGE
Need Authorization changes from service to be merged first
Proposed Changes
1.) Expose
Obligations
function to TDF/NanoTDF2.) Expose new
NanoTDFReader
withLoadNanoTDF
,Init
functions3.) Within
Obligations
function allow consumers to retrieve Obligations on-demand by calling AuthService4.) Refactor
bulk
to allow consumers to retrieve Obligations before decrypting content5.) Expose
PrepareBulkDecrypt
to retrieve Obligations before decrypting the contentManual E2E tests:
Caveats
Important
It's possible when making multiple reqs to different KAS's for the same policy,
if the last policy result for a specific KAS fails with a 500, the previous
obligations are cleared from the reader object.
This should be ok, since we allow customers to retrieve obligations on demand.
For bulk decryption, we do not allow on-demand obligation resolution.
Note
Bulk decrypt is designed in a way that will only allow
a global set of fulfillable obligations to be applied, not for
an individual TDF.
Important
We can not retrieve on-demand Obligations for NanoTDFs
where the policy is not plaintext.
Checklist
Testing Instructions