Skip to content

AAP-20982 Remove dead code via ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED setting#16419

Open
AlanCoding wants to merge 4 commits into
ansible:develfrom
AlanCoding:dead_rbac
Open

AAP-20982 Remove dead code via ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED setting#16419
AlanCoding wants to merge 4 commits into
ansible:develfrom
AlanCoding:dead_rbac

Conversation

@AlanCoding

@AlanCoding AlanCoding commented Apr 22, 2026

Copy link
Copy Markdown
Member
SUMMARY

This is a replacement to #16014 which stalled and never made any further progress.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

Note

Medium Risk
Changes touch core RBAC evaluation paths and remove RBAC-related database schema, so regressions could affect authorization results and upgrades if any code still implicitly depended on the ancestor table.

Overview
RBAC is simplified to a single path. All conditional branches guarded by ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED are removed, and access checks now consistently use ansible-base RBAC (RoleEvaluation / has_obj_perm) in the access layer, serializers, and the ResourceAccessList endpoint.

Legacy role-ancestor data structures are deleted. The Role.ancestors M2M, RoleAncestorEntry model, related signals/helpers (batch_role_ancestor_rebuilding, ancestor rebuild hooks), and the ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED default setting are removed, with a new migration (0206_remove_role_ancestors) dropping the field/table.

Behavior is covered by new regression tests. Adds functional tests ensuring access lists and role visibility/membership work without the removed ancestors table, and updates data-generation/inventory import/migration helpers to stop using ancestor-rebuild batching.

Reviewed by Cursor Bugbot for commit a2fb30e. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Refactor
    • Unified RBAC and permission evaluation across the app, removing legacy ancestor-based role logic for consistent access checks.
  • Database Migration
    • Removed legacy role-ancestor schema and related structures.
  • Tests
    • Added functional tests validating RBAC-driven role membership, visibility, and access-list behavior.
  • Chore
    • Simplified import/management flows and adjusted import/command logging for clearer diagnostics.

@coderabbitai

coderabbitai Bot commented Apr 22, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Removes the legacy role-ancestor hierarchy and the ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED gate; permission evaluation now always uses RoleEvaluation (DAB RBAC). Deletes Role.ancestors, RoleAncestorEntry, batch_role_ancestor_rebuilding, related signals, and updates models, API, migrations, commands, serializers, and tests.

Changes

Cohort / File(s) Summary
RBAC model & exports
awx/main/models/rbac.py, awx/main/models/__init__.py
Deleted Role.ancestors M2M and RoleAncestorEntry; removed batch_role_ancestor_rebuilding, TLS batching, ancestor-rebuild hooks, and ancestor-related helpers; updated Role membership/visibility to use RoleEvaluation exclusively; adjusted module exports.
API serialization & views
awx/api/serializers.py, awx/api/generics.py
Removed settings-gated branches; unified access-list computation to always use RoleEvaluation-derived permissive role IDs, include team-derived roles, and produce direct/indirect access deterministically.
Access control & models mixins
awx/main/access.py, awx/main/models/mixins.py
Eliminated ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED conditionals; standardized permission checks to use RoleEvaluation and access_ids_qs/access_qs paths; removed ancestor-table filtering and ContentType-based shortcuts.
Signals cleanup
awx/main/signals.py
Removed signal handlers that synced role ancestor data and superuser-role membership; disconnected registrations related to ancestor maintenance.
Migrations & RBAC helpers
awx/main/migrations/0206_remove_role_ancestors.py, awx/main/migrations/_rbac.py
Added migration to drop Role.ancestors and delete RoleAncestorEntry; removed batch-rebuild context from migration helpers and adjusted local Role lookup in helper functions.
Commands & tooling
awx/main/management/commands/inventory_import.py, tools/data_generators/rbac_dummy_data_generator.py
Removed batch_role_ancestor_rebuilding usage from import and data-gen flows; adjusted transaction/PermissionDenied handling and logging placement in inventory import.
Tests
awx/main/tests/functional/api/test_resource_access_lists.py, awx/main/tests/functional/rbac/test_rbac_role.py
Added tests validating access-list and object-role endpoints across orgs, RoleEvaluation-based containment/visibility, and schema assertion that Role.ancestors no longer exists.
Configuration
awx/settings/defaults.py
Removed the ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED default setting and its comment.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API as "API View / Serializer"
    participant User as "User model"
    participant RBAC as "RoleEvaluation (DB)"
    participant Teams as "Team memberships"
    Client->>API: Request resource access list
    API->>User: load user, is_superuser, memberships
    API->>RBAC: query RoleEvaluation for user's direct role_evals
    API->>Teams: query user's teams -> team role assignments
    Teams->>RBAC: query RoleEvaluation for team role_evals
    RBAC-->>API: return permissive role IDs / evaluations
    API->>API: compute direct_access & indirect_access (include implicit superuser/auditor entries)
    API->>Client: respond with paginated access list
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective: removing dead code made possible by committing to the ANSIBLE_BASE_ROLE_SYSTEM_ACTIVATED feature flag being enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share
Review rate limit: 6/8 reviews remaining, refill in 13 minutes and 53 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 96365f5. Configure here.

Comment thread awx/api/serializers.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
awx/main/migrations/_rbac.py (1)

167-177: ⚠️ Potential issue | 🟠 Major

Don't turn still-referenced historical migration helpers into no-ops.

restore_inventory_admins() and restore_inventory_admins_backward() are actively used by historical migration 0109_v370_job_template_organization_field.py (lines 12–13, 116). Converting _restore_inventory_admins() to pass breaks migration semantics for any database that has not yet applied that migration, removing data-migration behavior that should run.

Either restore the original implementation or update 0109_v370_job_template_organization_field.py to be explicitly independent before stubbing this out.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/migrations/_rbac.py` around lines 167 - 177,
_restore_inventory_admins was turned into a no-op, but restore_inventory_admins
and restore_inventory_admins_backward still call it from historical migration
0109, so restore the original implementation instead of leaving
_restore_inventory_admins as pass (or alternatively update the historical
migration to no longer depend on this helper); specifically, reinstate the
original data-migration logic inside _restore_inventory_admins so
restore_inventory_admins(apps, schema_editor) and
restore_inventory_admins_backward(apps, schema_editor) perform the same
operations they did prior to this change, ensuring the helper handles the
backward=True code path when called by restore_inventory_admins_backward.
awx/main/access.py (2)

704-712: ⚠️ Potential issue | 🟠 Major

Avoid granting orphan-user admin via an empty permission subset.

Line 712 returns True when target_perms is empty, because an empty set subtracts to empty for every actor. Since can_delete() calls this path with allow_orphans=True, orphan users with no evaluated permissions can be treated as administrable without an explicit actor admin context. Please add a non-vacuous guard here or special-case delete semantics before accepting the subset check.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/access.py` around lines 704 - 712, The subset check grants admin
when target_perms is empty; update the orphan handling in
user_is_orphaned/permission branch (the block using allow_orphans, target_perms,
user_perms) to avoid vacuous truth: before returning the subset test, ensure
target_perms is non-empty (e.g., if not target_perms: return False) or
special-case delete semantics in can_delete to require an explicit actor-admin
check; modify the code around the allow_orphans path to short-circuit empty
target_perms so orphan users with no evaluated permissions aren't considered
administrable.

2524-2531: ⚠️ Potential issue | 🟡 Minor

Use evaluated permissions for the generic schedule-add check.

Line 2529 checks direct role permission_partials, while the real add path accepts any executable UnifiedJobTemplate. This can false-negative users whose schedule permission is effective via role evaluation, and it appears to omit workflow job template execution.

Proposed fix
-            return self.user.has_roles.filter(permission_partials__codename__in=['execute_jobtemplate', 'update_project', 'update_inventory']).exists()
+            return RoleEvaluation.objects.filter(
+                role__in=self.user.has_roles.all(),
+                codename__in=[
+                    'execute_jobtemplate',
+                    'execute_workflowjobtemplate',
+                    'update_project',
+                    'update_inventory',
+                ],
+            ).exists()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/access.py` around lines 2524 - 2531, The current can_add
implementation uses a direct roles permission_partials check for the empty-data
schedule-add path which can false-negative evaluated permissions and omits
workflow templates; replace the
has_roles.filter(permission_partials__codename__in=[...]).exists() branch with
the same evaluated permission path used for non-empty data by invoking the
existing check_related logic for the 'unified_job_template' relation (i.e., call
self.check_related('unified_job_template', UnifiedJobTemplate, data,
role_field='execute_role', mandatory=True) or an equivalent
evaluated-permissions helper) after the JobLaunchConfigAccess check so that
permission evaluation (including workflow job templates) is used instead of
direct permission_partials lookups.
🧹 Nitpick comments (1)
awx/main/tests/functional/api/test_resource_access_lists.py (1)

75-82: Assert that the cross-org admin is excluded.

Line 72 creates org2_admin, but the test only checks that org1_admin appears. This would still pass if the access list leaked users from another organization.

Proposed test tightening
-    org1_admin_entry = [r for r in result.data['results'] if r['id'] == org1_admin.id]
-    assert len(org1_admin_entry) == 1
+    result_ids = {r['id'] for r in result.data['results']}
+    assert org1_admin.id in result_ids
+    assert org2_admin.id not in result_ids
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/tests/functional/api/test_resource_access_lists.py` around lines 75
- 82, The test currently only asserts that org1_admin appears in the job
template access list, but does not assert that org2_admin (created on line 72)
is excluded; update the assertions after calling
get(reverse('api:job_template_access_list', kwargs={'pk': jt.id}), org1_admin)
to also verify that no entry exists for org2_admin (e.g., assert len([r for r in
result.data['results'] if r['id'] == org2_admin.id]) == 0) and optionally
tighten by checking the total expected count or exact set of returned ids to
ensure cross-org users are not leaked.
🤖 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/serializers.py`:
- Around line 2791-2814: The code calls get_role_from_object_role() for every
RoleEvaluation.role (from RoleEvaluation and team_obj_roles) but that helper
only supports legacy/compat RoleDefinition names and will ValueError on custom
names (e.g., 'inventory-delete'); update the logic to validate or filter roles
before conversion: either filter the RoleEvaluation queryset to only include
roles whose RoleDefinition.name matches the legacy "model role" pattern (e.g.,
contains a space or otherwise detectable compat format) OR wrap the
get_role_from_object_role(role) call in a small guard that skips/handles
RoleDefinition names that do not match the compat pattern (fallback to treating
them as custom roles, adding them to indirect_access or logging and continuing)
so you never attempt to split an incompatible name; apply this change in both
loops that iterate over RoleEvaluation.objects.filter(...) and reference
get_role_from_object_role, new_role, RoleEvaluation.role, and
RoleDefinition.name.

In `@awx/main/management/commands/inventory_import.py`:
- Around line 1023-1057: Initialize license_fail before the try block to avoid
UnboundLocalError when PermissionDenied is raised during
self.load_into_database() or self.inventory.update_computed_fields(); move or
add a line setting license_fail = False immediately before the with
transaction.atomic() (or at the start of the try) so the except PermissionDenied
handler can safely inspect license_fail and call self.mark_license_failure(...)
or self.mark_org_limits_failure(...) as intended, leaving the rest of the logic
(self.check_license(), self.check_org_host_limit(), and raising the exception)
unchanged.

In `@awx/main/models/rbac.py`:
- Around line 191-192: The current filter_visible_roles implementation builds
raw SQL by embedding
RoleEvaluation.objects.filter(role__in=user.has_roles.all()).values_list('object_id',
'content_type_id').query into roles_qs.extra(where=...), which is unsafe and
deprecated; replace this with a correlated ORM Exists subquery: construct a
RoleEvaluation queryset filtered by role__in=user.has_roles.all() and matching
object_id and content_type_id to the outer role (use OuterRef('object_id') /
OuterRef('content_type_id')), wrap that in Exists(...) and apply
roles_qs.filter(Exists(...)); update the method named filter_visible_roles to
remove .extra(...) and use the Exists-based filter so the query is parameterized
and Ruff S610 compliant.

---

Outside diff comments:
In `@awx/main/access.py`:
- Around line 704-712: The subset check grants admin when target_perms is empty;
update the orphan handling in user_is_orphaned/permission branch (the block
using allow_orphans, target_perms, user_perms) to avoid vacuous truth: before
returning the subset test, ensure target_perms is non-empty (e.g., if not
target_perms: return False) or special-case delete semantics in can_delete to
require an explicit actor-admin check; modify the code around the allow_orphans
path to short-circuit empty target_perms so orphan users with no evaluated
permissions aren't considered administrable.
- Around line 2524-2531: The current can_add implementation uses a direct roles
permission_partials check for the empty-data schedule-add path which can
false-negative evaluated permissions and omits workflow templates; replace the
has_roles.filter(permission_partials__codename__in=[...]).exists() branch with
the same evaluated permission path used for non-empty data by invoking the
existing check_related logic for the 'unified_job_template' relation (i.e., call
self.check_related('unified_job_template', UnifiedJobTemplate, data,
role_field='execute_role', mandatory=True) or an equivalent
evaluated-permissions helper) after the JobLaunchConfigAccess check so that
permission evaluation (including workflow job templates) is used instead of
direct permission_partials lookups.

In `@awx/main/migrations/_rbac.py`:
- Around line 167-177: _restore_inventory_admins was turned into a no-op, but
restore_inventory_admins and restore_inventory_admins_backward still call it
from historical migration 0109, so restore the original implementation instead
of leaving _restore_inventory_admins as pass (or alternatively update the
historical migration to no longer depend on this helper); specifically,
reinstate the original data-migration logic inside _restore_inventory_admins so
restore_inventory_admins(apps, schema_editor) and
restore_inventory_admins_backward(apps, schema_editor) perform the same
operations they did prior to this change, ensuring the helper handles the
backward=True code path when called by restore_inventory_admins_backward.

---

Nitpick comments:
In `@awx/main/tests/functional/api/test_resource_access_lists.py`:
- Around line 75-82: The test currently only asserts that org1_admin appears in
the job template access list, but does not assert that org2_admin (created on
line 72) is excluded; update the assertions after calling
get(reverse('api:job_template_access_list', kwargs={'pk': jt.id}), org1_admin)
to also verify that no entry exists for org2_admin (e.g., assert len([r for r in
result.data['results'] if r['id'] == org2_admin.id]) == 0) and optionally
tighten by checking the total expected count or exact set of returned ids to
ensure cross-org users are not leaked.
🪄 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: Pro Plus

Run ID: 2a103647-d111-4b23-a4b3-5b4ad375654d

📥 Commits

Reviewing files that changed from the base of the PR and between 3fd3b74 and 96365f5.

📒 Files selected for processing (14)
  • awx/api/generics.py
  • awx/api/serializers.py
  • awx/main/access.py
  • awx/main/management/commands/inventory_import.py
  • awx/main/migrations/0206_remove_role_ancestors.py
  • awx/main/migrations/_rbac.py
  • awx/main/models/__init__.py
  • awx/main/models/mixins.py
  • awx/main/models/rbac.py
  • awx/main/signals.py
  • awx/main/tests/functional/api/test_resource_access_lists.py
  • awx/main/tests/functional/rbac/test_rbac_role.py
  • awx/settings/defaults.py
  • tools/data_generators/rbac_dummy_data_generator.py
💤 Files with no reviewable changes (3)
  • awx/main/models/init.py
  • awx/settings/defaults.py
  • awx/main/signals.py

Comment thread awx/api/serializers.py
Comment on lines +2791 to +2814
for evaluation in RoleEvaluation.objects.filter(role__in=user.has_roles.all(), **gfk_kwargs).prefetch_related('role'):
new_role = evaluation.role
if new_role.id in new_roles_seen:
continue
new_roles_seen.add(new_role.id)
old_role = get_role_from_object_role(new_role)
all_permissive_role_ids.add(old_role.id)

if int(new_role.object_id) == obj.id and new_role.content_type_id == content_type.id:
ret['summary_fields']['direct_access'].append(format_role_perm(old_role))
elif new_role.content_type_id == team_content_type.id:
all_team_roles.add(old_role)
else:
ret['summary_fields']['indirect_access'].append(format_role_perm(old_role))

indirect_access_roles = (
user.roles.filter(id__in=all_permissive_role_ids)
.exclude(id__in=direct_permissive_role_ids)
.exclude(id__in=direct_team_roles)
.exclude(id__in=indirect_team_roles)
)
user_teams_qs = permission_registry.team_model.objects.filter(member_roles__in=ObjectRole.objects.filter(users=user))
team_obj_roles = ObjectRole.objects.filter(teams__in=user_teams_qs)
for evaluation in RoleEvaluation.objects.filter(role__in=team_obj_roles, **gfk_kwargs).prefetch_related('role'):
new_role = evaluation.role
if new_role.id in new_roles_seen:
continue
new_roles_seen.add(new_role.id)
old_role = get_role_from_object_role(new_role)
all_permissive_role_ids.add(old_role.id)

@coderabbitai coderabbitai Bot Apr 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify whether custom/non-compat RoleDefinition assignments have access-list coverage.
# Expected: either tests proving non-compat roles are excluded/handled here, or product code preventing them from reaching this serializer.
rg -n -C4 --type=py 'RoleDefinition\.objects\.(create|get_or_create)|RoleDefinition\('
rg -n -C6 --type=py 'ResourceAccessListElementSerializer|get_role_from_object_role|access_list'

Repository: ansible/awx

Length of output: 50367


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the specific lines mentioned in the review
echo "=== Code at lines 2791-2814 in awx/api/serializers.py ==="
sed -n '2791,2814p' awx/api/serializers.py

# Get more context around these lines
echo -e "\n=== Additional context (lines 2780-2820) ==="
sed -n '2780,2820p' awx/api/serializers.py

Repository: ansible/awx

Length of output: 3615


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the complete get_role_from_object_role function
echo "=== get_role_from_object_role function ==="
sed -n '277,310p' awx/main/models/rbac.py

# Search for error handling or try/except around this function in serializers
echo -e "\n=== Searching for error handling around role conversion in serializers ==="
rg -n -B5 -A5 'get_role_from_object_role' awx/api/serializers.py

Repository: ansible/awx

Length of output: 3511


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check what happens when get_role_from_object_role is called with non-compat roles
# Look for tests that explicitly test non-compat/custom roles in access_list context
echo "=== Tests with custom/non-compat RoleDefinition ==="
rg -l 'ResourceAccessListElementSerializer|access_list.*custom|non.*compat.*role' awx/main/tests/ | head -5

# Check for any guards or filters that might prevent non-compat roles from reaching serializer
echo -e "\n=== Checking for role filtering before serialization ==="
rg -n -B10 -A5 'ResourceAccessListElementSerializer' awx/api/serializers.py | head -50

Repository: ansible/awx

Length of output: 106


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check if there's a try-except around the getattr call that could raise AttributeError
echo "=== Full context of get_role_from_object_role ==="
sed -n '277,320p' awx/main/models/rbac.py

# Look for any guards that filter roles BEFORE they reach the serializer
echo -e "\n=== Checking the role_summary_fields_generator function ==="
rg -n -B10 -A30 'def role_summary_fields_generator' awx/main/models/rbac.py | head -80

# Check what roles can be assigned and if custom roles have safeguards
echo -e "\n=== Check if custom RoleDefinitions can be assigned to objects ==="
rg -n 'role_definition' awx/main/models/rbac.py | grep -E '(ObjectRole|assign)' | head -20

Repository: ansible/awx

Length of output: 3615


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for where roles are filtered or validated before serialization
echo "=== Looking for the full method containing this code ==="
sed -n '2750,2830p' awx/api/serializers.py | head -100

# Search for ResourceAccessListElementSerializer and its usage
echo -e "\n=== ResourceAccessListElementSerializer definition ==="
rg -n -B5 -A25 'class ResourceAccessListElementSerializer' awx/api/serializers.py

Repository: ansible/awx

Length of output: 5851


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check one of the test files that creates custom roles
echo "=== test_dab_rbac_api.py with custom roles ==="
sed -n '72,100p' awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py

# Check if access_list tests use these custom roles
echo -e "\n=== Check test_access_list for custom role usage ==="
rg -n 'RoleDefinition\|access_list' awx/main/tests/functional/dab_rbac/test_access_list.py | head -20
</request>
</function_calls>

```shell
#!/bin/bash
set -euo pipefail

# Check if there's a try-except anywhere that handles this function
echo "=== Search for error handling around get_role_from_object_role calls ==="
rg -B5 -A5 'get_role_from_object_role' awx/api/serializers.py | grep -E '(try|except|AttributeError|ValueError)' -A3 -B3

# Check if the function is documented to handle only compat roles
echo -e "\n=== Check docstring and comments ==="
sed -n '277,285p' awx/main/models/rbac.py

# Test what happens with a name that doesn't match any pattern
echo -e "\n=== Simulate role name parsing with single word ==="
python3 << 'EOF'
role_name = "CustomRole"
try:
    model_name, role_name_part = role_name.split()
    print(f"Success: {model_name}, {role_name_part}")
except ValueError as e:
    print(f"Error: {e}")
EOF
</request>
</function_calls>

```shell
#!/bin/bash
set -euo pipefail

# Check if there's validation preventing single-word role names
echo "=== Check RoleDefinition.name field validation ==="
rg -n -B10 -A10 'class RoleDefinition' awx/main/models/__init__.py | head -50

# Look for imports and base model
rg -n 'from ansible_base' awx/main/models/__init__.py | head -10

# Check if name field has validators
rg -n 'validators.*RoleDefinition\|RoleDefinition.*validators' awx/main/models/
</request>
</function_calls>

```shell
#!/bin/bash
set -euo pipefail

# The RoleDefinition is from ansible_base. Check if there are migrations or guards
# that would prevent custom role names from being created

# Check how roles are actually created in code
echo "=== How RoleDefinition names are constructed in code ==="
rg -n 'RoleDefinition.*name\s*=' awx/main/models/rbac.py | head -20

# Check the test that creates a custom role with get_or_create
echo -e "\n=== Custom role test in detail ==="
sed -n '72,85p' awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py
</request>
</function_calls>

Here are the results from shell you requested:

```
Repository: ansible/awx
Exit code: 0


stdout:
```
=== How RoleDefinition names are constructed in code ==="
awx/main/models/rbac.py:246-            f'{object_model.__name__} {role_definition.role_field_name()}',
awx/main/models/rbac.py-246-            f'{object_model.__name__} {role_definition.role_field_name()}',
awx/main/models/rbac.py-246-            f'{object_model.__name__} {role_definition.role_definition_name()}',
awx/main/models/rbac.py-where the format is "[Model] [Role]" (space-separated).
awx/main/models/rbac.py-256-            defaults={'content_type': ct, 'managed': True, 'description': description},
-256-            defaults={'content_type': ct, 'managed': True, 'description': True},
-256-            defaults={'content_type': ct, 'managed': True', 'description': description},

=== Custom role test in detail ==="
    # TODO: just a delete_inventory, without change_inventory
    rd, _ = RoleDefinition.objects.get_or_create(
        name='inventory-delete',
        permissions=['delete_inventory', 'view_inventory', 'change_inventory'],
        content_type=permission_registry.content_type_model.objects.get_for_model(Inventory),
    )
```

Perfect! Now I can see the issue clearly. Let me verify what happens when `get_role_from_object_role` encounters this custom role name:
<function_calls>
<location>shell</location>
<request>#!/bin/bash
set -euo pipefail

# Simulate what get_role_from_object_role does with 'inventory-delete'
echo "=== Testing role name parsing logic ==="
python3 << 'EOF'
role_name = 'inventory-delete'

# Check each condition
checks = [
    ("endswith(' Compat')", role_name.endswith(' Compat')),
    ("endswith(' Admin') and count==2", role_name.endswith(' Admin') and role_name.count(' ') == 2),
    ("endswith(' Admin')", role_name.endswith(' Admin')),
    ("== 'Organization Audit'", role_name == 'Organization Audit'),
]

for check_name, result in checks:
    print(f"{check_name}: {result}")

# If none match, falls to else clause
print("\nFalls to else clause: split by space")
try:
    parts = role_name.split()
    print(f"Split result: {parts}")
    model_name, role_part = parts
except ValueError as e:
    print(f"ERROR: {e}")
    print(f"Split produced {len(parts)} parts, but trying to unpack into 2 variables")
EOF
</request>
</function_calls>

```shell
#!/bin/bash
set -euo pipefail

# Now check if there are any guards in the serializer that filter roles
echo "=== Check if roles are filtered before passing to get_role_from_object_role ==="
sed -n '2770,2800p' awx/api/serializers.py

# Also check the RoleEvaluation query - does it filter anything?
echo -e "\n=== RoleEvaluation filtering ==="
rg -n 'RoleEvaluation.objects.filter' awx/api/serializers.py
</request>
</function_calls>

Here are the results from shell you requested:

```
Repository: ansible/awx
Exit code: 0


stdout:

Repository: ansible/awx

Length of output: 1619


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look at the full test to see if access_list is called with custom roles
echo "=== test_assign_custom_delete_role full test ==="
sed -n '70,120p' awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py
</request>
</function_calls>

Here are the results from shell you requested:

Repository: ansible/awx

Length of output: 2567


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check generic views for list serializers
echo "=== Check for access_list generic views ==="
rg -n 'access_list\|AccessList' awx/api/generics.py | head -30
</request>
</function_calls>

Here are the results from shell you requested:

Repository: ansible/awx

Length of output: 98


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check ResourceAccessListView full implementation
echo "=== ResourceAccessListView ==="
sed -n '640,700p' awx/api/generics.py
</request>
</function_calls>

Here are the results from shell you requested:

Repository: ansible/awx

Length of output: 2467


Handle custom/non-compat RoleDefinition names before legacy conversion.

Lines 2796 and 2813 call get_role_from_object_role() on every RoleEvaluation.role without validation. This function only handles legacy/compat role naming patterns. Custom role names like 'inventory-delete' (which can be created via RoleDefinition.objects.get_or_create()) will fail in the else clause when attempting to unpack the split result: model_name, role_name = rd.name.split() raises ValueError: not enough values to unpack. This causes a 500 error when accessing the access_list endpoint for a resource where a user has been assigned such a role.

Either filter to compat roles before conversion, add exception handling for custom role names, or validate that only compat roles can be assigned in this context.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/api/serializers.py` around lines 2791 - 2814, The code calls
get_role_from_object_role() for every RoleEvaluation.role (from RoleEvaluation
and team_obj_roles) but that helper only supports legacy/compat RoleDefinition
names and will ValueError on custom names (e.g., 'inventory-delete'); update the
logic to validate or filter roles before conversion: either filter the
RoleEvaluation queryset to only include roles whose RoleDefinition.name matches
the legacy "model role" pattern (e.g., contains a space or otherwise detectable
compat format) OR wrap the get_role_from_object_role(role) call in a small guard
that skips/handles RoleDefinition names that do not match the compat pattern
(fallback to treating them as custom roles, adding them to indirect_access or
logging and continuing) so you never attempt to split an incompatible name;
apply this change in both loops that iterate over
RoleEvaluation.objects.filter(...) and reference get_role_from_object_role,
new_role, RoleEvaluation.role, and RoleDefinition.name.

@chrismeyersfsu chrismeyersfsu Apr 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This one is real, I was able to recreate it using the API.

Steps to recreate

  1. Create inventory
    POST /api/v2/inventories/ → {"name": "bug-repro-inventory", "organization": 1}
  2. Create non-admin user
    POST /api/v2/users/ → {"username": "bug-user", "password": "bug-password", "email": "bug@example.com", "is_superuser": false}
  3. Create RoleDefinition with single-word name (no space = triggers bug)
    POST /api/v2/role_definitions/ → {"name": "inventory-delete", "content_type": "awx.inventory", "permissions": ["awx.view_inventory", "awx.change_inventory", "awx.delete_inventory"]}
  4. Assign role to user on inventory
    POST /api/v2/role_user_assignments/ → {"user": <user_id>, "role_definition": <rd_id>, "object_id": <inv_id>}
  5. Trigger 500
    GET /api/v2/inventories/<inv_id>/access_list/ → ValueError: not enough values to unpack

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@chrismeyersfsu Thanks for the clear repro — confirmed. The 500 stems from ResourceAccessListElementSerializer calling get_role_from_object_role() unguarded; that helper only handles legacy/compat role names and raises on custom names like "inventory-delete".

Proposed fix

  • Make the serializer resilient: safely attempt the compat conversion and skip roles that can’t be converted, instead of erroring. This prevents 500s while keeping existing behavior for compat roles. We can later enhance representation for truly custom roles.

Plan and patch
[plan_code_changes]

  • Guard get_role_from_object_role() calls in ResourceAccessListElementSerializer.to_representation() and skip non-compat roles to avoid ValueError.
  • Add a regression test that follows your steps and asserts the access_list returns 200 and includes the user.

If you prefer, I can expand the fallback to display a minimal entry for custom roles (id=None, name=<RoleDefinition.name>, unattach=False), but the above keeps behavior unchanged for compat roles and resolves the 500 immediately.

Would you like me to open a follow-up issue to track richer access-list rendering for custom RoleDefinitions?


📝 Learnings were identified and not saved (knowledge base disabled). Enable

Comment thread awx/main/management/commands/inventory_import.py
Comment thread awx/main/models/rbac.py
Comment on lines +191 to +192
q = RoleEvaluation.objects.filter(role__in=user.has_roles.all()).values_list('object_id', 'content_type_id').query
return roles_qs.extra(where=[f'(object_id,content_type_id) in ({q})'])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify no raw `.extra(where=...)` remains in this RBAC visibility path after the change.
# Expected: no match for Role.filter_visible_roles.
rg -n -C3 --type=py '\.extra\s*\(\s*where='

Repository: ansible/awx

Length of output: 557


Avoid string-built SQL in filter_visible_roles.

Line 192 embeds a rendered Django Query into .extra(where=...), which is deprecated and flagged by Ruff S610. Use a correlated Exists to keep this parameterized and avoid SQL-fragment fragility.

Proposed ORM-only replacement
-        q = RoleEvaluation.objects.filter(role__in=user.has_roles.all()).values_list('object_id', 'content_type_id').query
-        return roles_qs.extra(where=[f'(object_id,content_type_id) in ({q})'])
+        visible_evaluations = RoleEvaluation.objects.filter(
+            role__in=user.has_roles.all(),
+            object_id=models.OuterRef('object_id'),
+            content_type_id=models.OuterRef('content_type_id'),
+        )
+        return roles_qs.annotate(_awx_visible=models.Exists(visible_evaluations)).filter(_awx_visible=True)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
q = RoleEvaluation.objects.filter(role__in=user.has_roles.all()).values_list('object_id', 'content_type_id').query
return roles_qs.extra(where=[f'(object_id,content_type_id) in ({q})'])
visible_evaluations = RoleEvaluation.objects.filter(
role__in=user.has_roles.all(),
object_id=models.OuterRef('object_id'),
content_type_id=models.OuterRef('content_type_id'),
)
return roles_qs.annotate(_awx_visible=models.Exists(visible_evaluations)).filter(_awx_visible=True)
🧰 Tools
🪛 Ruff (0.15.10)

[error] 192-192: Use of Django extra can lead to SQL injection vulnerabilities

(S610)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/models/rbac.py` around lines 191 - 192, The current
filter_visible_roles implementation builds raw SQL by embedding
RoleEvaluation.objects.filter(role__in=user.has_roles.all()).values_list('object_id',
'content_type_id').query into roles_qs.extra(where=...), which is unsafe and
deprecated; replace this with a correlated ORM Exists subquery: construct a
RoleEvaluation queryset filtered by role__in=user.has_roles.all() and matching
object_id and content_type_id to the outer role (use OuterRef('object_id') /
OuterRef('content_type_id')), wrap that in Exists(...) and apply
roles_qs.filter(Exists(...)); update the method named filter_visible_roles to
remove .extra(...) and use the Exists-based filter so the query is parameterized
and Ruff S610 compliant.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
awx/api/serializers.py (1)

2790-2813: ⚠️ Potential issue | 🟠 Major

Handle custom/non-compat roles before legacy role conversion.

get_role_from_object_role(new_role) is still called for every evaluated ObjectRole; custom RoleDefinition names can fail this conversion and turn the access-list endpoint into a 500. This was already flagged on the earlier revision, so please keep the guard/filter/fallback fix here as well.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/api/serializers.py` around lines 2790 - 2813, The code calls
get_role_from_object_role(new_role) unguarded for every
RoleEvaluation/ObjectRole which breaks for custom/non-compatible
RoleDefinitions; before converting, filter or guard against non-compatible roles
(e.g., check the RoleDefinition name/compatibility flag or wrap
get_role_from_object_role in a try/except) and skip or apply a safe fallback for
those entries so they do not raise a 500; apply this same guard in both loops
that iterate RoleEvaluation (the loop over user.has_roles and the loop over
team_obj_roles) and reference get_role_from_object_role, RoleEvaluation,
ObjectRole, and permission_registry.team_model when locating where to add the
check.
🤖 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/serializers.py`:
- Around line 2798-2803: The code casts new_role.object_id with int(...) before
checking for global/indirect roles which can be None; update the conditional
around new_role.object_id in the block that builds ret['summary_fields'] so you
first guard that new_role.object_id is not None (or is truthy) before converting
to int and comparing to obj.id; specifically change the branch that references
new_role.object_id/int(...) in the serializer where new_role and old_role are
handled (the RoleEvaluation handling block using new_role.object_id and
new_role.content_type_id) to check for None (or use a safe int conversion) and
only perform int(...) == obj.id when object_id exists, leaving the
team_content_type and indirect_access branches to handle None/global roles.

---

Duplicate comments:
In `@awx/api/serializers.py`:
- Around line 2790-2813: The code calls get_role_from_object_role(new_role)
unguarded for every RoleEvaluation/ObjectRole which breaks for
custom/non-compatible RoleDefinitions; before converting, filter or guard
against non-compatible roles (e.g., check the RoleDefinition name/compatibility
flag or wrap get_role_from_object_role in a try/except) and skip or apply a safe
fallback for those entries so they do not raise a 500; apply this same guard in
both loops that iterate RoleEvaluation (the loop over user.has_roles and the
loop over team_obj_roles) and reference get_role_from_object_role,
RoleEvaluation, ObjectRole, and permission_registry.team_model when locating
where to add the check.
🪄 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: Pro Plus

Run ID: cab1310d-c2cd-4a36-a437-de784f0d6e64

📥 Commits

Reviewing files that changed from the base of the PR and between 96365f5 and b0463e9.

📒 Files selected for processing (2)
  • awx/api/serializers.py
  • awx/main/migrations/_rbac.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • awx/main/migrations/_rbac.py

Comment thread awx/api/serializers.py
Comment on lines +2798 to +2803
if int(new_role.object_id) == obj.id and new_role.content_type_id == content_type.id:
ret['summary_fields']['direct_access'].append(format_role_perm(old_role))
elif new_role.content_type_id == team_content_type.id:
all_team_roles.add(old_role)
else:
ret['summary_fields']['indirect_access'].append(format_role_perm(old_role))

@coderabbitai coderabbitai Bot Apr 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Guard object_id before casting it.

Line 2798 casts new_role.object_id with int(...) before handling indirect/global roles. If RoleEvaluation returns a singleton/global role with object_id=None, this path raises before the later superuser/auditor indirect entries are added.

🐛 Proposed guard
-            if int(new_role.object_id) == obj.id and new_role.content_type_id == content_type.id:
+            if new_role.object_id is not None and int(new_role.object_id) == obj.id and new_role.content_type_id == content_type.id:
                 ret['summary_fields']['direct_access'].append(format_role_perm(old_role))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/api/serializers.py` around lines 2798 - 2803, The code casts
new_role.object_id with int(...) before checking for global/indirect roles which
can be None; update the conditional around new_role.object_id in the block that
builds ret['summary_fields'] so you first guard that new_role.object_id is not
None (or is truthy) before converting to int and comparing to obj.id;
specifically change the branch that references new_role.object_id/int(...) in
the serializer where new_role and old_role are handled (the RoleEvaluation
handling block using new_role.object_id and new_role.content_type_id) to check
for None (or use a safe int conversion) and only perform int(...) == obj.id when
object_id exists, leaving the team_content_type and indirect_access branches to
handle None/global roles.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tried and wasn't able to get object_id=None via API requests. Probably because object_id is not allowed to be null in the database. I would click Resolve comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@chrismeyersfsu Thanks for checking. Agreed: with DAB RBAC, the RoleEvaluation queryset here is constrained to the target object (content_type_id and object_id), and it doesn’t include global/singleton roles. Indirect entries (e.g., team/org roles) still have non-null object_id values (their team/org PK). Superuser/auditor are handled explicitly later, outside RoleEvaluation.

Given that, int(new_role.object_id) should not encounter None, and the guard isn’t strictly necessary. Adding the None check would be a harmless defensive change, but I’m fine resolving without it based on current behavior and constraints.

@AlanCoding

Copy link
Copy Markdown
Member Author

Baby YOLO Results — Build #145

Compared against latest devel reference #146. 21 total failures, 18 pre-existing in devel, 3 regressions unique to this branch:

Regressions

Test Error Concern
test_team_can_launch_workflow_job_templates_with_role[Organization WorkflowJobTemplate Admin-ORG-201-Gateway] 403 instead of 201 Real RBAC regression — team with Org WFJ Admin role can't launch workflows
test_user_can_launch_workflow_job_templates_with_role[None-ORG-403-Gateway] IndexError: pop from empty list (setup) RBAC test setup broken, likely related
test_job_template_change_request_without_usage_role_returns_code_403[Organization Member-403-Gateway] Project failed to enter a successful state (setup) Likely infra flake

The first two failures are RBAC permission tests that fail only on this branch — not in devel (#146) or other feature branches tested on the same day. Given this PR removes RBAC code, the workflow job template launch regression looks like a real issue. Pausing work on this until investigated.

Comment thread awx/main/migrations/_rbac.py Outdated
if changed_ct:
logger.info('{} explicit JT permission for {} users in {:.4f} seconds'.format('Removed' if backward else 'Added', changed_ct, time() - start))
"""Not used after DAB RBAC migration, ancestors table has been removed"""
pass

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Trying to think if it's OK to delete the logic that was deleted.

https://github.com/AlanCoding/awx/blob/b0463e9b50f561edb1582035ad99a53f625475f6/awx/main/migrations/0109_v370_job_template_organization_field.py#L116 <-- this migration uses it. Comment at the top says the migration was created in 2019.

0206_remove_role_ancestors.py deletes the ancestors table.

If a user migrates from < 0109_v370_job_template_organization_field.py to this version; then I think there would be a problem. But is that too big of a jump?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm tempted to agree that we should add it back. The only question is whether it would work? The problem we have in migrations is imports from current code (like batch_role_ancestor_rebuilding) which is a giant liability. But reading back over this method, I think it's written safely with models from the apps arg. So probably valid for those prior apps, and as long as it doesn't break any rules, then I'm tempted to agree, it should be forever-code.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
awx/main/management/commands/inventory_import.py (1)

1023-1058: ⚠️ Potential issue | 🟠 Major

Use tri-state failure tracking here; current default can misclassify errors.

At Line 1024, initializing license_fail = False means any PermissionDenied raised before the license check path is treated as org-limit failure. Also, at Line 1058, raise e loses traceback fidelity.

Safer exception classification + re-raise
-            try:
-                license_fail = False
+            try:
+                license_fail = None
@@
-                    license_fail = True
+                    license_fail = True
                     self.check_license()
@@
-                    license_fail = False
+                    license_fail = False
                     self.check_org_host_limit()
-            except PermissionDenied as e:
-                if license_fail:
+            except PermissionDenied:
+                if license_fail is True:
                     self.mark_license_failure(save=True)
-                else:
+                elif license_fail is False:
                     self.mark_org_limits_failure(save=True)
-                raise e
+                raise
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/management/commands/inventory_import.py` around lines 1023 - 1058,
The code misclassifies errors because license_fail is initialized to False and
re-raises the exception with "raise e" losing traceback; change license_fail to
a tri-state (e.g., None initially) near the top of the try block, set
license_fail = True immediately before calling self.check_license() and set
license_fail = False before self.check_org_host_limit(), then in the
PermissionDenied except branch call self.mark_license_failure(save=True) if
license_fail is True, self.mark_org_limits_failure(save=True) if license_fail is
False, and handle the None case if needed; finally re-raise the caught exception
using plain "raise" (not "raise e") to preserve the original traceback.
awx/api/serializers.py (1)

2816-2839: ⚠️ Potential issue | 🟠 Major

Guard legacy role conversion to prevent access_list 500s

Line 2821 and Line 2838 call get_role_from_object_role(new_role) unconditionally. Custom/non-compat RoleDefinition names can raise during conversion, which can still crash access_list serialization.

Proposed fix
         new_roles_seen = set()
         all_team_roles = set()
         all_permissive_role_ids = set()
+
+        def _safe_get_role_from_object_role(object_role):
+            try:
+                return get_role_from_object_role(object_role)
+            except Exception:
+                logger.debug(
+                    "Skipping non-compat role in access_list serialization: %s",
+                    getattr(getattr(object_role, "role_definition", None), "name", None),
+                )
+                return None
+
         for evaluation in RoleEvaluation.objects.filter(role__in=user.has_roles.all(), **gfk_kwargs).prefetch_related('role'):
             new_role = evaluation.role
             if new_role.id in new_roles_seen:
                 continue
             new_roles_seen.add(new_role.id)
-            old_role = get_role_from_object_role(new_role)
+            old_role = _safe_get_role_from_object_role(new_role)
+            if old_role is None:
+                continue
             all_permissive_role_ids.add(old_role.id)
@@
         for evaluation in RoleEvaluation.objects.filter(role__in=team_obj_roles, **gfk_kwargs).prefetch_related('role'):
             new_role = evaluation.role
             if new_role.id in new_roles_seen:
                 continue
             new_roles_seen.add(new_role.id)
-            old_role = get_role_from_object_role(new_role)
+            old_role = _safe_get_role_from_object_role(new_role)
+            if old_role is None:
+                continue
             all_permissive_role_ids.add(old_role.id)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/api/serializers.py` around lines 2816 - 2839, The calls to
get_role_from_object_role(new_role) in the RoleEvaluation loops can raise for
custom/non-compat RoleDefinition names; wrap each call to
get_role_from_object_role(new_role) inside a try/except that catches the
conversion errors (Exception or a more specific exception if available), skip
the role on error (do not add to new_roles_seen/all_permissive_role_ids), and
optionally log the failure with context (new_role.id, new_role.content_type_id)
so access_list serialization does not 500; apply this guard in both loops where
get_role_from_object_role is invoked (the loop that appends to
ret['summary_fields']['direct_access'] / 'indirect_access' using
format_role_perm and the loop iterating over team_obj_roles).
awx/main/models/rbac.py (1)

191-192: ⚠️ Potential issue | 🟠 Major

Replace .extra(where=...) with a correlated Exists.

This still embeds rendered SQL into Django’s legacy .extra() API on an authorization path. Ruff S610 flags it, and it is more brittle than the ORM-native equivalent.

Suggested fix
-        q = RoleEvaluation.objects.filter(role__in=user.has_roles.all()).values_list('object_id', 'content_type_id').query
-        return roles_qs.extra(where=[f'(object_id,content_type_id) in ({q})'])
+        visible_evaluations = RoleEvaluation.objects.filter(
+            role__in=user.has_roles.all(),
+            object_id=models.OuterRef('object_id'),
+            content_type_id=models.OuterRef('content_type_id'),
+        )
+        return roles_qs.annotate(
+            _awx_visible=models.Exists(visible_evaluations)
+        ).filter(_awx_visible=True)
#!/bin/bash
set -euo pipefail

echo "Current implementation:"
sed -n '185,193p' awx/main/models/rbac.py

echo
echo "Any remaining .extra(where=...) usages in this file:"
rg -n -C2 --type=py '\.extra\s*\(\s*where=' awx/main/models/rbac.py
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/models/rbac.py` around lines 191 - 192, Replace the raw SQL
.extra(...) call with a correlated Exists subquery: build a subquery from
RoleEvaluation.filtered by role__in=user.has_roles.all() and matching the outer
row via object_id=OuterRef('pk') and
content_type_id=OuterRef('content_type_id'), then filter roles_qs with
.filter(Exists(subquery)). Use Django ORM constructs Exists and OuterRef and
reference RoleEvaluation, user.has_roles and roles_qs to locate the code to
change.
🧹 Nitpick comments (1)
awx/main/tests/functional/api/test_resource_access_lists.py (1)

75-83: Strengthen these regression assertions to catch cross-org leakage.

Current checks validate HTTP 200 and minimal presence, but they don’t assert exclusion of unrelated org principals/roles.

Suggested test hardening
@@
     result = get(reverse('api:job_template_access_list', kwargs={'pk': jt.id}), org1_admin)
     assert result.status_code == 200
 
     org1_admin_entry = [r for r in result.data['results'] if r['id'] == org1_admin.id]
     assert len(org1_admin_entry) == 1
+    assert all(r['id'] != org2_admin.id for r in result.data['results'])
@@
     result = get(reverse('api:job_template_object_roles_list', kwargs={'pk': jt.id}), org_admin)
     assert result.status_code == 200
     assert result.data['count'] > 0
+    role_ids = {r['id'] for r in result.data['results']}
+    assert jt.admin_role.id in role_ids

As per coding guidelines, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”

Also applies to: 95-97

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/tests/functional/api/test_resource_access_lists.py` around lines 75
- 83, The test currently only checks status codes and that org1_admin appears in
the results, which can miss cross-org leakage; update the assertions after
calling reverse('api:job_template_access_list', kwargs={'pk': jt.id}) (the calls
assigning result and org1_admin_entry) to explicitly assert that the
result.data['results'] contains exactly the expected principals for jt's
organization (e.g., contains org1_admin and not admin or any principals from
other orgs), and add negative assertions that no entries with admin.id or any
principal/org IDs from other orgs appear; apply the same stricter
presence/absence checks to the similar block around lines 95-97 so the test
fails on cross-org leakage.
🤖 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/main/models/rbac.py`:
- Around line 143-149: Role.__contains__ is returning generic permission
codenames (e.g. "execute") for non-organization objects; change the lookup so
non-org permissions are model-specific (e.g. "execute_workflowjobtemplate"). In
Role.__contains__ (and where to_permissions is used), when resolving
self.role_field for a non-organization content_object, build the codename by
combining the base permission with content_object._meta.model_name (or otherwise
map to "permission_modelname") before calling
accessor.has_obj_perm(self.content_object, ...). Keep the existing
org_role_to_permission branch for organization-scoped mappings and use the
computed model-specific codename for all other object types to ensure
WorkflowJobTemplateAccess.can_start() checks the correct permission.

---

Duplicate comments:
In `@awx/api/serializers.py`:
- Around line 2816-2839: The calls to get_role_from_object_role(new_role) in the
RoleEvaluation loops can raise for custom/non-compat RoleDefinition names; wrap
each call to get_role_from_object_role(new_role) inside a try/except that
catches the conversion errors (Exception or a more specific exception if
available), skip the role on error (do not add to
new_roles_seen/all_permissive_role_ids), and optionally log the failure with
context (new_role.id, new_role.content_type_id) so access_list serialization
does not 500; apply this guard in both loops where get_role_from_object_role is
invoked (the loop that appends to ret['summary_fields']['direct_access'] /
'indirect_access' using format_role_perm and the loop iterating over
team_obj_roles).

In `@awx/main/management/commands/inventory_import.py`:
- Around line 1023-1058: The code misclassifies errors because license_fail is
initialized to False and re-raises the exception with "raise e" losing
traceback; change license_fail to a tri-state (e.g., None initially) near the
top of the try block, set license_fail = True immediately before calling
self.check_license() and set license_fail = False before
self.check_org_host_limit(), then in the PermissionDenied except branch call
self.mark_license_failure(save=True) if license_fail is True,
self.mark_org_limits_failure(save=True) if license_fail is False, and handle the
None case if needed; finally re-raise the caught exception using plain "raise"
(not "raise e") to preserve the original traceback.

In `@awx/main/models/rbac.py`:
- Around line 191-192: Replace the raw SQL .extra(...) call with a correlated
Exists subquery: build a subquery from RoleEvaluation.filtered by
role__in=user.has_roles.all() and matching the outer row via
object_id=OuterRef('pk') and content_type_id=OuterRef('content_type_id'), then
filter roles_qs with .filter(Exists(subquery)). Use Django ORM constructs Exists
and OuterRef and reference RoleEvaluation, user.has_roles and roles_qs to locate
the code to change.

---

Nitpick comments:
In `@awx/main/tests/functional/api/test_resource_access_lists.py`:
- Around line 75-83: The test currently only checks status codes and that
org1_admin appears in the results, which can miss cross-org leakage; update the
assertions after calling reverse('api:job_template_access_list', kwargs={'pk':
jt.id}) (the calls assigning result and org1_admin_entry) to explicitly assert
that the result.data['results'] contains exactly the expected principals for
jt's organization (e.g., contains org1_admin and not admin or any principals
from other orgs), and add negative assertions that no entries with admin.id or
any principal/org IDs from other orgs appear; apply the same stricter
presence/absence checks to the similar block around lines 95-97 so the test
fails on cross-org leakage.
🪄 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: 86ece6de-468e-4764-88c3-cbb649fe5677

📥 Commits

Reviewing files that changed from the base of the PR and between b0463e9 and afc67a1.

📒 Files selected for processing (14)
  • awx/api/generics.py
  • awx/api/serializers.py
  • awx/main/access.py
  • awx/main/management/commands/inventory_import.py
  • awx/main/migrations/0206_remove_role_ancestors.py
  • awx/main/migrations/_rbac.py
  • awx/main/models/__init__.py
  • awx/main/models/mixins.py
  • awx/main/models/rbac.py
  • awx/main/signals.py
  • awx/main/tests/functional/api/test_resource_access_lists.py
  • awx/main/tests/functional/rbac/test_rbac_role.py
  • awx/settings/defaults.py
  • tools/data_generators/rbac_dummy_data_generator.py
💤 Files with no reviewable changes (3)
  • awx/main/models/init.py
  • awx/settings/defaults.py
  • awx/main/signals.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • awx/api/generics.py

Comment thread awx/main/models/rbac.py
Comment on lines +143 to +149
if self.content_object and self.content_object._meta.model_name == 'organization' and self.role_field in org_role_to_permission:
codename = org_role_to_permission[self.role_field]
return accessor.has_obj_perm(self.content_object, codename)

return accessor.has_obj_perm(self.content_object, codename)

if self.role_field not in to_permissions:
raise Exception(f'{self.role_field} evaluated but not a translatable permission')
return accessor.has_obj_perm(self.content_object, to_permissions[self.role_field])
return self.ancestors.filter(members=accessor).exists()
if self.role_field not in to_permissions:
raise Exception(f'{self.role_field} evaluated but not a translatable permission')
return accessor.has_obj_perm(self.content_object, to_permissions[self.role_field])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Representative mappings used by Role.__contains__:"
rg -n -A20 -B3 'role_name_to_perm_mapping|org_role_to_permission' awx/main/constants.py

echo
echo "Call sites that depend on Role.__contains__ for launch/admin checks:"
rg -n -C2 'in obj\.(execute_role|admin_role)|workflow_admin_role|can_start\(' awx/main/access.py awx/main/models/rbac.py

echo
echo "Relevant workflow-launch regression coverage:"
rg -n -C2 'launch_workflow_job_templates|WorkflowJobTemplate Admin|ORG-201|ORG-403' awx/main/tests

Repository: ansible/awx

Length of output: 14111


🏁 Script executed:

# First, let's see the complete Role.__contains__ method and understand the mapping context
sed -n '130,160p' awx/main/models/rbac.py | cat -n

Repository: ansible/awx

Length of output: 1661


🏁 Script executed:

# Also check what to_permissions is - likely defined earlier in the method or class
sed -n '100,155p' awx/main/models/rbac.py | cat -n

Repository: ansible/awx

Length of output: 2846


🏁 Script executed:

# Search for any actual regression tests or issues mentioning 403 errors with workflows
rg -n 'HTTP.*403|workflow.*403|403.*workflow' awx/main/tests --type py -i

Repository: ansible/awx

Length of output: 297


🏁 Script executed:

# Check if there are any test failures or skip markers related to workflow launch
rg -n '@skip|@xfail|pytest.mark.skip|workflow.*launch.*403|WORKFLOW.*REGRESS' awx/main/tests -i

Repository: ansible/awx

Length of output: 171


🏁 Script executed:

# Find where to_permissions is defined
rg -n 'to_permissions\s*=' awx/main/models/rbac.py -B5 -A10

Repository: ansible/awx

Length of output: 425


🏁 Script executed:

# Check if execute_role and admin_role are properly mapped in both dictionaries
rg -n "execute_role|admin_role" awx/main/constants.py

Repository: ansible/awx

Length of output: 642


🏁 Script executed:

# Check the imports in rbac.py to understand what mappings are available
head -50 awx/main/models/rbac.py | cat -n

Repository: ansible/awx

Length of output: 2080


🏁 Script executed:

# Find where to_permissions is actually imported/used in rbac.py
rg -n 'from.*to_permissions|import.*to_permissions|to_permissions' awx/main/models/rbac.py

Repository: ansible/awx

Length of output: 278


🏁 Script executed:

# Check what codename should be used for workflow execute permission
rg -n 'execute.*workflow|workflow.*execute' awx/main/constants.py -i

Repository: ansible/awx

Length of output: 37


🏁 Script executed:

# Check if there's an 'execute_workflowjobtemplate' permission defined anywhere
rg -n 'execute_workflowjobtemplate|workflowjobtemplate.*execute' awx/main --type py -i

Repository: ansible/awx

Length of output: 585


🏁 Script executed:

# Check the actual permission registry to see what permissions exist for WorkflowJobTemplate
rg -n 'WorkflowJobTemplate.*permission|class.*WorkflowJobTemplate.*permission' awx/main --type py -i -A5

Repository: ansible/awx

Length of output: 446


🏁 Script executed:

# Verify the mapping issue: check what execute_role should map to for WorkflowJobTemplate
# Compare with JobTemplate to see the difference
rg -n "'execute_jobtemplate'|'execute_workflowjobtemplate'" awx/main --type py

Repository: ansible/awx

Length of output: 1167


🏁 Script executed:

# Check the actual Role definitions to understand the intent
rg -n "WorkflowJobTemplate.*Execute|Execute.*Workflow" awx/main/models/rbac.py -B2 -A2

Repository: ansible/awx

Length of output: 335


🏁 Script executed:

# Trace through: when a WorkflowJobTemplate checks execute_role, what should happen?
# Let's see what codename is actually being used in permission checks
rg -n "has_obj_perm.*execute" awx/main/access.py -B2 -A2

Repository: ansible/awx

Length of output: 37


🏁 Script executed:

# Critical check: what does to_permissions actually contain?
python3 << 'EOF'
role_name_to_perm_mapping = {
    'adhoc_role': ['adhoc_'],
    'approval_role': ['approve_'],
    'auditor_role': ['audit_'],
    'admin_role': ['change_', 'add_', 'delete_'],
    'execute_role': ['execute_'],
    'read_role': ['view_'],
    'update_role': ['update_'],
    'member_role': ['member_'],
    'use_role': ['use_'],
}

to_permissions = {}
for k, v in role_name_to_perm_mapping.items():
    to_permissions[k] = v[0].strip('_')

print("to_permissions mapping:")
for k, v in to_permissions.items():
    print(f"  {k}: '{v}'")

print("\nProblem: execute_role maps to 'execute', but:")
print("  - JobTemplate expects 'execute_jobtemplate'")
print("  - WorkflowJobTemplate expects 'execute_workflowjobtemplate'")
EOF

Repository: ansible/awx

Length of output: 442


🏁 Script executed:

# Confirm: are there any other permission codenames that don't match the pattern?
rg -n "permissions.*=.*\[" awx/main/models/ awx/main/migrations/0191 --type py | grep -E "execute|update|adhoc" | head -20

Repository: ansible/awx

Length of output: 400


🏁 Script executed:

# Check what permission is actually used when checking workflow execute
rg -n "class WorkflowJobTemplate" awx/main/models/workflow.py -A50 | grep -E "permissions|execute_workflowjobtemplate"

Repository: ansible/awx

Length of output: 170


🏁 Script executed:

# Verify that 'execute' permission doesn't actually exist as a standalone permission
rg -n "permissions.*=.*\['execute'" awx/main/models --type py

Repository: ansible/awx

Length of output: 37


🏁 Script executed:

# Check how the permission system works - are 'execute' permissions app-specific?
rg -n "'execute'" awx/main/models -B2 -A2 | grep -E "permissions|codename" | head -20

Repository: ansible/awx

Length of output: 37


🏁 Script executed:

# Final confirmation: Check what happens when has_obj_perm is called with just 'execute'
rg -n "def has_obj_perm" awx/main --type py -A10

Repository: ansible/awx

Length of output: 37


🏁 Script executed:

# Check if there are any fallback mechanisms or if 'execute' would just silently fail
rg -n "has_obj_perm.*execute" awx/main/models --type py

Repository: ansible/awx

Length of output: 37


Critical bug in role-to-codename translation breaks non-organization workflow launch access checks.

The Role.__contains__ translation at lines 143-149 has a fundamental flaw: it maps execute_role to a generic 'execute' codename via to_permissions, but Django expects model-specific permission names like 'execute_workflowjobtemplate'.

For WorkflowJobTemplate execute checks on non-organization objects:

  • WorkflowJobTemplateAccess.can_start() calls self.user in obj.execute_role
  • This triggers Role.__contains__ → line 149 calls has_obj_perm(obj, to_permissions['execute_role'])
  • to_permissions['execute_role'] returns 'execute' (constructed by stripping 'execute_' prefix)
  • Permission check fails because the actual permission is 'execute_workflowjobtemplate', causing 403

The org_role_to_permission dict correctly maps execute_role → 'execute_jobtemplate', but only applies to organization-scoped objects (line 144). Non-organization objects fall through to line 148-149 and use the broken generic mapping, causing the reported workflow launch regression.

The to_permissions dictionary construction must provide model-specific permission names, not generic prefixes stripped from role_name_to_perm_mapping.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/models/rbac.py` around lines 143 - 149, Role.__contains__ is
returning generic permission codenames (e.g. "execute") for non-organization
objects; change the lookup so non-org permissions are model-specific (e.g.
"execute_workflowjobtemplate"). In Role.__contains__ (and where to_permissions
is used), when resolving self.role_field for a non-organization content_object,
build the codename by combining the base permission with
content_object._meta.model_name (or otherwise map to "permission_modelname")
before calling accessor.has_obj_perm(self.content_object, ...). Keep the
existing org_role_to_permission branch for organization-scoped mappings and use
the computed model-specific codename for all other object types to ensure
WorkflowJobTemplateAccess.can_start() checks the correct permission.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
E Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants