group edit form destroys roles

[ietf-svn-bot]

owner:jennifer@painless-security.com resolution_fixed type_defect | by rjsparks@nostrum.com

---

I just edited the IESG to add Michelle as a trac-editor using the Edit Group button on the groups about page.

The result was very surprising, as captured in the history. Many roles were changed that shouldn't have been. Comments about changes of shepherd happened.

Something is really off the rails with this edit form.

We're putting the group back together using the admin.

---

Issue migrated from trac:3253 at 2022-03-04 08:37:20 +0000

[ietf-svn-bot]

@cmorgan@amsl.com commented

---

It looks like in addition to changing roles, this may have also cleared the Parent field. (This has been reset using the admin.)

[ietf-svn-bot]

@jennifer@painless-security.com changed status from new to assigned

[ietf-svn-bot]

@jennifer@painless-security.com changed owner from `` to jennifer@painless-security.com

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

The good / bad news is this seems to be repeatable - I see exactly the same result on my dev system. That should help with debugging, at least.

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

Update on investigation - please let me know if anything looks wrong here.

It looks to me like the group edit view does not handle 'ietf' type groups (like the IESG) correctly and has not for some time. For types other than 'rg', the parent field can only be of type 'area', which means that the IETF group (also type 'ietf') cannot be selected as a parent. As a result, it falls back to None, explaining the unexpected change of parent. As near as I can tell, this has been the case since at least 2017 when some code was refactored but allegedly did not change functionality.

There was also a refactoring in June 2020 that dynamically set up the role fields based on the used_roles of a group. I think this may have a conflict when a group type's used_roles includes 'ad', because there's a static field by that name. The static field has the label 'Shepherding AD', which strongly suggests the set of AD roles is being bashed by the contents of the 'Shepherding AD' field. I haven't fully investigated this, though.

I think this explains the behavior, I'm now looking to confirm this and try to fix it. Please let me know if it sounds like I'm heading in the wrong direction. Given the sparse edit history log for the IESG, it seems plausible that there's been a latent bug, but I'm not familiar with how frequently various groups are edited through the UI.

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

The editing of the IESG has historically been done through either the secr app (the rolodex) or via the admin.

I stepped in an unused and apparently badly tested corner by attempting to edit the group this way.

The long term goal is for the secretariat to be able to use the group edit form for everything they're currently using the /secr/ apps to do, so correcting this forms behavior for the iesg is a good use of time.

Building the tests out to do at least a sanity check across all the existing group types would be helpful if it isn't a large task.

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

btw - it's ok for now if this edit view just say "no" for obscure group types if it doesn't already.

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

Ok - I will see about putting in some tests. That will help understand how big a job it would be to allow these groups to be edited vs. refusing to edit obscure group types. It may be that it's just a couple of fields to coordinate.

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

For group parent, is there a rule for what group types are acceptable as a parent? That's part of the rub here. The GroupForm currently restricts the parent to 'irtf' for 'rg' groups and 'area' for all others.

I'm guessing the 'right' thing to do, at least in some sense of the word, would be another GroupFeatures addition to map group types to the allowed parent group types. It might make sense to think about it the other way around, mapping to allowed child group types, but in either case it'd be a many-to-many relationship. Formalizing the allowed relationships between groups would fit in with the other work I've been doing to rationalize the group modeling.

A simpler patch that would avoid a recurrence of this bug would be to allow the current group parent as an option, regardless of its type. (I'm rejecting the idea of allowing unfiltered access to parent groups, because a pull-down with 1400+ options is a bit unwieldy...)

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

A GroupFeature for allowed parent group types makes sense to me.

I think it will be fairly easy to programmatically extract what the initial values of such a feature would contain.

Note that we do allow nested parenting for some group types:

In 91093c4c8f45fcf13fd29578db8c037b2310cf5f: sg15 = Group.objects.get(acronym='itu-t-sg-15')

In 3598137fef9a41f40f0b5b6fa31667d73b2307ca: sg15.type_id
Out3598137fef9a41f40f0b5b6fa31667d73b2307ca: 'sdo'

In dc22fb842160a0c0387457a61838e807e01c8c4a: sg15.parent
Outdc22fb842160a0c0387457a61838e807e01c8c4a: <Group: ITU-T>

In 6d4a31ba35b707e1679740f4da71dd1cec12e4f9: sg15.parent.acronym
Out6d4a31ba35b707e1679740f4da71dd1cec12e4f9: 'itu-t'

In aaf74c2e5ab974642c181ce1fd0c7d09b207970a: sg15.parent.type
Outaaf74c2e5ab974642c181ce1fd0c7d09b207970a: <GroupTypeName: SDO>

In d8b432eabaa0496456ed4cc34e97ad500c848325: sg15.parent.parent
Outd8b432eabaa0496456ed4cc34e97ad500c848325: <Group: ITU>

In 5af8d2d095c84918a92c7038da20167cb59f861c: sg15.parent.parent.type
Out5af8d2d095c84918a92c7038da20167cb59f861c: <GroupTypeName: SDO>

In 02a2d27d1efbe53c5b46be4f5c32fefc3865f1e1: sg15.parent.parent.parent

In 07d3681982395b55cf66cd0b8a8ee4e83da11579:

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

I think this is the list of existing parent types for each group. Does it seem correct?

In 3c51692b3d63bcdd3a293ca607da27933151734d: [(t.slug, list(Group.objects.filter(type=t, parent__isnull=False).values_list('parent__type', flat=True).distinct())) for t in GroupTypeName.objects.all()]
Out3c51692b3d63bcdd3a293ca607da27933151734d: 
[('adhoc', ['ietf']),
 ('admin', []),
 ('ag', ['area', 'ietf']),
 ('area', ['ietf']),
 ('dir', ['area']),
 ('review', ['area']),
 ('iab', ['ietf']),
 ('iana', []),
 ('iesg', ['ietf']),
 ('ietf', []),
 ('individ', ['area']),
 ('irtf', ['irtf']),
 ('ise', []),
 ('isoc', ['isoc']),
 ('nomcom', ['area']),
 ('program', ['ietf']),
 ('rag', ['irtf']),
 ('rfcedtyp', []),
 ('rg', ['irtf']),
 ('sdo', ['sdo', 'area']),
 ('team', ['area']),
 ('wg', ['area'])]

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

There is a problem with the shepherding AD. If I understand correctly, this is represented as an 'ad' Role for the group. When set via the pull-down, any existing 'ad' Roles are deleted and one is created for the shepherd.

Separately, the 'Area Director' field updates the list of 'ad' Roles with one or more entries. Depending on the order of processing between the 'ad' and 'ad_roles' fields in the POST data, one of these overwrites the change made by the other.

I'm not sure how to resolve this conflict - as it is, I don't see how both the shepherding AD and a set of ADs can be represented for a group. Any suggestions?

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

Replying to ietf-svn-conversion/datatracker#3253 (comment:10):

I think this is the list of existing parent types for each group. Does it seem correct?
The lists should include None for some of the group types. It makes sense for some group types to not have a parent, and others must.

An sdo group could be a root group like itu, and have no parent, or a child with a parent that is of type sdo.

A wg must always have an area parent.

The 'individ' group type is special. Note that there is one group of that type and its acronym is 'none'. It exists to model individual submissions (those that have not been adopted by some other group). That it has an area parent (specifically the general area) is a consequence of an arbitrary past decision. But lets capture what we are doing now and look at improving how this is modeled later.

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

Replying to ietf-svn-conversion/datatracker#3253 (comment:11):

There is a problem with the shepherding AD.

The notion of a single responsible (or shepherding) AD comes from working groups. Reasearch groups, for example won't have one.

The group about page, at least simply takes the first AD role returned by the database for the group and declares that the shepherding AD (for groups that should have one).

I think we can change the form to just allow editing AD roles for the group, and remove the separate "shepherding AD" as a special thing as far as that form is concerned.

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

Replying to ietf-svn-conversion/datatracker#3253 (comment:10):

Here's what I come up with:

In a00b1631f4c7a6372fdbfd5dbf1bd54853a82bc4: r = defaultdict(set)

In 254cce4071414991b23b276b313ab5ac674d96d7: for child, parent in Group.objects.values_list('type','parent__type'):
    ...:     r[child].add(parent)
    ...:

In 98165ab39945f8c997b48d6ecebf5ed05c8d1f90: r
Out98165ab39945f8c997b48d6ecebf5ed05c8d1f90:
defaultdict(set,
            {'ietf': {None, 'ietf'},
             'irtf': {None, 'irtf'},
             'ise': {None},
             'iab': {None, 'ietf'},
             'iana': {None},
             'adhoc': {None, 'ietf'},
             'nomcom': {None, 'area'},
             'rg': {'irtf'},
             'sdo': {None, 'area', 'sdo'},
             'team': {None, 'area'},
             'area': {'ietf'},
             'wg': {None, 'area'},
             'ag': {None, 'area', 'ietf'},
             'individ': {'area'},
             'review': {'area'},
             'dir': {'area'},
             'rag': {None, 'irtf'},
             'rfcedtyp': {None},
             'admin': {None},
             'isoc': {None, 'isoc'},
             'program': {None, 'ietf'])

I suspect we'll need to change 'review' later to allow more than 'area', but we don't have to do that now.