fix(policy): protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc #2377
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
comp:policy
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. This PR focuses on updates to the policy subject mapping service. The primary changes involve removing the gRPC gateway integration for this service and enhancing the validation of action types using protovalidate to ensure action names are correctly specified.
Highlights
- gRPC Gateway Removal: The gRPC gateway integration has been removed for the SubjectMappingService. This includes removing the HTTP annotations from the
.protofile, unregistering the gateway handler in the Go service code, and removing the corresponding HTTP methods documentation. - Protovalidate Action Name Validation: New
protovalidaterules have been added to theCreateSubjectMappingRequestandUpdateSubjectMappingRequestmessages. These rules specifically validate theactionsfield, ensuring that thenamefield within eachpolicy.Actionis not empty. This likely enforces the use of the preferred action naming convention. - Test Updates: Unit tests for the subject mapping requests have been updated to reflect the new
protovalidaterules. Tests now specifically check for validation failures when action names are missing and have been updated to use thenamefield for actions in successful test cases.
Changelog
Click here to see the changelog
- docs/grpc/index.html
- Removed the documentation section detailing HTTP bindings for the
SubjectMappingService.
- Removed the documentation section detailing HTTP bindings for the
- service/policy/subjectmapping/subject_mapping.go
- Removed the registration of the gRPC gateway handler (
GRPCGatewayFunc) for theSubjectMappingService.
- Removed the registration of the gRPC gateway handler (
- service/policy/subjectmapping/subject_mapping.proto
- Removed the import for
google/api/annotations.proto(used for gRPC gateway HTTP annotations) (L6). - Added
protovalidateCEL rules to theactionsfield inCreateSubjectMappingRequestto ensure action names are not empty (L48-L54). - Added
protovalidateCEL rules to theactionsfield inUpdateSubjectMappingRequestto ensure action names are not empty if the field is provided (L87-L91). - Removed all
option (google.api.http)annotations from the RPC methods within theSubjectMappingService(L185, L187, L190, L194, L196, L198, L200, L204, L208, L210, L212, L214).
- Removed the import for
- service/policy/subjectmapping/subject_mapping_test.go
- Renamed
Test_CreateSubjectMappingRequest_PopulatedArray_BadValueID_FailstoTest_CreateSubjectMappingRequest_NoActionNameProvided_Fails(L226). - Updated
Test_CreateSubjectMappingRequest_NoActionNameProvided_Failsto test the new validation rule requiring action names (L227-L240). - Added a new test
Test_CreateSubjectMappingRequest_PopulatedArray_BadValueID_Failsto specifically test invalidAttributeValueIdwith valid actions (L242-L254). - Updated
Test_CreateSubjectMappingRequest_PopulatedArray_Succeedsto use theNamefield for actions (L263). - Updated
Test_CreateSubjectMappingRequest_WithExistingSubjectConditionSetID_Succeedsto use theNamefield for actions (L278). - Added tests to
Test_UpdateSubjectMappingRequest_Succeedsto validate theactionsfield with the new validation rule (L314-L320). - Added a new test
Test_UpdateSubjectMappingRequest_Failswith sub-tests covering various validation failures, including missing action names (L323-L388).
- Renamed
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.
From proto file, lines removed,
No HTTP path is approved.
Validation strict,
Action names picked,
Code cleaner, purpose proved.
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 effectively addresses two main goals: deprecating the old action types by enforcing the use of Action.name through protovalidate CEL rules, and removing the gRPC gateway functionality for the SubjectMappingService. The changes are well-implemented, and the accompanying test updates are thorough.
Key improvements include:
- Clear CEL validation rules for
Action.namein bothCreateSubjectMappingRequestandUpdateSubjectMappingRequest. - Removal of gRPC gateway configurations from the proto definition and Go service registration.
- Updated unit tests to reflect the new validation logic and the shift from
Action.ValuetoAction.Name.
I have one suggestion for improving the precision of an error message assertion in the tests. Overall, good work!
Summary of Findings
- gRPC Gateway Removal: The gRPC gateway functionality for the SubjectMappingService has been successfully removed by deleting HTTP binding options from the .proto file, removing the
google.api.annotations.protoimport, and updating the service registration in Go. The corresponding documentation indocs/grpc/index.htmlhas also been correctly updated. - Action Name Validation: New protovalidate CEL rules have been added to
CreateSubjectMappingRequestandUpdateSubjectMappingRequestto ensure that if actions are provided, theirnamefield is not empty. This enforces the usage ofAction.nameover the deprecatedAction.Value. - Unit Test Enhancements: Unit tests in
subject_mapping_test.gohave been significantly updated and expanded. This includes:
- Adapting existing tests to use
Action.name. - Adding new test cases to specifically cover the new CEL validations for action names, including scenarios for missing names.
- Improved test organization, such as separating tests for invalid
AttributeValueIdfrom tests for invalidActions.
- Test Assertion Precision: In
Test_UpdateSubjectMappingRequest_Fails, the assertion for the "missing action name" case could be more specific. Instead of checking for the generic term "name" in the error, it would be more robust to check for the specific validation message defined in the CEL rule, such as "Action names must not be empty if provided". This was commented on withmediumseverity. - Proto Formatting: Minor formatting changes were made in
subject_mapping.proto(e.g., making validation rules single-line). These changes are stylistic and slightly improve readability. (Not commented due to severity settings)
Merge Readiness
The pull request is in good shape and largely ready for merging. The core changes for removing the gRPC gateway and adding action name validation are well-implemented and tested. There is one medium severity suggestion regarding test assertion precision that should be considered for improvement. Once that is addressed or discussed, this PR should be good to go. As an AI, I am not authorized to approve pull requests; please ensure other reviewers approve this code before merging.
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 |
strantalis
approved these changes
Jun 4, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This was referenced Jun 4, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 5, 2025
🤖 I have created a release *beep* *boop* --- ## [0.4.0](protocol/go/v0.3.6...protocol/go/v0.4.0) (2025-06-05) ### Features * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * **policy:** protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc ([#2377](#2377)) ([54a6de0](54a6de0)) * **policy:** remove gRPC gateway in policy except where needed ([#2382](#2382)) ([1937acb](1937acb)) --- 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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 6, 2025
🤖 I have created a release *beep* *boop* --- ## [0.6.0](service/v0.5.5...service/v0.6.0) (2025-06-06) ### Features * **authz:** DSPX-894 auth svc registered resource GetEntitlement support ([#2358](#2358)) ([a199aa7](a199aa7)) * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **core:** DSPX-608 - Deprecate public_client_id ([#2185](#2185)) ([0f58efa](0f58efa)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** Unique name for the key provider. ([#2391](#2391)) ([bb58b78](bb58b78)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to 0.4.0 in /service ([#2399](#2399)) ([1c6fa75](1c6fa75)) * **deps:** bump the external group across 1 directory with 21 updates ([#2401](#2401)) ([3d0d4d1](3d0d4d1)) * **policy:** move action sub queries to CTE in sm list and match sql ([#2369](#2369)) ([0fd6feb](0fd6feb)) * **policy:** protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc ([#2377](#2377)) ([54a6de0](54a6de0)) * **policy:** remove gRPC gateway in policy except where needed ([#2382](#2382)) ([1937acb](1937acb)) * **policy:** remove support for creation/updation of SubjectMappings with deprecated proto actions ([#2373](#2373)) ([3660200](3660200)) --- 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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 9, 2025
🤖 I have created a release *beep* *boop* --- ## [0.7.0](protocol/go/v0.6.2...protocol/go/v0.7.0) (2025-08-08) ### ⚠ BREAKING CHANGES * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) * **core:** Require go 1.23+ ([#1979](#1979)) ### Features * add ability to retrieve policy resources by id or name ([#1901](#1901)) ([deb4455](deb4455)) * **authz:** authz v2, ers v2 protos and gencode for ABAC with actions & registered resource ([#2124](#2124)) ([ea7992a](ea7992a)) * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **authz:** sensible request limit upper bounds ([#2526](#2526)) ([b3093cc](b3093cc)) * **core:** adds bulk rewrap to sdk and service ([#1835](#1835)) ([11698ae](11698ae)) * **core:** EXPERIMENTAL: EC-wrapped key support ([#1902](#1902)) ([652266f](652266f)) * **core:** Require go 1.23+ ([#1979](#1979)) ([164c922](164c922)) * **core:** v2 ERS with proto updates ([#2210](#2210)) ([a161ef8](a161ef8)) * **policy:** add enhanced standard/custom actions protos ([#2020](#2020)) ([bbac53f](bbac53f)) * **policy:** Add legacy keys. ([#2613](#2613)) ([57370b0](57370b0)) * **policy:** Add list key mappings rpc. ([#2533](#2533)) ([fbc2724](fbc2724)) * **policy:** add obligation protos ([#2579](#2579)) ([50882e1](50882e1)) * **policy:** Add validation to delete keys ([#2576](#2576)) ([cc169d9](cc169d9)) * **policy:** add values to CreateObligationRequest ([#2614](#2614)) ([94535cc](94535cc)) * **policy:** adds new public keys table ([#1836](#1836)) ([cad5048](cad5048)) * **policy:** Allow the deletion of a key. ([#2575](#2575)) ([82b96f0](82b96f0)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Change return type for delete key proto. ([#2566](#2566)) ([c1ae924](c1ae924)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ([30f8cf5](30f8cf5)) * **policy:** DSPX-1018 NDR retrieval by FQN support ([#2131](#2131)) ([0001041](0001041)) * **policy:** DSPX-1057 registered resource action attribute values (protos only) ([#2217](#2217)) ([6375596](6375596)) * **policy:** DSPX-893 NDR define crud protos ([#2056](#2056)) ([55a5c27](55a5c27)) * **policy:** DSPX-902 NDR service crud protos only (1/2) ([#2092](#2092)) ([24b6cb5](24b6cb5)) * **policy:** Finish resource mapping groups ([#2224](#2224)) ([5ff754e](5ff754e)) * **policy:** key management crud ([#2110](#2110)) ([4c3d53d](4c3d53d)) * **policy:** Key management proto ([#2115](#2115)) ([561f853](561f853)) * **policy:** Modify get request to search for keys by kasid with keyid. ([#2147](#2147)) ([780d2e4](780d2e4)) * **policy:** Return KAS Key structure ([#2172](#2172)) ([7f97b99](7f97b99)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** rotate keys rpc ([#2180](#2180)) ([0d00743](0d00743)) * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * add pagination to list public key mappings response ([#1889](#1889)) ([9898fbd](9898fbd)) * **core:** Allow 521 curve to be used ([#2485](#2485)) ([aaf43dc](aaf43dc)) * **core:** Fixes protoJSON parse bug on ec rewrap ([#1943](#1943)) ([9bebfd0](9bebfd0)) * **core:** Update fixtures and flattening in sdk and service ([#1827](#1827)) ([d6d6a7a](d6d6a7a)) * **deps:** bump toolchain in /lib/fixtures and /examples to resolve CVE GO-2025-3563 ([#2061](#2061)) ([9c16843](9c16843)) * **policy:** protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc ([#2377](#2377)) ([54a6de0](54a6de0)) * **policy:** remove gRPC gateway in policy except where needed ([#2382](#2382)) ([1937acb](1937acb)) * **policy:** remove new public keys rpc's ([#1962](#1962)) ([5049bab](5049bab)) * **policy:** remove predefined rules in actions protos ([#2069](#2069)) ([060f059](060f059)) * **policy:** return kas uri on keys for definition, namespace and values ([#2186](#2186)) ([6c55fb8](6c55fb8)) * **sdk:** Fix compatibility between bulk and non-bulk rewrap ([#1914](#1914)) ([74abbb6](74abbb6)) * update key_mode to provide more context ([#2226](#2226)) ([44d0805](44d0805)) --- 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: Krish Suchak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Checklist
Testing Instructions