Skip to content

Conversation

@Dugowitch
Copy link
Contributor

@Dugowitch Dugowitch commented Feb 6, 2026

Secure Coding Practices Checklist GitHub Link

Secure Coding Checklist

  • Input Validation
  • Output Encoding
  • Authentication and Password Management
  • Session Management
  • Access Control
  • Cryptographic Practices
  • Error Handling and Logging
  • Data Protection
  • Communication Security
  • System Configuration
  • Database Security
  • File Management
  • Memory Management
  • General Coding Practices

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:

  • Grant broader UPDATE privileges for the manager role on system_inventory and system_patch tables to fix permission-related failures when updating systems via the system_platform view.

Enhancements:

  • Add reversible migration scripts to manage manager role privileges on system_inventory and system_patch tables and update the schema migration version.
  • Document the temporary nature of the expanded manager privileges in the template systems update controller for future cleanup.

@jira-linking
Copy link

jira-linking bot commented Feb 6, 2026

Referenced Jiras:
https://issues.redhat.com/browse/RHINENG-21214

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 6, 2026

Reviewer's Guide

This 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 tables

erDiagram
    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
Loading

File-Level Changes

Change Details Files
Broaden manager role UPDATE privileges on system_inventory and system_patch as a temporary hotfix.
  • Bump schema_migrations seed value from 144 to 145 to reflect the new migration.
  • Grant manager full UPDATE on system_inventory in addition to the existing column-specific grant.
  • Grant manager full UPDATE on system_patch in addition to the existing column-specific grant.
database_admin/schema/create_schema.sql
Add reversible migration to grant/revoke the expanded manager privileges.
  • Create 145_update_manager_privileges.up.sql to grant full UPDATE on system_inventory and system_patch to the manager role.
  • Create 145_update_manager_privileges.down.sql to revoke full UPDATE and restore the previous column-specific privileges on system_inventory and system_patch.
database_admin/migrations/145_update_manager_privileges.up.sql
database_admin/migrations/145_update_manager_privileges.down.sql
Document the temporary nature of the expanded privileges near the system platform update logic.
  • Add a TODO comment in the template systems update controller explaining that the broader manager privileges are temporary and pointing to migration 145 for the eventual rollback.
manager/controllers/template_systems_update.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Dugowitch Dugowitch marked this pull request as draft February 6, 2026 10:44
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.39%. Comparing base (9783708) to head (ba40da7).

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           
Flag Coverage Δ
unittests 59.39% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Dugowitch Dugowitch marked this pull request as ready for review February 9, 2026 12:34
Copy link

@sourcery-ai sourcery-ai bot left a 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.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.
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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@MichaelMraka MichaelMraka self-assigned this Feb 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants