AAP-78640 Block role assignments where you do not have assigned permissions - #1039
Open
AlanCoding wants to merge 11 commits into
Open
AAP-78640 Block role assignments where you do not have assigned permissions#1039AlanCoding wants to merge 11 commits into
AlanCoding wants to merge 11 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds configurable role-assignment permission gates with a default ChangesRBAC delegation controls
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains; the change is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant AssignmentAPI
participant BaseAssignmentSerializer
participant RBACPolicies
participant RoleDefinition
AssignmentAPI->>BaseAssignmentSerializer: create role assignment
BaseAssignmentSerializer->>RBACPolicies: check_content_obj_permission(role_definition)
RBACPolicies->>RoleDefinition: validate delegated permissions
RoleDefinition-->>RBACPolicies: return role permissions
RBACPolicies-->>AssignmentAPI: allow or deny assignment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/apps/rbac/for_app_developers.md (1)
309-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
Line 309 triggers markdownlint MD040 (fenced-code-language). Add
pythonafter the opening backticks.+```python
ANSIBLE_BASE_MANAGE_PERMISSION_ACTION = 'change' # default
ANSIBLE_BASE_MANAGE_PERMISSION_ACTION = 'administrate' # dedicated permission
ANSIBLE_BASE_MANAGE_PERMISSION_ACTION = None # require ALL permissions🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/apps/rbac/for_app_developers.md` around lines 309 - 313, The fenced code block in the ANSIBLE_BASE_MANAGE_PERMISSION_ACTION example is missing a language tag, triggering markdownlint MD040. Update the opening fence in this documentation snippet to use a Python specifier so the block is labeled consistently with its Python-like content.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ansible_base/lib/dynamic_config/settings_logic.py`:
- Around line 252-256: The manage-permission default in settings_logic is
treating an explicit None as if it were unset, which overwrites the stricter
mode with 'change'. Update the logic around the
ANSIBLE_BASE_MANAGE_PERMISSION_ACTION handling so the defaults path preserves an
explicit None while still falling back to 'change' only when the setting is
truly missing. Use the existing manage-permission initialization flow in
settings_logic to distinguish “unset” from “set to None” before applying any
fallback.
In `@ansible_base/rbac/policies.py`:
- Around line 124-128: The manage-permission check in the permission loop is
using a prefix match on permission.codenames, which can incorrectly treat
broader names like administrate_<model> as matching a configured action such as
admin. Update the logic in the same block that iterates over permissions to
require an exact match against the remote manage codename, consistent with
_model_has_permission_action, and keep the fallback behavior for remote objects
unchanged.
---
Nitpick comments:
In `@docs/apps/rbac/for_app_developers.md`:
- Around line 309-313: The fenced code block in the
ANSIBLE_BASE_MANAGE_PERMISSION_ACTION example is missing a language tag,
triggering markdownlint MD040. Update the opening fence in this documentation
snippet to use a Python specifier so the block is labeled consistently with its
Python-like content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8659624d-be56-4413-bdc9-4619d4cd269d
📒 Files selected for processing (7)
ansible_base/lib/dynamic_config/settings_logic.pyansible_base/rbac/api/serializers.pyansible_base/rbac/policies.pydocs/apps/rbac/for_app_developers.mdtest_app/migrations/0019_resourcewithadminperm.pytest_app/models.pytest_app/tests/rbac/api/test_assignment_permissions.py
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## devel #1039 +/- ##
==========================================
+ Coverage 94.77% 94.81% +0.03%
==========================================
Files 259 259
Lines 14485 14519 +34
Branches 2218 2227 +9
==========================================
+ Hits 13728 13766 +38
+ Misses 757 753 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
AlanCoding
changed the title
AlanCoding
force-pushed
the
firewalled_actions
branch
from
June 29, 2026 18:55
03a28af to
dd10058
Compare
|
Member
Author
|
Current values of Nothing in eda-server. So expected followup change in AWX (among other things, possibly migrations) would be removing that or shortening the list. |
15 tasks
AlanCoding
force-pushed
the
firewalled_actions
branch
from
August 18, 2026 15:26
d5cb567 to
713604e
Compare
AlanCoding
changed the title
When assigning a role to an object, the user must now have every permission contained in that role, not just the management gate permission (e.g. "change"). This prevents a user from granting permissions they do not themselves hold. The check_manage_permission_completeness validator is removed — it forced roles containing the manage action to include ALL other permissions, which was a blunt proxy for escalation prevention. The new per-assignment check makes it unnecessary and allows useful partial roles like "editor" (change + view, no delete). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ANSIBLE_BASE_CREATOR_DEFAULTS and ANSIBLE_BASE_MANAGE_PERMISSION_ACTION together define the full delegation chain: creator defaults set what permissions a creator starts with, and the escalation check constrains what they can delegate onward. This is the mechanism for separating "provisioner" users from "operator" users. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…faults The 'use' permission is relational — it gates whether a resource can reference another (e.g. attaching a credential to an inventory). Without it in creator defaults, a user who creates a resource can't wire it into their own scaffolding without a separate grant, breaking the separation between setup and action workflows. Add 'bop' and 'twist' custom action permissions to ResourceWithAdminPerm to enable targeted escalation testing. New TestEscalationWithCustomActions covers: selective delegation (has bop but not twist), full delegation (has both), blocked delegation (has neither), CRUD-only delegation, gate-less user rejection, and a full creator lifecycle test proving creators get CRUD but not custom action permissions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The _check_role_permissions escalation check used has_obj_perm per permission, which fails for child-model permissions (e.g. view_team in an org-scoped role) because RoleEvaluation entries for those live on child objects, not the parent. When ANSIBLE_BASE_CACHE_PARENT_PERMISSIONS is True, eval entries exist on the parent so has_obj_perm works. When False (the default), fall back to comparing role definition permissions directly — query which RoleDefinitions the user holds on the object (directly or via team membership) and check the superset. Also check has_super_permission first so superusers and users with global roles that include the needed permissions are handled correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… team) Use has_obj_perm for same-content-type permissions and only fall back to role definition lookup for cross-content-type cases. This correctly handles permissions inherited from parent objects (org admin assigning team roles) without needing to traverse the parent hierarchy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AlanCoding
force-pushed
the
firewalled_actions
branch
from
August 20, 2026 16:19
ba5f41b to
fe1c949
Compare
- Add type hints to all new helper methods in policies.py - Move `from django.db.models import Q` to top-level imports - Rename _check_role_permissions to _check_assignment_permissions_non_cached to clarify it's an assignment-specific escalation check using the non-cached path for cross-content-type permissions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The helper functions added for escalation checks were typed as Model, but check_content_obj_permission also receives RemoteObject instances which are not Django Model subclasses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…permission Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
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
1 participant
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
changecould grantadhocorexecutepermissions they don't have.check_manage_permission_completenessvalidator that forced roles containing the manage action to include ALL other permissions. The per-assignment escalation check makes it unnecessary and allows useful partial roles like "editor" (change + view only).ANSIBLE_BASE_CREATOR_DEFAULTSandANSIBLE_BASE_MANAGE_PERMISSION_ACTION— together these settings define the full delegation chain, enabling separation of "provisioner" users (CRUD only) from "operator" users (action permissions).Details
The escalation problem
Previously,
check_content_obj_permissiononly checked a single gate: does the user have the manage permission (defaultchange) on the object? If yes, they could assign any role, even one containing permissions they don't hold. For example, a user with onlychange_inventorycould assign a role containingadhoc_inventoryto themselves or others.The fix
check_content_obj_permissionnow accepts an optionalrole_definitionparameter. When the manage-action gate passes, a new_check_role_permissionsfunction iterates every permission in the role being assigned and verifies the user has each one. If any is missing, the assignment is denied with a message naming the missing permission.Two cases
ANSIBLE_BASE_MANAGE_PERMISSION_ACTIONset (e.g.'change')None/ model lacks the actionCreator defaults interaction
ANSIBLE_BASE_CREATOR_DEFAULTS(default['add', 'change', 'delete', 'view']) controls what permissions a creator receives. Action permissions likeexecuteoradhocare deliberately excluded, so creators cannot delegate them. An administrator with those permissions must grant them explicitly.Linked: AAPRFE-2838
Test plan
change) still passANSIBLE_BASE_MANAGE_PERMISSION_ACTION=Noneand='administrate'cases covered🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation