From 858530c21430859c6397de7a72483086139956f4 Mon Sep 17 00:00:00 2001 From: Russ Housley Date: Sat, 2 Apr 2016 21:06:33 +0000 Subject: [PATCH] Cleaned up the checking permission for a user to manage a group. Also, cleanly handle a set of group parent links did for a loop. - Legacy-Id: 11082 --- ietf/doc/views_charter.py | 8 ++++---- ietf/doc/views_doc.py | 4 ++-- ietf/group/edit.py | 25 ++++++++++-------------- ietf/group/info.py | 8 +++----- ietf/group/milestones.py | 11 ++++++----- ietf/group/models.py | 2 +- ietf/group/tests_info.py | 41 ++++++++++++++++++++++++++++++++++++++- ietf/group/utils.py | 17 +++++++++------- 8 files changed, 76 insertions(+), 40 deletions(-) diff --git a/ietf/doc/views_charter.py b/ietf/doc/views_charter.py index 39b091cff..251324a77 100644 --- a/ietf/doc/views_charter.py +++ b/ietf/doc/views_charter.py @@ -23,7 +23,7 @@ from ietf.doc.utils_charter import ( historic_milestones_for_charter, derive_new_work_text ) from ietf.doc.mails import email_state_changed, email_charter_internal_review from ietf.group.models import ChangeStateGroupEvent, MilestoneGroupEvent -from ietf.group.utils import save_group_in_history, save_milestone_in_history, can_manage_group_type +from ietf.group.utils import save_group_in_history, save_milestone_in_history, can_manage_group from ietf.ietfauth.utils import has_role, role_required from ietf.name.models import GroupStateName from ietf.person.models import Person @@ -58,7 +58,7 @@ def change_state(request, name, option=None): charter = get_object_or_404(Document, type="charter", name=name) group = charter.group - if not can_manage_group_type(request.user, group.type_id): + if not can_manage_group(request.user, group): return HttpResponseForbidden("You don't have permission to access this view") chartering_type = get_chartering_type(charter) @@ -246,7 +246,7 @@ def change_title(request, name, option=None): logging the title as a comment.""" charter = get_object_or_404(Document, type="charter", name=name) group = charter.group - if not can_manage_group_type(request.user, group.type_id): + if not can_manage_group(request.user, group): return HttpResponseForbidden("You don't have permission to access this view") login = request.user.person if request.method == 'POST': @@ -359,7 +359,7 @@ def submit(request, name=None, option=None): charter = get_object_or_404(Document, type="charter", name=name) group = charter.group - if not can_manage_group_type(request.user, group.type_id): + if not can_manage_group(request.user, group): return HttpResponseForbidden("You don't have permission to access this view") path = os.path.join(settings.CHARTER_PATH, '%s-%s.txt' % (charter.canonical_name(), charter.rev)) diff --git a/ietf/doc/views_doc.py b/ietf/doc/views_doc.py index 89218495f..edbad1e87 100644 --- a/ietf/doc/views_doc.py +++ b/ietf/doc/views_doc.py @@ -52,7 +52,7 @@ from ietf.doc.utils import ( add_links_in_new_revision_events, augment_events_wi get_initial_notify, make_notify_changed_event, crawl_history) from ietf.community.models import CommunityList from ietf.group.models import Role -from ietf.group.utils import can_manage_group_type, can_manage_materials +from ietf.group.utils import can_manage_group, can_manage_materials from ietf.ietfauth.utils import has_role, is_authorized_in_doc_stream, user_is_person, role_required from ietf.name.models import StreamName, BallotPositionName from ietf.person.models import Email @@ -445,7 +445,7 @@ def document_main(request, name, rev=None): if chartering and not snapshot: milestones = doc.group.groupmilestone_set.filter(state="charter") - can_manage = can_manage_group_type(request.user, doc.group.type_id) + can_manage = can_manage_group(request.user, doc.group) return render_to_response("doc/document_charter.html", dict(doc=doc, diff --git a/ietf/group/edit.py b/ietf/group/edit.py index 084b9540b..bd9ec2d7c 100644 --- a/ietf/group/edit.py +++ b/ietf/group/edit.py @@ -16,7 +16,7 @@ from ietf.doc.utils import get_tags_for_stream_id from ietf.doc.utils_charter import charter_name_for_group from ietf.group.models import ( Group, Role, GroupEvent, GroupHistory, GroupStateName, GroupStateTransitions, GroupTypeName, GroupURL, ChangeStateGroupEvent ) -from ietf.group.utils import save_group_in_history, can_manage_group_type, can_manage_team_group +from ietf.group.utils import save_group_in_history, can_manage_group from ietf.group.utils import get_group_or_404 from ietf.ietfauth.utils import has_role from ietf.person.fields import SearchableEmailsField @@ -186,7 +186,7 @@ def submit_initial_charter(request, group_type=None, acronym=None): # This is where we start ignoring the passed in group_type group_type = group.type_id - if not can_manage_group_type(request.user, group_type): + if not can_manage_group(request.user, group): return HttpResponseForbidden("You don't have permission to access this view") if not group.charter: @@ -199,18 +199,6 @@ def submit_initial_charter(request, group_type=None, acronym=None): def edit(request, group_type=None, acronym=None, action="edit"): """Edit or create a group, notifying parties as necessary and logging changes as group events.""" - group = get_group_or_404(acronym, group_type) - if not group_type and group: - group_type = group.type_id - - if group_type == "team": - can_edit = can_manage_team_group(request.user, group) or group.has_role(request.user, "chair") - else: - can_edit = can_manage_group_type(request.user, group_type) - - if not can_edit: - return HttpResponseForbidden("You don't have permission to access this view") - if action == "edit": new_group = False elif action in ("create","charter"): @@ -219,6 +207,13 @@ def edit(request, group_type=None, acronym=None, action="edit"): else: raise Http404 + if not new_group: + group = get_group_or_404(acronym, group_type) + if not group_type and group: + group_type = group.type_id + if not (can_manage_group(request.user, group) or group.has_role(request.user, "chair")): + return HttpResponseForbidden("You don't have permission to access this view") + if request.method == 'POST': form = GroupForm(request.POST, group=group, group_type=group_type) if form.is_valid(): @@ -369,7 +364,7 @@ def conclude(request, acronym, group_type=None): """Request the closing of group, prompting for instructions.""" group = get_group_or_404(acronym, group_type) - if not can_manage_group_type(request.user, group.type_id): + if not can_manage_group(request.user, group): return HttpResponseForbidden("You don't have permission to access this view") if request.method == 'POST': diff --git a/ietf/group/info.py b/ietf/group/info.py index 94ed3b2ec..d66719436 100644 --- a/ietf/group/info.py +++ b/ietf/group/info.py @@ -55,7 +55,7 @@ from ietf.doc.utils import get_chartering_type from ietf.doc.templatetags.ietf_filters import clean_whitespace from ietf.group.models import Group, Role, ChangeStateGroupEvent from ietf.name.models import GroupTypeName -from ietf.group.utils import get_charter_text, can_manage_group_type, can_manage_team_group, milestone_reviewer_for_group_type, can_provide_status_update +from ietf.group.utils import get_charter_text, can_manage_group_type, can_manage_group, milestone_reviewer_for_group_type, can_provide_status_update from ietf.group.utils import can_manage_materials, get_group_or_404 from ietf.utils.pipe import pipe from ietf.utils.textupload import get_cleaned_text_file_content @@ -365,9 +365,7 @@ def construct_group_menu_context(request, group, selected, group_type, others): actions = [] is_chair = group.has_role(request.user, "chair") - can_manage = can_manage_group_type(request.user, group.type_id) - if group.type_id == "team": - can_manage = can_manage_team_group(request.user, group) + can_manage = can_manage_group(request.user, group) if group.features.has_milestones: if group.state_id != "proposed" and (is_chair or can_manage): @@ -491,7 +489,7 @@ def group_about(request, acronym, group_type=None): e = group.latest_event(type__in=("changed_state", "requested_close",)) requested_close = group.state_id != "conclude" and e and e.type == "requested_close" - can_manage = can_manage_group_type(request.user, group.type_id) + can_manage = can_manage_group(request.user, group) can_provide_update = can_provide_status_update(request.user, group) status_update = group.latest_event(type="status_update") diff --git a/ietf/group/milestones.py b/ietf/group/milestones.py index 434294fff..0b316fbe8 100644 --- a/ietf/group/milestones.py +++ b/ietf/group/milestones.py @@ -12,7 +12,7 @@ from ietf.doc.models import DocEvent from ietf.doc.utils import get_chartering_type from ietf.doc.fields import SearchableDocumentsField from ietf.group.models import GroupMilestone, MilestoneGroupEvent -from ietf.group.utils import (save_milestone_in_history, can_manage_group_type, milestone_reviewer_for_group_type, +from ietf.group.utils import (save_milestone_in_history, can_manage_group, milestone_reviewer_for_group_type, get_group_or_404) from ietf.name.models import GroupMilestoneStateName from ietf.group.mails import email_milestones_changed @@ -93,8 +93,8 @@ def edit_milestones(request, acronym, group_type=None, milestone_set="current"): raise Http404 needs_review = False - if not can_manage_group_type(request.user, group.type_id): - if group.role_set.filter(name="chair", person__user=request.user): + if not can_manage_group(request.user, group): + if group.has_role(request.user, "chair"): if milestone_set == "current": needs_review = True else: @@ -329,8 +329,9 @@ def reset_charter_milestones(request, group_type, acronym): if not group.features.has_milestones: raise Http404 - if (not can_manage_group_type(request.user, group_type) and - not group.role_set.filter(name="chair", person__user=request.user)): + can_manage = can_manage_group(request.user, group) + is_chair = group.has_role(request.user, "chair") + if (not can_manage) and (not is_chair): return HttpResponseForbidden("You are not chair of this group.") current_milestones = group.groupmilestone_set.filter(state="active") diff --git a/ietf/group/models.py b/ietf/group/models.py index 68f4e2baf..29344fc2e 100644 --- a/ietf/group/models.py +++ b/ietf/group/models.py @@ -82,7 +82,7 @@ class Group(GroupInfo): def is_decendant_of(self, sought_parent): p = self.parent - while (p != None): + while ((p != None) and (p != self)): if p.acronym == sought_parent: return True p = p.parent diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index 8f87e18a9..a6dec522d 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -1125,5 +1125,44 @@ class StatusUpdateTests(TestCase): self.assertEqual(response.status_code, 302) self.assertEqual(chair.group.latest_event(type='status_update').desc,'This came from a file.') +class GroupParentLoopTests(TestCase): - + def test_group_parent_loop(self): + make_test_data() + mars = Group.objects.get(acronym="mars") + test1 = Group.objects.create( + type_id="team", + acronym="testteam1", + name="Test One", + description="The test team 1 is testing.", + state_id="active", + parent = mars, + ) + test2 = Group.objects.create( + type_id="team", + acronym="testteam2", + name="Test Two", + description="The test team 2 is testing.", + state_id="active", + parent = test1, + ) + # Change the parent of Mars to make a loop + mars.parent = test2 + + # In face of the loop in the parent links, the code should not loop forever + import signal + + def timeout_handler(signum, frame): + raise Exception("Infinite loop in parent links is not handeled properly.") + + signal.signal(signal.SIGALRM, timeout_handler) + signal.alarm(1) # One second + try: + test2.is_decendant_of("ietf") + except Exception: + raise + finally: + signal.alarm(0) + + # If we get here, then there is not an infinite loop + return \ No newline at end of file diff --git a/ietf/group/utils.py b/ietf/group/utils.py index 6d41fb95b..55b4faa0f 100644 --- a/ietf/group/utils.py +++ b/ietf/group/utils.py @@ -91,13 +91,16 @@ def can_manage_group_type(user, group_type): return has_role(user, 'Secretariat') -def can_manage_team_group(user, group): - if group.type_id != "team": - return False - elif group.is_decendant_of("ietf") and has_role(user, ('Area Director', 'Secretariat')): - return True - elif group.is_decendant_of("irtf") and has_role(user, ('IRTF Chair', 'Secretariat')): - return True +def can_manage_group(user, group): + if group.type_id == "rg": + return has_role(user, ('IRTF Chair', 'Secretariat')) + elif group.type_id == "wg": + return has_role(user, ('Area Director', 'Secretariat')) + elif group.type_id == "team": + if group.is_decendant_of("ietf"): + return has_role(user, ('Area Director', 'Secretariat')) + elif group.is_decendant_of("irtf"): + return has_role(user, ('IRTF Chair', 'Secretariat')) return has_role(user, ('Secretariat')) def milestone_reviewer_for_group_type(group_type):