[RBAC] Fix bug where team could not be given read_role to other team (#…

…15067)

* Fix bug where team could not be given read_role to other team

* Avoid unwanted triggers of parentage granting

* Restructure signal structure

* Fix another bug unmasked by team member permission fix

* Changes to live with test writing

* Use equality as opposed to string "in"

from Seth in review comment

Co-authored-by: Seth Foster <[email protected]>

---------

Co-authored-by: Seth Foster <[email protected]>

awx/api/views/__init__.py

@@ -797,7 +797,7 @@ def get_queryset(self):
         role = ObjectRole.objects.filter(object_id=team.id, content_type=parent_ct, role_definition=rd).first()
         if role is None:
             # Team has no permissions, therefore team has no projects
-            return self.model.none()
+            return self.model.objects.none()
         else:
             project_qs = self.model.accessible_objects(self.request.user, 'read_role')
             return project_qs.filter(id__in=RoleEvaluation.objects.filter(content_type_id=model_ct.id, role=role).values_list('object_id'))

awx/main/models/rbac.py

@@ -665,8 +665,6 @@ def sync_parents_to_new_rbac(instance, action, model, pk_set, reverse, **kwargs)
     elif action == 'post_clear':
         raise RuntimeError('Clearing of role members not supported')
 
-    from awx.main.models.organization import Team
-
     if reverse:
         parent_role = instance
     else:
@@ -680,15 +678,17 @@ def sync_parents_to_new_rbac(instance, action, model, pk_set, reverse, **kwargs)
 
         # To a fault, we want to avoid running this if triggered from implicit_parents management
         # we only want to do anything if we know for sure this is a non-implicit team role
-        if parent_role.role_field not in ('member_role', 'admin_role') or parent_role.content_type.model != 'team':
-            return
+        if parent_role.role_field == 'member_role' and parent_role.content_type.model == 'team':
+            # Team internal parents are member_role->read_role and admin_role->member_role
+            # for the same object, this parenting will also be implicit_parents management
+            # do nothing for internal parents, but OTHER teams may still be assigned permissions to a team
+            if (child_role.content_type_id == parent_role.content_type_id) and (child_role.object_id == parent_role.object_id):
+                return
 
-        # Team member role is a parent of its read role so we want to avoid this
-        if child_role.role_field == 'read_role' and child_role.content_type.model == 'team':
-            return
+            from awx.main.models.organization import Team
 
-        team = Team.objects.get(pk=parent_role.object_id)
-        give_or_remove_permission(child_role, team, giving=is_giving)
+            team = Team.objects.get(pk=parent_role.object_id)
+            give_or_remove_permission(child_role, team, giving=is_giving)
 
 
 m2m_changed.connect(sync_members_to_new_rbac, Role.members.through)

awx/main/tests/functional/dab_rbac/test_translation_layer.py

@@ -1,7 +1,9 @@
+from unittest import mock
+
 import pytest
 
 from awx.main.models.rbac import get_role_from_object_role, give_creator_permissions
-from awx.main.models import User, Organization, WorkflowJobTemplate, WorkflowJobTemplateNode
+from awx.main.models import User, Organization, WorkflowJobTemplate, WorkflowJobTemplateNode, Team
 from awx.api.versioning import reverse
 
 from ansible_base.rbac.models import RoleUserAssignment
@@ -81,3 +83,25 @@ def test_creator_permission(rando, admin_user, inventory):
     give_creator_permissions(rando, inventory)
     assert rando in inventory.admin_role
     assert rando in inventory.admin_role.members.all()
+
+
+@pytest.mark.django_db
+def test_team_team_read_role(rando, team, admin_user, post):
+    orgs = [Organization.objects.create(name=f'foo-{i}') for i in range(2)]
+    teams = [Team.objects.create(name=f'foo-{i}', organization=orgs[i]) for i in range(2)]
+    teams[1].member_role.members.add(rando)
+
+    # give second team read permission to first team through the API for regression testing
+    url = reverse('api:role_teams_list', kwargs={'pk': teams[0].read_role.pk, 'version': 'v2'})
+    post(url, {'id': teams[1].id}, user=admin_user)
+
+    # user should be able to view the first team
+    assert rando in teams[0].read_role
+
+
+@pytest.mark.django_db
+def test_implicit_parents_no_assignments(organization):
+    """Through the normal course of creating models, we should not be changing DAB RBAC permissions"""
+    with mock.patch('awx.main.models.rbac.give_or_remove_permission') as mck:
+        Team.objects.create(name='random team', organization=organization)
+    mck.assert_not_called()

awx/main/tests/functional/test_fixture_factories.py

@@ -50,13 +50,13 @@ def test_org_factory_roles(organization_factory):
         teams=['team1', 'team2'],
         users=['team1:foo', 'bar'],
         projects=['baz', 'bang'],
-        roles=['team2.member_role:foo', 'team1.admin_role:bar', 'team1.admin_role:team2.admin_role', 'baz.admin_role:foo'],
+        roles=['team2.member_role:foo', 'team1.admin_role:bar', 'team1.member_role:team2.admin_role', 'baz.admin_role:foo'],
     )
 
     assert objects.users.bar in objects.teams.team2.admin_role
     assert objects.users.foo in objects.projects.baz.admin_role
     assert objects.users.foo in objects.teams.team1.member_role
-    assert objects.teams.team2.admin_role in objects.teams.team1.admin_role.children.all()
+    assert objects.teams.team2.admin_role in objects.teams.team1.member_role.children.all()
 
 
 @pytest.mark.django_db