feat(sdk)!: add base key and support for key grants in ZTDF #271
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mkleene, 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 undertakes a minor refactor to enhance the modularity, encapsulation, and overall clarity of the codebase, primarily focusing on the Autoconfigure and TDF classes. The changes aim to improve internal structure and maintainability without altering external behavior, by centralizing key logic and refining class/method access.
Highlights
- Encapsulation Improvements: Restricted the visibility of several internal classes and methods within
Autoconfigure.javafrompublicto package-private orprivate, promoting better encapsulation and clearer API boundaries. - Code Modularity and Reusability: Extracted the logic for generating default key split plans from
TDF.javainto a new, dedicated static helper method,generatePlanFromDefaultKases, withinAutoconfigure.Granter. This centralizes the logic and reduces duplication across the codebase. - Code Cleanup and Simplification: Removed the
AttributeMappinginner class fromAutoconfigure.java, streamlining the codebase. Additionally, simplified thedefaultKasesmethod inTDF.javaand refined map population logic. - Type Safety and Immutability: Introduced
@Nonnullannotations for better nullability guarantees and made several fieldsfinalto enforce immutability where appropriate, enhancing code robustness.
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.
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
The pull request refactors the Autoconfigure and TDF classes, focusing on code structure and encapsulation. Key changes include reducing member visibility, making inner classes static, using final for fields, and extracting logic into new methods. These changes improve the maintainability and clarity of the codebase.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant refactoring by centralizing key split plan generation into a new Planner class, improving modularity and reducing duplication. It also integrates Key IDs (kid) into the key management and caching logic, enhancing key specificity. The changes include appropriate visibility updates for internal classes and services, aligning with good software engineering practices. However, a critical issue was identified in the KASKeyCache's key equality logic, which needs immediate attention.
|
/gemini review |
There was a problem hiding this comment.
Code Review
An excellent refactoring effort that significantly improves the structure and clarity of the SDK. The introduction of the Planner class is a great step towards better separation of concerns, and the reduced visibility of internal classes enhances encapsulation. I've identified a critical compilation issue and a medium-severity issue related to an unused parameter. Addressing these will further solidify the quality of this refactoring.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Planner class, which encapsulates the policy about how to create a split plan. It also adds support for key grants and a base key from a .well-known endpoint. The changes include updates to visibility for better encapsulation and general code cleanup. The review focuses on exception handling and code clarity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant and valuable features, including the Planner class for orchestrating key split plans, support for key grants, and the ability to fetch a base key from a well-known endpoint. The refactoring to centralize planning logic and improve encapsulation of internal classes is a great step towards better maintainability. I've found one critical issue in the grant resolution logic that needs to be addressed. Otherwise, the changes are well-implemented and the addition of new tests is commendable.
mkleene
changed the title
There was a problem hiding this comment.
One thing I see that is different from the golang SDK is when we go to create the key access object. Currently it looks like this is still relying on the algorithm passed in from the seeded options, which it should instead use the algorithm passed back from the grant/key mappings.
|
Introduces the
Plannerclass, which encapsulates the policy about how to create a split plan, basing itsdecision on
TDFConfig.autoconfigure, whether KASes were provided in config, and whether a split planwas manually provided in config.
Modifies the
Autoconfigure.Granterclass to consume key grants. When a key is specified using a KIDthe KID is used during key assignment. When there are no grants or mapped keys present
Autoconfigure.Granterreaches out to the
wellknownendpoint to download a base key and constructs a single element split plan.WellknownServiceClientInterfacein theSDK.Servicesinterface. Not sure if this is a good ideaor not but it doesn't seem dangerous.