From 91b9a099a30274ce8bae6d705fcd0a5150159739 Mon Sep 17 00:00:00 2001 From: Scot Hacker Date: Thu, 10 Jan 2019 00:39:21 -0800 Subject: [PATCH] Enforce and test TODO_STAFF_ONLY setting --- README.md | 9 ++++++--- todo/__init__.py | 2 +- todo/tests/test_views.py | 30 +++++++++++++++++++++--------- todo/utils.py | 24 ++++++++++-------------- todo/views/add_list.py | 12 +++++++++--- todo/views/del_list.py | 13 +++++-------- todo/views/delete_task.py | 5 ++++- todo/views/external_add.py | 4 +++- todo/views/list_detail.py | 5 +++-- todo/views/list_lists.py | 4 +++- todo/views/reorder_tasks.py | 6 ++++-- todo/views/search.py | 9 ++++++--- todo/views/task_detail.py | 5 +++-- todo/views/toggle_done.py | 4 +++- 14 files changed, 81 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 0101a92..65ede23 100644 --- a/README.md +++ b/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. diff --git a/todo/__init__.py b/todo/__init__.py index d682689..33eaf46 100644 --- a/todo/__init__.py +++ b/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' diff --git a/todo/tests/test_views.py b/todo/tests/test_views.py index dbfa1d0..72f2052 100644 --- a/todo/tests/test_views.py +++ b/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 diff --git a/todo/utils.py b/todo/utils.py index 6e85b06..dca0a11 100644 --- a/todo/utils.py +++ b/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): diff --git a/todo/views/add_list.py b/todo/views/add_list.py index e8a70ae..8cf9f03 100644 --- a/todo/views/add_list.py +++ b/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) diff --git a/todo/views/del_list.py b/todo/views/del_list.py index d2a9164..5ee1cd0 100644 --- a/todo/views/del_list.py +++ b/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 diff --git a/todo/views/delete_task.py b/todo/views/delete_task.py index a7c1334..9e3ec99 100644 --- a/todo/views/delete_task.py +++ b/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. diff --git a/todo/views/external_add.py b/todo/views/external_add.py index c8fdac1..2638996 100644 --- a/todo/views/external_add.py +++ b/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 diff --git a/todo/views/list_detail.py b/todo/views/list_detail.py index b9e2bba..32bb9b6 100644 --- a/todo/views/list_detail.py +++ b/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. """ diff --git a/todo/views/list_lists.py b/todo/views/list_lists.py index dda614f..7672eca 100644 --- a/todo/views/list_lists.py +++ b/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. """ diff --git a/todo/views/reorder_tasks.py b/todo/views/reorder_tasks.py index 2904f45..843a086 100644 --- a/todo/views/reorder_tasks.py +++ b/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 """ diff --git a/todo/views/search.py b/todo/views/search.py index d321cae..b7f5e17 100644 --- a/todo/views/search.py +++ b/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: diff --git a/todo/views/task_detail.py b/todo/views/task_detail.py index 4f81188..95432db 100644 --- a/todo/views/task_detail.py +++ b/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. """ diff --git a/todo/views/toggle_done.py b/todo/views/toggle_done.py index f80eaaf..6a3934e 100644 --- a/todo/views/toggle_done.py +++ b/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.