Clean up permission checking in community lists, fix split in

track/untrack between personal/group lists, get rid of remove_document
 - Legacy-Id: 10680

Author: OleLaursen

ietf/community/models.py

@@ -8,7 +8,7 @@
 
 from ietf.utils.mail import send_mail
 from ietf.doc.models import Document, DocEvent
-from ietf.group.models import Group, Role
+from ietf.group.models import Group
 
 from ietf.community.rules import TYPES_OF_RULES, RuleManager
 from ietf.community.display import (TYPES_OF_SORT, DisplayField,
@@ -24,21 +24,6 @@ class CommunityList(models.Model):
     secret = models.CharField(max_length=255, null=True, blank=True)
     cached = models.TextField(null=True, blank=True)
 
-    def check_manager(self, user):
-        if user == self.user:
-            return True
-        if not self.group or self.group.type.slug not in ('area', 'wg'):
-            return False
-        try:
-            person = user.person
-        except:
-            return False
-        if self.group.type.slug == 'area':
-            return bool(Role.objects.filter(name__slug='ad', email__in=person.email_set.all(), group=self.group).count())
-        elif self.group.type.slug == 'wg':
-            return bool(Role.objects.filter(name__slug='chair', email__in=person.email_set.all(), group=self.group).count())
-        return False
-
     def long_name(self):
         if self.user:
             return 'Personal ID list of %s' % self.user.username

ietf/community/tests.py

@@ -7,10 +7,10 @@
 from ietf.utils.test_utils import login_testing_unauthorized, TestCase
 
 class CommunityListTests(TestCase):
-    def test_track_untrack_document(self):
+    def test_track_untrack_document_for_personal_list_through_ajax(self):
         draft = make_test_data()
 
-        url = urlreverse("community_track_document", kwargs={ "name": draft.name })
+        url = urlreverse("community_personal_track_document", kwargs={ "name": draft.name })
         login_testing_unauthorized(self, "plain", url)
 
         # track
@@ -21,10 +21,35 @@ def test_track_untrack_document(self):
         self.assertEqual(list(clist.added_ids.all()), [draft])
 
         # untrack
-        url = urlreverse("community_untrack_document", kwargs={ "name": draft.name })
+        url = urlreverse("community_personal_untrack_document", kwargs={ "name": draft.name })
         r = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest')
         self.assertEqual(r.status_code, 200)
         self.assertEqual(json.loads(r.content)["success"], True)
         clist = CommunityList.objects.get(user__username="plain")
         self.assertEqual(list(clist.added_ids.all()), [])
+
+    def test_track_untrack_document_for_group_list(self):
+        draft = make_test_data()
+
+        url = urlreverse("community_group_track_document", kwargs={ "name": draft.name, "acronym": draft.group.acronym })
+        login_testing_unauthorized(self, "marschairman", url)
+
+        # track
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+
+        r = self.client.post(url)
+        self.assertEqual(r.status_code, 302)
+        clist = CommunityList.objects.get(group__acronym=draft.group.acronym)
+        self.assertEqual(list(clist.added_ids.all()), [draft])
+
+        # untrack
+        url = urlreverse("community_group_untrack_document", kwargs={ "name": draft.name, "acronym": draft.group.acronym })
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 200)
+
+        r = self.client.post(url)
+        self.assertEqual(r.status_code, 302)
+        clist = CommunityList.objects.get(group__acronym=draft.group.acronym)
+        self.assertEqual(list(clist.added_ids.all()), [])
 

ietf/community/urls.py

@@ -12,6 +12,8 @@
     url(r'^personal/(?P<secret>[a-f0-9]+)/subscribe/significant/$', 'subscribe_personal_list', {'significant': True}, name='subscribe_significant_personal_list'),
     url(r'^personal/(?P<secret>[a-f0-9]+)/unsubscribe/$', 'unsubscribe_personal_list', {'significant': False}, name='unsubscribe_personal_list'),
     url(r'^personal/(?P<secret>[a-f0-9]+)/unsubscribe/significant/$', 'unsubscribe_personal_list', {'significant': True}, name='unsubscribe_significant_personal_list'),
+    url(r'^personal/trackdocument/(?P<name>[^/]+)/$', 'track_document', name='community_personal_track_document'),
+    url(r'^personal/untrackdocument/(?P<name>[^/]+)/$', 'untrack_document', name='community_personal_untrack_document'),
 
     url(r'^group/(?P<acronym>[\w.@+-]+)/$', 'manage_group_list', name='manage_group_list'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/view/$', 'view_group_list', name='view_group_list'),
@@ -22,11 +24,10 @@
     url(r'^group/(?P<acronym>[\w.@+-]+)/subscribe/significant/$', 'subscribe_group_list', {'significant': True}, name='subscribe_significant_group_list'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/unsubscribe/$', 'unsubscribe_group_list', {'significant': False}, name='unsubscribe_group_list'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/unsubscribe/significant/$', 'unsubscribe_group_list', {'significant': True}, name='unsubscribe_significant_group_list'),
+    url(r'^group/(?P<acronym>[\w.@+-]+)/trackdocument/(?P<name>[^/]+)/$', 'track_document', name='community_group_track_document'),
+    url(r'^group/(?P<acronym>[\w.@+-]+)/untrackdocument/(?P<name>[^/]+)/$', 'untrack_document', name='community_group_untrack_document'),
 
-    url(r'^trackdocument/(?P<name>[^/]+)/$', 'track_document', name='community_track_document'),
-    url(r'^untrackdocument/(?P<name>[^/]+)/$', 'untrack_document', name='community_untrack_document'),
 
-    url(r'^(?P<list_id>[\d]+)/remove_document/(?P<name>[^/]+)/$', 'remove_document', name='community_remove_document'),
     url(r'^(?P<list_id>[\d]+)/remove_rule/(?P<rule_id>[^/]+)/$', 'remove_rule', name='community_remove_rule'),
     url(r'^(?P<list_id>[\d]+)/subscribe/confirm/(?P<email>[\w.@+-]+)/(?P<date>[\d]+)/(?P<confirm_hash>[a-f0-9]+)/$', 'confirm_subscription', name='confirm_subscription'),
     url(r'^(?P<list_id>[\d]+)/subscribe/significant/confirm/(?P<email>[\w.@+-]+)/(?P<date>[\d]+)/(?P<confirm_hash>[a-f0-9]+)/$', 'confirm_significant_subscription', name='confirm_significant_subscription'),

ietf/community/utils.py

@@ -0,0 +1,13 @@
+from ietf.group.models import Role
+
+def can_manage_community_list_for_group(user, group):
+    if not user or not user.is_authenticated() or not group:
+        return False
+
+    if group.type_id == 'area':
+        return Role.objects.filter(name__slug='ad', person__user=user, group=group).exists()
+    elif group.type_id in ('wg', 'rg'):
+        return Role.objects.filter(name__slug='chair', person__user=user, group=group).exists()
+    else:
+        return False
+

ietf/community/views.py

@@ -6,14 +6,13 @@
 
 from django.db import IntegrityError
 from django.conf import settings
-from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.http import HttpResponse, HttpResponseForbidden, Http404, HttpResponseRedirect
 from django.shortcuts import get_object_or_404, render, redirect
-from django.utils.http import urlquote
 from django.contrib.auth.decorators import login_required
 
 from ietf.community.models import CommunityList, Rule, EmailSubscription
 from ietf.community.forms import RuleForm, DisplayForm, SubscribeForm, UnSubscribeForm
+from ietf.community.utils import can_manage_community_list_for_group
 from ietf.group.models import Group
 from ietf.doc.models import DocEvent, Document
 
@@ -48,38 +47,37 @@ def _manage_list(request, clist):
                                'rule_form': rule_form})
 
 
+@login_required
 def manage_personal_list(request):
-    user = request.user
-    if not user.is_authenticated():
-        path = urlquote(request.get_full_path())
-        tup = settings.LOGIN_URL, REDIRECT_FIELD_NAME, path
-        return HttpResponseRedirect('%s?%s=%s' % tup)
     clist = CommunityList.objects.get_or_create(user=request.user)[0]
-    if not clist.check_manager(request.user):
-        path = urlquote(request.get_full_path())
-        tup = settings.LOGIN_URL, REDIRECT_FIELD_NAME, path
-        return HttpResponseRedirect('%s?%s=%s' % tup)
     return _manage_list(request, clist)
 
 
+@login_required
 def manage_group_list(request, acronym):
     group = get_object_or_404(Group, acronym=acronym)
-    if group.type.slug not in ('area', 'wg'):
-        raise Http404
+    if not can_manage_community_list_for_group(request.user, group):
+        return HttpResponseForbidden("You do not have permission to access this view")
+
     clist = CommunityList.objects.get_or_create(group=group)[0]
-    if not clist.check_manager(request.user):
-        path = urlquote(request.get_full_path())
-        tup = settings.LOGIN_URL, REDIRECT_FIELD_NAME, path
-        return HttpResponseRedirect('%s?%s=%s' % tup)
     return _manage_list(request, clist)
 
 @login_required
-def track_document(request, name):
+def track_document(request, name, acronym=None):
     doc = get_object_or_404(Document, docalias__name=name)
 
     if request.method == "POST":
-        clist = CommunityList.objects.get_or_create(user=request.user)[0]
+        if acronym:
+            group = get_object_or_404(Group, acronym=acronym)
+            if not can_manage_community_list_for_group(request.user, group):
+                return HttpResponseForbidden("You do not have permission to access this view")
+
+            clist = CommunityList.objects.get_or_create(group=group)[0]
+        else:
+            clist = CommunityList.objects.get_or_create(user=request.user)[0]
+
         clist.added_ids.add(doc)
+
         if request.is_ajax():
             return HttpResponse(json.dumps({ 'success': True }), content_type='text/plain')
         else:
@@ -90,9 +88,15 @@ def track_document(request, name):
     })
 
 @login_required
-def untrack_document(request, name):
+def untrack_document(request, name, acronym=None):
     doc = get_object_or_404(Document, docalias__name=name)
-    clist = get_object_or_404(CommunityList, user=request.user)
+    if acronym:
+        group = get_object_or_404(Group, acronym=acronym)
+        if not can_manage_community_list_for_group(request.user, group):
+            return HttpResponseForbidden("You do not have permission to access this view")
+        clist = get_object_or_404(CommunityList, group=group)
+    else:
+        clist = get_object_or_404(CommunityList, user=request.user)
 
     if request.method == "POST":
         clist.added_ids.remove(doc)
@@ -106,23 +110,13 @@ def untrack_document(request, name):
     })
 
 @login_required
-def remove_document(request, list_id, name):
+def remove_rule(request, list_id, rule_id):
     clist = get_object_or_404(CommunityList, pk=list_id)
-    if not clist.check_manager(request.user):
-        return HttpResponseForbidden("You do not have permission to access this view")
-
-    doc = get_object_or_404(Document, docalias__name=name)
-    clist.added_ids.remove(doc)
-
-    return HttpResponseRedirect(clist.get_manage_url())
 
+    if ((clist.user and clist.user != request.user)
+        or (clist.group and not can_manage_community_list_for_group(request.user, clist.group))):
+        return HttpResponseForbidden("You do not have permission to access this view")
 
-def remove_rule(request, list_id, rule_id):
-    clist = get_object_or_404(CommunityList, pk=list_id)
-    if not clist.check_manager(request.user):
-        path = urlquote(request.get_full_path())
-        tup = settings.LOGIN_URL, REDIRECT_FIELD_NAME, path
-        return HttpResponseRedirect('%s?%s=%s' % tup)
     rule = get_object_or_404(Rule, pk=rule_id)
     rule.delete()
     return HttpResponseRedirect(clist.get_manage_url())
@@ -218,30 +212,19 @@ def _csv_list(request, clist):
         writer.writerow(row)
     return response
 
-
+@login_required
 def csv_personal_list(request):
-    user = request.user
-    if not user.is_authenticated():
-        path = urlquote(request.get_full_path())
-        tup = settings.LOGIN_URL, REDIRECT_FIELD_NAME, path
-        return HttpResponseRedirect('%s?%s=%s' % tup)
-    clist = CommunityList.objects.get_or_create(user=user)[0]
-    if not clist.check_manager(user):
-        path = urlquote(request.get_full_path())
-        tup = settings.LOGIN_URL, REDIRECT_FIELD_NAME, path
-        return HttpResponseRedirect('%s?%s=%s' % tup)
+    clist = CommunityList.objects.get_or_create(user=request.user)[0]
     return _csv_list(request, clist)
 
 
+@login_required
 def csv_group_list(request, acronym):
     group = get_object_or_404(Group, acronym=acronym)
-    if group.type.slug not in ('area', 'wg'):
-        raise Http404
+    if not can_manage_community_list_for_group(request.user, group):
+        return HttpResponseForbidden("You do not have permission to access this view")
+
     clist = CommunityList.objects.get_or_create(group=group)[0]
-    if not clist.check_manager(request.user):
-        path = urlquote(request.get_full_path())
-        tup = settings.LOGIN_URL, REDIRECT_FIELD_NAME, path
-        return HttpResponseRedirect('%s?%s=%s' % tup)
     return _csv_list(request, clist)
 
 def view_csv_personal_list(request, secret):

ietf/templates/community/manage_clist.html

@@ -47,8 +47,8 @@ <h1>{{ cl.long_name }}</h1>
 	    <tr>
 	      <td>{{ doc.display_name }}</td>
 	      <td>{{ doc.get_state }}</td>
-	      <td><a href="{{ doc.get_absolute_url }}">{{ doc.title }}</a></td>
-	      <td><a class="btn btn-danger btn-xs" href="{% url "community_remove_document" cl.pk doc.pk %}">Remove</a></td>
+              <td><a href="{{ doc.get_absolute_url }}">{{ doc.title }}</a></td>
+              <td><a class="btn btn-danger btn-xs" href="{% if cl.user %}{% url "community_personal_untrack_document" doc.pk %}{% else %}{% url "community_group_untrack_document" %}{% endif %}">Remove</a></td>
 	    </tr>
 	  {% endfor %}
         </tbody>

ietf/templates/doc/document_draft.html

@@ -435,9 +435,9 @@
     </div>
     {% if user.is_authenticated %}
       {% if tracking_document %}
-        <a class="btn btn-default btn-xs community-list-add-remove-doc" href="{% url "community_untrack_document" doc.name %}" title="Remove from your personal ID list"><span class="fa fa-bookmark-o"></span> Untrack</a>
+        <a class="btn btn-default btn-xs community-list-add-remove-doc" href="{% url "community_personal_untrack_document" doc.name %}" title="Remove from your personal ID list"><span class="fa fa-bookmark-o"></span> Untrack</a>
       {% else %}
-        <a class="btn btn-default btn-xs community-list-add-remove-doc" href="{% url "community_track_document" doc.name %}" title="Add to your personal ID list"><span class="fa fa-bookmark"></span> Track</a>
+        <a class="btn btn-default btn-xs community-list-add-remove-doc" href="{% url "community_personal_track_document" doc.name %}" title="Add to your personal ID list"><span class="fa fa-bookmark"></span> Track</a>
       {% endif %}
     {% endif %}
 

ietf/templates/doc/search/search_result_row.html

@@ -14,11 +14,11 @@
   <td>
     {% if user.is_authenticated %}
       {% if doc.name in doc_is_tracked %}
-        <a href="{% url "community_untrack_document" doc.name %}" class="community-list-add-remove-doc" title="Remove from your personal ID list">
+        <a href="{% url "community_personal_untrack_document" doc.name %}" class="community-list-add-remove-doc" title="Remove from your personal ID list">
           <span class="fa fa-bookmark"></span>
         </a>
       {% else %}
-        <a href="{% url "community_track_document" doc.name %}" class="community-list-add-remove-doc" title="Add to your personal ID list">
+        <a href="{% url "community_personal_track_document" doc.name %}" class="community-list-add-remove-doc" title="Add to your personal ID list">
           <span class="fa fa-bookmark-o"></span>
         </a>
       {% endif %}