Improve permission checking on detail views

todo/templates/todo/task_detail.html

@@ -1,6 +1,8 @@
 {% extends "todo/base.html" %}
 
-{% block title %}Task: {{ task.title }}{% endblock %}
+{% block title %}Task:
+  {{ task.title }}
+{% endblock %}
 
 {% block content %}
 
@@ -16,14 +18,14 @@
     });
   </script>
 
-    {% if auth_ok %}
-
   <h2>{{ task }}</h2>
 
   <form action="" method="POST">
     {% csrf_token %}
 
-            <p id="slideToggle" ><strong>&rarr; Click to edit details &larr;</strong></p>
+    <p id="slideToggle">
+      <strong>&rarr; Click to edit details &larr;</strong>
+    </p>
 
     <p>
       <strong>In list:</strong>
@@ -32,10 +34,12 @@
       </a><br/>
 
       <strong>Assigned to:</strong>
-                  {% if task.assigned_to %}{{ task.assigned_to.get_full_name }}{% else %}Anyone{% endif %}<br />
+      {% if task.assigned_to %}{{ task.assigned_to.get_full_name }}
+      {% else %}Anyone{% endif %}<br/>
 
       <strong>Created by:</strong>
-                  {{ task.created_by.first_name }} {{ task.created_by.last_name }}<br />
+      {{ task.created_by.first_name }}
+      {{ task.created_by.last_name }}<br/>
 
       <strong>Due date:</strong>
       {{ task.due_date }}<br/>
@@ -45,7 +49,9 @@
     </p>
 
     {% if task.note %}
-                <div class="task_note"><strong>Note:</strong> {{ task.note|safe|urlize|linebreaks }}</div>
+      <div class="task_note">
+        <strong>Note:</strong>
+        {{ task.note|safe|urlize|linebreaks }}</div>
     {% endif %}
 
     <div id="TaskEdit">
@@ -54,32 +60,38 @@
       <table>
         <tr>
           <td>Title:</td>
-                        <td>{{ form.title }}  </td>
+          <td>{{ form.title }}
+          </td>
         </tr>
 
         <tr>
           <td>List:</td>
-                        <td>{{ form.task_list }}  </td>
+          <td>{{ form.task_list }}
+          </td>
         </tr>
 
         <tr>
           <td>Due:</td>
-                        <td>{{ form.due_date }}  </td>
+          <td>{{ form.due_date }}
+          </td>
         </tr>
 
         <tr>
           <td>Assigned to:</td>
-                        <td>{{ form.assigned_to }}  </td>
+          <td>{{ form.assigned_to }}
+          </td>
         </tr>
 
         <tr>
           <td valign="top">Note:</td>
-                        <td>{{ form.note }}  </td>
+          <td>{{ form.note }}
+          </td>
         </tr>
 
         <tr>
           <td>Priority:</td>
-                        <td>{{ form.priority }}  </td>
+          <td>{{ form.priority }}
+          </td>
         </tr>
 
       </table>
@@ -99,7 +111,8 @@
   <div class="task_comments">
     {% for comment in comment_list %}
       <p>
-                    <strong>{{ comment.author.first_name }} {{ comment.author.last_name }},
+        <strong>{{ comment.author.first_name }}
+          {{ comment.author.last_name }},
           {{ comment.date|date:"F d Y P" }}
         </strong>
       </p>
@@ -109,6 +122,4 @@
     {% endfor %}
   </div>
 
-    {% endif %}
-
 {% endblock %}

todo/views.py

@@ -5,6 +5,7 @@ from django.contrib.auth.decorators import user_passes_test, login_required
 from django.contrib.auth.models import User
 from django.contrib.sites.models import Site
 from django.core.mail import send_mail
+from django.core.exceptions import PermissionDenied
 from django.db import IntegrityError
 from django.db.models import Q
 from django.http import HttpResponse
@@ -21,8 +22,10 @@ from todo.utils import toggle_done, toggle_deleted, send_notify_mail
 
 def check_user_allowed(user):
     """
-    Conditions for user_passes_test decorator.
+    Verifies user is logged in, and in staff if that setting is enabled.
+    Per-object permission checks (e.g. to view a particular list) must be in the views that handle those objects.
     """
+
     if settings.STAFF_ONLY:
         return user.is_authenticated and user.is_staff
     else:
@@ -64,6 +67,11 @@ def del_list(request, list_id, list_slug):
     """
     task_list = get_object_or_404(TaskList, slug=list_slug)
 
+    # Ensure user has permission to delete list. Admins can delete all lists.
+    # Get the group this list belongs to, and check whether current user is a member of that group.
+    if task_list.group not in request.user.groups.all() or not request.user.is_staff:
+        raise PermissionDenied
+
     if request.method == 'POST':
         TaskList.objects.get(id=task_list.id).delete()
         messages.success(request, "{list_name} is gone.".format(list_name=task_list.name))
@@ -80,6 +88,13 @@ def list_detail(request, list_id=None, list_slug=None, view_completed=False):
     """Display and manage items in a todo list.
     """
 
+    task_list = get_object_or_404(TaskList, id=list_id, slug=list_slug)
+
+    # Ensure user has permission to view list. Admins can view all lists.
+    # Get the group this task_list belongs to, and check whether current user is a member of that group.
+    if task_list.group not in request.user.groups.all() and not request.user.is_staff:
+        raise PermissionDenied
+
     if request.POST:
         # Process completed and deleted requests on each POST
         toggle_done(request, request.POST.getlist('toggle_done_tasks'))
@@ -134,11 +149,10 @@ def task_detail(request, task_id):
     task = get_object_or_404(Item, pk=task_id)
     comment_list = Comment.objects.filter(task=task_id)
 
-    # Ensure user has permission to view item. Admins can edit all tasks.
+    # Ensure user has permission to view item. Admins can view all tasks.
     # Get the group this task belongs to, and check whether current user is a member of that group.
-
-    if task.task_list.group in request.user.groups.all() or request.user.is_staff:
-        auth_ok = True
+    if task.task_list.group not in request.user.groups.all() and not request.user.is_staff:
+        raise PermissionDenied
 
     if request.POST:
         form = EditItemForm(request.POST, instance=task)