fix(policy): fix casing bug and get provider config on update. #2403
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
external-contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @c-r33d, 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 to help everyone quickly understand the changes. This PR, authored by @c-r33d, addresses two main points related to provider configurations: fixing a potential casing issue when retrieving configurations by name and ensuring that the UpdateProviderConfig function returns the most up-to-date state of the configuration from the database after an update.
The changes involve modifying the database client logic to handle case-insensitivity for provider names during lookup and updating the UpdateProviderConfig implementation to fetch the record post-update. Corresponding integration tests have been updated and new ones added to cover these specific scenarios.
Highlights
- Case-Insensitive Provider Name Lookup: The
GetProviderConfigfunction in the database client now converts the input provider name to lowercase before querying, ensuring that lookups are case-insensitive. - Fetch Updated Config Post-Update: The
UpdateProviderConfigfunction in the database client no longer constructs the return object from the request parameters. Instead, it now fetches the provider configuration from the database after the update operation completes, guaranteeing the returned object reflects the actual state in the database. - Updated and New Integration Tests: Existing integration tests for
GetProviderConfigandUpdateProviderConfighave been updated to assert against lowercase names and use a test provider name with mixed casing. Two new tests were added to specifically verify updating only the config JSON or only the name, and confirming the returned object contains the correct, updated values.
Changelog
Click here to see the changelog
- service/integration/keymanagement_test.go
- Added
stringsimport for lowercase conversion. - Changed
testProvidervariable to use mixed casing (test-PROVIDER) to better test case-insensitivity. - Added
strings.ToLowerwhen asserting provider names inGetProviderConfigandUpdateProviderConfigtests (lines 127, 201, 216, 239). - Added
Test_UpdateProviderConfig_UpdatesConfigJson_Succeedstest to verify updating only the config JSON (lines 298-317). - Added
Test_UpdateProviderConfig_UpdatesConfigName_Succeedstest to verify updating only the name (lines 319-338).
- Added
- service/policy/db/key_management.go
- Removed unused
encoding/jsonimport (line 5). - Applied
strings.ToLowerto the input name when querying by name inGetProviderConfig(line 57). - Removed unused
mappedMetadatavariable and associatedjson.Unmarshalcall inGetProviderConfig(lines 73-76). - Modified
MarshalUpdateMetadatacall inUpdateProviderConfigto discard the unused returnedmetadatavariable (line 139). - Changed
UpdateProviderConfigto callGetProviderConfigafter the update query to return the actual state from the database (lines 168-170).
- Removed unused
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.
Code flows like a stream,
Fixing bugs, a sweet dream.
Tests confirm the fix,
No more casing tricks.
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 addresses a casing bug in the GetProviderConfig function and ensures that the provider configuration is retrieved after an update. The changes look good overall, with the addition of tests to verify the fixes. However, there are a few areas that could be improved for clarity and consistency.
Summary of Findings
- Test Variable Naming: The
testProvidervariable is being changed to uppercase, which might be confusing for future developers. It would be more clear to keep the original name and add a comment explaining the purpose of the uppercase name. - Unused Variable: The
MarshalUpdateMetadatafunction is called, but the returnedmetadatavariable is not used. This seems like an oversight, as the intention was likely to update the metadata. Consider using the returned metadata variable, or removing it if it's not needed. - Redundant Object Creation: The return value of
GetProviderConfigis already apolicy.KeyProviderConfig, so there is no need to create a new one. This change simplifies the code and reduces redundancy.
Merge Readiness
The pull request is almost ready for merging. Addressing the comments regarding the test variable naming, unused variable, and redundant object creation would improve the code's clarity and maintainability. I am unable to directly approve the pull request, and users should have others review and approve this code before merging. Once these issues are resolved, the pull request should be good to go.
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
previously approved these changes
Jun 6, 2025
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:
Standard Benchmark Metrics Skipped or Failed |
strantalis
approved these changes
Jun 6, 2025
jakedoublev
approved these changes
Jun 6, 2025
|
/backport |
opentdf-automation bot
pushed a commit
that referenced
this pull request
Jun 9, 2025
### Proposed Changes 1.) Update GetProviderConfig to lowercase the name 2.) Get the provider configuration after update. ### 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 a52b8f9)
|
Successfully created backport PR for |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-2403-to-release/protocol/go/v0.4
git worktree add --checkout .worktree/backport-2403-to-release/protocol/go/v0.4 backport-2403-to-release/protocol/go/v0.4
cd .worktree/backport-2403-to-release/protocol/go/v0.4
git reset --hard HEAD^
git cherry-pick -x a52b8f940c8e523d40275310be581e3383411717
git push --force-with-lease |
opentdf-automation bot
added a commit
that referenced
this pull request
Jun 9, 2025
### Proposed Changes 1.) Update GetProviderConfig to lowercase the name 2.) Get the provider configuration after update. ### 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 a52b8f9)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 24, 2025
🤖 I have created a release *beep* *boop* --- ## [0.7.0](service/v0.6.0...service/v0.7.0) (2025-06-24) ### ⚠ BREAKING CHANGES * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ### Features * **authz:** Add caching to keycloak ERS ([#2466](#2466)) ([f5b0a06](f5b0a06)) * **authz:** auth svc registered resource GetDecision support ([#2392](#2392)) ([5405674](5405674)) * **authz:** authz v2 GetBulkDecision ([#2448](#2448)) ([0da3363](0da3363)) * **authz:** cache entitlement policy within authorization service ([#2457](#2457)) ([c16361c](c16361c)) * **authz:** ensure logging parity between authz v2 and v1 ([#2443](#2443)) ([ef68586](ef68586)) * **core:** add cache manager ([#2449](#2449)) ([2b062c5](2b062c5)) * **core:** consume RPC interceptor request context metadata in logging ([#2442](#2442)) ([2769c48](2769c48)) * **core:** DSPX-609 - add cli-client to keycloak provisioning ([#2396](#2396)) ([48e7489](48e7489)) * **core:** ERS cache setup, fix cache initialization ([#2458](#2458)) ([d0c6938](d0c6938)) * inject logger and cache manager to key managers ([#2461](#2461)) ([9292162](9292162)) * **kas:** expose provider config from key details. ([#2459](#2459)) ([0e7d39a](0e7d39a)) * **main:** Add Close() method to cache manager ([#2465](#2465)) ([32630d6](32630d6)) * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ([30f8cf5](30f8cf5)) * **policy:** Restrict deletion of pc with used key. ([#2414](#2414)) ([3b40a46](3b40a46)) * **sdk:** allow Connect-Protocol-Version RPC header for cors ([#2437](#2437)) ([4bf241e](4bf241e)) ### Bug Fixes * **core:** remove generics on new platform cache manager and client ([#2456](#2456)) ([98c3c16](98c3c16)) * **core:** replace opentdf-public client with cli-client ([#2422](#2422)) ([fb18525](fb18525)) * **deps:** bump github.com/casbin/casbin/v2 from 2.106.0 to 2.107.0 in /service in the external group ([#2416](#2416)) ([43afd48](43afd48)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to 0.5.0 in /service ([#2470](#2470)) ([3a73fc9](3a73fc9)) * **deps:** bump github.com/opentdf/platform/sdk from 0.4.7 to 0.5.0 in /service ([#2473](#2473)) ([ad37476](ad37476)) * **deps:** bump the external group across 1 directory with 2 updates ([#2450](#2450)) ([9d8d1f1](9d8d1f1)) * **deps:** bump the external group across 1 directory with 2 updates ([#2472](#2472)) ([d45b3c8](d45b3c8)) * only request a token when near expiration ([#2370](#2370)) ([556d95e](556d95e)) * **policy:** fix casing bug and get provider config on update. ([#2403](#2403)) ([a52b8f9](a52b8f9)) * **policy:** properly formatted pem in test fixtures ([#2409](#2409)) ([54ffd23](54ffd23)) --- 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>
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
1.) Update GetProviderConfig to lowercase the name
2.) Get the provider configuration after update.
Checklist
Testing Instructions