From 06bb8871d787260d4153aa495ccd795fd80c997b Mon Sep 17 00:00:00 2001 From: Wayne Witzel III Date: Thu, 28 Jul 2016 19:26:30 -0400 Subject: [PATCH 1/6] do not allow membership changes to User.admin_role --- awx/api/views.py | 7 ++++++- awx/main/tests/functional/api/test_user.py | 10 ++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/awx/api/views.py b/awx/api/views.py index 1edeb4eb39..0fd632aa5a 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -1208,7 +1208,12 @@ class UserRolesList(SubListCreateAttachDetachAPIView): return Response(data, status=status.HTTP_400_BAD_REQUEST) if sub_id == self.request.user.admin_role.pk: - raise PermissionDenied('You may not remove your own admin_role.') + raise PermissionDenied('You may not perform any action with your own admin_role.') + + role = get_object_or_404(Role, pk=sub_id) + user_content_type = ContentType.objects.get_for_model(User) + if role.content_type == user_content_type: + raise PermissionDenied('You may not change the membership of a users admin_role') return super(UserRolesList, self).post(request, *args, **kwargs) diff --git a/awx/main/tests/functional/api/test_user.py b/awx/main/tests/functional/api/test_user.py index d739d417c0..4ebd46f225 100644 --- a/awx/main/tests/functional/api/test_user.py +++ b/awx/main/tests/functional/api/test_user.py @@ -66,3 +66,13 @@ def test_create_delete_create_user(post, delete, admin): }, admin) print(response.data) assert response.status_code == 201 + +@pytest.mark.django_db +def test_add_user_admin_role_member(post, user): + admin = user('admin', is_superuser=True) + normal = user('normal') + + url = reverse('api:user_roles_list', args=(admin.pk,)) + response = post(url, {'id':normal.admin_role.pk}, admin) + assert response.status_code == 403 + assert 'not change membership' in response.rendered_content From a431ac785407fad120b4adcbde96060992dc96a0 Mon Sep 17 00:00:00 2001 From: Wayne Witzel III Date: Thu, 28 Jul 2016 20:49:31 -0400 Subject: [PATCH 2/6] fix test --- awx/main/tests/functional/api/test_user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awx/main/tests/functional/api/test_user.py b/awx/main/tests/functional/api/test_user.py index 4ebd46f225..027acc0703 100644 --- a/awx/main/tests/functional/api/test_user.py +++ b/awx/main/tests/functional/api/test_user.py @@ -75,4 +75,4 @@ def test_add_user_admin_role_member(post, user): url = reverse('api:user_roles_list', args=(admin.pk,)) response = post(url, {'id':normal.admin_role.pk}, admin) assert response.status_code == 403 - assert 'not change membership' in response.rendered_content + assert 'not change the membership' in response.rendered_content From 52865eea6a0d745c0f38033daa5145ed83f7a265 Mon Sep 17 00:00:00 2001 From: Wayne Witzel III Date: Fri, 29 Jul 2016 09:42:04 -0400 Subject: [PATCH 3/6] restrict User.admin_role membership changes through RoleUsersList --- awx/api/views.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/awx/api/views.py b/awx/api/views.py index 0fd632aa5a..65fbc03e64 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -3653,6 +3653,15 @@ class RoleUsersList(SubListCreateAttachDetachAPIView): if not sub_id: data = dict(msg="User 'id' field is missing.") return Response(data, status=status.HTTP_400_BAD_REQUEST) + + role = self.get_parent_object() + if role == self.request.user.admin_role: + raise PermissionDenied('You may not perform any action with your own admin_role.') + + user_content_type = ContentType.objects.get_for_model(User) + if role.content_type == user_content_type: + raise PermissionDenied('You may not change the membership of a users admin_role') + return super(RoleUsersList, self).post(request, *args, **kwargs) From b127e74ae414bcda79f3b501ce5d3af13c6be8c0 Mon Sep 17 00:00:00 2001 From: Wayne Witzel III Date: Fri, 29 Jul 2016 10:37:49 -0400 Subject: [PATCH 4/6] refactor to unit tests --- awx/api/views.py | 2 +- awx/main/tests/functional/api/test_user.py | 10 --- awx/main/tests/unit/api/test_roles.py | 77 ++++++++++++++++++++++ 3 files changed, 78 insertions(+), 11 deletions(-) create mode 100644 awx/main/tests/unit/api/test_roles.py diff --git a/awx/api/views.py b/awx/api/views.py index 65fbc03e64..23a4c8cc64 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -1210,7 +1210,7 @@ class UserRolesList(SubListCreateAttachDetachAPIView): if sub_id == self.request.user.admin_role.pk: raise PermissionDenied('You may not perform any action with your own admin_role.') - role = get_object_or_404(Role, pk=sub_id) + role = Role.objects.get(pk=sub_id) user_content_type = ContentType.objects.get_for_model(User) if role.content_type == user_content_type: raise PermissionDenied('You may not change the membership of a users admin_role') diff --git a/awx/main/tests/functional/api/test_user.py b/awx/main/tests/functional/api/test_user.py index 027acc0703..d739d417c0 100644 --- a/awx/main/tests/functional/api/test_user.py +++ b/awx/main/tests/functional/api/test_user.py @@ -66,13 +66,3 @@ def test_create_delete_create_user(post, delete, admin): }, admin) print(response.data) assert response.status_code == 201 - -@pytest.mark.django_db -def test_add_user_admin_role_member(post, user): - admin = user('admin', is_superuser=True) - normal = user('normal') - - url = reverse('api:user_roles_list', args=(admin.pk,)) - response = post(url, {'id':normal.admin_role.pk}, admin) - assert response.status_code == 403 - assert 'not change the membership' in response.rendered_content diff --git a/awx/main/tests/unit/api/test_roles.py b/awx/main/tests/unit/api/test_roles.py new file mode 100644 index 0000000000..e15e691af9 --- /dev/null +++ b/awx/main/tests/unit/api/test_roles.py @@ -0,0 +1,77 @@ +import mock +from mock import PropertyMock + +import pytest + +from rest_framework.test import APIRequestFactory +from rest_framework.test import force_authenticate + +from django.contrib.contenttypes.models import ContentType + +from awx.api.views import ( + RoleUsersList, + UserRolesList, +) + +from awx.main.models import ( + User, + Role, +) + +@pytest.mark.parametrize("pk, err", [ + (111, "not change the membership"), + (1, "may not perform"), +]) +def test_user_roles_list_user_admin_role(pk, err): + with mock.patch('awx.api.views.Role.objects.get') as role_get, \ + mock.patch('awx.api.views.ContentType.objects.get_for_model') as ct_get: + + role_mock = mock.MagicMock(spec=Role, id=1, pk=1) + content_type_mock = mock.MagicMock(spec=ContentType) + role_mock.content_type = content_type_mock + role_get.return_value = role_mock + ct_get.return_value = content_type_mock + + with mock.patch('awx.api.views.User.admin_role', new_callable=PropertyMock, return_value=role_mock): + factory = APIRequestFactory() + view = UserRolesList.as_view() + + user = User(username="root", is_superuser=True) + + request = factory.post("/user/1/roles", {'id':pk}, format="json") + force_authenticate(request, user) + + response = view(request) + response.render() + + assert response.status_code == 403 + assert err in response.content + +@pytest.mark.parametrize("admin_role, err", [ + (True, "may not perform"), + (False, "not change the membership"), +]) +def test_role_users_list_other_user_admin_role(admin_role, err): + with mock.patch('awx.api.views.RoleUsersList.get_parent_object') as role_get, \ + mock.patch('awx.api.views.ContentType.objects.get_for_model') as ct_get: + + role_mock = mock.MagicMock(spec=Role, id=1) + content_type_mock = mock.MagicMock(spec=ContentType) + role_mock.content_type = content_type_mock + role_get.return_value = role_mock + ct_get.return_value = content_type_mock + + user_admin_role = role_mock if admin_role else None + with mock.patch('awx.api.views.User.admin_role', new_callable=PropertyMock, return_value=user_admin_role): + factory = APIRequestFactory() + view = RoleUsersList.as_view() + + user = User(username="root", is_superuser=True, pk=1, id=1) + request = factory.post("/role/1/users", {'id':1}, format="json") + force_authenticate(request, user) + + response = view(request) + response.render() + + assert response.status_code == 403 + assert err in response.content From 9baa9594c7284273abe40341b9ab89207927fb66 Mon Sep 17 00:00:00 2001 From: Wayne Witzel III Date: Fri, 29 Jul 2016 11:19:43 -0400 Subject: [PATCH 5/6] use get_object_or_400 to fetch Role --- awx/api/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awx/api/views.py b/awx/api/views.py index 23a4c8cc64..32fd3c5b45 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -1210,7 +1210,7 @@ class UserRolesList(SubListCreateAttachDetachAPIView): if sub_id == self.request.user.admin_role.pk: raise PermissionDenied('You may not perform any action with your own admin_role.') - role = Role.objects.get(pk=sub_id) + role = get_object_or_400(Role, pk=sub_id) user_content_type = ContentType.objects.get_for_model(User) if role.content_type == user_content_type: raise PermissionDenied('You may not change the membership of a users admin_role') From b862a13d92ad68e0d1a9609fe77fc208b94f9351 Mon Sep 17 00:00:00 2001 From: Wayne Witzel III Date: Fri, 29 Jul 2016 11:20:05 -0400 Subject: [PATCH 6/6] update unit tetsts --- awx/main/tests/unit/api/test_roles.py | 25 +++++++++++++++++- awx/main/tests/unit/api/test_views.py | 37 +-------------------------- 2 files changed, 25 insertions(+), 37 deletions(-) diff --git a/awx/main/tests/unit/api/test_roles.py b/awx/main/tests/unit/api/test_roles.py index e15e691af9..7a5112ceae 100644 --- a/awx/main/tests/unit/api/test_roles.py +++ b/awx/main/tests/unit/api/test_roles.py @@ -11,6 +11,7 @@ from django.contrib.contenttypes.models import ContentType from awx.api.views import ( RoleUsersList, UserRolesList, + TeamRolesList, ) from awx.main.models import ( @@ -23,7 +24,7 @@ from awx.main.models import ( (1, "may not perform"), ]) def test_user_roles_list_user_admin_role(pk, err): - with mock.patch('awx.api.views.Role.objects.get') as role_get, \ + with mock.patch('awx.api.views.get_object_or_400') as role_get, \ mock.patch('awx.api.views.ContentType.objects.get_for_model') as ct_get: role_mock = mock.MagicMock(spec=Role, id=1, pk=1) @@ -75,3 +76,25 @@ def test_role_users_list_other_user_admin_role(admin_role, err): assert response.status_code == 403 assert err in response.content + +def test_team_roles_list_post_org_roles(): + with mock.patch('awx.api.views.get_object_or_400') as role_get, \ + mock.patch('awx.api.views.ContentType.objects.get_for_model') as ct_get: + + role_mock = mock.MagicMock(spec=Role) + content_type_mock = mock.MagicMock(spec=ContentType) + role_mock.content_type = content_type_mock + role_get.return_value = role_mock + ct_get.return_value = content_type_mock + + factory = APIRequestFactory() + view = TeamRolesList.as_view() + + request = factory.post("/team/1/roles", {'id':1}, format="json") + force_authenticate(request, User(username="root", is_superuser=True)) + + response = view(request) + response.render() + + assert response.status_code == 400 + assert 'cannot assign' in response.content diff --git a/awx/main/tests/unit/api/test_views.py b/awx/main/tests/unit/api/test_views.py index c667be6450..6a97831f02 100644 --- a/awx/main/tests/unit/api/test_views.py +++ b/awx/main/tests/unit/api/test_views.py @@ -1,22 +1,11 @@ import mock import pytest -from rest_framework.test import APIRequestFactory -from rest_framework.test import force_authenticate - -from django.contrib.contenttypes.models import ContentType - from awx.api.views import ( ApiV1RootView, - TeamRolesList, JobTemplateLabelList, ) -from awx.main.models import ( - User, - Role, -) - @pytest.fixture def mock_response_new(mocker): m = mocker.patch('awx.api.views.Response.__new__') @@ -68,30 +57,6 @@ class TestJobTemplateLabelList: with mock.patch('awx.api.generics.DeleteLastUnattachLabelMixin.unattach') as mixin_unattach: view = JobTemplateLabelList() mock_request = mock.MagicMock() - + super(JobTemplateLabelList, view).unattach(mock_request, None, None) assert mixin_unattach.called_with(mock_request, None, None) - -@pytest.mark.parametrize("url", ["/team/1/roles", "/role/1/teams"]) -def test_team_roles_list_post_org_roles(url): - with mock.patch('awx.api.views.get_object_or_400') as mock_get_obj, \ - mock.patch('awx.api.views.ContentType.objects.get_for_model') as ct_get: - - role_mock = mock.MagicMock(spec=Role) - content_type_mock = mock.MagicMock(spec=ContentType) - role_mock.content_type = content_type_mock - mock_get_obj.return_value = role_mock - ct_get.return_value = content_type_mock - - factory = APIRequestFactory() - view = TeamRolesList.as_view() - - request = factory.post(url, {'id':1}, format="json") - force_authenticate(request, User(username="root", is_superuser=True)) - - response = view(request) - response.render() - - assert response.status_code == 400 - assert 'cannot assign' in response.content -