From b3f15a1e61921a71734df60e05b69c608bb1c001 Mon Sep 17 00:00:00 2001 From: Rebeccah Date: Mon, 23 Nov 2020 11:17:21 -0500 Subject: [PATCH 1/4] added function in signals to corroborate the RBAC to the database, prior it was only corroborating from the DB to RBAC and we need both ways --- awx/main/signals.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/awx/main/signals.py b/awx/main/signals.py index adfbc65d01..de89411ea2 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -121,6 +121,15 @@ def sync_superuser_status_to_rbac(instance, **kwargs): Role.singleton(ROLE_SINGLETON_SYSTEM_ADMINISTRATOR).members.remove(instance) +def sync_rbac_to_superuser_status(instance, sender, **kwargs): + 'When the is_superuser flag is false but a user has the System Admin role, update the database to reflect that' + if kwargs['action'] in ['pre_add', 'pre_remove']: + if hasattr(instance, 'content_type'): + if instance.content_type_id is None and instance.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR and kwargs['model'].is_superuser == False: + User.objects.filter(pk=kwargs['pk_set'].pop()).update(is_superuser = (kwargs['action'] == 'pre_add')) + + + def rbac_activity_stream(instance, sender, **kwargs): # Only if we are associating/disassociating if kwargs['action'] in ['pre_add', 'pre_remove']: @@ -197,6 +206,7 @@ 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) pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJobTemplate) From ee111be2615f99148d6a3298b315986c2aef4022 Mon Sep 17 00:00:00 2001 From: Rebeccah Date: Thu, 10 Dec 2020 12:26:20 -0500 Subject: [PATCH 2/4] move away from signals towards the origin of the POST to see if I can impact the data sent within the POST so that it can impact the User model, this may not work because the POST is related only to the Roles model --- awx/api/views/__init__.py | 11 ++++++++--- awx/main/signals.py | 16 +++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 43e845af0c..42edb07333 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -1079,7 +1079,7 @@ class UserTeamsList(SubListAPIView): class UserRolesList(SubListAttachDetachAPIView): - + # view of the roles that a user has associated with their id model = models.Role serializer_class = serializers.RoleSerializerWithParentAccess metadata_class = RoleMetadata @@ -1099,6 +1099,7 @@ class UserRolesList(SubListAttachDetachAPIView): ).exclude(content_type=content_type, object_id=u.id) def post(self, request, *args, **kwargs): + ret = super(UserRolesList, self).post(request, *args, **kwargs) sub_id = request.data.get('id', None) if not sub_id: return super(UserRolesList, self).post(request) @@ -1107,6 +1108,7 @@ class UserRolesList(SubListAttachDetachAPIView): role = get_object_or_400(models.Role, pk=sub_id) credential_content_type = ContentType.objects.get_for_model(models.Credential) + if role.content_type == credential_content_type: if 'disassociate' not in request.data and role.content_object.organization and user not in role.content_object.organization.member_role: data = dict(msg=_("You cannot grant credential access to a user not in the credentials' organization")) @@ -1115,7 +1117,10 @@ class UserRolesList(SubListAttachDetachAPIView): if not role.content_object.organization and not request.user.is_superuser: data = dict(msg=_("You cannot grant private credential access to another user")) return Response(data, status=status.HTTP_400_BAD_REQUEST) - + if request.data.get('id', None) == 1: + request.data['role_field'] = "System Administrator" + request.data["is_superuser"] = True + # this won't work because it doesn't impact the user model, which is where `is_superuser` is found and is what needs to be changed return super(UserRolesList, self).post(request, *args, **kwargs) @@ -4359,7 +4364,7 @@ class RoleDetail(RetrieveAPIView): class RoleUsersList(SubListAttachDetachAPIView): - + # view of all the users that are within a role model = models.User serializer_class = serializers.UserSerializer parent_model = models.Role diff --git a/awx/main/signals.py b/awx/main/signals.py index de89411ea2..3ecba41b78 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -121,12 +121,14 @@ def sync_superuser_status_to_rbac(instance, **kwargs): Role.singleton(ROLE_SINGLETON_SYSTEM_ADMINISTRATOR).members.remove(instance) -def sync_rbac_to_superuser_status(instance, sender, **kwargs): - 'When the is_superuser flag is false but a user has the System Admin role, update the database to reflect that' - if kwargs['action'] in ['pre_add', 'pre_remove']: - if hasattr(instance, 'content_type'): - if instance.content_type_id is None and instance.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR and kwargs['model'].is_superuser == False: - User.objects.filter(pk=kwargs['pk_set'].pop()).update(is_superuser = (kwargs['action'] == 'pre_add')) +# def sync_rbac_to_superuser_status(instance, sender, **kwargs): +# 'When the is_superuser flag is false but a user has the System Admin role, update the database to reflect that' +# if kwargs['action'] in ['pre_add', 'pre_remove']: +# if hasattr(instance, 'content_type'): +# import sdb; +# sdb.set_trace() +# if instance.content_type_id is None and instance.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR and kwargs['model'].is_superuser == False: +# User.objects.filter(pk=kwargs['pk_set'].pop()).update(is_superuser = (kwargs['action'] == 'pre_add')) @@ -206,7 +208,7 @@ 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) +#m2m_changed.connect(sync_rbac_to_superuser_status, Role.members.through) pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJob) pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJobTemplate) From d27afe96913528867be2100265bc256f86cdd708 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Thu, 10 Dec 2020 13:59:46 -0500 Subject: [PATCH 3/4] Small tweaks to logic to make is_superuser change take effect --- awx/api/views/__init__.py | 11 +++-------- awx/main/signals.py | 17 +++++++---------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 42edb07333..43e845af0c 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -1079,7 +1079,7 @@ class UserTeamsList(SubListAPIView): class UserRolesList(SubListAttachDetachAPIView): - # view of the roles that a user has associated with their id + model = models.Role serializer_class = serializers.RoleSerializerWithParentAccess metadata_class = RoleMetadata @@ -1099,7 +1099,6 @@ class UserRolesList(SubListAttachDetachAPIView): ).exclude(content_type=content_type, object_id=u.id) def post(self, request, *args, **kwargs): - ret = super(UserRolesList, self).post(request, *args, **kwargs) sub_id = request.data.get('id', None) if not sub_id: return super(UserRolesList, self).post(request) @@ -1108,7 +1107,6 @@ class UserRolesList(SubListAttachDetachAPIView): role = get_object_or_400(models.Role, pk=sub_id) credential_content_type = ContentType.objects.get_for_model(models.Credential) - if role.content_type == credential_content_type: if 'disassociate' not in request.data and role.content_object.organization and user not in role.content_object.organization.member_role: data = dict(msg=_("You cannot grant credential access to a user not in the credentials' organization")) @@ -1117,10 +1115,7 @@ class UserRolesList(SubListAttachDetachAPIView): if not role.content_object.organization and not request.user.is_superuser: data = dict(msg=_("You cannot grant private credential access to another user")) return Response(data, status=status.HTTP_400_BAD_REQUEST) - if request.data.get('id', None) == 1: - request.data['role_field'] = "System Administrator" - request.data["is_superuser"] = True - # this won't work because it doesn't impact the user model, which is where `is_superuser` is found and is what needs to be changed + return super(UserRolesList, self).post(request, *args, **kwargs) @@ -4364,7 +4359,7 @@ class RoleDetail(RetrieveAPIView): class RoleUsersList(SubListAttachDetachAPIView): - # view of all the users that are within a role + model = models.User serializer_class = serializers.UserSerializer parent_model = models.Role diff --git a/awx/main/signals.py b/awx/main/signals.py index 3ecba41b78..34d0003ab9 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -121,15 +121,12 @@ def sync_superuser_status_to_rbac(instance, **kwargs): Role.singleton(ROLE_SINGLETON_SYSTEM_ADMINISTRATOR).members.remove(instance) -# def sync_rbac_to_superuser_status(instance, sender, **kwargs): -# 'When the is_superuser flag is false but a user has the System Admin role, update the database to reflect that' -# if kwargs['action'] in ['pre_add', 'pre_remove']: -# if hasattr(instance, 'content_type'): -# import sdb; -# sdb.set_trace() -# if instance.content_type_id is None and instance.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR and kwargs['model'].is_superuser == False: -# User.objects.filter(pk=kwargs['pk_set'].pop()).update(is_superuser = (kwargs['action'] == 'pre_add')) - +def sync_rbac_to_superuser_status(instance, sender, **kwargs): + 'When the is_superuser flag is false but a user has the System Admin role, update the database to reflect that' + if kwargs['action'] in ['post_add', 'post_remove']: + if instance.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR: + new_status_value = bool(kwargs['action'] == 'post_add') + kwargs['model'].objects.filter(pk__in=kwargs['pk_set']).update(is_superuser=new_status_value) def rbac_activity_stream(instance, sender, **kwargs): @@ -208,7 +205,7 @@ 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) +m2m_changed.connect(sync_rbac_to_superuser_status, Role.members.through) pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJob) pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJobTemplate) From da3e521566c4303c84419e87b3fee1f15fec20b2 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Thu, 10 Dec 2020 14:36:53 -0500 Subject: [PATCH 4/4] Fix the reverse association and edge cases --- awx/main/signals.py | 21 ++++++++++--- awx/main/tests/functional/test_rbac_user.py | 34 +++++++++++++++++++-- 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/awx/main/signals.py b/awx/main/signals.py index 34d0003ab9..0a29fa9d6c 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -123,10 +123,23 @@ def sync_superuser_status_to_rbac(instance, **kwargs): def sync_rbac_to_superuser_status(instance, sender, **kwargs): 'When the is_superuser flag is false but a user has the System Admin role, update the database to reflect that' - if kwargs['action'] in ['post_add', 'post_remove']: - if instance.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR: - new_status_value = bool(kwargs['action'] == 'post_add') - kwargs['model'].objects.filter(pk__in=kwargs['pk_set']).update(is_superuser=new_status_value) + if kwargs['action'] in ['post_add', 'post_remove', 'post_clear']: + new_status_value = bool(kwargs['action'] == 'post_add') + if hasattr(instance, 'singleton_name'): # duck typing, role.members.add() vs user.roles.add() + role = instance + if role.singleton_name == ROLE_SINGLETON_SYSTEM_ADMINISTRATOR: + if kwargs['pk_set']: + kwargs['model'].objects.filter(pk__in=kwargs['pk_set']).update(is_superuser=new_status_value) + elif kwargs['action'] == 'post_clear': + kwargs['model'].objects.all().update(is_superuser=False) + else: + user = instance + if kwargs['action'] == 'post_clear': + user.is_superuser = False + user.save(update_fields=['is_superuser']) + elif kwargs['model'].objects.filter(pk__in=kwargs['pk_set'], singleton_name=ROLE_SINGLETON_SYSTEM_ADMINISTRATOR).exists(): + user.is_superuser = new_status_value + user.save(update_fields=['is_superuser']) def rbac_activity_stream(instance, sender, **kwargs): diff --git a/awx/main/tests/functional/test_rbac_user.py b/awx/main/tests/functional/test_rbac_user.py index ca3d268b18..b62a0db25f 100644 --- a/awx/main/tests/functional/test_rbac_user.py +++ b/awx/main/tests/functional/test_rbac_user.py @@ -4,7 +4,7 @@ from unittest import mock from django.test import TransactionTestCase from awx.main.access import UserAccess, RoleAccess, TeamAccess -from awx.main.models import User, Organization, Inventory +from awx.main.models import User, Organization, Inventory, Role class TestSysAuditorTransactional(TransactionTestCase): @@ -170,4 +170,34 @@ def test_org_admin_cannot_delete_member_attached_to_other_group(org_admin, org_m access = UserAccess(org_admin) other_org.member_role.members.add(org_member) assert not access.can_delete(org_member) - \ No newline at end of file + + +@pytest.mark.parametrize('reverse', (True, False)) +@pytest.mark.django_db +def test_consistency_of_is_superuser_flag(reverse): + users = [User.objects.create(username='rando_{}'.format(i)) for i in range(2)] + for u in users: + assert u.is_superuser is False + + system_admin = Role.singleton('system_administrator') + if reverse: + for u in users: + u.roles.add(system_admin) + else: + system_admin.members.add(*[u.id for u in users]) # like .add(42, 54) + + for u in users: + u.refresh_from_db() + assert u.is_superuser is True + + users[0].roles.clear() + for u in users: + u.refresh_from_db() + assert users[0].is_superuser is False + assert users[1].is_superuser is True + + system_admin.members.clear() + + for u in users: + u.refresh_from_db() + assert u.is_superuser is False