fix(station): index EditPermission policies under the permission resource - #649
Open
MRmarioruci wants to merge 1 commit into
Open
fix(station): index EditPermission policies under the permission resource#649MRmarioruci wants to merge 1 commit into
MRmarioruci wants to merge 1 commit into
Conversation
…urce RequestSpecifier::EditPermission(Resource(R)) indexed the policy under R itself rather than under Resource::Permission(Update). The EditPermission operation only ever emits Permission(Update), so the granular form never governed permission changes, and because the resource index is the sole policy selector it was instead returned for any operation whose resource is R. With OR-combined evaluation that turned a rule about who may change a permission into an additional, weaker approval path for the operations on that resource. Index both scopes under Permission(Update), and rebuild the request policy resource index on upgrade so policies already stored under the old keys are refiled. Correcting the mapper alone would leave existing entries pointing at the wrong resources. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to ab053f2. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a correctness/security issue in Station approval-policy selection where EditPermission(Resource(...)) policies were indexed under the target resource instead of the EditPermission operation’s resource key (Permission(Update)), causing them to (a) not govern permission changes and (b) accidentally become an extra approval path for unrelated operations on that resource. Also ensures existing persisted policies are re-indexed on upgrade.
Changes:
- Update
RequestSpecifier::EditPermission(_)resource indexing to always file underResource::Permission(PermissionResourceAction::Update). - Rebuild the request-policy resource index on every upgrade to correct existing stored index entries.
- Add unit/repository tests to assert the indexing invariant and prevent unrelated-operation matches.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/station/impl/src/mappers/request_policy.rs | Fix EditPermission specifier resource mapping and add tests asserting correct indexing/invariant behavior. |
| core/station/impl/src/migration.rs | Add post_run() upgrade hook to rebuild the request policy resource index from stored policies. |
| core/station/impl/src/repositories/request_policy.rs | Add repository-level regression test ensuring EditPermission policies don’t match the target resource bucket. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+65
to
+73
| fn rebuild_request_policy_resource_index() { | ||
| let policies = REQUEST_POLICY_REPOSITORY.list(); | ||
|
|
||
| REQUEST_POLICY_REPOSITORY.clear_indexes(); | ||
|
|
||
| for policy in &policies { | ||
| REQUEST_POLICY_REPOSITORY.add_entry_indexes(policy); | ||
| } | ||
| } |
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
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.
Change
Approval policies are selected through a resource index: a policy is filed under the keys its specifier produces, and at evaluation time policies are looked up by the keys the operation produces. The two sides have to agree.
RequestSpecifier::EditPermission(Resource(R))filed the policy underR, whileRequestOperation::EditPermissiononly ever emitsResource::Permission(Update). Both scopes now index underPermission(Update).migration.rsalso rebuilds the request policy resource index on upgrade. The index is written once when a policy is inserted, so correcting the mapping alone would leave existing policies filed under the old keys. The rebuild is idempotent and O(stored policies).Tests
edit_permission_specifier_indexes_under_the_operation_resource— both scopes map toPermission(Update).edit_permission_specifier_never_matches_an_unrelated_operation— round-trip invariant: every key a specifier indexes under is emitted by its own operation, and none by an unrelated one.edit_permission_policy_is_not_returned_for_the_target_resource— repository level.cargo test -p station --libpasses (397). Clippy warning count unchanged from baseline;cargo fmtclean.Behaviour notes for review
Any.EditPermission(Resource(R))andEditPermission(Any)index identically, so a rule intended for one resource now matches every permission-change request. Deciding the fate of the granular form — implement real two-sided scoping via a dedicated key, or reject it at validation — is left as follow-up.Follow-up
Generalising the round-trip invariant to a table over every
RequestSpecifiervariant would be worthwhile, but it would also flagSetDisasterRecovery/SystemUpgrade/SystemRestore, which deliberately shareSystem(Upgrade)and would need an explicit allowance.