Skip to content

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

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.

Sign up for GitHub

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

Merged
dpaes merged 1 commit into from
May 14, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions app/controllers/api/v1/roles_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def create
name: role_params[:name],
description: role_params[:description],
system: false,
type: current_api_user.has_role?('super_admin') ? (role_params[:type].presence || 'account') : 'account'
type: 'account'
)

unless role.save
Expand All @@ -53,7 +53,7 @@ def update
return error_response('FORBIDDEN', 'Cannot modify key or name of a system role', status: :forbidden)
end

unless @role.update(role_params.except(:key, :type))
unless @role.update(role_params)
return render_unprocessable_entity(@role.errors)
end

Expand Down Expand Up @@ -92,12 +92,19 @@ def bulk_update_permissions
end

unless current_api_user.has_role?('super_admin')
caller_permissions = Set.new(current_api_user.all_permissions)
unauthorized_keys = valid_keys.reject { |k| caller_permissions.include?(k) }
if unauthorized_keys.any?
caller_perms = Set.new(current_api_user.all_permissions)
target_set = valid_keys.to_set
current_set = @role.permission_keys.to_set

granted = target_set - current_set
revoked = current_set - target_set
diffs = granted | revoked

unauthorized = diffs.reject { |k| caller_perms.include?(k) }
if unauthorized.any?
return error_response(
'FORBIDDEN',
"Cannot grant permissions you do not hold: #{unauthorized_keys.join(', ')}",
"Cannot grant or revoke permissions you do not hold: #{unauthorized.join(', ')}",
status: :forbidden
)
end
Expand All @@ -116,7 +123,7 @@ def bulk_update_permissions

# Get available roles for account users (agent and account_owner)
def account_user_roles
roles = Role.account_type.where(key: ['agent', 'account_owner']).map do |role|
roles = Role.where(key: ['agent', 'account_owner']).map do |role|
RoleSerializer.basic(role)
end

Expand All @@ -139,7 +146,7 @@ def full
private

def role_params
params.permit(:name, :description, :key, :type)
params.permit(:name, :description)
end

def load_role
Expand Down
2 changes: 1 addition & 1 deletion app/models/role.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def prevent_system_role_deletion
end

def prevent_system_role_key_modification
if system? && saved_change_to_attribute?(:key)
if system? && will_save_change_to_attribute?(:key)
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

errors.add(:key, "cannot be modified for system roles")
throw :abort
end
Expand Down