From 9c7120443556b90586da2654b2d105cec4c15a56 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Mon, 3 Dec 2018 14:08:23 -0500 Subject: [PATCH] show activity stream entry for system auditor association --- awx/api/serializers.py | 18 ++++++- awx/api/views/__init__.py | 15 ------ awx/main/fields.py | 12 ++++- awx/main/models/__init__.py | 9 +++- awx/main/models/activity_stream.py | 5 ++ awx/main/signals.py | 20 +++++--- .../functional/models/test_activity_stream.py | 47 +++++++++++++++---- awx/main/utils/common.py | 6 +-- 8 files changed, 92 insertions(+), 40 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 19b40a82f7..1f6cf2818d 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -893,7 +893,7 @@ class UserSerializer(BaseSerializer): def get_validation_exclusions(self, obj=None): ret = super(UserSerializer, self).get_validation_exclusions(obj) - ret.append('password') + ret.extend(['password', 'is_system_auditor']) return ret def validate_password(self, value): @@ -937,14 +937,20 @@ class UserSerializer(BaseSerializer): def create(self, validated_data): new_password = validated_data.pop('password', None) + is_system_auditor = validated_data.pop('is_system_auditor', None) obj = super(UserSerializer, self).create(validated_data) self._update_password(obj, new_password) + if is_system_auditor is not None: + obj.is_system_auditor = is_system_auditor return obj def update(self, obj, validated_data): new_password = validated_data.pop('password', None) + is_system_auditor = validated_data.pop('is_system_auditor', None) obj = super(UserSerializer, self).update(obj, validated_data) self._update_password(obj, new_password) + if is_system_auditor is not None: + obj.is_system_auditor = is_system_auditor return obj def get_related(self, obj): @@ -996,6 +1002,16 @@ class UserSerializer(BaseSerializer): return self._validate_ldap_managed_field(value, 'is_superuser') +class UserActivityStreamSerializer(UserSerializer): + """Changes to system auditor status are shown as separate entries, + so by excluding it from fields here we avoid duplication, which + would carry some unintended consequences. + """ + class Meta: + model = User + fields = ('*', '-is_system_auditor') + + class BaseOAuth2TokenSerializer(BaseSerializer): refresh_token = serializers.SerializerMethodField() diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 3aa1b387b0..bdf93b888c 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -922,21 +922,6 @@ class UserList(ListCreateAPIView): serializer_class = serializers.UserSerializer permission_classes = (UserPermission,) - def post(self, request, *args, **kwargs): - ret = super(UserList, self).post( request, *args, **kwargs) - try: - if request.data.get('is_system_auditor', False): - # This is a faux-field that just maps to checking the system - # auditor role member list.. unfortunately this means we can't - # set it on creation, and thus needs to be set here. - user = models.User.objects.get(id=ret.data['id']) - user.is_system_auditor = request.data['is_system_auditor'] - ret.data['is_system_auditor'] = request.data['is_system_auditor'] - except AttributeError as exc: - print(exc) - pass - return ret - class UserMeList(ListAPIView): diff --git a/awx/main/fields.py b/awx/main/fields.py index 7f77d5632a..963723319d 100644 --- a/awx/main/fields.py +++ b/awx/main/fields.py @@ -43,7 +43,10 @@ from rest_framework import serializers from awx.main.utils.filters import SmartFilter from awx.main.utils.encryption import encrypt_value, decrypt_value, get_encryption_key from awx.main.validators import validate_ssh_private_key -from awx.main.models.rbac import batch_role_ancestor_rebuilding, Role +from awx.main.models.rbac import ( + batch_role_ancestor_rebuilding, Role, + ROLE_SINGLETON_SYSTEM_ADMINISTRATOR, ROLE_SINGLETON_SYSTEM_AUDITOR +) from awx.main.constants import ENV_BLACKLIST from awx.main import utils @@ -159,6 +162,13 @@ def is_implicit_parent(parent_role, child_role): the model definition. This does not include any role parents that might have been set by the user. ''' + if child_role.content_object is None: + # The only singleton implicit parent is the system admin being + # a parent of the system auditor role + return bool( + child_role.singleton_name == ROLE_SINGLETON_SYSTEM_AUDITOR and + parent_role.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR + ) # Get the list of implicit parents that were defined at the class level. implicit_parents = getattr( child_role.content_object.__class__, child_role.role_field diff --git a/awx/main/models/__init__.py b/awx/main/models/__init__.py index 67b686aaac..337e44bf42 100644 --- a/awx/main/models/__init__.py +++ b/awx/main/models/__init__.py @@ -142,10 +142,15 @@ def user_is_system_auditor(user): def user_is_system_auditor(user, tf): if user.id: if tf: - Role.singleton('system_auditor').members.add(user) + role = Role.singleton('system_auditor') + # must check if member to not duplicate activity stream + if user not in role.members.all(): + role.members.add(user) user._is_system_auditor = True else: - Role.singleton('system_auditor').members.remove(user) + role = Role.singleton('system_auditor') + if user in role.members.all(): + role.members.remove(user) user._is_system_auditor = False diff --git a/awx/main/models/activity_stream.py b/awx/main/models/activity_stream.py index bd983dc58b..4cbf79f5dc 100644 --- a/awx/main/models/activity_stream.py +++ b/awx/main/models/activity_stream.py @@ -76,6 +76,11 @@ class ActivityStream(models.Model): setting = JSONField(blank=True) + def __str__(self): + operation = self.operation if 'operation' in self.__dict__ else '_delayed_' + timestamp = self.timestamp.isoformat() if 'timestamp' in self.__dict__ else '_delayed_' + return u'%s-%s-pk=%s' % (operation, timestamp, self.pk) + def get_absolute_url(self, request=None): return reverse('api:activity_stream_detail', kwargs={'pk': self.pk}, request=request) diff --git a/awx/main/signals.py b/awx/main/signals.py index db913a5251..ab62d27d98 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -20,7 +20,6 @@ from django.db.models.signals import ( ) from django.dispatch import receiver from django.contrib.auth import SESSION_KEY -from django.contrib.contenttypes.models import ContentType from django.contrib.sessions.models import Session from django.utils import timezone @@ -192,22 +191,28 @@ def sync_superuser_status_to_rbac(instance, **kwargs): def rbac_activity_stream(instance, sender, **kwargs): - user_type = ContentType.objects.get_for_model(User) # Only if we are associating/disassociating if kwargs['action'] in ['pre_add', 'pre_remove']: - # Only if this isn't for the User.admin_role - if hasattr(instance, 'content_type'): - if instance.content_type in [None, user_type]: + 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 - elif sender.__name__ == 'Role_parents': + # 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 - instance = instance.content_object + # 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) @@ -403,6 +408,7 @@ def model_serializer_mapping(): from awx.conf.serializers import SettingSerializer return { Setting: SettingSerializer, + models.User: serializers.UserActivityStreamSerializer, models.Organization: serializers.OrganizationSerializer, models.Inventory: serializers.InventorySerializer, models.Host: serializers.HostSerializer, diff --git a/awx/main/tests/functional/models/test_activity_stream.py b/awx/main/tests/functional/models/test_activity_stream.py index 707b86645c..d218d18a18 100644 --- a/awx/main/tests/functional/models/test_activity_stream.py +++ b/awx/main/tests/functional/models/test_activity_stream.py @@ -11,7 +11,8 @@ from awx.main.models import ( Credential, CredentialType, Inventory, - InventorySource + InventorySource, + User ) # other AWX @@ -75,31 +76,57 @@ class TestImplicitRolesOmitted: assert qs[0].operation == 'create' +@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 - finds a false-negative. + in signals is wrong. ''' - @pytest.mark.django_db def test_non_implicit_associations_are_recorded(self, project): org2 = Organization.objects.create(name='test-organization2') - project.admin_role.parents.add(org2.admin_role) - assert ActivityStream.objects.filter( - role=org2.admin_role, - organization=org2, - project=project - ).count() == 1 + # 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 - @pytest.mark.django_db def test_model_associations_are_recorded(self, organization): proj1 = organization.projects.create(name='proj1') proj2 = organization.projects.create(name='proj2') 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) + if value: + auditor_changes = json.loads(entry_qs[1].changes) + assert auditor_changes['object2'] == 'user' + assert auditor_changes['object2_pk'] == u.pk + + 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.fixture diff --git a/awx/main/utils/common.py b/awx/main/utils/common.py index a9c72677c0..98b3551a37 100644 --- a/awx/main/utils/common.py +++ b/awx/main/utils/common.py @@ -423,10 +423,8 @@ def model_to_dict(obj, serializer_mapping=None): allowed_fields = get_allowed_fields(obj, serializer_mapping) - for field in obj._meta.fields: - if field.name not in allowed_fields: - continue - attr_d[field.name] = _convert_model_field_for_display(obj, field.name, password_fields=password_fields) + for field_name in allowed_fields: + attr_d[field_name] = _convert_model_field_for_display(obj, field_name, password_fields=password_fields) return attr_d