diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index c8e1846c51..c517c56faf 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -667,8 +667,6 @@ def give_creator_permissions(user, obj): def sync_members_to_new_rbac(instance, action, model, pk_set, reverse, **kwargs): - from awx.main.signals import disable_activity_stream - if action.startswith('pre_'): return if not rbac_sync_enabled.enabled: @@ -692,11 +690,7 @@ def sync_members_to_new_rbac(instance, action, model, pk_set, reverse, **kwargs) else: user = get_user_model().objects.get(pk=user_or_role_id) rd = get_role_definition(role) - # This mirrors an already-recorded legacy Role.members change into the new RBAC - # system. Suppress activity stream here so the mirrored RoleUserAssignment/ - # RoleTeamAssignment write isn't recorded a second time (see record_role_assignment_activity_stream). - with disable_activity_stream(): - assignment = give_or_remove_permission(role, user, giving=is_giving, rd=rd) + assignment = give_or_remove_permission(role, user, giving=is_giving, rd=rd) # sync to resource server if rbac_sync_enabled.enabled: @@ -707,8 +701,6 @@ def sync_members_to_new_rbac(instance, action, model, pk_set, reverse, **kwargs) def sync_parents_to_new_rbac(instance, action, model, pk_set, reverse, **kwargs): - from awx.main.signals import disable_activity_stream - if action.startswith('pre_'): return @@ -749,10 +741,7 @@ def sync_parents_to_new_rbac(instance, action, model, pk_set, reverse, **kwargs) team = Team.objects.get(pk=parent_role.object_id) rd = get_role_definition(child_role) - # See comment in sync_members_to_new_rbac: suppress activity stream for this - # mirrored write to avoid double-recording an already-recorded legacy change. - with disable_activity_stream(): - assignment = give_or_remove_permission(child_role, team, giving=is_giving, rd=rd) + assignment = give_or_remove_permission(child_role, team, giving=is_giving, rd=rd) # sync to resource server if rbac_sync_enabled.enabled: diff --git a/awx/main/signals.py b/awx/main/signals.py index 8159eb4bf4..bc9740d860 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -56,8 +56,6 @@ from awx.main.models import ( from awx.main.utils import model_instance_diff, model_to_dict, camelcase_to_underscore, get_current_apps from awx.main.utils import ignore_inventory_computed_fields, ignore_inventory_group_removal, _inventory_updates from awx.main.tasks.system import update_inventory_computed_fields, handle_removed_image -from awx.main.fields import is_implicit_parent - from awx.main import consumers from awx.conf.utils import conf_to_dict @@ -155,34 +153,6 @@ def sync_rbac_to_superuser_status(instance, sender, **kwargs): user.save(update_fields=['is_superuser']) -def rbac_activity_stream(instance, sender, **kwargs): - # Only if we are associating/disassociating - if kwargs['action'] in ['pre_add', 'pre_remove']: - if hasattr(instance, 'content_type'): # Duck typing, migration-independent isinstance(instance, Role) - if instance.content_type_id is None and instance.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR: - # Skip entries for the system admin role because user serializer covers it - # System auditor role is shown in the serializer, but its relationship is - # managed separately, its value is incorrect, and a correction entry is needed - return - # This juggles which role to use, because could be A->B or B->A association - if sender.__name__ == 'Role_parents': - role = kwargs['model'].objects.filter(pk__in=kwargs['pk_set']).first() - # don't record implicit creation / parents in activity stream - if role is not None and is_implicit_parent(parent_role=role, child_role=instance): - return - else: - role = instance - # If a singleton role is the instance, the singleton role is acted on - # otherwise the related object is considered to be acted on - if instance.content_object: - instance = instance.content_object - else: - # Association with actor, like role->user - role = kwargs['model'].objects.filter(pk__in=kwargs['pk_set']).first() - - activity_stream_associate(sender, instance, role=role, **kwargs) - - def _record_role_assignment_activity_stream(instance, operation): if not activity_stream_enabled: return @@ -284,8 +254,6 @@ def connect_computed_field_signals(): connect_computed_field_signals() m2m_changed.connect(rebuild_role_ancestor_list, Role.parents.through) -m2m_changed.connect(rbac_activity_stream, Role.members.through) -m2m_changed.connect(rbac_activity_stream, Role.parents.through) post_save.connect(sync_superuser_status_to_rbac, sender=User) m2m_changed.connect(sync_rbac_to_superuser_status, Role.members.through) pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJob) @@ -572,17 +540,11 @@ def activity_stream_associate(sender, instance, **kwargs): getattr(activity_entry, object1).add(obj1.pk) getattr(activity_entry, object2).add(obj2_actual.pk) - # Record the role for RBAC changes if 'role' in kwargs: role = kwargs['role'] if role.content_object is not None: obj_rel = '.'.join([role.content_object.__module__, role.content_object.__class__.__name__, role.role_field]) - # If the m2m is from the User side we need to - # set the content_object of the Role for our entry. - if type(instance) == User and role.content_object is not None: - getattr(activity_entry, role.content_type.name.replace(' ', '_')).add(role.content_object) - activity_entry.role.add(role) activity_entry.object_relationship_type = obj_rel activity_entry.save() diff --git a/awx/main/tests/functional/api/test_activity_streams.py b/awx/main/tests/functional/api/test_activity_streams.py index 2d35373a34..ba9b6ed1c6 100644 --- a/awx/main/tests/functional/api/test_activity_streams.py +++ b/awx/main/tests/functional/api/test_activity_streams.py @@ -64,8 +64,8 @@ def test_rbac_stream_resource_roles(activity_stream_entry, organization, org_adm settings.ACTIVITY_STREAM_ENABLED = True assert activity_stream_entry.user.first() == org_admin assert activity_stream_entry.organization.first() == organization - assert activity_stream_entry.role.first() == organization.admin_role - assert activity_stream_entry.object_relationship_type == 'awx.main.models.organization.Organization.admin_role' + assert activity_stream_entry.object1 == 'organization' + assert activity_stream_entry.object2 == 'user' @pytest.mark.django_db @@ -73,8 +73,8 @@ def test_rbac_stream_user_roles(activity_stream_entry, organization, org_admin, settings.ACTIVITY_STREAM_ENABLED = True assert activity_stream_entry.user.first() == org_admin assert activity_stream_entry.organization.first() == organization - assert activity_stream_entry.role.first() == organization.admin_role - assert activity_stream_entry.object_relationship_type == 'awx.main.models.organization.Organization.admin_role' + assert activity_stream_entry.object1 == 'organization' + assert activity_stream_entry.object2 == 'user' @pytest.mark.django_db @@ -169,11 +169,9 @@ def test_stream_user_direct_role_updates(get, post, organization_factory): url = reverse('api:user_roles_list', kwargs={'pk': objects.users.test.pk}) post(url, dict(id=objects.inventories.inv1.read_role.pk), objects.superusers.admin) - activity_stream = ActivityStream.objects.filter( - inventory__pk=objects.inventories.inv1.pk, user__pk=objects.users.test.pk, role__pk=objects.inventories.inv1.read_role.pk - ).first() + activity_stream = ActivityStream.objects.filter(inventory__pk=objects.inventories.inv1.pk, user__pk=objects.users.test.pk, operation='associate').first() url = reverse('api:activity_stream_detail', kwargs={'pk': activity_stream.pk}) response = get(url, objects.users.test) - assert response.data['object1'] == 'user' - assert response.data['object2'] == 'inventory' + assert response.data['object1'] == 'inventory' + assert response.data['object2'] == 'user' diff --git a/awx/main/tests/functional/models/test_activity_stream.py b/awx/main/tests/functional/models/test_activity_stream.py index 8be052628d..3a13bc8ef7 100644 --- a/awx/main/tests/functional/models/test_activity_stream.py +++ b/awx/main/tests/functional/models/test_activity_stream.py @@ -4,7 +4,7 @@ from unittest import mock import json # AWX models -from awx.main.models import ActivityStream, Organization, JobTemplate, Credential, CredentialType, Inventory, InventorySource, Project, User +from awx.main.models import ActivityStream, Organization, JobTemplate, Credential, CredentialType, Inventory, InventorySource, User # other AWX from awx.main.utils import model_to_dict, model_instance_diff @@ -22,8 +22,6 @@ class TestImplicitRolesOmitted: """ Test that there is exactly 1 "create" entry in the activity stream for common items in the system. - These tests will fail if `rbac_activity_stream` creates - false-positive entries. """ @pytest.mark.django_db @@ -68,54 +66,33 @@ class TestImplicitRolesOmitted: @pytest.mark.django_db -class TestRolesAssociationEntries: - """ - Test that non-implicit role associations have a corresponding - activity stream entry. - These tests will fail if `rbac_activity_stream` skipping logic - in signals is wrong. - """ +@pytest.mark.parametrize('value', [True, False]) +def test_auditor_is_recorded(post, value): + u = User.objects.create(username='foouser') + assert not u.is_system_auditor + u.is_system_auditor = value + u = User.objects.get(pk=u.pk) # refresh from db + assert u.is_system_auditor == value + entry_qs = ActivityStream.objects.filter(user=u) + if value: + assert len(entry_qs) == 2 + else: + assert len(entry_qs) == 1 + # unfortunate, the original creation does _not_ set a real is_auditor field + assert 'is_system_auditor' not in json.loads(entry_qs[0].changes) # NOTE: if this fails, see special note + # special note - if system auditor flag is moved to user model then we expect this assertion to be changed + # make sure that an extra entry is not created, expectation for count would change to 1 + if value: + entry = entry_qs[1] + assert json.loads(entry.changes) == {'is_system_auditor': [False, True]} + assert entry.object1 == 'user' - def test_non_implicit_associations_are_recorded(self, project): - org2 = Organization.objects.create(name='test-organization2') - # check that duplicate adds do not get recorded in 2nd loop - for i in range(2): - # Not supported, should not be possible via API - # org2.admin_role.children.add(project.admin_role) - project.admin_role.parents.add(org2.admin_role) - assert ActivityStream.objects.filter(role=org2.admin_role, organization=org2, project=project).count() == 1, 'In loop %s' % i - def test_model_associations_are_recorded(self, organization): - proj1 = Project.objects.create(name='proj1', organization=organization) - proj2 = Project.objects.create(name='proj2', organization=organization) - proj2.use_role.parents.add(proj1.admin_role) - assert ActivityStream.objects.filter(role=proj1.admin_role, project=proj2).count() == 1 - - @pytest.mark.parametrize('value', [True, False]) - def test_auditor_is_recorded(self, post, value): - u = User.objects.create(username='foouser') - assert not u.is_system_auditor - u.is_system_auditor = value - u = User.objects.get(pk=u.pk) # refresh from db - assert u.is_system_auditor == value - entry_qs = ActivityStream.objects.filter(user=u) - if value: - assert len(entry_qs) == 2 - else: - assert len(entry_qs) == 1 - # unfortunate, the original creation does _not_ set a real is_auditor field - assert 'is_system_auditor' not in json.loads(entry_qs[0].changes) # NOTE: if this fails, see special note - # special note - if system auditor flag is moved to user model then we expect this assertion to be changed - # make sure that an extra entry is not created, expectation for count would change to 1 - if value: - entry = entry_qs[1] - assert json.loads(entry.changes) == {'is_system_auditor': [False, True]} - assert entry.object1 == 'user' - - def test_user_no_op_api(self, system_auditor): - as_ct = ActivityStream.objects.count() - system_auditor.is_system_auditor = True # already auditor - assert ActivityStream.objects.count() == as_ct +@pytest.mark.django_db +def test_user_no_op_api(system_auditor): + as_ct = ActivityStream.objects.count() + system_auditor.is_system_auditor = True # already auditor + assert ActivityStream.objects.count() == as_ct @pytest.fixture