-
Notifications
You must be signed in to change notification settings - Fork 43
RHINENG-21214: hotfix manager privileges #2043
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
base: master
Are you sure you want to change the base?
Conversation
|
Referenced Jiras: |
Reviewer's GuideThis PR temporarily broadens the database UPDATE privileges for the manager role on system_inventory and system_patch, wires that into the schema/migration system, and documents the temporary nature of the change near the affected controller logic. Entity relationship diagram for manager role privileges on system tableserDiagram
MANAGER_ROLE {
bool can_select_system_inventory
bool can_update_system_inventory_all_columns
bool can_update_system_inventory_stale_column
bool can_select_system_patch
bool can_update_system_patch_all_columns
bool can_update_system_patch_specific_columns
}
LISTENER_ROLE {
bool can_select_system_inventory
bool can_insert_system_inventory
bool can_update_system_inventory_all_columns
}
VMAAS_SYNC_ROLE {
bool can_select_system_inventory
bool can_update_system_inventory_all_columns
bool can_delete_system_inventory
bool can_select_system_patch
bool can_update_system_patch_all_columns
bool can_delete_system_patch
}
SYSTEM_INVENTORY {
uuid id
bool stale
string other_columns
}
SYSTEM_PATCH {
int installable_advisory_count_cache
int installable_advisory_enh_count_cache
int installable_advisory_bug_count_cache
int installable_advisory_sec_count_cache
int applicable_advisory_count_cache
int applicable_advisory_enh_count_cache
int applicable_advisory_bug_count_cache
int applicable_advisory_sec_count_cache
int template_id
string other_columns
}
MANAGER_ROLE ||--o{ SYSTEM_INVENTORY : manages_privileges_on
MANAGER_ROLE ||--o{ SYSTEM_PATCH : manages_privileges_on
LISTENER_ROLE ||--o{ SYSTEM_INVENTORY : manages_privileges_on
VMAAS_SYNC_ROLE ||--o{ SYSTEM_INVENTORY : manages_privileges_on
VMAAS_SYNC_ROLE ||--o{ SYSTEM_PATCH : manages_privileges_on
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've left some high level feedback:
- Consider adding a brief SQL comment in the 145_up/down migration files explaining that this is a temporary broadening of manager privileges tied to the SystemPlatform removal, so the context isn’t only captured in the Go TODO.
- You may want to wrap the REVOKE/GRANT statements in 145_update_manager_privileges.down.sql in an explicit transaction to avoid any transient period where manager has neither the broad nor column-specific UPDATE privileges if the migration is interrupted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a brief SQL comment in the 145_up/down migration files explaining that this is a temporary broadening of manager privileges tied to the SystemPlatform removal, so the context isn’t only captured in the Go TODO.
- You may want to wrap the REVOKE/GRANT statements in 145_update_manager_privileges.down.sql in an explicit transaction to avoid any transient period where manager has neither the broad nor column-specific UPDATE privileges if the migration is interrupted.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2043 +/- ##
=======================================
Coverage 59.39% 59.39%
=======================================
Files 134 134
Lines 8678 8678
=======================================
Hits 5154 5154
Misses 2977 2977
Partials 547 547
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Hey - I've left some high level feedback:
- In
create_schema.sqlyou now have both a column-specificGRANT ... UPDATE (stale)and a full-tableGRANT ... UPDATEforsystem_inventory(and similarly forsystem_patch); consider replacing or adjusting the existing GRANTs instead of adding overlapping ones to avoid confusion about the effective privileges. - The comment on the new
GRANT SELECT, UPDATE ON system_inventory TO managerline still mentions updating theopt_outcolumn, but the GRANT now covers the entire table, which can be misleading; consider updating the comment to accurately describe the broader, temporary privilege scope.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `create_schema.sql` you now have both a column-specific `GRANT ... UPDATE (stale)` and a full-table `GRANT ... UPDATE` for `system_inventory` (and similarly for `system_patch`); consider replacing or adjusting the existing GRANTs instead of adding overlapping ones to avoid confusion about the effective privileges.
- The comment on the new `GRANT SELECT, UPDATE ON system_inventory TO manager` line still mentions updating the `opt_out` column, but the GRANT now covers the entire table, which can be misleading; consider updating the comment to accurately describe the broader, temporary privilege scope.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Adjust manager database privileges as a temporary hotfix to resolve system update issues while preserving a path to revert to more restrictive access.
Bug Fixes:
Enhancements: