fix(docs): correct boolean logic for multiple SubjectMappings#3222
fix(docs): correct boolean logic for multiple SubjectMappings#3222eugenioenko wants to merge 1 commit intomainfrom
Conversation
…docs The migration doc incorrectly stated that multiple SubjectMappings for the same AttributeValue are ANDed together. The actual implementation ORs them: if any SubjectMapping evaluates to true, the subject is entitled to the AttributeValue. Updated the doc to match the code behavior in subject_mapping_builtin_actions.go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Eugene Yakhnenko <eugene.yakhnenko@virtru.com>
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 corrects a technical inaccuracy in the documentation regarding how SubjectMappings are evaluated. By aligning the documentation with the actual implementation in the PDP, it ensures that developers and administrators have a correct understanding of the boolean logic governing subject entitlement. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 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. The logic was stated as AND, / But code had a different command. / With OR in the flow, / The docs had to grow, / To help all the users understand. Footnotes
|
github-actions
bot
added
comp:db
📝 WalkthroughWalkthroughThis change updates migration documentation to clarify PDP boolean evaluation semantics. It refines how multiple SubjectMappings, SubjectSets, and ConditionGroups are combined, replacing a global AND relationship with finer-grained logic: OR for SubjectMappings, AND for SubjectSets and ConditionGroups. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@service/policy/db/migrations/20240305000000_add_subject_condition_sets.md`:
- Around line 32-41: The Background paragraph earlier in this migration doc
incorrectly states that multiple SubjectMapping entries are ANDed; update that
paragraph to say multiple SubjectMapping entries are ORed (i.e., any matching
SubjectMapping entitles the subject) and ensure it references SubjectMapping,
SubjectConditionSet, and ConditionGroup consistently with the later section so
the document uniformly describes: SubjectMappings = OR, multiple SubjectSets in
a SubjectConditionSet = AND, and multiple ConditionGroups on a SubjectSet = AND.
🪄 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: f94b2c24-c03c-46c8-86ea-23125600143c
📒 Files selected for processing (1)
service/policy/db/migrations/20240305000000_add_subject_condition_sets.md
| A PDP evaluates subject mappings with the following boolean logic: | ||
| - more than one SubjectMapping for an Attribute Value: OR (any matching mapping entitles the subject) | ||
| - more than one SubjectSet in the JSONB of a SubjectConditionSet: AND | ||
| - more than one ConditionGroup on a SubjectSet: AND | ||
|
|
||
| ConditionGroups are the only place a policy platform administrator has direct control over the boolean | ||
| operator to associate Conditions via AND or OR. If an admin needs OR logic across SubjectSets | ||
| in a SubjectConditionSet, that can be accomplished with multiple SubjectMappings for the same | ||
| AttributeValue (since SubjectMappings are ORed), or with multiple AttributeValues and an | ||
| AttributeDefinition rule of ANY_OF associating them together. |
There was a problem hiding this comment.
Resolve conflicting boolean semantics elsewhere in this same doc
This section is now correct, but the Background still says multiple SubjectMappings are ANDed (Line 8 and Line 9), which directly contradicts this update. Please update that earlier paragraph too, otherwise the migration doc remains internally inconsistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/policy/db/migrations/20240305000000_add_subject_condition_sets.md`
around lines 32 - 41, The Background paragraph earlier in this migration doc
incorrectly states that multiple SubjectMapping entries are ANDed; update that
paragraph to say multiple SubjectMapping entries are ORed (i.e., any matching
SubjectMapping entitles the subject) and ensure it references SubjectMapping,
SubjectConditionSet, and ConditionGroup consistently with the later section so
the document uniformly describes: SubjectMappings = OR, multiple SubjectSets in
a SubjectConditionSet = AND, and multiple ConditionGroups on a SubjectSet = AND.
![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 documentation for PDP boolean logic in the subject condition sets migration file, clarifying that multiple SubjectMappings for an Attribute Value are evaluated using OR logic. Feedback indicates that the Background section of the same document needs to be updated to reflect this change and maintain internal consistency.
| in a SubjectSet, or multiple SubjectSets in a SubjectMapping, that can be accomplished with multiple | ||
| AttributeValues and an AttributeDefinition rule of ANY_OF associating them together. | ||
| A PDP evaluates subject mappings with the following boolean logic: | ||
| - more than one SubjectMapping for an Attribute Value: OR (any matching mapping entitles the subject) |
There was a problem hiding this comment.
This correction to OR logic is accurate and improves clarity. However, it creates an inconsistency with the Background section of this same document.
Lines 7-9 still state an AND relationship for multiple SubjectMappings:
An `Attribute Value` will relate to one or more `SubjectMapping`s with an
`AND` relationship, and each `SubjectMapping` will relate to one `SubjectConditionSet` to contextualize the
Attribute so each `Attribute Value -> 1+ SubjectConditionSets` considered `AND`ed together.To ensure the documentation is consistent, please consider updating the Background section to also reflect the OR logic for multiple SubjectMappings per Attribute Value.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
X-Test Failure Report✅ go-v0.9.0 |
1 participant
Summary
subject_mapping_builtin_actions.go) ORs them: if any SubjectMapping evaluates to true, the subject is entitled to the AttributeValueTest plan
service/internal/subjectmappingbuiltin/subject_mapping_builtin_actions.go🤖 Generated with Claude Code
Summary by CodeRabbit