Consolidate validation rules for same-org restrictions#16427
Consolidate validation rules for same-org restrictions#16427AlanCoding wants to merge 4 commits intoansible:develfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughView-layer inline credential-to-target checks were removed and replaced by conditional delegation to model-level validate_role_assignment calls; model validation no longer consults the DAB resource-server fallback for organization checks; tests updated to reflect changed authorization responses. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant View as API View
participant Model as ContentObject (e.g., Credential / ExecEnv)
participant OrgDB as Organization / Permissions DB
Client->>View: POST role assignment (attach)
View->>Model: validate_role_assignment(target, requesting_user)
Model->>OrgDB: check organization membership / view permission
OrgDB-->>Model: membership / permission result
alt allowed
Model-->>View: success
View-->>Client: 204 / success
else denied
Model-->>View: raise ValidationError / deny
View-->>Client: 403 / error detail
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
AlanCoding
requested review from
chrismeyersfsu,
fosterseth and
stevensonmichel
For something that is mainly deleting in the main code path, that's just a messed up metric. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/api/views/__init__.py`:
- Around line 803-807: The code is incorrectly gating validate_role_assignment()
on the presence of the 'disassociate' key which allows payloads like
{"disassociate": false} to bypass attach validation; change the conditional to
check the action is an attach (reuse the same "action == 'attach'" pattern used
in RoleTeamsList) so validate_role_assignment(team, role_definition=None,
requesting_user=request.user) runs only for attach flows; update the same
pattern in the other occurrences that currently check 'disassociate' (the blocks
around the role/content_object calls referenced in the review) and ensure you
still obtain team via get_object_or_404(models.Team, pk=self.kwargs['pk']) and
call content_object.validate_role_assignment with the same arguments.
- Around line 803-807: The model hook calls to validate_role_assignment can
raise django.core.exceptions.ValidationError which isn't handled by the API
layer; wrap each call to content_object.validate_role_assignment (and any
similar delegation spots) in a try/except that catches
django.core.exceptions.ValidationError and re-raises it as
rest_framework.exceptions.ValidationError (converting message_dict/messages
appropriately) so the API returns a 400 instead of a 500; apply this same
wrapper around the validate_role_assignment invocations referenced (the call
inside the view method that fetches Team and content_object and the other
similar delegation sites).
🪄 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: c15505a1-7821-4875-973d-ed9f21506a74
📒 Files selected for processing (5)
awx/api/views/__init__.pyawx/main/models/credential.pyawx/main/models/execution_environments.pyawx/main/tests/functional/api/test_credential.pyawx/main/tests/functional/dab_rbac/test_dab_rbac_api.py
💤 Files with no reviewable changes (1)
- awx/main/models/execution_environments.py
stevensonmichel
left a comment
stevensonmichel
left a comment
There was a problem hiding this comment.
The PR looks great to me. One point of clarification I might have. I see that check_resource_server_for_user_in_organization was removed. Is the purpose of this function handled elsewhere in the newest version (2.7, 2.6, ...) ?
|
Baby YOLO Results — org_rule_updateBuild: containerized 158— FAILURE, 26 failures, 2460 tests ran Cross-org credential behavior changed as expectedCompared to devel reference builds (e.g. 146, 151), the cross-org credential RBAC tests changed form:
So this branch:
Other failures (22) — all pre-existing / flakyThe remaining 22 failures all appear in devel reference builds and are not caused by this branch:
SummaryThe cross-org credential behavior change is confirmed working. The |
| return safe_env | ||
|
|
||
|
|
||
| def check_resource_server_for_user_in_organization(user, organization, requesting_user): |
chrismeyersfsu
left a comment
chrismeyersfsu
left a comment
There was a problem hiding this comment.
I understand this change 👍 Re-use the existing validate_role_assignment that is present on the Credential model.
| post(url=url, data={"user": rando.id, "role_definition": rd.id, "object_id": credential.id}, user=admin_user, expect=201) | ||
|
|
||
| # non-superuser (org_admin) cannot assign cross-org | ||
| resp = post(url=url, data={"user": rando.id, "role_definition": rd.id, "object_id": credential.id}, user=org_admin, expect=400) |
There was a problem hiding this comment.
Consider using a different user than rando, that user has already been given Credential Admin via the superuser.
This is a non-blocking comment, the assertion below checks for the reason the 400 is returned.
3 participants
SUMMARY
This has been causing a lot of test failures and been very difficult to troubleshoot.
I believe that the prior update to these rules, #16106, didn't do what was expected because that was changing the rules for the deprecated endpoints. The new, DAB RBAC, system has rules implemented in another place.
That other place (introduced in #15508) did some very strange backflips that are no longer necessary since the introduction of permission synchronization. So the extra logic there is removed.
Additionally, this shares the logic between the new and old endpoints so that they will give the same behavior. There are 6 endpoints where you might assign these permissions. But only 1 method in the new system for the callback because it is for both user/team. The old system has 4 points of interception in the views because of user vs team in forward vs reverse.
ISSUE TYPE
COMPONENT NAME
Note
Medium Risk
Changes authorization/validation behavior for credential and execution environment role assignments (including cross-organization and private credential cases), which can affect who can be granted access. Risk is mitigated by updated functional tests but should be reviewed carefully for permission regressions.
Overview
Consolidates same-organization role-assignment validation by routing legacy RBAC endpoints (team/user/role attach flows) through each resource’s
validate_role_assignmenthook instead of endpoint-specific credential checks, and skips this validation ondisassociate.Removes resource-server-based membership backflips and updates
Credential.validate_role_assignmentto allow superusers (as the requesting user) to bypass same-org restrictions; similarly drops the resource-server exception inExecutionEnvironment.validate_role_assignment. Functional tests are updated to reflect the new centralized behavior and revised status codes/messages for legacy endpoints and DAB RBAC cross-org assignment.Reviewed by Cursor Bugbot for commit 70121bd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests