Enforce and test TODO_STAFF_ONLY setting

README.md

@@ -30,7 +30,7 @@ The assumption is that your organization/publication/company has multiple groups
 
 You must have at least one Group set up in Django admin, and that group must have at least one User as a member. This is true even if you're the sole user of django-todo.
 
-Users can view and modify all to-do lists belonging to their group(s). Only users with `is_staff()` can add or delete lists.
+Users can view and modify all to-do lists belonging to their group(s). Only users with `is_staff` can add or delete lists.
 
 Identical list names can exist in different groups, but not in the same group.
 
@@ -108,8 +108,9 @@ If you wish to use the public ticket-filing system, first create the list into w
 Optional configuration options:
 
 ```
-# Restrict access to todo lists/views to `is_staff()` users.
-# False here falls back to `is_authenticated()` users.
+# Restrict access to ALL todo lists/views to `is_staff` users.
+# If False or unset, all users can see all views (but more granular permissions are still enforced
+# within views, such as requiring staff for adding and deleting lists).
 TODO_STAFF_ONLY = True
 
 # If you use the "public" ticket filing option, to whom should these tickets be assigned?
@@ -167,6 +168,8 @@ The previous `tox` system was removed with the v2 release, since we no longer ai
 
 # Version History
 
+**2.2.0** Re-instate enforcement of TODO_STAFF_ONLY setting
+
 **2.1.1** Correct Python version requirement in documentation to Python 3.6
 
 **2.1.1** Split up views into separate modules.

todo/__init__.py

@@ -1,7 +1,7 @@
 """
 A multi-user, multi-group task management and assignment system for Django.
 """
-__version__ = '2.1.2'
+__version__ = '2.2.0'
 
 __author__ = 'Scot Hacker'
 __email__ = 'shacker@birdhouse.org'

todo/tests/test_views.py

@@ -144,18 +144,11 @@ def test_no_javascript_in_comments(todo_setup, client):
 
 # ### PERMISSIONS ###
 
-"""
-Some views are for staff users only.
-We've already smoke-tested with Admin user - try these with normal user.
-These exercise our custom @staff_only decorator without calling that function explicitly.
-"""
-
-
 def test_view_add_list_nonadmin(todo_setup, client):
     url = reverse("todo:add_list")
     client.login(username="you", password="password")
     response = client.get(url)
-    assert response.status_code == 403
+    assert response.status_code == 302  # Redirected to login
 
 
 def test_view_del_list_nonadmin(todo_setup, client):
@@ -163,7 +156,7 @@ def test_view_del_list_nonadmin(todo_setup, client):
     url = reverse("todo:del_list", kwargs={"list_id": tlist.id, "list_slug": tlist.slug})
     client.login(username="you", password="password")
     response = client.get(url)
-    assert response.status_code == 403
+    assert response.status_code == 302  # Fedirected to login
 
 
 def test_view_list_mine(todo_setup, client):
@@ -220,3 +213,22 @@ def test_view_task_not_in_my_group(todo_setup, client):
     response = client.get(url)
     assert response.status_code == 403
 
+
+def test_setting_TODO_STAFF_ONLY_False(todo_setup, client, settings):
+    # We use Django's user_passes_test to call `staff_check` utility function on all views.
+    # Just testing one view here; if it works, it works for all of them.
+    settings.TODO_STAFF_ONLY = False
+    url = reverse("todo:lists")
+    client.login(username="u2", password="password")
+    response = client.get(url)
+    assert response.status_code == 200
+
+
+def test_setting_TODO_STAFF_ONLY_True(todo_setup, client, settings):
+    # We use Django's user_passes_test to call `staff_check` utility function on all views.
+    # Just testing one view here; if it works, it works for all of them.
+    settings.TODO_STAFF_ONLY = True
+    url = reverse("todo:lists")
+    client.login(username="u2", password="password")
+    response = client.get(url)
+    assert response.status_code == 302  # Redirected to login view

todo/utils.py

@@ -1,26 +1,22 @@
+from django.conf import settings
 from django.contrib.sites.models import Site
-from django.core.exceptions import PermissionDenied
 from django.core.mail import send_mail
 from django.template.loader import render_to_string
 
 from todo.models import Comment, Task
 
 
-def staff_only(function):
-    """
-    Custom view decorator allows us to raise 403 on insufficient permissions,
-    rather than redirect user to login view.
+def staff_check(user):
+    """If TODO_STAFF_ONLY is set to True, limit view access to staff users only.
+        # FIXME: More granular access control is needed... but need to do it generically,
+        # to satisfy all possible todo implementations.
     """
 
-    def wrap(request, *args, **kwargs):
-        if request.user.is_staff:
-            return function(request, *args, **kwargs)
-        else:
-            raise PermissionDenied
-
-    wrap.__doc__ = function.__doc__
-    wrap.__name__ = function.__name__
-    return wrap
+    if hasattr(settings, "TODO_STAFF_ONLY") and settings.TODO_STAFF_ONLY:
+        return user.is_staff
+    else:
+        # If unset or False, allow all logged in users
+        return True
 
 
 def send_notify_mail(new_task):

todo/views/add_list.py

@@ -1,20 +1,25 @@
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
+from django.core.exceptions import PermissionDenied
 from django.db import IntegrityError
 from django.http import HttpResponse
 from django.shortcuts import redirect, render
 from django.utils.text import slugify
 
 from todo.forms import AddTaskListForm
-from todo.utils import staff_only
+from todo.utils import staff_check
 
 
-@staff_only
 @login_required
+@user_passes_test(staff_check)
 def add_list(request) -> HttpResponse:
     """Allow users to add a new todo list to the group they're in.
     """
 
+    # Only staffers can add lists.
+    if not request.user.is_staff:
+        raise PermissionDenied
+
     if request.POST:
         form = AddTaskListForm(request.user, request.POST)
         if form.is_valid():
@@ -33,6 +38,7 @@ def add_list(request) -> HttpResponse:
                 )
     else:
         if request.user.groups.all().count() == 1:
+            # FIXME: Assuming first of user's groups here; better to prompt for group
             form = AddTaskListForm(request.user, initial={"group": request.user.groups.all()[0]})
         else:
             form = AddTaskListForm(request.user)

todo/views/del_list.py

@@ -1,25 +1,22 @@
-import datetime
-
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.http import HttpResponse
 from django.shortcuts import render, redirect, get_object_or_404
 from django.core.exceptions import PermissionDenied
 
 from todo.models import Task, TaskList
-from todo.utils import staff_only
+from todo.utils import staff_check
 
 
-@staff_only
 @login_required
+@user_passes_test(staff_check)
 def del_list(request, list_id: int, list_slug: str) -> HttpResponse:
     """Delete an entire list. Only staff members should be allowed to access this view.
     """
     task_list = get_object_or_404(TaskList, id=list_id)
 
-    # 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.
-    # FIXME: This means any group member can delete lists, which is probably too permissive.
+    # Ensure user has permission to delete list. Get the group this list belongs to,
+    # and check whether current user is a member of that group AND a staffer.
     if task_list.group not in request.user.groups.all() and not request.user.is_staff:
         raise PermissionDenied
 

todo/views/delete_task.py

@@ -1,13 +1,16 @@
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404, redirect
 from django.urls import reverse
 
 from todo.models import Task
+from todo.utils import staff_check
+
 
 @login_required
+@user_passes_test(staff_check)
 def delete_task(request, task_id: int) -> HttpResponse:
     """Delete specified task.
     Redirect to the list from which the task came.

todo/views/external_add.py

@@ -1,6 +1,6 @@
 from django.conf import settings
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.contrib.auth.models import User
 from django.contrib.sites.models import Site
 from django.core.mail import send_mail
@@ -10,9 +10,11 @@ from django.template.loader import render_to_string
 
 from todo.forms import AddExternalTaskForm
 from todo.models import TaskList
+from todo.utils import staff_check
 
 
 @login_required
+@user_passes_test(staff_check)
 def external_add(request) -> HttpResponse:
     """Allow authenticated users who don't have access to the rest of the ticket system to file a ticket
     in the list specified in settings (e.g. django-todo can be used a ticket filing system for a school, where

todo/views/list_detail.py

@@ -1,6 +1,6 @@
 import bleach
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404, redirect, render
@@ -8,10 +8,11 @@ from django.utils import timezone
 
 from todo.forms import AddEditTaskForm
 from todo.models import Task, TaskList
-from todo.utils import send_notify_mail
+from todo.utils import send_notify_mail, staff_check
 
 
 @login_required
+@user_passes_test(staff_check)
 def list_detail(request, list_id=None, list_slug=None, view_completed=False) -> HttpResponse:
     """Display and manage tasks in a todo list.
     """

todo/views/list_lists.py

@@ -1,15 +1,17 @@
 import datetime
 
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.http import HttpResponse
 from django.shortcuts import render
 
 from todo.forms import SearchForm
 from todo.models import Task, TaskList
+from todo.utils import staff_check
 
 
 @login_required
+@user_passes_test(staff_check)
 def list_lists(request) -> HttpResponse:
     """Homepage view - list of lists a user can view, and ability to add a list.
     """

todo/views/reorder_tasks.py

@@ -1,12 +1,14 @@
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.http import HttpResponse
+from django.views.decorators.csrf import csrf_exempt
 
 from todo.models import Task
-from django.views.decorators.csrf import csrf_exempt
+from todo.utils import staff_check
 
 
 @csrf_exempt
 @login_required
+@user_passes_test(staff_check)
 def reorder_tasks(request) -> HttpResponse:
     """Handle task re-ordering (priorities) from JQuery drag/drop in list_detail.html
     """

todo/views/search.py

@@ -1,18 +1,22 @@
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.db.models import Q
 from django.http import HttpResponse
 from django.shortcuts import render
 
 from todo.models import Task
+from todo.utils import staff_check
 
 
 @login_required
+@user_passes_test(staff_check)
 def search(request) -> HttpResponse:
     """Search for tasks user has permission to see.
     """
+
+    query_string = ""
+
     if request.GET:
 
-        query_string = ""
         found_tasks = None
         if ("q" in request.GET) and request.GET["q"].strip():
             query_string = request.GET["q"]
@@ -29,7 +33,6 @@ def search(request) -> HttpResponse:
             found_tasks = found_tasks.exclude(completed=True)
 
     else:
-        query_string = None
         found_tasks = None
 
     # Only include tasks that are in groups of which this user is a member:

todo/views/task_detail.py

@@ -2,17 +2,18 @@ import datetime
 
 import bleach
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404, redirect, render
 
 from todo.forms import AddEditTaskForm
 from todo.models import Comment, Task
-from todo.utils import send_email_to_thread_participants, toggle_task_completed
+from todo.utils import send_email_to_thread_participants, toggle_task_completed, staff_check
 
 
 @login_required
+@user_passes_test(staff_check)
 def task_detail(request, task_id: int) -> HttpResponse:
     """View task details. Allow task details to be edited. Process new comments on task.
     """

todo/views/toggle_done.py

@@ -1,5 +1,5 @@
 from django.contrib import messages
-from django.contrib.auth.decorators import login_required
+from django.contrib.auth.decorators import login_required, user_passes_test
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404, redirect
@@ -7,9 +7,11 @@ from django.urls import reverse
 
 from todo.models import Task
 from todo.utils import toggle_task_completed
+from todo.utils import staff_check
 
 
 @login_required
+@user_passes_test(staff_check)
 def toggle_done(request, task_id: int) -> HttpResponse:
     """Toggle the completed status of a task from done to undone, or vice versa.
     Redirect to the list from which the task came.