From 4379e8b83deadb5fedd21cec57283bb4208032e0 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Wed, 18 Oct 2023 10:42:39 -0400 Subject: [PATCH 1/6] fix: Don't allow group chair to change group parent (#6037) --- ietf/group/forms.py | 31 ++++++++++++++++++------------- ietf/group/tests_info.py | 27 ++++++++++++++++++++++++++- ietf/group/views.py | 7 ++++--- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/ietf/group/forms.py b/ietf/group/forms.py index c93ca1d63c..7e00429271 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -1,4 +1,4 @@ -# Copyright The IETF Trust 2017-2020, All Rights Reserved +# Copyright The IETF Trust 2017-2023, All Rights Reserved # -*- coding: utf-8 -*- @@ -103,6 +103,8 @@ def __init__(self, *args, **kwargs): else: field = None + self.hide_parent = kwargs.pop('hide_parent', False) + super(self.__class__, self).__init__(*args, **kwargs) if not group_features or group_features.has_chartering_process: @@ -138,18 +140,21 @@ def __init__(self, *args, **kwargs): self.fields['acronym'].widget.attrs['readonly'] = "" # Sort out parent options - self.fields['parent'].queryset = self.fields['parent'].queryset.filter(type__in=parent_types) - if need_parent: - self.fields['parent'].required = True - self.fields['parent'].empty_label = None - # if this is a new group, fill in the default parent, if any - if self.group is None or (not hasattr(self.group, 'pk')): - self.fields['parent'].initial = self.fields['parent'].queryset.filter( - acronym=default_parent - ).first() - # label the parent field as 'IETF Area' if appropriate, for consistency with past behavior - if parent_types.count() == 1 and parent_types.first().pk == 'area': - self.fields['parent'].label = "IETF Area" + if self.hide_parent: + self.fields.pop('parent') + else: + self.fields['parent'].queryset = self.fields['parent'].queryset.filter(type__in=parent_types) + if need_parent: + self.fields['parent'].required = True + self.fields['parent'].empty_label = None + # if this is a new group, fill in the default parent, if any + if self.group is None or (not hasattr(self.group, 'pk')): + self.fields['parent'].initial = self.fields['parent'].queryset.filter( + acronym=default_parent + ).first() + # label the parent field as 'IETF Area' if appropriate, for consistency with past behavior + if parent_types.count() == 1 and parent_types.first().pk == 'area': + self.fields['parent'].label = "IETF Area" if field: keys = list(self.fields.keys()) diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index e0a738d073..ba37dabe73 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -1,4 +1,4 @@ -# Copyright The IETF Trust 2009-2022, All Rights Reserved +# Copyright The IETF Trust 2009-2023, All Rights Reserved # -*- coding: utf-8 -*- @@ -950,6 +950,31 @@ def test_edit_description_field(self): group = Group.objects.get(pk=group.pk) # refresh self.assertEqual(group.description, 'Updated description') + def test_edit_parent_field(self): + group = GroupFactory(acronym='mars', parent=Group.objects.get(acronym='farfut')) + RoleFactory(group=group, name_id='chair') + url = urlreverse('ietf.group.views.edit', kwargs={'acronym': group.acronym, 'action': 'edit', 'field': 'parent'}) + + # parent is not shown to group chair + login_testing_unauthorized(self, 'mars-chair', url) + r = self.client.get(url) + self.assertEqual(r.status_code, 302) + self.assertEqual(len(r.content), 0) + + # parent is shown to AD and Secretariat + for priv_user in ('ad', 'secretary'): + self.client.logout() + login_testing_unauthorized(self, priv_user, url) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + q = PyQuery(r.content) + self.assertEqual(len(q('div#content > form input[name=parent], div#content > form select[name=parent]')), 1) + new_parent = GroupFactory(type_id='area', parent__type_id='ietf') + r = self.client.post(url, {'parent': new_parent.pk}) + self.assertEqual(r.status_code, 302) + group.refresh_from_db() + self.assertEqual(group.parent, new_parent) + def test_conclude(self): group = GroupFactory(acronym="mars") diff --git a/ietf/group/views.py b/ietf/group/views.py index 2b93e07607..3d07b65c36 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -945,14 +945,15 @@ def diff(attr, name): if not (can_manage_group(request.user, group) or group.has_role(request.user, group.features.groupman_roles)): permission_denied(request, "You don't have permission to access this view") + hide_parent = not has_role(request.user, ("Secretariat", "Area Director", "IRTF Chair")) else: # This allows ADs to create RG and the IRTF Chair to create WG, but we trust them not to if not has_role(request.user, ("Secretariat", "Area Director", "IRTF Chair")): permission_denied(request, "You don't have permission to access this view") - + hide_parent = False if request.method == 'POST': - form = GroupForm(request.POST, group=group, group_type=group_type, field=field) + form = GroupForm(request.POST, group=group, group_type=group_type, field=field, hide_parent=hide_parent) if form.is_valid(): clean = form.cleaned_data if new_group: @@ -1114,7 +1115,7 @@ def diff(attr, name): else: init = dict() - form = GroupForm(initial=init, group=group, group_type=group_type, field=field) + form = GroupForm(initial=init, group=group, group_type=group_type, field=field, hide_parent=hide_parent) return render(request, 'group/edit.html', dict(group=group, From a97ff1141705986cc7b110b68ac10b4c8e745a9c Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Wed, 18 Oct 2023 19:04:35 -0400 Subject: [PATCH 2/6] test: Fix test_edit_parent_field, add test_edit_parent (whole form) --- ietf/group/tests_info.py | 55 ++++++++++++++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 10 deletions(-) diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index ba37dabe73..35b6855f4e 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -950,16 +950,49 @@ def test_edit_description_field(self): group = Group.objects.get(pk=group.pk) # refresh self.assertEqual(group.description, 'Updated description') + def test_edit_parent(self): + group = GroupFactory.create(type_id='wg', parent=GroupFactory.create(type_id='area')) + chair = RoleFactory(group=group, name_id='chair').person + url = urlreverse('ietf.group.views.edit', kwargs=dict(group_type=group.type_id, acronym=group.acronym, action='edit')) + + # parent is not shown to group chair + login_testing_unauthorized(self, chair.user.username, url) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + q = PyQuery(r.content) + self.assertEqual(len(q('form select[name=parent]')), 0) + + # parent is shown to AD and Secretariat + for priv_user in ('ad', 'secretary'): + self.client.logout() + login_testing_unauthorized(self, priv_user, url) + r = self.client.get(url) + self.assertEqual(r.status_code, 200) + q = PyQuery(r.content) + self.assertEqual(len(q('form select[name=parent]')), 1) + + new_parent = GroupFactory(type_id='area') + self.assertNotEqual(new_parent.acronym, group.parent.acronym) + r = self.client.post(url, dict( + name=group.name, + acronym=group.acronym, + state=group.state_id, + parent=new_parent.pk)) + self.assertEqual(r.status_code, 302) + group = Group.objects.get(pk=group.pk) + self.assertEqual(group.parent, new_parent) + def test_edit_parent_field(self): - group = GroupFactory(acronym='mars', parent=Group.objects.get(acronym='farfut')) - RoleFactory(group=group, name_id='chair') - url = urlreverse('ietf.group.views.edit', kwargs={'acronym': group.acronym, 'action': 'edit', 'field': 'parent'}) + group = GroupFactory.create(type_id='wg', parent=GroupFactory.create(type_id='area')) + chair = RoleFactory(group=group, name_id='chair').person + url = urlreverse('ietf.group.views.edit', kwargs=dict(group_type=group.type_id, acronym=group.acronym, action='edit', field='parent')) # parent is not shown to group chair - login_testing_unauthorized(self, 'mars-chair', url) + login_testing_unauthorized(self, chair.user.username, url) r = self.client.get(url) - self.assertEqual(r.status_code, 302) - self.assertEqual(len(r.content), 0) + self.assertEqual(r.status_code, 200) + q = PyQuery(r.content) + self.assertEqual(len(q('form select[name=parent]')), 0) # parent is shown to AD and Secretariat for priv_user in ('ad', 'secretary'): @@ -968,11 +1001,13 @@ def test_edit_parent_field(self): r = self.client.get(url) self.assertEqual(r.status_code, 200) q = PyQuery(r.content) - self.assertEqual(len(q('div#content > form input[name=parent], div#content > form select[name=parent]')), 1) - new_parent = GroupFactory(type_id='area', parent__type_id='ietf') - r = self.client.post(url, {'parent': new_parent.pk}) + self.assertEqual(len(q('form select[name=parent]')), 1) + + new_parent = GroupFactory(type_id='area') + self.assertNotEqual(new_parent.acronym, group.parent.acronym) + r = self.client.post(url, dict(parent=new_parent.pk)) self.assertEqual(r.status_code, 302) - group.refresh_from_db() + group = Group.objects.get(pk=group.pk) self.assertEqual(group.parent, new_parent) def test_conclude(self): From 97f27a0f5db6b76c0452017c8cfc83334381a37f Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Thu, 19 Oct 2023 10:24:13 -0400 Subject: [PATCH 3/6] test: Verify that the chair can't circumvent the system to change the group parent --- ietf/group/tests_info.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index 35b6855f4e..5b878511a5 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -962,6 +962,20 @@ def test_edit_parent(self): q = PyQuery(r.content) self.assertEqual(len(q('form select[name=parent]')), 0) + # view ignores attempt to change parent + old_parent = group.parent + new_parent = GroupFactory(type_id='area') + self.assertNotEqual(new_parent.acronym, group.parent.acronym) + r = self.client.post(url, dict( + name=group.name, + acronym=group.acronym, + state=group.state_id, + parent=new_parent.pk)) + self.assertEqual(r.status_code, 302) + group = Group.objects.get(pk=group.pk) + self.assertNotEqual(group.parent, new_parent) + self.assertEqual(group.parent, old_parent) + # parent is shown to AD and Secretariat for priv_user in ('ad', 'secretary'): self.client.logout() @@ -994,6 +1008,16 @@ def test_edit_parent_field(self): q = PyQuery(r.content) self.assertEqual(len(q('form select[name=parent]')), 0) + # view ignores attempt to change parent + old_parent = group.parent + new_parent = GroupFactory(type_id='area') + self.assertNotEqual(new_parent.acronym, group.parent.acronym) + r = self.client.post(url, dict(parent=new_parent.pk)) + self.assertEqual(r.status_code, 302) + group = Group.objects.get(pk=group.pk) + self.assertNotEqual(group.parent, new_parent) + self.assertEqual(group.parent, old_parent) + # parent is shown to AD and Secretariat for priv_user in ('ad', 'secretary'): self.client.logout() From a709f911b8e3460bc1304692f052c65374294ff3 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Thu, 19 Oct 2023 15:38:00 -0400 Subject: [PATCH 4/6] fix: 403 if user tries to edit an unknown or hidden field --- ietf/group/tests_info.py | 14 ++++---------- ietf/group/views.py | 4 ++++ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index 5b878511a5..136e195494 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -946,7 +946,7 @@ def test_edit_description_field(self): r = self.client.post(url, { 'description': 'Ignored description', }) - self.assertEqual(r.status_code, 302) + self.assertEqual(r.status_code, 403) group = Group.objects.get(pk=group.pk) # refresh self.assertEqual(group.description, 'Updated description') @@ -1004,19 +1004,13 @@ def test_edit_parent_field(self): # parent is not shown to group chair login_testing_unauthorized(self, chair.user.username, url) r = self.client.get(url) - self.assertEqual(r.status_code, 200) - q = PyQuery(r.content) - self.assertEqual(len(q('form select[name=parent]')), 0) + self.assertEqual(r.status_code, 403) - # view ignores attempt to change parent - old_parent = group.parent + # chair is not allowed to change parent new_parent = GroupFactory(type_id='area') self.assertNotEqual(new_parent.acronym, group.parent.acronym) r = self.client.post(url, dict(parent=new_parent.pk)) - self.assertEqual(r.status_code, 302) - group = Group.objects.get(pk=group.pk) - self.assertNotEqual(group.parent, new_parent) - self.assertEqual(group.parent, old_parent) + self.assertEqual(r.status_code, 403) # parent is shown to AD and Secretariat for priv_user in ('ad', 'secretary'): diff --git a/ietf/group/views.py b/ietf/group/views.py index 3d07b65c36..593c649bb1 100644 --- a/ietf/group/views.py +++ b/ietf/group/views.py @@ -954,6 +954,8 @@ def diff(attr, name): if request.method == 'POST': form = GroupForm(request.POST, group=group, group_type=group_type, field=field, hide_parent=hide_parent) + if field and not form.fields: + permission_denied(request, "You don't have permission to edit this field") if form.is_valid(): clean = form.cleaned_data if new_group: @@ -1116,6 +1118,8 @@ def diff(attr, name): else: init = dict() form = GroupForm(initial=init, group=group, group_type=group_type, field=field, hide_parent=hide_parent) + if field and not form.fields: + permission_denied(request, "You don't have permission to edit this field") return render(request, 'group/edit.html', dict(group=group, From 44c4745b796b80dfa8dc917eef2fa06a21a1939a Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Thu, 19 Oct 2023 18:12:53 -0400 Subject: [PATCH 5/6] fix: Give edwg GroupFeatures a parent type This tracks a change that was made directly in the production database to fix the immediate cause of #6037. --- ietf/name/fixtures/names.json | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ietf/name/fixtures/names.json b/ietf/name/fixtures/names.json index d105e8c61c..4dfe1574a2 100644 --- a/ietf/name/fixtures/names.json +++ b/ietf/name/fixtures/names.json @@ -3034,7 +3034,9 @@ "material_types": "[\n \"slides\"\n]", "matman_roles": "[\n \"chair\"\n]", "need_parent": false, - "parent_types": [], + "parent_types": [ + "rfcedtyp" + ], "req_subm_approval": true, "role_order": "[\n \"chair\"\n]", "session_purposes": "[\n \"regular\"\n]", From 0593b8f9bd7d412c5a5f1703d37d870e7f6ea845 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Thu, 2 Nov 2023 11:11:04 -0700 Subject: [PATCH 6/6] Empty commit to trigger github unit test