From 93b6153e4102ade1391d55a45e342f33a35b1662 Mon Sep 17 00:00:00 2001 From: Lindsey Jacks Date: Mon, 22 Aug 2016 13:24:32 -0400 Subject: [PATCH] Fixes #548: Admin users can no longer delete themselves from an organization administrator can no longer change their own role in the organization. --- cadasta/organization/forms.py | 17 ++++++-- cadasta/organization/serializers.py | 10 ++++- cadasta/organization/tests/test_forms.py | 26 +++++++++++- .../tests/test_views_api_organizations.py | 31 ++++++++++++-- .../tests/test_views_default_organizations.py | 5 ++- cadasta/organization/views/api.py | 13 +++++- cadasta/organization/views/default.py | 13 +++++- .../organization_members_edit.html | 17 ++++++-- .../organizations/test_organization_member.py | 41 ++++++++++++++++++- functional_tests/pages/OrganizationMember.py | 17 ++++++++ 10 files changed, 171 insertions(+), 19 deletions(-) diff --git a/cadasta/organization/forms.py b/cadasta/organization/forms.py index 26f900155..2f4699270 100644 --- a/cadasta/organization/forms.py +++ b/cadasta/organization/forms.py @@ -187,12 +187,12 @@ def save(self): class EditOrganizationMemberForm(forms.Form): org_role = forms.ChoiceField(choices=ADMIN_CHOICES) - def __init__(self, data, organization, user, *args, **kwargs): + def __init__(self, data, org, user, current_user, *args, **kwargs): super(EditOrganizationMemberForm, self).__init__(data, *args, **kwargs) self.data = data - self.organization = organization + self.organization = org self.user = user - + self.current_user = current_user self.org_role_instance = OrganizationRole.objects.get( user=user, organization=self.organization) @@ -200,7 +200,7 @@ def __init__(self, data, organization, user, *args, **kwargs): self.initial['org_role'] = 'A' if self.org_role_instance.admin else 'M' project_roles = ProjectRole.objects.filter( - project__organization=organization).values('project__id', 'role') + project__organization=org).values('project__id', 'role') project_roles = {r['project__id']: r['role'] for r in project_roles} for p in self.organization.projects.values_list('id', 'name'): @@ -213,6 +213,15 @@ def __init__(self, data, organization, user, *args, **kwargs): initial=role ) + def clean_org_role(self): + org_role = self.cleaned_data['org_role'] + if self.org_role_instance.admin and self.current_user == self.user: + if self.data.get('org_role') != 'A': + raise forms.ValidationError( + _("Organization administrators cannot change their own" + + " role in the organization.")) + return org_role + def save(self): self.org_role_instance.admin = self.data.get('org_role') == 'A' self.org_role_instance.save() diff --git a/cadasta/organization/serializers.py b/cadasta/organization/serializers.py index 5a89752e0..7c58f110c 100644 --- a/cadasta/organization/serializers.py +++ b/cadasta/organization/serializers.py @@ -151,6 +151,15 @@ def validate_username(self, value): if error: raise serializers.ValidationError(error.format(value)) + def validate_admin(self, role_value): + if 'request' in self.context: + if self.context['request'].user == self.instance: + if role_value != self.get_roles_object(self.instance).admin: + raise serializers.ValidationError( + _("Organization administrators cannot change their " + "own permissions within the organization")) + return role_value + def create(self, validated_data): obj = self.context[self.Meta.context_key] @@ -171,7 +180,6 @@ def create(self, validated_data): def update(self, instance, validated_data): role = self.get_roles_object(instance) - role_value = validated_data[self.Meta.role_key] if self.Meta.role_key in validated_data: diff --git a/cadasta/organization/tests/test_forms.py b/cadasta/organization/tests/test_forms.py index f1972c203..42b11b9e0 100644 --- a/cadasta/organization/tests/test_forms.py +++ b/cadasta/organization/tests/test_forms.py @@ -239,11 +239,14 @@ class EditOrganizationMemberFormTest(UserTestCase): def test_edit_org_role(self): org = OrganizationFactory.create() user = UserFactory.create() + current_user = UserFactory.create() data = {'org_role': 'A'} org_role = OrganizationRole.objects.create(organization=org, user=user) - form = forms.EditOrganizationMemberForm(data, org, user) + OrganizationRole.objects.create( + organization=org, user=current_user, admin=True) + form = forms.EditOrganizationMemberForm(data, org, user, current_user) form.save() org_role.refresh_from_db() @@ -251,6 +254,25 @@ def test_edit_org_role(self): assert form.is_valid() is True assert org_role.admin is True + def test_edit_admin_role(self): + # Should fail, since admins are not allowed to alter + # their own permissions. + org = OrganizationFactory.create() + user = UserFactory.create() + + data = {'org_role': 'M'} + + org_role = OrganizationRole.objects.create( + organization=org, user=user, admin=True) + form = forms.EditOrganizationMemberForm(data, org, user, user) + assert not form.is_valid() + assert form.errors == { + 'org_role': ["Organization administrators cannot change their" + + " own role in the organization."] + } + org_role.refresh_from_db() + assert org_role.admin is True + def test_edit_project_roles(self): user = UserFactory.create() org = OrganizationFactory.create() @@ -270,7 +292,7 @@ def test_edit_project_roles(self): prj_4.id: 'Pb' } - form = forms.EditOrganizationMemberForm(data, org, user) + form = forms.EditOrganizationMemberForm(data, org, user, user) form.save() org_role.refresh_from_db() diff --git a/cadasta/organization/tests/test_views_api_organizations.py b/cadasta/organization/tests/test_views_api_organizations.py index 5df73d205..a1c0ced35 100644 --- a/cadasta/organization/tests/test_views_api_organizations.py +++ b/cadasta/organization/tests/test_views_api_organizations.py @@ -73,7 +73,6 @@ def test_list_only_one_organization_is_authorized(self): """ OrganizationFactory.create_from_kwargs([{}, {'slug': 'unauthorized'}]) content = self._get(user=self.unauth_user, status=200, length=1) - print(content[0]) assert content[0]['slug'] != 'unauthorized' def test_full_list_with_unauthorized_user(self): @@ -427,12 +426,14 @@ def _get(self, org, username, user=None, status=None): assert response.status_code == status return content - def _patch(self, org, username, data, status=None, count=None): + def _patch(self, org, username, data, user=None, status=None, count=None): + if not user: + user = self.user url = '/v1/organizations/{org}/users/{username}' request = APIRequestFactory().patch( url.format(org=org.slug, username=username), data=data, format='json') - force_authenticate(request, user=self.user) + force_authenticate(request, user=user) response = self.view(request, slug=org.slug, username=username).render() content = json.loads(response.content.decode('utf-8')) @@ -478,6 +479,22 @@ def test_update_user(self): role = OrganizationRole.objects.get(organization=org, user=user) assert role.admin is True + def test_update_admin_user(self): + user = UserFactory.create() + org = OrganizationFactory.create(add_users=[user]) + self._patch(org, user.username, data={'admin': 'true'}, + status=200, count=1) + role = OrganizationRole.objects.get(organization=org, user=user) + assert role.admin is True + + content = self._patch(org, user.username, user=user, + data={'admin': 'false'}, status=400, count=1) + assert content['admin'][0] == ("Organization administrators cannot" + " change their own permissions within" + " the organization") + role = OrganizationRole.objects.get(organization=org, user=user) + assert role.admin is True + def test_remove_user(self): user = UserFactory.create() user_to_remove = UserFactory.create() @@ -486,6 +503,14 @@ def test_remove_user(self): assert user_to_remove not in org.users.all() assert user_to_remove in User.objects.all() + def test_remove_admin_user(self): + user = UserFactory.create() + org = OrganizationFactory.create(add_users=[user]) + self._patch(org, user.username, data={'admin': 'true'}, + status=200, count=1) + self._delete(org, user.username, user=user, status=403, count=1) + assert user in org.users.all() + def test_remove_with_unauthorized_user(self): user = UserFactory.create() user_to_remove = UserFactory.create() diff --git a/cadasta/organization/tests/test_views_default_organizations.py b/cadasta/organization/tests/test_views_default_organizations.py index f35f7c22b..f60d18c7a 100644 --- a/cadasta/organization/tests/test_views_default_organizations.py +++ b/cadasta/organization/tests/test_views_default_organizations.py @@ -697,6 +697,7 @@ def test_get_with_authorized_user(self): user = UserFactory.create() assign_user_policies(user, self.policy) setattr(self.request, 'user', user) + OrganizationRole.objects.create(organization=self.org, user=user) response = self.view( self.request, @@ -708,7 +709,8 @@ def test_get_with_authorized_user(self): context['object'] = self.member context['organization'] = self.org context['form'] = forms.EditOrganizationMemberForm( - None, self.org, self.member) + None, self.org, self.member, user) + # context['current_user'] = user expected = render_to_string( 'organization/organization_members_edit.html', @@ -791,6 +793,7 @@ def test_post_with_unauthenticated_user(self): def test_post_with_invalid_form(self): user = UserFactory.create() assign_user_policies(user, self.policy) + OrganizationRole.objects.create(organization=self.org, user=user) setattr(self.request, 'user', user) setattr(self.request, 'method', 'POST') setattr(self.request, 'POST', {'org_role': 'X'}) diff --git a/cadasta/organization/views/api.py b/cadasta/organization/views/api.py index fa81dbc98..a369151b9 100644 --- a/cadasta/organization/views/api.py +++ b/cadasta/organization/views/api.py @@ -1,3 +1,4 @@ +from django.utils.translation import ugettext as _ from rest_framework.response import Response from rest_framework import generics, filters, status from tutelary.mixins import APIPermissionRequiredMixin @@ -63,11 +64,19 @@ class OrganizationUsersDetail(APIPermissionRequiredMixin, permission_required = 'org.users.remove' def destroy(self, request, *args, **kwargs): + user = self.get_object() + if user == request.user: + return Response( + { + _('WARNING'): + _("Organization administrators cannot delete themselves.") + }, + status=status.HTTP_403_FORBIDDEN) OrganizationRole.objects.get( - organization=self.org, user=self.get_object() + organization=self.org, user=user ).delete() - return Response(status=status.HTTP_204_NO_CONTENT) + return Response(status=status.HTTP_204_NO_CONTENT, ) class UserAdminList(APIPermissionRequiredMixin, generics.ListAPIView): diff --git a/cadasta/organization/views/default.py b/cadasta/organization/views/default.py index eb3ee803e..284f9dedc 100644 --- a/cadasta/organization/views/default.py +++ b/cadasta/organization/views/default.py @@ -186,16 +186,25 @@ def get_form(self): if self.request.method == 'POST': return self.form_class(self.request.POST, self.get_organization(), - self.get_object()) + self.get_object(), + self.request.user) else: return self.form_class(None, self.get_organization(), - self.get_object()) + self.get_object(), + self.request.user) def get_context_data(self, *args, **kwargs): context = super().get_context_data(*args, **kwargs) context['organization'] = self.get_organization() context['form'] = self.get_form() + + org_admin = OrganizationRole.objects.get( + user=self.request.user, + organization=context['organization']).admin + context['org_admin'] = (org_admin and + not self.is_superuser and + context['org_member'] == self.request.user) return context def post(self, request, *args, **kwargs): diff --git a/cadasta/templates/organization/organization_members_edit.html b/cadasta/templates/organization/organization_members_edit.html index 6f5006acf..4a9dd0549 100644 --- a/cadasta/templates/organization/organization_members_edit.html +++ b/cadasta/templates/organization/organization_members_edit.html @@ -33,11 +33,21 @@

{{ object.username }}

- {% render_field form.org_role class+="form-control" %} + {% if org_admin %} + {% render_field form.org_role class+="form-control" %} + {% else %} + {% render_field form.org_role class+="form-control" %} + {% endif %}
- + {% if org_admin %} + + {% endif %}
@@ -83,7 +93,7 @@

{{ object.username }}

{% endblock %} {% block modals %} - +{% if not org_admin %} +{% endif %} {% endblock %} diff --git a/functional_tests/organizations/test_organization_member.py b/functional_tests/organizations/test_organization_member.py index a33ed86ea..60809dc1c 100644 --- a/functional_tests/organizations/test_organization_member.py +++ b/functional_tests/organizations/test_organization_member.py @@ -21,6 +21,7 @@ def setUp(self): organization=test_objs['organizations'][0], user=UserFactory.create( username='admin_user', + full_name='Admin User', password='password'), admin=True) @@ -64,6 +65,29 @@ def test_changing_a_members_organizational_role(self): roles = page.get_role_options() assert roles["admin"].text == roles["selected"].text + def test_changing_an_admins_organizational_role(self): + """An admin member can change a member's role in the organization.""" + + LoginPage(self).login('admin_user', 'password') + page = OrganizationMemberPage(self) + page.go_to() + OrganizationPage(self).go_to_organization_page() + OrganizationMemberListPage(self).go_to_member_list_page() + page.go_to_admin_member_page() + + roles = page.get_role_options() + assert roles["admin"].text == roles["selected"].text + + roles["member"].click() + page.click_submit_button() + self.get_screenshot() + # get error message + # assert you stayed on the same page. + roles = page.get_role_options() + errors = page.get_org_role_error() + assert errors.text == ("Organization administrators cannot change" + + " their own role in the organization.") + def test_removing_a_member_from_an_organization(self): """An admin member can remove a member from an organization.""" @@ -82,6 +106,21 @@ def test_removing_a_member_from_an_organization(self): members = page.get_table_row().text assert "Test User" not in members + def test_removing_an_admin_member_from_an_organization(self): + """An admin member can remove a member from an organization.""" + + LoginPage(self).login('admin_user', 'password') + page = OrganizationMemberPage(self) + page.go_to() + OrganizationPage(self).go_to_organization_page() + OrganizationMemberListPage(self).go_to_member_list_page() + page.go_to_admin_member_page() + + assert page.get_member_title() == "MEMBER: Admin User" + + page.click_disabled_remove_button() + assert page.get_member_title() == "MEMBER: Admin User" + def test_changing_member_project_permissions(self): """An admin user can change a member's permissions on individual projects.""" @@ -121,4 +160,4 @@ def test_admin_creation_when_adding_organization(self): page.go_to_testuser_member_page() roles = page.get_role_options() - assert roles["admin"].text == roles["selected"].text + assert roles['selected'].text == "Administrator" diff --git a/functional_tests/pages/OrganizationMember.py b/functional_tests/pages/OrganizationMember.py index e2521d3c2..03fd26b85 100644 --- a/functional_tests/pages/OrganizationMember.py +++ b/functional_tests/pages/OrganizationMember.py @@ -36,6 +36,14 @@ def go_to_testuser_member_page(self): testuser_title = self.get_member_title() return testuser_title + def go_to_admin_member_page(self): + testuser_page = self.get_table_row( + "[contains(@onclick, '/admin_user/')]" + ) + self.click_through(testuser_page, (By.CLASS_NAME, 'page-title')) + testuser_title = self.get_member_title() + return testuser_title + def get_form_field(self, xpath): return self.test.form_field( 'org-member-edit', xpath) @@ -54,6 +62,10 @@ def get_member_role_option(self): def get_admin_role_option(self): return self.get_member_role_select("//option[contains(@value, 'A')]") + def get_org_role_error(self): + return self.get_form_field("div[contains(@class, 'member-role')]" + + "//ul[contains(@class, 'errorlist')]") + def get_selected_role(self): return self.get_member_role_select( "//option[contains(@selected, 'selected')]") @@ -75,6 +87,11 @@ def click_remove_button(self): self.test.button("remove"), (By.CSS_SELECTOR, "div.modal.fade.in") ) + def click_disabled_remove_button(self): + self.test.click_through_close( + self.test.button("remove"), (By.CSS_SELECTOR, "div.modal.fade.in") + ) + def click_remove_member_and_confirm_buttons(self): self.click_remove_button() self.click_through(