fix(auth): address round-3 review findings in roles controller (EVO-1061)

[marcelogorutuba]

Summary

• Check both granted and revoked permissions in bulk_update_permissions — caller cannot revoke keys they do not hold (M-R3-1)
• Fix account_user_roles to return account_owner role via Role.where instead of Role.account_type.where (L-R3-1)
• Remove dead :key/:type from role_params permit; simplify create type to explicit 'account' (L-R3-2)
• Fix prevent_system_role_key_modification callback to use will_save_change_to_attribute? — was no-op with saved_change_to_attribute? (L-R3-3)
• Remove dead .except(:key, :type) from update action

Validation

• evo-auth-service-community: ruby -c app/controllers/api/v1/roles_controller.rb → Syntax OK
• evo-auth-service-community: ruby -c app/models/role.rb → Syntax OK

Changed Files

• app/controllers/api/v1/roles_controller.rb
• app/models/role.rb

Related PRs

• Frontend: feat(frontend): add Roles & Permissions admin UI (EVO-1061) evo-ai-frontend-community#55

Linked Issue

• EVO-1061

Summary by Sourcery

Address review feedback on role management by tightening permission checks, simplifying role parameters, and fixing system role protections.

Bug Fixes:

• Ensure bulk permission updates validate both granted and revoked permissions against the caller's own permissions.
• Fix account user role lookup so that the account_owner role is fetched regardless of account-specific scoping.
• Correct the system role key modification guard so it triggers before saving instead of after, preventing key updates on system roles.

Enhancements:

• Simplify role creation to always use the account type for new roles and remove unused key/type parameters from permitted role params.
• Allow role updates to use the full set of permitted attributes now that key/type are no longer permitted.

[sourcery-ai]

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Updates the roles controller and Role model to tighten authorization around bulk permission updates, simplify role creation/update params, correct the account user roles lookup, and ensure system role key changes are properly blocked via the model callback.

Sequence diagram for bulk_update_permissions authorization check

sequenceDiagram
  actor ApiUser
  participant RolesController
  participant Role

  ApiUser->>RolesController: bulk_update_permissions
  RolesController->>Role: permission_keys
  RolesController->>RolesController: compute target_set
  RolesController->>RolesController: compute current_set
  RolesController->>RolesController: compute granted and revoked
  RolesController->>RolesController: compute diffs
  RolesController->>RolesController: compute unauthorized
  alt [unauthorized.any?]
    RolesController-->>ApiUser: error_response FORBIDDEN
  else [no unauthorized]
    RolesController->>Role: update permission_keys
    RolesController-->>ApiUser: success response
  end

Loading

Sequence diagram for system role key modification prevention

sequenceDiagram
  actor ApiUser
  participant RolesController
  participant Role

  ApiUser->>RolesController: update
  RolesController->>Role: update
  Role->>Role: prevent_system_role_key_modification
  alt [system? && will_save_change_to_attribute?(:key)]
    Role->>Role: errors.add key
    Role->>Role: throw abort
    RolesController-->>ApiUser: render_unprocessable_entity
  else [no system key change]
    Role-->>RolesController: update success
    RolesController-->>ApiUser: success response
  end

Loading

File-Level Changes

Change	Details	Files
Tighten authorization in bulk permission updates to verify both granted and revoked permissions are held by the caller.	Compute target, current, granted, and revoked permission sets in bulk_update_permissions Determine diffs as union of granted and revoked keys and reject any not held by caller Update error message to mention both granting and revoking permissions	app/controllers/api/v1/roles_controller.rb
Simplify role params handling and role type assignment for create/update actions.	Force created roles to use type 'account' regardless of request params Remove :key and :type from permitted role_params Update the update action to use role_params directly instead of excluding :key and :type	app/controllers/api/v1/roles_controller.rb
Fix account_user_roles to correctly include the account_owner role.	Replace Role.account_type scope usage with direct Role.where(key: ['agent', 'account_owner']) lookup	app/controllers/api/v1/roles_controller.rb
Ensure system role key modifications are prevented before save rather than after.	Change prevent_system_role_key_modification callback to use will_save_change_to_attribute? instead of saved_change_to_attribute?	app/models/role.rb

---

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

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="app/models/role.rb" line_range="69" />
<code_context>

   def prevent_system_role_key_modification
-    if system? && saved_change_to_attribute?(:key)
+    if system? && will_save_change_to_attribute?(:key)
       errors.add(:key, "cannot be modified for system roles")
       throw :abort
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `will_save_change_to_attribute?` in this callback is correct, but consider if other code paths can still bypass the system-role key protection.

This change correctly makes the guard work in `before_*` callbacks. Please also check whether system-role keys are ever updated via callback-bypassing methods like `update_columns` or `update_all`. If so, this protection won’t apply there; you may need to centralize updates or enforce immutability via a database constraint instead.

Suggested implementation:

```ruby
  end

  def prevent_system_role_key_modification
    if system? && will_save_change_to_attribute?(:key)
      errors.add(:key, "cannot be modified for system roles")
      throw :abort
    end
  end

  # Ensure system roles are immutable at the ActiveRecord layer so that their
  # attributes (including `key`) cannot be changed via callback-bypassing
  # methods like `update_columns`.
  def readonly?
    system? || super
  end

```

To fully implement the review suggestion:
1. Audit the codebase for any usage of `Role.update_all` or raw SQL updates touching system roles; those will still bypass model-level protections.
2. Consider adding a database-level constraint (e.g., a partial index or CHECK constraint) to prevent updates to the `key` column for rows where `system = TRUE`, which will also cover `update_all` and direct SQL.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (bug_risk): Using will_save_change_to_attribute? in this callback is correct, but consider if other code paths can still bypass the system-role key protection.

This change correctly makes the guard work in before_* callbacks. Please also check whether system-role keys are ever updated via callback-bypassing methods like update_columns or update_all. If so, this protection won’t apply there; you may need to centralize updates or enforce immutability via a database constraint instead.

Suggested implementation:

  end

  def prevent_system_role_key_modification
    if system? && will_save_change_to_attribute?(:key)
      errors.add(:key, "cannot be modified for system roles")
      throw :abort
    end
  end

  # Ensure system roles are immutable at the ActiveRecord layer so that their
  # attributes (including `key`) cannot be changed via callback-bypassing
  # methods like `update_columns`.
  def readonly?
    system? || super
  end

To fully implement the review suggestion:

1. Audit the codebase for any usage of Role.update_all or raw SQL updates touching system roles; those will still bypass model-level protections.
2. Consider adding a database-level constraint (e.g., a partial index or CHECK constraint) to prevent updates to the key column for rows where system = TRUE, which will also cover update_all and direct SQL.

[dpaes]

Approved.

Round 3 findings (M-R3-1, L-R3-1, L-R3-2, L-R3-3) all closed correctly. The bulk_update_permissions intersection now covers both granted and revoked keys, account_user_roles returns account_owner, role_params is trimmed to (:name, :description), and prevent_system_role_key_modification uses will_save_change_to_attribute?.

One low-priority cleanup noted (the system-role guard at roles_controller.rb:53 still checks role_params.key?(:key), which is now always false since :key is no longer permitted — safe to drop that leg). Non-blocking; tracked in the Linear card.

Merging with --delete-branch.