feat(roles): add full CRUD API with account_owner scoping (EVO-1061)#21
Merged
Conversation
- Extend ResourceActionsConfig with create/update/delete/bulk_update_permissions actions for roles resource - Grant account_owner roles.create/update/delete/bulk_update_permissions (removed from exclusive list) - Implement index/show/create/update/destroy/bulk_update_permissions controller actions - Scope account_owner to type='account' roles only; 403 on user-type role mutations - Add account_user_roles collection route and bulk_update_permissions member route Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideImplements full CRUD and bulk permission update APIs for roles with account-owner scoping, wires them into routing and authorization, and extends the RBAC configuration to expose the new role actions while removing those permissions from the account_owner seed role. Sequence diagram for role show with account_owner scopingsequenceDiagram
actor AccountOwner
participant RolesController as ApiV1RolesController
participant Role
AccountOwner->>RolesController: show(id)
activate RolesController
RolesController->>RolesController: check_authorization
RolesController->>RolesController: authorize_resource!(roles, read)
RolesController->>Role: find(id)
Role-->>RolesController: role
RolesController->>RolesController: enforce_role_scope!
alt [account_owner_only? and role.type != account]
RolesController-->>AccountOwner: 403 Cannot access or modify user-type roles
else [allowed]
RolesController-->>AccountOwner: success_response(role_serializer(role))
end
deactivate RolesController
Sequence diagram for roles bulk_update_permissionssequenceDiagram
actor AccountOwner
participant RolesController as ApiV1RolesController
participant ResourceActionsConfig
participant Role
participant RolePermissionsAction as RolePermissionsActions
AccountOwner->>RolesController: bulk_update_permissions(id, permission_keys)
activate RolesController
RolesController->>RolesController: check_authorization
RolesController->>RolesController: authorize_resource!(roles, bulk_update_permissions)
RolesController->>Role: find(id)
Role-->>RolesController: role
RolesController->>RolesController: enforce_role_scope!
alt [permission_keys is not Array]
RolesController-->>AccountOwner: error_response(VALIDATION_ERROR)
else [permission_keys is Array]
loop each permission_key
RolesController->>ResourceActionsConfig: valid_permission?(permission_key)
ResourceActionsConfig-->>RolesController: true or false
end
alt [any invalid_keys]
RolesController-->>AccountOwner: error_response(VALIDATION_ERROR)
else [all valid]
RolesController->>RolePermissionsAction: transaction destroy_all
RolesController->>RolePermissionsAction: create!(permission_key) * valid_keys
RolesController->>Role: reload
Role-->>RolesController: role
RolesController-->>AccountOwner: success_response(role_serializer(role))
end
end
deactivate RolesController
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
create, the generatedkeyfromnamedoesn’t account for collisions with existing role keys or reserved/system keys; consider validating and surfacing a clearer error if the derived key is already taken or invalid. enforce_role_scope!returns a raw JSON error payload instead of usingerror_response, which is used elsewhere in the controller; aligning on a single error-response helper would keep API responses consistent.- The
scoped_roles/account_owner_only?logic is duplicated conceptually in multiple places (e.g.,index,enforce_role_scope!); consider centralizing role-type scoping into a single helper or model scope to reduce drift and make the behavior easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `create`, the generated `key` from `name` doesn’t account for collisions with existing role keys or reserved/system keys; consider validating and surfacing a clearer error if the derived key is already taken or invalid.
- `enforce_role_scope!` returns a raw JSON error payload instead of using `error_response`, which is used elsewhere in the controller; aligning on a single error-response helper would keep API responses consistent.
- The `scoped_roles`/`account_owner_only?` logic is duplicated conceptually in multiple places (e.g., `index`, `enforce_role_scope!`); consider centralizing role-type scoping into a single helper or model scope to reduce drift and make the behavior easier to reason about.
## Individual Comments
### Comment 1
<location path="app/controllers/api/v1/roles_controller.rb" line_range="142-148" />
<code_context>
+ current_api_user.has_role?('account_owner') && !current_api_user.has_role?('super_admin')
+ end
+
+ def enforce_role_scope!
+ return unless @role
+ return unless account_owner_only?
+ return if @role.type == 'account'
+
+ render json: { error: 'Cannot access or modify user-type roles' }, status: :forbidden
+ end
+
</code_context>
<issue_to_address>
**suggestion:** Align the forbidden response with the other controller error helpers for consistency.
Other controller actions (like `destroy`/`update`) use `error_response`/`render_unprocessable_entity`, which likely wrap errors in a consistent envelope (`code`, `message`, etc.). This path returns a raw `{ error: ... }` hash, which changes the response shape and may force clients to special-case it. Consider calling something like `error_response('FORBIDDEN', 'Cannot access or modify user-type roles', status: :forbidden)` here to keep error responses consistent.
```suggestion
def enforce_role_scope!
return unless @role
return unless account_owner_only?
return if @role.type == 'account'
error_response('FORBIDDEN', 'Cannot access or modify user-type roles', status: :forbidden)
end
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Prevent privilege escalation: intersect bulk_update_permissions keys with caller's own permissions when account_owner_only? - Add rbac:reseed_account_owner rake task for idempotent re-seed (M1) - Fix N+1: use .size and .map(&:permission_key) in role_helper, role.rb#permissions_by_resource, permission_keys, can_be_deleted? (M2) - Unify forbidden error shape in enforce_role_scope! via error_response (M3) - Remove duplicate private callbacks in role.rb Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ler (EVO-1061) - enforce_role_scope! now blocks all non-super-admin callers on user-type roles instead of only account_owner_only? callers, closing the delegation escalation vector (H1 residual) - bulk_update_permissions intersection guard now applies to every non-super-admin caller, not only account_owner_only? (H1 residual) - create forces type='account' for all non-super-admin callers regardless of role delegation (M4/H1 class) - update blocks name changes on system roles via API, not only key (M3) - create rejects names that produce a blank key with a friendly error (M2) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary
enforce_role_scope!andbulk_update_permissionsnow apply their guards to all non-super-admin callers, not onlyaccount_owner_only?— blocking the cross-tenant escalation vector via delegated roles (H1 residual)type='account'oncreatefor all non-super-admin callers regardless of delegation (M4/H1 class)namechanges on system roles viaupdateAPI, not onlykeychanges (M3)keywith a user-friendly validation error (M2)Validation
ruby -c app/controllers/api/v1/roles_controller.rb→ Syntax OKChanged Files
app/controllers/api/v1/roles_controller.rbRelated PRs
Linked Issue