fix: Don't allow group chair to change group parent (ietf-tools#6496)

pselkirk · web-flow · commit b5ee9ec9ab94 · 2023-11-05T12:32:03.000+01:00
* fix: Don't allow group chair to change group parent (ietf-tools#6037) * test: Fix test_edit_parent_field, add test_edit_parent (whole form) * test: Verify that the chair can't circumvent the system to change the group parent * fix: 403 if user tries to edit an unknown or hidden field * 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 ietf-tools#6037. * Empty commit to trigger github unit test
diff --git 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
@@ -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 -*-
 
 
@@ -946,10 +946,88 @@ 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')
 
+    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)
+
+        # 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()
+            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.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, chair.user.username, url)
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 403)
+
+        # 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, 403)
+
+        # 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(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_conclude(self):
         group = GroupFactory(acronym="mars")
 
diff --git a/ietf/group/views.py b/ietf/group/views.py
@@ -945,14 +945,17 @@ 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 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:
@@ -1114,7 +1117,9 @@ 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)
+        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,
diff --git 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]",
