diff --git a/awx/main/models/__init__.py b/awx/main/models/__init__.py index 8a71fb79c3..1bb5f87f24 100644 --- a/awx/main/models/__init__.py +++ b/awx/main/models/__init__.py @@ -201,6 +201,8 @@ def user_is_system_auditor(user): @user_is_system_auditor.setter def user_is_system_auditor(user, tf): + from awx.main.signals import disable_activity_stream + if not user.id: # If the user doesn't have a primary key yet (i.e., this is the *first* # time they've logged in, and we've just created the new User in this @@ -210,10 +212,14 @@ def user_is_system_auditor(user, tf): assignment = RoleUserAssignment.objects.filter(user=user, role_definition=rd).first() prior_value = bool(assignment) if prior_value != bool(tf): - if assignment: - assignment.delete() - else: - rd.give_global_permission(user) + # The role assignment change below is already recorded by this function's own + # 'update' entry, which captures the prior/new boolean value. Suppress activity + # stream for the assignment create/delete so it isn't recorded a second time. + with disable_activity_stream(): + if assignment: + assignment.delete() + else: + rd.give_global_permission(user) user._is_system_auditor = bool(tf) entry = ActivityStream.objects.create(changes=json.dumps({"is_system_auditor": [prior_value, bool(tf)]}), object1='user', operation='update') entry.user.add(user) diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 3b88c7794f..c8e1846c51 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -651,16 +651,24 @@ def disable_rbac_sync(): def give_creator_permissions(user, obj): + from awx.main.signals import disable_activity_stream + assignment = RoleDefinition.objects.give_creator_permissions(user, obj) if assignment: with disable_rbac_sync(): old_role = get_role_from_object_role(assignment.object_role) if old_role is None: return - old_role.members.add(user) + # The new-side assignment above is already recorded by + # record_role_assignment_activity_stream. Suppress activity stream for this + # mirrored write so it isn't recorded a second time. + with disable_activity_stream(): + old_role.members.add(user) 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: @@ -684,7 +692,11 @@ 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) - assignment = give_or_remove_permission(role, user, giving=is_giving, rd=rd) + # 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) # sync to resource server if rbac_sync_enabled.enabled: @@ -695,6 +707,8 @@ 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 @@ -735,7 +749,10 @@ 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) - assignment = give_or_remove_permission(child_role, team, giving=is_giving, rd=rd) + # 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) # sync to resource server if rbac_sync_enabled.enabled: diff --git a/awx/main/signals.py b/awx/main/signals.py index fc536eff0c..8159eb4bf4 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -9,7 +9,7 @@ import json import sys # Django -from django.db import connection +from django.db import connection, models from django.conf import settings from django.db.models.signals import ( pre_save, @@ -27,6 +27,9 @@ from django.utils import timezone from crum import get_current_request, get_current_user from crum.signals import current_user_getter +# Ansible_base app +from ansible_base.rbac.models import RoleUserAssignment, RoleTeamAssignment + # AWX from awx.main.models import ( ActivityStream, @@ -180,6 +183,87 @@ def rbac_activity_stream(instance, sender, **kwargs): activity_stream_associate(sender, instance, role=role, **kwargs) +def _record_role_assignment_activity_stream(instance, operation): + if not activity_stream_enabled: + return + # Avoid flooding the activity stream when assignments are cascade-deleted as part + # of setup_managed_role_definitions() pruning stale managed RoleDefinitions on migrate. + if 'migrate' in sys.argv: + return + + if isinstance(instance, RoleUserAssignment): + actor_field = 'user' + actor_obj = instance.user + else: + actor_field = 'team' + actor_obj = instance.team + + content_object = None + if instance.object_id: + content_object = instance.content_object + if content_object is None and operation == 'disassociate': + # The target object is unresolvable, e.g. cascade-deleted along with its + # parent object, or a federated/remote content type. Skip recording a + # contentless entry rather than guessing at what changed. + # Note: FederatedForeignKey swallows ObjectDoesNotExist and returns None + # rather than raising, so there's no exception to catch here. + return + + changes = {'role_definition': instance.role_definition.name, actor_field: getattr(actor_obj, 'username', None) or str(actor_obj)} + + # object1 is the role's content object and object2 the associated user/team, matching + # the convention used for legacy Role.members association entries (rbac_activity_stream + # below), so this reads e.g. "disassociated organization X from user Y". + object1 = '' + activity_stream_cls = get_activity_stream_class() + if content_object is not None: + object1 = camelcase_to_underscore(content_object.__class__.__name__) + changes['object_type'] = object1 + changes['object_id'] = content_object.pk + changes['object_name'] = str(content_object) + obj_rel = f'{content_object.__class__.__module__}.{content_object.__class__.__name__}.{instance.role_definition_id}' + if not hasattr(activity_stream_cls, object1): + logger.warning('ActivityStream has no relation field for object type %r; role assignment entry will not be linked to it', object1) + object1 = '' + else: + obj_rel = str(instance.role_definition_id) + + activity_entry = activity_stream_cls( + operation=operation, + object1=object1, + object2=actor_field, + object_relationship_type=obj_rel, + changes=json.dumps(changes), + actor=get_current_user_or_none(), + ) + activity_entry.save() + getattr(activity_entry, actor_field).add(actor_obj.pk) + if object1: + getattr(activity_entry, object1).add(content_object.pk) + connection.on_commit(lambda: emit_activity_stream_change(activity_entry)) + + +@receiver(post_save, sender=RoleUserAssignment) +@receiver(post_save, sender=RoleTeamAssignment) +def record_role_assignment_activity_stream(instance, created, **kwargs): + if created: + _record_role_assignment_activity_stream(instance, 'associate') + + +@receiver(post_delete, sender=RoleUserAssignment) +@receiver(post_delete, sender=RoleTeamAssignment) +def record_role_unassignment_activity_stream(instance, origin=None, **kwargs): + # Skip cascade deletes from non-assignment origins, same reasoning as + # sync_assignments_to_old_rbac_delete: the actor or content object is itself + # being deleted in the same cascade, so linking a new activity stream entry + # to it would leave a dangling foreign key once the cascade completes. + if isinstance(origin, models.Model) and origin._meta.app_label != 'dab_rbac': + return + if isinstance(origin, models.QuerySet) and origin.model is not type(instance): + return + _record_role_assignment_activity_stream(instance, 'disassociate') + + def cleanup_detached_labels_on_deleted_parent(sender, instance, **kwargs): for label in instance.labels.all(): if label.is_candidate_for_detach(): diff --git a/awx/main/tests/functional/dab_rbac/test_dab_rbac_activity_stream.py b/awx/main/tests/functional/dab_rbac/test_dab_rbac_activity_stream.py new file mode 100644 index 0000000000..29d5966ca9 --- /dev/null +++ b/awx/main/tests/functional/dab_rbac/test_dab_rbac_activity_stream.py @@ -0,0 +1,165 @@ +import json + +import pytest + +from awx.api.versioning import reverse + +from ansible_base.rbac.models import RoleDefinition, RoleUserAssignment +from ansible_base.rbac import permission_registry + +from awx.main.models import ActivityStream, Inventory, Organization + + +@pytest.mark.django_db +class TestCreatorPermissionActivityStream: + ''' + give_creator_permissions grants the new-side RoleUserAssignment and mirrors it into + the legacy Role.members m2m. Only the new-side grant should be recorded - the mirrored + write must not produce a second entry. + ''' + + def test_object_creation_records_single_entry(self, post, rando, organization, setup_managed_roles): + rd, _ = RoleDefinition.objects.get_or_create( + name='inventory-add', + permissions=['add_inventory', 'view_organization'], + content_type=permission_registry.content_type_model.objects.get_for_model(Organization), + ) + rd.give_permission(rando, organization) + + url = reverse('api:inventory_list') + response = post(url=url, data={'name': 'rando-created-inventory', 'organization': organization.id}, user=rando, expect=201) + inventory = Inventory.objects.get(pk=response.data['id']) + + assert rando in inventory.admin_role.members.all() + entries = ActivityStream.objects.filter(operation='associate', user=rando, changes__icontains='Inventory Admin') + assert entries.count() == 1 + entry = entries.get() + assert entry.object1 == 'inventory' + assert entry.object2 == 'user' + + +@pytest.mark.django_db +class TestRoleAssignmentActivityStream: + ''' + Tests that assigning/removing a DAB RBAC role records an ActivityStream entry, + independent of whether the role has a legacy Role equivalent. + ''' + + def test_custom_role_assignment_recorded(self, rando, inventory, setup_managed_roles): + rd, _ = RoleDefinition.objects.get_or_create( + name='inventory-custom-role', + permissions=['view_inventory', 'change_inventory'], + content_type=permission_registry.content_type_model.objects.get_for_model(Inventory), + ) + rd.give_permission(rando, inventory) + + entries = ActivityStream.objects.filter(operation='associate', user=rando, changes__icontains=rd.name) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'user': rando.username, + 'object_type': 'inventory', + 'object_id': inventory.id, + 'object_name': str(inventory), + } + # object1 is the role's content object and object2 the associated actor, matching + # the convention used for legacy Role.members association entries, so this reads + # "disassociated inventory X from user Y" rather than the other way around. + assert entry.object1 == 'inventory' + assert entry.object2 == 'user' + assert entry.object_relationship_type == f'{Inventory.__module__}.Inventory.{rd.id}' + + def test_custom_role_removal_recorded(self, rando, inventory, setup_managed_roles): + rd, _ = RoleDefinition.objects.get_or_create( + name='inventory-custom-role', + permissions=['view_inventory', 'change_inventory'], + content_type=permission_registry.content_type_model.objects.get_for_model(Inventory), + ) + rd.give_permission(rando, inventory) + rd.remove_permission(rando, inventory) + + entries = ActivityStream.objects.filter(operation='disassociate', user=rando, changes__icontains=rd.name) + assert entries.count() == 1 + entry = entries.get() + assert entry.object1 == 'inventory' + assert entry.object2 == 'user' + + def test_custom_role_team_assignment_recorded(self, team, inventory, setup_managed_roles): + rd, _ = RoleDefinition.objects.get_or_create( + name='inventory-custom-role', + permissions=['view_inventory', 'change_inventory'], + content_type=permission_registry.content_type_model.objects.get_for_model(Inventory), + ) + rd.give_permission(team, inventory) + + entries = ActivityStream.objects.filter(operation='associate', team=team, changes__icontains=rd.name) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'team': str(team), + 'object_type': 'inventory', + 'object_id': inventory.id, + 'object_name': str(inventory), + } + assert entry.object1 == 'inventory' + assert entry.object2 == 'team' + + def test_custom_role_team_removal_recorded(self, team, inventory, setup_managed_roles): + rd, _ = RoleDefinition.objects.get_or_create( + name='inventory-custom-role', + permissions=['view_inventory', 'change_inventory'], + content_type=permission_registry.content_type_model.objects.get_for_model(Inventory), + ) + rd.give_permission(team, inventory) + rd.remove_permission(team, inventory) + + entries = ActivityStream.objects.filter(operation='disassociate', team=team, changes__icontains=rd.name) + assert entries.count() == 1 + entry = entries.get() + assert entry.object1 == 'inventory' + assert entry.object2 == 'team' + + def test_global_role_assignment_recorded(self, rando, setup_managed_roles): + rd, _ = RoleDefinition.objects.get_or_create(name='global-view-role', content_type=None) + RoleUserAssignment.objects.create(user=rando, role_definition=rd) + + entries = ActivityStream.objects.filter(operation='associate', user=rando, changes__icontains=rd.name) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == {'role_definition': rd.name, 'user': rando.username} + # No content object for a global role assignment, so object1 is left blank. + assert entry.object1 == '' + assert entry.object2 == 'user' + assert entry.object_relationship_type == str(rd.id) + + def test_cascade_delete_does_not_record_contentless_entry(self, rando, setup_managed_roles): + ''' + Deleting an object cascade-deletes its RoleUserAssignments. By the time the + post_delete handler runs, the object is already gone, so there is nothing + useful to record - it should be skipped rather than logged with no context. + ''' + org = Organization.objects.create(name='cascade-delete-org') + rd = RoleDefinition.objects.get(name='Organization Admin') + rd.give_permission(rando, org) + assert ActivityStream.objects.filter(operation='associate', user=rando, changes__icontains=rd.name).count() == 1 + + org.delete() + + assert ActivityStream.objects.filter(operation='disassociate', user=rando, changes__icontains=rd.name).count() == 0 + + def test_cascade_delete_of_team_does_not_leave_dangling_link(self, team, inventory, setup_managed_roles): + ''' + Deleting a team's organization cascades to delete the team, which cascades to + delete its RoleTeamAssignments. Recording a fresh ActivityStream entry linked to + the team in response to that cascade would leave a dangling foreign key once the + team row is actually removed later in the same cascade - it must be skipped. + ''' + rd = RoleDefinition.objects.get(name='Inventory Admin') + rd.give_permission(team, inventory) + assert ActivityStream.objects.filter(operation='associate', team=team, changes__icontains=rd.name).count() == 1 + + team.organization.delete() + + assert ActivityStream.objects.filter(operation='disassociate', team=team, changes__icontains=rd.name).count() == 0 diff --git a/awx/main/tests/functional/dab_rbac/test_translation_layer.py b/awx/main/tests/functional/dab_rbac/test_translation_layer.py index cf061196a1..538ed90192 100644 --- a/awx/main/tests/functional/dab_rbac/test_translation_layer.py +++ b/awx/main/tests/functional/dab_rbac/test_translation_layer.py @@ -7,7 +7,7 @@ from crum import impersonate from awx.main.fields import ImplicitRoleField from awx.main.models.rbac import get_role_from_object_role, give_creator_permissions, get_role_codenames, get_role_definition -from awx.main.models import User, Organization, WorkflowJobTemplate, WorkflowJobTemplateNode, Team +from awx.main.models import ActivityStream, User, Organization, WorkflowJobTemplate, WorkflowJobTemplateNode, Team from awx.api.versioning import reverse from ansible_base.rbac.models import RoleUserAssignment, RoleDefinition @@ -41,6 +41,22 @@ def test_round_trip_roles(organization, rando, role_name, setup_managed_roles): assert old_role.id == getattr(organization, role_name).id +@pytest.mark.django_db +def test_bulk_member_addition_records_one_entry_per_user(organization, rando, alice, setup_managed_roles): + """ + Adding multiple users to a legacy role in one m2m .add() call syncs each user to the + new RBAC system individually (sync_members_to_new_rbac). The legacy-side add already + records one ActivityStream entry per user via rbac_activity_stream; the new-side mirror + must stay suppressed so this doesn't turn into two entries per user. + """ + organization.admin_role.members.add(rando, alice) + + assert RoleUserAssignment.objects.filter(user=rando, role_definition__name='Organization Admin').exists() + assert RoleUserAssignment.objects.filter(user=alice, role_definition__name='Organization Admin').exists() + assert ActivityStream.objects.filter(operation='associate', user=rando).count() == 1 + assert ActivityStream.objects.filter(operation='associate', user=alice).count() == 1 + + @pytest.mark.django_db @pytest.mark.parametrize('model', sorted(permission_registry.all_registered_models, key=lambda cls: cls._meta.model_name)) def test_role_migration_matches(request, model, setup_managed_roles): @@ -57,11 +73,11 @@ def test_role_migration_matches(request, model, setup_managed_roles): rd = get_role_definition(old_role) new_codenames = set(rd.permissions.values_list('codename', flat=True)) # all the old roles should map to a non-Compat role definition + rd_data = {} if 'Compat' not in rd.name: model_rds = RoleDefinition.objects.filter(content_type=permission_registry.content_type_model.objects.get_for_model(obj)) - rd_data = {} - for rd in model_rds: - rd_data[rd.name] = list(rd.permissions.values_list('codename', flat=True)) + for other_rd in model_rds: + rd_data[other_rd.name] = list(other_rd.permissions.values_list('codename', flat=True)) assert 'Compat' not in rd.name, ( f'Permissions for old vs new roles did not match.\nold {field.name}: {old_codenames}\nnew:\n{json.dumps(rd_data, indent=2)}' ) diff --git a/awx/main/tests/functional/dab_rbac/test_translation_layer_new_to_old.py b/awx/main/tests/functional/dab_rbac/test_translation_layer_new_to_old.py index 779c874e25..a7873fe5ca 100644 --- a/awx/main/tests/functional/dab_rbac/test_translation_layer_new_to_old.py +++ b/awx/main/tests/functional/dab_rbac/test_translation_layer_new_to_old.py @@ -1,9 +1,12 @@ +import json from unittest import mock from ansible_base.rbac.models import ObjectRole, RoleDefinition, RoleUserAssignment, RoleTeamAssignment from ansible_base.lib.utils.response import get_relative_url import pytest +from awx.main.models import ActivityStream + @pytest.mark.django_db class TestNewToOld: @@ -21,6 +24,19 @@ class TestNewToOld: url = get_relative_url('roleuserassignment-list') post(url, user=admin, data={'role_definition': rd.id, 'user': bob.id, 'object_id': inventory.id}, expect=201) assert bob in inventory.admin_role.members.all() + # the new-side assignment should be recorded exactly once, not double-recorded via the old-side mirror + entries = ActivityStream.objects.filter(operation='associate', user=bob) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'user': bob.username, + 'object_type': 'inventory', + 'object_id': inventory.id, + 'object_name': str(inventory), + } + assert entry.object1 == 'inventory' + assert entry.object2 == 'user' def test_new_to_old_rbac_removal(self, admin, delete, inventory, bob, setup_managed_roles): ''' @@ -34,6 +50,18 @@ class TestNewToOld: url = get_relative_url('roleuserassignment-detail', kwargs={'pk': user_assignment.id}) delete(url, user=admin, expect=204) assert bob not in inventory.admin_role.members.all() + entries = ActivityStream.objects.filter(operation='disassociate', user=bob) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'user': bob.username, + 'object_type': 'inventory', + 'object_id': inventory.id, + 'object_name': str(inventory), + } + assert entry.object1 == 'inventory' + assert entry.object2 == 'user' def test_new_to_old_rbac_team_member_addition(self, admin, post, team, bob, setup_managed_roles): ''' @@ -44,6 +72,18 @@ class TestNewToOld: url = get_relative_url('roleuserassignment-list') post(url, user=admin, data={'role_definition': rd.id, 'user': bob.id, 'object_id': team.id}, expect=201) assert bob in team.member_role.members.all() + entries = ActivityStream.objects.filter(operation='associate', user=bob) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'user': bob.username, + 'object_type': 'team', + 'object_id': team.id, + 'object_name': str(team), + } + assert entry.object1 == 'team' + assert entry.object2 == 'user' def test_new_to_old_rbac_team_member_removal(self, admin, delete, team, bob, setup_managed_roles): ''' @@ -57,6 +97,18 @@ class TestNewToOld: url = get_relative_url('roleuserassignment-detail', kwargs={'pk': user_assignment.id}) delete(url, user=admin, expect=204) assert bob not in team.member_role.members.all() + entries = ActivityStream.objects.filter(operation='disassociate', user=bob) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'user': bob.username, + 'object_type': 'team', + 'object_id': team.id, + 'object_name': str(team), + } + assert entry.object1 == 'team' + assert entry.object2 == 'user' def test_new_to_old_rbac_team_addition(self, admin, post, team, inventory, setup_managed_roles): ''' @@ -67,6 +119,18 @@ class TestNewToOld: url = get_relative_url('roleteamassignment-list') post(url, user=admin, data={'role_definition': rd.id, 'team': team.id, 'object_id': inventory.id}, expect=201) assert team.member_role in inventory.admin_role.parents.all() + entries = ActivityStream.objects.filter(operation='associate', team=team) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'team': str(team), + 'object_type': 'inventory', + 'object_id': inventory.id, + 'object_name': str(inventory), + } + assert entry.object1 == 'inventory' + assert entry.object2 == 'team' def test_new_to_old_rbac_team_removal(self, admin, delete, team, inventory, setup_managed_roles): ''' @@ -80,6 +144,18 @@ class TestNewToOld: url = get_relative_url('roleteamassignment-detail', kwargs={'pk': team_assignment.id}) delete(url, user=admin, expect=204) assert team.member_role not in inventory.admin_role.parents.all() + entries = ActivityStream.objects.filter(operation='disassociate', team=team) + assert entries.count() == 1 + entry = entries.get() + assert json.loads(entry.changes) == { + 'role_definition': rd.name, + 'team': str(team), + 'object_type': 'inventory', + 'object_id': inventory.id, + 'object_name': str(inventory), + } + assert entry.object1 == 'inventory' + assert entry.object2 == 'team' def test_flush_rbac_cleanup_skips_sync(self, inventory, bob, setup_managed_roles): """Simulate what defer_rbac_computations._flush_rbac does on exit: