-
Notifications
You must be signed in to change notification settings - Fork 24
feat(policy): Allow for additional context to be added to obligation triggers #2705
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
Conversation
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.
Summary of Changes
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!
This pull request introduces a significant enhancement to the policy system by enabling the association of Registered Policy Enforcement Points (PEPs) directly with obligation triggers. This change allows for more granular and context-aware policy enforcement by ensuring that specific PEPs can be linked to the conditions that activate certain obligations. The update involves both database schema modifications and corresponding API definition changes to support this new capability.
Highlights
- Database Schema Update: A new
registered_peps
column has been added to theobligation_triggers
table in the database. This column is of type JSONB and is intended to store a map ofclient_id
toRegisteredPEP
objects, linking Policy Enforcement Points (PEPs) to obligation triggers. - Protocol Buffer Definitions: New Protocol Buffer messages and fields have been introduced to support the integration of Registered PEPs. This includes a new
RegisteredPEP
message and the addition of apeps
map field to theObligationTrigger
,AddObligationTriggerRequest
, andValueTriggerRequest
messages. Thepeps
field in requests is marked as required. - Documentation Updates: The gRPC documentation (
docs/grpc/index.html
) has been updated to reflect the newRegisteredPEP
message and thepeps
map withinObligationTrigger
and related request messages, ensuring the documentation aligns with the API changes.
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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.
A policy, a rule, a guide, With triggers set, where truths reside. Now PEPs join the flow, so grand, Enforcing access, hand in hand.
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.
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 adds a registered_peps
column to the obligation_triggers
table and updates the corresponding protobuf messages to associate registered PEPs with obligation triggers. While the database migration and protobuf definitions are in place, the implementation seems incomplete. The application code that inserts into the obligation_triggers
table (likely in service/policy/db/queries/obligations.sql
and service/policy/db/obligations.go
) has not been updated to handle the new registered_peps
field. Since the new column is NOT NULL
, any attempt to create an obligation trigger will fail. This is a critical omission that needs to be addressed. Additionally, there are a few issues to address: the SQL migration for adding the NOT NULL
column will fail on a non-empty table, there is some scope creep in the ERD file, and the generated documentation is inconsistent with the protobuf definitions. Please address these points to ensure the feature is fully functional and the codebase remains consistent.
service/policy/db/migrations/20250909000000_add_registered_peps_to_obligations_triggers.sql
Outdated
Show resolved
Hide resolved
service/policy/db/migrations/20250909000000_add_registered_peps_to_obligations_triggers.md
Outdated
Show resolved
Hide resolved
service/policy/db/migrations/20250909000000_add_registered_peps_to_obligations_triggers.sql
Outdated
Show resolved
Hide resolved
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:
|
service/policy/db/migrations/20250909000000_add_registered_peps_to_obligations_triggers.sql
Outdated
Show resolved
Hide resolved
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 a client_id
to obligation triggers, effectively linking them to Policy Enforcement Points (PEPs). The changes are comprehensive, spanning from database schema modifications and proto updates to adjustments in the data access layer and tests. Overall, the implementation is solid, but there are a few critical issues to address, primarily a potential panic due to a nil pointer dereference and a typo in a public-facing proto message name. I've also included some suggestions to improve maintainability and clarity.
service/policy/db/migrations/20250909000000_add_registered_peps_to_obligations_triggers.md
Outdated
Show resolved
Hide resolved
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:
|
🤖 I have created a release *beep* *boop* --- ## [0.9.0](protocol/go/v0.8.0...protocol/go/v0.9.0) (2025-09-11) ### Features * **policy:** add FQN of obligation definitions/values to protos ([#2703](#2703)) ([45ded0e](45ded0e)) * **policy:** Add obligation triggers ([#2675](#2675)) ([22d0837](22d0837)) * **policy:** Allow creation and update of triggers on Obligation Values ([#2691](#2691)) ([b1e7ba1](b1e7ba1)) * **policy:** Allow for additional context to be added to obligation triggers ([#2705](#2705)) ([7025599](7025599)) * **policy:** obligations + values CRUD ([#2545](#2545)) ([c194e35](c194e35)) ### Bug Fixes * **deps:** update protovalidate to v0.14.2 to use new buf validate MessageOneofRule ([#2698](#2698)) ([1cae18e](1cae18e)) --- 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.10.0](service/v0.9.0...service/v0.10.0) (2025-09-17) ### ⚠ BREAKING CHANGES * **policy:** Add manager column to provider configuration for multi-instance support ([#2601](#2601)) ### Features * **authz:** add obligation policy decision point ([#2706](#2706)) ([bb2a4f8](bb2a4f8)) * **core:** add service negation for op mode ([#2680](#2680)) ([029db8c](029db8c)) * **core:** Bump default write timeout. ([#2671](#2671)) ([6a233c1](6a233c1)) * **core:** Encapsulate>Encrypt ([#2676](#2676)) ([3c5a614](3c5a614)) * **core:** Lets key manager factory take context ([#2715](#2715)) ([8d70993](8d70993)) * **policy:** add FQN of obligation definitions/values to protos ([#2703](#2703)) ([45ded0e](45ded0e)) * **policy:** Add manager column to provider configuration for multi-instance support ([#2601](#2601)) ([a5fc994](a5fc994)) * **policy:** Add obligation triggers ([#2675](#2675)) ([22d0837](22d0837)) * **policy:** add protovalidate for obligation defs + vals ([#2699](#2699)) ([af5c049](af5c049)) * **policy:** Allow creation and update of triggers on Obligation Values ([#2691](#2691)) ([b1e7ba1](b1e7ba1)) * **policy:** Allow for additional context to be added to obligation triggers ([#2705](#2705)) ([7025599](7025599)) * **policy:** Include Triggers in GET/LISTable reqs ([#2704](#2704)) ([b4381d1](b4381d1)) * **policy:** obligations + values CRUD ([#2545](#2545)) ([c194e35](c194e35)) * use public AES protected key from lib/ocrypto ([#2600](#2600)) ([75d7590](75d7590)) ### Bug Fixes * **core:** remove extraneous comment ([#2741](#2741)) ([ada8da6](ada8da6)) * **core:** return services in the order they were registered ([#2733](#2733)) ([1d661db](1d661db)) * **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.3.0 to 0.6.0 in /service ([#2714](#2714)) ([00354b3](00354b3)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.7.0 to 0.9.0 in /service ([#2726](#2726)) ([9004368](9004368)) * **deps:** bump protocol/go to 0.10.0 in service ([#2734](#2734)) ([11e6201](11e6201)) * **deps:** update protovalidate to v0.14.2 to use new buf validate MessageOneofRule ([#2698](#2698)) ([1cae18e](1cae18e)) * **policy:** Registered Resources should consider actions correctly within Decision Requests ([#2681](#2681)) ([cf264a2](cf264a2)) * sanitize db schema identifiers ([#2682](#2682)) ([0d3dd94](0d3dd94)) --- 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>
Proposed Changes
1.) Add
client_id
column toobligation_triggers
table.2.) Modify ObligationTrigger protos to take in an optional RequestContext where the
client_Id
is stored within the triggerChecklist
Testing Instructions