fix(policy)!: GetPC does not respect manager field. - #3752
Conversation
github-actions
Bot
added
comp:db
📝 WalkthroughWalkthroughProvider configuration retrieval now supports name-and-manager identifiers. Typed service and database requests, manager-aware CLI flags, deprecated name-only error mapping, dependency updates, and integration/end-to-end coverage were added. ChangesProvider configuration lookup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProviderHandler
participant KeyManagementService
participant PolicyDBClient
CLI->>ProviderHandler: provide id, name, manager
ProviderHandler->>KeyManagementService: send GetProviderConfigRequest
KeyManagementService->>PolicyDBClient: pass typed identifier
PolicyDBClient-->>KeyManagementService: return provider config or error
KeyManagementService-->>CLI: return response
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)service/go.modTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, 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 bug where the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 the 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 counterproductive. 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. The manager field was lost in the fray, / But now it is found in the light of day. / With name and with manager, clearly defined, / The provider config is easy to find. Footnotes
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![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 updates the key provider configuration retrieval process to support and require a manager parameter when fetching by name. This change is implemented across the CLI commands, documentation, E2E tests, handlers, and database queries. The reviewer raised a valid concern that while the CLI enforces the manager parameter when retrieving by name, the API level does not validate this, which could lead to non-deterministic behavior if multiple configurations share the same name but have different managers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
c-r33d
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdfctl/e2e/provider-config.bats`:
- Around line 112-116: In the test “fail to get provider configuration with id,
name,” rename the misspelled assert_ouput command to assert_output and add the
--partial option so the expected validation message matches output containing
CLI prefixes or trailing newlines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b461899-433c-4c72-a549-fa6f5beb0718
📒 Files selected for processing (7)
otdfctl/e2e/provider-config.batsotdfctl/pkg/handlers/provider-config.goservice/integration/keymanagement_test.goservice/pkg/db/errors.goservice/policy/db/key_management.goservice/policy/keymanagement/key_management.goservice/policy/keymanagement/key_management_test.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/integration/keymanagement_test.go (1)
609-620: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise distinct providers when testing manager uniqueness.
Test_CreateProviderConfig_SameNameDifferentManager_Succeedscurrently creates two different provider names with the same manager, then assertspc1.GetName() != pc2.GetName(). Use the same provider name but a distinct manager forpc2so this actually covers the composite(provider_name, manager)uniqueness constraint instead of just provider-name uniqueness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/integration/keymanagement_test.go` around lines 609 - 620, The test setup in Test_CreateProviderConfig_SameNameDifferentManager_Succeeds should reuse the same provider name for pc2 and pass a distinct valid manager instead of basicManager. Update the related assertions to verify equal provider names and different managers, preserving the test’s successful creation behavior for the composite uniqueness constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@service/integration/keymanagement_test.go`:
- Around line 609-620: The test setup in
Test_CreateProviderConfig_SameNameDifferentManager_Succeeds should reuse the
same provider name for pc2 and pass a distinct valid manager instead of
basicManager. Update the related assertions to verify equal provider names and
different managers, preserving the test’s successful creation behavior for the
composite uniqueness constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d681e7b7-c529-438a-b463-745e70326118
⛔ Files ignored due to path filters (1)
service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
otdfctl/e2e/provider-config.batsotdfctl/pkg/handlers/provider-config.goservice/go.modservice/integration/keymanagement_test.goservice/pkg/db/errors.goservice/policy/db/key_management.goservice/policy/keymanagement/key_management_test.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
🤖 I have created a release *beep* *boop* --- ## [0.21.0](opentdf/platform@service/v0.20.0...service/v0.21.0) (2026-07-22) ### ⚠ BREAKING CHANGES * **policy:** GetPC does not respect manager field. ([opentdf#3752](opentdf#3752)) ### Features * **core:** Update fixtures ([opentdf#3766](opentdf#3766)) ([9ac1640](opentdf@9ac1640)) * **policy:** add entitleable namespace field and narrow-read-API doc corrections ([opentdf#3727](opentdf#3727)) ([0fc2f01](opentdf@0fc2f01)) * **policy:** Add name and manager lookup ([opentdf#3753](opentdf#3753)) ([bfbc65d](opentdf@bfbc65d)) * **policy:** Add UnsafeUpdateKey implementation ([opentdf#3731](opentdf#3731)) ([cd47518](opentdf@cd47518)) * **policy:** Add UnsafeUpdateKey rpc. ([opentdf#3728](opentdf#3728)) ([c89b193](opentdf@c89b193)) * **policy:** create subject mappings with attribute values ([opentdf#3741](opentdf#3741)) ([718320b](opentdf@718320b)) * **policy:** deprecate GetAttributeValuesByFqns in favor of narrow read APIs ([opentdf#3745](opentdf#3745)) ([baba709](opentdf@baba709)) * **policy:** dynamic attribute value entitlement mappings ([opentdf#3568](opentdf#3568)) ([21e95e0](opentdf@21e95e0)) * **policy:** populate entitleable definition namespace ([opentdf#3737](opentdf#3737)) ([3f32839](opentdf@3f32839)) ### Bug Fixes * **authz:** use standard Keycloak token exchange ([opentdf#3754](opentdf#3754)) ([de9ae0d](opentdf@de9ae0d)) * **core:** Pass the platform logger to the authz role provider factory ([opentdf#3732](opentdf#3732)) ([9c32554](opentdf@9c32554)) * **core:** prevent OpenTelemetry resource schema conflicts ([opentdf#3769](opentdf#3769)) ([4a0c41c](opentdf@4a0c41c)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.38.0 to 0.39.0 in /service ([opentdf#3764](opentdf#3764)) ([8d430f5](opentdf@8d430f5)) * **ers:** coerce attempted_strategies []string to []interface{} for structpb ([opentdf#3645](opentdf#3645)) ([71d6e90](opentdf@71d6e90)) * **policy:** GetPC does not respect manager field. ([opentdf#3752](opentdf#3752)) ([0d3c0a0](opentdf@0d3c0a0)) --- 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>
🤖 I have created a release *beep* *boop* --- ## [0.36.0](opentdf/platform@otdfctl/v0.35.0...otdfctl/v0.36.0) (2026-08-04) ### ⚠ BREAKING CHANGES * **policy:** GetPC does not respect manager field. ([opentdf#3752](opentdf#3752)) ### Features * **authz:** dynamic value mapping and direct entitlement e2e coverage ([opentdf#3774](opentdf#3774)) ([49c80a0](opentdf@49c80a0)) * **cli:** add an sdkfactory for optional override in otdfctl for extension ([opentdf#3793](opentdf#3793)) ([c2f2afe](opentdf@c2f2afe)) * **cli:** add dynamic value mapping CRUD commands and e2e tests ([opentdf#3768](opentdf#3768)) ([aecd265](opentdf@aecd265)) * **cli:** expose handler hook for injecting SDK options ([opentdf#3755](opentdf#3755)) ([76305b0](opentdf@76305b0)) * **cli:** Unsafe update key cli ([opentdf#3733](opentdf#3733)) ([65d888e](opentdf@65d888e)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/protocol/go from 0.36.0 to 0.39.0 in /otdfctl ([opentdf#3765](opentdf#3765)) ([a3346f8](opentdf@a3346f8)) * **deps:** bump github.com/opentdf/platform/sdk from 0.25.0 to 0.27.0 in /otdfctl ([opentdf#3773](opentdf#3773)) ([490c990](opentdf@490c990)) * **policy:** GetPC does not respect manager field. ([opentdf#3752](opentdf#3752)) ([0d3c0a0](opentdf@0d3c0a0)) --- 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: Elizabeth Healy <35498075+elizabethhealy@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [0.36.0](opentdf/platform@otdfctl/v0.35.0...otdfctl/v0.36.0) (2026-08-04) ### ⚠ BREAKING CHANGES * **policy:** GetPC does not respect manager field. ([opentdf#3752](opentdf#3752)) ### Features * **authz:** dynamic value mapping and direct entitlement e2e coverage ([opentdf#3774](opentdf#3774)) ([49c80a0](opentdf@49c80a0)) * **cli:** add an sdkfactory for optional override in otdfctl for extension ([opentdf#3793](opentdf#3793)) ([c2f2afe](opentdf@c2f2afe)) * **cli:** add dynamic value mapping CRUD commands and e2e tests ([opentdf#3768](opentdf#3768)) ([aecd265](opentdf@aecd265)) * **cli:** expose handler hook for injecting SDK options ([opentdf#3755](opentdf#3755)) ([76305b0](opentdf@76305b0)) * **cli:** Unsafe update key cli ([opentdf#3733](opentdf#3733)) ([65d888e](opentdf@65d888e)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/protocol/go from 0.36.0 to 0.39.0 in /otdfctl ([opentdf#3765](opentdf#3765)) ([a3346f8](opentdf@a3346f8)) * **deps:** bump github.com/opentdf/platform/sdk from 0.25.0 to 0.27.0 in /otdfctl ([opentdf#3773](opentdf#3773)) ([490c990](opentdf@490c990)) * **policy:** GetPC does not respect manager field. ([opentdf#3752](opentdf#3752)) ([0d3c0a0](opentdf@0d3c0a0)) --- 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>
3 participants
Caution
THIS IS A BREAKING CHANGE
1.) Add the new
manager_nameselector and deprecate thenameonly selector.2.) Change otdfctl to match the api changes.
Summary by CodeRabbit
--manager(-m) support to the provider retrieval command.otdfctlman page with--managerdetails and examples.